fix(router-core): don't treat the current pathname as a route template when canonicalizing on load#7611
Open
Armin0001 wants to merge 1 commit into
Open
Conversation
…e when canonicalizing on load
On mount (Transitioner) and on server startup (beforeLoad) the current
location is rebuilt via buildLocation({ to: latestLocation.pathname }).
Interpolating that concrete pathname mangles segments that merely look
like params (possible with pathParamsAllowedCharacters: ['$']) into
"undefined" and commits the broken URL with replace: true.
buildLocation now takes an internal _concreteTo flag, set by the four
round-trip call sites: the pathname is never interpolated and a
routesByPath miss falls through to concrete route matching, so search
middlewares run for these URLs again.
Fixes TanStack#7598
Contributor
📝 WalkthroughWalkthroughThis PR fixes path parameters starting with ChangesConcrete Pathname Fix
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
.changeset/heavy-donkeys-wave.md (1)
8-8: 💤 Low valueUse hyphenated compound adjective: "full-page load".
In release notes, the compound adjective form
"full-page load"is preferred over"full page load"for consistency and clarity.-Fix path param values starting with `$` being rewritten to "undefined" on full page load. The canonical-URL check on mount and on server startup no longer treats the current concrete pathname as a route template. +Fix path param values starting with `$` being rewritten to "undefined" on full-page load. The canonical-URL check on mount and on server startup no longer treats the current concrete pathname as a route template.🤖 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 @.changeset/heavy-donkeys-wave.md at line 8, Update the release note text to use the hyphenated compound adjective "full-page load" instead of "full page load"; locate the sentence containing "Fix path param values starting with `$` being rewritten to \"undefined\" on full page load." and change it to read "Fix path param values starting with `$` being rewritten to \"undefined\" on full-page load." to maintain consistent style.packages/router-core/tests/build-location.test.ts (1)
2325-2334: ⚡ Quick winAvoid
as anyin the new helper and keep strict typing.Please type the
buildLocationoptions instead of casting toanyso this regression helper still benefits from strict-mode checks.Suggested refactor
function buildCurrentLocation(router: ReturnType<typeof setup>) { - return router.buildLocation({ + const opts: Parameters<typeof router.buildLocation>[0] = { to: router.latestLocation.pathname, search: true, params: true, hash: true, state: true, _includeValidateSearch: true, _concreteTo: true, - } as any) + } + return router.buildLocation(opts) }As per coding guidelines, "
**/*.{ts,tsx}: Use TypeScript strict mode with extensive type safety".🤖 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/tests/build-location.test.ts` around lines 2325 - 2334, The helper is using an unsafe cast "as any"; replace it with the actual options type for buildLocation by typing the object with Parameters<ReturnType<typeof setup>['buildLocation']>[0] (or a similarly derived type) and pass that typed object to router.buildLocation; update buildCurrentLocation to create a strongly-typed opts variable (e.g. const opts: Parameters<ReturnType<typeof setup>['buildLocation']>[0] = { ... }) and then return router.buildLocation(opts).Source: Coding guidelines
🤖 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.
Nitpick comments:
In @.changeset/heavy-donkeys-wave.md:
- Line 8: Update the release note text to use the hyphenated compound adjective
"full-page load" instead of "full page load"; locate the sentence containing
"Fix path param values starting with `$` being rewritten to \"undefined\" on
full page load." and change it to read "Fix path param values starting with `$`
being rewritten to \"undefined\" on full-page load." to maintain consistent
style.
In `@packages/router-core/tests/build-location.test.ts`:
- Around line 2325-2334: The helper is using an unsafe cast "as any"; replace it
with the actual options type for buildLocation by typing the object with
Parameters<ReturnType<typeof setup>['buildLocation']>[0] (or a similarly derived
type) and pass that typed object to router.buildLocation; update
buildCurrentLocation to create a strongly-typed opts variable (e.g. const opts:
Parameters<ReturnType<typeof setup>['buildLocation']>[0] = { ... }) and then
return router.buildLocation(opts).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 34da1c68-2e96-4058-8ca8-3f0d5564297d
📒 Files selected for processing (9)
.changeset/heavy-donkeys-wave.mdpackages/react-router/src/Transitioner.tsxpackages/react-router/tests/router.test.tsxpackages/router-core/src/RouterProvider.tspackages/router-core/src/router.tspackages/router-core/tests/build-location.test.tspackages/router-core/tests/load.test.tspackages/solid-router/src/Transitioner.tsxpackages/vue-router/src/Transitioner.tsx
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.
Fixes #7598
The bug
With
pathParamsAllowedCharacters: ['$']the router itself generates URLs whose param values start with$, e.g./files/$EXAMPLE_CODE%2Ffile.abapfor the route/files/$filePath. Client-side navigation works, but a full page load destroys the URL: it is rewritten to/files/undefinedviahistory.replaceStateanduseParams()returnsfilePath: "undefined".Root cause
On mount,
Transitionerrebuilds the current location throughbuildLocation({ to: router.latestLocation.pathname, ... })to check whether the URL needs canonicalizing (router.beforeLoaddoes the same on the server).buildLocationtreatstoas a route template, so the concrete segment$EXAMPLE_CODE%2Ffile.abapis parsed as a param placeholder. No such param exists, andinterpolatePathfalls back to"undefined". The same ambiguity makes theroutesByPathlookup miss, sodestRoutescomes back empty and search middlewares are skipped for these URLs.The fix
buildLocationgets an internal_concreteToflag, set by the four current-location round-trip call sites (react/solid/vueTransitionerandrouter.beforeLoad). With the flag set,tois never interpolated (a concrete pathname has nothing to interpolate) and aroutesByPathmiss falls through to concrete route matching instead of bailing out todestRoutes = []. All otherbuildLocation/navigatecallers are unchanged, in particular the template-miss behavior for anavigate({ to })containing$.Changed tests
The three
load.test.tscases added in #6882 asserted the old behavior (server startup at/$a,/$toString,/$__proto__redirects to/undefined). They now assert that the path is preserved and matched as a param value. That keeps their original point - inherited object properties must not leak into the URL - and also covers what #6881 originally reported:/$toStringno longer redirects at all.New regression tests: the round-trip scenarios in
build-location.test.ts(pathname preserved, search middlewares applied to a$-containing concrete pathname) and an integration test inreact-router/tests/router.test.tsxreproducing the reload scenario from #7598. Both fail without the fix.Summary by CodeRabbit
$characters were being incorrectly rewritten to"undefined"during page reloads or server-side startup. The canonical URL handling has been improved to correctly preserve these parameter values in your routes across all supported routers.