fix: fix hash scroll restoration precedence#7332
fix: fix hash scroll restoration precedence#7332schiller-manuel wants to merge 5 commits intomainfrom
Conversation
📝 WalkthroughWalkthroughRemoves the standalone post-navigation ChangesScroll restoration, history action, and hash navigation
Sequence DiagramsequenceDiagram
actor User
participant Router as Router.load()
participant ParsedLoc as ParsedLocation
participant Transitioner as Transitioner
participant ScrollRestore as setupScrollRestoration
User->>Router: navigate (e.g. PUSH) with opts.action
Router->>ParsedLoc: stamp historyAction via symbol (historyActionKey)
Router->>Transitioner: emit navigation events (includes location with stamped action)
Transitioner->>Router: complete transition (no handleHashScroll call)
Note over ScrollRestore: During restoration
ScrollRestore->>ScrollRestore: compute skipWindowRestore:<br/>dest.hash && __hashScrollIntoViewOptions && (PUSH|REPLACE)
alt skipWindowRestore = true
ScrollRestore->>User: skip window restore so native hash scrollIntoView can run
else
ScrollRestore->>User: restore window scroll from sessionStorage
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Review rate limit: 4/5 reviews remaining, refill in 12 minutes. Comment |
|
View your CI Pipeline Execution ↗ for commit f2c377a
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview4 package(s) bumped directly, 19 bumped as dependents. 🟩 Patch bumps
|
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
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/router.ts (1)
2432-2446:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate
historyActionthrough the directcommitLocation → load()fallback.
load()now recordsopts?.action?.type, butcommitLocation()still calls barethis.load()when there are no history subscribers (Line 2230). That means framework-agnostic usage still drops PUSH/REPLACE metadata, so the new hash-vs-restoration precedence fix won't apply on that path.Suggested fix
- if (!this.history.subscribers.size) { - this.load() - } + if (!this.history.subscribers.size) { + this.load({ + action: { type: next.replace ? 'REPLACE' : 'PUSH' }, + }) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/router.ts` around lines 2432 - 2446, The commitLocation → load() fallback drops the new historyAction (opts?.action?.type) because commitLocation calls this.load() with no opts; update commitLocation to capture the location's history action (or derive it the same way load expects) and call this.load({ action: { type: historyAction } }) so load() receives and propagates historyAction; adjust references in commitLocation and any call sites that call this.load() directly to pass the action object so the historyAction-based precedence logic in load (and setLocationHistoryAction) works consistently.
🤖 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 152-153: The server setup guard uses the nullish coalescing
operator with isServer (isServer ?? router.isServer) which wrongly ignores
router.isServer when isServer is false; update the condition in the scroll
restoration setup to use logical OR so it reads (isServer || router.isServer)
and keep the existing short-circuit check with router.isScrollRestorationSetup
to prevent setup when either side indicates server context or restoration is
already configured.
---
Outside diff comments:
In `@packages/router-core/src/router.ts`:
- Around line 2432-2446: The commitLocation → load() fallback drops the new
historyAction (opts?.action?.type) because commitLocation calls this.load() with
no opts; update commitLocation to capture the location's history action (or
derive it the same way load expects) and call this.load({ action: { type:
historyAction } }) so load() receives and propagates historyAction; adjust
references in commitLocation and any call sites that call this.load() directly
to pass the action object so the historyAction-based precedence logic in load
(and setLocationHistoryAction) works consistently.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c1e8e065-50fa-4e4f-9fa7-106c646f2187
📒 Files selected for processing (13)
.changeset/brave-dingos-hope.mde2e/react-start/scroll-restoration/src/router.tsxe2e/react-start/scroll-restoration/src/routes/(tests)/hash-scroll-repro.tsxe2e/react-start/scroll-restoration/tests/hash-scroll-repro.spec.tspackages/react-router/src/Transitioner.tsxpackages/router-core/src/hash-scroll.tspackages/router-core/src/index.tspackages/router-core/src/router.tspackages/router-core/src/scroll-restoration-inline.tspackages/router-core/src/scroll-restoration-script/server.tspackages/router-core/src/scroll-restoration.tspackages/solid-router/src/Transitioner.tsxpackages/vue-router/src/Transitioner.tsx
💤 Files with no reviewable changes (2)
- packages/router-core/src/hash-scroll.ts
- packages/router-core/src/index.ts
| if ((isServer ?? router.isServer) || router.isScrollRestorationSetup) { | ||
| return |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Confirm the exact guard currently used
rg -n "isServer \\?\\? router\\.isServer|isServer \\|\\| router\\.isServer" \
packages/router-core/src/scroll-restoration.ts -C2
# 2) Inspect isServer declaration(s) to verify expected boolean semantics
rg -n "export\\s+const\\s+isServer\\b|declare\\s+const\\s+isServer\\b" \
packages --iglob '*isServer*.ts' -C2Repository: TanStack/router
Length of output: 173
🏁 Script executed:
cat -n packages/router-core/src/scroll-restoration.ts | head -180 | tail -50Repository: TanStack/router
Length of output: 1930
🏁 Script executed:
sed -n '130,160p' packages/router-core/src/scroll-restoration.tsRepository: TanStack/router
Length of output: 954
🏁 Script executed:
head -100 packages/router-core/src/scroll-restoration.tsRepository: TanStack/router
Length of output: 2930
🏁 Script executed:
find packages -name "isServer*" -type fRepository: TanStack/router
Length of output: 41
🏁 Script executed:
rg "export.*isServer" packages --iglob "*.ts" --iglob "*.tsx" -A 3Repository: TanStack/router
Length of output: 2640
Use || instead of ?? in the server setup guard.
Line 152 currently uses isServer ?? router.isServer, which ignores router.isServer whenever isServer is false (in client builds). Since isServer can be false, true, or undefined depending on the build target, the nullish coalescing operator skips the router-level server check when the module exports false. The logical OR operator correctly handles all cases: when isServer is false or undefined, it falls back to router.isServer, ensuring that router-level server contexts prevent scroll restoration setup.
Suggested fix
- if ((isServer ?? router.isServer) || router.isScrollRestorationSetup) {
+ if ((isServer || router.isServer) || router.isScrollRestorationSetup) {
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.ts` around lines 152 - 153, The
server setup guard uses the nullish coalescing operator with isServer (isServer
?? router.isServer) which wrongly ignores router.isServer when isServer is
false; update the condition in the scroll restoration setup to use logical OR so
it reads (isServer || router.isServer) and keep the existing short-circuit check
with router.isScrollRestorationSetup to prevent setup when either side indicates
server context or restoration is already configured.
There was a problem hiding this comment.
isServer is undefined at development time and either constant true or false for treeshaking. check how isServer is defined!
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Find the isServer module definition
fd "isServer" packages --type f
echo "---"
# Also check node_modules for the package if available
find . -path "*/router-core/isServer*" -not -path "*/node_modules/*" 2>/dev/null | head -20
echo "---"
# Search for isServer declaration in packages
rg -n "isServer" packages --iglob "*.ts" -l | head -20
Length of output: 918
🏁 Script executed:
#!/bin/bash
cat packages/router-core/src/isServer/server.ts
echo "---server---"
cat packages/router-core/src/isServer/client.ts
echo "---client---"
cat packages/router-core/src/isServer/development.ts
echo "---development---"
# Also check the vite config to see how the export condition is set up
cat packages/router-core/vite.config.ts
Length of output: 1684
@schiller-manuel, you are absolutely right — my original suggestion was incorrect. Here's the breakdown:
| Build target | isServer value |
isServer ?? router.isServer |
isServer || router.isServer |
|---|---|---|---|
| Development / test | undefined |
router.isServer ✓ |
router.isServer ✓ |
| Production server | true |
true (short-circuits) ✓ |
true (short-circuits) ✓ |
| Production client | false |
false (short-circuits) ✓ |
router.isServer ← ❌ wrong |
In a client build, isServer is definitively false, meaning it is not a server environment. The ?? operator correctly short-circuits to false, allowing scroll restoration setup to proceed. Switching to || would incorrectly fall back to router.isServer in that case, which could block setup if a router instance happened to have router.isServer = true inside a client bundle.
The ?? operator is intentional and correct here. I'll dismiss my comment.
✏️ Learnings added
Learnt from: schiller-manuel
Repo: TanStack/router PR: 7332
File: packages/router-core/src/scroll-restoration.ts:152-153
Timestamp: 2026-05-03T11:34:24.919Z
Learning: In `packages/router-core/src/scroll-restoration.ts`, the guard `(isServer ?? router.isServer)` is intentional and correct. `isServer` is imported from `tanstack/router-core/isServer` and has three possible values: `undefined` in development/test (via `development.ts`), `true` in production server builds (via `server.ts`), and `false` in production client builds (via `client.ts`). The `??` (nullish coalescing) operator is used so that `false` (client build) short-circuits to `false` without falling back to `router.isServer`, while `undefined` (dev/test) correctly falls back to `router.isServer`. Using `||` would be wrong because `false || router.isServer` would add an unnecessary and potentially incorrect fallback in client builds.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
Merging this PR will not alter performance
Comparing 
Footnotes
|
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
Co-authored-by: schiller-manuel <schiller-manuel@users.noreply.github.com>
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/router.ts (1)
2424-2447:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPropagate the action through the no-subscriber fallback.
load()only stampslatestLocationwhenopts.actionis present, butcommitLocationstill falls back to a barethis.load()whenhistory.subscribers.size === 0. That leaves the new hash-scroll precedence signal absent in a reachable navigation path.🔧 Proposed fix
- this.load() + this.load({ action: { type: next.replace ? 'REPLACE' : 'PUSH' } })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/router.ts` around lines 2424 - 2447, The no-subscriber fallback path in commitLocation calls this.load() without passing through the history action stamping, so the historyActionKey is never set on latestLocation when history.subscribers.size === 0; update the fallback in commitLocation to call this.load with the same opts (or construct an opts containing the original action) so that load() can apply the historyAction onto this.latestLocation using historyActionKey (refer to load, commitLocation, latestLocation, history.subscribers, historyAction, and historyActionKey) ensuring the new hash-scroll precedence signal is present in all navigation paths.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/router-core/src/router.ts`:
- Around line 2424-2447: The no-subscriber fallback path in commitLocation calls
this.load() without passing through the history action stamping, so the
historyActionKey is never set on latestLocation when history.subscribers.size
=== 0; update the fallback in commitLocation to call this.load with the same
opts (or construct an opts containing the original action) so that load() can
apply the historyAction onto this.latestLocation using historyActionKey (refer
to load, commitLocation, latestLocation, history.subscribers, historyAction, and
historyActionKey) ensuring the new hash-scroll precedence signal is present in
all navigation paths.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f5f5f95c-01c5-47c0-9512-508f9e3dd835
📒 Files selected for processing (1)
packages/router-core/src/router.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
There was a problem hiding this comment.
Nx Cloud has identified a flaky task in your failed CI:
🔂 Since the failure was identified as flaky, we triggered a CI rerun by adding an empty commit to this branch.
🎓 Learn more about Self-Healing CI on nx.dev
1 participant
Summary by CodeRabbit
Bug Fixes
Tests