From 3d28e21a5a7b711b1f7df84dfe8df16d8609c699 Mon Sep 17 00:00:00 2001 From: lucaronin Date: Sat, 6 Jun 2026 14:35:01 +0200 Subject: [PATCH] fix: keep palette keyboard selection stable --- src/components/CommandPalette.test.tsx | 22 ++++++ src/components/CommandPalette.tsx | 2 +- src/components/NoteSearchList.test.tsx | 74 +++++++++++--------- src/components/NoteSearchList.tsx | 74 +++++++++++++------- src/components/QuickOpenPalette.test.tsx | 11 +++ tests/smoke/keyboard-command-routing.spec.ts | 59 +++++++++++++--- 6 files changed, 169 insertions(+), 73 deletions(-) diff --git a/src/components/CommandPalette.test.tsx b/src/components/CommandPalette.test.tsx index 7d8a0f3e..f5f45cf4 100644 --- a/src/components/CommandPalette.test.tsx +++ b/src/components/CommandPalette.test.tsx @@ -278,6 +278,28 @@ describe('CommandPalette', () => { expect(onClose).toHaveBeenCalled() }) + it('keeps keyboard selection stable when the mouse is already over a row', () => { + render() + + fireEvent.mouseEnter(screen.getByText('Commit & Push')) + fireEvent.keyDown(window, { key: 'ArrowDown' }) + fireEvent.keyDown(window, { key: 'Enter' }) + + expect(commands[1].execute).toHaveBeenCalledOnce() + expect(commands[2].execute).not.toHaveBeenCalled() + expect(onClose).toHaveBeenCalledOnce() + }) + + it('lets real mouse movement take over command selection', () => { + render() + + fireEvent.mouseMove(screen.getByText('Commit & Push')) + fireEvent.keyDown(window, { key: 'Enter' }) + + expect(commands[2].execute).toHaveBeenCalledOnce() + expect(onClose).toHaveBeenCalledOnce() + }) + it('keeps a short query keyboard-selectable after ArrowDown and Enter', () => { const changeNoteType = makeCommand({ id: 'change-note-type', diff --git a/src/components/CommandPalette.tsx b/src/components/CommandPalette.tsx index 44d48124..1a7bf663 100644 --- a/src/components/CommandPalette.tsx +++ b/src/components/CommandPalette.tsx @@ -484,7 +484,7 @@ function CommandRow({ command, selected, onHover, onSelect }: CommandRowProps) { selected ? 'bg-accent' : 'hover:bg-secondary', )} onClick={onSelect} - onMouseEnter={onHover} + onMouseMove={onHover} > {command.label} {command.shortcut && ( diff --git a/src/components/NoteSearchList.test.tsx b/src/components/NoteSearchList.test.tsx index 29aae475..e39c9cf7 100644 --- a/src/components/NoteSearchList.test.tsx +++ b/src/components/NoteSearchList.test.tsx @@ -33,6 +33,34 @@ describe('NoteSearchList', () => { const onItemClick = vi.fn() const onItemHover = vi.fn() + const renderList = ({ + listItems = items, + selectedIndex = 0, + getItemKey = (item: TestItem) => item.id, + itemClick = onItemClick, + itemHover, + activateOnMouseDown, + emptyMessage, + }: { + listItems?: TestItem[] + selectedIndex?: number + getItemKey?: (item: TestItem, index: number) => string + itemClick?: (item: TestItem, index: number) => void + itemHover?: (index: number) => void + activateOnMouseDown?: boolean + emptyMessage?: string + } = {}) => render( + , + ) + beforeEach(() => { vi.clearAllMocks() }) @@ -112,39 +140,17 @@ describe('NoteSearchList', () => { }) it('shows empty message when no items', () => { - render( - ''} - onItemClick={onItemClick} - emptyMessage="No matching notes" - />, - ) + renderList({ listItems: [], getItemKey: () => '', emptyMessage: 'No matching notes' }) expect(screen.getByText('No matching notes')).toBeInTheDocument() }) it('shows default empty message', () => { - render( - ''} - onItemClick={onItemClick} - />, - ) + renderList({ listItems: [], getItemKey: () => '' }) expect(screen.getByText('No results')).toBeInTheDocument() }) it('calls onItemClick when an item is clicked', () => { - render( - item.id} - onItemClick={onItemClick} - />, - ) + renderList() fireEvent.click(screen.getByText('Beta Notes')) expect(onItemClick).toHaveBeenCalledWith(items[1], 1) }) @@ -184,17 +190,15 @@ describe('NoteSearchList', () => { expect(preventDefault).toHaveBeenCalledOnce() }) - it('calls onItemHover when mouse enters an item', () => { - render( - item.id} - onItemClick={onItemClick} - onItemHover={onItemHover} - />, - ) + it('does not call onItemHover for mouse enter alone', () => { + renderList({ itemHover: onItemHover }) fireEvent.mouseEnter(screen.getByText('Gamma Experiment')) + expect(onItemHover).not.toHaveBeenCalled() + }) + + it('calls onItemHover when the mouse actually moves over an item', () => { + renderList({ itemHover: onItemHover }) + fireEvent.mouseMove(screen.getByText('Gamma Experiment')) expect(onItemHover).toHaveBeenCalledWith(2) }) diff --git a/src/components/NoteSearchList.tsx b/src/components/NoteSearchList.tsx index 85387687..156ff75c 100644 --- a/src/components/NoteSearchList.tsx +++ b/src/components/NoteSearchList.tsx @@ -36,6 +36,47 @@ interface NoteSearchListItemProps { activateOnMouseDown?: boolean } +type SearchItemPressEvent = MouseEvent | PointerEvent + +function useSearchItemActivation({ + item, + index, + onItemClick, + activateOnMouseDown, +}: Pick, 'item' | 'index' | 'onItemClick' | 'activateOnMouseDown'>) { + const pressActivatedRef = useRef(false) + + const activateItem = () => onItemClick(item, index) + + const clearPressActivation = () => { + pressActivatedRef.current = false + } + + const activateFromPress = (event: SearchItemPressEvent) => { + event.preventDefault() + if (!activateOnMouseDown) return + + event.stopPropagation() + if (pressActivatedRef.current) return + + pressActivatedRef.current = true + window.setTimeout(clearPressActivation, 0) + activateItem() + } + + const handleClick = (event: MouseEvent) => { + if (!activateOnMouseDown) { + activateItem() + return + } + + event.preventDefault() + event.stopPropagation() + } + + return { activateFromPress, handleClick } +} + function NoteSearchListItem({ item, index, @@ -44,31 +85,12 @@ function NoteSearchListItem({ onItemHover, activateOnMouseDown, }: NoteSearchListItemProps) { - const pressActivatedRef = useRef(false) - - const activateFromPress = (event: MouseEvent | PointerEvent) => { - event.preventDefault() - if (!activateOnMouseDown) return - - event.stopPropagation() - if (pressActivatedRef.current) return - - pressActivatedRef.current = true - window.setTimeout(() => { - pressActivatedRef.current = false - }, 0) - onItemClick(item, index) - } - - const handleClick = (event: MouseEvent) => { - if (activateOnMouseDown) { - event.preventDefault() - event.stopPropagation() - return - } - - onItemClick(item, index) - } + const { activateFromPress, handleClick } = useSearchItemActivation({ + item, + index, + onItemClick, + activateOnMouseDown, + }) return (
({ onPointerDownCapture={activateFromPress} onMouseDownCapture={activateFromPress} onClick={handleClick} - onMouseEnter={() => onItemHover?.(index)} + onMouseMove={() => onItemHover?.(index)} > {item.TypeIcon && ( diff --git a/src/components/QuickOpenPalette.test.tsx b/src/components/QuickOpenPalette.test.tsx index e6040d9a..5fb526fa 100644 --- a/src/components/QuickOpenPalette.test.tsx +++ b/src/components/QuickOpenPalette.test.tsx @@ -161,6 +161,17 @@ describe('QuickOpenPalette', () => { expect(onClose).toHaveBeenCalled() }) + it('keeps keyboard selection stable when the mouse is already over a result', () => { + render() + + fireEvent.mouseEnter(screen.getByText('Gamma Experiment')) + fireEvent.keyDown(window, { key: 'ArrowDown' }) + fireEvent.keyDown(window, { key: 'Enter' }) + + expect(onSelect).toHaveBeenCalledWith(entries[1]) + expect(onClose).toHaveBeenCalledOnce() + }) + it('does not go below the last item with ArrowDown', () => { render() diff --git a/tests/smoke/keyboard-command-routing.spec.ts b/tests/smoke/keyboard-command-routing.spec.ts index e96b982c..046902ae 100644 --- a/tests/smoke/keyboard-command-routing.spec.ts +++ b/tests/smoke/keyboard-command-routing.spec.ts @@ -75,6 +75,28 @@ async function dispatchAppCommand(page: Page, id: string): Promise { }, id) } +async function openQuickOpenFromKeyboard(page: Page): Promise { + await dispatchShortcutEvent(page, { + key: 'p', + code: 'KeyP', + ctrlKey: false, + metaKey: true, + shiftKey: false, + altKey: false, + bubbles: true, + cancelable: true, + }) + await expect(page.getByTestId('quick-open-palette')).toBeVisible({ timeout: 5_000 }) +} + +function selectedQuickOpenTitle(page: Page) { + return page.getByTestId('quick-open-palette').locator('div.bg-accent > button:has([data-testid="note-search-item-icon"])').first() +} + +function quickOpenTitleAt(page: Page, index: number) { + return page.getByTestId('quick-open-palette').locator('button:has([data-testid="note-search-item-icon"])').nth(index) +} + async function installGlobalSearchResultsHarness(page: Page): Promise { await page.waitForFunction(() => Boolean(window.__mockHandlers?.search_vault)) await page.evaluate((results) => { @@ -134,17 +156,7 @@ test.describe('keyboard command routing', () => { test('desktop shortcut bridge opens quick open through both Cmd+P and Cmd+O @smoke', async ({ page }) => { await openFixtureVaultDesktopHarness(page, tempVaultDir) - await dispatchShortcutEvent(page, { - key: 'p', - code: 'KeyP', - ctrlKey: false, - metaKey: true, - shiftKey: false, - altKey: false, - bubbles: true, - cancelable: true, - }) - await expect(page.getByTestId('quick-open-palette')).toBeVisible({ timeout: 5_000 }) + await openQuickOpenFromKeyboard(page) await expect(page.locator('input[placeholder="Search notes..."]')).toBeFocused() await page.keyboard.press('Escape') @@ -164,6 +176,31 @@ test.describe('keyboard command routing', () => { await expect(page.locator('input[placeholder="Search notes..."]')).toBeFocused() }) + test('quick open ignores a stationary pointer until it moves @smoke', async ({ page }) => { + await openFixtureVaultDesktopHarness(page, tempVaultDir) + await openQuickOpenFromKeyboard(page) + + const initialTitle = await selectedQuickOpenTitle(page).textContent() + const secondTitle = quickOpenTitleAt(page, 1) + const targetTitle = await secondTitle.textContent() + const secondTitleBox = await secondTitle.boundingBox() + if (!initialTitle) throw new Error('Quick open did not select an initial note') + if (!targetTitle) throw new Error('Quick open did not render a second note title') + if (!secondTitleBox) throw new Error('Quick open did not render a second note box') + + await page.keyboard.press('Escape') + await expect(page.getByTestId('quick-open-palette')).not.toBeVisible({ timeout: 5_000 }) + await page.mouse.move( + secondTitleBox.x + secondTitleBox.width / 2, + secondTitleBox.y + secondTitleBox.height / 2, + ) + + await openQuickOpenFromKeyboard(page) + await expect(selectedQuickOpenTitle(page)).toHaveText(initialTitle) + await page.keyboard.press('ArrowDown') + await expect(selectedQuickOpenTitle(page)).toHaveText(targetTitle) + }) + test('global search arrow keys move one result at a time @smoke', async ({ page }) => { await openFixtureVaultDesktopHarness(page, tempVaultDir) await installGlobalSearchResultsHarness(page)