Skip to content

fix(router-core): preserve element scroll restoration in scrollToTopSelectors#7704

Open
EduardF1 wants to merge 1 commit into
TanStack:mainfrom
EduardF1:fix/scroll-restoration-element-reset
Open

fix(router-core): preserve element scroll restoration in scrollToTopSelectors#7704
EduardF1 wants to merge 1 commit into
TanStack:mainfrom
EduardF1:fix/scroll-restoration-element-reset

Conversation

@EduardF1

@EduardF1 EduardF1 commented Jun 26, 2026

Copy link
Copy Markdown

Summary

Fixes #7687.

When a scroll container element is both a scroll-restoration target and listed in scrollToTopSelectors, its restored scroll position was immediately reset to 0.

In the onRendered handler of setupScrollRestoration the scroll-to-top fallback was only suppressed for the window (via the windowRestored flag). After an element was successfully restored (element.scrollTop = scrollY), the fallback loop still ran element.scrollTo({ top: 0 }) on that same element, wiping the just-restored position.

Fix

Track successfully restored elements in a restoredElements set and continue past them in the scroll-to-top fallback loop — mirroring exactly how windowRestored already guards the window. Minimal and surgical: no behavior change for elements that are only in scrollToTopSelectors (and were not restored), which are still scrolled to top as before.

Validation

  • Added a regression test in packages/router-core/tests/scroll-restoration.test.ts that drives onBeforeLoad + onRendered with an element that is both restored and present in scrollToTopSelectors. It asserts the restored scrollTop (100) survives.
    • Confirmed the test fails without the source change (expected +0 to be 100) and passes with it.
  • vitest run for @tanstack/router-core: 1179 passed, 3 expected-fail (pre-existing), 0 type errors (39 files).
  • eslint on both changed files: clean.

Not a duplicate

No open PR addresses #7687. Related open scroll PRs target different concerns: #7332 (hash precedence), #7335 (carrying nested positions across new restoration keys), #7574 (docs), #4059 (feature to disable restoration). This fixes the fallback-vs-restore ordering for elements specifically.

Summary by CodeRabbit

  • Bug Fixes
    • Improved scroll restoration so restored element positions are no longer overwritten by the automatic “scroll to top” fallback.
    • Preserved scroll position for scrollable page elements during navigation, including cases where specific elements are targeted for top scrolling.

…electors

When an element is both a scroll-restoration target and listed in
`scrollToTopSelectors`, the scroll-to-top fallback in `onRendered` ran
after the element was restored and reset it back to 0. The fallback was
only suppressed for the window (via `windowRestored`), not for elements.

Track successfully restored elements in a `restoredElements` set and skip
them in the scroll-to-top fallback loop, mirroring the existing
`windowRestored` guard. Adds a regression test that fails without the fix.

Closes TanStack#7687

Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 2c6e52c9-1c15-4cc7-9aff-caa367336127

📥 Commits

Reviewing files that changed from the base of the PR and between bb2daa6 and 8eda690.

📒 Files selected for processing (2)
  • packages/router-core/src/scroll-restoration.ts
  • packages/router-core/tests/scroll-restoration.test.ts

📝 Walkthrough

Walkthrough

Scroll restoration now tracks restored non-window elements during onRendered and skips those elements in the scrollToTopSelectors fallback. Tests add a jsdom-backed scrollable element case that verifies restored scroll positions are preserved.

Changes

Element scroll restoration fix

Layer / File(s) Summary
Track restored elements
packages/router-core/src/scroll-restoration.ts
A set records restored non-window elements, and the scroll-to-top fallback skips any element already restored in the same render cycle.
Element restoration test
packages/router-core/tests/scroll-restoration.test.ts
A new test helper creates a mocked scrollable element and verifies that element restoration is not overwritten by the fallback reset.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • TanStack/router#7464: Also adjusts scroll-restoration behavior in packages/router-core/src/scroll-restoration.ts, focusing on fallback scroll-reset handling.

Suggested labels

package: router-core

Poem

A bunny hopped where scrolls once slid,
And kept the list where memories hid.
No top-reset nibble, no surprise zero,
Just cozy scrolls for the page-level hero. 🐇

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring 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 name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main fix: preserving element scroll restoration when scrollToTopSelectors is also configured.
Linked Issues check ✅ Passed The code updates onRendered to skip fallback scrolling for restored elements, matching the linked issue's expected behavior, and adds a regression test.
Out of Scope Changes check ✅ Passed The changes stay focused on the reported scroll-restoration bug and its regression test, with no unrelated scope visible.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

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

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Element scroll restoration is reset to top by scrollToTopSelectors fallback when the scroll container is an element

1 participant