fix(web): bound scroll restoration cache by collapsing keys to pathname#632
Merged
Merged
Conversation
Fixes tiann#611. The default getScrollRestorationKey returns a per-history-entry __TSR_key, which makes the cache grow unbounded across navigations and eventually exhausts sessionStorage, triggering QuotaExceededError that blocks React commit. Collapse to pathname so cache growth is bounded by distinct pathnames visited rather than per-history-entry keys.
There was a problem hiding this comment.
Findings
- [Major] Search-param routes now share one scroll bucket —
/sessions/$sessionId/fileuses?path=...as the file identity (web/src/router.tsx:578,web/src/routes/sessions/file.tsx:407), but the new key returns onlylocation.pathname. After visiting/scrolled file A, opening/scrolled file B, then using browser Back to file A, TanStack restores the shared/sessions/<id>/filebucket instead of file A's position. That regresses scroll restoration for file/diff navigation while fixing the quota issue.web/src/router.tsx:673
Suggested fix:Keep the quota fix by pairing the semantic keys above with a capped/LRU scroll cache or quota-safe storage pruning, rather than collapsing every search-param route to the same content bucket.getScrollRestorationKey: (location) => { const search = location.search as { path?: unknown; tab?: unknown; machineId?: unknown } if (location.pathname.endsWith('/file') && typeof search.path === 'string') { return `${location.pathname}?path=${search.path}` } if (location.pathname.endsWith('/files') && search.tab === 'directories') { return `${location.pathname}?tab=directories` } return location.pathname },
Questions
- None.
Summary
- Review mode: initial
- One issue found: the cache key is bounded, but it also merges distinct file-route contents that are differentiated only by search params.
Testing
- Not run (automation; review-only, PR code not executed).
HAPI Bot
… keys Addresses bot review feedback on tiann#632. Pathname-only collapse merged the file diff view and the directories tab into a single cache entry per session, losing per-file and per-tab scroll positions. Switch to a dedicated key function that keeps `?path=` for `/sessions/$id/file` and `?tab=directories` for `/sessions/$id/files`, so scroll positions are remembered per file and per non-default tab. Other routes still collapse to pathname.
Addresses bot review feedback on tiann#632. Per-route keys still let the cache grow with the number of files opened in `/sessions/$id/file?path=...`, so a long browsing session can still reach the ~5 MB sessionStorage quota. Install a wrapper at bootstrap that intercepts the QuotaExceededError on the scroll restoration storage key only: it prunes the oldest entries (by JSON property insertion order — recently visited routes are kept) and retries the write once; if the value is not valid JSON or the retry still fails, the key is cleared so navigation continues without crashing.
There was a problem hiding this comment.
Findings
- [Major] File scroll keys merge staged and unstaged diffs —
/sessions/$sessionId/filecarriesstagedthrough search validation and uses it in the diff query (web/src/router.tsx:573,web/src/routes/sessions/file.tsx:416), but the new key only includespath. Opening the staged and unstaged views for the same file now writes both scroll positions to the same bucket, so Back/Forward can restore the other diff's position.
Suggested fix:if (FILE_ROUTE.test(location.pathname) && typeof search.path === 'string') { const stagedSuffix = search.staged === true ? '&staged=true' : '' return `${location.pathname}?path=${search.path}${stagedSuffix}` }
Questions
- None.
Summary
- Review mode: follow-up after new commits
- The prior file-path finding is mostly addressed, but the file route still merges staged and unstaged diff identities for the same path.
Testing
- Not run (automation; review-only, PR code not executed).
HAPI Bot
Differentiate staged vs unstaged diffs for the same file path so each view keeps its own scroll position. `staged=true` adds an explicit suffix; `staged=false` and absent both collapse to the unstaged bucket, matching how the file route renders by default. Addresses bot review feedback on tiann#632.
There was a problem hiding this comment.
Findings
- [Minor]
/browsescroll state is shared across machine-scoped views —/browsevalidatesmachineIdfrom search and passes it intoWorkspaceBrowser, so different machines can render completely different directory lists for the same pathname (web/src/router.tsx:486,web/src/router.tsx:633). The new fallback key stores all of those scroll positions under/browse, so opening one machine's browser can restore the scroll offset from another machine's workspace list.
Suggested fix:const search = location.search as { path?: unknown; staged?: unknown; tab?: unknown; machineId?: unknown } if (location.pathname === '/browse' && typeof search.machineId === 'string') { return `${location.pathname}?machineId=${search.machineId}` }
Questions
- None.
Summary
- Review mode: follow-up after new commits
- The prior staged/unstaged file diff collision is fixed. Remaining risk is one search-scoped route still falling through to the pathname-only bucket.
Testing
- Not run (automation; review-only, PR code not executed).
HAPI Bot
Different machines render completely different directory trees in WorkspaceBrowser, so /browse without machineId differentiation restores scroll positions across machines. Addresses bot review feedback on tiann#632.
There was a problem hiding this comment.
Findings
- None.
Questions
- None.
Summary
- Review mode: follow-up after new commits
- No issues found in the updated diff. The prior
/browsemachine-scoped scroll key issue is addressed, and the new guard/key tests cover the reviewed edge cases. Residual risk: the guard depends on TanStack Router's internaltsr-scroll-restoration-v1_3storage key, so a future router upgrade could require updating that constant.
Testing
- Not run (automation; review-only, PR code not executed).
HAPI Bot
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
After navigating through several sessions, the browser console shows:
This blocks the React commit and requires a hard reload to recover. Fixes #611.
The root cause is that TanStack Router's default
getScrollRestorationKeyreturnslocation.state.__TSR_key, a unique string generated for every history entry. Each navigation creates a new cache entry, so thesessionStoragevalue grows without bound until it exhausts the ~5 MB limit.(Note: the linked issue mentions
localStorage; the actual storage issessionStorage. Same Web Storage quota applies, so the symptom is identical.)Solution
Pass a
getScrollRestorationKeyfunction tocreateRouterthat collapses the key tolocation.pathname. This reduces cache growth from O(every navigation) to O(distinct pathnames visited), which keeps a typical session comfortably under the quota.Trade-offs
Revisiting the same pathname (e.g. switching back to a session) now restores the last scroll position for that route — generally nicer UX than always starting at the top. URLs that differ only in search params share a single cache entry, so per-query scroll positions are no longer tracked. In particular, on
/sessions/$sessionId/file?path=..., switching between files within a session no longer remembers a per-file scroll position; the diff view always opens at the top.Tests
bun run typecheck: clean.bun run test:web: 60 files / 417 tests passed.__TSR_keystrings); with the pathname key, 28+ navigations across 5 sessions produced 6 entries (one per distinct pathname). FillingsessionStorageto the quota beforehand no longer throws on subsequent navigation.Follow-up
A boot-time size guard (clearing the key when the existing cache already exceeds a threshold) would help users whose
sessionStorageis already at quota before this fix is deployed. A per-route key scheme that keeps query params for routes whose identity is search-param-defined (e.g. the file diff view) would restore per-file scroll memory. Both are out of scope for this PR.