Conversation
…o add children routes to an index route
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 16m 37s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 24s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-11-18 23:03:58 UTC
WalkthroughRuntime validation is added (development-only) to BaseRoute construction and child attachment to ensure index routes (non-root routes whose Changes
Sequence Diagram(s)sequenceDiagram
participant Builder as RouterBuilder
participant Base as BaseRoute (init / ctor)
participant AddChildren as RouteAddChildren (attach children)
participant Dev as Developer
Note over Builder,Base: Route creation (dev-only validations applied)
Builder->>Base: construct/init route with optional parent
alt parent exists and is invalid index-parent or missing child relation
Base-->>Dev: throw invariant error (parent-child integrity)
else
Base->>AddChildren: proceed to attach children (if any)
alt route.fullPath ends with '/' and has children
AddChildren-->>Dev: throw invariant error (index route cannot have children)
else
AddChildren->>Builder: children attached (success)
end
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Areas to review:
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/route.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (4)
📓 Common learnings
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.
📚 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/route.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/route.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/route.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: Preview
- GitHub Check: Test
🔇 Additional comments (1)
packages/router-core/src/route.ts (1)
1744-1751: Confirm the implementation is correct and reliable.The verification confirms that detecting index routes via
fullPath.endsWith('/')is a reliable pattern throughout the codebase:
Construction: Index routes consistently have fullPath ending with '/' because they use empty path values that get joined with parent paths via
joinPaths([this.parentRoute.fullPath, path]).Consistency: The pattern is used identically at line 679 in
new-process-route-tree.tsand verified through comprehensive tests across static, dynamic, and optional route types.Validation: Test suite (
new-process-route-tree.test.ts) explicitly validates that index routes (e.g., '/a/', '/$a/', '/{-$a}/') consistently have trailing slashes, while non-index routes do not.The check at lines 1744-1751 correctly prevents adding children to index routes using this reliable detection method.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
packages/router-core/src/route.ts (1)
1678-1691: Use a stable identifier for error messages instead ofthis.id(initialized later).At this point in
init,_idhasn’t been assigned yet, sothis.idisundefinedand both invariant messages will print...on 'undefined'..., which matches the earlier review note. You already haveoptionsin scope here; you can derive a stable identifier fromoptions.idoroptions.pathand reuse it for both messages.Example:
- if (process.env.NODE_ENV !== 'production') { - if (this.parentRoute) { - invariant( - this.parentRoute.isRoot || !this.parentRoute.fullPath.endsWith('/'), - `Parent route with id '${this.parentRoute.id}' returned by getParentRoute on '${this.id}' is an index route and cannot have child routes.`, - ) - } - if (this.parentRoute) { - invariant( - this.parentRoute.children && this.parentRoute.children.includes(this), - `Parent route with id '${this.parentRoute.id}' returned by getParentRoute has no child route with id '${this.id}'. Did you forget to call .addChildren()?`, - ) - } - } + if (process.env.NODE_ENV !== 'production') { + if (this.parentRoute) { + const routeIdForMessage = + options?.id ?? options?.path ?? this.id ?? '(unknown)' + + invariant( + this.parentRoute.isRoot || !this.parentRoute.fullPath.endsWith('/'), + `Parent route with id '${this.parentRoute.id}' returned by getParentRoute on '${routeIdForMessage}' is an index route and cannot have child routes.`, + ) + + invariant( + this.parentRoute.children && this.parentRoute.children.includes(this), + `Parent route with id '${this.parentRoute.id}' returned by getParentRoute has no child route with id '${routeIdForMessage}'. Did you forget to call .addChildren()?`, + ) + } + }This keeps the checks as-is but guarantees a meaningful identifier in the messages even before
_idis set.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/route.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (3)
📓 Common learnings
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.
📚 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/route.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/route.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: Preview
- GitHub Check: Test
| } | ||
| if (this.children) { | ||
| invariant( | ||
| this.isRoot || !this.fullPath.endsWith('/'), |
There was a problem hiding this comment.
is the negation incorrect here?
There was a problem hiding this comment.
Hmmm in invariant the 1st param must be true to not throw, right? If this is correct, then I think the condition is good:
- If there are children
- the route must either be the root
- or not be an index (i.e. not
endsWith('/'))
Just checked the docs https://www.npmjs.com/package/tiny-invariant:
invariant(truthyValue, 'This should not throw!');
invariant(falsyValue, 'This will throw!');
// Error('Invariant violation: This will throw!');This seems correct to me then
2 participants
It seems some theoretically invalid route tree configurations used to work before #5867 but don't work anymore.
This PR proposes that we add some runtime checks (when
NODE_ENV !== 'production') to warn users of such patterns.Those env checks persist in the
distbuild of tanstack/router-core distributed through npmBut get erased at build-time (tested w/ Vite rollup 7.1.12) when the production environment is set (
vite build --mode productionor simplyvite build)Summary by CodeRabbit