Skip to content

Commit 6647808

Browse files
waleedlatif1Waleed Latif
andauthored
fix(sidebar): stop the workspace switcher stranding a phantom hover (#6096)
* 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. * test(sidebar): assert the keyboard cursor on a row that isn't selected 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. * fix(sidebar): do not arm Enter without a visible target 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. --------- Co-authored-by: Waleed Latif <waleed@simstudio.ai>
1 parent d95127d commit 6647808

2 files changed

Lines changed: 273 additions & 3 deletions

File tree

Lines changed: 237 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,237 @@
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+
const onWorkspaceSwitch = vi.fn()
11+
12+
vi.mock('@tanstack/react-query', () => ({
13+
useQueryClient: () => ({ invalidateQueries: vi.fn(), setQueryData: vi.fn() }),
14+
}))
15+
vi.mock('@/lib/auth/auth-client', () => ({ useActiveOrganization: () => ({ data: null }) }))
16+
vi.mock('@/hooks/use-settings-navigation', () => ({
17+
useSettingsNavigation: () => ({ navigateToSettings: mockNavigateToSettings }),
18+
}))
19+
vi.mock('@/hooks/use-permission-config', () => ({
20+
usePermissionConfig: () => ({ isInvitationsDisabled: false }),
21+
}))
22+
vi.mock('@/app/workspace/[workspaceId]/providers/workspace-permissions-provider', () => ({
23+
useWorkspacePermissionsContext: () => ({
24+
userPermissions: { canAdmin: true, canEdit: true, canRead: true },
25+
}),
26+
}))
27+
vi.mock('@/hooks/queries/invitations', () => ({ invitationKeys: { all: ['invitations'] } }))
28+
vi.mock('@/hooks/queries/workspace', () => ({ workspaceKeys: { all: ['workspaces'] } }))
29+
30+
/** Modal/menu siblings are irrelevant to the highlight and drag in heavy trees. */
31+
vi.mock('@/app/workspace/[workspaceId]/components/invite-modal', () => ({
32+
InviteModal: () => null,
33+
}))
34+
vi.mock(
35+
'@/app/workspace/[workspaceId]/w/components/sidebar/components/workflow-list/components/context-menu/context-menu',
36+
() => ({ ContextMenu: () => null })
37+
)
38+
vi.mock(
39+
'@/app/workspace/[workspaceId]/w/components/sidebar/components/workflow-list/components/delete-modal/delete-modal',
40+
() => ({ DeleteModal: () => null })
41+
)
42+
vi.mock(
43+
'@/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/components/create-workspace-modal/create-workspace-modal',
44+
() => ({ CreateWorkspaceModal: () => null })
45+
)
46+
vi.mock(
47+
'@/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/components/pending-invitations/view-invitations-menu-item',
48+
() => ({ ViewInvitationsMenuItem: () => null })
49+
)
50+
vi.mock(
51+
'@/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/components/pending-invitations/view-invitations-modal',
52+
() => ({ ViewInvitationsModal: () => null })
53+
)
54+
55+
import { WorkspaceHeader } from '@/app/workspace/[workspaceId]/w/components/sidebar/components/workspace-header/workspace-header'
56+
57+
/**
58+
* `@sim/emcn` is deliberately NOT mocked: the assertion is about the background class
59+
* `chipVariants` produces, so a stubbed chip would only assert the stub.
60+
*/
61+
const ACTIVE_BG = 'bg-[var(--surface-active)]'
62+
63+
/**
64+
* Above `WORKSPACE_SEARCH_THRESHOLD` (3), so the searchable/keyboard list renders.
65+
* The current workspace is deliberately NOT first: the highlight is seeded to row 0 on
66+
* open, so a current workspace sitting at row 0 would mask the double-mark this guards.
67+
*/
68+
const WORKSPACES = [
69+
{ id: 'ws-rvt', name: 'RVT' },
70+
{ id: 'ws-emir', name: "Emir's Workspace" },
71+
{ id: 'ws-acme', name: 'Acme' },
72+
{ id: 'ws-globex', name: 'Globex' },
73+
] as unknown as Parameters<typeof WorkspaceHeader>[0]['workspaces']
74+
75+
let container: HTMLDivElement
76+
let root: Root
77+
78+
function render() {
79+
;(globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true
80+
container = document.createElement('div')
81+
document.body.appendChild(container)
82+
root = createRoot(container)
83+
act(() => {
84+
root.render(
85+
<WorkspaceHeader
86+
activeWorkspace={{ name: "Emir's Workspace" }}
87+
workspaceId='ws-emir'
88+
workspaces={WORKSPACES}
89+
isWorkspacesLoading={false}
90+
isCreatingWorkspace={false}
91+
isWorkspaceMenuOpen
92+
setIsWorkspaceMenuOpen={() => {}}
93+
onWorkspaceSwitch={onWorkspaceSwitch}
94+
onCreateWorkspace={async () => {}}
95+
onRenameWorkspace={async () => {}}
96+
onDeleteWorkspace={async () => {}}
97+
isDeletingWorkspace={false}
98+
onUploadLogo={() => {}}
99+
onLeaveWorkspace={async () => {}}
100+
isLeavingWorkspace={false}
101+
/>
102+
)
103+
})
104+
}
105+
106+
function row(name: string): HTMLElement {
107+
const found = [...document.querySelectorAll('[data-workspace-row-idx]')].find((el) =>
108+
el.textContent?.includes(name)
109+
)
110+
if (!found) throw new Error(`No workspace row rendered for "${name}"`)
111+
return found as HTMLElement
112+
}
113+
114+
/**
115+
* Whether a row is painted with the persistent active fill.
116+
*
117+
* Matches an exact class token, never a substring: the inactive chip carries
118+
* `hover-hover:bg-[var(--surface-active)]`, which *contains* the active class, so a
119+
* substring check reports every row as marked.
120+
*/
121+
function isMarked(name: string): boolean {
122+
return [...row(name).querySelectorAll<HTMLElement>('*')].some((el) =>
123+
el.classList.contains(ACTIVE_BG)
124+
)
125+
}
126+
127+
/**
128+
* Types into a React-controlled input. Assigning `.value` directly is ignored: React
129+
* tracks the previous value on the node, so the change must go through the native
130+
* setter for its synthetic `onChange` to fire.
131+
*/
132+
function typeInto(input: HTMLInputElement, value: string) {
133+
const setValue = Object.getOwnPropertyDescriptor(HTMLInputElement.prototype, 'value')?.set
134+
setValue?.call(input, value)
135+
input.dispatchEvent(new Event('input', { bubbles: true }))
136+
}
137+
138+
beforeEach(() => {
139+
vi.clearAllMocks()
140+
// jsdom implements neither; the component scrolls the active row into view.
141+
Element.prototype.scrollIntoView = vi.fn()
142+
})
143+
144+
afterEach(() => {
145+
act(() => root.unmount())
146+
container.remove()
147+
})
148+
149+
describe('WorkspaceHeader workspace switcher highlight', () => {
150+
it('leaves Enter unarmed until a cursor is on screen', () => {
151+
render()
152+
153+
const search = document.querySelector('input[placeholder="Search workspaces..."]')
154+
act(() => {
155+
search?.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter', bubbles: true }))
156+
})
157+
158+
// The search field is focused on open, so acting on the seeded row here would
159+
// switch workspace with nothing marked.
160+
expect(onWorkspaceSwitch).not.toHaveBeenCalled()
161+
})
162+
163+
it('arms Enter on the top result once the user types', () => {
164+
render()
165+
166+
const search = document.querySelector(
167+
'input[placeholder="Search workspaces..."]'
168+
) as HTMLInputElement | null
169+
act(() => {
170+
if (search) typeInto(search, 'Acme')
171+
})
172+
// Typing counts as keyboard intent, so the target is visible before Enter fires.
173+
expect(isMarked('Acme')).toBe(true)
174+
175+
act(() => {
176+
search?.dispatchEvent(new KeyboardEvent('keydown', { key: 'Enter', bubbles: true }))
177+
})
178+
expect(onWorkspaceSwitch).toHaveBeenCalledWith(expect.objectContaining({ id: 'ws-acme' }))
179+
})
180+
181+
it('marks only the current workspace when the menu opens', () => {
182+
render()
183+
184+
expect(isMarked("Emir's Workspace")).toBe(true)
185+
// Regression: the keyboard cursor used to be seeded to row 0 on open, so a second
186+
// row was marked in the same colour as hover before any interaction.
187+
expect(isMarked('RVT')).toBe(false)
188+
})
189+
190+
it('does not leave a highlight behind when the pointer moves across a row', () => {
191+
render()
192+
193+
act(() => {
194+
row('RVT').dispatchEvent(new MouseEvent('mousemove', { bubbles: true }))
195+
})
196+
197+
// The pointer has moved on; nothing should be painted as if still hovered.
198+
// Real CSS :hover handles the row actually under the cursor and leaves with it.
199+
expect(isMarked('RVT')).toBe(false)
200+
expect(isMarked("Emir's Workspace")).toBe(true)
201+
})
202+
203+
it('shows the cursor once the user navigates by keyboard', () => {
204+
render()
205+
206+
const search = document.querySelector('input[placeholder="Search workspaces..."]')
207+
expect(search).not.toBeNull()
208+
act(() => {
209+
search?.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowUp', bubbles: true }))
210+
})
211+
212+
// ArrowUp from the seeded first row wraps to the last. Asserting on Globex, not on
213+
// the current workspace: the current one carries its own `isActive` fill, so it
214+
// stays marked either way and would prove nothing about the cursor being painted.
215+
expect(isMarked('Globex')).toBe(true)
216+
})
217+
218+
it('drops the keyboard cursor again as soon as the pointer moves', () => {
219+
render()
220+
221+
const search = document.querySelector('input[placeholder="Search workspaces..."]')
222+
act(() => {
223+
search?.dispatchEvent(new KeyboardEvent('keydown', { key: 'ArrowUp', bubbles: true }))
224+
})
225+
expect(isMarked('Globex')).toBe(true)
226+
227+
act(() => {
228+
row('Acme').dispatchEvent(new MouseEvent('mousemove', { bubbles: true }))
229+
})
230+
231+
// Back in pointer mode: the cursor is gone and the row just crossed is unmarked,
232+
// leaving only the current workspace's own fill.
233+
expect(isMarked('Globex')).toBe(false)
234+
expect(isMarked('Acme')).toBe(false)
235+
expect(isMarked("Emir's Workspace")).toBe(true)
236+
})
237+
})

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

Lines changed: 36 additions & 3 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)
@@ -526,21 +540,33 @@ function WorkspaceHeaderImpl({
526540
icon={Search}
527541
placeholder='Search workspaces...'
528542
value={workspaceSearch}
529-
onChange={(e) => setWorkspaceSearch(e.target.value)}
543+
onChange={(e) => {
544+
// Typing is keyboard intent, so the cursor appears on the top
545+
// result and Enter has a visible target.
546+
setIsKeyboardNav(true)
547+
setWorkspaceSearch(e.target.value)
548+
}}
530549
onKeyDown={(e) => {
531550
e.stopPropagation()
532551
if (e.nativeEvent.isComposing) return
533552
if (filteredWorkspaces.length === 0) return
534553
if (e.key === 'ArrowDown') {
535554
e.preventDefault()
555+
setIsKeyboardNav(true)
536556
const next = (activeIndex + 1) % filteredWorkspaces.length
537557
setHighlightedId(filteredWorkspaces[next].id)
538558
} else if (e.key === 'ArrowUp') {
539559
e.preventDefault()
560+
setIsKeyboardNav(true)
540561
const next =
541562
(activeIndex - 1 + filteredWorkspaces.length) % filteredWorkspaces.length
542563
setHighlightedId(filteredWorkspaces[next].id)
543564
} else if (e.key === 'Enter') {
565+
// Only armed once a cursor is actually on screen. The search
566+
// field is focused on open, so acting on the seeded row here
567+
// would switch workspace with nothing marked — emcn's popover
568+
// likewise holds its selection at -1 until keyboard nav starts.
569+
if (!isKeyboardNav) return
544570
e.preventDefault()
545571
const target = filteredWorkspaces[activeIndex]
546572
if (target) onWorkspaceSwitch(target)
@@ -562,7 +588,7 @@ function WorkspaceHeaderImpl({
562588
const initial = getWorkspaceInitial(workspace.name)
563589
const isActive = workspace.id === workspaceId
564590
const isMenuOpen = menuOpenWorkspaceId === workspace.id
565-
const isKeyboardHighlighted = showSearch && idx === activeIndex
591+
const isKeyboardHighlighted = showSearch && isKeyboardNav && idx === activeIndex
566592

567593
/**
568594
* Hover-highlight is wired to `onMouseMove`, not `onMouseEnter`: a
@@ -575,7 +601,14 @@ function WorkspaceHeaderImpl({
575601
<div
576602
key={workspace.id}
577603
data-workspace-row-idx={showSearch ? idx : undefined}
578-
onMouseMove={showSearch ? () => setHighlightedId(workspace.id) : undefined}
604+
onMouseMove={
605+
showSearch
606+
? () => {
607+
setIsKeyboardNav(false)
608+
setHighlightedId(workspace.id)
609+
}
610+
: undefined
611+
}
579612
>
580613
{editingWorkspaceId === workspace.id ? (
581614
<div

0 commit comments

Comments
 (0)