From 0a7c150c64a084654d7c6d5b1f12fb35182cab02 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 30 Jul 2026 11:36:05 -0700 Subject: [PATCH 1/3] fix(sidebar): stop the workspace switcher stranding a phantom hover MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The switcher's keyboard cursor is set from `onMouseMove` and only cleared when the menu closes, and it paints in `--surface-active` — the same token hover uses. So the last row the pointer crossed keeps a background indistinguishable from hover long after the pointer has gone, sitting alongside the equally-`--surface-active` current workspace as a second phantom-hovered row. It shows without any hovering too: the cursor is seeded to row 0 on open, so any user whose current workspace is not first sees two marked rows immediately. Only bites above the 3-workspace search threshold, which is why it went unnoticed. Paint the cursor only while the user is actually navigating by keyboard. Arrow keys enter that mode, any pointer motion leaves it, so in pointer mode the sole mark is real CSS :hover — which follows the pointer and leaves with it. `highlightedId` still tracks the pointer, so Enter keeps targeting the row last touched; only whether it is drawn changes. This is the pattern emcn's own popover already uses (`isKeyboardNav`, commented "prevent dual highlights") and that `tag-dropdown` consumes, and it matches how Headless UI models a combobox: one modality-driven focus marker, separate from the selected value. The list carries no `aria-activedescendant`/`aria-selected`, so the highlight is purely visual and nothing in the a11y contract changes. --- .../workspace-header.test.tsx | 189 ++++++++++++++++++ .../workspace-header/workspace-header.tsx | 27 ++- 2 files changed, 214 insertions(+), 2 deletions(-) create mode 100644 apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.test.tsx diff --git a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.test.tsx b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.test.tsx new file mode 100644 index 00000000000..04743f43d5e --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.test.tsx @@ -0,0 +1,189 @@ +/** + * @vitest-environment jsdom + */ +import { act } from 'react' +import { createRoot, type Root } from 'react-dom/client' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +const { mockNavigateToSettings } = vi.hoisted(() => ({ mockNavigateToSettings: vi.fn() })) + +vi.mock('@tanstack/react-query', () => ({ + useQueryClient: () => ({ invalidateQueries: vi.fn(), setQueryData: vi.fn() }), +})) +vi.mock('@/lib/auth/auth-client', () => ({ useActiveOrganization: () => ({ data: null }) })) +vi.mock('@/hooks/use-settings-navigation', () => ({ + useSettingsNavigation: () => ({ navigateToSettings: mockNavigateToSettings }), +})) +vi.mock('@/hooks/use-permission-config', () => ({ + usePermissionConfig: () => ({ isInvitationsDisabled: false }), +})) +vi.mock('@/app/workspace/[workspaceId]/providers/workspace-permissions-provider', () => ({ + useWorkspacePermissionsContext: () => ({ + userPermissions: { canAdmin: true, canEdit: true, canRead: true }, + }), +})) +vi.mock('@/hooks/queries/invitations', () => ({ invitationKeys: { all: ['invitations'] } })) +vi.mock('@/hooks/queries/workspace', () => ({ workspaceKeys: { all: ['workspaces'] } })) + +/** Modal/menu siblings are irrelevant to the highlight and drag in heavy trees. */ +vi.mock('@/app/workspace/[workspaceId]/components/invite-modal', () => ({ + InviteModal: () => null, +})) +vi.mock( + '@/app/workspace/[workspaceId]/w/components/sidebar/components/workflow-list/components/context-menu/context-menu', + () => ({ ContextMenu: () => null }) +) +vi.mock( + '@/app/workspace/[workspaceId]/w/components/sidebar/components/workflow-list/components/delete-modal/delete-modal', + () => ({ DeleteModal: () => null }) +) +vi.mock( + '@/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/components/create-workspace-modal/create-workspace-modal', + () => ({ CreateWorkspaceModal: () => null }) +) +vi.mock( + '@/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/components/pending-invitations/view-invitations-menu-item', + () => ({ ViewInvitationsMenuItem: () => null }) +) +vi.mock( + '@/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/components/pending-invitations/view-invitations-modal', + () => ({ ViewInvitationsModal: () => null }) +) + +import { WorkspaceHeader } from '@/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header' + +/** + * `@sim/emcn` is deliberately NOT mocked: the assertion is about the background class + * `chipVariants` produces, so a stubbed chip would only assert the stub. + */ +const ACTIVE_BG = 'bg-[var(--surface-active)]' + +/** + * Above `WORKSPACE_SEARCH_THRESHOLD` (3), so the searchable/keyboard list renders. + * The current workspace is deliberately NOT first: the highlight is seeded to row 0 on + * open, so a current workspace sitting at row 0 would mask the double-mark this guards. + */ +const WORKSPACES = [ + { id: 'ws-rvt', name: 'RVT' }, + { id: 'ws-emir', name: "Emir's Workspace" }, + { id: 'ws-acme', name: 'Acme' }, + { id: 'ws-globex', name: 'Globex' }, +] as unknown as Parameters[0]['workspaces'] + +let container: HTMLDivElement +let root: Root + +function render() { + ;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + act(() => { + root.render( + {}} + onWorkspaceSwitch={() => {}} + onCreateWorkspace={async () => {}} + onRenameWorkspace={async () => {}} + onDeleteWorkspace={async () => {}} + isDeletingWorkspace={false} + onUploadLogo={() => {}} + onLeaveWorkspace={async () => {}} + isLeavingWorkspace={false} + /> + ) + }) +} + +function row(name: string): HTMLElement { + const found = [...document.querySelectorAll('[data-workspace-row-idx]')].find((el) => + el.textContent?.includes(name) + ) + if (!found) throw new Error(`No workspace row rendered for "${name}"`) + return found as HTMLElement +} + +/** + * Whether a row is painted with the persistent active fill. + * + * Matches an exact class token, never a substring: the inactive chip carries + * `hover-hover:bg-[var(--surface-active)]`, which *contains* the active class, so a + * substring check reports every row as marked. + */ +function isMarked(name: string): boolean { + return [...row(name).querySelectorAll('*')].some((el) => + el.classList.contains(ACTIVE_BG) + ) +} + +beforeEach(() => { + vi.clearAllMocks() + // jsdom implements neither; the component scrolls the active row into view. + Element.prototype.scrollIntoView = vi.fn() +}) + +afterEach(() => { + act(() => root.unmount()) + container.remove() +}) + +describe('WorkspaceHeader workspace switcher highlight', () => { + it('marks only the current workspace when the menu opens', () => { + render() + + expect(isMarked("Emir's Workspace")).toBe(true) + // Regression: the keyboard cursor used to be seeded to row 0 on open, so a second + // row was marked in the same colour as hover before any interaction. + expect(isMarked('RVT')).toBe(false) + }) + + it('does not leave a highlight behind when the pointer moves across a row', () => { + render() + + act(() => { + row('RVT').dispatchEvent(new MouseEvent('mousemove', { bubbles: true })) + }) + + // The pointer has moved on; nothing should be painted as if still hovered. + // Real CSS :hover handles the row actually under the cursor and leaves with it. + expect(isMarked('RVT')).toBe(false) + expect(isMarked("Emir's Workspace")).toBe(true) + }) + + it('shows the cursor once the user navigates by keyboard', () => { + render() + + const search = document.querySelector('input[placeholder="Search workspaces..."]') + expect(search).not.toBeNull() + act(() => { + search?.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true })) + }) + + // ArrowDown from the seeded first row (RVT) lands on the second. + expect(isMarked("Emir's Workspace")).toBe(true) + }) + + it('drops the keyboard cursor again as soon as the pointer moves', () => { + render() + + const search = document.querySelector('input[placeholder="Search workspaces..."]') + act(() => { + search?.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true })) + }) + expect(isMarked("Emir's Workspace")).toBe(true) + + act(() => { + row('Acme').dispatchEvent(new MouseEvent('mousemove', { bubbles: true })) + }) + + // Back in pointer mode: only the current workspace keeps a persistent mark. + expect(isMarked('Acme')).toBe(false) + expect(isMarked('RVT')).toBe(false) + }) +}) diff --git a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.tsx b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.tsx index 8a14799949d..dad8b3f7ede 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.tsx @@ -169,6 +169,19 @@ function WorkspaceHeaderImpl({ const [workspaceSearch, setWorkspaceSearch] = useState('') const [highlightedId, setHighlightedId] = useState(null) + /** + * Which input the user is currently driving the list with. The highlight is only + * painted in keyboard mode, because it renders in `--surface-active` — the same + * token hover uses — so a highlight left behind by the pointer is indistinguishable + * from a stuck hover, and sits alongside the equally-`--surface-active` current + * workspace as a second phantom-hovered row. + * + * `highlightedId` itself still tracks the pointer, so Enter always targets the row + * the user last touched; only whether it is *drawn* depends on the mode. Mirrors + * `isKeyboardNav` in emcn's popover ("prevent dual highlights") and the single + * modality-driven focus marker Headless UI's Combobox exposes. + */ + const [isKeyboardNav, setIsKeyboardNav] = useState(false) const showSearch = workspaces.length > WORKSPACE_SEARCH_THRESHOLD const searchQuery = workspaceSearch.trim().toLowerCase() @@ -228,6 +241,7 @@ function WorkspaceHeaderImpl({ if (isWorkspaceMenuOpen) return setWorkspaceSearch('') setHighlightedId(null) + setIsKeyboardNav(false) }, [isWorkspaceMenuOpen]) const [isMounted, setIsMounted] = useState(false) @@ -533,10 +547,12 @@ function WorkspaceHeaderImpl({ if (filteredWorkspaces.length === 0) return if (e.key === 'ArrowDown') { e.preventDefault() + setIsKeyboardNav(true) const next = (activeIndex + 1) % filteredWorkspaces.length setHighlightedId(filteredWorkspaces[next].id) } else if (e.key === 'ArrowUp') { e.preventDefault() + setIsKeyboardNav(true) const next = (activeIndex - 1 + filteredWorkspaces.length) % filteredWorkspaces.length setHighlightedId(filteredWorkspaces[next].id) @@ -562,7 +578,7 @@ function WorkspaceHeaderImpl({ const initial = getWorkspaceInitial(workspace.name) const isActive = workspace.id === workspaceId const isMenuOpen = menuOpenWorkspaceId === workspace.id - const isKeyboardHighlighted = showSearch && idx === activeIndex + const isKeyboardHighlighted = showSearch && isKeyboardNav && idx === activeIndex /** * Hover-highlight is wired to `onMouseMove`, not `onMouseEnter`: a @@ -575,7 +591,14 @@ function WorkspaceHeaderImpl({
setHighlightedId(workspace.id) : undefined} + onMouseMove={ + showSearch + ? () => { + setIsKeyboardNav(false) + setHighlightedId(workspace.id) + } + : undefined + } > {editingWorkspaceId === workspace.id ? (
Date: Thu, 30 Jul 2026 11:41:23 -0700 Subject: [PATCH 2/3] test(sidebar): assert the keyboard cursor on a row that isn't selected MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 1: the keyboard-positive assertions landed on the current workspace, which carries its own `isActive` fill, so they held whether or not the cursor was painted — the tests could not have caught deleting keyboard-cursor rendering. Navigate with ArrowUp instead, wrapping from the seeded first row to the last, and assert on that row. Verified both directions now: removing the modality gate reddens three tests, removing keyboard painting reddens two. --- .../workspace-header/workspace-header.test.tsx | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.test.tsx b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.test.tsx index 04743f43d5e..e0bad761b47 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.test.tsx @@ -162,11 +162,13 @@ describe('WorkspaceHeader workspace switcher highlight', () => { const search = document.querySelector('input[placeholder="Search workspaces..."]') expect(search).not.toBeNull() act(() => { - search?.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true })) + search?.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowUp', bubbles: true })) }) - // ArrowDown from the seeded first row (RVT) lands on the second. - expect(isMarked("Emir's Workspace")).toBe(true) + // ArrowUp from the seeded first row wraps to the last. Asserting on Globex, not on + // the current workspace: the current one carries its own `isActive` fill, so it + // stays marked either way and would prove nothing about the cursor being painted. + expect(isMarked('Globex')).toBe(true) }) it('drops the keyboard cursor again as soon as the pointer moves', () => { @@ -174,16 +176,18 @@ describe('WorkspaceHeader workspace switcher highlight', () => { const search = document.querySelector('input[placeholder="Search workspaces..."]') act(() => { - search?.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true })) + search?.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowUp', bubbles: true })) }) - expect(isMarked("Emir's Workspace")).toBe(true) + expect(isMarked('Globex')).toBe(true) act(() => { row('Acme').dispatchEvent(new MouseEvent('mousemove', { bubbles: true })) }) - // Back in pointer mode: only the current workspace keeps a persistent mark. + // Back in pointer mode: the cursor is gone and the row just crossed is unmarked, + // leaving only the current workspace's own fill. + expect(isMarked('Globex')).toBe(false) expect(isMarked('Acme')).toBe(false) - expect(isMarked('RVT')).toBe(false) + expect(isMarked("Emir's Workspace")).toBe(true) }) }) From b9b2efdc7bead7a0f017bfdb8041addd9ffc3e4e Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 30 Jul 2026 11:44:51 -0700 Subject: [PATCH 3/3] fix(sidebar): do not arm Enter without a visible target MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review round 1: gating the cursor's paint on keyboard mode left Enter still acting on the seeded first row while that row was unmarked. The search field takes focus on open, so Enter could switch workspace with nothing shown as the target — emcn's popover instead holds its selection at -1 and ignores Enter until keyboard nav begins. Enter now acts only once a cursor is on screen, and typing counts as keyboard intent so the common "filter, then Enter" flow lands on a visible top result. Every path to Enter therefore has a marked target. --- .../workspace-header.test.tsx | 46 ++++++++++++++++++- .../workspace-header/workspace-header.tsx | 12 ++++- 2 files changed, 56 insertions(+), 2 deletions(-) diff --git a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.test.tsx b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.test.tsx index e0bad761b47..a315e751edd 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.test.tsx @@ -7,6 +7,8 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' const { mockNavigateToSettings } = vi.hoisted(() => ({ mockNavigateToSettings: vi.fn() })) +const onWorkspaceSwitch = vi.fn() + vi.mock('@tanstack/react-query', () => ({ useQueryClient: () => ({ invalidateQueries: vi.fn(), setQueryData: vi.fn() }), })) @@ -88,7 +90,7 @@ function render() { isCreatingWorkspace={false} isWorkspaceMenuOpen setIsWorkspaceMenuOpen={() => {}} - onWorkspaceSwitch={() => {}} + onWorkspaceSwitch={onWorkspaceSwitch} onCreateWorkspace={async () => {}} onRenameWorkspace={async () => {}} onDeleteWorkspace={async () => {}} @@ -122,6 +124,17 @@ function isMarked(name: string): boolean { ) } +/** + * Types into a React-controlled input. Assigning `.value` directly is ignored: React + * tracks the previous value on the node, so the change must go through the native + * setter for its synthetic `onChange` to fire. + */ +function typeInto(input: HTMLInputElement, value: string) { + const setValue = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, 'value')?.set + setValue?.call(input, value) + input.dispatchEvent(new Event('input', { bubbles: true })) +} + beforeEach(() => { vi.clearAllMocks() // jsdom implements neither; the component scrolls the active row into view. @@ -134,6 +147,37 @@ afterEach(() => { }) describe('WorkspaceHeader workspace switcher highlight', () => { + it('leaves Enter unarmed until a cursor is on screen', () => { + render() + + const search = document.querySelector('input[placeholder="Search workspaces..."]') + act(() => { + search?.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter', bubbles: true })) + }) + + // The search field is focused on open, so acting on the seeded row here would + // switch workspace with nothing marked. + expect(onWorkspaceSwitch).not.toHaveBeenCalled() + }) + + it('arms Enter on the top result once the user types', () => { + render() + + const search = document.querySelector( + 'input[placeholder="Search workspaces..."]' + ) as HTMLInputElement | null + act(() => { + if (search) typeInto(search, 'Acme') + }) + // Typing counts as keyboard intent, so the target is visible before Enter fires. + expect(isMarked('Acme')).toBe(true) + + act(() => { + search?.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter', bubbles: true })) + }) + expect(onWorkspaceSwitch).toHaveBeenCalledWith(expect.objectContaining({ id: 'ws-acme' })) + }) + it('marks only the current workspace when the menu opens', () => { render() diff --git a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.tsx b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.tsx index dad8b3f7ede..2357a7a4e42 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.tsx @@ -540,7 +540,12 @@ function WorkspaceHeaderImpl({ icon={Search} placeholder='Search workspaces...' value={workspaceSearch} - onChange={(e) => setWorkspaceSearch(e.target.value)} + onChange={(e) => { + // Typing is keyboard intent, so the cursor appears on the top + // result and Enter has a visible target. + setIsKeyboardNav(true) + setWorkspaceSearch(e.target.value) + }} onKeyDown={(e) => { e.stopPropagation() if (e.nativeEvent.isComposing) return @@ -557,6 +562,11 @@ function WorkspaceHeaderImpl({ (activeIndex - 1 + filteredWorkspaces.length) % filteredWorkspaces.length setHighlightedId(filteredWorkspaces[next].id) } else if (e.key === 'Enter') { + // Only armed once a cursor is actually on screen. The search + // field is focused on open, so acting on the seeded row here + // would switch workspace with nothing marked — emcn's popover + // likewise holds its selection at -1 until keyboard nav starts. + if (!isKeyboardNav) return e.preventDefault() const target = filteredWorkspaces[activeIndex] if (target) onWorkspaceSwitch(target)