Skip to content

fix(scroll-restoration): save scroll position when navigating with resetScroll=false - #6597

Closed
leesb971204 wants to merge 3 commits into
TanStack:mainfrom
leesb971204:fix/scroll-restoration
Closed

fix(scroll-restoration): save scroll position when navigating with resetScroll=false#6597
leesb971204 wants to merge 3 commits into
TanStack:mainfrom
leesb971204:fix/scroll-restoration

Conversation

@leesb971204

@leesb971204leesb971204 commented Feb 5, 2026

Copy link
Copy Markdown
Contributor

fixes#6595

Problem Analysis

When navigating A → B → C → B:

  1. User scrolls on A (Home) to position 1000
  2. User navigates to B (About) with resetScroll=false (scroll position maintained)
  3. User navigates to C (Foo) without scrolling on B
  4. User goes back to B (About)
  5. Bug: Scroll position is 0 (B has no cache entry because no scroll event occurred on B)

The root cause is that scroll positions are only saved through the onScroll event handler. If the user never scrolls on page B, there's no cache entry for B.

Solution

Save the current scroll position to the destination route's cache key when navigating with resetScroll=false. This must happen beforehistory.push/replace because the browser resets scroll position after the history change.

Why commitLocation (before history.push)?

I evaluated several insertion points:

LocationTimingResult
onRendered eventAfter page transitionwindow.scrollY already 0
onBeforeLoad eventAfter history.pushwindow.scrollY already 0
commitLocation before history.pushBefore history change✅ Captures current scroll

The scroll position must be captured beforehistory.push/replace is called, making commitLocation the only viable location.

Summary by CodeRabbit

  • New Features

    • Example app now uses a persistent sidebar layout and configures scroll restoration for that sidebar.
  • Bug Fixes

    • Improved scroll restoration on back navigation when scroll reset is disabled — restores both page and element (e.g., sidebar) scroll positions without extra scroll events.
  • Tests

    • Added end-to-end regression tests covering page and element scroll restoration across navigation.

…setScroll=false
Signed-off-by: leesb971204 <leesb971204@gmail.com>
@coderabbitai

coderabbitaiBot commented Feb 5, 2026

Copy link
Copy Markdown
Contributor
📝 Walkthrough

Walkthrough

Adds scroll-restoration caching on navigation when resetScroll=false, persisting window and element scroll positions into the scroll restoration cache so back/forward restores positions for intermediate pages; also adds E2E tests and a persistent #sidebar element with scrollToTopSelectors to validate behavior.

Changes

Cohort / File(s)Summary
E2E tests
e2e/react-router/basic-scroll-restoration/tests/app.spec.ts
Adds two Playwright tests verifying per-path and per-element scroll positions are saved to sessionStorage and restored on back navigation when resetScroll=false, including assertions that no extra scroll event is emitted.
Router core
packages/router-core/src/router.ts
Imports defaultGetScrollRestorationKey, getCssSelector, scrollRestorationCache, setupScrollRestoration; when navigating with resetScroll=false and scroll restoration active, persists current window.scrollY and per-element scroll positions into scrollRestorationCache keyed by the location key.
Example app / layout
e2e/react-router/basic-scroll-restoration/src/main.tsx
Replaces top nav with a persistent #sidebar element and content wrapper; configures router scrollToTopSelectors: ['#sidebar'] so the sidebar's scroll is tracked/restored during navigation.

Sequence Diagram(s)

sequenceDiagram
participant Browser as Browser Page (Client)
participant Router as Router.commitLocation
participant Cache as scrollRestorationCache (sessionStorage)
participant DOM as DOM / `#sidebar` / window
Browser->>Router: navigate (resetScroll=false)
Router->>DOM: read window.scrollY & element scrollTop (getCssSelector)
Router->>Cache: write per-path scroll state (defaultGetScrollRestorationKey) rgba(100,150,240,0.5)
Note right of Cache: stored in sessionStorage per-path
Browser->>Router: back navigation
Router->>Cache: read stored scroll state rgba(200,120,80,0.5)
Router->>DOM: restore window.scrollY & element scrollTop
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • birkskyum
  • schiller-manuel

Poem

🐰
I tuck a scroll in my fluffy paw,
Across three pages I hop and I saw —
When you go back, I whisper the spot,
No leap to the top, I remembered the plot.
Hooray for caches and a sidebar thought!

🚥 Pre-merge checks | ✅ 4 | ❌ 1
❌ Failed checks (1 warning)
Check nameStatusExplanationResolution
Docstring Coverage⚠️ WarningDocstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check nameStatusExplanation
Description Check✅ PassedCheck skipped - CodeRabbit’s high-level summary is enabled.
Title check✅ PassedThe title 'fix(scroll-restoration): save scroll position when navigating with resetScroll=false' clearly and concisely summarizes the main change—saving scroll position during navigation with resetScroll=false.
Linked Issues check✅ PassedThe implementation addresses all coding requirements from #6595: saving scroll position for intermediate pages on forward navigation, supporting nested scrollable elements via scrollToTopSelectors, and preserving existing behavior while capturing scroll state without explicit onScroll events.
Out of Scope Changes check✅ PassedAll changes are scoped to scroll restoration: test cases for regression coverage, router.ts implementation of scroll caching logic, and e2e test app configuration with sidebar and scrollToTopSelectors feature.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

@nx-cloud

nx-cloudBot commented Feb 5, 2026

Copy link
Copy Markdown
Contributor

View your CI Pipeline Execution ↗ for commit b49afae

CommandStatusDurationResult
nx affected --targets=test:eslint,test:unit,tes...✅ Succeeded15m 6sView ↗
nx run-many --target=build --exclude=examples/*...✅ Succeeded1m 44sView ↗

☁️ Nx Cloud last updated this comment at 2026-02-05 12:13:08 UTC

@pkg-pr-new

pkg-pr-newBot commented Feb 5, 2026

Copy link
Copy Markdown
More templates

@tanstack/arktype-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/arktype-adapter@6597

@tanstack/eslint-plugin-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/eslint-plugin-router@6597

@tanstack/history

npm i https://pkg.pr.new/TanStack/router/@tanstack/history@6597

@tanstack/nitro-v2-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/nitro-v2-vite-plugin@6597

@tanstack/react-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router@6597

@tanstack/react-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-devtools@6597

@tanstack/react-router-ssr-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-ssr-query@6597

@tanstack/react-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start@6597

@tanstack/react-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-client@6597

@tanstack/react-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-server@6597

@tanstack/router-cli

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-cli@6597

@tanstack/router-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-core@6597

@tanstack/router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools@6597

@tanstack/router-devtools-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools-core@6597

@tanstack/router-generator

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-generator@6597

@tanstack/router-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-plugin@6597

@tanstack/router-ssr-query-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-ssr-query-core@6597

@tanstack/router-utils

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-utils@6597

@tanstack/router-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-vite-plugin@6597

@tanstack/solid-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router@6597

@tanstack/solid-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router-devtools@6597

@tanstack/solid-router-ssr-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router-ssr-query@6597

@tanstack/solid-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start@6597

@tanstack/solid-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-client@6597

@tanstack/solid-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-server@6597

@tanstack/start-client-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-client-core@6597

@tanstack/start-fn-stubs

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-fn-stubs@6597

@tanstack/start-plugin-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-plugin-core@6597

@tanstack/start-server-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-core@6597

@tanstack/start-static-server-functions

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-static-server-functions@6597

@tanstack/start-storage-context

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-storage-context@6597

@tanstack/valibot-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/valibot-adapter@6597

@tanstack/virtual-file-routes

npm i https://pkg.pr.new/TanStack/router/@tanstack/virtual-file-routes@6597

@tanstack/vue-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/vue-router@6597

@tanstack/vue-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/vue-router-devtools@6597

@tanstack/vue-router-ssr-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/vue-router-ssr-query@6597

@tanstack/vue-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/vue-start@6597

@tanstack/vue-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/vue-start-client@6597

@tanstack/vue-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/vue-start-server@6597

@tanstack/zod-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/zod-adapter@6597

commit: b49afae

@coderabbitaicoderabbitaiBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@packages/router-core/src/router.ts`:
- Around line 2135-2151: The block that runs when next.resetScroll === false and
this.isScrollRestoring only seeds the "window" entry in scrollRestorationCache,
which leaves element-based entries missing/stale; update this code to snapshot
all tracked elements (using the same keying logic as setupScrollRestoration) or
rebuild the entire destination entry instead of only setting 'window'.
Specifically, when computing toKey via this.options.getScrollRestorationKey ||
defaultGetScrollRestorationKey, read the current element selectors used by
setupScrollRestoration and populate keyEntry for each element (e.g., by querying
the elements and storing their scroll positions) so
scrollRestorationCache.set(state => { state[toKey] = { window: ..., [elementId]:
{ scrollX, scrollY }... }; return state; }). Ensure this integrates with
scrollRestorationCache, isScrollRestoring and next.resetScroll semantics and
preserves existing window behavior.

Comment threadpackages/router-core/src/router.ts Outdated
…ositions
Signed-off-by: leesb971204 <leesb971204@gmail.com>
@leesb971204
leesb971204force-pushed the fix/scroll-restoration branch from 3e3b3f4 to 4bc8837CompareFebruary 5, 2026 11:55
@TomVHPresto

Copy link
Copy Markdown

I was testing this and found it didn't seem to work. While it did pass with the basic scroll restoration tests, those use location.pathname as the scroll restoration key - if you use the default scroll restoration key rather than location.pathname it did not work. This seems to be because next.state.__TSR_key is undefined at the point of commitLocation, so it ends up using the wrong cache key when writing the state and doesn't restore the scroll correctly. While testing I ended up writing what I think is a working solution for this. Shall I make a new PR?

@leesb971204

Copy link
Copy Markdown
ContributorAuthor

I was testing this and found it didn't seem to work. While it did pass with the basic scroll restoration tests, those use location.pathname as the scroll restoration key - if you use the default scroll restoration key rather than location.pathname it did not work. This seems to be because next.state.__TSR_key is undefined at the point of commitLocation, so it ends up using the wrong cache key when writing the state and doesn't restore the scroll correctly. While testing I ended up writing what I think is a working solution for this. Shall I make a new PR?

Would you be able to add code as part of the this PR?

@TomVHPresto

Copy link
Copy Markdown

I was testing this and found it didn't seem to work. While it did pass with the basic scroll restoration tests, those use location.pathname as the scroll restoration key - if you use the default scroll restoration key rather than location.pathname it did not work. This seems to be because next.state.__TSR_key is undefined at the point of commitLocation, so it ends up using the wrong cache key when writing the state and doesn't restore the scroll correctly. While testing I ended up writing what I think is a working solution for this. Shall I make a new PR?

Would you be able to add code as part of the this PR?

I've had to start from scratch so I don't know if I can really

@schiller-manuel

Copy link
Copy Markdown
Collaborator

closing in favor of #7447

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Scroll position should be remembered for intermediate pages when navigating with resetScroll=false

3 participants

@leesb971204@TomVHPresto@schiller-manuel