fix: Keep a deep-linked page on the Library table - #384
Conversation
A cold load of /library?page=2 rendered page 0 and stripped `page` from the URL. Two independent causes, both live, and guarding either alone leaves the bug standing: - The page-clamp effect ran while `data` was still empty, where `pageCount` is 0 and says nothing about whether the requested page exists. It now returns early until rows arrive. - TanStack's `_autoResetPageIndex` queues onto a microtask that runs after the render which consumed the row model — by then the render-time clamp has already raised `pageIndex` to the requested page, so the reset's 0 differs from it, passes the pagination handler's guard, and writes `page: null`. `autoResetPageIndex` is now armed one render after `data` first becomes non-empty, which keeps today's reset-on-data-change while letting the initial arrival through. Gating on a ref read during render works because the option is read synchronously in `_autoResetPageIndex`, not inside the microtask. Adds a deep-link regression test plus coverage for the two behaviours that must survive: an out-of-range `?page=99` settles onto the last page, and a later row-set change still resets to page 0. Wayfinder #381 (map #353); mechanism established on #372. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019TGrvFNfYXfV5U1aMMZvHb
🦋 Changeset detectedLatest commit: 1557dc0 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughChangesLibrary pagination
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Browser
participant SeriesTable
participant TanStackTable
Browser->>SeriesTable: Load deep-linked page parameter
SeriesTable->>TanStackTable: Render with empty data without resetting page
SeriesTable->>TanStackTable: Apply pagination after data arrives
TanStackTable->>Browser: Preserve or clamp the page URL
Browser->>SeriesTable: Update the underlying row set
SeriesTable->>Browser: Reset the page query parameter
Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install failed. For unrecoverable errors, disable the tool in CodeRabbit configuration. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Resolves Ship the deep-linked-page pre-fix on SeriesTable on map Wayfinder: One useTableState hook for every table. Mechanism was established on #372; this ships it ahead of the
useTableStateseam.The bug
A cold load of
/library?page=2rendered page 0 and strippedpagefrom the URL. Bookmarks and shared links to any page but the first silently lost the page.Two causes, both live, neither sufficient alone
1. The clamp effect wrote on empty data (
SeriesTable.tsx:280). Before rows arrive,pageCountis 0, somaxPageis 0 and any requested page clamps to 0 — the effect then writespage: null. It now returns early whiledatais empty; once rows are known the clamp is meaningful again.2. Auto-reset lands one microtask too late.
_autoResetPageIndexqueues onto a microtask that runs after the render which consumed the row model. By then the render-time clamp has already raisedpageIndexto the requested page, so the reset's 0 differs from it, passesonPaginationChange's guard, and writespage: null.autoResetPageIndexis now armed one render afterdatafirst becomes non-empty:This preserves today's reset-on-data-change, which
autoResetPageIndex: falsewould silently drop. Gating on a ref read during render is sound becausetable.options.autoResetPageIndexis read synchronously inside_autoResetPageIndex(table-core/features/RowPagination.js:49), not inside the queued microtask.I verified independently that each half alone leaves the regression test failing — reverting either one reproduces the bug.
Why pre-fix rather than fix during the migration
The
autoResetPageIndexline transfers verbatim intouseTableState; only the effect guard is throwaway (the effect is deleted outright at migration). And the regression test can only be written against working code, which makes it the concrete parity artifact #365 needs while still blocked.Behaviour note
An out-of-range
?page=99renders the last page and the URL is rewritten to?page=3. #372's no URL tidying describes the post-migration state, where the effect is gone entirely; while the effect survives as the throwaway half, this stays as it is today. Captured as a test so the change is visible when the effect is deleted.Tests
Three cases in
frontend/tests/components/SeriesTable.test.tsx:?page=2with rows arriving after mount — page 2 renders andpageis never stripped from any URL the app commits?page=99settles onto the last pageFull frontend suite: 143 passed / 27 files.
typecheck,lint:frontend,lint:modernandlint:backendall clean.Scope
Deliberately excluded, per #372:
IssuesTable's stale-key decode andActivityPage.tsx:590's index-embedded row id. Those stay under the map's Where the two identity fixes ship.🤖 Generated with Claude Code
https://claude.ai/code/session_019TGrvFNfYXfV5U1aMMZvHb
Summary by CodeRabbit
/library?page=2after reload.