fix(router-core): empty wilcard node is matched over sibling layout route #5971
+30
−0
Conversation
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
Contributor
WalkthroughUpdates route matching so when both a Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Request as HTTP Request (path)
participant Router as new-process-route-tree
participant isSpec as isFrameMoreSpecific
Note right of Router `#d1f0d9`: match discovery phase
Request->>Router: find candidate frames (bestMatch, wildcardMatch, others)
alt both bestMatch & wildcardMatch exist
Router->>isSpec: isFrameMoreSpecific(bestMatch, wildcardMatch)
alt bestMatch more specific
isSpec-->>Router: bestMatch
Router->>Router: select bestMatch
else wildcard more specific
isSpec-->>Router: wildcardMatch
Router->>Router: select wildcardMatch
end
else one or none present
Router->>Router: proceed with existing bestMatch checks
end
Router->>Router: continue standard resolution & return chosen match
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit b11d530
☁️ Nx Cloud last updated this comment at |
![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: 0
🧹 Nitpick comments (2)
packages/router-core/src/new-process-route-tree.ts (1)
1021-1025: Specificity resolution betweenbestMatchandwildcardMatchlooks correct but is subtleThe new logic correctly defers to
isFrameMoreSpecificto pick between a concrete match and a wildcard match, and the choice of argument order:if (bestMatch && wildcardMatch) { return isFrameMoreSpecific(wildcardMatch, bestMatch) ? bestMatch : wildcardMatch }means:
- If
bestMatchis strictly more specific thanwildcardMatch, you returnbestMatch.- On ties,
isFrameMoreSpecificreturnsfalse, so the wildcard wins, which matches the new tests (e.g./avs/a/$and index vs wildcard).This aligns with the intended behavior and the pre‑existing comparator semantics.
Because the helper is documented as “prev best, next candidate”, using
wildcardMatchasprevis a bit non‑intuitive on first read. A tiny inline comment here (e.g. “// wildcard wins ties, so treat it as prev”) would make this intent clearer to future readers, but functionally the change looks sound.packages/router-core/tests/new-process-route-tree.test.ts (1)
549-565: New trailing-empty-wildcard tests are well targetedThese tests precisely cover the intended behavior:
/a/$matching both/aand/a/.- With a sibling layout route, the trailing wildcard wins (
/a+/a/$→/a/$).- With a sibling index route, the index correctly wins over the wildcard (
/a/+/a/$→/a/).They effectively lock in the fix for #5969 and protect the index‑vs‑wildcard precedence. If you want even more confidence later, you could add a deeper variant mirroring the original report (e.g.
/alpha/bravo/$charlievs/alpha/bravo/$charlie/$), but the current coverage is already solid.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-core/src/new-process-route-tree.ts(1 hunks)packages/router-core/tests/new-process-route-tree.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety throughout the codebase
Files:
packages/router-core/tests/new-process-route-tree.test.tspackages/router-core/src/new-process-route-tree.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: nlynzaad
Repo: TanStack/router PR: 5284
File: e2e/react-start/basic/server.js:50-0
Timestamp: 2025-09-28T21:41:45.233Z
Learning: In Express v5, catch-all routes must use named wildcards. Use `/*splat` to match everything except root path, or `/{*splat}` (with braces) to match including root path. The old `*` syntax is not allowed and will cause "Missing parameter name" errors. This breaking change requires explicit naming of wildcard parameters.
Learnt from: nlynzaad
Repo: TanStack/router PR: 5182
File: e2e/react-router/basic-file-based/src/routes/non-nested/named/$baz_.bar.tsx:3-5
Timestamp: 2025-09-22T00:56:49.237Z
Learning: In TanStack Router, underscores are intentionally stripped from route segments (e.g., `$baz_` becomes `baz` in generated types) but should be preserved in base path segments. This is the correct behavior as of the fix in PR #5182.
Learnt from: nlynzaad
Repo: TanStack/router PR: 5182
File: e2e/react-router/basic-file-based/tests/non-nested-paths.spec.ts:167-172
Timestamp: 2025-09-22T00:56:53.426Z
Learning: In TanStack Router, underscores are intentionally stripped from route segments during path parsing, but preserved in base path segments. This is the expected behavior implemented in PR #5182.
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/router-core/tests/new-process-route-tree.test.ts
📚 Learning: 2025-09-28T21:41:45.233Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5284
File: e2e/react-start/basic/server.js:50-0
Timestamp: 2025-09-28T21:41:45.233Z
Learning: In Express v5, catch-all routes must use named wildcards. Use `/*splat` to match everything except root path, or `/{*splat}` (with braces) to match including root path. The old `*` syntax is not allowed and will cause "Missing parameter name" errors. This breaking change requires explicit naming of wildcard parameters.
Applied to files:
packages/router-core/tests/new-process-route-tree.test.tspackages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-10-14T18:59:33.990Z
Learnt from: FatahChan
Repo: TanStack/router PR: 5475
File: e2e/react-start/basic-prerendering/src/routes/redirect/$target/via-beforeLoad.tsx:8-0
Timestamp: 2025-10-14T18:59:33.990Z
Learning: In TanStack Router e2e test files, when a route parameter is validated at the route level (e.g., using zod in validateSearch or param validation), switch statements on that parameter do not require a default case, as the validation ensures only expected values will reach the switch.
Applied to files:
packages/router-core/tests/new-process-route-tree.test.ts
📚 Learning: 2025-11-25T00:18:21.258Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T00:18:21.258Z
Learning: Applies to packages/solid-router/**/*.{ts,tsx} : Solid Router components and primitives should use the tanstack/solid-router package
Applied to files:
packages/router-core/tests/new-process-route-tree.test.ts
📚 Learning: 2025-11-25T00:18:21.258Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T00:18:21.258Z
Learning: Applies to **/src/routes/**/*.{ts,tsx} : Use file-based routing in src/routes/ directories or code-based routing with route definitions
Applied to files:
packages/router-core/tests/new-process-route-tree.test.ts
🧬 Code graph analysis (1)
packages/router-core/tests/new-process-route-tree.test.ts (1)
packages/router-core/src/new-process-route-tree.ts (1)
findRouteMatch(612-634)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: Test
- GitHub Check: Preview
- GitHub Check: autofix
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/router-core/tests/new-process-route-tree.test.ts (1)
549-572: Consider asserting splat params for the empty wildcard caseThe new tests nicely pin down the routing priority around
/a/$, layouts, and index routes. To fully lock in the behavior for the empty wildcard specifically, it would help to also assert the splat params in the"basic"case (similar to the existing root wildcard tests), e.g. that the empty tail yields{ '*': '', _splat: '' }.That way future refactors can’t accidentally change the param semantics while still passing on route IDs.
Example diff:
- it('basic', () => { - const tree = makeTree(['/a/$']) - expect(findRouteMatch('/a/', tree)?.route.id).toBe('/a/$') - expect(findRouteMatch('/a', tree)?.route.id).toBe('/a/$') - }) + it('basic', () => { + const tree = makeTree(['/a/$']) + + const withSlash = findRouteMatch('/a/', tree) + expect(withSlash?.route.id).toBe('/a/$') + expect(withSlash?.params).toEqual({ '*': '', _splat: '' }) + + const withoutSlash = findRouteMatch('/a', tree) + expect(withoutSlash?.route.id).toBe('/a/$') + expect(withoutSlash?.params).toEqual({ '*': '', _splat: '' }) + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/tests/new-process-route-tree.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety throughout the codebase
Files:
packages/router-core/tests/new-process-route-tree.test.ts
🧠 Learnings (6)
📓 Common learnings
Learnt from: nlynzaad
Repo: TanStack/router PR: 5284
File: e2e/react-start/basic/server.js:50-0
Timestamp: 2025-09-28T21:41:45.233Z
Learning: In Express v5, catch-all routes must use named wildcards. Use `/*splat` to match everything except root path, or `/{*splat}` (with braces) to match including root path. The old `*` syntax is not allowed and will cause "Missing parameter name" errors. This breaking change requires explicit naming of wildcard parameters.
Learnt from: nlynzaad
Repo: TanStack/router PR: 5182
File: e2e/react-router/basic-file-based/src/routes/non-nested/named/$baz_.bar.tsx:3-5
Timestamp: 2025-09-22T00:56:49.237Z
Learning: In TanStack Router, underscores are intentionally stripped from route segments (e.g., `$baz_` becomes `baz` in generated types) but should be preserved in base path segments. This is the correct behavior as of the fix in PR #5182.
Learnt from: nlynzaad
Repo: TanStack/router PR: 5182
File: e2e/react-router/basic-file-based/tests/non-nested-paths.spec.ts:167-172
Timestamp: 2025-09-22T00:56:53.426Z
Learning: In TanStack Router, underscores are intentionally stripped from route segments during path parsing, but preserved in base path segments. This is the expected behavior implemented in PR #5182.
📚 Learning: 2025-10-08T08:11:47.088Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5402
File: packages/router-generator/tests/generator/no-formatted-route-tree/routeTree.nonnested.snapshot.ts:19-21
Timestamp: 2025-10-08T08:11:47.088Z
Learning: Test snapshot files in the router-generator tests directory (e.g., files matching the pattern `packages/router-generator/tests/generator/**/routeTree*.snapshot.ts` or `routeTree*.snapshot.js`) should not be modified or have issues flagged, as they are fixtures used to verify the generator's output and are intentionally preserved as-is.
Applied to files:
packages/router-core/tests/new-process-route-tree.test.ts
📚 Learning: 2025-10-14T18:59:33.990Z
Learnt from: FatahChan
Repo: TanStack/router PR: 5475
File: e2e/react-start/basic-prerendering/src/routes/redirect/$target/via-beforeLoad.tsx:8-0
Timestamp: 2025-10-14T18:59:33.990Z
Learning: In TanStack Router e2e test files, when a route parameter is validated at the route level (e.g., using zod in validateSearch or param validation), switch statements on that parameter do not require a default case, as the validation ensures only expected values will reach the switch.
Applied to files:
packages/router-core/tests/new-process-route-tree.test.ts
📚 Learning: 2025-09-28T21:41:45.233Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5284
File: e2e/react-start/basic/server.js:50-0
Timestamp: 2025-09-28T21:41:45.233Z
Learning: In Express v5, catch-all routes must use named wildcards. Use `/*splat` to match everything except root path, or `/{*splat}` (with braces) to match including root path. The old `*` syntax is not allowed and will cause "Missing parameter name" errors. This breaking change requires explicit naming of wildcard parameters.
Applied to files:
packages/router-core/tests/new-process-route-tree.test.ts
📚 Learning: 2025-11-25T00:18:21.258Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T00:18:21.258Z
Learning: Applies to packages/solid-router/**/*.{ts,tsx} : Solid Router components and primitives should use the tanstack/solid-router package
Applied to files:
packages/router-core/tests/new-process-route-tree.test.ts
📚 Learning: 2025-11-25T00:18:21.258Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-11-25T00:18:21.258Z
Learning: Applies to **/src/routes/**/*.{ts,tsx} : Use file-based routing in src/routes/ directories or code-based routing with route definitions
Applied to files:
packages/router-core/tests/new-process-route-tree.test.ts
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Test
Sheraff
deleted the
fix-router-core-empty-wildcard-beats-layout-route-match
branch
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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 #5969
Summary by CodeRabbit
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.