feat: match params#7263
Merged
Merged
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:
📝 WalkthroughWalkthroughIntroduces experimental route-matching controls ( Changes
Sequence Diagram(s)sequenceDiagram
participant Browser
participant RouterProvider
participant RouterCore
participant RouteMatcher
participant RouteComponent
Browser->>RouterProvider: navigation event / link click
RouterProvider->>RouterCore: buildLocation / resolve destination
RouterCore->>RouteMatcher: findRouteMatch(rawPath, rawParams)
RouteMatcher->>RouteMatcher: invoke params.match? / params.parse?
alt params.match returns false or parse returns false
RouteMatcher-->>RouterCore: skip route -> try next candidate
else params.match/parse pass
RouteMatcher-->>RouterCore: selected match (typed params)
end
RouterCore->>RouteComponent: render selected route with params
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
|
View your CI Pipeline Execution ↗ for commit af011ad
☁️ Nx Cloud last updated this comment at |
Contributor
🚀 Changeset Version Preview5 package(s) bumped directly, 18 bumped as dependents. 🟨 Minor bumps
🟩 Patch bumps
|
Contributor
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
Contributor
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/new-process-route-tree.ts (1)
473-478:⚠️ Potential issue | 🟠 MajorSort
pathlesscandidates before traversal.
matchPriorityis applied todynamic/optional/wildcardnodes, but not tonode.pathless. That means two sibling pathless layouts that both useparams.matchare still resolved by declaration order, so a lower-priority matcher can shadow a higher-priority one.Suggested fix
function sortTreeNodes(node: SegmentNode<RouteLike>) { if (node.pathless) { + node.pathless.sort((a, b) => { + if (a.skipOnParamError && !b.skipOnParamError) return -1 + if (!a.skipOnParamError && b.skipOnParamError) return 1 + if (a.skipOnParamError && b.skipOnParamError) { + return b.matchPriority - a.matchPriority + } + return 0 + }) for (const child of node.pathless) { sortTreeNodes(child) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/new-process-route-tree.ts` around lines 473 - 478, The traversal in sortTreeNodes currently recurses over node.pathless in declaration order, letting lower-priority pathless layouts shadow higher-priority ones; update sortTreeNodes to sort node.pathless by the same matchPriority logic used for dynamic/optional/wildcard nodes before iterating (i.e., compute each child's matchPriority and sort the node.pathless array descending so higher-priority candidates are visited first), then recurse as before into each child; reference the existing sortTreeNodes function and the node.pathless children and reuse the same matchPriority comparator used elsewhere to ensure consistent ordering.
🤖 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/new-process-route-tree.ts`:
- Around line 473-478: The traversal in sortTreeNodes currently recurses over
node.pathless in declaration order, letting lower-priority pathless layouts
shadow higher-priority ones; update sortTreeNodes to sort node.pathless by the
same matchPriority logic used for dynamic/optional/wildcard nodes before
iterating (i.e., compute each child's matchPriority and sort the node.pathless
array descending so higher-priority candidates are visited first), then recurse
as before into each child; reference the existing sortTreeNodes function and the
node.pathless children and reuse the same matchPriority comparator used
elsewhere to ensure consistent ordering.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b0707134-a8d1-4562-bb36-dc542db09dde
📥 Commits
Reviewing files that changed from the base of the PR and between 97207a1 and 4e4130cd14ed36dd50bf272693c0cd73b32a86de.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (20)
docs/router/api/router/RouteOptionsType.mde2e/react-router/match-params/.gitignoree2e/react-router/match-params/index.htmle2e/react-router/match-params/package.jsone2e/react-router/match-params/playwright.config.tse2e/react-router/match-params/src/main.tsxe2e/react-router/match-params/tests/match-params.spec.tse2e/react-router/match-params/tsconfig.jsone2e/react-router/match-params/vite.config.jspackages/react-router/src/index.tsxpackages/router-core/src/index.tspackages/router-core/src/new-process-route-tree.tspackages/router-core/src/route.tspackages/router-core/src/router.tspackages/router-core/tests/build-location.test.tspackages/router-core/tests/match-params.test.tspackages/router-core/tests/new-process-route-tree.test.tspackages/router-core/tests/skip-route-on-parse-error.test.tspackages/solid-router/src/index.tsxpackages/vue-router/src/index.tsx
💤 Files with no reviewable changes (1)
- packages/router-core/tests/skip-route-on-parse-error.test.ts
Sheraff
reviewed
Apr 25, 2026
Merging this PR will not alter performance
Comparing 
Footnotes
|
6fec866 to
db920c6
Compare
Contributor
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/new-process-route-tree.ts (1)
1019-1025:⚠️ Potential issue | 🟠 MajorHonor
params.matchbefore taking the root-index shortcut.This fast path now skips
validateMatchParams, so a guarded root index route still matches/even when its matcher returnsfalse. It also prevents fallback to sibling optional/wildcard routes. The safest fix is to only keep the shortcut for unguarded root indexes and let guarded ones fall through to the normal stack logic.Suggested fix
- if (path === '/' && segmentTree.index) + if (path === '/' && segmentTree.index && !segmentTree.index.match) return { node: segmentTree.index, skipped: 0 } as Pick< Frame, 'node' | 'skipped' >🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/new-process-route-tree.ts` around lines 1019 - 1025, The root-index fast path incorrectly bypasses guarded matchers; change the shortcut so it only returns the index immediately when path === '/' and segmentTree.index exists AND that index has no matcher to validate (i.e., params.match is undefined/not present) — otherwise fall through to the existing stack logic which calls validateMatchParams. Locate the block that returns { node: segmentTree.index, skipped: 0 } and add a guard checking the index node's params.match (or equivalent matcher field) is absent before returning, ensuring guarded root indexes are validated by validateMatchParams instead of being short-circuited.
🧹 Nitpick comments (3)
e2e/react-router/match-params/src/main.tsx (2)
235-239: Avoid non-null assertion for the mount element.Using
!here bypasses runtime safety. A guard keeps the failure explicit and type-safe.Proposed fix
-const rootElement = document.getElementById('app')! +const rootElement = document.getElementById('app') -if (!rootElement.innerHTML) { +if (!rootElement) { + throw new Error('Missing `#app` mount element') +} + +if (!rootElement.innerHTML) { ReactDOM.createRoot(rootElement).render(<RouterProvider router={router} />) }As 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 `@e2e/react-router/match-params/src/main.tsx` around lines 235 - 239, Remove the non-null assertion on document.getElementById('app') and add an explicit runtime guard: change the const rootElement = document.getElementById('app')! to const rootElement = document.getElementById('app'); then check if (!rootElement) { throw new Error('Mount element "app" not found'); } (or log and exit), and only call ReactDOM.createRoot(rootElement).render(<RouterProvider router={router} />) when rootElement exists; update references in this block (rootElement and ReactDOM.createRoot) accordingly to satisfy TypeScript strict mode and keep failure explicit.
81-84: Makelanguageparsing explicit instead of defaulting unknown values topl-PL.The current fallback silently coerces unexpected values. Prefer explicit branches so parsing remains invariant with the match contract.
Proposed fix
parse: ({ language }) => { if (language === 'en') return { language: 'en-US' as const } - return { language: 'pl-PL' as const } + if (language === 'pl') return { language: 'pl-PL' as const } + throw new Error(`Unexpected language segment: ${language}`) },As 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 `@e2e/react-router/match-params/src/main.tsx` around lines 81 - 84, The parse function currently defaults any non-'en' value to 'pl-PL'; change the parse implementation for the parse({ language }) handler to be explicit: handle 'en' (return { language: 'en-US' }) and handle 'pl' (return { language: 'pl-PL' }), and for any other input do not coerce—either throw an Error (e.g. "Unsupported language") or return undefined so the match fails; update the parse function in main.tsx accordingly to ensure it only accepts explicit supported values.packages/router-core/src/router.ts (1)
1357-1364: ClearrouteBranchCachewhen route trees are rebuilt.
getRouteBranchcaches by route object identity, but the cache is never reset whenupdate()swaps/reprocesses the route tree. If route objects are reused across updates (eg HMR-like flows), cached ancestry can go stale and produce wrong middleware/params-stringify traversal inbuildLocation.♻️ Proposed fix
setRoutes({ routesById, routesByPath, processedTree, }: ProcessRouteTreeResult<TRouteTree>) { + this.routeBranchCache = new WeakMap() this.routesById = routesById as RoutesById<TRouteTree> this.routesByPath = routesByPath as RoutesByPath<TRouteTree> this.processedTree = processedTree🤖 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 1357 - 1364, getRouteBranch caches branches by route object identity using routeBranchCache but the cache is never invalidated when the router's route tree is rebuilt; update() should clear or reset routeBranchCache whenever the route tree is swapped/reprocessed so stale branches (from reused route objects) are not returned. Modify the update() implementation to call this.routeBranchCache.clear() (or replace the Map) immediately after the route tree is replaced/reprocessed (the code paths that reconstruct routes), so getRouteBranch and buildRouteBranch always compute branches against the current tree.
🤖 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/match-params/src/main.tsx`:
- Around line 108-114: The numeric route matcher currently uses
Number.isInteger(Number(userId)) which allows scientific notation and signed
forms; change the match function for the route (the match: ({ userId }) => ...)
to use a strict digits-only regex test like /^\d+$/. After validating with the
regex, keep parse: ({ userId }) => ({ userId: Number(userId) }) to convert the
string to a number. Apply the same change to the other numeric matcher block
later in the file (the similar match/parse pair around lines 140-146).
---
Outside diff comments:
In `@packages/router-core/src/new-process-route-tree.ts`:
- Around line 1019-1025: The root-index fast path incorrectly bypasses guarded
matchers; change the shortcut so it only returns the index immediately when path
=== '/' and segmentTree.index exists AND that index has no matcher to validate
(i.e., params.match is undefined/not present) — otherwise fall through to the
existing stack logic which calls validateMatchParams. Locate the block that
returns { node: segmentTree.index, skipped: 0 } and add a guard checking the
index node's params.match (or equivalent matcher field) is absent before
returning, ensuring guarded root indexes are validated by validateMatchParams
instead of being short-circuited.
---
Nitpick comments:
In `@e2e/react-router/match-params/src/main.tsx`:
- Around line 235-239: Remove the non-null assertion on
document.getElementById('app') and add an explicit runtime guard: change the
const rootElement = document.getElementById('app')! to const rootElement =
document.getElementById('app'); then check if (!rootElement) { throw new
Error('Mount element "app" not found'); } (or log and exit), and only call
ReactDOM.createRoot(rootElement).render(<RouterProvider router={router} />) when
rootElement exists; update references in this block (rootElement and
ReactDOM.createRoot) accordingly to satisfy TypeScript strict mode and keep
failure explicit.
- Around line 81-84: The parse function currently defaults any non-'en' value to
'pl-PL'; change the parse implementation for the parse({ language }) handler to
be explicit: handle 'en' (return { language: 'en-US' }) and handle 'pl' (return
{ language: 'pl-PL' }), and for any other input do not coerce—either throw an
Error (e.g. "Unsupported language") or return undefined so the match fails;
update the parse function in main.tsx accordingly to ensure it only accepts
explicit supported values.
In `@packages/router-core/src/router.ts`:
- Around line 1357-1364: getRouteBranch caches branches by route object identity
using routeBranchCache but the cache is never invalidated when the router's
route tree is rebuilt; update() should clear or reset routeBranchCache whenever
the route tree is swapped/reprocessed so stale branches (from reused route
objects) are not returned. Modify the update() implementation to call
this.routeBranchCache.clear() (or replace the Map) immediately after the route
tree is replaced/reprocessed (the code paths that reconstruct routes), so
getRouteBranch and buildRouteBranch always compute branches against the current
tree.
🪄 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: 1243a0b5-386d-4e2a-a4f6-24021fa0e42a
📥 Commits
Reviewing files that changed from the base of the PR and between 4e4130cd14ed36dd50bf272693c0cd73b32a86de and 6fec86621ddde1e996fc9d36e7032a99e16ec48b.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
.changeset/clean-routes-match.mddocs/router/api/router/RouteOptionsType.mde2e/react-router/match-params/.gitignoree2e/react-router/match-params/index.htmle2e/react-router/match-params/package.jsone2e/react-router/match-params/playwright.config.tse2e/react-router/match-params/src/main.tsxe2e/react-router/match-params/tests/match-params.spec.tse2e/react-router/match-params/tsconfig.jsone2e/react-router/match-params/vite.config.jspackages/react-router/src/index.tsxpackages/router-core/src/index.tspackages/router-core/src/new-process-route-tree.tspackages/router-core/src/path.tspackages/router-core/src/route.tspackages/router-core/src/router.tspackages/router-core/tests/build-location.test.tspackages/router-core/tests/match-params.test.tspackages/router-core/tests/new-process-route-tree.test.tspackages/router-core/tests/path.test.tspackages/router-core/tests/skip-route-on-parse-error.test.tspackages/solid-router/src/index.tsxpackages/vue-router/src/index.tsx
💤 Files with no reviewable changes (1)
- packages/router-core/tests/skip-route-on-parse-error.test.ts
✅ Files skipped from review due to trivial changes (9)
- packages/vue-router/src/index.tsx
- e2e/react-router/match-params/index.html
- e2e/react-router/match-params/vite.config.js
- packages/solid-router/src/index.tsx
- packages/react-router/src/index.tsx
- e2e/react-router/match-params/.gitignore
- .changeset/clean-routes-match.md
- packages/router-core/src/index.ts
- e2e/react-router/match-params/package.json
🚧 Files skipped from review as they are similar to previous changes (5)
- docs/router/api/router/RouteOptionsType.md
- e2e/react-router/match-params/tests/match-params.spec.ts
- e2e/react-router/match-params/playwright.config.ts
- packages/router-core/tests/build-location.test.ts
- e2e/react-router/match-params/tsconfig.json
Comment on lines
+108
to
+114
| match: ({ userId }) => { | ||
| const parsed = Number(userId) | ||
| return Number.isInteger(parsed) | ||
| }, | ||
| parse: ({ userId }) => { | ||
| return { userId: Number(userId) } | ||
| }, |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n e2e/react-router/match-params/src/main.tsx | sed -n '100,150p'Repository: TanStack/router
Length of output: 1658
🏁 Script executed:
python3 << 'EOF'
# Test Number.isInteger() behavior with the claimed examples
test_values = ["1", "1e2", "+7", "-1", "123abc", "123"]
for val in test_values:
# Simulate Number(val) in JavaScript -> converts to number or NaN
try:
if val == "123abc":
num_result = float('nan') # Number("123abc") returns NaN in JS
else:
num_result = float(val)
is_integer = (num_result == int(num_result)) if num_result == num_result else False # NaN check
print(f"Number('{val}') = {num_result}, Number.isInteger() = {is_integer}")
except ValueError:
print(f"Number('{val}') = NaN, Number.isInteger() = False")
EOFRepository: TanStack/router
Length of output: 346
🏁 Script executed:
# Look for any tests related to numeric params matching
fd -e test -e spec | head -20
rg "numericUserRoute|numericOrgRoute" --type tsx --type tsRepository: TanStack/router
Length of output: 87
🏁 Script executed:
# Check if there's any documentation or comments about the intended behavior
rg -A 5 -B 5 "match.*userId" e2e/react-router/match-params/src/main.tsx | head -50Repository: TanStack/router
Length of output: 345
🏁 Script executed:
# Check if there are e2e tests that verify the numeric matching behavior
rg "1e2|\\+7|-1" e2e/react-router/match-params/ --type ts --type jsRepository: TanStack/router
Length of output: 41
🏁 Script executed:
# Look for test files related to match-params
find e2e/react-router/match-params -name "*.test.*" -o -name "*.spec.*" | head -10Repository: TanStack/router
Length of output: 116
🏁 Script executed:
# Check the full context of the file to understand the purpose of numericUserRoute
cat -n e2e/react-router/match-params/src/main.tsx | head -80Repository: TanStack/router
Length of output: 2481
🏁 Script executed:
# Look for comments or documentation about the intended matching behavior
rg -B 10 "numericUserRoute|numericOrgRoute" e2e/react-router/match-params/src/main.tsx | grep -A 10 -B 10 "match\|parse"Repository: TanStack/router
Length of output: 1474
🏁 Script executed:
cat -n e2e/react-router/match-params/tests/match-params.spec.tsRepository: TanStack/router
Length of output: 2988
🏁 Script executed:
# Check if there are any issues or PRs discussing this specific behavior
git log --oneline --all --grep="numeric.*match\|match.*param" | head -20Repository: TanStack/router
Length of output: 954
🏁 Script executed:
# Look at the route tree structure to understand how routes are prioritized
cat -n e2e/react-router/match-params/src/main.tsx | sed -n '240,260p'Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Check if there are explicit numeric validation patterns elsewhere in the router codebase
rg "\\^\\\\d\|test.*digit\|numeric.*regex" packages/ --type ts --type tsx -A 2 | head -40Repository: TanStack/router
Length of output: 87
🏁 Script executed:
# Look at the git history for the match-params e2e test to understand intent
git show 6fec86621 --statRepository: TanStack/router
Length of output: 50372
🏁 Script executed:
# Check if there are any tests that explicitly test scientific notation or special numeric forms
rg "1e2|\\+.*userId|\\-.*userId|\\.isInteger" e2e/react-router/match-params/tests/Repository: TanStack/router
Length of output: 41
🏁 Script executed:
# Look for any documentation about the params.match intent
rg -B 5 -A 5 "match.*intent\|match.*behavior\|numeric.*routing" e2e/react-router/match-params/ --type tsRepository: TanStack/router
Length of output: 41
🏁 Script executed:
# Check if there are any discussions in recent commits about this behavior
git log --oneline --all --since="2025-01-01" | grep -i "match\|param" | head -20Repository: TanStack/router
Length of output: 1757
🏁 Script executed:
# Double-check: verify the test only uses normal digits and strings, no scientific notation
grep -n "goto.*users\|goto.*orgs" e2e/react-router/match-params/tests/match-params.spec.tsRepository: TanStack/router
Length of output: 258
Numeric route matchers are too permissive for digit-only matching.
Number.isInteger(Number(param)) accepts scientific notation (e.g., 1e2 → 100), signed numbers (e.g., +7, -1), and other coercible forms. This allows unintended paths like /users/1e2 to match the numeric route. The e2e tests expect only plain digit strings to match (e.g., /users/123 matches, /users/alice does not).
Use /^\d+$/ to enforce strict digit-only validation:
Proposed fix
+const digitsOnly = /^\d+$/
+
const numericUserRoute = createRoute({
getParentRoute: () => usersRoute,
path: '$userId',
params: {
match: ({ userId }) => {
- const parsed = Number(userId)
- return Number.isInteger(parsed)
+ return digitsOnly.test(userId)
},
parse: ({ userId }) => {
- return { userId: Number(userId) }
+ return { userId: Number.parseInt(userId, 10) }
},
stringify: ({ userId }) => ({ userId: String(userId) }),
},
component: NumericUserComponent,
})
@@
const numericOrgRoute = createRoute({
getParentRoute: () => rootRoute,
path: '/orgs/$orgId',
params: {
match: ({ orgId }) => {
- const parsed = Number(orgId)
- return Number.isInteger(parsed)
+ return digitsOnly.test(orgId)
},
parse: ({ orgId }) => {
- return { orgId: Number(orgId) }
+ return { orgId: Number.parseInt(orgId, 10) }
},
stringify: ({ orgId }) => ({ orgId: String(orgId) }),
},
})Also applies to: 140-146
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@e2e/react-router/match-params/src/main.tsx` around lines 108 - 114, The
numeric route matcher currently uses Number.isInteger(Number(userId)) which
allows scientific notation and signed forms; change the match function for the
route (the match: ({ userId }) => ...) to use a strict digits-only regex test
like /^\d+$/. After validating with the regex, keep parse: ({ userId }) => ({
userId: Number(userId) }) to convert the string to a number. Apply the same
change to the other numeric matcher block later in the file (the similar
match/parse pair around lines 140-146).
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/router-core/src/route.ts (1)
188-194: Consider documentingparams.matchandparams.matchPriorityinline.These are user-facing API knobs; short JSDoc here would reduce misuse and clarify priority semantics.
📝 Suggested doc-only improvement
export type ParamsOptions<in out TPath extends string, in out TParams> = { params?: { + /** + * Boolean predicate evaluated during route matching using raw path params. + * Return `true` to allow this route candidate, `false` to skip it. + */ match?: MatchParamsFn<TPath, TParams> + /** + * Higher values are evaluated first when multiple sibling routes define `params.match`. + * `@default` 0 + */ matchPriority?: number parse?: ParseParamsFn<TPath, TParams> stringify?: StringifyParamsFn<TPath, TParams> }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/route.ts` around lines 188 - 194, Add concise JSDoc on the ParamsOptions type for the params.match and params.matchPriority fields: describe that params.match is a user-supplied matcher implementing MatchParamsFn<TPath,TParams> used to determine whether a path matches and to extract params, and note expected return/side-effect semantics (e.g., truthy for match + extracted params); for params.matchPriority document that it is a numeric tie-breaker used when multiple matchers apply (higher number wins, default value if omitted) and how it interacts with router selection. Place these inline above the params.match and params.matchPriority declarations in the ParamsOptions type so users see the API contract and priority semantics immediately.
🤖 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/src/route.ts`:
- Around line 188-194: Add concise JSDoc on the ParamsOptions type for the
params.match and params.matchPriority fields: describe that params.match is a
user-supplied matcher implementing MatchParamsFn<TPath,TParams> used to
determine whether a path matches and to extract params, and note expected
return/side-effect semantics (e.g., truthy for match + extracted params); for
params.matchPriority document that it is a numeric tie-breaker used when
multiple matchers apply (higher number wins, default value if omitted) and how
it interacts with router selection. Place these inline above the params.match
and params.matchPriority declarations in the ParamsOptions type so users see the
API contract and priority semantics immediately.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f526d26-a7b0-4c10-bfb2-e5c11a005420
📥 Commits
Reviewing files that changed from the base of the PR and between 6fec86621ddde1e996fc9d36e7032a99e16ec48b and db920c6.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (23)
.changeset/clean-routes-match.mddocs/router/api/router/RouteOptionsType.mde2e/react-router/match-params/.gitignoree2e/react-router/match-params/index.htmle2e/react-router/match-params/package.jsone2e/react-router/match-params/playwright.config.tse2e/react-router/match-params/src/main.tsxe2e/react-router/match-params/tests/match-params.spec.tse2e/react-router/match-params/tsconfig.jsone2e/react-router/match-params/vite.config.jspackages/react-router/src/index.tsxpackages/router-core/src/index.tspackages/router-core/src/new-process-route-tree.tspackages/router-core/src/path.tspackages/router-core/src/route.tspackages/router-core/src/router.tspackages/router-core/tests/build-location.test.tspackages/router-core/tests/match-params.test.tspackages/router-core/tests/new-process-route-tree.test.tspackages/router-core/tests/path.test.tspackages/router-core/tests/skip-route-on-parse-error.test.tspackages/solid-router/src/index.tsxpackages/vue-router/src/index.tsx
💤 Files with no reviewable changes (1)
- packages/router-core/tests/skip-route-on-parse-error.test.ts
✅ Files skipped from review due to trivial changes (9)
- e2e/react-router/match-params/vite.config.js
- e2e/react-router/match-params/index.html
- packages/react-router/src/index.tsx
- packages/solid-router/src/index.tsx
- e2e/react-router/match-params/.gitignore
- e2e/react-router/match-params/tsconfig.json
- .changeset/clean-routes-match.md
- e2e/react-router/match-params/package.json
- packages/router-core/src/router.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- packages/vue-router/src/index.tsx
- docs/router/api/router/RouteOptionsType.md
- packages/router-core/tests/path.test.ts
- e2e/react-router/match-params/playwright.config.ts
- packages/router-core/tests/build-location.test.ts
- e2e/react-router/match-params/src/main.tsx
- packages/router-core/src/new-process-route-tree.ts
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
ace7bb7 to
af0a966
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
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/new-process-route-tree.ts (1)
431-458:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon't let
parseoutrank more specific dynamic patterns.Putting the
parsecheck before prefix/suffix and case-sensitivity means a guarded plain route like/a/$idcan win over a more specific sibling such as/a/file{$slug}.txtwhenever its guard returns truthy. That changes matching semantics for concrete paths instead of only breaking ties between otherwise equivalent dynamic routes.Suggested comparator order
function sortDynamic( a: { prefix?: string suffix?: string caseSensitive: boolean parse: null | ((params: Record<string, string>) => unknown) }, b: { prefix?: string suffix?: string caseSensitive: boolean parse: null | ((params: Record<string, string>) => unknown) }, ) { - if (a.parse && !b.parse) return -1 - if (!a.parse && b.parse) return 1 if (a.prefix && b.prefix && a.prefix !== b.prefix) { if (a.prefix.startsWith(b.prefix)) return -1 if (b.prefix.startsWith(a.prefix)) return 1 } if (a.suffix && b.suffix && a.suffix !== b.suffix) { if (a.suffix.endsWith(b.suffix)) return -1 if (b.suffix.endsWith(a.suffix)) return 1 } if (a.prefix && !b.prefix) return -1 if (!a.prefix && b.prefix) return 1 if (a.suffix && !b.suffix) return -1 if (!a.suffix && b.suffix) return 1 if (a.caseSensitive && !b.caseSensitive) return -1 if (!a.caseSensitive && b.caseSensitive) return 1 + if (a.parse && !b.parse) return -1 + if (!a.parse && b.parse) return 1 return 0 }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/new-process-route-tree.ts` around lines 431 - 458, The comparator currently checks a.parse vs b.parse first, which lets guarded dynamic routes outrank more specific patterns; move the parse presence checks (the lines "if (a.parse && !b.parse) ... if (!a.parse && b.parse) ...") so they occur after the prefix, suffix and caseSensitive comparisons (i.e., after the blocks that compare a.prefix/a.suffix and a.caseSensitive) so that parse only breaks ties among otherwise equal-specific routes; keep the final "return 0" to preserve stable declaration order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/router/api/router/RouteOptionsType.md`:
- Around line 86-97: Update the docs for params.parse to document that it may
return false to skip a route (same behavior as throwing), mention that this
interacts with params.match/params.matchPriority so routes can be skipped either
during match (params.match returning false) or during parse (params.parse
returning false or throwing), and add a short note in the RouteOptionsType.md
section for params.parse explaining returning false skips the route and how that
affects matching priority/tie-breaking with params.matchPriority; reference
params.parse, params.match, and params.matchPriority in the text so readers can
discover the migration path.
In `@packages/router-core/src/router.ts`:
- Line 975: The routeBranchCache (private routeBranchCache) is never cleared
when update() replaces the route tree from processRouteTree, so buildLocation()
may use stale ancestor chains; modify update() (the code that applies
processRouteTree results) to invalidate/clear routeBranchCache (e.g.,
routeBranchCache = new WeakMap() or routeBranchCache.clear()) whenever the route
tree is rebuilt, and apply the same invalidation in the other code paths that
rebuild the tree (the alternate update/path-rebuild block corresponding to the
second region referenced) so buildLocation() and related logic always compute
fresh ancestor chains.
In `@packages/solid-router/tests/useParams.test.tsx`:
- Around line 191-192: The test's flaky assertion checks a dev-only parse side
effect via mockedfn; make it env-aware by gating that expectation on the runtime
environment (e.g. only call expect(mockedfn).toHaveBeenCalled() when
process.env.NODE_ENV !== 'production'), or better replace the parse-side-effect
assertion with a stable assertion of the rendered navigation result (assert the
DOM/route outcome produced by the navigation instead of relying on mockedfn).
Locate the assertion referencing mockedfn in the useParams.test.tsx test and
update it accordingly.
In `@packages/vue-router/tests/useParams.test.tsx`:
- Around line 196-197: The test's expect(mockedfn).toHaveBeenCalled() depends on
a dev-only side-effect; make the assertion env-aware by either wrapping it in a
conditional: if (process.env.NODE_ENV !== 'production')
expect(mockedfn).toHaveBeenCalled(), or replace it with a stable assertion that
checks the navigation/rendered state instead (e.g., assert the expected
DOM/render result or router state) so the test doesn't rely on the dev-only
roundtrip validation; locate the usage of mockedfn in the useParams.test.tsx
test and apply one of these fixes.
---
Outside diff comments:
In `@packages/router-core/src/new-process-route-tree.ts`:
- Around line 431-458: The comparator currently checks a.parse vs b.parse first,
which lets guarded dynamic routes outrank more specific patterns; move the parse
presence checks (the lines "if (a.parse && !b.parse) ... if (!a.parse &&
b.parse) ...") so they occur after the prefix, suffix and caseSensitive
comparisons (i.e., after the blocks that compare a.prefix/a.suffix and
a.caseSensitive) so that parse only breaks ties among otherwise equal-specific
routes; keep the final "return 0" to preserve stable declaration order.
🪄 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: a6176814-b269-401e-83e7-25bb7e14a8ab
📥 Commits
Reviewing files that changed from the base of the PR and between db920c6 and af0a966bf89992c4dbc215ffcf7c7be157821d5e.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
.changeset/clean-routes-match.mddocs/router/api/router/RouteOptionsType.mde2e/react-router/match-params/.gitignoree2e/react-router/match-params/index.htmle2e/react-router/match-params/package.jsone2e/react-router/match-params/playwright.config.tse2e/react-router/match-params/src/main.tsxe2e/react-router/match-params/tests/match-params.spec.tse2e/react-router/match-params/tsconfig.jsone2e/react-router/match-params/vite.config.jspackages/react-router/src/RouterProvider.tsxpackages/react-router/src/link.tsxpackages/react-router/tests/useParams.test.tsxpackages/router-core/src/index.tspackages/router-core/src/new-process-route-tree.tspackages/router-core/src/path.tspackages/router-core/src/route.tspackages/router-core/src/router.tspackages/router-core/src/utils.tspackages/router-core/tests/build-location.test.tspackages/router-core/tests/match-params.test.tspackages/router-core/tests/new-process-route-tree.test.tspackages/router-core/tests/path.test.tspackages/router-core/tests/skip-route-on-parse-error.test.tspackages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsxpackages/solid-router/src/link.tsxpackages/solid-router/tests/useParams.test.tsxpackages/start-plugin-core/src/rsbuild/plugin.tspackages/start-plugin-core/src/start-compiler/handleCreateServerFn.tspackages/vue-router/src/link.tsxpackages/vue-router/tests/useParams.test.tsx
💤 Files with no reviewable changes (1)
- packages/router-core/tests/skip-route-on-parse-error.test.ts
✅ Files skipped from review due to trivial changes (10)
- e2e/react-router/match-params/.gitignore
- packages/start-plugin-core/src/start-compiler/handleCreateServerFn.ts
- packages/solid-router/src/link.tsx
- .changeset/clean-routes-match.md
- packages/start-plugin-core/src/rsbuild/plugin.ts
- e2e/react-router/match-params/package.json
- e2e/react-router/match-params/tsconfig.json
- packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx
- packages/vue-router/src/link.tsx
- e2e/react-router/match-params/tests/match-params.spec.ts
🚧 Files skipped from review as they are similar to previous changes (7)
- e2e/react-router/match-params/vite.config.js
- e2e/react-router/match-params/index.html
- packages/router-core/src/route.ts
- packages/router-core/tests/build-location.test.ts
- packages/router-core/src/index.ts
- e2e/react-router/match-params/playwright.config.ts
- e2e/react-router/match-params/src/main.tsx
af0a966 to
af011ad
Compare
e0dd96c to
056e0b6
Compare
Contributor
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/new-process-route-tree.ts (1)
427-459:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep structural specificity ahead of
parseprecedence.Comparing
a.parse/b.parsebefore prefix/suffix makes a generic parsed route like/$idoutrank a more specific unparsed route like/prefix{$id}for/prefix123. That changes the existing specificity rules and can send matching to the wrong branch even when both candidates match.parseshould only break ties after the structural comparisons.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/router-core/src/new-process-route-tree.ts` around lines 427 - 459, In sortDynamic, the parse-precedence checks (a.parse / b.parse) must be moved after the structural comparisons so structural specificity (a.prefix / b.prefix, a.suffix / b.suffix, and a.caseSensitive / b.caseSensitive) wins first; update the function so you first compare prefix startsWith logic, suffix endsWith logic, presence of prefix/suffix, and caseSensitive, and only if those yield equality evaluate a.parse vs b.parse to break ties (preserving the final return 0 for stable sort).
♻️ Duplicate comments (1)
packages/router-core/src/router.ts (1)
1357-1364:⚠️ Potential issue | 🟠 Major | ⚡ Quick winClear
routeBranchCachewhen the route tree is rebuilt.
buildRouteBranch()is cached by route object identity, butprocessRouteTree()reinitializesparentRouteon reused route instances duringupdate(). SincesetRoutes()never resets thisWeakMap, a rebuild can keep returning stale ancestor chains here and make the exact-path fast path use the wrong route branch.Suggested fix
setRoutes({ routesById, routesByPath, processedTree, }: ProcessRouteTreeResult<TRouteTree>) { this.routesById = routesById as RoutesById<TRouteTree> this.routesByPath = routesByPath as RoutesByPath<TRouteTree> this.processedTree = processedTree + this.routeBranchCache = new WeakMap() const notFoundRoute = this.options.notFoundRoute🤖 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 1357 - 1364, The routeBranchCache WeakMap can hold stale branch data when the route tree is rebuilt; update setRoutes (or the function that fully reconstructs parentRoute, e.g., processRouteTree/update) to clear or reinitialize routeBranchCache so getRouteBranch and buildRouteBranch always compute fresh branches for the new tree; specifically, add a routeBranchCache.clear() (or assign a new WeakMap to routeBranchCache) at the start/end of setRoutes/processRouteTree so that cached entries keyed by old route identity are invalidated when parentRoute relationships are reset.
🤖 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/router.ts`:
- Around line 1873-1896: The fast-path uses routesByPath and
this.getRouteBranch(destRoute) which can skip runtime matching (and thus
route-level validateSearch/search middlewares); instead call
this.getMatchedRoutes(nextTo) and use its matchedRoutes unless the matched leaf
equals the routesByPath entry. Concretely, after resolving destRoute from
routesByPath, compute destMatchResult = this.getMatchedRoutes(nextTo) and set
destRoutes = destMatchResult.matchedRoutes; only if destMatchResult.foundRoute
=== destRoute (i.e., the matched leaf is the same route object) may you replace
destRoutes with this.getRouteBranch(destRoute); otherwise keep matchedRoutes so
validateSearch and middlewares run.
---
Outside diff comments:
In `@packages/router-core/src/new-process-route-tree.ts`:
- Around line 427-459: In sortDynamic, the parse-precedence checks (a.parse /
b.parse) must be moved after the structural comparisons so structural
specificity (a.prefix / b.prefix, a.suffix / b.suffix, and a.caseSensitive /
b.caseSensitive) wins first; update the function so you first compare prefix
startsWith logic, suffix endsWith logic, presence of prefix/suffix, and
caseSensitive, and only if those yield equality evaluate a.parse vs b.parse to
break ties (preserving the final return 0 for stable sort).
---
Duplicate comments:
In `@packages/router-core/src/router.ts`:
- Around line 1357-1364: The routeBranchCache WeakMap can hold stale branch data
when the route tree is rebuilt; update setRoutes (or the function that fully
reconstructs parentRoute, e.g., processRouteTree/update) to clear or
reinitialize routeBranchCache so getRouteBranch and buildRouteBranch always
compute fresh branches for the new tree; specifically, add a
routeBranchCache.clear() (or assign a new WeakMap to routeBranchCache) at the
start/end of setRoutes/processRouteTree so that cached entries keyed by old
route identity are invalidated when parentRoute relationships are reset.
🪄 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: fdd45c0d-8a93-44ea-b12f-12f14ead179d
📥 Commits
Reviewing files that changed from the base of the PR and between af0a966bf89992c4dbc215ffcf7c7be157821d5e and e0dd96c346886df8a80ee5c2933989ce55ec29a0.
⛔ Files ignored due to path filters (1)
pnpm-lock.yamlis excluded by!**/pnpm-lock.yaml
📒 Files selected for processing (31)
.changeset/clean-routes-match.mddocs/router/api/router/RouteOptionsType.mde2e/react-router/match-params/.gitignoree2e/react-router/match-params/index.htmle2e/react-router/match-params/package.jsone2e/react-router/match-params/playwright.config.tse2e/react-router/match-params/src/main.tsxe2e/react-router/match-params/tests/match-params.spec.tse2e/react-router/match-params/tsconfig.jsone2e/react-router/match-params/vite.config.jspackages/react-router/src/RouterProvider.tsxpackages/react-router/src/link.tsxpackages/react-router/tests/useParams.test.tsxpackages/router-core/src/index.tspackages/router-core/src/new-process-route-tree.tspackages/router-core/src/path.tspackages/router-core/src/route.tspackages/router-core/src/router.tspackages/router-core/src/utils.tspackages/router-core/tests/build-location.test.tspackages/router-core/tests/match-params.test.tspackages/router-core/tests/new-process-route-tree.test.tspackages/router-core/tests/path.test.tspackages/router-core/tests/skip-route-on-parse-error.test.tspackages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsxpackages/solid-router/src/link.tsxpackages/solid-router/tests/useParams.test.tsxpackages/start-plugin-core/src/rsbuild/plugin.tspackages/start-plugin-core/src/start-compiler/handleCreateServerFn.tspackages/vue-router/src/link.tsxpackages/vue-router/tests/useParams.test.tsx
💤 Files with no reviewable changes (1)
- packages/router-core/tests/skip-route-on-parse-error.test.ts
✅ Files skipped from review due to trivial changes (11)
- e2e/react-router/match-params/vite.config.js
- packages/start-plugin-core/src/rsbuild/plugin.ts
- e2e/react-router/match-params/index.html
- packages/start-plugin-core/src/start-compiler/handleCreateServerFn.ts
- e2e/react-router/match-params/package.json
- .changeset/clean-routes-match.md
- packages/router-devtools-core/src/BaseTanStackRouterDevtoolsPanel.tsx
- packages/router-core/src/index.ts
- packages/solid-router/src/link.tsx
- packages/vue-router/src/link.tsx
- packages/router-core/tests/build-location.test.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- packages/react-router/src/RouterProvider.tsx
- packages/router-core/src/path.ts
- e2e/react-router/match-params/tsconfig.json
- packages/vue-router/tests/useParams.test.tsx
- packages/solid-router/tests/useParams.test.tsx
- e2e/react-router/match-params/playwright.config.ts
- docs/router/api/router/RouteOptionsType.md
- packages/react-router/tests/useParams.test.tsx
- packages/router-core/tests/new-process-route-tree.test.ts
- e2e/react-router/match-params/.gitignore
- packages/react-router/src/link.tsx
- e2e/react-router/match-params/src/main.tsx
- e2e/react-router/match-params/tests/match-params.spec.ts
Contributor
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
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.
Summary by CodeRabbit
New Features
Removals
Documentation
Tests