refactor(router-core): index routes have index nodes in the segment tree #6171
Conversation
WalkthroughAdds explicit index-segment support and an Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit a8b8021
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/router-core/src/new-process-route-tree.ts (1)
770-774: Potential issue withskippednon-null assertion.When
extractParamsis called with a leaf from the early root index return (line 850:return { node: segmentTree.index }),leaf.skippedwill beundefined. The non-null assertionleaf.skipped!on line 771 could lead to unexpected behavior:if (leaf.skipped! & (1 << nodeIndex)) {In JavaScript,
undefined & numbercoerces to0 & number = 0, so this happens to work correctly (the condition is always false when skipped is undefined). However, this relies on implicit type coercion and would fail TypeScript strict checks if the!were removed.Consider using a nullish coalescing operator for clarity:
🔎 Suggested clarification
- if (leaf.skipped! & (1 << nodeIndex)) { + if ((leaf.skipped ?? 0) & (1 << nodeIndex)) {
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-core/src/new-process-route-tree.ts(13 hunks)packages/router-core/tests/new-process-route-tree.test.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/router-core/tests/new-process-route-tree.test.tspackages/router-core/src/new-process-route-tree.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/tests/new-process-route-tree.test.tspackages/router-core/src/new-process-route-tree.ts
🧠 Learnings (7)
📓 Common learnings
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.
📚 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.tspackages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-12-17T02:17:55.086Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
Applied to files:
packages/router-core/tests/new-process-route-tree.test.tspackages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
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-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Applied to files:
packages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Implement type-safe routing with search params and path params
Applied to files:
packages/router-core/src/new-process-route-tree.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). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (10)
packages/router-core/tests/new-process-route-tree.test.ts (1)
720-724: LGTM!The test route structure change correctly reflects the new index segment tree behavior. The nested route under
/dashboard/properly exercises the fuzzy matching scenario where an index route should not be used as a fuzzy match target.packages/router-core/src/new-process-route-tree.ts (9)
10-24: LGTM!Good design choice keeping
SEGMENT_TYPE_INDEXinternal whileSegmentKindremains the public API for route path segments. TheExtendedSegmentKindtype cleanly separates the tree representation concern from the path parsing concern.
338-358: LGTM!The index node creation logic correctly identifies leaf routes with trailing slashes and creates a dedicated index node with highest priority. The depth tracking and parent/child relationships are properly maintained.
429-477: LGTM!Both
createStaticNodeandcreateDynamicNodeconsistently initialize theindexproperty tonull, ensuring the new node structure is properly set up across all node types.
479-527: LGTM!The type definitions are well-structured:
StaticSegmentNodecorrectly includesSEGMENT_TYPE_INDEXsince index nodes are static by natureSegmentNodeusesExtendedSegmentKindinternally while keeping the public API clean- Priority order is clearly documented in comments
847-850: Clarify the TODO comment regarding correctness.The comment states this is an optimization but then contradicts itself with "TODO: it doesn't actually work correctly without this block". This suggests the early return is actually required for correctness, not just performance.
If this is a known limitation, consider documenting why the algorithm doesn't handle root index correctly without this special case, or refactoring to fix the underlying issue.
887-894: LGTM!Correctly excludes index nodes from fuzzy matching candidates. Index routes represent exact path matches and should not serve as partial match fallbacks for longer paths.
896-917: LGTM!The matching logic correctly handles the new index segment type:
- Only matches index nodes when the path has a trailing slash (
pathIsIndex)- Perfect match optimization now correctly requires
SEGMENT_TYPE_INDEXfor early return- Continues exploration when the node has an
indexchild
1051-1064: LGTM!The index matching logic correctly:
- Only attempts index match when path segments are consumed (
index >= partsLength)- Guards against matching at the root level (
index > 1)- Maintains correct statistics (statics not incremented since index doesn't consume a segment)
1092-1111: LGTM!The
isFrameMoreSpecificfunction correctly prioritizes index nodes in the specificity comparison. The boolean-to-number coercion in(next.node.kind === SEGMENT_TYPE_INDEX) > (prev.node.kind === SEGMENT_TYPE_INDEX)is valid JavaScript/TypeScript behavior (true > false evaluates as 1 > 0).
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/router-core/src/new-process-route-tree.ts(13 hunks)packages/router-plugin/src/core/route-hmr-statement.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/router-plugin/src/core/route-hmr-statement.tspackages/router-core/src/new-process-route-tree.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-plugin/src/core/route-hmr-statement.tspackages/router-core/src/new-process-route-tree.ts
🧠 Learnings (7)
📓 Common learnings
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-12-17T02:17:55.086Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
Applied to files:
packages/router-plugin/src/core/route-hmr-statement.tspackages/router-core/src/new-process-route-tree.ts
📚 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-plugin/src/core/route-hmr-statement.tspackages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
packages/router-plugin/src/core/route-hmr-statement.ts
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
Applied to files:
packages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Applied to files:
packages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Implement type-safe routing with search params and path params
Applied to files:
packages/router-core/src/new-process-route-tree.ts
🧬 Code graph analysis (1)
packages/router-core/src/new-process-route-tree.ts (1)
packages/router-core/src/route.ts (1)
path(1553-1555)
⏰ 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). (2)
- GitHub Check: Preview
- GitHub Check: Test
🔇 Additional comments (9)
packages/router-core/src/new-process-route-tree.ts (8)
10-10: LGTM: Type system updates for index nodes.The introduction of
SEGMENT_TYPE_INDEXandExtendedSegmentKind, along with the newindexfield on segment nodes, properly extends the type system to support explicit index route representation. The documentation clearly indicates the priority ordering.Also applies to: 21-24, 480-480, 498-506
435-435: LGTM: Index field initialization.Both node creation functions correctly initialize the
indexfield tonull, consistent with other child collection fields.Also applies to: 464-464
338-358: Index node creation logic is correct.The implementation properly creates explicit index nodes for leaf routes with trailing slashes. The logic ensures:
- Index nodes are only created for leaf routes (
route.pathor no children) with trailing slashes- The index node becomes a child of the path segment node via
node.index- The route is associated with the index node (line 356 executes after
nodeis reassigned toindexNode)This correctly separates layout nodes (e.g.,
/foo) from their index route nodes (e.g.,/foo/).
724-727: LGTM: Fuzzy match support in findMatch.The optional chaining at line 724 correctly handles the case where
getNodeMatchreturnsnull. The subsequent extraction of the'**'parameter (line 726) supports fuzzy matching where partial paths are captured. The logic is sound since line 724's early return guaranteesleaf.node.routeexists for lines 726-727.
887-894: LGTM: Index nodes correctly excluded from fuzzy matching.Index nodes represent exact terminal routes ending with
/and should not serve as fuzzy match candidates for partial path prefixes. The conditionnode.kind !== SEGMENT_TYPE_INDEXappropriately restricts fuzzy matching to layout and non-index route nodes.
896-916: LGTM: Index-aware exact matching logic.The matching logic correctly handles index routes:
- Line 898 ensures paths ending with
/only match index nodes- Lines 905-913 provide an optimization for perfect index matches (all static segments, no dynamics/optionals)
- Line 916 correctly allows only optional, wildcard, or index children when beyond the path length
The logic maintains correct matching semantics for the new index node structure.
1052-1063: Index node traversal logic is correct (but depends on root index special case).The logic correctly adds index nodes to the traversal stack when:
- The node has an index child
- We're beyond the root segment (
index > 1)- We've consumed all path parts (
index >= partsLength)The
index > 1condition assumes the root index case is handled by the early return at line 850 (already flagged as a concern in a separate comment). If that early return were removed, root index matching would break.
1106-1110: LGTM: Index node priority in match specificity.The comparison correctly prioritizes index nodes over layout nodes when all other matching factors (statics, dynamics, optionals) are equal. This ensures that routes with trailing slashes (
/foo/) are preferred over their layout counterparts (/foo) when both could match.packages/router-plugin/src/core/route-hmr-statement.ts (1)
43-43: LGTM: HMR traversal updated for index nodes.The change correctly replaces the removed
node.notFoundhandling with recursion vianode.index. This is consistent with how other child collections (static, dynamic, etc.) are traversed in the same function. The route ID match check at line 42 handles updating the route reference when found in the tree.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/router-core/src/new-process-route-tree.ts (1)
726-726: Consider using explicit undefined check instead ofinoperator.The
inoperator on line 726 works correctly for checking if the'**'property exists, but it's an unusual pattern for optional properties in this codebase. An explicit check would be more conventional:- if ('**' in leaf) params['**'] = leaf['**']! + if (leaf['**'] !== undefined) params['**'] = leaf['**']This also avoids the non-null assertion.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/new-process-route-tree.ts(13 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/router-core/src/new-process-route-tree.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/src/new-process-route-tree.ts
🧠 Learnings (5)
📓 Common learnings
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/src/new-process-route-tree.ts
📚 Learning: 2025-12-17T02:17:55.086Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
Applied to files:
packages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
packages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Implement type-safe routing with search params and path params
Applied to files:
packages/router-core/src/new-process-route-tree.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). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (8)
packages/router-core/src/new-process-route-tree.ts (8)
10-10: LGTM: Index segment type is appropriately scoped as internal.The
SEGMENT_TYPE_INDEXconstant is intentionally not exported (unlike the otherSEGMENT_TYPE_*constants on lines 6-9), which correctly reflects that index nodes are an internal implementation detail of the segment tree. TheExtendedSegmentKindtype properly extendsSegmentKindfor internal use.Also applies to: 21-24
339-358: LGTM: Index node creation logic correctly handles route assignment.The flow properly handles index routes:
- Creates an index child node when the path ends with
/(line 342-352)- Reassigns
nodeto point to the newly created index node (line 351)- Assigns the route to the current
node, which is now the index node if one was created (lines 355-358)This ensures index routes are stored on dedicated index nodes rather than shared nodes with layout routes.
435-435: LGTM: Type definitions and initializations are consistent.The
indexfield is properly:
- Initialized to
nullin bothcreateStaticNodeandcreateDynamicNode- Typed as
StaticSegmentNode<T> | null, correctly restricting index nodes to static segments- Declared with
ExtendedSegmentKindfor the parent node'skindfieldThe priority comment on line 500 correctly identifies index segments as highest priority.
Also applies to: 464-464, 480-480, 498-501
886-893: LGTM: Index routes correctly excluded from fuzzy matching.Index routes require exact path matches (with trailing slash), so excluding them from fuzzy matching on line 889 is correct. Fuzzy matching is for finding partial matches, which doesn't apply to index routes.
897-897: LGTM: Index matching conditions are correct.Line 897 correctly requires
node.kind === SEGMENT_TYPE_INDEXwhen the path has a trailing slash (pathIsIndex), ensuring only index nodes match paths like/foo/.The perfect match optimization (lines 904-912) provides an early return when all parts are static, no dynamics/optionals are involved, and the node is an index—this is a valid optimization for the most common exact-match case.
Also applies to: 904-912
915-915: LGTM: Beyond-path logic correctly handles index nodes.When beyond the path parts, the algorithm now correctly continues exploring
node.index,node.optional, andnode.wildcardbranches. This allows index nodes to match when the path has been fully consumed.
1051-1062: LGTM: Index node traversal correctly handles path termination.This new traversal branch correctly adds index child nodes to the stack when:
- An index node exists (
node.index)- All path segments have been consumed (
index >= partsLength)The frame counters are appropriately updated: only
depthis incremented whileindex,statics,dynamics, andoptionalsremain unchanged, because index nodes don't consume path parts and represent a distinct matching category.
1105-1109: LGTM: Frame comparison correctly prioritizes index nodes.The updated comparison logic correctly prioritizes index nodes over non-index nodes when all other factors (statics, dynamics, optionals) are equal. The boolean comparison
(next.node.kind === SEGMENT_TYPE_INDEX) > (prev.node.kind === SEGMENT_TYPE_INDEX)evaluates totrueonly whennextis an index node andprevis not, which is the intended behavior.When both frames have the same index status, depth provides the tiebreaker (line 1109).
| }, | ||
| { | ||
| id: '/dashboard', | ||
| id: '/dashboard/invoices', |
There was a problem hiding this comment.
This has nothing to do w/ the rest of this PR, it was just incorrect before so i did a drive-by fix
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/router-core/src/new-process-route-tree.ts (1)
847-850: Update misleading comment about root index optimization.The comment claims "this is an optimization, algorithm should work correctly without this block," but based on previous review feedback, this is actually a necessary correctness fix, not an optional optimization.
The general index matching logic at lines 1053-1062 may not handle the root index case correctly, making this early return essential for proper routing behavior.
🔎 Suggested comment update
- // quick check for root index - // this is an optimization, algorithm should work correctly without this block + // Fast path for root index route + // This is required for correctness (general algorithm doesn't handle root index properly) if (path === '/' && segmentTree.index) return { node: segmentTree.index, skipped: 0 }
🧹 Nitpick comments (1)
packages/router-core/src/new-process-route-tree.ts (1)
726-727: Type safety: Wildcard parameter access relies on runtime check without proper typing.The code uses
'**' in leafto guardleaf['**']!, butleaf's type signature (line 737) doesn't include the optional'**': stringproperty that fuzzy matching adds (lines 1082-1086). TypeScript cannot verify the non-null assertion is safe.🔎 Suggested type improvement
Update the return type of
getNodeMatchand theleafparameter type inextractParamsto reflect the optional wildcard property:// In getNodeMatch return type (around line 1082-1086) return { node: bestFuzzy.node, skipped: bestFuzzy.skipped, '**': decodeURIComponent(splat), } as { node: AnySegmentNode<T>; skipped: number; '**'?: string } // Update extractParams signature (line 737) function extractParams<T extends RouteLike>( path: string, parts: Array<string>, leaf: { node: AnySegmentNode<T>; skipped: number; '**'?: string }, ) {Then replace the non-null assertion with a proper check:
- if ('**' in leaf) params['**'] = leaf['**']! + if (leaf['**'] !== undefined) params['**'] = leaf['**']
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/new-process-route-tree.ts(11 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/router-core/src/new-process-route-tree.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/src/new-process-route-tree.ts
🧠 Learnings (6)
📓 Common learnings
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.
📚 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/src/new-process-route-tree.ts
📚 Learning: 2025-12-17T02:17:55.086Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
Applied to files:
packages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
Applied to files:
packages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
packages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Implement type-safe routing with search params and path params
Applied to files:
packages/router-core/src/new-process-route-tree.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). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (3)
packages/router-core/src/new-process-route-tree.ts (3)
339-358: Well-structured index node creation.The logic correctly:
- Identifies leaf routes ending with
/- Creates a dedicated index child node
- Maintains proper parent-child relationships and depth tracking
- Makes the index node matchable for terminal matches
This implementation cleanly achieves the PR's goal of separating index routes into dedicated nodes in the segment tree.
1051-1063: Index traversal logic is sound, but relies on buggy terminal filter.The index node traversal correctly:
- Triggers only when at or beyond path length (
index >= partsLength)- Maintains proper frame state (doesn't inflate
staticscount)- Enables index routes to participate in matching
However, this depends on the terminal node filter at line 898 to prevent index nodes from matching non-trailing-slash paths, which currently has a bug (see previous comment).
1106-1110: Specificity comparison correctly prioritizes index nodes.The updated comparison properly places index nodes higher in the priority hierarchy (after statics, dynamics, and optionals). The boolean comparison
(kind === SEGMENT_TYPE_INDEX) > ...correctly evaluates to prefer index-typed nodes.This logic is sound in principle, but it relies on the terminal filter at line 898 to ensure index and non-index nodes don't compete inappropriately. Once that filter is fixed, this comparison will work correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
packages/router-core/src/new-process-route-tree.ts (1)
771-771: Unsafe non-null assertion on optional field.The non-null assertion
leaf.skipped!is applied to an optional field. Whileundefined & numberevaluates to falsyNaN, it's semantically incorrect.🔎 Proposed fix
- if (leaf.skipped & (1 << nodeIndex)) { + if ((leaf.skipped ?? 0) & (1 << nodeIndex)) {
🧹 Nitpick comments (2)
packages/router-core/src/new-process-route-tree.ts (2)
726-727: Type safety: Return type doesn't include optional**property.The code checks for a
**property onleaf(line 726) and extracts it (line 727), but the functiongetNodeMatchreturns{ node: AnySegmentNode<T>; skipped: number }which doesn't declare the optional**field. Line 1085 shows that fuzzy matches return this additional property.Per coding guidelines requiring TypeScript strict mode, the return type should be a union:
{ node: AnySegmentNode<T>; skipped: number } | { node: AnySegmentNode<T>; skipped: number; '**': string }Or use an intersection with an optional property:
{ node: AnySegmentNode<T>; skipped: number; '**'?: string }🔎 Suggested type-safe approach
Update the return type of
getNodeMatcharound line 841:function getNodeMatch<T extends RouteLike>( path: string, parts: Array<string>, segmentTree: AnySegmentNode<T>, fuzzy: boolean, -) { +): { node: AnySegmentNode<T>; skipped: number; '**'?: string } | null {This makes the optional
**property explicit in the type system.
1106-1110: Index node specificity correctly prioritized.The specificity comparison correctly prioritizes index nodes over non-index nodes when all other metrics (statics, dynamics, optionals) are equal. The boolean-to-number coercion via
>is a valid and common pattern for this type of comparison.The specificity order is now:
statics > dynamics > optionals > index > depth, which aligns with the PR objectives.Optional: Improve readability with explicit number coercion
While the boolean comparison works correctly, explicit coercion might improve readability:
- ((next.node.kind === SEGMENT_TYPE_INDEX) > - (prev.node.kind === SEGMENT_TYPE_INDEX) || + (Number(next.node.kind === SEGMENT_TYPE_INDEX) > + Number(prev.node.kind === SEGMENT_TYPE_INDEX) ||However, this is purely stylistic and the current approach is a well-established pattern.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/new-process-route-tree.ts(11 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/router-core/src/new-process-route-tree.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/src/new-process-route-tree.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: Sheraff
Repo: TanStack/router PR: 6171
File: packages/router-core/src/new-process-route-tree.ts:898-898
Timestamp: 2025-12-21T12:52:27.078Z
Learning: In `packages/router-core/src/new-process-route-tree.ts`, the matching logic intentionally allows paths without trailing slashes to match index routes with trailing slashes (e.g., `/a` can match `/a/` route), but not vice-versa (e.g., `/a/` cannot match `/a` layout route). This is implemented via the condition `!pathIsIndex || node.kind === SEGMENT_TYPE_INDEX` and is a deliberate design decision to provide better UX by being permissive with missing trailing slashes.
📚 Learning: 2025-12-21T12:52:27.078Z
Learnt from: Sheraff
Repo: TanStack/router PR: 6171
File: packages/router-core/src/new-process-route-tree.ts:898-898
Timestamp: 2025-12-21T12:52:27.078Z
Learning: In `packages/router-core/src/new-process-route-tree.ts`, the matching logic intentionally allows paths without trailing slashes to match index routes with trailing slashes (e.g., `/a` can match `/a/` route), but not vice-versa (e.g., `/a/` cannot match `/a` layout route). This is implemented via the condition `!pathIsIndex || node.kind === SEGMENT_TYPE_INDEX` and is a deliberate design decision to provide better UX by being permissive with missing trailing slashes.
Applied to files:
packages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-12-17T02:17:55.086Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
Applied to files:
packages/router-core/src/new-process-route-tree.ts
📚 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/src/new-process-route-tree.ts
📚 Learning: 2025-09-22T00:56:49.237Z
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.
Applied to files:
packages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
packages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Applied to files:
packages/router-core/src/new-process-route-tree.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/src/new-process-route-tree.ts
📚 Learning: 2025-09-22T00:56:53.426Z
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.
Applied to files:
packages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
Applied to files:
packages/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/src/new-process-route-tree.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Implement type-safe routing with search params and path params
Applied to files:
packages/router-core/src/new-process-route-tree.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). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (8)
packages/router-core/src/new-process-route-tree.ts (8)
10-24: LGTM: Clean type extension for index segments.The introduction of
SEGMENT_TYPE_INDEXandExtendedSegmentKindproperly extends the existing segment type system with clear documentation.
339-358: LGTM: Index node creation logic is sound.The logic correctly creates index nodes for leaf routes ending with
/, properly maintains the parent-child relationship, advances depth, and makes the node matchable. The conditionisLeaf && path.endsWith('/')accurately identifies when an index node should be created.
435-435: LGTM: Factory functions updated consistently.Both
createStaticNodeandcreateDynamicNodecorrectly initialize the newindexfield tonull.Also applies to: 464-464
480-480: LGTM: Type definitions correctly reflect index node semantics.The type updates properly extend
StaticSegmentNodeto includeSEGMENT_TYPE_INDEX, add theindexfield toSegmentNode, and update the kind toExtendedSegmentKind. The constraint thatindexmust be aStaticSegmentNode<T>is correct since index routes are static pathname segments with trailing slashes.Also applies to: 498-527
847-850: LGTM: Valid optimization for root index route.This fast path for root index routes (
/) is a valid optimization. The general algorithm would handle this case via the index traversal logic at lines 1052-1063, but this early return avoids unnecessary stack operations.
887-894: LGTM: Fuzzy matching correctly excludes index nodes.The condition
node.kind !== SEGMENT_TYPE_INDEXensures that fuzzy matches (partial path matches) don't resolve to index routes, which makes sense because index routes represent exact trailing-slash endpoints. Fuzzy matching should match layout/parent routes instead.
896-917: LGTM: Beyond-path matching implements intended index semantics.The matching logic at line 898 correctly implements the design decision documented in learnings: paths without trailing slashes can match index routes (e.g.,
/amatches/a/), but not vice-versa. The optimization at lines 905-911 efficiently short-circuits when a perfect index match is found. Line 916 appropriately allows continued traversal only for index, optional, and wildcard nodes when beyond the path.Based on learnings: This permissive behavior provides better UX.
1052-1063: LGTM: Index node traversal correctly integrated.The traversal logic for index nodes is correctly implemented:
- Only traverses to index when
isBeyondPath(as index represents trailing/)- Properly increments
depth(index is a child node)- Correctly maintains
statics,dynamics, andoptionalscounts (index specificity is handled separately at lines 1106-1110)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/router-core/src/new-process-route-tree.ts (1)
717-732: Consider improving type safety for fuzzy match return value.The code safely handles the
'**'property that can be added during fuzzy matching (line 1085), using anincheck before access. However, the type system doesn't capture this optional property on theleafobject.This is existing behavior and works correctly at runtime, but you could optionally improve type safety by defining a union type that includes the fuzzy match variant.
💡 Optional type safety improvement
You could define a more precise return type for
getNodeMatchthat includes the optional'**'property for fuzzy matches:type NodeMatchResult<T> = | { node: AnySegmentNode<T>; skipped: number } | { node: AnySegmentNode<T>; skipped: number; '**': string }This would make the type system aware of the fuzzy match variant.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/new-process-route-tree.ts(11 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript strict mode with extensive type safety for all code
Files:
packages/router-core/src/new-process-route-tree.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/src/new-process-route-tree.ts
🧠 Learnings (12)
📓 Common learnings
Learnt from: Sheraff
Repo: TanStack/router PR: 6171
File: packages/router-core/src/new-process-route-tree.ts:898-898
Timestamp: 2025-12-21T12:52:27.078Z
Learning: In `packages/router-core/src/new-process-route-tree.ts`, the matching logic intentionally allows paths without trailing slashes to match index routes with trailing slashes (e.g., `/a` can match `/a/` route), but not vice-versa (e.g., `/a/` cannot match `/a` layout route). This is implemented via the condition `!pathIsIndex || node.kind === SEGMENT_TYPE_INDEX` and is a deliberate design decision to provide better UX by being permissive with missing trailing slashes.
📚 Learning: 2025-12-21T12:52:27.078Z
Learnt from: Sheraff
Repo: TanStack/router PR: 6171
File: packages/router-core/src/new-process-route-tree.ts:898-898
Timestamp: 2025-12-21T12:52:27.078Z
Learning: In `packages/router-core/src/new-process-route-tree.ts`, the matching logic intentionally allows paths without trailing slashes to match index routes with trailing slashes (e.g., `/a` can match `/a/` route), but not vice-versa (e.g., `/a/` cannot match `/a` layout route). This is implemented via the condition `!pathIsIndex || node.kind === SEGMENT_TYPE_INDEX` and is a deliberate design decision to provide better UX by being permissive with missing trailing slashes.
Applied to files:
packages/router-core/src/new-process-route-tree.ts
📚 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/src/new-process-route-tree.ts
📚 Learning: 2025-12-17T02:17:55.086Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:55.086Z
Learning: In `packages/router-generator/src/generator.ts`, pathless_layout routes must receive a `path` property when they have a `cleanedPath`, even though they are non-path routes. This is necessary because child routes inherit the path from their parent, and without this property, child routes would not have the correct full path at runtime.
Applied to files:
packages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-10-01T18:31:35.420Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.420Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.
Applied to files:
packages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-10-01T18:30:26.591Z
Learnt from: schiller-manuel
Repo: TanStack/router PR: 5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.591Z
Learning: In `packages/router-core/src/router.ts`, the `resolveRedirect` method intentionally strips the router's origin from redirect URLs when they match (e.g., `https://foo.com/bar` → `/bar` for same-origin redirects) while preserving the full URL for cross-origin redirects. This logic should not be removed or simplified to use `location.publicHref` directly.
Applied to files:
packages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-09-22T00:56:49.237Z
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.
Applied to files:
packages/router-core/src/new-process-route-tree.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Applied to files:
packages/router-core/src/new-process-route-tree.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/src/new-process-route-tree.ts
📚 Learning: 2025-09-22T00:56:53.426Z
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.
Applied to files:
packages/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/src/new-process-route-tree.ts
📚 Learning: 2025-12-06T15:03:07.223Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-12-06T15:03:07.223Z
Learning: Implement type-safe routing with search params and path params
Applied to files:
packages/router-core/src/new-process-route-tree.ts
🧬 Code graph analysis (1)
packages/router-core/src/new-process-route-tree.ts (1)
packages/router-core/src/route.ts (1)
path(1553-1555)
⏰ 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). (2)
- GitHub Check: Test
- GitHub Check: Preview
🔇 Additional comments (11)
packages/router-core/src/new-process-route-tree.ts (11)
10-24: LGTM: Index segment type introduced cleanly.The internal
SEGMENT_TYPE_INDEXconstant andExtendedSegmentKindtype extension follow the existing pattern for segment types. The comments clearly distinguish route path segments from segment tree segments.
339-358: LGTM: Index node creation logic is correct.The logic correctly creates index nodes for leaf routes with trailing slashes. The
isLeafcondition properly identifies routes that define matchable paths (even if they have children), and the index node is appropriately configured with incremented depth and proper parent linkage.
429-445: LGTM: Static node initialization includes index property.The
index: nullinitialization is consistent with other optional child properties and aligns with the updatedSegmentNodetype.
451-477: LGTM: Dynamic node initialization includes index property.The
index: nullinitialization follows the same pattern as the static node and maintains consistency with the type definition.
479-527: LGTM: Type definitions properly updated for index support.The type definitions correctly extend
SegmentNodeto include theindexproperty and updatekindto useExtendedSegmentKind. The comment clearly indicates that index segments have the highest priority, which aligns with the matching logic.
847-850: LGTM: Root index fast path is a valid optimization.The fast path for root index matching (
path === '/') is correctly identified as an optimization. The general index matching logic at lines 1052-1063 would handle this case, but the fast path avoids unnecessary stack operations for this common scenario.
886-894: LGTM: Fuzzy matching correctly excludes index nodes.The fuzzy matching logic appropriately excludes index nodes (
node.kind !== SEGMENT_TYPE_INDEX) when tracking the best partial match. This ensures that partial path matches prefer layout routes over index routes, which is the correct behavior for navigating to parent routes.
896-914: LGTM: Perfect match optimization is correctly implemented.The early return for perfect index matches (all static segments, no dynamics/optionals/wildcards) is a valid optimization that avoids unnecessary traversal when we've found an exact match. The comment accurately describes this as an optimization rather than a correctness requirement.
Based on learnings: The matching condition at line 898 intentionally allows paths without trailing slashes to match index routes, providing better UX.
915-917: LGTM: Beyond-path matching logic is correct.The comment and logic correctly identify that only index segments, optional segments, and wildcard segments can match when the path has been fully consumed. This aligns with the routing semantics where these segment types can match at or beyond the path boundary.
1051-1064: LGTM: General index matching logic is sound.The index matching logic correctly handles the case where the path has been fully consumed and an index child exists. The stack frame properly increments depth while maintaining the path index, reflecting that index nodes are structural children in the tree without consuming additional path segments.
1092-1112: LGTM: Specificity comparison correctly prioritizes index nodes.The specificity comparison properly includes index node preference as a tiebreaker. The expression
(next.node.kind === SEGMENT_TYPE_INDEX) > (prev.node.kind === SEGMENT_TYPE_INDEX)compares boolean values as numbers (true=1, false=0), effectively preferring index routes over layout routes when other factors are equal. This aligns with the design where index routes represent more specific trailing-slash matches.
Context
For future improvements (namely #5936) we need the
segmentTreenodes to be able to carry route-specific metadata that can be accessed during the path matching algorithm.Currently, both a layout route and an index route can be carried by the same segment tree node. For example
would both be on the
'foo'static node, withnode.routerepresenting/foo/node.notFoundrepresenting/fooIt was done this way because there is no possibility for a layout route (here
/foo) to be matched if there is also an index route (here/foo/) at the same level. It could only be used as a fuzzy match (i.e. not found).But now, we need more information than just the path for the matching algorithm (e.g. in #5936 we also need
params.parseandskipRouteOnParseError) and this additional metadata cannot be represented on a single node.Solution
This proposes that we introduce a new type of node to the
segmentTree: index nodes. These supersede thenode.notFoundandnode.isIndexattributes, and are now represented as regular children nodes.Performance
This change means that we have a little bit more work to do to reach an index match (or to eliminate one from the candidate pool). However, it's negligible, and not actually measurable when benchmarking:
1.01x is within the margin of error, and re-running the benchmark we get this number in either direction (old > new or new > old).
If we remove the cache, we can observe a slight performance degradation. This one is significant, but small enough.
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.