From 5a26d800ee1e1a7024dd8a7ef9fb1c6ef19dfd73 Mon Sep 17 00:00:00 2001 From: Sergey Kozyrenko Date: Wed, 20 May 2026 18:38:49 +0700 Subject: [PATCH] feat(frontend): in-sheet search + focus-steal fix in detail navigation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `useDetailNavigation` now owns a controllable local search (debounced, ANDed with the URL `?q=` filter) and `DetailNavigationSheet` surfaces it as a header input by default (`hasSearch={true}`). Sheet auto-focus no longer steals focus from the search input when `filteredItems` shrinks on a debounce flush — gate the option-focus effect on `document.activeElement` so typing past the boundary keeps the caret in place. ArrowDown from the input synchronously promotes focus into the listbox. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../detail-navigation-buttons.tsx | 3 + .../detail-navigation-sheet.test.tsx | 188 ++++++++++++++- .../detail-navigation-sheet.tsx | 137 ++++++++++- .../detail-navigation-toolbar.tsx | 11 + .../use-detail-navigation.test.tsx | 221 ++++++++++++++++++ .../use-detail-navigation.ts | 103 +++++++- .../detail-navigation/use-navigation.test.ts | 57 +++++ .../detail-navigation/use-navigation.ts | 43 +++- 8 files changed, 748 insertions(+), 15 deletions(-) diff --git a/frontend/src/components/shared/detail-navigation/detail-navigation-buttons.tsx b/frontend/src/components/shared/detail-navigation/detail-navigation-buttons.tsx index 40bcfc70..d60451f4 100644 --- a/frontend/src/components/shared/detail-navigation/detail-navigation-buttons.tsx +++ b/frontend/src/components/shared/detail-navigation/detail-navigation-buttons.tsx @@ -47,6 +47,7 @@ export function DetailNavigationButtons({ disabled={!controller.prevId} onClick={controller.goToPrev} size="icon" + type="button" variant="outline" > @@ -64,6 +65,7 @@ export function DetailNavigationButtons({ )} disabled={!controller.hasEntries} onClick={controller.openSheet} + type="button" variant="outline" > {controller.positionLabel} @@ -79,6 +81,7 @@ export function DetailNavigationButtons({ disabled={!controller.nextId} onClick={controller.goToNext} size="icon" + type="button" variant="outline" > diff --git a/frontend/src/components/shared/detail-navigation/detail-navigation-sheet.test.tsx b/frontend/src/components/shared/detail-navigation/detail-navigation-sheet.test.tsx index 9965a92a..b0eddc50 100644 --- a/frontend/src/components/shared/detail-navigation/detail-navigation-sheet.test.tsx +++ b/frontend/src/components/shared/detail-navigation/detail-navigation-sheet.test.tsx @@ -39,8 +39,11 @@ const LocationReadout = () => { interface HarnessProps { currentId?: null | string; + defaultSearchQuery?: string; filter?: string; + hasSearch?: boolean; items?: readonly Item[]; + searchPlaceholder?: string; } /** @@ -48,19 +51,31 @@ interface HarnessProps { * focus / a11y interactions can be exercised without round-tripping through * the toolbar's position button. Keeps each test focused on the leaf. */ -const SheetHarness = ({ currentId = 'c', items = ITEMS }: HarnessProps) => { +const SheetHarness = ({ + currentId = 'c', + defaultSearchQuery, + hasSearch, + items = ITEMS, + searchPlaceholder, +}: HarnessProps) => { const nav = useDetailNavigation({ currentId, defaultOpen: true, + defaultSearchQuery, getHref, getLabel, getSearchableText, items, + // Skip the debounce so each test sees the post-typing subset on the + // next paint instead of having to wait `>150ms` per keystroke. + searchDebounceMs: 0, }); return ( controller={nav} + hasSearch={hasSearch} + searchPlaceholder={searchPlaceholder} sheetTitle="Items" /> ); @@ -239,3 +254,174 @@ describe('DetailNavigationSheet — selection', () => { }); }); }); + +describe('DetailNavigationSheet — search input', () => { + it('renders the search input by default (hasSearch=true)', async () => { + renderSheet({ currentId: 'c' }); + + const input = await screen.findByRole('textbox'); + expect(input).toBeInTheDocument(); + expect(input).toHaveValue(''); + }); + + it('hides the search input when hasSearch is false (opt-out)', async () => { + renderSheet({ currentId: 'c', hasSearch: false }); + + await screen.findByRole('listbox'); + expect(screen.queryByRole('textbox')).not.toBeInTheDocument(); + }); + + it('uses the supplied placeholder', async () => { + renderSheet({ currentId: 'c', searchPlaceholder: 'Find item' }); + + const input = await screen.findByPlaceholderText('Find item'); + expect(input).toBeInTheDocument(); + }); + + it('typing narrows the listbox immediately when searchDebounceMs=0', async () => { + const user = userEvent.setup(); + renderSheet({ currentId: 'a' }); + + const input = await screen.findByRole('textbox'); + await user.type(input, 'cha'); + + const listbox = await screen.findByRole('listbox'); + + await waitFor(() => { + const labels = within(listbox) + .getAllByRole('option') + .map((option) => option.textContent ?? ''); + expect(labels).toEqual(['Charlie']); + }); + }); + + it('renders a clear button while the query is non-empty; clicking it resets', async () => { + const user = userEvent.setup(); + renderSheet({ currentId: 'a', defaultSearchQuery: 'cha' }); + + const clearButton = await screen.findByRole('button', { name: 'Clear search' }); + await user.click(clearButton); + + const input = await screen.findByRole('textbox'); + expect(input).toHaveValue(''); + + const listbox = await screen.findByRole('listbox'); + + await waitFor(() => { + const labels = within(listbox) + .getAllByRole('option') + .map((option) => option.textContent ?? ''); + expect(labels).toEqual(['Alpha', 'Bravo', 'Charlie', 'Delta']); + }); + }); + + it('Escape clears a non-empty query without closing the sheet', async () => { + const user = userEvent.setup(); + renderSheet({ currentId: 'a', defaultSearchQuery: 'cha' }); + + const input = await screen.findByRole('textbox'); + input.focus(); + await user.keyboard('{Escape}'); + + expect(input).toHaveValue(''); + // Sheet stayed mounted because the keydown was prevented + stopped. + expect(screen.queryByRole('listbox', { name: 'Items' })).toBeInTheDocument(); + }); + + it('shows a query-specific empty state when nothing matches', async () => { + renderSheet({ currentId: 'a', defaultSearchQuery: 'zzzzz' }); + + await waitFor(() => { + expect(screen.getByText(/No items match "zzzzz"\./)).toBeInTheDocument(); + }); + expect(screen.queryByRole('listbox', { name: 'Items' })).not.toBeInTheDocument(); + }); + + it('ANDs the in-sheet search with the URL ?q= filter', async () => { + // URL filter narrows to Alpha + Bravo + Charlie (substring "a"); the + // local "bra" then narrows further to just Bravo. Use a single + // `fireEvent.change` instead of `user.type` so the assertion does + // not race with React's per-keystroke re-renders. + renderSheet({ currentId: 'b', filter: 'a' }); + + const input = await screen.findByRole('textbox'); + fireEvent.change(input, { target: { value: 'bra' } }); + + const listbox = await screen.findByRole('listbox', { name: 'Items' }); + + await waitFor(() => { + const labels = within(listbox) + .getAllByRole('option') + .map((option) => option.textContent ?? ''); + expect(labels).toEqual(['Bravo']); + }); + }); + + it('ArrowDown from the input moves roving focus into the listbox', async () => { + // Start at currentId 'b' so the initial roving focus is 'b'; that way + // we can prove ArrowDown from the *input* targeted the first item ('a') + // rather than just no-op'ing on already-focused 'a'. + renderSheet({ currentId: 'b' }); + + const input = await screen.findByRole('textbox'); + fireEvent.keyDown(input, { key: 'ArrowDown' }); + + const listbox = await screen.findByRole('listbox'); + + await waitFor(() => { + const focused = within(listbox) + .getAllByRole('option') + .find((option) => option.getAttribute('tabindex') === '0'); + expect(focused).toHaveAttribute('data-item-id', 'a'); + }); + // ArrowDown jumps DOM focus to the new option synchronously — the + // auto-focus effect skips when focus is on the search input, so this + // handler has to move focus itself. Without it, the user would have + // to click the option to interact with it after typing. + expect(document.activeElement).toHaveAttribute('data-item-id', 'a'); + }); + + it('does not yank focus onto a listbox option when filteredItems shrinks (regression)', async () => { + // Reproduces the bug from the original implementation: typing a + // query that drops the current item from the subset triggered the + // auto-focus effect to grab focus onto the new `focusedId` (the + // first survivor) at the debounce boundary, interrupting typing. + // + // `currentId='d'` + query `'rav'` → Delta is gone, subset becomes + // [Bravo], render-phase reconciliation sets `focusedId='b'`. With + // the fix the effect observes the search input is focused and + // refuses to steal focus onto the option. + // + // (The exact post-shrink `activeElement` is sensitive to Radix's + // JSDOM-only focus-trap fallback behaviour — in a real browser + // focus stays on the input, but JSDOM may bounce it onto the + // dialog container if the previously-focused option unmounts. The + // bug is specifically about the listbox option stealing focus, so + // we assert against *that*, not against equality with the input.) + renderSheet({ currentId: 'd' }); + + const input = await screen.findByRole('textbox'); + + // Radix Dialog runs its own open-time focus trap. Wait for it to + // settle, then put focus on the input as the user would after + // clicking it. + await waitFor(() => { + input.focus(); + expect(document.activeElement).toBe(input); + }); + + fireEvent.change(input, { target: { value: 'rav' } }); + + await waitFor(() => { + const labels = within(screen.getByRole('listbox')) + .getAllByRole('option') + .map((option) => option.textContent ?? ''); + expect(labels).toEqual(['Bravo']); + }); + + // The exact failure mode of the original bug: focus jumps onto a + // `role="option"` button when the list shrinks. With the fix it + // never lands there until the user explicitly arrows into the list. + expect(document.activeElement?.getAttribute('role')).not.toBe('option'); + }); +}); diff --git a/frontend/src/components/shared/detail-navigation/detail-navigation-sheet.tsx b/frontend/src/components/shared/detail-navigation/detail-navigation-sheet.tsx index fb6de70d..7e7c6236 100644 --- a/frontend/src/components/shared/detail-navigation/detail-navigation-sheet.tsx +++ b/frontend/src/components/shared/detail-navigation/detail-navigation-sheet.tsx @@ -1,6 +1,8 @@ +import { Search, X } from 'lucide-react'; import { type KeyboardEvent, type ReactNode, useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { Badge } from '@/components/ui/badge'; +import { InputGroup, InputGroupAddon, InputGroupButton, InputGroupInput } from '@/components/ui/input-group'; import { ScrollArea } from '@/components/ui/scroll-area'; import { Sheet, SheetContent, SheetHeader, SheetTitle } from '@/components/ui/sheet'; import { cn } from '@/lib/utils'; @@ -9,7 +11,15 @@ import type { DetailNavigationController } from './use-detail-navigation'; interface DetailNavigationSheetProps { controller: DetailNavigationController; + /** + * Render a free-text search input in the sheet header that drives + * `controller.setSearchQuery`. Defaults to `true` — pass `false` to opt + * out for the rare consumer that wants the sheet to stay URL-filter-only. + */ + hasSearch?: boolean; renderItem?: (item: T, isCurrent: boolean) => ReactNode; + /** Placeholder for the in-sheet search input. Defaults to "Search…". */ + searchPlaceholder?: string; sheetIcon?: ReactNode; sheetTitle: string; } @@ -27,7 +37,9 @@ interface DetailNavigationSheetProps { */ export function DetailNavigationSheet({ controller, + hasSearch = true, renderItem, + searchPlaceholder = 'Search…', sheetIcon, sheetTitle, }: DetailNavigationSheetProps) { @@ -35,6 +47,7 @@ export function DetailNavigationSheet({ // read individual fields rather than the controller object — keeps the // identity story the same as before the refactor. const { + clearSearchQuery, currentId, currentIndex, filteredItems: items, @@ -42,15 +55,20 @@ export function DetailNavigationSheet({ getLabel, handleItemSelect: onItemSelect, isSheetOpen: open, + searchQuery, + setSearchQuery, setSheetOpen: onOpenChange, total, } = controller; const listRef = useRef(null); + const searchInputRef = useRef(null); const buttonRefs = useRef(new Map()); const [focusedId, setFocusedId] = useState(null); const hasEntries = items.length > 0; + const trimmedQuery = searchQuery.trim(); + const hasClearButton = hasSearch && trimmedQuery.length > 0; // Build an `id → index` map once per `items`/`getId` change so both the // per-render membership check below and the keyboard handler's lookup @@ -126,11 +144,35 @@ export function DetailNavigationSheet({ // After roving focus moves, push the focus into the DOM. `rAF` defers past // Radix's own focus management so we don't fight its open-time focus trap. + // + // Gate: only auto-focus an option when the user's focus is already + // somewhere that *expects* roving (a list option) or has not yet landed + // (`document.activeElement === document.body` right after open, before + // Radix moves focus into the dialog). Crucially, we must NOT steal focus + // when the user is typing in the search input: every keystroke that + // flushes the debounce can re-target `focusedId` (when `currentId` falls + // out of the filtered subset, render-phase reconciliation snaps focus to + // the first survivor). Without this gate, focus jumps out of the input + // mid-type and the user can only enter a few characters before losing + // their place — exact repro: type "aes" with current flow 837 visible, + // focus moves to button 836 at the 150 ms debounce boundary. + // + // The fix is at the cause, not the symptom: a local input mirror (the + // `InputSearch` / `DataTableFilter` pattern) would keep keystrokes from + // being dropped during a state round-trip, but it would not stop this + // effect from yanking focus away. The two patterns solve different bugs. useEffect(() => { if (!open || focusedId === null) { return; } + const activeEl = document.activeElement; + const focusIsOnSearchInput = activeEl !== null && activeEl === searchInputRef.current; + + if (focusIsOnSearchInput) { + return; + } + const id = requestAnimationFrame(() => { const node = buttonRefs.current.get(focusedId); @@ -206,6 +248,61 @@ export function DetailNavigationSheet({ [onItemSelect], ); + // ArrowDown from the input jumps focus into the listbox so keyboard + // users can flow `type → arrow down → enter` without hunting for the + // list. We move focus *here* synchronously (not in the auto-focus + // effect) because that effect now skips when focus is on the search + // input — otherwise it would yank focus mid-type. Escape is *not* + // handled here — Radix listens for it on a native document handler + // that React-level `stopPropagation` cannot reach. See + // `handleEscapeKeyDown` below, wired through `onEscapeKeyDown`. + const handleSearchKeyDown = useCallback( + (event: React.KeyboardEvent) => { + if (event.key === 'ArrowDown' && hasEntries) { + event.preventDefault(); + const first = items[0]; + + if (!first) { + return; + } + + const firstId = String(getId(first)); + setFocusedId(firstId); + // Button refs for the current items are already mounted — + // this handler fires during a real user keystroke, so the + // listbox commit that wired them up has already happened. + buttonRefs.current.get(firstId)?.focus(); + } + }, + [getId, hasEntries, items], + ); + + // Intercept Esc at the Radix-Content level so we can clear a non-empty + // search before the dialog's built-in "close on Esc" fires. Once the + // query is empty, we let Radix close as usual — matches the two-step + // Esc affordance of `InputSearch` (clear, then dismiss). + const handleEscapeKeyDown = useCallback( + (event: KeyboardEvent) => { + if (hasSearch && trimmedQuery.length > 0) { + event.preventDefault(); + clearSearchQuery(); + } + }, + [clearSearchQuery, hasSearch, trimmedQuery.length], + ); + + const handleSearchChange = useCallback( + (event: React.ChangeEvent) => { + setSearchQuery(event.target.value); + }, + [setSearchQuery], + ); + + const handleSearchClear = useCallback(() => { + clearSearchQuery(); + searchInputRef.current?.focus(); + }, [clearSearchQuery]); + // One stable callback ref reused for every button. The previous shape — // `setButtonRef(id) => (node) => …` — manufactured a new closure per id // on every render, which made React re-attach refs (a `delete` + `set` @@ -241,9 +338,10 @@ export function DetailNavigationSheet({ // listbox of items, the `SheetTitle` already describes it. aria-describedby={undefined} className="flex w-full max-w-sm flex-col gap-0 p-0 sm:max-w-sm" + onEscapeKeyDown={handleEscapeKeyDown} side="right" > - + {sheetIcon} {sheetTitle} @@ -254,6 +352,39 @@ export function DetailNavigationSheet({ {total} + {hasSearch ? ( + + + + + {hasClearButton ? ( + + + + + ) : null} + + ) : null} {hasEntries ? ( @@ -301,7 +432,9 @@ export function DetailNavigationSheet({ ) : (
- No items match the current filter. + {trimmedQuery.length > 0 + ? `No items match "${trimmedQuery}".` + : 'No items match the current filter.'}
)} diff --git a/frontend/src/components/shared/detail-navigation/detail-navigation-toolbar.tsx b/frontend/src/components/shared/detail-navigation/detail-navigation-toolbar.tsx index 8514e39d..1935bd4e 100644 --- a/frontend/src/components/shared/detail-navigation/detail-navigation-toolbar.tsx +++ b/frontend/src/components/shared/detail-navigation/detail-navigation-toolbar.tsx @@ -7,7 +7,14 @@ import { DetailNavigationSheet } from './detail-navigation-sheet'; export interface DetailNavigationToolbarProps { controller: DetailNavigationController; + /** + * Forwarded to `` — controls the in-sheet search + * input. Defaults to `true`; pass `false` to opt out. + */ + hasSearch?: boolean; renderItem?: (item: T, isCurrent: boolean) => ReactNode; + /** Forwarded placeholder for the in-sheet search input. */ + searchPlaceholder?: string; sheetIcon?: ReactNode; sheetTitle: string; } @@ -24,7 +31,9 @@ export interface DetailNavigationToolbarProps { */ export function DetailNavigationToolbar({ controller, + hasSearch, renderItem, + searchPlaceholder, sheetIcon, sheetTitle, }: DetailNavigationToolbarProps) { @@ -40,7 +49,9 @@ export function DetailNavigationToolbar({ /> diff --git a/frontend/src/components/shared/detail-navigation/use-detail-navigation.test.tsx b/frontend/src/components/shared/detail-navigation/use-detail-navigation.test.tsx index 81d0657c..a9797427 100644 --- a/frontend/src/components/shared/detail-navigation/use-detail-navigation.test.tsx +++ b/frontend/src/components/shared/detail-navigation/use-detail-navigation.test.tsx @@ -443,6 +443,227 @@ describe('useDetailNavigation — current item bookkeeping', () => { }); }); +describe('useDetailNavigation — local search (uncontrolled)', () => { + it('starts with an empty searchQuery and `isSearchActive=false` by default', () => { + const { result } = renderHook( + () => + useDetailNavigation({ + currentId: 'b', + getHref, + getLabel, + getSearchableText, + items: ITEMS, + }), + { wrapper: renderInRoute(['/items/b']) }, + ); + + expect(result.current.searchQuery).toBe(''); + expect(result.current.debouncedSearchQuery).toBe(''); + expect(result.current.isSearchActive).toBe(false); + // Without a query, the full subset is visible. + expect(result.current.filteredItems.map((item) => item.id)).toEqual(['a', 'b', 'c']); + }); + + it('honours defaultSearchQuery on first render and surfaces it as active', async () => { + const { result } = renderHook( + () => + useDetailNavigation({ + currentId: 'b', + defaultSearchQuery: 'bravo', + getHref, + getLabel, + getSearchableText, + items: ITEMS, + searchDebounceMs: 0, + }), + { wrapper: renderInRoute(['/items/b']) }, + ); + + expect(result.current.searchQuery).toBe('bravo'); + + await waitFor(() => { + expect(result.current.debouncedSearchQuery).toBe('bravo'); + }); + expect(result.current.isSearchActive).toBe(true); + expect(result.current.filteredItems.map((item) => item.id)).toEqual(['b']); + }); + + it('setSearchQuery + debounce narrows filteredItems', async () => { + const { result } = renderHook( + () => + useDetailNavigation({ + currentId: 'a', + getHref, + getLabel, + getSearchableText, + items: ITEMS, + searchDebounceMs: 0, + }), + { wrapper: renderInRoute(['/items/a']) }, + ); + + act(() => { + result.current.setSearchQuery('cha'); + }); + + // searchQuery (raw) flips instantly for the input value. + expect(result.current.searchQuery).toBe('cha'); + + await waitFor(() => { + expect(result.current.filteredItems.map((item) => item.id)).toEqual(['c']); + }); + // Current id 'a' is no longer in the subset → -1 / null neighbours. + expect(result.current.currentIndex).toBe(-1); + expect(result.current.prevId).toBeNull(); + expect(result.current.nextId).toBeNull(); + }); + + it('clearSearchQuery resets the value and restores the full subset', async () => { + const { result } = renderHook( + () => + useDetailNavigation({ + currentId: 'a', + defaultSearchQuery: 'pha', + getHref, + getLabel, + getSearchableText, + items: ITEMS, + searchDebounceMs: 0, + }), + { wrapper: renderInRoute(['/items/a']) }, + ); + + await waitFor(() => { + expect(result.current.filteredItems.map((item) => item.id)).toEqual(['a']); + }); + + act(() => { + result.current.clearSearchQuery(); + }); + + expect(result.current.searchQuery).toBe(''); + + await waitFor(() => { + expect(result.current.filteredItems.map((item) => item.id)).toEqual(['a', 'b', 'c']); + }); + }); + + it('combines URL filter and local search with AND semantics', async () => { + // URL ?q=a narrows to Alpha + Bravo + Charlie. Local "bra" then keeps + // only Bravo. Without AND we'd see Alpha (matches `a`) or Bravo+Charlie + // (matches local in isolation). + const { result } = renderHook( + () => + useDetailNavigation({ + currentId: 'b', + getHref, + getLabel, + getSearchableText, + items: ITEMS, + searchDebounceMs: 0, + }), + { wrapper: renderInRoute(['/items/b?q=a']) }, + ); + + await waitFor(() => { + expect(result.current.debouncedFilter).toBe('a'); + }); + + act(() => { + result.current.setSearchQuery('bra'); + }); + + await waitFor(() => { + expect(result.current.filteredItems.map((item) => item.id)).toEqual(['b']); + }); + }); +}); + +describe('useDetailNavigation — local search (controlled)', () => { + it('respects the controlled searchQuery and fires onSearchQueryChange', () => { + const onSearchQueryChange = vi.fn(); + + const { rerender, result } = renderHook( + ({ searchQuery }: { searchQuery: string }) => + useDetailNavigation({ + currentId: 'b', + getHref, + getLabel, + getSearchableText, + items: ITEMS, + onSearchQueryChange, + searchDebounceMs: 0, + searchQuery, + }), + { + initialProps: { searchQuery: 'bravo' }, + wrapper: renderInRoute(['/items/b']), + }, + ); + + expect(result.current.searchQuery).toBe('bravo'); + + act(() => { + result.current.setSearchQuery('charlie'); + }); + + // Parent owns the value — controller does not flip without a prop change. + expect(result.current.searchQuery).toBe('bravo'); + // But the consumer is told to update. + expect(onSearchQueryChange).toHaveBeenLastCalledWith('charlie'); + + rerender({ searchQuery: 'charlie' }); + expect(result.current.searchQuery).toBe('charlie'); + }); + + it('fires onSearchQueryChange in uncontrolled mode too', () => { + const onSearchQueryChange = vi.fn(); + + const { result } = renderHook( + () => + useDetailNavigation({ + currentId: 'b', + getHref, + getLabel, + getSearchableText, + items: ITEMS, + onSearchQueryChange, + }), + { wrapper: renderInRoute(['/items/b']) }, + ); + + act(() => { + result.current.setSearchQuery('alpha'); + }); + + expect(onSearchQueryChange).toHaveBeenLastCalledWith('alpha'); + expect(result.current.searchQuery).toBe('alpha'); + }); +}); + +describe('useDetailNavigation — local search identity stability', () => { + it('keeps setSearchQuery / clearSearchQuery identity-stable across re-renders', () => { + const { rerender, result } = renderHook( + () => + useDetailNavigation({ + currentId: 'b', + getHref, + getLabel, + getSearchableText, + items: ITEMS, + }), + { wrapper: renderInRoute(['/items/b']) }, + ); + + const firstSet = result.current.setSearchQuery; + const firstClear = result.current.clearSearchQuery; + rerender(); + + expect(result.current.setSearchQuery).toBe(firstSet); + expect(result.current.clearSearchQuery).toBe(firstClear); + }); +}); + describe('useDetailNavigation — navigation back to list does not leak the filter', () => { it('keeps the URL `?q=` intact after a navigate inside the same router', async () => { const Harness = () => { diff --git a/frontend/src/components/shared/detail-navigation/use-detail-navigation.ts b/frontend/src/components/shared/detail-navigation/use-detail-navigation.ts index c6536bde..64898e85 100644 --- a/frontend/src/components/shared/detail-navigation/use-detail-navigation.ts +++ b/frontend/src/components/shared/detail-navigation/use-detail-navigation.ts @@ -1,20 +1,32 @@ import { useCallback, useMemo, useState } from 'react'; import { useNavigate, useSearchParams } from 'react-router-dom'; +import { useDebouncedValue } from '@/hooks/use-debounced-value'; import { useLatestRef } from '@/hooks/use-latest-ref'; import { useTableQueryFilterReader } from '@/hooks/use-table-query-filter'; import { mergeHrefWithSearchParams } from '@/lib/url-params'; import { useNavigation } from './use-navigation'; +/** + * Default debounce for the in-sheet local search. Shorter than the URL filter's + * 200 ms (`useTableQueryFilterReader`) — local search doesn't round-trip + * through `history` / re-render the route subtree, so we can afford to react + * faster while still coalescing burst typing. + */ +const DEFAULT_SEARCH_DEBOUNCE_MS = 150; + /** * Headless controller for a detail page that walks a filtered list. * * Owns: - * - the filtered/sorted subset (pure `computeNavigation`), + * - the filtered/sorted subset (pure `computeNavigation`), narrowed by + * the URL filter AND an optional in-controller local search, * - the resolved Prev / Next sibling ids and the pre-formatted position * label (so leaf components don't recompute), * - the sheet open state (controllable via `open` / `onOpenChange`), + * - the local search query state (controllable via `searchQuery` / + * `onSearchQueryChange`), with its own debounce independent of the URL, * - navigation actions that thread the current `?=` into every * prev / next / item-select destination. * @@ -24,6 +36,8 @@ import { useNavigation } from './use-navigation'; * controller can rely on referential equality for downstream memos. */ export interface DetailNavigationController { + /** Reset the local search to an empty string. Convenience for "X" buttons. */ + clearSearchQuery: () => void; closeSheet: () => void; /** Active `currentId` coerced to a string, or `null` when absent. */ currentId: null | string; @@ -33,6 +47,11 @@ export interface DetailNavigationController { currentItem: null | T; /** Debounced URL filter the controller is filtering against. */ debouncedFilter: string; + /** + * Debounced version of {@link searchQuery} — this is what actually narrows + * `filteredItems`. Combined with `debouncedFilter` via AND. + */ + debouncedSearchQuery: string; /** Sorted+filtered subset that drives prev / next / sheet listing. */ filteredItems: readonly T[]; /** Stable id accessor (defaults to `item.id` when not supplied). */ @@ -53,6 +72,8 @@ export interface DetailNavigationController { handleItemSelect: (item: T) => void; /** `true` iff `filteredItems.length > 0`. */ hasEntries: boolean; + /** `true` iff `debouncedSearchQuery` is non-empty (UI hints / clear button). */ + isSearchActive: boolean; isSheetOpen: boolean; /** @@ -69,6 +90,13 @@ export interface DetailNavigationController { positionLabel: string; /** ID of the previous filtered sibling, or `null` at the start / off-subset. */ prevId: null | string; + /** + * Current local search value (raw, **not** debounced) — bind directly to + * an `` for instant caret feedback. The debounced + * mirror is what filters the list, see {@link debouncedSearchQuery}. + */ + searchQuery: string; + setSearchQuery: (value: string) => void; setSheetOpen: (open: boolean) => void; /** Same as `filteredItems.length`, named explicitly for clarity. */ @@ -79,6 +107,8 @@ interface UseDetailNavigationOptions { currentId: null | string | undefined; /** Initial value for the uncontrolled case. Defaults to `false`. */ defaultOpen?: boolean; + /** Initial local search value for the uncontrolled case. Defaults to `''`. */ + defaultSearchQuery?: string; getHref: (item: T) => string; getId?: (item: T) => string; getLabel: (item: T) => string; @@ -86,6 +116,13 @@ interface UseDetailNavigationOptions { items: readonly T[]; onOpenChange?: (open: boolean) => void; + /** + * Fires on every `setSearchQuery` call — useful when the consumer wants + * to mirror the value into URL params, localStorage, or analytics. Like + * `onOpenChange`, it fires in both controlled and uncontrolled mode. + */ + onSearchQueryChange?: (query: string) => void; + /** * Controlled-mode opt-in for the sheet. When `open` is `undefined` the * controller owns the state internally; when a value is provided the @@ -96,6 +133,22 @@ interface UseDetailNavigationOptions { * `@/components/ui/autocomplete.tsx`. */ open?: boolean; + + /** + * Debounce (ms) applied between the raw `searchQuery` typed by the user + * and the `debouncedSearchQuery` that actually filters `filteredItems`. + * Defaults to {@link DEFAULT_SEARCH_DEBOUNCE_MS}. Pass `0` for instant + * filtering on small lists. + */ + searchDebounceMs?: number; + + /** + * Controlled-mode opt-in for the local search query — same `undefined` + * → uncontrolled / value → controlled convention as `open`. Use this + * when the consumer wants to surface the search outside the sheet (e.g. + * a page-level search box that also drives sibling navigation). + */ + searchQuery?: string; sortFn?: (a: T, b: T) => number; } @@ -133,13 +186,17 @@ const defaultGetId = (item: { id: string }): string => item.id; export function useDetailNavigation({ currentId, defaultOpen, + defaultSearchQuery, getHref, getId, getLabel, getSearchableText, items, onOpenChange, + onSearchQueryChange, open, + searchDebounceMs, + searchQuery, sortFn, }: UseDetailNavigationOptions): DetailNavigationController { const navigate = useNavigate(); @@ -156,12 +213,43 @@ export function useDetailNavigation({ [getSearchableText, getLabel], ); + // Controllable local search (same pattern as the sheet's `open` below). + // Uncontrolled is the common case — most consumers want the controller to + // own the value so the in-sheet input "just works". Controlled callers + // (e.g. a page that mirrors the value to its own URL param) pass + // `searchQuery` and observe via `onSearchQueryChange`. + const onSearchQueryChangeRef = useLatestRef(onSearchQueryChange); + const [internalSearchQuery, setInternalSearchQuery] = useState(defaultSearchQuery ?? ''); + const isSearchControlled = searchQuery !== undefined; + const activeSearchQuery = isSearchControlled ? searchQuery : internalSearchQuery; + const debouncedSearchQuery = useDebouncedValue(activeSearchQuery, searchDebounceMs ?? DEFAULT_SEARCH_DEBOUNCE_MS); + + const setSearchQuery = useCallback( + (next: string) => { + if (!isSearchControlled) { + setInternalSearchQuery(next); + } + + onSearchQueryChangeRef.current?.(next); + }, + [isSearchControlled, onSearchQueryChangeRef], + ); + const clearSearchQuery = useCallback(() => setSearchQuery(''), [setSearchQuery]); + + // Stabilise the query tuple so `useNavigation`'s `useMemo` only refires + // when either source actually moved. Inline `[a, b]` per render would + // defeat the memo even when both strings are unchanged. + const queryTerms = useMemo( + () => [debouncedFilter, debouncedSearchQuery] as const, + [debouncedFilter, debouncedSearchQuery], + ); + const { currentIndex, currentItem, filteredItems, nextId, prevId, total } = useNavigation({ currentId, getId: resolvedGetId, getSearchableText: resolvedGetSearchableText, items, - query: debouncedFilter, + query: queryTerms, sortFn, }); @@ -243,14 +331,17 @@ export function useDetailNavigation({ const normalizedCurrentId = currentId != null ? String(currentId) : null; const hasEntries = filteredItems.length > 0; const itemsEmpty = items.length === 0; + const isSearchActive = debouncedSearchQuery.length > 0; return useMemo>( () => ({ + clearSearchQuery, closeSheet, currentId: normalizedCurrentId, currentIndex, currentItem, debouncedFilter, + debouncedSearchQuery, filteredItems, getId: resolvedGetId, getLabel, @@ -259,21 +350,27 @@ export function useDetailNavigation({ goToPrev, handleItemSelect, hasEntries, + isSearchActive, isSheetOpen, itemsEmpty, nextId, openSheet, positionLabel, prevId, + searchQuery: activeSearchQuery, + setSearchQuery, setSheetOpen, total, }), [ + activeSearchQuery, + clearSearchQuery, closeSheet, normalizedCurrentId, currentIndex, currentItem, debouncedFilter, + debouncedSearchQuery, filteredItems, resolvedGetId, getLabel, @@ -282,12 +379,14 @@ export function useDetailNavigation({ goToPrev, handleItemSelect, hasEntries, + isSearchActive, isSheetOpen, itemsEmpty, nextId, openSheet, positionLabel, prevId, + setSearchQuery, setSheetOpen, total, ], diff --git a/frontend/src/components/shared/detail-navigation/use-navigation.test.ts b/frontend/src/components/shared/detail-navigation/use-navigation.test.ts index f666cd41..df0e520b 100644 --- a/frontend/src/components/shared/detail-navigation/use-navigation.test.ts +++ b/frontend/src/components/shared/detail-navigation/use-navigation.test.ts @@ -258,6 +258,63 @@ describe('computeNavigation', () => { expect(result.currentIndex).toBe(0); }); + it('treats an empty array query as "no filter"', () => { + const result = computeNavigation({ + currentId: 'c', + getId, + getSearchableText: getTitle, + items: ROWS, + query: [], + }); + + expect(result.filteredItems.map(getId)).toEqual(['a', 'b', 'c', 'd']); + expect(result.currentIndex).toBe(2); + }); + + it('ANDs every non-empty term when `query` is an array', () => { + // Both terms must match the haystack — substring "a" matches Alpha, + // Bravo, Charlie; substring "ph" then narrows to just Alpha. + const result = computeNavigation({ + currentId: 'a', + getId, + getSearchableText: getTitle, + items: ROWS, + query: ['a', 'ph'], + }); + + expect(result.filteredItems.map(getId)).toEqual(['a']); + expect(result.currentIndex).toBe(0); + expect(result.prevId).toBeNull(); + expect(result.nextId).toBeNull(); + }); + + it('drops empty strings from an array query without throwing', () => { + // Callers can pass `[urlFilter, localSearch]` straight through; either + // one being empty must not turn the whole filter off — the other term + // is still expected to narrow. + const result = computeNavigation({ + currentId: 'b', + getId, + getSearchableText: getTitle, + items: ROWS, + query: ['', 'bravo', ''], + }); + + expect(result.filteredItems.map(getId)).toEqual(['b']); + }); + + it('falls back to "no filter" when every array term is empty', () => { + const result = computeNavigation({ + currentId: 'a', + getId, + getSearchableText: getTitle, + items: ROWS, + query: ['', '', ''], + }); + + expect(result.filteredItems.map(getId)).toEqual(['a', 'b', 'c', 'd']); + }); + it('matches numeric item ids with string currentId', () => { type NumRow = { id: number; title: string }; const rows: readonly NumRow[] = [ diff --git a/frontend/src/components/shared/detail-navigation/use-navigation.ts b/frontend/src/components/shared/detail-navigation/use-navigation.ts index fe455012..67541ff3 100644 --- a/frontend/src/components/shared/detail-navigation/use-navigation.ts +++ b/frontend/src/components/shared/detail-navigation/use-navigation.ts @@ -12,12 +12,17 @@ interface NavigationInput { getSearchableText?: (item: T) => null | string | undefined; items: readonly T[]; /** - * Free-text filter to narrow the navigable subset. Empty / `undefined` is - * treated as "no filter" and every item passes. Matching reuses - * `createTextMatcher`, so behaviour matches the list page's column - * filter (substring, case + diacritic insensitive). + * Free-text filter(s) to narrow the navigable subset. Accepts: + * - `undefined` / empty string / empty array → no filter, every item passes; + * - a single string → substring match (case + diacritic insensitive); + * - a `readonly string[]` → AND of every non-empty entry. Empty strings + * inside the array are ignored, so callers can pass + * `[urlFilter, localSearch]` without checking emptiness at the call site. + * + * Matching reuses `createTextMatcher`, so behaviour stays in lockstep with + * the list page's column filter regardless of how many sources combine here. */ - query?: string; + query?: readonly string[] | string; sortFn?: (a: T, b: T) => number; } @@ -48,10 +53,24 @@ export const computeNavigation = ({ query, sortFn, }: NavigationInput): NavigationResult => { - const hasQuery = query !== undefined && query.length > 0; + // Normalise `string | string[] | undefined` to a list of non-empty terms. + // Empties are dropped here once so the hot `items.filter` below never + // re-checks them — and callers can splice `[urlFilter, localSearch]` + // straight through without short-circuiting at the call site. + const queryTerms = query === undefined ? [] : Array.isArray(query) ? query : [query as string]; + const activeTerms = queryTerms.filter((term): term is string => term.length > 0); + const hasQuery = activeTerms.length > 0; + // Build each matcher once at the top of the filter pass — `createTextMatcher` + // normalises the term, so doing it inside the per-item loop would burn + // O(items × terms) on the same work. + const matchers = hasQuery && getSearchableText ? activeTerms.map(createTextMatcher) : null; const filtered = - hasQuery && getSearchableText - ? items.filter((item) => createTextMatcher(query)(getSearchableText(item))) + matchers && getSearchableText + ? items.filter((item) => { + const text = getSearchableText(item); + + return matchers.every((match) => match(text)); + }) : items; const ordered = sortFn ? [...filtered].sort(sortFn) : filtered; @@ -98,8 +117,12 @@ interface UseNavigationOptions { */ getSearchableText?: (item: T) => null | string | undefined; items: readonly T[]; - /** Free-text filter. Empty / `undefined` → unfiltered. */ - query?: string; + /** + * Free-text filter. Accepts a single string or an array (AND of every + * non-empty entry). Empty / `undefined` / `[]` → unfiltered. See + * {@link NavigationInput.query} for details. + */ + query?: readonly string[] | string; /** * Optional comparator. When omitted, the input array order is preserved — * the navigation walks `items` exactly as the caller arranged them, which