From 6d427e82dfef70ab55e6b4fe280305bf7b6c06a0 Mon Sep 17 00:00:00 2001 From: teknium1 <127238744+teknium1@users.noreply.github.com> Date: Sun, 26 Jul 2026 14:41:56 -0700 Subject: [PATCH] feat(desktop): complete find-in-page with the find-next/find-previous accelerators MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #53891 (cherry-picked in the three preceding commits) landed the Electron bridge, the store, the find bar overlay, and Cmd/Ctrl+F to open. It stopped short of the rest of the accelerator set and had two lifecycle leaks. This completes the surface to match the platform convention that Chrome, Safari, VS Code, and Claude Desktop's own findInPage bundle all ship. Accelerators now wired: - Cmd/Ctrl+F open the find bar (view.findInPage, from the PR) - Cmd/Ctrl+G find next (new) - Cmd/Ctrl+Shift+G find previous (new) - Enter / Shift+Enter step from the input (from the PR) - Escape close + stopFindInPage('clearSelection') (from the PR) Keyboard ownership (the substantive fix) Cmd+G was already bound to `view.toggleReview` and Escape to `composer.cancel`. The find bar's own capture-phase window listener cannot win those keys by calling stopPropagation: the keybind dispatcher's listener sits on the SAME window target in the SAME phase, and propagation control does not suppress sibling listeners on one target. Left alone, Cmd+G would step a match AND toggle the review pane, and Escape would dismiss the bar AND abort a running turn. So ownership is decided by the dispatcher, which AGENTS.md already names the single owner of combo dispatch: `findBarClaimsCombo` is consulted in use-keybinds before the registry lookup, and yields mod+g / mod+shift+g / escape to the bar only while it is open. Closing the bar hands every one of them straight back. That is the "keyboard ownership follows focus / one cancel gesture does exactly one thing" invariant. `view.findNext` / `view.findPrevious` are registered with EMPTY defaults on purpose — shipping mod+g as a second default would flag a permanent conflict in the keybinds panel against view.toggleReview. The entries document the pair and let a user bind a dedicated chord; stepping is a no-op unless the bar is open with a query, so a bound key can never search invisibly. Listener-leak fixes - The found-in-page bridge subscription is now refcounted in the store, so a remount (the connection re-home path remounts the global overlays) cannot stack duplicate subscribers that each re-dispatch the same result and outlive their component. The subscription is deliberately mount-scoped, not active-scoped: results for an in-flight search must still land if the bar just closed. - `setFindQuery` now refuses to search a closed bar. The component clears its debounce on close, but a 200ms timer that already fired would re-issue a find and re-highlight the page after the user pressed Escape. Caught by the test, fixed in the store rather than papered over in the component. - `closeFindBar` is idempotent — Escape is a shared gesture, so a second close must not reach into Electron again. Pure logic extracted for testing (no source regexing) - `src/lib/find-in-page.ts`: `formatMatchLabel` (three distinct counter states: hidden with no query, explicit 0/0, ordinal/count; clamps the ordinal and never emits NaN — Electron legitimately reports ordinal 0 on a non-final update), `findBarKeyAction` (the keybinding matcher, DOM-free), and `findBarClaimsCombo` (the ownership predicate above). Also: match counter and buttons get accessible names and the counter is aria-live, the hardcoded English "Previous"/"Next"/"Close" tooltips move to i18n (en + zh) alongside the new keybind labels, and the input gets an aria-label so the bar is reachable by role. Tests: apps/desktop/src/components/find-bar.test.tsx — 42 cases over the pure helpers, the store (open/close, next/prev dispatch shape, escape clears, refcount, double-release), and the component (focus on open, debounce coalescing, Cmd+G from outside the input, unmount releases both the bridge subscription and the window listener). cd apps/desktop && npx vitest run src/components/find-bar.test.tsx \ electron/find-in-page.test.ts -> 62 passed (42 new + 20 from the PR) Adjacent suites (src/lib/keybinds, src/i18n, src/store): 487 passed. `tsc -p tsconfig.electron.json --noEmit` clean; `tsc -p .` has 114 pre-existing errors vs 120 on the merge base (all @assistant-ui / bippy / composer), none in the touched files. eslint clean on every touched file. Co-authored-by: David Metcalfe --- apps/desktop/src/app/hooks/use-keybinds.ts | 25 +- apps/desktop/src/components/find-bar.test.tsx | 518 ++++++++++++++++++ apps/desktop/src/components/find-bar.tsx | 138 +++-- apps/desktop/src/i18n/en.ts | 7 + apps/desktop/src/i18n/types.ts | 6 + apps/desktop/src/i18n/zh.ts | 7 + apps/desktop/src/lib/find-in-page.ts | 109 ++++ apps/desktop/src/lib/keybinds/actions.ts | 11 + apps/desktop/src/store/find-in-page.ts | 92 +++- 9 files changed, 838 insertions(+), 75 deletions(-) create mode 100644 apps/desktop/src/components/find-bar.test.tsx create mode 100644 apps/desktop/src/lib/find-in-page.ts diff --git a/apps/desktop/src/app/hooks/use-keybinds.ts b/apps/desktop/src/app/hooks/use-keybinds.ts index c2c414a8b99e..8a1fe275478e 100644 --- a/apps/desktop/src/app/hooks/use-keybinds.ts +++ b/apps/desktop/src/app/hooks/use-keybinds.ts @@ -5,12 +5,18 @@ import { closeActiveTab } from '@/app/chat/close-tab' import { $terminalTakeover, setTerminalTakeover } from '@/app/right-sidebar/store' import { closeActiveTerminal, createTerminal, cycleTerminal } from '@/app/right-sidebar/terminal/terminals' import { activateTreeTabSlot, cycleTreeTabInFocusedZone, layoutHasRootSide } from '@/components/pane-shell/tree/store' +import { findBarClaimsCombo } from '@/lib/find-in-page' import { contributedKeybindHandler, PROFILE_SLOT_COUNT, SESSION_SLOT_COUNT } from '@/lib/keybinds/actions' import { comboAllowedInInput, comboFromEvent, isEditableTarget } from '@/lib/keybinds/combo' import { composerFocusKeysAllowed, isComposerFocusSoftCombo, typeToFocusChar } from '@/lib/keybinds/composer-focus-keys' import { $repoStatus } from '@/store/coding-status' import { toggleCommandPalette } from '@/store/command-palette' -import { openFindBar } from '@/store/find-in-page' +import { + $findInPage, + findNext as findNextMatch, + findPrevious as findPreviousMatch, + openFindBar +} from '@/store/find-in-page' import { $capture, $comboIndex, endCapture, setBinding } from '@/store/keybinds' import { requestSessionSearchFocus, @@ -190,6 +196,13 @@ export function useKeybinds(deps: KeybindRuntimeDeps): void { 'view.closeTab': () => void closeActiveTab(id => navigate(sessionRoute(id))), 'view.reopenTab': reopenLastClosedTile, 'view.findInPage': openFindBar, + // ⌘G / ⌘⇧G are handled by the find bar's own capture-phase listener while + // it is open (so they don't collide with `view.toggleReview`). These + // registry handlers cover a user-assigned dedicated chord: stepping is a + // no-op unless the bar is open with a query, so a bound key can't search + // invisibly. + 'view.findNext': findNextMatch, + 'view.findPrevious': findPreviousMatch, 'appearance.toggleMode': () => setMode(resolvedMode === 'dark' ? 'light' : 'dark'), @@ -245,6 +258,16 @@ export function useKeybinds(deps: KeybindRuntimeDeps): void { return } + // The open find bar owns ⌘G / ⌘⇧G / Escape. Its own capture-phase + // listener runs those actions; bail here so the registry doesn't ALSO + // fire the action bound to the same combo (⌘G = view.toggleReview, + // Escape = composer.cancel, which would abort a live turn). Both + // listeners are on `window`, so stopPropagation in the bar can't + // suppress this one — the dispatcher has to yield explicitly. + if ($findInPage.get().active && findBarClaimsCombo(combo)) { + return + } + const actionId = $comboIndex.get().get(combo) // Unbound printable → type-to-focus. Bound chords (shift+n, …) win above. diff --git a/apps/desktop/src/components/find-bar.test.tsx b/apps/desktop/src/components/find-bar.test.tsx new file mode 100644 index 000000000000..a25b41514135 --- /dev/null +++ b/apps/desktop/src/components/find-bar.test.tsx @@ -0,0 +1,518 @@ +import { act, cleanup, fireEvent, render, screen, waitFor } from '@testing-library/react' +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest' + +import { FindBar } from '@/components/find-bar' +import { I18nProvider } from '@/i18n' +import { findBarClaimsCombo, findBarKeyAction, formatMatchLabel } from '@/lib/find-in-page' +import { + $findInPage, + closeFindBar, + findInPageListenerCount, + findNext, + findPrevious, + initFindInPageListener, + openFindBar, + resetFindInPageListenerForTest, + setFindQuery, + updateFindResults +} from '@/store/find-in-page' + +// ── Bridge double ─────────────────────────────────────────────────────────── +// Stands in for the preload `hermesDesktop` surface. `onFoundInPage` records +// its subscribers so the tests can assert the listener refcount and drive +// results back into the store the way the main process would. + +interface FoundResult { + activeMatchOrdinal: number + count: number +} + +function installBridge() { + const findInPage = vi.fn().mockResolvedValue({ count: 0 }) + const stopFindInPage = vi.fn().mockResolvedValue(undefined) + const subscribers = new Set<(result: FoundResult) => void>() + + const onFoundInPage = vi.fn((callback: (result: FoundResult) => void) => { + subscribers.add(callback) + + return () => subscribers.delete(callback) + }) + + ;(window as unknown as { hermesDesktop: unknown }).hermesDesktop = { + findInPage, + stopFindInPage, + onFoundInPage + } + + return { + findInPage, + stopFindInPage, + onFoundInPage, + subscribers, + emit(result: FoundResult) { + for (const callback of [...subscribers]) { + callback(result) + } + } + } +} + +function resetStore() { + $findInPage.set({ active: false, query: '', matchOrdinal: 0, matchCount: 0 }) +} + +// Zero the bridge refcount so a leaked subscription can't bleed between tests. +function drainListeners() { + resetFindInPageListenerForTest() +} + +let bridge: ReturnType + +beforeEach(() => { + bridge = installBridge() + resetStore() +}) + +afterEach(() => { + cleanup() + resetStore() + drainListeners() + vi.restoreAllMocks() + delete (window as unknown as { hermesDesktop?: unknown }).hermesDesktop +}) + +// ── Pure: match-count formatting ──────────────────────────────────────────── + +describe('formatMatchLabel', () => { + it('hides the counter entirely when there is no query', () => { + expect(formatMatchLabel('', 0, 0)).toBe('') + // Even if stale counts linger from a previous search. + expect(formatMatchLabel('', 3, 12)).toBe('') + }) + + it('reports an explicit zero when a query matches nothing', () => { + expect(formatMatchLabel('nope', 0, 0)).toBe('0/0') + }) + + it('formats ordinal over count', () => { + expect(formatMatchLabel('hit', 3, 12)).toBe('3/12') + expect(formatMatchLabel('hit', 1, 1)).toBe('1/1') + }) + + it('shows 0 ordinal for the frame before the first match is selected', () => { + // Electron legitimately reports matches with activeMatchOrdinal 0 on the + // first (non-final) update of a fresh search. + expect(formatMatchLabel('hit', 0, 5)).toBe('0/5') + }) + + it('clamps an out-of-range ordinal into the count', () => { + expect(formatMatchLabel('hit', 99, 5)).toBe('5/5') + expect(formatMatchLabel('hit', -3, 5)).toBe('0/5') + }) + + it('never emits NaN for non-finite input', () => { + expect(formatMatchLabel('hit', Number.NaN, 4)).toBe('0/4') + expect(formatMatchLabel('hit', 2, Number.NaN)).toBe('0/0') + expect(formatMatchLabel('hit', 2, Number.POSITIVE_INFINITY)).toBe('0/0') + }) + + it('floors fractional counts rather than rendering decimals', () => { + expect(formatMatchLabel('hit', 2.7, 9.9)).toBe('2/9') + }) +}) + +// ── Pure: keybinding matcher ──────────────────────────────────────────────── + +describe('findBarKeyAction', () => { + it('maps Escape to close', () => { + expect(findBarKeyAction({ key: 'Escape' })).toBe('close') + }) + + it('maps Cmd+G / Ctrl+G to next from anywhere', () => { + expect(findBarKeyAction({ key: 'g', metaKey: true })).toBe('next') + expect(findBarKeyAction({ key: 'g', ctrlKey: true })).toBe('next') + }) + + it('maps Cmd+Shift+G / Ctrl+Shift+G to previous', () => { + // event.key is uppercase 'G' when Shift is held — matched case-insensitively. + expect(findBarKeyAction({ key: 'G', metaKey: true, shiftKey: true })).toBe('previous') + expect(findBarKeyAction({ key: 'g', ctrlKey: true, shiftKey: true })).toBe('previous') + }) + + it('steps with Enter / Shift+Enter only while focus is in the input', () => { + expect(findBarKeyAction({ key: 'Enter' }, { inInput: true })).toBe('next') + expect(findBarKeyAction({ key: 'Enter', shiftKey: true }, { inInput: true })).toBe('previous') + // Bare Enter outside the input belongs to the composer, not the find bar. + expect(findBarKeyAction({ key: 'Enter' })).toBeNull() + }) + + it('ignores a bare g so typing never triggers a step', () => { + expect(findBarKeyAction({ key: 'g' })).toBeNull() + expect(findBarKeyAction({ key: 'g' }, { inInput: true })).toBeNull() + expect(findBarKeyAction({ key: 'G', shiftKey: true }, { inInput: true })).toBeNull() + }) + + it('falls through when Alt is held so ⌥⌘G is not swallowed', () => { + expect(findBarKeyAction({ key: 'g', metaKey: true, altKey: true })).toBeNull() + expect(findBarKeyAction({ key: 'Escape', altKey: true })).toBeNull() + }) + + it('does not treat Cmd+Escape as close', () => { + expect(findBarKeyAction({ key: 'Escape', metaKey: true })).toBeNull() + }) + + it('ignores unrelated keys', () => { + expect(findBarKeyAction({ key: 'f', metaKey: true })).toBeNull() + expect(findBarKeyAction({ key: 'Tab' }, { inInput: true })).toBeNull() + }) +}) + +describe('findBarClaimsCombo', () => { + it('claims the step accelerators and Escape', () => { + expect(findBarClaimsCombo('mod+g')).toBe(true) + expect(findBarClaimsCombo('mod+shift+g')).toBe(true) + expect(findBarClaimsCombo('escape')).toBe(true) + }) + + it('leaves every other combo to the keybind registry', () => { + // ⌘F must still reach view.findInPage even while the bar is open. + expect(findBarClaimsCombo('mod+f')).toBe(false) + expect(findBarClaimsCombo('mod+k')).toBe(false) + expect(findBarClaimsCombo('mod+b')).toBe(false) + expect(findBarClaimsCombo('enter')).toBe(false) + }) +}) + +// ── Store: open/close state + dispatch ────────────────────────────────────── + +describe('find-in-page store', () => { + it('opens with a cleared query and counters', () => { + updateFindResults(3, 12) + openFindBar() + + expect($findInPage.get()).toEqual({ active: true, query: '', matchOrdinal: 0, matchCount: 0 }) + }) + + it('closing clears state and stops the native find (clears selection)', () => { + openFindBar() + setFindQuery('needle') + updateFindResults(2, 7) + + closeFindBar() + + expect($findInPage.get().active).toBe(false) + expect($findInPage.get().query).toBe('') + expect($findInPage.get().matchCount).toBe(0) + expect(bridge.stopFindInPage).toHaveBeenCalledTimes(1) + }) + + it('closing an already-closed bar does not re-issue stopFindInPage', () => { + openFindBar() + closeFindBar() + closeFindBar() + + expect(bridge.stopFindInPage).toHaveBeenCalledTimes(1) + }) + + it('a fresh query searches from scratch (findNext false)', () => { + openFindBar() + setFindQuery('needle') + + expect(bridge.findInPage).toHaveBeenCalledWith('needle', { forward: true, findNext: false }) + }) + + it('clearing the query stops the find instead of searching for empty', () => { + openFindBar() + setFindQuery('needle') + bridge.findInPage.mockClear() + + setFindQuery('') + + expect(bridge.findInPage).not.toHaveBeenCalled() + expect(bridge.stopFindInPage).toHaveBeenCalled() + expect($findInPage.get().matchCount).toBe(0) + }) + + it('findNext steps forward and findPrevious steps backward on the same query', () => { + openFindBar() + setFindQuery('needle') + bridge.findInPage.mockClear() + + findNext() + expect(bridge.findInPage).toHaveBeenLastCalledWith('needle', { forward: true, findNext: true }) + + findPrevious() + expect(bridge.findInPage).toHaveBeenLastCalledWith('needle', { forward: false, findNext: true }) + }) + + it('stepping with no query is a no-op (never searches invisibly)', () => { + openFindBar() + + findNext() + findPrevious() + + expect(bridge.findInPage).not.toHaveBeenCalled() + }) + + it('setFindQuery on a closed bar never searches', () => { + // A debounce timer that already fired, or any late caller, must not + // re-highlight the page after the user dismissed the bar. + setFindQuery('needle') + + expect(bridge.findInPage).not.toHaveBeenCalled() + expect($findInPage.get().query).toBe('') + }) + + it('found-in-page results land on the store', () => { + openFindBar() + setFindQuery('needle') + const release = initFindInPageListener() + + bridge.emit({ activeMatchOrdinal: 4, count: 9 }) + + expect($findInPage.get().matchOrdinal).toBe(4) + expect($findInPage.get().matchCount).toBe(9) + release() + }) + + it('refcounts the bridge listener so remounts cannot stack subscriptions', () => { + const first = initFindInPageListener() + const second = initFindInPageListener() + + // One real bridge subscription regardless of subscriber count. + expect(bridge.onFoundInPage).toHaveBeenCalledTimes(1) + expect(bridge.subscribers.size).toBe(1) + expect(findInPageListenerCount()).toBe(2) + + first() + // Still one holder → the bridge listener stays installed. + expect(bridge.subscribers.size).toBe(1) + + second() + // Last holder released → the listener is detached, nothing leaks. + expect(bridge.subscribers.size).toBe(0) + expect(findInPageListenerCount()).toBe(0) + }) + + it('releasing the same subscription twice cannot drive the refcount negative', () => { + const release = initFindInPageListener() + release() + release() + + expect(findInPageListenerCount()).toBe(0) + + // A fresh subscribe after a double-release still installs exactly one. + const next = initFindInPageListener() + expect(bridge.subscribers.size).toBe(1) + next() + }) +}) + +// ── Component ─────────────────────────────────────────────────────────────── + +// Store mutations that a MOUNTED FindBar subscribes to must be act()-wrapped +// so React flushes the resulting re-render inside the test. +function actStore(mutate: () => void) { + act(() => { + mutate() + }) +} + +function renderFindBar() { + return render( + + + + ) +} + +describe('FindBar', () => { + it('renders nothing while closed', () => { + renderFindBar() + + expect(screen.queryByRole('search')).toBeNull() + }) + + it('renders input, counter and close button when open with results', async () => { + openFindBar() + renderFindBar() + + const input = await screen.findByRole('textbox', { name: /find in page/i }) + expect(input).toBeTruthy() + expect(screen.getByRole('button', { name: /close/i })).toBeTruthy() + expect(screen.getByRole('button', { name: /next match/i })).toBeTruthy() + expect(screen.getByRole('button', { name: /previous match/i })).toBeTruthy() + + // Counter appears once a query + results exist. + expect(screen.queryByText('3/12')).toBeNull() + actStore(() => $findInPage.set({ active: true, query: 'needle', matchOrdinal: 3, matchCount: 12 })) + await waitFor(() => expect(screen.getByText('3/12')).toBeTruthy()) + }) + + it('focuses the input on open', async () => { + openFindBar() + renderFindBar() + + const input = await screen.findByRole('textbox', { name: /find in page/i }) + // eslint-disable-next-line no-restricted-globals -- asserting real focus requires the live document + await waitFor(() => expect(document.activeElement).toBe(input)) + }) + + it('debounces typing into a single findInPage call', async () => { + vi.useFakeTimers() + + try { + openFindBar() + renderFindBar() + + const input = screen.getByRole('textbox', { name: /find in page/i }) + fireEvent.change(input, { target: { value: 'n' } }) + fireEvent.change(input, { target: { value: 'ne' } }) + fireEvent.change(input, { target: { value: 'nee' } }) + + expect(bridge.findInPage).not.toHaveBeenCalled() + + // Flushing the debounce updates the store, which re-renders the bar. + act(() => { + vi.advanceTimersByTime(200) + }) + + expect(bridge.findInPage).toHaveBeenCalledTimes(1) + expect(bridge.findInPage).toHaveBeenCalledWith('nee', { forward: true, findNext: false }) + } finally { + vi.useRealTimers() + } + }) + + it('does not fire a pending search after the bar closes', async () => { + vi.useFakeTimers() + + try { + openFindBar() + renderFindBar() + + fireEvent.change(screen.getByRole('textbox', { name: /find in page/i }), { + target: { value: 'needle' } + }) + + actStore(closeFindBar) + vi.advanceTimersByTime(500) + + expect(bridge.findInPage).not.toHaveBeenCalled() + } finally { + vi.useRealTimers() + } + }) + + it('Enter dispatches next and Shift+Enter dispatches previous', async () => { + $findInPage.set({ active: true, query: 'needle', matchOrdinal: 1, matchCount: 4 }) + renderFindBar() + + const input = screen.getByRole('textbox', { name: /find in page/i }) + + fireEvent.keyDown(input, { key: 'Enter' }) + expect(bridge.findInPage).toHaveBeenLastCalledWith('needle', { forward: true, findNext: true }) + + fireEvent.keyDown(input, { key: 'Enter', shiftKey: true }) + expect(bridge.findInPage).toHaveBeenLastCalledWith('needle', { forward: false, findNext: true }) + }) + + it('Cmd+G / Cmd+Shift+G step from outside the input while the bar is open', async () => { + $findInPage.set({ active: true, query: 'needle', matchOrdinal: 1, matchCount: 4 }) + renderFindBar() + + // Fired on window (not the input) — the accelerator must not require focus. + fireEvent.keyDown(window, { key: 'g', metaKey: true }) + expect(bridge.findInPage).toHaveBeenLastCalledWith('needle', { forward: true, findNext: true }) + + fireEvent.keyDown(window, { key: 'G', metaKey: true, shiftKey: true }) + expect(bridge.findInPage).toHaveBeenLastCalledWith('needle', { forward: false, findNext: true }) + }) + + it('Escape closes the bar and clears the native selection', async () => { + openFindBar() + renderFindBar() + + fireEvent.keyDown(window, { key: 'Escape' }) + + expect($findInPage.get().active).toBe(false) + expect(bridge.stopFindInPage).toHaveBeenCalledTimes(1) + await waitFor(() => expect(screen.queryByRole('search')).toBeNull()) + }) + + it('the close button clears state and stops the find', async () => { + openFindBar() + renderFindBar() + + fireEvent.click(screen.getByRole('button', { name: /close/i })) + + expect($findInPage.get().active).toBe(false) + expect(bridge.stopFindInPage).toHaveBeenCalledTimes(1) + }) + + it('the next / previous buttons dispatch a step', () => { + $findInPage.set({ active: true, query: 'needle', matchOrdinal: 1, matchCount: 4 }) + renderFindBar() + + fireEvent.click(screen.getByRole('button', { name: /next match/i })) + expect(bridge.findInPage).toHaveBeenLastCalledWith('needle', { forward: true, findNext: true }) + + fireEvent.click(screen.getByRole('button', { name: /previous match/i })) + expect(bridge.findInPage).toHaveBeenLastCalledWith('needle', { forward: false, findNext: true }) + }) + + it('keeps exactly one bridge subscription across an open/close cycle', async () => { + renderFindBar() + + // Mounted-but-closed already holds the subscription: results for an + // in-flight search must still land after the bar hides. + expect(bridge.subscribers.size).toBe(1) + + actStore(openFindBar) + await waitFor(() => expect(screen.getByRole('search')).toBeTruthy()) + actStore(closeFindBar) + await waitFor(() => expect(screen.queryByRole('search')).toBeNull()) + actStore(openFindBar) + + // Toggling visibility must not stack listeners — the subscription is + // mount-scoped, not active-scoped. + expect(bridge.onFoundInPage).toHaveBeenCalledTimes(1) + expect(bridge.subscribers.size).toBe(1) + }) + + it('unmount releases the bridge subscription (no leak across route changes)', () => { + const { unmount } = renderFindBar() + expect(bridge.subscribers.size).toBe(1) + + unmount() + + expect(bridge.subscribers.size).toBe(0) + expect(findInPageListenerCount()).toBe(0) + }) + + it('a remount does not stack subscriptions', () => { + const first = renderFindBar() + first.unmount() + + const second = renderFindBar() + + expect(bridge.subscribers.size).toBe(1) + second.unmount() + expect(bridge.subscribers.size).toBe(0) + }) + + it('the window key listener is removed on unmount', () => { + openFindBar() + const { unmount } = renderFindBar() + + unmount() + bridge.stopFindInPage.mockClear() + + // Reopen the store WITHOUT a mounted bar; a leaked listener would still + // handle Escape and call into the bridge. + actStore(openFindBar) + fireEvent.keyDown(window, { key: 'Escape' }) + + expect(bridge.stopFindInPage).not.toHaveBeenCalled() + }) +}) diff --git a/apps/desktop/src/components/find-bar.tsx b/apps/desktop/src/components/find-bar.tsx index aa46bd3e4e44..20aad96e450a 100644 --- a/apps/desktop/src/components/find-bar.tsx +++ b/apps/desktop/src/components/find-bar.tsx @@ -3,6 +3,7 @@ import { useEffect, useRef, useState } from 'react' import { Tip } from '@/components/ui/tooltip' import { useI18n } from '@/i18n' +import { findBarKeyAction, formatMatchLabel } from '@/lib/find-in-page' import { cn } from '@/lib/utils' import { $findInPage, @@ -14,13 +15,21 @@ import { } from '@/store/find-in-page' /** - * Find-in-page overlay (Ctrl/Cmd+F). + * Find-in-page overlay (⌘F). * * Drives Electron's `webContents.findInPage` via the preload bridge so the - * user gets the native browser-like incremental search (highlight, Enter to - * step, Shift+Enter to step backwards, Escape to close) over the rendered - * chat transcript + editor panels. Multi-window routing is handled in the - * main process — see apps/desktop/electron/find-in-page.ts. + * user gets the native browser-like incremental search (highlight, step, + * Escape to clear) over the rendered chat transcript + editor panels. Multi- + * window routing is handled in the main process — see + * apps/desktop/electron/find-in-page.ts. + * + * Accelerators, matching the platform convention (and Claude Desktop's set): + * ⌘F opens (via the `view.findInPage` keybind), ⌘G / ⌘⇧G step next/previous + * from anywhere while the bar is open, Enter / ⇧Enter step from the input, + * and Escape closes + clears the native selection. + * + * Key routing lives in `lib/find-in-page.ts` as a pure matcher so the + * accelerator set is testable without a DOM. */ export function FindBar() { const { t } = useI18n() @@ -41,51 +50,53 @@ export function FindBar() { return undefined }, [active]) - // Subscribe to found-in-page results from the main process. - useEffect(() => { - const unsub = initFindInPageListener() - - return unsub - }, []) + // Subscribe to found-in-page results from the main process. Refcounted in + // the store, so a remount (connection re-home) can't stack listeners; the + // subscription is deliberately mount-scoped and NOT tied to `active` — + // results for an in-flight search must still land if the bar just closed. + useEffect(() => initFindInPageListener(), []) // Debounce search — fire findInPage 200ms after the user stops typing. - // The ref lets us cancel the pending timeout when the bar closes. - const debounceRef = useRef | null>(null) - useEffect(() => { if (!active || !localQuery) { return undefined } const id = setTimeout(() => setFindQuery(localQuery), 200) - debounceRef.current = id - return () => { - clearTimeout(id) - debounceRef.current = null - } + // Cleanup covers every exit: another keystroke, the bar closing, and + // unmount. Nothing can fire a find after the bar is gone. + return () => clearTimeout(id) }, [active, localQuery]) - // Cancel pending debounce + close highlights when the bar closes. - useEffect(() => { - if (!active && debounceRef.current) { - clearTimeout(debounceRef.current) - debounceRef.current = null - } - }, [active]) - - // Global Escape listener — works even when focus is outside the input. - // Captured so the find bar can always close regardless of which element - // inside the shell owns focus (composer textarea, side panel button, etc). + // Global accelerators while the bar is open: Escape closes, ⌘G / ⌘⇧G step. + // Capture-phase so they win regardless of which element inside the shell + // owns focus (composer textarea, side panel button, …). ⌘G is also bound to + // `view.toggleReview` in the keybinds registry — this listener runs in the + // capture phase and stops propagation, so while the find bar is open ⌘G + // means "find next" and the review toggle does not also fire. Closing the + // bar hands ⌘G straight back to the review pane. useEffect(() => { if (!active) { return undefined } - const onKeyDown = (e: KeyboardEvent) => { - if (e.key === 'Escape') { - e.preventDefault() + const onKeyDown = (event: KeyboardEvent) => { + const action = findBarKeyAction(event) + + if (!action) { + return + } + + event.preventDefault() + event.stopPropagation() + + if (action === 'close') { closeFindBar() + } else if (action === 'next') { + findNext() + } else { + findPrevious() } } @@ -98,37 +109,36 @@ export function FindBar() { return null } - const onInput = (e: React.ChangeEvent) => { - const val = e.target.value - setLocalQuery(val) + const onInput = (event: React.ChangeEvent) => { + const value = event.target.value + setLocalQuery(value) - // Empty query: clear highlights. - if (!val) { + // Empty query: clear highlights immediately rather than after the debounce. + if (!value) { setFindQuery('') } } - const onKeyDown = (e: React.KeyboardEvent) => { - if (e.key === 'Escape') { - e.preventDefault() - closeFindBar() - } else if (e.key === 'Enter') { - e.preventDefault() + // Enter / ⇧Enter step while focus is in the input. Escape and ⌘G are handled + // by the window listener above, so they are intentionally not duplicated + // here — `inInput` only unlocks the bare-Enter family. + const onKeyDown = (event: React.KeyboardEvent) => { + const action = findBarKeyAction(event, { inInput: true }) - if (e.shiftKey) { - findPrevious() - } else { - findNext() - } + if (action !== 'next' && action !== 'previous') { + return + } + + event.preventDefault() + + if (action === 'next') { + findNext() + } else { + findPrevious() } } - const matchLabel = - query && matchCount > 0 - ? `${matchOrdinal}/${matchCount}` - : query && matchCount === 0 - ? '0/0' - : '' + const matchLabel = formatMatchLabel(query, matchOrdinal, matchCount) return (
{matchLabel && ( - + {matchLabel} )} - +