Skip to content

Commit fbc6fa1

Browse files
author
Waleed Latif
committed
fix(sidebar): stop the workspace switcher stranding a phantom hover
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.
1 parent 32293f4 commit fbc6fa1

2 files changed

Lines changed: 214 additions & 2 deletions

File tree

Lines changed: 189 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,189 @@
1+
/**
2+
* @vitest-environment jsdom
3+
*/
4+
import { act } from 'react'
5+
import { createRoot, type Root } from 'react-dom/client'
6+
import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
7+
8+
const { mockNavigateToSettings } = vi.hoisted(() => ({ mockNavigateToSettings: vi.fn() }))
9+
10+
vi.mock('@tanstack/react-query', () => ({
11+
useQueryClient: () => ({ invalidateQueries: vi.fn(), setQueryData: vi.fn() }),
12+
}))
13+
vi.mock('@/lib/auth/auth-client', () => ({ useActiveOrganization: () => ({ data: null }) }))
14+
vi.mock('@/hooks/use-settings-navigation', () => ({
15+
useSettingsNavigation: () => ({ navigateToSettings: mockNavigateToSettings }),
16+
}))
17+
vi.mock('@/hooks/use-permission-config', () => ({
18+
usePermissionConfig: () => ({ isInvitationsDisabled: false }),
19+
}))
20+
vi.mock('@/app/workspace/[workspaceId]/providers/workspace-permissions-provider', () => ({
21+
useWorkspacePermissionsContext: () => ({
22+
userPermissions: { canAdmin: true, canEdit: true, canRead: true },
23+
}),
24+
}))
25+
vi.mock('@/hooks/queries/invitations', () => ({ invitationKeys: { all: ['invitations'] } }))
26+
vi.mock('@/hooks/queries/workspace', () => ({ workspaceKeys: { all: ['workspaces'] } }))
27+
28+
/** Modal/menu siblings are irrelevant to the highlight and drag in heavy trees. */
29+
vi.mock('@/app/workspace/[workspaceId]/components/invite-modal', () => ({
30+
InviteModal: () => null,
31+
}))
32+
vi.mock(
33+
'@/app/workspace/[workspaceId]/w/components/sidebar/components/workflow-list/components/context-menu/context-menu',
34+
() => ({ ContextMenu: () => null })
35+
)
36+
vi.mock(
37+
'@/app/workspace/[workspaceId]/w/components/sidebar/components/workflow-list/components/delete-modal/delete-modal',
38+
() => ({ DeleteModal: () => null })
39+
)
40+
vi.mock(
41+
'@/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/components/create-workspace-modal/create-workspace-modal',
42+
() => ({ CreateWorkspaceModal: () => null })
43+
)
44+
vi.mock(
45+
'@/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/components/pending-invitations/view-invitations-menu-item',
46+
() => ({ ViewInvitationsMenuItem: () => null })
47+
)
48+
vi.mock(
49+
'@/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/components/pending-invitations/view-invitations-modal',
50+
() => ({ ViewInvitationsModal: () => null })
51+
)
52+
53+
import { WorkspaceHeader } from '@/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header'
54+
55+
/**
56+
* `@sim/emcn` is deliberately NOT mocked: the assertion is about the background class
57+
* `chipVariants` produces, so a stubbed chip would only assert the stub.
58+
*/
59+
const ACTIVE_BG = 'bg-[var(--surface-active)]'
60+
61+
/**
62+
* Above `WORKSPACE_SEARCH_THRESHOLD` (3), so the searchable/keyboard list renders.
63+
* The current workspace is deliberately NOT first: the highlight is seeded to row 0 on
64+
* open, so a current workspace sitting at row 0 would mask the double-mark this guards.
65+
*/
66+
const WORKSPACES = [
67+
{ id: 'ws-rvt', name: 'RVT' },
68+
{ id: 'ws-emir', name: "Emir's Workspace" },
69+
{ id: 'ws-acme', name: 'Acme' },
70+
{ id: 'ws-globex', name: 'Globex' },
71+
] as unknown as Parameters<typeof WorkspaceHeader>[0]['workspaces']
72+
73+
let container: HTMLDivElement
74+
let root: Root
75+
76+
function render() {
77+
;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true
78+
container = document.createElement('div')
79+
document.body.appendChild(container)
80+
root = createRoot(container)
81+
act(() => {
82+
root.render(
83+
<WorkspaceHeader
84+
activeWorkspace={{ name: "Emir's Workspace" }}
85+
workspaceId='ws-emir'
86+
workspaces={WORKSPACES}
87+
isWorkspacesLoading={false}
88+
isCreatingWorkspace={false}
89+
isWorkspaceMenuOpen
90+
setIsWorkspaceMenuOpen={() => {}}
91+
onWorkspaceSwitch={() => {}}
92+
onCreateWorkspace={async () => {}}
93+
onRenameWorkspace={async () => {}}
94+
onDeleteWorkspace={async () => {}}
95+
isDeletingWorkspace={false}
96+
onUploadLogo={() => {}}
97+
onLeaveWorkspace={async () => {}}
98+
isLeavingWorkspace={false}
99+
/>
100+
)
101+
})
102+
}
103+
104+
function row(name: string): HTMLElement {
105+
const found = [...document.querySelectorAll('[data-workspace-row-idx]')].find((el) =>
106+
el.textContent?.includes(name)
107+
)
108+
if (!found) throw new Error(`No workspace row rendered for "${name}"`)
109+
return found as HTMLElement
110+
}
111+
112+
/**
113+
* Whether a row is painted with the persistent active fill.
114+
*
115+
* Matches an exact class token, never a substring: the inactive chip carries
116+
* `hover-hover:bg-[var(--surface-active)]`, which *contains* the active class, so a
117+
* substring check reports every row as marked.
118+
*/
119+
function isMarked(name: string): boolean {
120+
return [...row(name).querySelectorAll<HTMLElement>('*')].some((el) =>
121+
el.classList.contains(ACTIVE_BG)
122+
)
123+
}
124+
125+
beforeEach(() => {
126+
vi.clearAllMocks()
127+
// jsdom implements neither; the component scrolls the active row into view.
128+
Element.prototype.scrollIntoView = vi.fn()
129+
})
130+
131+
afterEach(() => {
132+
act(() => root.unmount())
133+
container.remove()
134+
})
135+
136+
describe('WorkspaceHeader workspace switcher highlight', () => {
137+
it('marks only the current workspace when the menu opens', () => {
138+
render()
139+
140+
expect(isMarked("Emir's Workspace")).toBe(true)
141+
// Regression: the keyboard cursor used to be seeded to row 0 on open, so a second
142+
// row was marked in the same colour as hover before any interaction.
143+
expect(isMarked('RVT')).toBe(false)
144+
})
145+
146+
it('does not leave a highlight behind when the pointer moves across a row', () => {
147+
render()
148+
149+
act(() => {
150+
row('RVT').dispatchEvent(new MouseEvent('mousemove', { bubbles: true }))
151+
})
152+
153+
// The pointer has moved on; nothing should be painted as if still hovered.
154+
// Real CSS :hover handles the row actually under the cursor and leaves with it.
155+
expect(isMarked('RVT')).toBe(false)
156+
expect(isMarked("Emir's Workspace")).toBe(true)
157+
})
158+
159+
it('shows the cursor once the user navigates by keyboard', () => {
160+
render()
161+
162+
const search = document.querySelector('input[placeholder="Search workspaces..."]')
163+
expect(search).not.toBeNull()
164+
act(() => {
165+
search?.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true }))
166+
})
167+
168+
// ArrowDown from the seeded first row (RVT) lands on the second.
169+
expect(isMarked("Emir's Workspace")).toBe(true)
170+
})
171+
172+
it('drops the keyboard cursor again as soon as the pointer moves', () => {
173+
render()
174+
175+
const search = document.querySelector('input[placeholder="Search workspaces..."]')
176+
act(() => {
177+
search?.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowDown', bubbles: true }))
178+
})
179+
expect(isMarked("Emir's Workspace")).toBe(true)
180+
181+
act(() => {
182+
row('Acme').dispatchEvent(new MouseEvent('mousemove', { bubbles: true }))
183+
})
184+
185+
// Back in pointer mode: only the current workspace keeps a persistent mark.
186+
expect(isMarked('Acme')).toBe(false)
187+
expect(isMarked('RVT')).toBe(false)
188+
})
189+
})

apps/sim/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header.tsx

Lines changed: 25 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -169,6 +169,19 @@ function WorkspaceHeaderImpl({
169169

170170
const [workspaceSearch, setWorkspaceSearch] = useState('')
171171
const [highlightedId, setHighlightedId] = useState<string | null>(null)
172+
/**
173+
* Which input the user is currently driving the list with. The highlight is only
174+
* painted in keyboard mode, because it renders in `--surface-active` — the same
175+
* token hover uses — so a highlight left behind by the pointer is indistinguishable
176+
* from a stuck hover, and sits alongside the equally-`--surface-active` current
177+
* workspace as a second phantom-hovered row.
178+
*
179+
* `highlightedId` itself still tracks the pointer, so Enter always targets the row
180+
* the user last touched; only whether it is *drawn* depends on the mode. Mirrors
181+
* `isKeyboardNav` in emcn's popover ("prevent dual highlights") and the single
182+
* modality-driven focus marker Headless UI's Combobox exposes.
183+
*/
184+
const [isKeyboardNav, setIsKeyboardNav] = useState(false)
172185

173186
const showSearch = workspaces.length > WORKSPACE_SEARCH_THRESHOLD
174187
const searchQuery = workspaceSearch.trim().toLowerCase()
@@ -228,6 +241,7 @@ function WorkspaceHeaderImpl({
228241
if (isWorkspaceMenuOpen) return
229242
setWorkspaceSearch('')
230243
setHighlightedId(null)
244+
setIsKeyboardNav(false)
231245
}, [isWorkspaceMenuOpen])
232246

233247
const [isMounted, setIsMounted] = useState(false)
@@ -533,10 +547,12 @@ function WorkspaceHeaderImpl({
533547
if (filteredWorkspaces.length === 0) return
534548
if (e.key === 'ArrowDown') {
535549
e.preventDefault()
550+
setIsKeyboardNav(true)
536551
const next = (activeIndex + 1) % filteredWorkspaces.length
537552
setHighlightedId(filteredWorkspaces[next].id)
538553
} else if (e.key === 'ArrowUp') {
539554
e.preventDefault()
555+
setIsKeyboardNav(true)
540556
const next =
541557
(activeIndex - 1 + filteredWorkspaces.length) % filteredWorkspaces.length
542558
setHighlightedId(filteredWorkspaces[next].id)
@@ -562,7 +578,7 @@ function WorkspaceHeaderImpl({
562578
const initial = getWorkspaceInitial(workspace.name)
563579
const isActive = workspace.id === workspaceId
564580
const isMenuOpen = menuOpenWorkspaceId === workspace.id
565-
const isKeyboardHighlighted = showSearch && idx === activeIndex
581+
const isKeyboardHighlighted = showSearch && isKeyboardNav && idx === activeIndex
566582

567583
/**
568584
* Hover-highlight is wired to `onMouseMove`, not `onMouseEnter`: a
@@ -575,7 +591,14 @@ function WorkspaceHeaderImpl({
575591
<div
576592
key={workspace.id}
577593
data-workspace-row-idx={showSearch ? idx : undefined}
578-
onMouseMove={showSearch ? () => setHighlightedId(workspace.id) : undefined}
594+
onMouseMove={
595+
showSearch
596+
? () => {
597+
setIsKeyboardNav(false)
598+
setHighlightedId(workspace.id)
599+
}
600+
: undefined
601+
}
579602
>
580603
{editingWorkspaceId === workspace.id ? (
581604
<div

0 commit comments

Comments
 (0)