fix(router-core): skip scroll restoration setup when disabled#7562
Conversation
|
Complex PR? Review this PR in Change Stack to move by importance, not file order. 📝 WalkthroughWalkthroughScroll state management is refactored from separate boolean flags into a structured ChangesScroll restoration conditional setup
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 93895cc
💡 Verify your cache is correct by running tasks in a sandbox. Read docs ↗ ☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview1 package(s) bumped directly, 22 bumped as dependents. 🟩 Patch bumps
|
Bundle Size Benchmarks
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/router-core/src/scroll-restoration.ts (1)
196-203:⚠️ Potential issue | 🟠 Major | ⚡ Quick winUse
window-qualified browser APIs to fully avoid non-browser-global runtime crashes.The new conditional setup is good, but this path still uses bare globals (
history,addEventListener,scrollX/scrollY,scrollTo). In environments where these are not onglobalThis, setup/navigation can still throw.Suggested patch
- history.scrollRestoration = 'manual' + window.history.scrollRestoration = 'manual' @@ - addEventListener('pagehide', () => { + window.addEventListener('pagehide', () => { @@ - setTrackedScrollEntry(windowScrollTarget, scrollX, scrollY) + setTrackedScrollEntry(windowScrollTarget, window.scrollX, window.scrollY) @@ - scrollTo({ + window.scrollTo({ top: scrollY, left: scrollX, behavior, }) @@ - scrollTo(scrollOptions) + window.scrollTo(scrollOptions)Also applies to: 227-247, 330-347, 366-366
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/router-core/src/scroll-restoration.ts` around lines 196 - 203, The handlers (e.g., onScroll) and related logic currently use bare browser globals which can throw in non-browser global environments; update all usages to use window-qualified APIs (e.g., window.history, window.addEventListener / window.removeEventListener, window.scrollX / window.scrollY, window.scrollTo) and ensure any references to history/scrollX/scrollY/scrollTo in functions like onScroll, setTrackedScrollEntry, and other scroll-restoration handlers are replaced with window.X equivalents so they safely no-op or throw only in true browser contexts; apply the same change to the other affected blocks indicated (~lines 227–247, 330–347, 366).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@packages/router-core/src/scroll-restoration.ts`:
- Around line 196-203: The handlers (e.g., onScroll) and related logic currently
use bare browser globals which can throw in non-browser global environments;
update all usages to use window-qualified APIs (e.g., window.history,
window.addEventListener / window.removeEventListener, window.scrollX /
window.scrollY, window.scrollTo) and ensure any references to
history/scrollX/scrollY/scrollTo in functions like onScroll,
setTrackedScrollEntry, and other scroll-restoration handlers are replaced with
window.X equivalents so they safely no-op or throw only in true browser
contexts; apply the same change to the other affected blocks indicated (~lines
227–247, 330–347, 366).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 79696389-ba37-4cf9-9e60-27486215b45f
📒 Files selected for processing (5)
.changeset/fix-disabled-scroll-restoration.mdpackages/router-core/src/router.tspackages/router-core/src/scroll-restoration.tspackages/router-core/tests/scroll-restoration.test.tspackages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx
Merging this PR will not alter performance
Comparing Footnotes
|
fixes #7472
Summary by CodeRabbit
Release Notes