refactor(frontend): one outbound timer for filter input emit

Previously DataTableFilter (and InputSearch) layered four cells holding
the same logical value — `localValue` (sync useState), `debouncedValue`
(async useState inside `useDebouncedValue`), `lastEmittedReference`
(ref), and the parent's `query` prop (async round-trip) — and stitched
them together with two opposing useEffect's. The async `debouncedValue`
cell could be read out of phase from any other effect, which leaked
through as four distinct races: X clear after debounce flush (URL/state
snap back ~50–80 ms), X clear *before* debounce flush (pending timer
fires after clear), external `query` change mid-debounce (pending
typed value clobbers external), and tail-end character loss on fast
typing across the round-trip boundary.

Drop the extra storage cell. The debounce is now an imperative
`pendingTimerReference` mutated only by `handleChange`,
`cancelPendingEmit`, and the external-sync effect. All outbound
emission goes through one `emit(next)` function that synchronously
cancels any pending timer before sending. `handleClear` → `emit('')`.
External `query` change → cancel pending + accept. Unmount → cancel.
`onQueryChange` lives behind `useLatestRef` so effect deps stay
stable across parent re-renders.

Four regression tests in data-table.test.tsx pin all four scenarios.
Verified by rolling back the fix temporarily — the X-clear race test
fails with the exact `emitted = ['', 'alpha', '']` from the original
browser repro. With the fix in place all four pass.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
Sergey Kozyrenko
2026-05-23 00:24:09 +07:00
co-authored by Claude Opus 4.7
parent 4a83b29c3d
commit ba354f3701
3 changed files with 286 additions and 121 deletions
+134 -35
View File
@@ -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 (
<DataTable<Row>
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 (
<DataTable<Row>
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(<Host />, { wrapper: Wrapper });
render(<FilterHost emitted={emitted} />, { 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(<FilterHost emitted={emitted} />, { 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(
<FilterHost
emitted={emitted}
onSetValueRef={(setter) => {
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(<FilterHost emitted={emitted} initialValue="seed" />, { 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', () => {
+87 -38
View File
@@ -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<TData, TValue = unknown>({ 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 | number>(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<HTMLInputElement>) => {
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 (
<InputGroup className="max-w-sm">
@@ -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}
+65 -48
View File
@@ -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<HTMLInputElement>(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 | number>(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<HTMLInputElement>) => {
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<HTMLInputElement>) => {
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}