fix: scroll restorating without throttling#7042
Open
schiller-manuel wants to merge 7 commits intomainfrom
Open
fix: scroll restorating without throttling#7042schiller-manuel wants to merge 7 commits intomainfrom
schiller-manuel wants to merge 7 commits intomainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughReworks scroll restoration: capture scroll keys at event time, remove throttled key capture, switch to unthrottled snapshot persistence with explicit cache.persist(), add inline restoration routine and script-generation APIs, clear stale cache before restore, add hash-scroll helper, update framework integrations, and add E2E repro routes/tests for the fast-scroll race. Changes
Sequence DiagramsequenceDiagram
participant Browser as Client (Browser)
participant Router as Router Runtime
participant Cache as Scroll Cache (memory + sessionStorage)
participant Server as SSR / Build
Note over Browser,Router: Capture restoreKey at event time to avoid stale keys
Browser->>Router: scroll event
Router->>Router: resolve elementSelector & restoreKey immediately
Router->>Cache: schedule/save scroll position (args preserved)
Browser->>Router: click / navigate
Router->>Router: onRendered (navigation complete)
Router->>Cache: clear stale cache entry for destination key (if applicable)
Server->>Browser: SSR-generated inline script (if provided)
Browser->>Cache: restoration script runs -> reads cache -> apply entry or hash/top fallback
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit b195a5a
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version Preview4 package(s) bumped directly, 18 bumped as dependents. 🟩 Patch bumps
|
Contributor
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
schiller-manuel
force-pushed
the
fix-7040
branch
from
72585d4 to
c63bdad
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
e2e/react-router/scroll-restoration-sandbox-vite/tests/issue-7040.repro.spec.ts (1)
82-85: Use sharedstorageKeyconstant to avoid brittle test coupling.Line 84 hard-codes the storage key string; this can fail when key versioning changes even if behavior is correct.
♻️ Proposed refactor
import { expect, test } from '@playwright/test' import { toRuntimePath } from '@tanstack/router-e2e-utils' +import { storageKey } from '@tanstack/router-core' @@ - const after = await page.evaluate(() => { + const after = await page.evaluate((key) => { const destinationKey = window.history.state.__TSR_key - const raw = sessionStorage.getItem('tsr-scroll-restoration-v1_3') + const raw = sessionStorage.getItem(key) const parsed = raw ? JSON.parse(raw) : {} return { destinationKey, @@ destinationEntry: parsed[destinationKey], } - }) + }, storageKey)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-router/scroll-restoration-sandbox-vite/tests/issue-7040.repro.spec.ts` around lines 82 - 85, The test is hard-coding the sessionStorage key ('tsr-scroll-restoration-v1_3') which makes it brittle; replace the literal with the shared storage key constant (e.g. storageKey) used by the implementation under test so the test follows version changes. Update the evaluation snippet that reads sessionStorage.getItem(...) to reference the exported/shared storageKey (import or access it in the page context as needed) and ensure the variable name destinationKey / after logic remains unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.changeset/tall-buses-bow.md:
- Line 8: The changeset line "fix: scroll restorating without throttling"
contains a typo—replace "restorating" with "restoration" so the summary reads
"fix: scroll restoration without throttling"; update the changeset description
string accordingly to correct the spelling.
In `@packages/router-core/src/scroll-restoration-inline.ts`:
- Around line 24-39: The loop over elementEntries currently assumes parsed
session data is valid; add type guards and selector validation to avoid runtime
errors by verifying each entry is an object with numeric scrollX/scrollY before
using them (validate entry.scrollX and entry.scrollY are finite numbers), and
wrap document.querySelector(elementSelector) in a try-catch to guard against
invalid CSS selectors; update the logic inside the for (const elementSelector in
elementEntries) block and references to elementEntries, elementSelector,
entry.scrollX, entry.scrollY, and document.querySelector to bail out or skip
entries that fail validation.
In `@packages/router-core/src/scroll-restoration.ts`:
- Line 172: The WeakMap elementScrollRestorationSelectorCache storing
getCssSelector() results is unsafe because selectors with nth-child(...) become
stale when DOM structure changes; remove the per-Element memoization (delete the
elementScrollRestorationSelectorCache declaration and any reads/writes to it)
and change code paths that call getCssSelector() (and the restore/save flows
that used the cache) to compute the selector on-demand or use a more stable
identifier strategy; also remove or update the similar caching logic referenced
around the other occurrence (the block that currently reads/writes the same
cache between lines 395-402) so no stale selector is stored or reused.
- Around line 57-69: readScrollRestorationState currently returns whatever
JSON.parse yields (e.g., null), which later causes indexed access on
scrollRestorationCache.state to throw; update readScrollRestorationState (and
use the same validation logic as restoreScroll) to parse the stored string then
verify the result is a plain object (not null, array, or primitive) and if not,
return {}; reference the function name readScrollRestorationState, the storage
key parameter (key / storageKey), and the scrollRestorationCache.state usage so
you clamp non-object JSON payloads back to {} for safe degradation.
---
Nitpick comments:
In
`@e2e/react-router/scroll-restoration-sandbox-vite/tests/issue-7040.repro.spec.ts`:
- Around line 82-85: The test is hard-coding the sessionStorage key
('tsr-scroll-restoration-v1_3') which makes it brittle; replace the literal with
the shared storage key constant (e.g. storageKey) used by the implementation
under test so the test follows version changes. Update the evaluation snippet
that reads sessionStorage.getItem(...) to reference the exported/shared
storageKey (import or access it in the page context as needed) and ensure the
variable name destinationKey / after logic remains unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef63a01f-a3f4-4b54-bd2d-0816c9a65c94
📒 Files selected for processing (17)
.changeset/tall-buses-bow.mde2e/react-router/scroll-restoration-sandbox-vite/src/routeTree.gen.tse2e/react-router/scroll-restoration-sandbox-vite/src/routes/(tests)/issue-7040-source.tsxe2e/react-router/scroll-restoration-sandbox-vite/src/routes/(tests)/issue-7040-target.tsxe2e/react-router/scroll-restoration-sandbox-vite/src/routes/__root.tsxe2e/react-router/scroll-restoration-sandbox-vite/src/routes/index.tsxe2e/react-router/scroll-restoration-sandbox-vite/tests/issue-7040.repro.spec.tspackages/react-router/src/Match.tsxpackages/react-router/src/scroll-restoration.tsxpackages/router-core/src/index.tspackages/router-core/src/scroll-restoration-inline.tspackages/router-core/src/scroll-restoration.tspackages/router-core/vite-minify-plugin.tspackages/solid-router/src/Match.tsxpackages/solid-router/src/scroll-restoration.tsxpackages/vue-router/src/Match.tsxpackages/vue-router/src/scroll-restoration.tsx
Merging this PR will improve performance by 9.31%
|
| Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|
| ⚡ | client-side navigation loop (vue) |
90.8 ms | 83.1 ms | +9.31% |
Comparing fix-7040 (b195a5a) with main (5016e4e)1
Footnotes
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
e2e/react-router/scroll-restoration-sandbox-vite/tests/issue-7040.repro.spec.ts (1)
97-101: Consider adding explicit timeout forwaitForFunction.The
waitForFunction(() => window.scrollY === 0)relies on Playwright's default 30-second timeout. If the fix regresses, this will cause slow test failures. Consider an explicit shorter timeout to fail faster in CI.💡 Optional: explicit timeout
- await page.waitForFunction(() => window.scrollY === 0) + await page.waitForFunction(() => window.scrollY === 0, { timeout: 5000 })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@e2e/react-router/scroll-restoration-sandbox-vite/tests/issue-7040.repro.spec.ts` around lines 97 - 101, The test helper waitForTargetPageToSettle uses page.waitForFunction(() => window.scrollY === 0) which relies on Playwright's 30s default; add an explicit, shorter timeout to that call (e.g., pass { timeout: 5000 } or a configured constant) so failures surface quickly in CI—update the waitForFunction invocation in waitForTargetPageToSettle to include the timeout option.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@e2e/react-router/scroll-restoration-sandbox-vite/tests/issue-7040.repro.spec.ts`:
- Around line 1-3: Reorder the imports so the non-type import from
'@tanstack/router-e2e-utils' appears before the type-only import from
'@playwright/test'; specifically, keep the main import "import { expect, test }
from '@playwright/test'" as-is, move "import { toRuntimePath } from
'@tanstack/router-e2e-utils'" above the type import, and then have "import type
{ Page } from '@playwright/test'" last to satisfy ESLint's import order rule.
In `@packages/router-core/src/scroll-restoration.ts`:
- Around line 81-89: The warning message in the persist function uses the
incorrect prefix "[ts-router]"; update the console.warn call inside persist
(referencing persist, storageKey, safeSessionStorage, and state) to use the
correct prefix "[TanStack Router]" (e.g., "[TanStack Router] Could not persist
scroll restoration state to sessionStorage.") so it matches the rest of the
project’s logging conventions.
- Around line 326-333: The restoreScroll export currently JSON.parses
sessionStorage without validating the result and logs parse errors to console;
mirror readScrollRestorationState by safely parsing and validating that the
parsed value is a plain object (e.g., typeof parsed === 'object' && parsed !==
null && !Array.isArray(parsed')) and otherwise falling back to an empty object
for byKey; also remove or silence the console.error on parse failure so
corruption doesn't spam logs—catch errors, set byKey = {}, and continue using
the validated byKey in restoreScroll.
---
Nitpick comments:
In
`@e2e/react-router/scroll-restoration-sandbox-vite/tests/issue-7040.repro.spec.ts`:
- Around line 97-101: The test helper waitForTargetPageToSettle uses
page.waitForFunction(() => window.scrollY === 0) which relies on Playwright's
30s default; add an explicit, shorter timeout to that call (e.g., pass {
timeout: 5000 } or a configured constant) so failures surface quickly in
CI—update the waitForFunction invocation in waitForTargetPageToSettle to include
the timeout option.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: fd89e22b-b068-4474-92d2-5e3dd15b5784
📒 Files selected for processing (17)
.changeset/tall-buses-bow.mde2e/react-router/scroll-restoration-sandbox-vite/src/routeTree.gen.tse2e/react-router/scroll-restoration-sandbox-vite/src/routes/(tests)/issue-7040-source.tsxe2e/react-router/scroll-restoration-sandbox-vite/src/routes/(tests)/issue-7040-target.tsxe2e/react-router/scroll-restoration-sandbox-vite/src/routes/__root.tsxe2e/react-router/scroll-restoration-sandbox-vite/src/routes/index.tsxe2e/react-router/scroll-restoration-sandbox-vite/tests/issue-7040.repro.spec.tspackages/react-router/src/Match.tsxpackages/react-router/src/scroll-restoration.tsxpackages/router-core/src/index.tspackages/router-core/src/scroll-restoration-inline.tspackages/router-core/src/scroll-restoration.tspackages/router-core/vite-minify-plugin.tspackages/solid-router/src/Match.tsxpackages/solid-router/src/scroll-restoration.tsxpackages/vue-router/src/Match.tsxpackages/vue-router/src/scroll-restoration.tsx
✅ Files skipped from review due to trivial changes (7)
- packages/router-core/vite-minify-plugin.ts
- e2e/react-router/scroll-restoration-sandbox-vite/src/routes/__root.tsx
- .changeset/tall-buses-bow.md
- packages/vue-router/src/scroll-restoration.tsx
- packages/solid-router/src/Match.tsx
- packages/router-core/src/scroll-restoration-inline.ts
- e2e/react-router/scroll-restoration-sandbox-vite/src/routeTree.gen.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- e2e/react-router/scroll-restoration-sandbox-vite/src/routes/index.tsx
- packages/react-router/src/Match.tsx
- packages/react-router/src/scroll-restoration.tsx
- packages/solid-router/src/scroll-restoration.tsx
- e2e/react-router/scroll-restoration-sandbox-vite/src/routes/(tests)/issue-7040-target.tsx
- packages/router-core/src/index.ts
- e2e/react-router/scroll-restoration-sandbox-vite/src/routes/(tests)/issue-7040-source.tsx
Comment on lines
+1
to
+3
| import { expect, test } from '@playwright/test' | ||
| import type { Page } from '@playwright/test' | ||
| import { toRuntimePath } from '@tanstack/router-e2e-utils' |
Contributor
There was a problem hiding this comment.
Fix import order per ESLint.
The static analysis tool flags that the @tanstack/router-e2e-utils import should occur before the type import from @playwright/test.
🔧 Suggested fix
import { expect, test } from '@playwright/test'
+import { toRuntimePath } from '@tanstack/router-e2e-utils'
import type { Page } from '@playwright/test'
-import { toRuntimePath } from '@tanstack/router-e2e-utils'🧰 Tools
🪛 ESLint
[error] 3-3: @tanstack/router-e2e-utils import should occur before type import of @playwright/test
(import/order)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@e2e/react-router/scroll-restoration-sandbox-vite/tests/issue-7040.repro.spec.ts`
around lines 1 - 3, Reorder the imports so the non-type import from
'@tanstack/router-e2e-utils' appears before the type-only import from
'@playwright/test'; specifically, keep the main import "import { expect, test }
from '@playwright/test'" as-is, move "import { toRuntimePath } from
'@tanstack/router-e2e-utils'" above the type import, and then have "import type
{ Page } from '@playwright/test'" last to satisfy ESLint's import order rule.
Comment on lines
+81
to
+89
| const persist = () => { | ||
| try { | ||
| safeSessionStorage.setItem(storageKey, JSON.stringify(state)) | ||
| } catch { | ||
| console.warn( | ||
| '[ts-router] Could not persist scroll restoration state to sessionStorage.', | ||
| ) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Minor typo in warning message.
The warning prefix [ts-router] appears to be a typo. Other TanStack Router code typically uses [TanStack Router] or similar.
🔧 Suggested fix
console.warn(
- '[ts-router] Could not persist scroll restoration state to sessionStorage.',
+ '[TanStack Router] Could not persist scroll restoration state to sessionStorage.',
)🤖 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 81 - 89, The
warning message in the persist function uses the incorrect prefix "[ts-router]";
update the console.warn call inside persist (referencing persist, storageKey,
safeSessionStorage, and state) to use the correct prefix "[TanStack Router]"
(e.g., "[TanStack Router] Could not persist scroll restoration state to
sessionStorage.") so it matches the rest of the project’s logging conventions.
Sheraff
reviewed
Mar 25, 2026
| if (typeof document !== 'undefined') { | ||
| document.addEventListener('scroll', throttle(onScroll, 100), true) | ||
| } | ||
| document.addEventListener('scroll', onScroll, true) |
Contributor
There was a problem hiding this comment.
I'm not sure we need to run on the capture phase here.
Suggested change
| document.addEventListener('scroll', onScroll, true) | |
| document.addEventListener('scroll', onScroll) |
Sheraff
reviewed
Mar 25, 2026
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/router-core/src/scroll-restoration.ts (1)
68-72:⚠️ Potential issue | 🟡 MinorMinor typo in warning message prefix.
The warning prefix
[ts-router]should be[TanStack Router]to match the project's naming conventions used elsewhere.🔧 Suggested fix
if (process.env.NODE_ENV !== 'production') { console.warn( - '[ts-router] Could not persist scroll restoration state to sessionStorage.', + '[TanStack Router] Could not persist scroll restoration state to sessionStorage.', ) }🤖 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 68 - 72, Update the warning string used in the scroll restoration persistence code so it uses the correct project prefix; replace the current '[ts-router]' prefix in the console.warn call found in scroll-restoration persistence logic (the block guarded by if (process.env.NODE_ENV !== 'production')) with '[TanStack Router]' so the message matches the project's naming conventions.
🧹 Nitpick comments (2)
packages/router-core/src/scroll-restoration.ts (1)
250-250: Consider whether capture phase is necessary for the scroll listener.Using
true(capture phase) for the scroll listener is defensive and ensures scroll events are captured even if other handlers stop propagation. While typically not required for most use cases, it's a reasonable choice for scroll restoration reliability.🤖 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` at line 250, The scroll listener is currently registered in capture phase via document.addEventListener('scroll', onScroll, true); review whether capture is required—if not, change the registration to non-capturing (use false or omit the third arg) so onScroll runs in bubble phase, or otherwise explicitly pass an options object ({ capture: true }) and add a short comment explaining why capture is necessary; update the addEventListener call referencing onScroll accordingly.packages/router-core/src/scroll-restoration-inline.ts (1)
1-14: Function correctly validates JSON parsing with error handling.The try-catch properly handles corrupted sessionStorage data. However,
console.error(error)on parse failure might be noisy for users with corrupted storage. Consider silently failing since the function already returns early.♻️ Consider silent failure for corrupted storage
try { byKey = JSON.parse(sessionStorage.getItem(options.storageKey) || '{}') } catch (error) { - console.error(error) return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/scroll-restoration-inline.ts` around lines 1 - 14, The try-catch around JSON.parse(sessionStorage.getItem(options.storageKey) || '{}') correctly handles corrupted session storage but currently calls console.error(error), which can be noisy; remove the console.error(error) (or replace it with a no-op) and simply return so the default behavior proceeds silently; update the default exported function handling around byKey and options.storageKey to omit logging on parse failure.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/react-router/tests/Scripts.test.tsx`:
- Line 165: The test currently asserts a hard-coded container.innerHTML string
that includes an empty <script></script>, which couples the test to unrelated
rendering details; update the assertion in
packages/react-router/tests/Scripts.test.tsx to instead assert presence of the
managed script elements by checking for <script src="script.js"> and <script
src="script3.js"> (e.g., via container.querySelectorAll('script[src="..."]') or
getByTestId then querySelector) and assert the absence of <script
src="script2.js">, removing the exact innerHTML string comparison that includes
the empty <script></script>.
---
Duplicate comments:
In `@packages/router-core/src/scroll-restoration.ts`:
- Around line 68-72: Update the warning string used in the scroll restoration
persistence code so it uses the correct project prefix; replace the current
'[ts-router]' prefix in the console.warn call found in scroll-restoration
persistence logic (the block guarded by if (process.env.NODE_ENV !==
'production')) with '[TanStack Router]' so the message matches the project's
naming conventions.
---
Nitpick comments:
In `@packages/router-core/src/scroll-restoration-inline.ts`:
- Around line 1-14: The try-catch around
JSON.parse(sessionStorage.getItem(options.storageKey) || '{}') correctly handles
corrupted session storage but currently calls console.error(error), which can be
noisy; remove the console.error(error) (or replace it with a no-op) and simply
return so the default behavior proceeds silently; update the default exported
function handling around byKey and options.storageKey to omit logging on parse
failure.
In `@packages/router-core/src/scroll-restoration.ts`:
- Line 250: The scroll listener is currently registered in capture phase via
document.addEventListener('scroll', onScroll, true); review whether capture is
required—if not, change the registration to non-capturing (use false or omit the
third arg) so onScroll runs in bubble phase, or otherwise explicitly pass an
options object ({ capture: true }) and add a short comment explaining why
capture is necessary; update the addEventListener call referencing onScroll
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a3dd68dd-15c6-47f8-82c2-fa42ebc7e9ae
📒 Files selected for processing (6)
.changeset/tall-buses-bow.mde2e/react-router/basepath-file-based/tests/scroll-restoration-session-storage-error.test.tspackages/react-router/tests/Scripts.test.tsxpackages/router-core/src/index.tspackages/router-core/src/scroll-restoration-inline.tspackages/router-core/src/scroll-restoration.ts
✅ Files skipped from review due to trivial changes (1)
- .changeset/tall-buses-bow.md
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/router-core/src/index.ts
|
|
||
| expect(container.innerHTML).toEqual( | ||
| `<div><div data-testid="root">root</div><div data-testid="index">index</div><script src="script.js"></script><script src="script3.js"></script></div>`, | ||
| `<div><div data-testid="root">root</div><div data-testid="index">index</div><script></script><script src="script.js"></script><script src="script3.js"></script></div>`, |
Contributor
There was a problem hiding this comment.
This expectation is over-coupled and weakens the test’s intent.
Line 165 now hard-codes an empty <script></script> in a test that’s supposed to verify undefined scripts are excluded. That makes the assertion brittle and tied to unrelated script rendering details. Please assert only the managed src scripts (and absence of script2.js) instead of full innerHTML.
Suggested assertion change
- expect(container.innerHTML).toEqual(
- `<div><div data-testid="root">root</div><div data-testid="index">index</div><script></script><script src="script.js"></script><script src="script3.js"></script></div>`,
- )
+ const renderedScriptSrcs = Array.from(
+ container.querySelectorAll('script[src]'),
+ ).map((el) => el.getAttribute('src'))
+
+ expect(renderedScriptSrcs).toEqual(['script.js', 'script3.js'])
+ expect(container.querySelector('script[src="script2.js"]')).toBeNull()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/react-router/tests/Scripts.test.tsx` at line 165, The test currently
asserts a hard-coded container.innerHTML string that includes an empty
<script></script>, which couples the test to unrelated rendering details; update
the assertion in packages/react-router/tests/Scripts.test.tsx to instead assert
presence of the managed script elements by checking for <script src="script.js">
and <script src="script3.js"> (e.g., via
container.querySelectorAll('script[src="..."]') or getByTestId then
querySelector) and assert the absence of <script src="script2.js">, removing the
exact innerHTML string comparison that includes the empty <script></script>.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/router-core/src/scroll-restoration.ts (1)
149-158: ReplaceanyingetCssSelector.This helper is now part of the save/restore hot path, and
el: anyturns all of its DOM access back into runtime-only validation. Tightening it to DOM types keeps the new flow aligned with the repo’s TS strictness.As per coding guidelines, "Use TypeScript strict mode with extensive type safety".
🤖 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 149 - 158, The getCssSelector helper should use strict DOM types: change the parameter to el: Element and the return type to string, and replace parentNode with parentElement so parent is typed as Element | null (or use Parent type narrowing) to allow safe access to tagName and children; update the while loop to use a non-null check (e.g., while ((parent = el.parentElement)) and keep Array.prototype.indexOf.call(parent.children, el) working with the Element type so the function is fully typed under strict TS.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/router-core/src/scroll-restoration.ts`:
- Around line 262-263: Validate and normalize the per-route bucket before
writing into it: ensure cache.state[restoreKey] is an object of type
ScrollRestorationByElement (not a primitive) before assigning keyEntry[selector]
= position; replace the direct assignment const keyEntry =
(cache.state[restoreKey] ||= {} as ScrollRestorationByElement) with a guard that
checks typeof cache.state[restoreKey] === "object" && cache.state[restoreKey]
!== null, and if not, replace it with a fresh {} (and then cast to
ScrollRestorationByElement); apply the same normalization/guard logic to the
other write site that assigns into the per-route bucket (the code block that
currently writes at the later location around where keyEntry/selector/position
are used) so invalid primitives like 1 are overwritten with a proper object
before any property writes.
- Around line 180-188: The selector built from options.id and
scrollRestorationIdAttribute can be invalid if the id contains special chars;
update the elementSelector construction (where elementSelector =
`[${scrollRestorationIdAttribute}="${options.id}"]`) to escape the attribute
value using CSS.escape (with a safe fallback if not available) before
interpolating, and reuse the same escaping approach wherever attribute-based
selectors are composed (refer to scrollRestorationIdAttribute, elementSelector,
and getCssSelector) so document.querySelector never receives an unescaped
string.
---
Nitpick comments:
In `@packages/router-core/src/scroll-restoration.ts`:
- Around line 149-158: The getCssSelector helper should use strict DOM types:
change the parameter to el: Element and the return type to string, and replace
parentNode with parentElement so parent is typed as Element | null (or use
Parent type narrowing) to allow safe access to tagName and children; update the
while loop to use a non-null check (e.g., while ((parent = el.parentElement))
and keep Array.prototype.indexOf.call(parent.children, el) working with the
Element type so the function is fully typed under strict TS.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b30928a2-150b-4cca-9952-9ff26f4a703b
📒 Files selected for processing (1)
packages/router-core/src/scroll-restoration.ts
Comment on lines
+180
to
+188
| if (options.id) { | ||
| elementSelector = `[${scrollRestorationIdAttribute}="${options.id}"]` | ||
| } else { | ||
| const element = options.getElement?.() | ||
| if (!element) { | ||
| return | ||
| } | ||
| elementSelector = | ||
| element instanceof Window ? windowScrollTarget : getCssSelector(element) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n packages/router-core/src/scroll-restoration.ts | head -200Repository: TanStack/router
Length of output: 7067
🏁 Script executed:
cat -n packages/router-core/src/scroll-restoration.ts | sed -n '265,285p'Repository: TanStack/router
Length of output: 766
🏁 Script executed:
rg -A 2 -B 2 "CSS\.escape" packages/Repository: TanStack/router
Length of output: 41
🏁 Script executed:
rg '\$\{.*\}.*\[.*\$\{' packages/router-core/src/scroll-restoration.tsRepository: TanStack/router
Length of output: 41
🏁 Script executed:
rg "selector.*\`" packages/router-core/src/scroll-restoration.tsRepository: TanStack/router
Length of output: 41
🏁 Script executed:
rg "querySelector|querySelectors|getAttribute.*scrollRestoration" packages/router-core/src/Repository: TanStack/router
Length of output: 733
🏁 Script executed:
find packages/router-core/src -name "*scroll-restoration*" -type fRepository: TanStack/router
Length of output: 159
🏁 Script executed:
cat -n packages/router-core/src/scroll-restoration-inline.tsRepository: TanStack/router
Length of output: 2492
🏁 Script executed:
cat -n packages/router-core/src/scroll-restoration.ts | sed -n '270,275p'Repository: TanStack/router
Length of output: 303
🏁 Script executed:
cat -n packages/router-core/src/scroll-restoration.ts | sed -n '285,295p'Repository: TanStack/router
Length of output: 509
🏁 Script executed:
rg "CSS\.escape|escape.*CSS" packages/Repository: TanStack/router
Length of output: 41
🏁 Script executed:
cat packages/router-core/tsconfig.jsonRepository: TanStack/router
Length of output: 317
🏁 Script executed:
cat tsconfig.jsonRepository: TanStack/router
Length of output: 846
🏁 Script executed:
cat -n packages/router-core/src/scroll-restoration.ts | sed -n '300,330p'Repository: TanStack/router
Length of output: 1416
🏁 Script executed:
cat -n packages/router-core/src/scroll-restoration.ts | sed -n '330,370p'Repository: TanStack/router
Length of output: 1376
🏁 Script executed:
cat -n packages/router-core/src/scroll-restoration.ts | sed -n '370,385p'Repository: TanStack/router
Length of output: 543
🏁 Script executed:
cat -n packages/router-core/src/scroll-restoration.ts | sed -n '175,195p'Repository: TanStack/router
Length of output: 746
Escape data-scroll-restoration-id before composing the selector.
Lines 181 and 272 interpolate arbitrary attribute values directly into CSS selectors. An id containing " or \ produces an invalid selector, causing document.querySelector() to throw an error (caught at line 374) and silently skip scroll restoration.
Proposed fix
if (options.id) {
return scrollRestorationCache?.state[restoreKey]?.[
- `[${scrollRestorationIdAttribute}="${options.id}"]`
+ `[${scrollRestorationIdAttribute}="${CSS.escape(options.id)}"]`
] const attrId = target.getAttribute(scrollRestorationIdAttribute)
selector = attrId
- ? `[${scrollRestorationIdAttribute}="${attrId}"]`
+ ? `[${scrollRestorationIdAttribute}="${CSS.escape(attrId)}"]`
: getCssSelector(target)🤖 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 180 - 188, The
selector built from options.id and scrollRestorationIdAttribute can be invalid
if the id contains special chars; update the elementSelector construction (where
elementSelector = `[${scrollRestorationIdAttribute}="${options.id}"]`) to escape
the attribute value using CSS.escape (with a safe fallback if not available)
before interpolating, and reuse the same escaping approach wherever
attribute-based selectors are composed (refer to scrollRestorationIdAttribute,
elementSelector, and getCssSelector) so document.querySelector never receives an
unescaped string.
Comment on lines
+262
to
+263
| const keyEntry = (cache.state[restoreKey] ||= | ||
| {} as ScrollRestorationByElement) |
Contributor
There was a problem hiding this comment.
Normalize per-route buckets before writing into them.
Only the top-level storage payload is validated right now. If storage contains something like { "/foo": 1 }, Line 262 keeps that truthy primitive, and the next keyEntry[selector] = position throws; the same invalid bucket also survives Line 425.
Proposed fix
+ const getOrCreateKeyEntry = (restoreKey: string) => {
+ const existing = cache.state[restoreKey]
+ if (isPlainObject(existing)) {
+ return existing as ScrollRestorationByElement
+ }
+ return (cache.state[restoreKey] = {} as ScrollRestorationByElement)
+ }
+
const snapshotCurrentScrollTargets = (restoreKey?: string) => {
if (
!router.isScrollRestoring ||
!restoreKey ||
trackedScrollEntries.size === 0 ||
!cache
) {
return
}
- const keyEntry = (cache.state[restoreKey] ||=
- {} as ScrollRestorationByElement)
+ const keyEntry = getOrCreateKeyEntry(restoreKey)
...
if (router.isScrollRestoring) {
cache.set((state) => {
- state[cacheKey] ||= {} as ScrollRestorationByElement
+ if (!isPlainObject(state[cacheKey])) {
+ state[cacheKey] = {} as ScrollRestorationByElement
+ }
return state
})
}Also applies to: 425-427
🤖 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 262 - 263,
Validate and normalize the per-route bucket before writing into it: ensure
cache.state[restoreKey] is an object of type ScrollRestorationByElement (not a
primitive) before assigning keyEntry[selector] = position; replace the direct
assignment const keyEntry = (cache.state[restoreKey] ||= {} as
ScrollRestorationByElement) with a guard that checks typeof
cache.state[restoreKey] === "object" && cache.state[restoreKey] !== null, and if
not, replace it with a fresh {} (and then cast to ScrollRestorationByElement);
apply the same normalization/guard logic to the other write site that assigns
into the per-route bucket (the code block that currently writes at the later
location around where keyEntry/selector/position are used) so invalid primitives
like 1 are overwritten with a proper object before any property writes.
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
2 participants
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.
fixes #7040
Summary by CodeRabbit
Bug Fixes
New Features
Tests
Chores