fix(scroll-restoration): capture restoreKey at scroll-event time to prevent throttle race condition#7046
fix(scroll-restoration): capture restoreKey at scroll-event time to prevent throttle race condition#7046sleitor wants to merge 2 commits intoTanStack:mainfrom
Conversation
…ttle race condition When the onScroll throttle executed, it read router.state.location at execution time (up to 100ms after the scroll event fired). If the user scrolled and navigated within that 100ms window, the old page's scroll position was saved under the new page's cache key. Fix 1: Capture restoreKey eagerly at scroll-event time by separating the event listener from a dedicated throttled save function that receives the key as an argument. Fix 2: Clear any stale cache entry for the destination key before calling restoreScroll() in the onRendered subscriber, guarding against browser-generated scroll events during DOM transitions. Fixes TanStack#7040
📝 WalkthroughWalkthroughThis PR fixes a race condition in scroll restoration where throttled scroll event handlers read route keys at execution time (up to 100ms later) instead of at event time, causing old page scroll positions to be saved under new route keys. Two complementary fixes address this: capturing the key immediately when the scroll event fires, and clearing stale cache entries before scroll restoration executes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 11m | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 46s | View ↗ |
☁️ Nx Cloud last updated this comment at 2026-03-26 10:52:23 UTC
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/router-core/src/scroll-restoration.ts (3)
414-417: Remove duplicate JSDoc comment.There are two JSDoc comments for
handleHashScroll. Keep the shorter one since the longer one is also marked@private.🧹 Proposed cleanup
-/** - * `@private` - * Handles hash-based scrolling after navigation completes. - * To be used in framework-specific <Transitioner> components during the onResolved event. - * - * Provides hash scrolling for programmatic navigation when default browser handling is prevented. - * `@param` router The router instance containing current location and state - */ /** * `@private` * Handles hash-based scrolling after navigation completes. * To be used in framework-specific Transitioners. */ export function handleHashScroll(router: AnyRouter) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/scroll-restoration.ts` around lines 414 - 417, There are duplicate JSDoc comments for the function handleHashScroll; remove the longer/extra JSDoc block and keep the shorter `@private` comment so only one JSDoc remains for handleHashScroll, ensuring references to handleHashScroll in scroll-restoration.ts are left unchanged.
218-219: Remove duplicate JSDoc comment.There are two identical JSDoc comments for
setupScrollRestoration.🧹 Proposed cleanup
-/** Setup global listeners and hooks to support scroll restoration. */ /** Setup global listeners and hooks to support scroll restoration. */ export function setupScrollRestoration(router: AnyRouter, force?: boolean) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/scroll-restoration.ts` around lines 218 - 219, There are two identical JSDoc comment lines above setupScrollRestoration; remove the duplicate so only one JSDoc comment remains for the setupScrollRestoration function (locate the duplicated /** Setup global listeners and hooks to support scroll restoration. */ lines and delete the redundant one).
37-39: Remove duplicate JSDoc comments.There are three identical/similar JSDoc comments for
storageKey. Only one is needed.🧹 Proposed cleanup
-/** SessionStorage key used to persist scroll restoration state. */ -/** SessionStorage key used to store scroll positions across navigations. */ /** SessionStorage key used to store scroll positions across navigations. */ export const storageKey = 'tsr-scroll-restoration-v1_3'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/scroll-restoration.ts` around lines 37 - 39, Remove the duplicated JSDoc comments above the storageKey declaration: keep a single clear JSDoc line (e.g., "SessionStorage key used to store scroll positions across navigations.") and delete the other two redundant comment lines so only one JSDoc comment remains immediately preceding the storageKey symbol.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/router-core/src/scroll-restoration.ts`:
- Around line 414-417: There are duplicate JSDoc comments for the function
handleHashScroll; remove the longer/extra JSDoc block and keep the shorter
`@private` comment so only one JSDoc remains for handleHashScroll, ensuring
references to handleHashScroll in scroll-restoration.ts are left unchanged.
- Around line 218-219: There are two identical JSDoc comment lines above
setupScrollRestoration; remove the duplicate so only one JSDoc comment remains
for the setupScrollRestoration function (locate the duplicated /** Setup global
listeners and hooks to support scroll restoration. */ lines and delete the
redundant one).
- Around line 37-39: Remove the duplicated JSDoc comments above the storageKey
declaration: keep a single clear JSDoc line (e.g., "SessionStorage key used to
store scroll positions across navigations.") and delete the other two redundant
comment lines so only one JSDoc comment remains immediately preceding the
storageKey symbol.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 812e567e-fe98-4f7f-bde7-8c9e4e6529cb
📒 Files selected for processing (2)
.changeset/fix-scroll-restoration-race-7040.mdpackages/router-core/src/scroll-restoration.ts
|
closed in favor of #7042 |

Summary
Fixes #7040
Fixes a race condition in where the throttle read
router.state.locationat execution time (up to 100ms after the scroll event fired), rather than at the time of the event.Root Cause
If a user scrolled and then immediately clicked a
<Link>within the 100ms throttle window,router.state.locationcould have already changed to the new route when the throttle finally fired. The old page's scroll position would then be saved under the new page's cache key, causing the new page to scroll partway down on arrival.Fix 1: Capture
restoreKeyat event timeSplit
onScrollinto an immediate event listener and a throttled save function. The event listener capturesrestoreKeyeagerly, then passes it as an argument to the throttled save. Since the throttle preserves the arguments from the first call in each window, the key always reflects the route active at the time of the scroll.Fix 2: Clear stale cache before
restoreScroll()In the
onRenderedsubscriber, delete any existing cache entry for the destination key before callingrestoreScroll(). This guards against browser-generated scroll events that can fire during DOM transitions afterrouter.state.locationhas already advanced to the new route.Testing
The reporter confirmed the fix with a Playwright test suite. Before the fix, dumped sessionStorage showed game page keys with home page scroll positions. After the fix, the destination key always has
scrollY: 0for fresh forward navigations.Summary by CodeRabbit