diff --git a/frontend/src/components/ui/data-table.test.tsx b/frontend/src/components/ui/data-table.test.tsx index 8dd2c6ce..f1be2b37 100644 --- a/frontend/src/components/ui/data-table.test.tsx +++ b/frontend/src/components/ui/data-table.test.tsx @@ -137,46 +137,60 @@ describe('DataTable — controlled filter projection', () => { expect(onFilterChange).toHaveBeenCalledWith(''); }); + // The next four tests exercise every race window the inline-arrow + + // useDebouncedValue + two-effects design used to leak through. They + // share a `Host` shape — a parent that mirrors the controlled value to + // local state and hands `DataTableFilter` a fresh inline `onFilterChange` + // per render (the same pattern real callers use through + // `useTableQueryFilter`). The tests assert on the sequence of emitted + // values rather than on internal state, so they remain valid even if + // the underlying implementation changes shape again. + + interface FilterHostProps { + emitted: string[]; + initialValue?: string; + onSetValueRef?: (setter: (next: string) => void) => void; + } + + const FilterHost = ({ emitted, initialValue = '', onSetValueRef }: FilterHostProps) => { + const [value, setValue] = useState(initialValue); + + // Expose `setValue` for tests that need to mutate the controlled + // prop without typing into the input (simulating an external source + // like back-button or sibling control). + if (onSetValueRef) { + onSetValueRef(setValue); + } + + return ( + + columns={COLUMNS} + data={ROWS} + filterColumn="name" + filterPlaceholder="Filter..." + filterValue={value} + onFilterChange={(next) => { + emitted.push(next); + setValue(next); + }} + /> + ); + }; + it('does not resurrect the previous query after the X clears past a settled debounce', async () => { - // Regression for the X-clear race: parents typically funnel - // `onFilterChange` through an inline arrow (`(v) => setUrl(v)`), which - // is a fresh reference per render. The emit effect inside the filter - // input used to list that handler in its deps; combined with - // `useDebouncedValue` (a `useState` that lags behind `localValue` by - // a `setTimeout`), the very next parent render after `handleClear` - // saw `debouncedValue='alpha'` while `lastEmitted=''` and re-emitted - // `'alpha'` — the user saw the URL/value snap back briefly. + // Regression for the X-clear race seen in the browser: + // typed → debounce flushes → URL=?q=alpha → X click. The old design + // re-emitted 'alpha' on the next parent render (new inline handler, + // stale `debouncedValue`). With the new single-timer path, X clear + // cancels any in-flight emit synchronously. const emitted: string[] = []; const user = userEvent.setup(); - const Host = () => { - const [value, setValue] = useState(''); - - return ( - - columns={COLUMNS} - data={ROWS} - filterColumn="name" - filterPlaceholder="Filter..." - filterValue={value} - // Intentionally a fresh inline arrow per render — - // mirrors how pages wire this up. - onFilterChange={(next) => { - emitted.push(next); - setValue(next); - }} - /> - ); - }; - - render(, { wrapper: Wrapper }); + render(, { wrapper: Wrapper }); const input = screen.getByPlaceholderText('Filter...'); await user.type(input, 'alpha'); - // Wait until the debounce has flushed and the parent has committed - // 'alpha' upstream. From here the bug needs `debouncedValue` to be - // stuck at the old value across the clear+next-render boundary. await waitFor(() => expect(emitted.at(-1)).toBe('alpha')); const inputGroup = input.closest('[data-slot="input-group"]'); @@ -184,9 +198,6 @@ describe('DataTable — controlled filter projection', () => { await user.click(clearButton); - // Give the debounce timer and any follow-up effects plenty of time - // to misfire. With the fix in place no further 'alpha' emission - // should appear; only the trailing '' clear is expected. await new Promise((resolve) => setTimeout(resolve, 400)); const after = emitted.slice(emitted.indexOf('alpha') + 1); @@ -194,6 +205,94 @@ describe('DataTable — controlled filter projection', () => { expect(after.at(-1)).toBe(''); expect((input as HTMLInputElement).value).toBe(''); }); + + it('drops a pending typed emit when the X is clicked before the debounce flushes', async () => { + // The pre-refactor code relied on `useDebouncedValue`'s internal + // setTimeout, which `handleClear` had no way to cancel. So if a user + // typed quickly and immediately clicked X, the still-pending timer + // would land *after* the clear, re-emitting the typed value. The + // refactor cancels the pending timer inside `handleClear` → `emit`. + const emitted: string[] = []; + const user = userEvent.setup(); + + render(, { wrapper: Wrapper }); + + const input = screen.getByPlaceholderText('Filter...'); + await user.type(input, 'bravo', { delay: 5 }); + // No `waitFor` here — we want the X click to happen mid-debounce, + // before any emit has reached the parent. + expect(emitted).toEqual([]); + + const inputGroup = input.closest('[data-slot="input-group"]'); + const clearButton = within(inputGroup as HTMLElement).getByRole('button'); + + await user.click(clearButton); + + // Wait long enough for any stale timer to have fired by now. + await new Promise((resolve) => setTimeout(resolve, 400)); + + // The clear may emit '' (if the parent's filterValue was previously + // non-empty — irrelevant here, but harmless), but it must never + // emit any partial of 'bravo'. + expect(emitted.some((value) => value.length > 0)).toBe(false); + expect((input as HTMLInputElement).value).toBe(''); + }); + + it('lets an external change win when it lands mid-debounce', async () => { + // If the parent flips `filterValue` while we're still holding a + // pending debounce of locally-typed text, the external value wins + // and the pending emit is dropped. Without this guarantee, the + // pending timer would later fire and clobber the external value. + const emitted: string[] = []; + const user = userEvent.setup(); + let setValueExternal: ((next: string) => void) | undefined; + + render( + { + setValueExternal = setter; + }} + />, + { wrapper: Wrapper }, + ); + + const input = screen.getByPlaceholderText('Filter...'); + await user.type(input, 'cha', { delay: 5 }); + + // While the debounce is still pending, simulate an external write + // (e.g. back-button popping to a different `?q=`). + expect(setValueExternal).toBeDefined(); + act(() => setValueExternal!('external')); + + // Give any leftover timer enough time to fire. + await new Promise((resolve) => setTimeout(resolve, 400)); + + // The pending typed value must never have escaped upstream. + expect(emitted).not.toContain('cha'); + // The input snaps to the external value. + expect((input as HTMLInputElement).value).toBe('external'); + }); + + it('honours typed input when it eventually matches an unrelated external value', async () => { + // A subtler invariant: if the user types something that happens to + // equal a value the parent previously set externally, the typed + // emit must still fire — we shouldn't dedupe against a value the + // parent already knows about, only against the value we last sent. + const emitted: string[] = []; + const user = userEvent.setup(); + + render(, { wrapper: Wrapper }); + + const input = screen.getByPlaceholderText('Filter...'); + // Clear what's in the input and type the same string fresh. + await user.clear(input); + await waitFor(() => expect(emitted.at(-1)).toBe('')); + await user.type(input, 'seed'); + + await waitFor(() => expect(emitted.at(-1)).toBe('seed')); + expect((input as HTMLInputElement).value).toBe('seed'); + }); }); describe('DataTable — uncontrolled filter is still routed through the same input', () => { diff --git a/frontend/src/components/ui/data-table.tsx b/frontend/src/components/ui/data-table.tsx index d8256306..271e6424 100644 --- a/frontend/src/components/ui/data-table.tsx +++ b/frontend/src/components/ui/data-table.tsx @@ -28,6 +28,7 @@ import { X, } from 'lucide-react'; import { + type ChangeEvent, Fragment, type ReactElement, type ReactNode, @@ -53,7 +54,6 @@ import { Empty, EmptyDescription, EmptyHeader, EmptyMedia, EmptyTitle } from '@/ import { InputGroup, InputGroupAddon, InputGroupButton, InputGroupInput } from '@/components/ui/input-group'; import { Select, SelectContent, SelectItem, SelectTrigger, SelectValue } from '@/components/ui/select'; import { Table, TableBody, TableCell, TableHead, TableHeader, TableRow } from '@/components/ui/table'; -import { useDebouncedValue } from '@/hooks/use-debounced-value'; import { useEffectAfterMount } from '@/hooks/use-effect-after-mount'; import { useLatestRef } from '@/hooks/use-latest-ref'; import { usePageStorageKeys } from '@/hooks/use-page-storage-keys'; @@ -934,35 +934,40 @@ function DataTableColumnHeader({ column, title }: DataT } /** - * Search input for the table's global filter. Keystrokes update an internal - * `localValue` state synchronously so the input feels instant; the debounced - * mirror is what we propagate upstream via `onQueryChange`. The previous - * design committed every keystroke straight into `useTableQueryFilter`, which - * synchronously walked the router and re-rendered the entire route subtree — - * with the Flows page that ran ~250 ms per keystroke, so consecutive - * keypresses queued behind the in-flight reconciliation and showed up as - * input-delay INP. Debouncing the commit drops the upstream cascade rate - * from per-keystroke to once per typing pause. + * Search input for the table's global filter. Two storage cells, one + * outbound path: * - * External `query` changes (X button, programmatic clear, URL back-button, - * route swap) are reconciled into `localValue` through the sync effect: we - * skip when the incoming value matches what we last emitted, so our own - * round-trip through the parent doesn't fight with active typing. + * 1. `localValue` — `useState`, drives the controlled input. Updated + * synchronously on every keystroke so the caret never lags. + * 2. `pendingTimerRef` — the *only* pending emit. `handleChange` schedules + * it; everything else (`handleClear`, external `query` change, unmount) + * cancels it synchronously before doing its own thing. + * + * The earlier design layered `useDebouncedValue` (its own delayed `useState`) + * plus a `lastEmittedReference` ref and two opposing `useEffect`s on top of + * the same value. That introduced a third storage cell (`debouncedValue`) + * which lagged behind `localValue` by a `setTimeout` and could be read out + * of phase: a synchronous clear ran while `debouncedValue` still held the + * pre-clear text, so the emit effect re-fired and resurrected it. Moving + * the debounce into an imperative timer eliminates that hidden cell — + * there is no "stale debounced value" to read from any effect. + * + * Why we debounce at all: previously every keystroke synchronously walked + * `useTableQueryFilter` → router → route subtree, ~250 ms per keystroke on + * Flows. The debounce drops upstream commits to once per typing pause. */ function DataTableFilter({ onQueryChange, placeholder, query }: DataTableFilterProps) { const [localValue, setLocalValue] = useState(query); - const debouncedValue = useDebouncedValue(localValue, FILTER_DEBOUNCE_MS); + // Tracks the last value we successfully emitted upstream so the + // external-sync effect can tell "the parent confirmed our own commit" + // (skip) from "the parent changed `query` independently — back button, + // shared link, sibling control" (accept and override local edits). + // Mutated only inside `emit`, so the invariant is local to one function. const lastEmittedReference = useRef(query); - // Stash the handler in a ref so the emit effect can depend on - // `debouncedValue` alone. Parents typically pass an inline arrow - // (`onQueryChange={(v) => table.setGlobalFilter(v)}`), which is a fresh - // reference each render. If the effect listed it in deps, the X-button - // clear race resurrected the previous query: handleClear sets - // `lastEmitted=''` synchronously, but `debouncedValue` is `useState` - // inside `useDebouncedValue` and stays at the old value until its - // own setTimeout fires — so the very next parent render (with a new - // inline `onQueryChange`) triggered the effect, saw - // `debouncedValue='jwt' !== lastEmitted=''`, and re-emitted `'jwt'`. + const pendingTimerReference = useRef(null); + // The handler from the parent is typically an inline arrow, so its + // identity churns every render. We never want effect deps to depend on + // that — read the latest one through this ref instead. const onQueryChangeReference = useLatestRef(onQueryChange); // Generated per-instance so pages with multiple DataTables (e.g. // /settings/prompts) don't end up with duplicate `id` attributes — that @@ -970,25 +975,69 @@ function DataTableFilter({ onQueryChange, placeholder, query }: DataTableFilterP // relies on the input id. const fieldId = useId(); - useEffect(() => { - if (query !== lastEmittedReference.current) { - setLocalValue(query); - lastEmittedReference.current = query; + const cancelPendingEmit = useCallback(() => { + if (pendingTimerReference.current !== null) { + window.clearTimeout(pendingTimerReference.current); + pendingTimerReference.current = null; } - }, [query]); + }, []); + // Single outbound path. Always cancels any pending debounce so the + // caller's value wins; skips no-ops where the parent already holds the + // same string. Records what went out so external-sync can recognise + // the round-trip. + const emit = useCallback( + (next: string) => { + cancelPendingEmit(); + + if (next === lastEmittedReference.current) { + return; + } + + lastEmittedReference.current = next; + onQueryChangeReference.current(next); + }, + [cancelPendingEmit, onQueryChangeReference], + ); + + // External → local sync. Runs only when `query` is something we didn't + // emit ourselves — that's the signal an outside source (back button, + // sibling control, programmatic clear) moved the source of truth out + // from under us. Any in-flight debounce of locally-typed text is dropped + // synchronously; the external value wins, by design. useEffect(() => { - if (debouncedValue !== lastEmittedReference.current) { - lastEmittedReference.current = debouncedValue; - onQueryChangeReference.current(debouncedValue); + if (query === lastEmittedReference.current) { + return; } - }, [debouncedValue, onQueryChangeReference]); + + cancelPendingEmit(); + lastEmittedReference.current = query; + setLocalValue(query); + }, [query, cancelPendingEmit]); + + // Cancel any pending timer on unmount so it can't fire into a dead + // component (React would warn, and the upstream parent might still + // accept the emit). + useEffect(() => cancelPendingEmit, [cancelPendingEmit]); + + const handleChange = useCallback( + (event: ChangeEvent) => { + const next = event.target.value; + + setLocalValue(next); + cancelPendingEmit(); + pendingTimerReference.current = window.setTimeout(() => { + pendingTimerReference.current = null; + emit(next); + }, FILTER_DEBOUNCE_MS); + }, + [cancelPendingEmit, emit], + ); const handleClear = useCallback(() => { setLocalValue(''); - lastEmittedReference.current = ''; - onQueryChangeReference.current(''); - }, [onQueryChangeReference]); + emit(''); + }, [emit]); return ( @@ -998,7 +1047,7 @@ function DataTableFilter({ onQueryChange, placeholder, query }: DataTableFilterP id={fieldId} maxLength={FILTER_MAX_LENGTH} name={fieldId} - onChange={(event) => setLocalValue(event.target.value)} + onChange={handleChange} placeholder={placeholder} type="text" value={localValue} diff --git a/frontend/src/components/ui/input-search.tsx b/frontend/src/components/ui/input-search.tsx index 0c5e56c2..9424b845 100644 --- a/frontend/src/components/ui/input-search.tsx +++ b/frontend/src/components/ui/input-search.tsx @@ -1,20 +1,18 @@ import { Search } from 'lucide-react'; import { motion } from 'motion/react'; -import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; +import { type ChangeEvent, useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { InputGroup, InputGroupAddon, InputGroupButton, InputGroupInput } from '@/components/ui/input-group'; import { Kbd, KbdGroup } from '@/components/ui/kbd'; import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip'; -import { useDebouncedValue } from '@/hooks/use-debounced-value'; import { useLatestRef } from '@/hooks/use-latest-ref'; import { cn } from '@/lib/utils'; import { isMac } from '@/lib/utils/platform'; -// Burst-typing protection: keystrokes hit `localValue` synchronously for an -// instant UI response; the debounced mirror is what we propagate upstream -// (which may walk the router and re-render the whole page subtree). Mirrors -// `FILTER_DEBOUNCE_MS` in `data-table.tsx` so both search inputs on a page -// share a single "feels responsive" target. +// Keystrokes hit `localValue` synchronously for an instant UI response; the +// outbound emit goes through a timer ref (see `pendingTimerReference`). Same +// shape as `DataTableFilter`; mirrors `FILTER_DEBOUNCE_MS` so both search +// inputs on a page share one "feels responsive" target. const COMMIT_DEBOUNCE_MS = 150; interface InputSearchProps { @@ -68,48 +66,54 @@ export function InputSearch({ const [isExpanded, setIsExpanded] = useState(() => searchQuery.trim().length > 0); const inputRef = useRef(null); - // Local mirror of the controlled `searchQuery`. Keystrokes set this - // synchronously so the caret never lags; the debounced shadow below is - // what we hand back to the parent. Mirrors `DataTableFilter`'s approach - // — without it, parents that funnel `onSearchChange` through the router - // (URL → re-render → controlled value snaps back) can drop characters - // when typing faster than the round-trip. + // Same two-cell shape as `DataTableFilter`: `localValue` drives the input + // (synchronous, instant caret response); `pendingTimerReference` is the + // only outbound emit in flight. `lastEmittedReference` records the most + // recent value we've handed upstream so the external-sync effect can + // distinguish our own round-trip from a true external change. const [localValue, setLocalValue] = useState(searchQuery); - const debouncedLocalValue = useDebouncedValue(localValue, COMMIT_DEBOUNCE_MS); - // Tracks the last value we either emitted upstream or accepted from the - // parent, so the two sync effects below can distinguish "our own - // round-trip arrived" from "the parent reset me from elsewhere" without - // a race. const lastEmittedReference = useRef(searchQuery); - // Mirror onto a ref so the emit effect can depend on `debouncedLocalValue` - // alone. Parents commonly pass an inline arrow; listing it in deps would - // re-run the emit effect on every parent render with a stale - // `debouncedLocalValue` (still holding the pre-clear value), letting a - // synchronous Esc / programmatic clear race the debounce timer and - // resurrect the previous query. See same fix in `DataTableFilter`. + const pendingTimerReference = useRef(null); const onSearchChangeReference = useLatestRef(onSearchChange); - // External → local sync. The parent owns the source of truth (URL, - // upstream state, etc.); when *they* change it (Esc clear, programmatic - // wipe of `?qs=`, back button), reset the local mirror. Skip when the - // incoming value matches what we last emitted — that's our own commit - // coming home and overwriting in-flight typing would lose characters. - useEffect(() => { - if (searchQuery !== lastEmittedReference.current) { - setLocalValue(searchQuery); - lastEmittedReference.current = searchQuery; + const cancelPendingEmit = useCallback(() => { + if (pendingTimerReference.current !== null) { + window.clearTimeout(pendingTimerReference.current); + pendingTimerReference.current = null; } - }, [searchQuery]); + }, []); - // Local → external emit, debounced. We only call `onSearchChange` when - // the debounced value differs from what's already upstream — otherwise - // every external reset would round-trip back through this effect. + const emit = useCallback( + (next: string) => { + cancelPendingEmit(); + + if (next === lastEmittedReference.current) { + return; + } + + lastEmittedReference.current = next; + onSearchChangeReference.current(next); + }, + [cancelPendingEmit, onSearchChangeReference], + ); + + // External → local sync. Runs only when `searchQuery` is something we + // didn't emit ourselves — programmatic wipe of `?qs=`, back button, + // sibling control. Any in-flight debounce of locally-typed text is + // dropped synchronously; the external value wins, by design. useEffect(() => { - if (debouncedLocalValue !== lastEmittedReference.current) { - lastEmittedReference.current = debouncedLocalValue; - onSearchChangeReference.current(debouncedLocalValue); + if (searchQuery === lastEmittedReference.current) { + return; } - }, [debouncedLocalValue, onSearchChangeReference]); + + cancelPendingEmit(); + lastEmittedReference.current = searchQuery; + setLocalValue(searchQuery); + }, [searchQuery, cancelPendingEmit]); + + // Cancel any pending timer on unmount so it can't fire into a dead + // component. + useEffect(() => cancelPendingEmit, [cancelPendingEmit]); const expand = useCallback(() => setIsExpanded(true), []); @@ -204,10 +208,24 @@ export function InputSearch({ } }, [searchQuery]); + const handleChange = useCallback( + (event: ChangeEvent) => { + const next = event.target.value; + + setLocalValue(next); + cancelPendingEmit(); + pendingTimerReference.current = window.setTimeout(() => { + pendingTimerReference.current = null; + emit(next); + }, COMMIT_DEBOUNCE_MS); + }, + [cancelPendingEmit, emit], + ); + // Escape: first press clears the value, second press collapses + blurs. - // Decision and emit both run against `localValue` so the keypress feels - // instant: clear is unconditional and side-steps the debounce, going - // straight to the parent without waiting on the next tick. + // Clear goes through `emit('')`, which cancels any pending debounce + // synchronously — no chance for an in-flight typed value to land after + // the Esc. const handleInputKeyDown = useCallback( (event: React.KeyboardEvent) => { if (event.key !== 'Escape') { @@ -218,8 +236,7 @@ export function InputSearch({ if (localValue.length > 0) { setLocalValue(''); - lastEmittedReference.current = ''; - onSearchChangeReference.current(''); + emit(''); return; } @@ -227,7 +244,7 @@ export function InputSearch({ inputRef.current?.blur(); setIsExpanded(false); }, - [localValue, onSearchChangeReference], + [emit, localValue], ); return ( @@ -292,7 +309,7 @@ export function InputSearch({ // bottoming out on the implicit `min-content` width. className="h-8 min-w-0 py-0 pl-2" onBlur={handleInputBlur} - onChange={(event) => setLocalValue(event.target.value)} + onChange={handleChange} onKeyDown={handleInputKeyDown} placeholder={placeholder} ref={inputRef}