fix: preserve unmasked route on SSR hard reload#7116
Open
tmm wants to merge 6 commits intoTanStack:mainfrom
Open
fix: preserve unmasked route on SSR hard reload#7116tmm wants to merge 6 commits intoTanStack:mainfrom
tmm wants to merge 6 commits intoTanStack:mainfrom
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:
📝 WalkthroughWalkthroughAdds an optional "unmask on reload" pre-hydration inline script: server-side rendering emits the script when route masks opt into Changes
Sequence DiagramsequenceDiagram
autonumber
participant Server as Server (SSR)
participant Browser as Browser (HTML)
participant Script as Unmask Script
participant Router as Router (Client)
Server->>Server: Inspect router.options.routeMasks for unmaskOnReload
Server->>Browser: Render HTML including inline unmask script (with nonce if provided)
Browser->>Browser: Load HTML / begin hydration
Browser->>Script: Execute inline unmask script
Script->>Script: Read window.history.state?.__tempLocation
Script->>Script: Build RegExp list from embedded routeMaskSources
Script->>Script: Compare stored pathname vs current location
alt stored pathname differs and matches a mask
Script->>Browser: window.location.replace(stored pathname + search + hash)
Browser->>Router: Browser navigates / reloads to real route
else no relevant stored location or no match
Script->>Script: Do nothing
end
Router->>Browser: Hydration continues on current location
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 unit tests (beta)
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. Comment |
Contributor
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/src/headContentUtils.tsx`:
- Around line 458-478: The regex builder routePathToRegExpSource currently
yields "^$" for the root path because no segments are added; update
routePathToRegExpSource so that when routePath === "/" or when no non-empty
segments are present (i.e., regExpSource is still "^") it appends a "/" before
returning, producing "^/$"; make this change inside routePathToRegExpSource
(referencing the function name) so all root-path masks correctly match "/"
instead of the empty string.
🪄 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: cada9402-3d78-4982-bcf2-ef1523ec7a43
📒 Files selected for processing (2)
packages/react-router/src/headContentUtils.tsxpackages/react-router/tests/Scripts.test.tsx
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/react-router/src/headContentUtils.tsx (1)
404-408: ESLint rule configuration issue (not a code defect).The static analysis reports the
react-hooks/rules-of-hooksrule definition is missing. This is a linter configuration issue rather than a code problem. The eslint-disable comment follows the established pattern used throughout this file for theisServerconditional branching.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/react-router/src/headContentUtils.tsx` around lines 404 - 408, The eslint-disable-next-line comment suppressing react-hooks/rules-of-hooks for the useMemo that computes unmaskOnReloadScript (calling buildUnmaskOnReloadHeadScript with router and nonce) is a linter config workaround; instead remove that inline disable and fix the ESLint configuration to include the react-hooks plugin and rules (ensure "plugin:react-hooks/recommended" or "react-hooks/rules-of-hooks": "error" is enabled, and add an override if this file legitimately uses static conditionals), so the rule is present and the existing pattern for isServer branching remains valid without inline disables.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/react-router/src/headContentUtils.tsx`:
- Around line 404-408: The eslint-disable-next-line comment suppressing
react-hooks/rules-of-hooks for the useMemo that computes unmaskOnReloadScript
(calling buildUnmaskOnReloadHeadScript with router and nonce) is a linter config
workaround; instead remove that inline disable and fix the ESLint configuration
to include the react-hooks plugin and rules (ensure
"plugin:react-hooks/recommended" or "react-hooks/rules-of-hooks": "error" is
enabled, and add an override if this file legitimately uses static
conditionals), so the rule is present and the existing pattern for isServer
branching remains valid without inline disables.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bca4e4d7-9a61-42c6-92a3-943894c14e40
📒 Files selected for processing (4)
packages/react-router/src/headContentUtils.tsxpackages/router-core/src/index.tspackages/router-core/src/path.tspackages/router-core/tests/path.test.ts
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/router-core/tests/unmask-on-reload-script.test.ts (1)
9-15: Consider adding an escaping regression assertion.It would be useful to add one test that passes a mask source containing
</>and asserts the emitted script string does not contain a literal</script>sequence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/tests/unmask-on-reload-script.test.ts` around lines 9 - 15, Add a regression assertion to the existing test that calls getUnmaskOnReloadScript with a mask source containing angle-bracket characters (for example a string including "<" and ">" or the literal "</script>") and assert that the returned script string does not contain the raw literal "</script>" sequence; this verifies proper escaping/encoding in getUnmaskOnReloadScript so the emitted inline script cannot prematurely close a containing script tag.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/router-core/tests/unmask-on-reload-script.test.ts`:
- Around line 9-15: Add a regression assertion to the existing test that calls
getUnmaskOnReloadScript with a mask source containing angle-bracket characters
(for example a string including "<" and ">" or the literal "</script>") and
assert that the returned script string does not contain the raw literal
"</script>" sequence; this verifies proper escaping/encoding in
getUnmaskOnReloadScript so the emitted inline script cannot prematurely close a
containing script tag.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: a2982799-a78d-45a8-9cc0-44727d865c7d
📒 Files selected for processing (5)
packages/react-router/src/headContentUtils.tsxpackages/router-core/src/index.tspackages/router-core/src/unmask-on-reload-inline.tspackages/router-core/src/unmask-on-reload-script.tspackages/router-core/tests/unmask-on-reload-script.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/router-core/src/index.ts
tmm
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/react-router/src/headContentUtils.tsx (1)
432-436: Prefer omittingattrsinstead of setting it toundefined.Using conditional object spread keeps the tag shape tighter and avoids optional-property edge cases in stricter TS configurations.
Proposed refactor
return { tag: 'script', - attrs: nonce ? { nonce } : undefined, + ...(nonce ? { attrs: { nonce } } : {}), children: script, } satisfies RouterManagedTagAs per coding guidelines: "
**/*.{ts,tsx}: 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/react-router/src/headContentUtils.tsx` around lines 432 - 436, The returned tag object currently sets attrs to undefined when nonce is falsy; instead omit the attrs property entirely to avoid optional-property edge cases. Change the return to conditionally spread attrs only when nonce is present (e.g., spread {...(nonce ? { attrs: { nonce } } : {})}) so the object only contains attrs when nonce is truthy; keep tag: 'script', children: script and the trailing satisfies RouterManagedTag as-is. Ensure you reference the nonce variable and the RouterManagedTag assertion in headContentUtils.tsx.
🤖 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/unmask-on-reload-script.ts`:
- Around line 1-4: The import order breaks the import/order rule because the
type import AnyRoute and RouteMask is placed before a regular import
(escapeHtml); move the type-only import from './route' to after the other
non-type imports so all sibling type imports are grouped at the end—ensure
imports remain: minifiedUnmaskOnReloadScript, routePathToRegExpSource,
escapeHtml first, then import type { AnyRoute, RouteMask } from './route'.
---
Nitpick comments:
In `@packages/react-router/src/headContentUtils.tsx`:
- Around line 432-436: The returned tag object currently sets attrs to undefined
when nonce is falsy; instead omit the attrs property entirely to avoid
optional-property edge cases. Change the return to conditionally spread attrs
only when nonce is present (e.g., spread {...(nonce ? { attrs: { nonce } } :
{})}) so the object only contains attrs when nonce is truthy; keep tag:
'script', children: script and the trailing satisfies RouterManagedTag as-is.
Ensure you reference the nonce variable and the RouterManagedTag assertion in
headContentUtils.tsx.
🪄 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: da57c7cd-dd37-457e-b8ab-580f44471f44
📒 Files selected for processing (6)
.changeset/calmly-fox-smiled.md.changeset/swiftly-otter-jumped.mdpackages/react-router/src/headContentUtils.tsxpackages/router-core/src/index.tspackages/router-core/src/unmask-on-reload-script.tspackages/router-core/tests/unmask-on-reload-script.test.ts
✅ Files skipped from review due to trivial changes (2)
- .changeset/calmly-fox-smiled.md
- .changeset/swiftly-otter-jumped.md
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/router-core/tests/unmask-on-reload-script.test.ts
- packages/router-core/src/index.ts
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
1 participant
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.
SSR hard reloads on masked routes can lose the real route because the server only sees the masked URL, so hydration can drop modal or dialog UI that depends on the unmasked route.
This injects a small pre-hydration script for masks with
unmaskOnReloadthat restoreshistory.state.__tempLocationbefore hydration (while preserving the SSR nonce).Closes #7115.
Summary by CodeRabbit
New Features
Tests