diff --git a/frontend/src/components/ui/data-table.tsx b/frontend/src/components/ui/data-table.tsx index 271e6424..a09170c4 100644 --- a/frontend/src/components/ui/data-table.tsx +++ b/frontend/src/components/ui/data-table.tsx @@ -54,6 +54,7 @@ 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 { useDebouncedCallback } from '@/hooks/use-debounced-callback'; import { useEffectAfterMount } from '@/hooks/use-effect-after-mount'; import { useLatestRef } from '@/hooks/use-latest-ref'; import { usePageStorageKeys } from '@/hooks/use-page-storage-keys'; @@ -934,110 +935,81 @@ function DataTableColumnHeader({ column, title }: DataT } /** - * Search input for the table's global filter. Two storage cells, one - * outbound path: + * Search input for the table's global filter. * - * 1. `localValue` — `useState`, drives the controlled input. Updated + * Controlled debounced input — the kind the React docs warn you cannot + * model with `useDeferredValue` alone, because we want to throttle the + * *side-effect* (URL writes via the parent's `onQueryChange`) rather than + * the render of the filtered rows (`useDeferredValue` already handles the + * latter at the table level — see `deferredGlobalFilter` above). React's + * own recommendation for this case is plain debouncing. + * + * The whole component is two state cells and one effect: + * + * 1. `localValue` (`useState`) — drives the controlled input, updates * 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. + * 2. `lastEmittedReference` (`useRef`) — records the most recent value + * we've handed upstream so the external-sync effect can distinguish + * "this is our own commit coming back through the router" (skip) + * from "something outside changed the source of truth" (accept, + * override any in-flight typing). + * 3. One `useEffect` reconciling external `query` with local state. * - * 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. + * Everything timer-related lives inside `useDebouncedCallback`, which + * exposes a single `.cancel()` we call from the only two places that need + * to override the schedule — external sync and `handleClear`. There is no + * stale `debouncedValue` cell for an effect to read out of phase, which + * was the entire class of races the previous design carried. */ function DataTableFilter({ onQueryChange, placeholder, query }: DataTableFilterProps) { const [localValue, setLocalValue] = useState(query); - // 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); - 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 // breaks `getElementById`, a11y semantics, and any test selector that // relies on the input id. const fieldId = useId(); - const cancelPendingEmit = useCallback(() => { - if (pendingTimerReference.current !== null) { - window.clearTimeout(pendingTimerReference.current); - pendingTimerReference.current = null; + const debouncedEmit = useDebouncedCallback((next: string) => { + if (next === lastEmittedReference.current) { + return; } - }, []); - // 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], - ); + lastEmittedReference.current = next; + onQueryChange(next); + }, FILTER_DEBOUNCE_MS); // 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 + // emit ourselves: back button, programmatic clear, sibling control. + // Any in-flight debounce of locally-typed text is cancelled // synchronously; the external value wins, by design. useEffect(() => { if (query === lastEmittedReference.current) { return; } - cancelPendingEmit(); + debouncedEmit.cancel(); 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]); + }, [query, debouncedEmit]); 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); + setLocalValue(event.target.value); + debouncedEmit(event.target.value); }, - [cancelPendingEmit, emit], + [debouncedEmit], ); const handleClear = useCallback(() => { setLocalValue(''); - emit(''); - }, [emit]); + debouncedEmit.cancel(); + + if (lastEmittedReference.current !== '') { + lastEmittedReference.current = ''; + onQueryChange(''); + } + }, [debouncedEmit, onQueryChange]); return ( diff --git a/frontend/src/components/ui/input-search.tsx b/frontend/src/components/ui/input-search.tsx index 9424b845..a1b2701d 100644 --- a/frontend/src/components/ui/input-search.tsx +++ b/frontend/src/components/ui/input-search.tsx @@ -5,14 +5,14 @@ import { type ChangeEvent, useCallback, useEffect, useMemo, useRef, useState } f 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 { useLatestRef } from '@/hooks/use-latest-ref'; +import { useDebouncedCallback } from '@/hooks/use-debounced-callback'; import { cn } from '@/lib/utils'; import { isMac } from '@/lib/utils/platform'; // 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. +// upstream emit is debounced via `useDebouncedCallback` (see below). Same +// shape and timing as `DataTableFilter` so both search inputs on a page +// share one "feels responsive" target. const COMMIT_DEBOUNCE_MS = 150; interface InputSearchProps { @@ -67,53 +67,36 @@ export function InputSearch({ const inputRef = useRef(null); // 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. + // (synchronous, instant caret response); `lastEmittedReference` records + // the most recent value we handed upstream so the external-sync effect + // can distinguish our own round-trip from a true external change. All + // timer logic lives inside `useDebouncedCallback` — see its docstring + // for the rationale. const [localValue, setLocalValue] = useState(searchQuery); const lastEmittedReference = useRef(searchQuery); - const pendingTimerReference = useRef(null); - const onSearchChangeReference = useLatestRef(onSearchChange); - const cancelPendingEmit = useCallback(() => { - if (pendingTimerReference.current !== null) { - window.clearTimeout(pendingTimerReference.current); - pendingTimerReference.current = null; + const debouncedEmit = useDebouncedCallback((next: string) => { + if (next === lastEmittedReference.current) { + return; } - }, []); - const emit = useCallback( - (next: string) => { - cancelPendingEmit(); - - if (next === lastEmittedReference.current) { - return; - } - - lastEmittedReference.current = next; - onSearchChangeReference.current(next); - }, - [cancelPendingEmit, onSearchChangeReference], - ); + lastEmittedReference.current = next; + onSearchChange(next); + }, COMMIT_DEBOUNCE_MS); // 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. + // cancelled synchronously; the external value wins, by design. useEffect(() => { if (searchQuery === lastEmittedReference.current) { return; } - cancelPendingEmit(); + debouncedEmit.cancel(); 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]); + }, [searchQuery, debouncedEmit]); const expand = useCallback(() => setIsExpanded(true), []); @@ -210,22 +193,15 @@ export function InputSearch({ 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); + setLocalValue(event.target.value); + debouncedEmit(event.target.value); }, - [cancelPendingEmit, emit], + [debouncedEmit], ); // Escape: first press clears the value, second press collapses + blurs. - // Clear goes through `emit('')`, which cancels any pending debounce - // synchronously — no chance for an in-flight typed value to land after - // the Esc. + // The clear path cancels any pending debounce synchronously, then emits + // `''` directly — no chance for an in-flight typed value to land after. const handleInputKeyDown = useCallback( (event: React.KeyboardEvent) => { if (event.key !== 'Escape') { @@ -236,7 +212,12 @@ export function InputSearch({ if (localValue.length > 0) { setLocalValue(''); - emit(''); + debouncedEmit.cancel(); + + if (lastEmittedReference.current !== '') { + lastEmittedReference.current = ''; + onSearchChange(''); + } return; } @@ -244,7 +225,7 @@ export function InputSearch({ inputRef.current?.blur(); setIsExpanded(false); }, - [emit, localValue], + [debouncedEmit, localValue, onSearchChange], ); return ( diff --git a/frontend/src/hooks/use-debounced-callback.test.tsx b/frontend/src/hooks/use-debounced-callback.test.tsx new file mode 100644 index 00000000..f3f1fe47 --- /dev/null +++ b/frontend/src/hooks/use-debounced-callback.test.tsx @@ -0,0 +1,111 @@ +import { act, renderHook } from '@testing-library/react'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; + +import { useDebouncedCallback } from './use-debounced-callback'; + +describe('useDebouncedCallback', () => { + beforeEach(() => { + vi.useFakeTimers(); + }); + + afterEach(() => { + vi.useRealTimers(); + }); + + it('defers invocation until delayMs of silence', () => { + const fn = vi.fn(); + const { result } = renderHook(() => useDebouncedCallback(fn, 100)); + + act(() => { + result.current('a'); + result.current('b'); + result.current('c'); + }); + + expect(fn).not.toHaveBeenCalled(); + + act(() => { + vi.advanceTimersByTime(100); + }); + + // Only the latest call lands; intermediate ones are dropped. + expect(fn).toHaveBeenCalledExactlyOnceWith('c'); + }); + + it('cancel() drops the pending dispatch', () => { + const fn = vi.fn(); + const { result } = renderHook(() => useDebouncedCallback(fn, 100)); + + act(() => { + result.current('x'); + result.current.cancel(); + vi.advanceTimersByTime(500); + }); + + expect(fn).not.toHaveBeenCalled(); + }); + + it('isPending() flips with the timer', () => { + const fn = vi.fn(); + const { result } = renderHook(() => useDebouncedCallback(fn, 100)); + + expect(result.current.isPending()).toBe(false); + + act(() => { + result.current('a'); + }); + + expect(result.current.isPending()).toBe(true); + + act(() => { + vi.advanceTimersByTime(100); + }); + + expect(result.current.isPending()).toBe(false); + }); + + it('reads the latest `fn` closure at fire time, not at schedule time', () => { + // Mirrors the production use case: a stale fn would close over an + // old prop. The debounced callback identity stays stable across + // renders, but the dispatched fn must be the freshest one. + const calls: string[] = []; + const { rerender, result } = renderHook(({ tag }: { tag: string }) => useDebouncedCallback(() => calls.push(tag), 100), { + initialProps: { tag: 'old' }, + }); + + const initialDebounced = result.current; + + act(() => { + result.current(); + }); + + rerender({ tag: 'new' }); + + // Same callback identity across renders — caller can pass it to + // memoised children or list it in effect deps without churn. + expect(result.current).toBe(initialDebounced); + + act(() => { + vi.advanceTimersByTime(100); + }); + + expect(calls).toEqual(['new']); + }); + + it('cancels pending dispatch on unmount so dead components never fire', () => { + const fn = vi.fn(); + const { result, unmount } = renderHook(() => useDebouncedCallback(fn, 100)); + + act(() => { + result.current('a'); + }); + + unmount(); + + act(() => { + vi.advanceTimersByTime(500); + }); + + expect(fn).not.toHaveBeenCalled(); + }); +}); diff --git a/frontend/src/hooks/use-debounced-callback.ts b/frontend/src/hooks/use-debounced-callback.ts new file mode 100644 index 00000000..dc66cea4 --- /dev/null +++ b/frontend/src/hooks/use-debounced-callback.ts @@ -0,0 +1,95 @@ +import { useEffect, useState } from 'react'; + +import { useLatestRef } from './use-latest-ref'; + +/** + * A function that defers invoking `fn` until `delayMs` have elapsed since the + * last call. The returned function is stable across renders and exposes + * imperative escape hatches for callers that need to override the schedule. + */ +export interface DebouncedCallback { + (...args: Args): void; + /** + * Drop any pending call. After this, the next invocation starts a fresh + * timer. Idempotent — safe to call from `handleClear`, route transitions, + * external-sync effects, or anywhere else that needs to override the + * schedule. + */ + cancel: () => void; + /** + * Whether a call is currently scheduled but not yet dispatched. Useful + * for tests; rarely needed in product code. + */ + isPending: () => boolean; +} + +/** + * Same idea as `lodash.debounce` / `use-debounce`, but small and + * dependency-free: returns a stable callback that defers invoking `fn` until + * `delayMs` have elapsed since the most recent call. New calls reset the + * timer; the most recent arguments are the ones eventually dispatched. + * + * Why a hook (instead of inlining `setTimeout`): + * - `cancel()` is a single, synchronous call site — usable from `handleClear`, + * external-sync effects, or anywhere else that needs to override the + * pending dispatch. There is no opportunity for a stale timer to fire + * after the cancel, which was the entire class of races in our previous + * filter implementation. + * - The returned function identity is stable across renders, so it can be + * passed to memoised children and listed in effect deps without churn. + * - `fn` is captured in a latest-ref, so the dispatched call always sees the + * most recent closure (props/state at fire time, not at schedule time). + * This is the standard React idiom for stashing an event-like handler. + * - Unmount cleanup is automatic — `useEffect` cancels any pending timer + * so dispatched functions can't land in a torn-down tree. + * + * Tradeoff vs `useDeferredValue`: `useDeferredValue` is React's preferred + * tool for deferring *rendering* work (a memoised list re-renders later + * while the input stays snappy). It does not defer side-effects. Reach for + * `useDebouncedCallback` whenever you need to throttle imperative work that + * leaves React's render tree — URL writes, network requests, persistence — + * and `useDeferredValue` for the render pipeline itself. + * + * Implementation note: the entire API object is built once inside a + * `useState` lazy initialiser, with the timer handle living in a *closure + * variable* (`timerId`) rather than a React `useRef`. This keeps the + * returned `debounced` reference stable forever and side-steps the React + * Compiler rule that flags reading refs during render — there are no refs + * to read, only a private variable closed over by the three methods. + */ +export function useDebouncedCallback( + fn: (...args: Args) => void, + delayMs: number, +): DebouncedCallback { + const fnReference = useLatestRef(fn); + const delayReference = useLatestRef(delayMs); + + const [debounced] = useState>(() => { + let timerId: null | number = null; + + const cancel = () => { + if (timerId !== null) { + window.clearTimeout(timerId); + timerId = null; + } + }; + + const isPending = () => timerId !== null; + + const invoker = (...args: Args) => { + cancel(); + timerId = window.setTimeout(() => { + timerId = null; + fnReference.current(...args); + }, delayReference.current); + }; + + return Object.assign(invoker, { cancel, isPending }); + }); + + // Cancel any pending dispatch when the host component unmounts so it + // can't fire into a torn-down tree. + useEffect(() => debounced.cancel, [debounced]); + + return debounced; +}