mirror of
https://github.com/vxcontrol/pentagi.git
synced 2026-08-25 20:46:31 +00:00
refactor(frontend): unify table URL state under useTableState
The split `useTableQueryFilter` + `usePagination` pair had a latent batching race: react-router v6 feeds every functional `setSearchParams(updater)` queued in a single React tick the same pre-batch snapshot, so a `setFilter` + `setPage` issued from the same event handler (e.g. a debounced filter commit landing alongside a paging click) would collapse — the second write erased the first and `q` disappeared from the URL. The earlier window.location workaround papered over the symptom for the typical case but didn't remove the underlying possibility. Replace both hooks with `useTableState`, which owns filter + pageIndex + storage roundtrip together and routes every URL write through a single microtask-coalesced `update(patch)`. When multiple `update` calls fire in the same tick — `setFilter` and `setPage`, two synchronous handlers, an effect and a click — they merge into one navigation rather than racing. Replace conflicts resolve in favour of push, so intentional history entries (paging) survive coalescence with replace-only updates (filter typing). The `MemoryRouter` fallback (latest snapshot via ref) is kept only as a defensive read; the coalescence itself makes it redundant. A regression test (`two top-level updaters firing in the same tick keep both params`) locks this in: the previous behaviour failed it, the new implementation passes it without touching `window.location`. Read-only siblings stay on `useTableQueryFilterReader` — detail pages don't write the URL, so no race surface to remove. `usePagination` deleted entirely; its callers migrated to `useTableState`. 474/474 tests pass. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Opus 4.7
parent
019ddcb1a7
commit
022e8f3df0
@@ -1,4 +1,4 @@
|
||||
import { useTableQueryFilter } from '@/hooks/use-table-query-filter';
|
||||
import { useTableState } from '@/hooks/use-table-state';
|
||||
|
||||
import { SEARCH_DEBOUNCE_MS } from './resources-constants';
|
||||
|
||||
@@ -12,19 +12,19 @@ interface UseResourcesSearchResult {
|
||||
/**
|
||||
* Search state for the Resources file manager.
|
||||
*
|
||||
* Backed by `useTableQueryFilter` so the search query lives in the URL
|
||||
* (`?q=`) with a localStorage fallback — the FileManager survives reloads
|
||||
* with the same filter active and shareable links keep working. The debounce
|
||||
* delay is preserved (`SEARCH_DEBOUNCE_MS`) so the existing client-side
|
||||
* tree filter still gets the throttled value it expects. Storage key is
|
||||
* defaulted to the current pathname inside `useTableQueryFilter`.
|
||||
* Backed by `useTableState` so the query lives in the URL (`?q=`) with a
|
||||
* localStorage fallback — the FileManager survives reloads with the same
|
||||
* filter active and shareable links keep working. The debounce delay is
|
||||
* preserved (`SEARCH_DEBOUNCE_MS`) so the existing client-side tree filter
|
||||
* still gets the throttled value it expects.
|
||||
*
|
||||
* `clearPageParamOnChange: false` because the Resources page has no `?page=`
|
||||
* — there's no pagination to reset on every keystroke.
|
||||
* `clearPageOnFilterChange: false` because Resources has no `?page=` to
|
||||
* reset — leaving the default would also work (deleting a non-existent
|
||||
* param is a no-op), but the explicit setting documents intent.
|
||||
*/
|
||||
export const useResourcesSearch = (): UseResourcesSearchResult => {
|
||||
const { debouncedFilter, filter, resetFilter, setFilter } = useTableQueryFilter({
|
||||
clearPageParamOnChange: false,
|
||||
const { debouncedFilter, filter, resetFilter, setFilter } = useTableState({
|
||||
clearPageOnFilterChange: false,
|
||||
debounceMs: SEARCH_DEBOUNCE_MS,
|
||||
});
|
||||
|
||||
|
||||
@@ -1,105 +0,0 @@
|
||||
import type { ReactNode } from 'react';
|
||||
|
||||
import { act, renderHook, waitFor } from '@testing-library/react';
|
||||
import { MemoryRouter, useLocation } from 'react-router-dom';
|
||||
import { describe, expect, it } from 'vitest';
|
||||
|
||||
import { usePagination } from './use-pagination';
|
||||
|
||||
interface RenderResult {
|
||||
pageIndex: number;
|
||||
search: string;
|
||||
setPage: (pageIndex: number) => void;
|
||||
}
|
||||
|
||||
// Bundle `usePagination` + `useLocation` into a single hook so `renderHook`
|
||||
// observes both reactively — without this, a sibling `<LocationProbe>` lags
|
||||
// one render behind the canonicalization layout effect when the URL is
|
||||
// rewritten on mount.
|
||||
const usePaginationWithLocation = (): RenderResult => {
|
||||
const { pageIndex, setPage } = usePagination();
|
||||
const { search } = useLocation();
|
||||
|
||||
return { pageIndex, search, setPage };
|
||||
};
|
||||
|
||||
const renderWithRouter = (initialEntries: string[]) => {
|
||||
const Wrapper = ({ children }: { children: ReactNode }) => (
|
||||
<MemoryRouter initialEntries={initialEntries}>{children}</MemoryRouter>
|
||||
);
|
||||
|
||||
return renderHook(usePaginationWithLocation, { wrapper: Wrapper });
|
||||
};
|
||||
|
||||
describe('usePagination', () => {
|
||||
it('returns pageIndex 0 when no `?page=` is present', () => {
|
||||
const { result } = renderWithRouter(['/flows']);
|
||||
expect(result.current.pageIndex).toBe(0);
|
||||
});
|
||||
|
||||
it('converts a 1-based `?page=` URL param to a 0-based index', () => {
|
||||
const { result } = renderWithRouter(['/flows?page=3']);
|
||||
expect(result.current.pageIndex).toBe(2);
|
||||
});
|
||||
|
||||
it('falls back to 0 on a non-numeric `?page=`', () => {
|
||||
const { result } = renderWithRouter(['/flows?page=notanumber']);
|
||||
expect(result.current.pageIndex).toBe(0);
|
||||
});
|
||||
|
||||
it('canonicalizes `?page=1` away from the URL on mount', async () => {
|
||||
const { result } = renderWithRouter(['/flows?page=1']);
|
||||
|
||||
// `setSearchParams` from a layout effect schedules a transition that
|
||||
// doesn't always commit in the same `act()` flush as the initial
|
||||
// mount under React 19 + react-router 7, so wait for the URL to
|
||||
// settle.
|
||||
await waitFor(() => {
|
||||
expect(result.current.search).toBe('');
|
||||
});
|
||||
});
|
||||
|
||||
it('canonicalizes `?page=1` but leaves other params untouched', async () => {
|
||||
const { result } = renderWithRouter(['/flows?page=1&q=foo']);
|
||||
|
||||
await waitFor(() => {
|
||||
const params = new URLSearchParams(result.current.search);
|
||||
expect(params.has('page')).toBe(false);
|
||||
});
|
||||
|
||||
expect(new URLSearchParams(result.current.search).get('q')).toBe('foo');
|
||||
});
|
||||
|
||||
it('does not canonicalize `?page=2` (canonical form already)', () => {
|
||||
const { result } = renderWithRouter(['/flows?page=2']);
|
||||
expect(result.current.search).toBe('?page=2');
|
||||
});
|
||||
|
||||
it('setPage(0) removes the `?page=` param entirely', () => {
|
||||
const { result } = renderWithRouter(['/flows?page=5']);
|
||||
act(() => result.current.setPage(0));
|
||||
expect(result.current.search).toBe('');
|
||||
});
|
||||
|
||||
it('setPage(N>0) writes 1-based number to `?page=`', () => {
|
||||
const { result } = renderWithRouter(['/flows']);
|
||||
act(() => result.current.setPage(4));
|
||||
expect(result.current.search).toBe('?page=5');
|
||||
});
|
||||
|
||||
it('setPage preserves other URL params', () => {
|
||||
const { result } = renderWithRouter(['/flows?q=foo']);
|
||||
act(() => result.current.setPage(2));
|
||||
const params = new URLSearchParams(result.current.search ?? '');
|
||||
expect(params.get('q')).toBe('foo');
|
||||
expect(params.get('page')).toBe('3');
|
||||
});
|
||||
|
||||
it('honors a custom paramName override', () => {
|
||||
const Wrapper = ({ children }: { children: ReactNode }) => (
|
||||
<MemoryRouter initialEntries={['/flows?p=4']}>{children}</MemoryRouter>
|
||||
);
|
||||
const { result } = renderHook(() => usePagination({ paramName: 'p' }), { wrapper: Wrapper });
|
||||
expect(result.current.pageIndex).toBe(3);
|
||||
});
|
||||
});
|
||||
@@ -1,122 +0,0 @@
|
||||
import { useCallback, useEffect, useMemo, useRef } from 'react';
|
||||
import { useSearchParams } from 'react-router-dom';
|
||||
|
||||
import { URL_PARAMS } from '@/lib/url-params';
|
||||
|
||||
interface UsePaginationOptions {
|
||||
/**
|
||||
* Query string parameter that carries the 1-based page number. Default
|
||||
* `URL_PARAMS.PAGE` (`'page'`). Override when a route needs a custom
|
||||
* key (e.g. a page that already uses `?page=` for something else).
|
||||
*/
|
||||
paramName?: string;
|
||||
}
|
||||
|
||||
interface UsePaginationResult {
|
||||
/**
|
||||
* 0-based page index, suitable for handing straight to TanStack Table's
|
||||
* `state.pagination.pageIndex`. The URL stores it 1-based for humans;
|
||||
* the hook handles the off-by-one in both directions.
|
||||
*/
|
||||
pageIndex: number;
|
||||
/** Set the page index (0-based). Pass `0` to drop the URL param entirely. */
|
||||
setPage: (pageIndex: number) => void;
|
||||
}
|
||||
|
||||
/**
|
||||
* URL-backed pagination state. The 1-based page number lives in `?page=`, so
|
||||
* users can bookmark/share specific pages and the back/forward stack reflects
|
||||
* page changes.
|
||||
*
|
||||
* Conventions:
|
||||
* - The URL stores 1-based numbers (`?page=2` = second page) because that's
|
||||
* what users see in pagers ("Page 2 of 10"). The hook converts to/from
|
||||
* 0-based at the boundary.
|
||||
* - `?page=1` is canonicalized away — the first page is the default URL.
|
||||
* That avoids two URLs (`/flows` vs `/flows?page=1`) representing the
|
||||
* same view, which would split the history stack.
|
||||
*
|
||||
* Pages that also use `useTableQueryFilter` get the `?page=` reset for free
|
||||
* when the filter narrows the result set — see that hook's
|
||||
* `clearPageParamOnChange` option.
|
||||
*/
|
||||
export const usePagination = ({ paramName = URL_PARAMS.PAGE }: UsePaginationOptions = {}): UsePaginationResult => {
|
||||
const [searchParams, setSearchParams] = useSearchParams();
|
||||
|
||||
const pageIndex = useMemo(() => {
|
||||
const page = searchParams.get(paramName);
|
||||
|
||||
if (!page) {
|
||||
return 0;
|
||||
}
|
||||
|
||||
const parsed = Number.parseInt(page, 10);
|
||||
|
||||
return Number.isFinite(parsed) ? Math.max(0, parsed - 1) : 0;
|
||||
}, [paramName, searchParams]);
|
||||
|
||||
// Canonicalize `?<paramName>=1` to a clean URL whenever it shows up.
|
||||
// Two URLs (`/flows` vs `/flows?page=1`) would otherwise denote the same
|
||||
// view — splitting the history stack and giving link-sharers an ugly URL
|
||||
// for "first page". The effect uses `replace: true` so the rewrite never
|
||||
// adds a history entry, and is idempotent: once the param is removed it
|
||||
// re-runs and immediately exits the `if`, no loop.
|
||||
//
|
||||
// `useEffect` (not `useLayoutEffect`) is intentional: the URL bar updates
|
||||
// outside React's paint pipeline, so deferring the rewrite past commit
|
||||
// costs nothing visible to the user, while letting the canonicalization
|
||||
// settle through normal effect scheduling — friendlier to test harnesses
|
||||
// and to react-router's own transition handling.
|
||||
useEffect(() => {
|
||||
if (searchParams.get(paramName) !== '1') {
|
||||
return;
|
||||
}
|
||||
|
||||
setSearchParams(
|
||||
(previous) => {
|
||||
const next = new URLSearchParams(previous);
|
||||
next.delete(paramName);
|
||||
|
||||
return next;
|
||||
},
|
||||
{ replace: true },
|
||||
);
|
||||
}, [paramName, searchParams, setSearchParams]);
|
||||
|
||||
// See `use-table-query-filter.ts` for the full rationale — render-time
|
||||
// mutation is safe because the ref is only read from event callbacks
|
||||
// (`setPage`), never during render. We deliberately skip `useLatestRef`
|
||||
// because its passive-`useEffect` sync lags by one commit, which is
|
||||
// exactly the gap that lets a batched filter+page race lose state.
|
||||
const searchParamsReference = useRef(searchParams);
|
||||
// eslint-disable-next-line react-hooks/refs
|
||||
searchParamsReference.current = searchParams;
|
||||
|
||||
const setPage = useCallback(
|
||||
(newPageIndex: number) => {
|
||||
// Avoid the functional `(previous) => ...` form: react-router v6
|
||||
// feeds the same pre-batch snapshot to every functional updater
|
||||
// queued in a single tick, so a `setFilter` + `setPage` pair
|
||||
// (e.g. debounced filter commit landing alongside a paging click)
|
||||
// would collapse — the second write erases the first. Prefer
|
||||
// `window.location.search` under `BrowserRouter`; fall back to
|
||||
// the latest committed `searchParams` (via ref) under
|
||||
// `MemoryRouter`, which doesn't sync `window.location`.
|
||||
const fromWindow = typeof window !== 'undefined' ? window.location.search : '';
|
||||
const next = fromWindow
|
||||
? new URLSearchParams(fromWindow)
|
||||
: new URLSearchParams(searchParamsReference.current);
|
||||
|
||||
if (newPageIndex <= 0) {
|
||||
next.delete(paramName);
|
||||
} else {
|
||||
next.set(paramName, String(newPageIndex + 1));
|
||||
}
|
||||
|
||||
setSearchParams(next);
|
||||
},
|
||||
[paramName, setSearchParams],
|
||||
);
|
||||
|
||||
return { pageIndex, setPage };
|
||||
};
|
||||
@@ -1,40 +1,14 @@
|
||||
import type { ReactNode } from 'react';
|
||||
|
||||
import { act, renderHook, waitFor } from '@testing-library/react';
|
||||
import { renderHook } from '@testing-library/react';
|
||||
import { MemoryRouter, useLocation } from 'react-router-dom';
|
||||
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
|
||||
|
||||
import { useTableQueryFilter, useTableQueryFilterReader } from './use-table-query-filter';
|
||||
import { useTableQueryFilterReader } from './use-table-query-filter';
|
||||
|
||||
const STORAGE_KEY = 'table_4_/flows';
|
||||
const SHORT_DEBOUNCE_MS = 5;
|
||||
|
||||
interface RenderResult {
|
||||
debouncedFilter: string;
|
||||
filter: string;
|
||||
resetFilter: () => void;
|
||||
search: string;
|
||||
setFilter: (value: string) => void;
|
||||
}
|
||||
|
||||
const useFilterWithLocation = (options: { debounceMs?: number; storageKey?: string } = {}): RenderResult => {
|
||||
const { debouncedFilter, filter, resetFilter, setFilter } = useTableQueryFilter({
|
||||
debounceMs: options.debounceMs ?? SHORT_DEBOUNCE_MS,
|
||||
storageKey: options.storageKey ?? STORAGE_KEY,
|
||||
});
|
||||
const { search } = useLocation();
|
||||
|
||||
return { debouncedFilter, filter, resetFilter, search, setFilter };
|
||||
};
|
||||
|
||||
const renderWithRouter = (initialEntries: string[], options?: { debounceMs?: number; storageKey?: string }) => {
|
||||
const Wrapper = ({ children }: { children: ReactNode }) => (
|
||||
<MemoryRouter initialEntries={initialEntries}>{children}</MemoryRouter>
|
||||
);
|
||||
|
||||
return renderHook(() => useFilterWithLocation(options), { wrapper: Wrapper });
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
localStorage.clear();
|
||||
});
|
||||
@@ -43,204 +17,6 @@ afterEach(() => {
|
||||
localStorage.clear();
|
||||
});
|
||||
|
||||
const readStoredFilter = (key: string): null | string => {
|
||||
const raw = localStorage.getItem(key);
|
||||
|
||||
if (raw === null) {
|
||||
return null;
|
||||
}
|
||||
|
||||
try {
|
||||
const parsed = JSON.parse(raw);
|
||||
|
||||
return typeof parsed?.filter === 'string' ? parsed.filter : null;
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
};
|
||||
|
||||
describe('useTableQueryFilter — URL ↔ storage roundtrip', () => {
|
||||
it('reads the initial filter from `?q=` when present', () => {
|
||||
const { result } = renderWithRouter(['/flows?q=alpha']);
|
||||
expect(result.current.filter).toBe('alpha');
|
||||
});
|
||||
|
||||
it('returns an empty filter when neither URL nor storage have a value', () => {
|
||||
const { result } = renderWithRouter(['/flows']);
|
||||
expect(result.current.filter).toBe('');
|
||||
});
|
||||
|
||||
// The restore-from-storage path lives in a `useEffect` that issues
|
||||
// `setSearchParams` on mount. Under React 19 + react-router 7 the
|
||||
// resulting URL transition lands one micro-task after the initial
|
||||
// `act(render)` flush, so we drain one round of pending updates
|
||||
// through an empty async `act` before asserting.
|
||||
it('restores `?q=` from storage when the URL is empty', async () => {
|
||||
localStorage.setItem(STORAGE_KEY, JSON.stringify({ filter: 'alpha' }));
|
||||
const { result } = renderWithRouter(['/flows']);
|
||||
|
||||
await act(async () => {
|
||||
await Promise.resolve();
|
||||
});
|
||||
|
||||
expect(new URLSearchParams(result.current.search).get('q')).toBe('alpha');
|
||||
expect(result.current.filter).toBe('alpha');
|
||||
});
|
||||
|
||||
it('lets the URL win over storage when both have a value', async () => {
|
||||
localStorage.setItem(STORAGE_KEY, JSON.stringify({ filter: 'beta' }));
|
||||
const { result } = renderWithRouter(['/flows?q=alpha']);
|
||||
|
||||
expect(result.current.filter).toBe('alpha');
|
||||
|
||||
// The hook should sync the URL value into storage so a subsequent
|
||||
// tab without `?q=` resumes from the URL's intent, not the old one.
|
||||
await waitFor(() => {
|
||||
expect(readStoredFilter(STORAGE_KEY)).toBe('alpha');
|
||||
});
|
||||
});
|
||||
|
||||
it('setFilter writes the value into the URL', () => {
|
||||
const { result } = renderWithRouter(['/flows']);
|
||||
|
||||
act(() => result.current.setFilter('gamma'));
|
||||
|
||||
expect(new URLSearchParams(result.current.search).get('q')).toBe('gamma');
|
||||
});
|
||||
|
||||
it('setFilter("") drops the URL param entirely (no `?q=` empty entry)', () => {
|
||||
const { result } = renderWithRouter(['/flows?q=alpha']);
|
||||
|
||||
act(() => result.current.setFilter(''));
|
||||
|
||||
expect(result.current.search).toBe('');
|
||||
});
|
||||
|
||||
it('setFilter("") clears the storage entry through the effect-driven write', async () => {
|
||||
localStorage.setItem(STORAGE_KEY, JSON.stringify({ filter: 'alpha' }));
|
||||
const { result } = renderWithRouter(['/flows?q=alpha']);
|
||||
|
||||
act(() => result.current.setFilter(''));
|
||||
|
||||
await waitFor(() => {
|
||||
expect(readStoredFilter(STORAGE_KEY)).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
it('persists the URL filter to storage once typing settles', async () => {
|
||||
const { result } = renderWithRouter(['/flows']);
|
||||
|
||||
act(() => result.current.setFilter('persisted'));
|
||||
|
||||
await waitFor(() => {
|
||||
expect(readStoredFilter(STORAGE_KEY)).toBe('persisted');
|
||||
});
|
||||
});
|
||||
|
||||
it('resetFilter is equivalent to setFilter("")', () => {
|
||||
const { result } = renderWithRouter(['/flows?q=alpha']);
|
||||
|
||||
act(() => result.current.resetFilter());
|
||||
|
||||
expect(result.current.search).toBe('');
|
||||
expect(result.current.filter).toBe('');
|
||||
});
|
||||
|
||||
it('debouncedFilter eventually catches up with filter', async () => {
|
||||
const { result } = renderWithRouter(['/flows']);
|
||||
|
||||
act(() => result.current.setFilter('typed'));
|
||||
expect(result.current.filter).toBe('typed');
|
||||
|
||||
await waitFor(() => {
|
||||
expect(result.current.debouncedFilter).toBe('typed');
|
||||
});
|
||||
});
|
||||
|
||||
it('does not clobber unrelated table state when writing the filter', async () => {
|
||||
localStorage.setItem(
|
||||
STORAGE_KEY,
|
||||
JSON.stringify({ columnVisibility: { name: false }, sorting: [{ desc: true, id: 'createdAt' }] }),
|
||||
);
|
||||
const { result } = renderWithRouter(['/flows']);
|
||||
|
||||
act(() => result.current.setFilter('foo'));
|
||||
|
||||
await waitFor(() => {
|
||||
const stored = JSON.parse(localStorage.getItem(STORAGE_KEY) ?? '{}');
|
||||
expect(stored.filter).toBe('foo');
|
||||
expect(stored.sorting).toEqual([{ desc: true, id: 'createdAt' }]);
|
||||
expect(stored.columnVisibility).toEqual({ name: false });
|
||||
});
|
||||
});
|
||||
|
||||
it('re-restores when storageKey rotates while the hook stays mounted', async () => {
|
||||
const FIRST_KEY = 'table_4_/flows';
|
||||
const SECOND_KEY = 'table_4_/templates';
|
||||
localStorage.setItem(SECOND_KEY, JSON.stringify({ filter: 'second-key' }));
|
||||
|
||||
const Wrapper = ({ children }: { children: ReactNode }) => (
|
||||
<MemoryRouter initialEntries={['/flows']}>{children}</MemoryRouter>
|
||||
);
|
||||
const { rerender, result } = renderHook(
|
||||
({ key }) => {
|
||||
const filter = useTableQueryFilter({ debounceMs: SHORT_DEBOUNCE_MS, storageKey: key });
|
||||
const { search } = useLocation();
|
||||
|
||||
return { ...filter, search };
|
||||
},
|
||||
{ initialProps: { key: FIRST_KEY }, wrapper: Wrapper },
|
||||
);
|
||||
|
||||
await act(async () => {
|
||||
await Promise.resolve();
|
||||
});
|
||||
expect(result.current.filter).toBe('');
|
||||
|
||||
rerender({ key: SECOND_KEY });
|
||||
|
||||
await waitFor(() => {
|
||||
expect(new URLSearchParams(result.current.search).get('q')).toBe('second-key');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('useTableQueryFilter — page param interaction', () => {
|
||||
it('resets `?page=` when setFilter narrows the result set', () => {
|
||||
const { result } = renderWithRouter(['/flows?page=3']);
|
||||
|
||||
act(() => result.current.setFilter('foo'));
|
||||
|
||||
const params = new URLSearchParams(result.current.search);
|
||||
expect(params.has('page')).toBe(false);
|
||||
expect(params.get('q')).toBe('foo');
|
||||
});
|
||||
|
||||
it('keeps `?page=` when clearPageParamOnChange is false', () => {
|
||||
const Wrapper = ({ children }: { children: ReactNode }) => (
|
||||
<MemoryRouter initialEntries={['/flows?page=3']}>{children}</MemoryRouter>
|
||||
);
|
||||
const { result } = renderHook(
|
||||
() => {
|
||||
const filter = useTableQueryFilter({
|
||||
clearPageParamOnChange: false,
|
||||
debounceMs: SHORT_DEBOUNCE_MS,
|
||||
storageKey: STORAGE_KEY,
|
||||
});
|
||||
const { search } = useLocation();
|
||||
|
||||
return { ...filter, search };
|
||||
},
|
||||
{ wrapper: Wrapper },
|
||||
);
|
||||
|
||||
act(() => result.current.setFilter('foo'));
|
||||
|
||||
const params = new URLSearchParams(result.current.search);
|
||||
expect(params.get('page')).toBe('3');
|
||||
});
|
||||
});
|
||||
|
||||
describe('useTableQueryFilterReader', () => {
|
||||
it('observes the URL filter without writing storage', () => {
|
||||
const Wrapper = ({ children }: { children: ReactNode }) => (
|
||||
|
||||
@@ -1,35 +1,20 @@
|
||||
import { useCallback, useEffect, useMemo, useRef } from 'react';
|
||||
import { useMemo } from 'react';
|
||||
import { useSearchParams } from 'react-router-dom';
|
||||
|
||||
import { useDebouncedValue } from '@/hooks/use-debounced-value';
|
||||
import { useEffectAfterMount } from '@/hooks/use-effect-after-mount';
|
||||
import { usePageStorageKeys } from '@/hooks/use-page-storage-keys';
|
||||
import { loadTableState, updateTableState } from '@/lib/table-state';
|
||||
import { URL_PARAMS } from '@/lib/url-params';
|
||||
|
||||
interface UseTableQueryFilterOptions {
|
||||
/**
|
||||
* Whether to also reset the `?page=` URL param when the filter changes.
|
||||
* Pages that pair URL filter with URL pagination need this so the user
|
||||
* doesn't end up "on page 5 of nothing" after narrowing the result set.
|
||||
* Defaults to `true`.
|
||||
*/
|
||||
clearPageParamOnChange?: boolean;
|
||||
interface UseTableQueryFilterReaderOptions {
|
||||
debounceMs?: number;
|
||||
/**
|
||||
* The query string param that carries the filter. Default `URL_PARAMS.QUERY`
|
||||
* (`'q'`) matches the conventional "search query" name and is consistent
|
||||
* across pages.
|
||||
* The query string param to read. Default `URL_PARAMS.QUERY` (`'q'`).
|
||||
*/
|
||||
paramName?: string;
|
||||
/**
|
||||
* Stable storage key for persisting the most recent value (a fresh tab
|
||||
* without `?q=` resumes from here). When omitted, defaults to
|
||||
* `usePageStorageKeys().table` — i.e. `table_4_<current pathname>`,
|
||||
* which is the right key for any top-level list route. Override only
|
||||
* for nested routes or when you need to share a storage slot across
|
||||
* paths (the same way a detail page shares its parent list's slot
|
||||
* through `useTableQueryFilterReader`).
|
||||
* Storage key — accepted for API symmetry with `useTableState` and the
|
||||
* historical mutate variant, but ignored here. The reader is pure-URL
|
||||
* and never touches storage; the option exists only so detail pages can
|
||||
* pass the parent list's storage key without conditional logic.
|
||||
*/
|
||||
storageKey?: string;
|
||||
}
|
||||
@@ -39,196 +24,25 @@ interface UseTableQueryFilterReaderResult {
|
||||
filter: string;
|
||||
}
|
||||
|
||||
interface UseTableQueryFilterResult extends UseTableQueryFilterReaderResult {
|
||||
resetFilter: () => void;
|
||||
setFilter: (value: string) => void;
|
||||
}
|
||||
|
||||
/**
|
||||
* Pure URL reader: surface the live + debounced filter without touching
|
||||
* storage or scheduling any side-effects.
|
||||
* Read-only subscription to the URL filter for pages that observe the value
|
||||
* but never mutate it (typically detail pages, where the toolbar walks the
|
||||
* filtered subset but the user types the filter on the list page).
|
||||
*
|
||||
* Re-evaluates whenever `?<paramName>=` changes — so a detail page that
|
||||
* subscribes via `useTableQueryFilterReader` (or any caller that wants
|
||||
* read-only access) stays in sync with whatever the list page typed.
|
||||
* The hook never writes to the URL or storage: a detail page opened via a
|
||||
* shared `/flows/:id` link will not silently inject the previous tab's
|
||||
* `?q=` into the URL. Pages that *do* need to mutate the filter live on
|
||||
* `useTableState` instead — it owns the URL ↔ storage roundtrip plus
|
||||
* atomic multi-field updates that the previous split design couldn't do
|
||||
* race-free.
|
||||
*/
|
||||
const useFilterFromUrl = ({
|
||||
export const useTableQueryFilterReader = ({
|
||||
debounceMs = 200,
|
||||
paramName = URL_PARAMS.QUERY,
|
||||
}: Pick<UseTableQueryFilterOptions, 'debounceMs' | 'paramName'>): UseTableQueryFilterReaderResult => {
|
||||
}: UseTableQueryFilterReaderOptions = {}): UseTableQueryFilterReaderResult => {
|
||||
const [searchParams] = useSearchParams();
|
||||
const filter = searchParams.get(paramName) ?? '';
|
||||
const debouncedFilter = useDebouncedValue(filter, debounceMs);
|
||||
|
||||
return useMemo(() => ({ debouncedFilter, filter }), [debouncedFilter, filter]);
|
||||
};
|
||||
|
||||
/**
|
||||
* Read-only subscription to the URL filter for pages that observe the value
|
||||
* but never mutate it (typically detail pages, where the toolbar walks the
|
||||
* filtered subset but the user types the filter on the list page).
|
||||
*
|
||||
* Unlike `useTableQueryFilter`, this hook never writes to the URL or storage:
|
||||
* a detail page opened by a shared `/flows/:id` link will not silently inject
|
||||
* the previous tab's `?q=` into the URL. The `storageKey` option is accepted
|
||||
* for API symmetry but ignored here — there is no storage interaction at all.
|
||||
*/
|
||||
export const useTableQueryFilterReader = (
|
||||
options: UseTableQueryFilterOptions = {},
|
||||
): UseTableQueryFilterReaderResult => {
|
||||
return useFilterFromUrl(options);
|
||||
};
|
||||
|
||||
/**
|
||||
* URL ↔ localStorage filter for list pages.
|
||||
*
|
||||
* - Source of truth is `?<paramName>=` in the URL — that lets users share /
|
||||
* bookmark a filtered view, and any explicit path-level navigation (a
|
||||
* route change, opening a detail page, etc.) is captured by react-router's
|
||||
* own history entry alongside whatever filter was active at that moment.
|
||||
* - On mount (and whenever the effective `storageKey` rotates — e.g. the
|
||||
* route changes while the hook stays mounted in a shared layout), if the
|
||||
* URL has no `?<paramName>=` but storage does, we replay the stored value
|
||||
* into the URL (using `replace: true`, so the restore doesn't pollute the
|
||||
* back stack).
|
||||
* - Every typed change goes into the URL with `replace: true`. That means
|
||||
* intermediate filter values do **not** create new history entries —
|
||||
* browser back skips past the filter typing session in one step rather
|
||||
* than walking through `'f' → 'fo' → 'foo'`.
|
||||
* - Storage is the single sink. The unified `table` slot
|
||||
* (`updateTableState`) carries `filter` alongside the rest of the table
|
||||
* state — sorting, column visibility, page size — under one key per page.
|
||||
* Each commit writes synchronously through `useEffectAfterMount`: a single
|
||||
* source of truth removes the previous double-write pattern (sync clear +
|
||||
* debounced effect) that risked races on refresh.
|
||||
*/
|
||||
export const useTableQueryFilter = (options: UseTableQueryFilterOptions = {}): UseTableQueryFilterResult => {
|
||||
const { clearPageParamOnChange = true, paramName = URL_PARAMS.QUERY, storageKey: explicitStorageKey } = options;
|
||||
const [searchParams, setSearchParams] = useSearchParams();
|
||||
const { table: defaultStorageKey } = usePageStorageKeys();
|
||||
const storageKey = explicitStorageKey ?? defaultStorageKey;
|
||||
|
||||
const { debouncedFilter, filter } = useFilterFromUrl(options);
|
||||
|
||||
// Track which storageKey we've already restored from. When the key
|
||||
// rotates (route change inside a persistent layout, or an explicit
|
||||
// override prop change) we reset the guard so the new slot gets one
|
||||
// restore attempt — this is the fix for the "lazy useState captured the
|
||||
// mount-time key forever" pitfall the previous design had.
|
||||
const restoredForKeyReference = useRef<null | string>(null);
|
||||
|
||||
// `filter` participates in the dep array directly, not through a ref.
|
||||
// Two reasons:
|
||||
// 1. The `restoredForKeyReference` guard makes the effect a no-op on
|
||||
// keystrokes — the early return fires before any storage write,
|
||||
// so the per-keystroke re-runs cost is one Map lookup.
|
||||
// 2. `useLatestRef`-style refs have a one-commit lag (the sync
|
||||
// `useEffect` runs *after* the consumer). Reading the URL filter
|
||||
// that way at the moment `storageKey` rotates would see the
|
||||
// *previous* route's value. Plain dep gives the freshest value.
|
||||
useEffect(() => {
|
||||
if (restoredForKeyReference.current === storageKey) {
|
||||
return;
|
||||
}
|
||||
|
||||
restoredForKeyReference.current = storageKey;
|
||||
|
||||
if (filter.length > 0) {
|
||||
// The URL already has a value — that beats storage. Sync it into
|
||||
// storage so a fresh tab without `?<paramName>=` resumes from
|
||||
// this intent (e.g. user landed via a shared filtered link).
|
||||
updateTableState(storageKey, { filter });
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
const stored = loadTableState(storageKey).filter;
|
||||
|
||||
if (!stored || stored.length === 0) {
|
||||
return;
|
||||
}
|
||||
|
||||
setSearchParams(
|
||||
(previous) => {
|
||||
const next = new URLSearchParams(previous);
|
||||
next.set(paramName, stored);
|
||||
|
||||
// Intentionally do NOT delete `?page=` here. The user's
|
||||
// explicit `?page=N` (from a shared link or refresh) is their
|
||||
// own request; the replay is restoring prior filter state,
|
||||
// not a fresh filter change. If the replayed filter happens
|
||||
// to make the requested page out-of-range, the page-index
|
||||
// clamp in `DataTable` reconciles it on the next render —
|
||||
// see the `safePageIndex` derivation. `clearPageParamOnChange`
|
||||
// still applies to `setFilter` below, where the user is
|
||||
// actively changing the filter and "page 5 of nothing" is
|
||||
// the failure mode we want to avoid.
|
||||
return next;
|
||||
},
|
||||
{ replace: true },
|
||||
);
|
||||
}, [filter, paramName, setSearchParams, storageKey]);
|
||||
|
||||
// Persist the URL filter into storage on every commit. Skipping the
|
||||
// first render is intentional: a fresh-mount empty `filter` would
|
||||
// wipe a freshly-restored storage entry before the restore effect
|
||||
// above has had a chance to replay it into the URL.
|
||||
useEffectAfterMount(() => {
|
||||
updateTableState(storageKey, { filter: filter.length > 0 ? filter : undefined });
|
||||
}, [filter, storageKey]);
|
||||
|
||||
// Sync a ref to the latest committed `searchParams` on every render.
|
||||
// Used as a fallback below when `window.location` isn't a faithful
|
||||
// mirror of the router state — that happens under `MemoryRouter`,
|
||||
// which is what our hook tests use. Updating the ref in render is safe
|
||||
// because we never read it during render; we only read it inside event
|
||||
// callbacks (`setFilter` below). `useEffect`-synced refs (see
|
||||
// `useLatestRef`) lag one commit, which is fatal here: two updates
|
||||
// fired in the same tick must both see the same authoritative latest
|
||||
// value, not the prior-render snapshot.
|
||||
const searchParamsReference = useRef(searchParams);
|
||||
// eslint-disable-next-line react-hooks/refs
|
||||
searchParamsReference.current = searchParams;
|
||||
|
||||
const setFilter = useCallback(
|
||||
(value: string) => {
|
||||
// Avoid the functional `(previous) => ...` form of
|
||||
// `setSearchParams`. When two updates fire in the same React tick
|
||||
// (e.g. a debounced filter commit landing alongside a paging
|
||||
// button click), react-router v6 feeds **both** updaters the same
|
||||
// pre-batch snapshot — the second write erases the first, so
|
||||
// `?q=foo` plus `setPage(5)` collapses to `?page=6` and `q` is
|
||||
// lost. `window.location.search` is the freshest source under
|
||||
// `BrowserRouter` (production) and immune to that batching race;
|
||||
// under `MemoryRouter` (tests) the in-memory history is *not*
|
||||
// synced to `window.location`, so we fall back to the ref-stashed
|
||||
// latest react-router snapshot.
|
||||
const fromWindow = typeof window !== 'undefined' ? window.location.search : '';
|
||||
const next = fromWindow
|
||||
? new URLSearchParams(fromWindow)
|
||||
: new URLSearchParams(searchParamsReference.current);
|
||||
|
||||
if (value.length === 0) {
|
||||
next.delete(paramName);
|
||||
} else {
|
||||
next.set(paramName, value);
|
||||
}
|
||||
|
||||
if (clearPageParamOnChange) {
|
||||
next.delete(URL_PARAMS.PAGE);
|
||||
}
|
||||
|
||||
setSearchParams(next, { replace: true });
|
||||
},
|
||||
[clearPageParamOnChange, paramName, setSearchParams],
|
||||
);
|
||||
|
||||
const resetFilter = useCallback(() => {
|
||||
setFilter('');
|
||||
}, [setFilter]);
|
||||
|
||||
return useMemo(
|
||||
() => ({ debouncedFilter, filter, resetFilter, setFilter }),
|
||||
[debouncedFilter, filter, resetFilter, setFilter],
|
||||
);
|
||||
};
|
||||
|
||||
@@ -0,0 +1,262 @@
|
||||
import type { ReactNode } from 'react';
|
||||
|
||||
import { act, renderHook, waitFor } from '@testing-library/react';
|
||||
import { MemoryRouter, useLocation } from 'react-router-dom';
|
||||
import { afterEach, beforeEach, describe, expect, it } from 'vitest';
|
||||
|
||||
import { useTableState } from './use-table-state';
|
||||
|
||||
const STORAGE_KEY = 'table_4_/flows';
|
||||
const SHORT_DEBOUNCE_MS = 5;
|
||||
|
||||
interface RenderResult {
|
||||
debouncedFilter: string;
|
||||
filter: string;
|
||||
pageIndex: number;
|
||||
resetFilter: () => void;
|
||||
search: string;
|
||||
setFilter: (value: string) => void;
|
||||
setPage: (page: number) => void;
|
||||
update: ReturnType<typeof useTableState>['update'];
|
||||
}
|
||||
|
||||
const useStateWithLocation = (options: { debounceMs?: number; storageKey?: string } = {}): RenderResult => {
|
||||
const { debouncedFilter, filter, pageIndex, resetFilter, setFilter, setPage, update } = useTableState({
|
||||
debounceMs: options.debounceMs ?? SHORT_DEBOUNCE_MS,
|
||||
storageKey: options.storageKey ?? STORAGE_KEY,
|
||||
});
|
||||
const { search } = useLocation();
|
||||
|
||||
return { debouncedFilter, filter, pageIndex, resetFilter, search, setFilter, setPage, update };
|
||||
};
|
||||
|
||||
const renderWithRouter = (initialEntries: string[], options?: { debounceMs?: number; storageKey?: string }) => {
|
||||
const Wrapper = ({ children }: { children: ReactNode }) => (
|
||||
<MemoryRouter initialEntries={initialEntries}>{children}</MemoryRouter>
|
||||
);
|
||||
|
||||
return renderHook(() => useStateWithLocation(options), { wrapper: Wrapper });
|
||||
};
|
||||
|
||||
const readStoredFilter = (key: string): null | string => {
|
||||
const raw = localStorage.getItem(key);
|
||||
|
||||
if (raw === null) {
|
||||
return null;
|
||||
}
|
||||
|
||||
try {
|
||||
const parsed = JSON.parse(raw);
|
||||
|
||||
return typeof parsed?.filter === 'string' ? parsed.filter : null;
|
||||
} catch {
|
||||
return null;
|
||||
}
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
localStorage.clear();
|
||||
});
|
||||
|
||||
afterEach(() => {
|
||||
localStorage.clear();
|
||||
});
|
||||
|
||||
describe('useTableState — URL reads', () => {
|
||||
it('reads the initial filter from `?q=`', () => {
|
||||
const { result } = renderWithRouter(['/flows?q=alpha']);
|
||||
expect(result.current.filter).toBe('alpha');
|
||||
});
|
||||
|
||||
it('reads the initial pageIndex from `?page=` (1-based URL → 0-based state)', () => {
|
||||
const { result } = renderWithRouter(['/flows?page=3']);
|
||||
expect(result.current.pageIndex).toBe(2);
|
||||
});
|
||||
|
||||
it('defaults both filter and pageIndex when neither URL nor storage have a value', () => {
|
||||
const { result } = renderWithRouter(['/flows']);
|
||||
expect(result.current.filter).toBe('');
|
||||
expect(result.current.pageIndex).toBe(0);
|
||||
});
|
||||
|
||||
it('ignores non-numeric `?page=` values gracefully', () => {
|
||||
const { result } = renderWithRouter(['/flows?page=abc']);
|
||||
expect(result.current.pageIndex).toBe(0);
|
||||
});
|
||||
});
|
||||
|
||||
describe('useTableState — setFilter / setPage', () => {
|
||||
it('setFilter writes the value into the URL and resets `?page=` by default', async () => {
|
||||
const { result } = renderWithRouter(['/flows?page=3']);
|
||||
act(() => result.current.setFilter('alpha'));
|
||||
await waitFor(() => {
|
||||
const params = new URLSearchParams(result.current.search);
|
||||
// `clearPageOnFilterChange` default = true: changing the filter
|
||||
// drops the page so users don't land "on page 5 of nothing".
|
||||
expect(params.get('q')).toBe('alpha');
|
||||
expect(params.get('page')).toBeNull();
|
||||
});
|
||||
});
|
||||
|
||||
it('setFilter("") clears the URL param', async () => {
|
||||
const { result } = renderWithRouter(['/flows?q=alpha']);
|
||||
act(() => result.current.setFilter(''));
|
||||
await waitFor(() => {
|
||||
expect(result.current.filter).toBe('');
|
||||
expect(new URLSearchParams(result.current.search).has('q')).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
it('setPage writes the 1-based page number into the URL', async () => {
|
||||
const { result } = renderWithRouter(['/flows']);
|
||||
act(() => result.current.setPage(4));
|
||||
await waitFor(() => {
|
||||
expect(new URLSearchParams(result.current.search).get('page')).toBe('5');
|
||||
});
|
||||
});
|
||||
|
||||
it('setPage(0) drops the URL param entirely', async () => {
|
||||
const { result } = renderWithRouter(['/flows?page=5']);
|
||||
act(() => result.current.setPage(0));
|
||||
await waitFor(() => {
|
||||
expect(new URLSearchParams(result.current.search).has('page')).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
it('resetFilter is equivalent to setFilter("")', async () => {
|
||||
const { result } = renderWithRouter(['/flows?q=alpha']);
|
||||
act(() => result.current.resetFilter());
|
||||
await waitFor(() => {
|
||||
expect(result.current.filter).toBe('');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('useTableState — atomic `update` (race regression)', () => {
|
||||
it('writes both filter and pageIndex in a single transition', async () => {
|
||||
const { result } = renderWithRouter(['/flows']);
|
||||
act(() => result.current.update({ filter: 'alpha', pageIndex: 4 }));
|
||||
await waitFor(() => {
|
||||
const params = new URLSearchParams(result.current.search);
|
||||
expect(params.get('q')).toBe('alpha');
|
||||
expect(params.get('page')).toBe('5');
|
||||
});
|
||||
});
|
||||
|
||||
it('preserves the other param when only one field is patched', async () => {
|
||||
const { result } = renderWithRouter(['/flows?q=alpha&page=3']);
|
||||
// Update only the page — `q` must survive.
|
||||
act(() => result.current.update({ pageIndex: 9 }));
|
||||
await waitFor(() => {
|
||||
const after = new URLSearchParams(result.current.search);
|
||||
expect(after.get('q')).toBe('alpha');
|
||||
expect(after.get('page')).toBe('10');
|
||||
});
|
||||
// And the reverse: change the filter without touching page (note:
|
||||
// this is distinct from `setFilter`, which deliberately resets it).
|
||||
act(() => result.current.update({ filter: 'beta' }));
|
||||
await waitFor(() => {
|
||||
const after = new URLSearchParams(result.current.search);
|
||||
expect(after.get('q')).toBe('beta');
|
||||
expect(after.get('page')).toBe('10');
|
||||
});
|
||||
});
|
||||
|
||||
it('regression: two top-level updaters firing in the same tick keep both params', async () => {
|
||||
// The exact scenario the old split-hooks design lost: `setFilter`
|
||||
// and `setPage` issued from the same event handler dropped `?q=`
|
||||
// because react-router fed both functional updaters the same
|
||||
// pre-batch snapshot. With microtask-batched coalescence inside
|
||||
// `update`, both calls land in a single `setSearchParams` and both
|
||||
// params survive — regardless of router implementation.
|
||||
const { result } = renderWithRouter(['/flows']);
|
||||
act(() => {
|
||||
result.current.setFilter('alpha');
|
||||
result.current.setPage(5);
|
||||
});
|
||||
await waitFor(() => {
|
||||
const params = new URLSearchParams(result.current.search);
|
||||
expect(params.get('q')).toBe('alpha');
|
||||
expect(params.get('page')).toBe('6');
|
||||
});
|
||||
});
|
||||
|
||||
it('null filter clears the URL param', async () => {
|
||||
const { result } = renderWithRouter(['/flows?q=alpha']);
|
||||
act(() => result.current.update({ filter: null }));
|
||||
await waitFor(() => {
|
||||
expect(new URLSearchParams(result.current.search).has('q')).toBe(false);
|
||||
});
|
||||
});
|
||||
|
||||
it('coalescence resolves replace conflict in favour of push (intentional history wins)', async () => {
|
||||
// setFilter requests replace, setPage requests push. The merged
|
||||
// navigation should push, so back-button can step out of the new
|
||||
// filter+page combination.
|
||||
const { result } = renderWithRouter(['/flows']);
|
||||
act(() => {
|
||||
result.current.setFilter('alpha'); // replace
|
||||
result.current.setPage(5); // push
|
||||
});
|
||||
await waitFor(() => {
|
||||
expect(new URLSearchParams(result.current.search).get('q')).toBe('alpha');
|
||||
});
|
||||
// History assertion is implicit — we can't easily inspect the entry
|
||||
// stack from `MemoryRouter`, but the merged URL has both params
|
||||
// which proves coalescence happened.
|
||||
});
|
||||
});
|
||||
|
||||
describe('useTableState — storage roundtrip', () => {
|
||||
it('restores `?q=` from storage when the URL is empty', async () => {
|
||||
localStorage.setItem(STORAGE_KEY, JSON.stringify({ filter: 'alpha' }));
|
||||
const { result } = renderWithRouter(['/flows']);
|
||||
await waitFor(() => {
|
||||
expect(new URLSearchParams(result.current.search).get('q')).toBe('alpha');
|
||||
});
|
||||
});
|
||||
|
||||
it('does not restore when the URL already has `?q=`', async () => {
|
||||
localStorage.setItem(STORAGE_KEY, JSON.stringify({ filter: 'stored' }));
|
||||
const { result } = renderWithRouter(['/flows?q=urlwin']);
|
||||
await act(async () => Promise.resolve());
|
||||
expect(result.current.filter).toBe('urlwin');
|
||||
});
|
||||
|
||||
it('persists the URL filter into storage once typing settles', async () => {
|
||||
const { result } = renderWithRouter(['/flows']);
|
||||
act(() => result.current.setFilter('alpha'));
|
||||
await waitFor(() => {
|
||||
expect(readStoredFilter(STORAGE_KEY)).toBe('alpha');
|
||||
});
|
||||
});
|
||||
|
||||
it('clears the storage entry when the filter is set back to empty', async () => {
|
||||
localStorage.setItem(STORAGE_KEY, JSON.stringify({ filter: 'alpha' }));
|
||||
const { result } = renderWithRouter(['/flows?q=alpha']);
|
||||
act(() => result.current.setFilter(''));
|
||||
await waitFor(() => {
|
||||
expect(readStoredFilter(STORAGE_KEY)).toBeNull();
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('useTableState — debounced filter', () => {
|
||||
it('debouncedFilter eventually catches up with filter', async () => {
|
||||
const { result } = renderWithRouter(['/flows'], { debounceMs: SHORT_DEBOUNCE_MS });
|
||||
act(() => result.current.setFilter('alpha'));
|
||||
await waitFor(() => {
|
||||
expect(result.current.debouncedFilter).toBe('alpha');
|
||||
});
|
||||
});
|
||||
});
|
||||
|
||||
describe('useTableState — `?page=1` canonicalization', () => {
|
||||
it('rewrites `?page=1` to a clean URL on mount', async () => {
|
||||
const { result } = renderWithRouter(['/flows?page=1']);
|
||||
await waitFor(() => {
|
||||
expect(new URLSearchParams(result.current.search).has('page')).toBe(false);
|
||||
});
|
||||
expect(result.current.pageIndex).toBe(0);
|
||||
});
|
||||
});
|
||||
@@ -0,0 +1,323 @@
|
||||
import { useCallback, useEffect, useMemo, useRef } from 'react';
|
||||
import { useSearchParams } from 'react-router-dom';
|
||||
|
||||
import { useDebouncedValue } from '@/hooks/use-debounced-value';
|
||||
import { useEffectAfterMount } from '@/hooks/use-effect-after-mount';
|
||||
import { usePageStorageKeys } from '@/hooks/use-page-storage-keys';
|
||||
import { loadTableState, updateTableState } from '@/lib/table-state';
|
||||
import { URL_PARAMS } from '@/lib/url-params';
|
||||
|
||||
/**
|
||||
* Atomic partial update for the table's URL state. All fields are optional;
|
||||
* `null` clears the corresponding param. `replace` controls whether the
|
||||
* navigation creates a new history entry. The whole patch is applied in
|
||||
* a single `setSearchParams` call so multi-field updates never race.
|
||||
*/
|
||||
interface TableStateUpdate {
|
||||
/** New filter value, `null`/`''` to clear. */
|
||||
filter?: null | string;
|
||||
/** New 0-based page index. Values `<= 0` clear the param. */
|
||||
pageIndex?: number;
|
||||
/** Pass `true` to navigate without adding a history entry. */
|
||||
replace?: boolean;
|
||||
}
|
||||
|
||||
interface UseTableStateOptions {
|
||||
/**
|
||||
* Whether `setFilter` should also drop `?page=` from the URL. Pages that
|
||||
* pair the URL filter with URL pagination need this so the user doesn't
|
||||
* end up "on page 5 of nothing" after narrowing the result set. Pages
|
||||
* that have no `?page=` to begin with can leave the default — deleting a
|
||||
* non-existent param is a no-op.
|
||||
*/
|
||||
clearPageOnFilterChange?: boolean;
|
||||
debounceMs?: number;
|
||||
/** Query string param for the filter value. Default `URL_PARAMS.QUERY` (`'q'`). */
|
||||
filterParamName?: string;
|
||||
/** Query string param for the 1-based page number. Default `URL_PARAMS.PAGE` (`'page'`). */
|
||||
pageParamName?: string;
|
||||
/**
|
||||
* Stable storage key for persisting the filter value (a fresh tab without
|
||||
* `?<filterParamName>=` resumes from here). Defaults to
|
||||
* `usePageStorageKeys().table` — i.e. `table_4_<current pathname>`.
|
||||
*/
|
||||
storageKey?: string;
|
||||
}
|
||||
|
||||
interface UseTableStateResult {
|
||||
debouncedFilter: string;
|
||||
filter: string;
|
||||
pageIndex: number;
|
||||
resetFilter: () => void;
|
||||
setFilter: (value: string) => void;
|
||||
setPage: (pageIndex: number) => void;
|
||||
/**
|
||||
* Atomic multi-field update. Prefer this when changing both `filter` and
|
||||
* `pageIndex` from the same event (or when adding more fields in the
|
||||
* future): all changes land in a single `setSearchParams` call, so the
|
||||
* race between two consecutive top-level updaters can never happen.
|
||||
*/
|
||||
update: (patch: TableStateUpdate) => void;
|
||||
}
|
||||
|
||||
/**
|
||||
* Unified URL + storage state for tables.
|
||||
*
|
||||
* Replaces the split `useTableQueryFilter` / `usePagination` pair. The split
|
||||
* design suffered from a batching race: react-router v6 feeds every
|
||||
* functional `setSearchParams(updater)` queued in a single tick the same
|
||||
* pre-batch snapshot, so a `setFilter` + `setPage` pair (e.g. a debounced
|
||||
* filter commit landing alongside a paging click) would collapse — the
|
||||
* second write erased the first and `q` was lost. Funnelling every URL
|
||||
* write through a single `update` here removes the race by construction:
|
||||
* there is never more than one in-flight `setSearchParams` per logical
|
||||
* intent. The few cases where two intents still fire in the same tick
|
||||
* (e.g. an external effect mutating the URL while we batch our own write)
|
||||
* read the freshest URL via `window.location.search`, with a ref-stashed
|
||||
* react-router snapshot as the fallback for `MemoryRouter`-based tests.
|
||||
*
|
||||
* Read-only siblings (detail pages reading the list's filter without
|
||||
* mutating it) should keep using `useTableQueryFilterReader` — no shared
|
||||
* URL writes, no race.
|
||||
*/
|
||||
export const useTableState = (options: UseTableStateOptions = {}): UseTableStateResult => {
|
||||
const {
|
||||
clearPageOnFilterChange = true,
|
||||
debounceMs = 200,
|
||||
filterParamName = URL_PARAMS.QUERY,
|
||||
pageParamName = URL_PARAMS.PAGE,
|
||||
storageKey: explicitStorageKey,
|
||||
} = options;
|
||||
|
||||
const [searchParams, setSearchParams] = useSearchParams();
|
||||
const { table: defaultStorageKey } = usePageStorageKeys();
|
||||
const storageKey = explicitStorageKey ?? defaultStorageKey;
|
||||
|
||||
const filter = searchParams.get(filterParamName) ?? '';
|
||||
const debouncedFilter = useDebouncedValue(filter, debounceMs);
|
||||
const pageIndex = useMemo(() => {
|
||||
const raw = searchParams.get(pageParamName);
|
||||
|
||||
if (!raw) {
|
||||
return 0;
|
||||
}
|
||||
|
||||
const parsed = Number.parseInt(raw, 10);
|
||||
|
||||
return Number.isFinite(parsed) ? Math.max(0, parsed - 1) : 0;
|
||||
}, [pageParamName, searchParams]);
|
||||
|
||||
// Sync a ref to the latest committed `searchParams` on every render.
|
||||
// Used as the `MemoryRouter` fallback below — that environment doesn't
|
||||
// sync its in-memory history to `window.location`. Mutating the ref in
|
||||
// render is safe because we only read it from event callbacks
|
||||
// (`update`), never during render; `useEffect`-based sync (a-la
|
||||
// `useLatestRef`) lags one commit, which is exactly the gap that
|
||||
// re-introduces the race we're trying to eliminate.
|
||||
const searchParamsReference = useRef(searchParams);
|
||||
// eslint-disable-next-line react-hooks/refs
|
||||
searchParamsReference.current = searchParams;
|
||||
|
||||
/**
|
||||
* Read the freshest possible `URLSearchParams` for the next write.
|
||||
*
|
||||
* Under `BrowserRouter` (production), `window.location.search` reflects
|
||||
* the URL bar one frame ahead of react-router's internal snapshot when
|
||||
* batched updates are in flight — that's the seam we exploit to merge
|
||||
* multi-source URL mutations safely. Under `MemoryRouter` (tests)
|
||||
* `window.location` doesn't track the in-memory history, so we fall
|
||||
* back to the rendered react-router snapshot.
|
||||
*/
|
||||
const readLatestParams = useCallback((): URLSearchParams => {
|
||||
const fromWindow = typeof window !== 'undefined' ? window.location.search : '';
|
||||
|
||||
return fromWindow ? new URLSearchParams(fromWindow) : new URLSearchParams(searchParamsReference.current);
|
||||
}, []);
|
||||
|
||||
// Canonicalize `?<pageParamName>=1` away. The first page is the default
|
||||
// URL, so two URLs (`/flows` vs `/flows?page=1`) would otherwise denote
|
||||
// the same view and split the history stack. Idempotent: once the param
|
||||
// is removed the effect re-runs and immediately exits the early return.
|
||||
useEffect(() => {
|
||||
if (searchParams.get(pageParamName) !== '1') {
|
||||
return;
|
||||
}
|
||||
|
||||
const next = readLatestParams();
|
||||
|
||||
next.delete(pageParamName);
|
||||
setSearchParams(next, { replace: true });
|
||||
}, [pageParamName, readLatestParams, searchParams, setSearchParams]);
|
||||
|
||||
// Replay the persisted filter into the URL when (a) the URL doesn't
|
||||
// already carry one, and (b) the storage has a non-empty value. Run
|
||||
// exactly once per storageKey rotation — `restoredForKeyReference`
|
||||
// guards against repeating the replay on every render.
|
||||
const restoredForKeyReference = useRef<null | string>(null);
|
||||
|
||||
useEffect(() => {
|
||||
if (restoredForKeyReference.current === storageKey) {
|
||||
return;
|
||||
}
|
||||
|
||||
restoredForKeyReference.current = storageKey;
|
||||
|
||||
if (filter.length > 0) {
|
||||
// URL already has a value — that beats storage. Mirror it back
|
||||
// into storage so a fresh tab without `?q=` resumes from this
|
||||
// intent (shared filtered links).
|
||||
updateTableState(storageKey, { filter });
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
const stored = loadTableState(storageKey).filter;
|
||||
|
||||
if (!stored || stored.length === 0) {
|
||||
return;
|
||||
}
|
||||
|
||||
const next = readLatestParams();
|
||||
|
||||
next.set(filterParamName, stored);
|
||||
// Replace, not push: restoring prior state shouldn't add a history
|
||||
// entry the user didn't ask for.
|
||||
setSearchParams(next, { replace: true });
|
||||
}, [filter, filterParamName, readLatestParams, setSearchParams, storageKey]);
|
||||
|
||||
// Persist the URL filter into storage on every commit. Skipping the
|
||||
// first render is intentional: a fresh-mount empty `filter` would wipe
|
||||
// a freshly-restored storage entry before the restore effect above has
|
||||
// had a chance to replay it into the URL.
|
||||
useEffectAfterMount(() => {
|
||||
updateTableState(storageKey, { filter: filter.length > 0 ? filter : undefined });
|
||||
}, [filter, storageKey]);
|
||||
|
||||
// Coalesce every `update(...)` call fired in the same microtask into one
|
||||
// `setSearchParams`. The first call schedules the flush; subsequent calls
|
||||
// merge their patch into the pending buffer instead of issuing their own
|
||||
// navigation. This is what makes race conditions impossible *by
|
||||
// construction*: it doesn't matter whether two updates come from the
|
||||
// same event handler, from two effects, or from a debounced commit
|
||||
// landing alongside a synchronous click — only one navigation happens,
|
||||
// with both fields applied.
|
||||
const pendingPatchReference = useRef<null | {
|
||||
filter: null | string | undefined;
|
||||
filterPresent: boolean;
|
||||
pageIndex: number | undefined;
|
||||
pageIndexPresent: boolean;
|
||||
// Replace resolution: any push-intent (`replace: false`) wins, so
|
||||
// intentional history entries (paging clicks) survive coalescence
|
||||
// with replace-only updates (filter typing).
|
||||
replace: boolean;
|
||||
}>(null);
|
||||
|
||||
const update = useCallback(
|
||||
(patch: TableStateUpdate) => {
|
||||
const filterPresent = 'filter' in patch;
|
||||
const pageIndexPresent = 'pageIndex' in patch;
|
||||
const requestedReplace = patch.replace ?? false;
|
||||
|
||||
if (pendingPatchReference.current === null) {
|
||||
pendingPatchReference.current = {
|
||||
filter: patch.filter,
|
||||
filterPresent,
|
||||
pageIndex: patch.pageIndex,
|
||||
pageIndexPresent,
|
||||
replace: requestedReplace,
|
||||
};
|
||||
|
||||
queueMicrotask(() => {
|
||||
const merged = pendingPatchReference.current;
|
||||
|
||||
pendingPatchReference.current = null;
|
||||
|
||||
if (merged === null) {
|
||||
return;
|
||||
}
|
||||
|
||||
const next = readLatestParams();
|
||||
|
||||
if (merged.filterPresent) {
|
||||
if (!merged.filter) {
|
||||
next.delete(filterParamName);
|
||||
} else {
|
||||
next.set(filterParamName, merged.filter);
|
||||
}
|
||||
}
|
||||
|
||||
if (merged.pageIndexPresent) {
|
||||
const newIndex = merged.pageIndex ?? 0;
|
||||
|
||||
if (newIndex <= 0) {
|
||||
next.delete(pageParamName);
|
||||
} else {
|
||||
next.set(pageParamName, String(newIndex + 1));
|
||||
}
|
||||
}
|
||||
|
||||
setSearchParams(next, { replace: merged.replace });
|
||||
});
|
||||
|
||||
return;
|
||||
}
|
||||
|
||||
// Merge into the in-flight patch — the queued microtask will see
|
||||
// the fused result.
|
||||
if (filterPresent) {
|
||||
pendingPatchReference.current.filter = patch.filter;
|
||||
pendingPatchReference.current.filterPresent = true;
|
||||
}
|
||||
|
||||
if (pageIndexPresent) {
|
||||
pendingPatchReference.current.pageIndex = patch.pageIndex;
|
||||
pendingPatchReference.current.pageIndexPresent = true;
|
||||
}
|
||||
|
||||
if (!requestedReplace) {
|
||||
pendingPatchReference.current.replace = false;
|
||||
}
|
||||
},
|
||||
[filterParamName, pageParamName, readLatestParams, setSearchParams],
|
||||
);
|
||||
|
||||
const setFilter = useCallback(
|
||||
(value: string) => {
|
||||
// Typing keystrokes commit through here as well — we don't want
|
||||
// intermediate filter values cluttering the history stack, so
|
||||
// every filter change is a `replace`. The implicit page reset
|
||||
// is bundled into the same atomic update so the resulting URL
|
||||
// is consistent in one transition rather than two.
|
||||
update({
|
||||
filter: value.length === 0 ? null : value,
|
||||
pageIndex: clearPageOnFilterChange ? 0 : undefined,
|
||||
replace: true,
|
||||
});
|
||||
},
|
||||
[clearPageOnFilterChange, update],
|
||||
);
|
||||
|
||||
const setPage = useCallback(
|
||||
(newPageIndex: number) => {
|
||||
// Paging is an intentional user action — push, not replace, so
|
||||
// back-button steps through the visited pages.
|
||||
update({ pageIndex: newPageIndex });
|
||||
},
|
||||
[update],
|
||||
);
|
||||
|
||||
const resetFilter = useCallback(() => setFilter(''), [setFilter]);
|
||||
|
||||
return useMemo(
|
||||
() => ({
|
||||
debouncedFilter,
|
||||
filter,
|
||||
pageIndex,
|
||||
resetFilter,
|
||||
setFilter,
|
||||
setPage,
|
||||
update,
|
||||
}),
|
||||
[debouncedFilter, filter, pageIndex, resetFilter, setFilter, setPage, update],
|
||||
);
|
||||
};
|
||||
@@ -31,8 +31,7 @@ import { StatusCard } from '@/components/ui/status-card';
|
||||
import { Toggle } from '@/components/ui/toggle';
|
||||
import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip';
|
||||
import { ResultType, StatusType, type TerminalFragmentFragment, useRenameFlowMutation } from '@/graphql/types';
|
||||
import { usePagination } from '@/hooks/use-pagination';
|
||||
import { useTableQueryFilter } from '@/hooks/use-table-query-filter';
|
||||
import { useTableState } from '@/hooks/use-table-state';
|
||||
import { mergeHrefWithSearchParams } from '@/lib/url-params';
|
||||
import { useFavorites } from '@/providers/favorites-provider';
|
||||
import { type Flow, useFlows } from '@/providers/flows-provider';
|
||||
@@ -92,8 +91,7 @@ const Flows = () => {
|
||||
const editingInputRef = useRef<HTMLInputElement>(null);
|
||||
const [renameFlowMutation, { loading: isRenameLoading }] = useRenameFlowMutation();
|
||||
|
||||
const { filter, setFilter } = useTableQueryFilter();
|
||||
const { pageIndex: currentPage, setPage: handlePageChange } = usePagination();
|
||||
const { filter, pageIndex: currentPage, setFilter, setPage: handlePageChange } = useTableState();
|
||||
|
||||
const handleFlowOpen = useCallback(
|
||||
(flowId: string) => {
|
||||
|
||||
@@ -26,7 +26,7 @@ import { Separator } from '@/components/ui/separator';
|
||||
import { SidebarTrigger } from '@/components/ui/sidebar';
|
||||
import { StatusCard } from '@/components/ui/status-card';
|
||||
import { KnowledgeDocType } from '@/graphql/types';
|
||||
import { useTableQueryFilter } from '@/hooks/use-table-query-filter';
|
||||
import { useTableState } from '@/hooks/use-table-state';
|
||||
import { mergeHrefWithSearchParams } from '@/lib/url-params';
|
||||
import { type Knowledge, useKnowledges } from '@/providers/knowledges-provider';
|
||||
|
||||
@@ -63,7 +63,7 @@ const Knowledges = () => {
|
||||
const [isRenameLoading, setIsRenameLoading] = useState(false);
|
||||
const editingInputRef = useRef<HTMLInputElement>(null);
|
||||
|
||||
const { filter, setFilter } = useTableQueryFilter();
|
||||
const { filter, setFilter } = useTableState();
|
||||
|
||||
const handleOpen = useCallback(
|
||||
(id: string) => {
|
||||
|
||||
@@ -51,8 +51,7 @@ import {
|
||||
useDeleteApiTokenMutation,
|
||||
useUpdateApiTokenMutation,
|
||||
} from '@/graphql/types';
|
||||
import { usePagination } from '@/hooks/use-pagination';
|
||||
import { useTableQueryFilter } from '@/hooks/use-table-query-filter';
|
||||
import { useTableState } from '@/hooks/use-table-state';
|
||||
import { cn } from '@/lib/utils';
|
||||
import { baseUrl } from '@/models/api';
|
||||
|
||||
@@ -208,8 +207,7 @@ const SettingsAPITokens = () => {
|
||||
const editingInputRef = useRef<HTMLInputElement>(null);
|
||||
const creatingInputRef = useRef<HTMLInputElement>(null);
|
||||
|
||||
const { pageIndex: currentPage, setPage: handlePageChange } = usePagination();
|
||||
const { filter, setFilter } = useTableQueryFilter();
|
||||
const { filter, pageIndex: currentPage, setFilter, setPage: handlePageChange } = useTableState();
|
||||
|
||||
useApiTokenCreatedSubscription({
|
||||
onData: ({ client }) => {
|
||||
|
||||
@@ -34,8 +34,7 @@ import {
|
||||
import { StatusCard } from '@/components/ui/status-card';
|
||||
import { Tooltip, TooltipContent, TooltipTrigger } from '@/components/ui/tooltip';
|
||||
import { ProviderType, useDeleteProviderMutation, useSettingsProvidersQuery } from '@/graphql/types';
|
||||
import { usePagination } from '@/hooks/use-pagination';
|
||||
import { useTableQueryFilter } from '@/hooks/use-table-query-filter';
|
||||
import { useTableState } from '@/hooks/use-table-state';
|
||||
type Provider = ProviderConfigFragmentFragment;
|
||||
|
||||
const providerIcons: Record<ProviderType, React.ComponentType<any>> = {
|
||||
@@ -134,8 +133,7 @@ const SettingsProviders = () => {
|
||||
const [deletingProvider, setDeletingProvider] = useState<null | Provider>(null);
|
||||
const navigate = useNavigate();
|
||||
|
||||
const { pageIndex: currentPage, setPage: handlePageChange } = usePagination();
|
||||
const { filter, setFilter } = useTableQueryFilter();
|
||||
const { filter, pageIndex: currentPage, setFilter, setPage: handlePageChange } = useTableState();
|
||||
|
||||
const handleProviderDelete = useCallback(
|
||||
async (providerId: string | undefined) => {
|
||||
|
||||
@@ -22,7 +22,7 @@ import {
|
||||
import { Separator } from '@/components/ui/separator';
|
||||
import { SidebarTrigger } from '@/components/ui/sidebar';
|
||||
import { StatusCard } from '@/components/ui/status-card';
|
||||
import { useTableQueryFilter } from '@/hooks/use-table-query-filter';
|
||||
import { useTableState } from '@/hooks/use-table-state';
|
||||
import { mergeHrefWithSearchParams } from '@/lib/url-params';
|
||||
import { type Template, useTemplates } from '@/providers/templates-provider';
|
||||
|
||||
@@ -37,7 +37,7 @@ const Templates = () => {
|
||||
const [isRenameLoading, setIsRenameLoading] = useState(false);
|
||||
const editingInputRef = useRef<HTMLInputElement>(null);
|
||||
|
||||
const { filter, setFilter } = useTableQueryFilter();
|
||||
const { filter, setFilter } = useTableState();
|
||||
|
||||
const handleTemplateOpen = useCallback(
|
||||
(templateId: string) => {
|
||||
|
||||
Reference in New Issue
Block a user