From 692d91cffaef08e34519f961697ccffbf495d2b2 Mon Sep 17 00:00:00 2001 From: Sergey Kozyrenko Date: Fri, 15 May 2026 00:46:17 +0700 Subject: [PATCH] fix(frontend): sync URL on pageSize change, label knowledge columns MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - DataTable: route handlePageSizeChange through handlePaginationChange so onPageChange fires when picking "All" on a high page drops pageIndex to 0. Without this, the URL kept a stale ?page=N while the display correctly clamped to "Page 1 of 1". - DataTable: reconcile effect in controlled mode now compares externalPageIndex (the URL source of truth) against safePageIndex instead of the internal mirror, which can drop to 0 via handlePageSizeChange and hide the URL mismatch from the previous comparison. - knowledges: add meta.columnMenuLabel to docType and question columns so the Columns dropdown reads "Type" / "Question" — same labels as the table headers — aligning with the Flags/Preview entries. - data-table.test: cover both pathways — out-of-range controlled pageIndex on mount, and pageSize=All from page 2. - vitest.setup: polyfill hasPointerCapture / setPointerCapture / releasePointerCapture so Radix Select interactions don't crash in jsdom. Co-Authored-By: Claude Opus 4.7 (1M context) --- .../src/components/ui/data-table.test.tsx | 55 +++++++++++++++++++ frontend/src/components/ui/data-table.tsx | 34 +++++++++--- frontend/src/pages/knowledges/knowledges.tsx | 2 + frontend/vitest.setup.ts | 15 +++++ 4 files changed, 97 insertions(+), 9 deletions(-) diff --git a/frontend/src/components/ui/data-table.test.tsx b/frontend/src/components/ui/data-table.test.tsx index 3ebe48c..e37f41b 100644 --- a/frontend/src/components/ui/data-table.test.tsx +++ b/frontend/src/components/ui/data-table.test.tsx @@ -206,6 +206,61 @@ describe('DataTable — sorting state persists to storage', () => { }); }); +describe('DataTable — controlled pageIndex reconciliation', () => { + it('asks the parent to clamp an out-of-range controlled pageIndex on mount', () => { + const onPageChange = vi.fn(); + + render( + + columns={COLUMNS} + data={ROWS} + onPageChange={onPageChange} + pageIndex={9} + />, + { wrapper: Wrapper }, + ); + + // 3 rows / default pageSize 10 → 1 page. The URL-driven pageIndex 9 + // is out of range, so the parent must be told to drop to 0 so the + // canonical URL no longer points past the dataset. + expect(onPageChange).toHaveBeenCalledWith(0); + }); + + it('clears the URL page when picking "All" on a high page in controlled mode', async () => { + const user = userEvent.setup(); + // 15 rows × default pageSize 10 → 2 pages; start on page 2. + const manyRows: Row[] = Array.from({ length: 15 }, (_, index) => ({ + id: String(index + 1), + name: `Row ${index + 1}`, + })); + const onPageChange = vi.fn(); + + render( + + columns={COLUMNS} + data={manyRows} + onPageChange={onPageChange} + pageIndex={1} + />, + { wrapper: Wrapper }, + ); + + // Initial mirror of the controlled pageIndex shouldn't trigger a + // reconcile — page 2 of 2 is in range. + expect(onPageChange).not.toHaveBeenCalled(); + + // Open the rows-per-page select and pick "All". + const trigger = screen.getByRole('combobox'); + await user.click(trigger); + const option = await screen.findByRole('option', { name: 'All' }); + await user.click(option); + + // "All" collapses the dataset to a single page; the parent has to + // hear about pageIndex 0 so `?page=2` is dropped from the URL. + expect(onPageChange).toHaveBeenCalledWith(0); + }); +}); + describe('DataTable — empty results', () => { it('does not render "Page 1 of 0" when there are no rows', () => { render( diff --git a/frontend/src/components/ui/data-table.tsx b/frontend/src/components/ui/data-table.tsx index 67a4139..166fe65 100644 --- a/frontend/src/components/ui/data-table.tsx +++ b/frontend/src/components/ui/data-table.tsx @@ -289,10 +289,6 @@ function DataTable({ } }, [externalPageIndex]); - const handlePageSizeChange = useCallback((newPageSize: number) => { - setPagination({ pageIndex: 0, pageSize: newPageSize }); - }, []); - const paginationReference = useLatestRef(pagination); const handlePaginationChange = useCallback( @@ -312,6 +308,18 @@ function DataTable({ [onPageChange, paginationReference], ); + // Route page-size changes through the same channel as TanStack's + // pagination mutations so `onPageChange` fires when the URL-driven + // pageIndex needs to drop to 0. Without this, picking "All" on a high + // page leaves `?page=5` stranded in the URL even though the display + // correctly clamps to "Page 1 of 1". + const handlePageSizeChange = useCallback( + (newPageSize: number) => { + handlePaginationChange({ pageIndex: 0, pageSize: newPageSize }); + }, + [handlePaginationChange], + ); + const table = useReactTable({ autoResetPageIndex: false, columns, @@ -370,21 +378,29 @@ function DataTable({ // pipeline, which React forbids during render. The early return makes the // effect a no-op on every render where the URL is already in range, so // the cost on the happy path is one comparison. + // + // Controlled mode compares the URL (`externalPageIndex`) — the canonical + // source of truth — rather than the internal mirror, because internal + // state can drop to a clamped value via `handlePageSizeChange` while the + // URL still points at the stale page. Uncontrolled mode keeps comparing + // the internal pageIndex since there's no other source. useEffect(() => { if (pageCount === 0) { return; } - if (pagination.pageIndex === safePageIndex) { + if (isPageControlled) { + if (externalPageIndex !== undefined && externalPageIndex !== safePageIndex) { + onPageChange?.(safePageIndex); + } + return; } - if (isPageControlled) { - onPageChange?.(safePageIndex); - } else { + if (pagination.pageIndex !== safePageIndex) { setPagination((previous) => ({ ...previous, pageIndex: safePageIndex })); } - }, [isPageControlled, onPageChange, pageCount, pagination.pageIndex, safePageIndex]); + }, [externalPageIndex, isPageControlled, onPageChange, pageCount, pagination.pageIndex, safePageIndex]); return (
diff --git a/frontend/src/pages/knowledges/knowledges.tsx b/frontend/src/pages/knowledges/knowledges.tsx index 3b4e38f..cb97b01 100644 --- a/frontend/src/pages/knowledges/knowledges.tsx +++ b/frontend/src/pages/knowledges/knowledges.tsx @@ -173,6 +173,7 @@ const Knowledges = () => { }, header: ({ column }) => , maxSize: 180, + meta: { columnMenuLabel: 'Type' }, minSize: 110, size: 130, }, @@ -209,6 +210,7 @@ const Knowledges = () => { ); }, header: ({ column }) => , + meta: { columnMenuLabel: 'Question' }, minSize: 180, size: 280, }, diff --git a/frontend/vitest.setup.ts b/frontend/vitest.setup.ts index 1077c3e..97f63d4 100644 --- a/frontend/vitest.setup.ts +++ b/frontend/vitest.setup.ts @@ -31,6 +31,21 @@ if (typeof globalThis.ResizeObserver === 'undefined') { globalThis.ResizeObserver = NoopResizeObserver as unknown as typeof ResizeObserver; } +// Radix Select gates pointer events behind `hasPointerCapture` / `setPointerCapture`, +// which jsdom doesn't implement. Without these no-ops, `userEvent.click` on a +// SelectTrigger throws "target.hasPointerCapture is not a function" mid-render. +if (typeof Element !== 'undefined' && !Element.prototype.hasPointerCapture) { + Element.prototype.hasPointerCapture = function hasPointerCapture() { + return false; + }; + Element.prototype.setPointerCapture = function setPointerCapture() { + /* no-op for jsdom */ + }; + Element.prototype.releasePointerCapture = function releasePointerCapture() { + /* no-op for jsdom */ + }; +} + // React Testing Library leaves rendered nodes attached to `document.body` // after each test. Without this, tests would leak DOM state into each other // — e.g. two `render()` calls would both end up on screen at once.