-
-
Notifications
You must be signed in to change notification settings - Fork 1.4k
fix: don't create virtual routes for pathless layouts #6120
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
WalkthroughThis PR changes router generation to add a path→RouteNode map ( Changes
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 8d9efba
☁️ Nx Cloud last updated this comment at |
1cab73e to
8d9efba
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 1
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
e2e/react-router/basic-file-based/src/routeTree.gen.ts(32 hunks)e2e/react-router/basic-file-based/src/routes/__root.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/child.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/index.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/route.tsx(1 hunks)e2e/react-router/basic-file-based/src/routes/pathless-layout/route.tsx(1 hunks)e2e/react-router/basic-file-based/tests/app.spec.ts(1 hunks)e2e/react-router/basic-react-query-file-based/src/routeTree.gen.ts(1 hunks)e2e/react-start/basic/src/routeTree.gen.ts(6 hunks)packages/router-generator/src/generator.ts(5 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:
e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/route.tsxe2e/react-router/basic-file-based/src/routes/pathless-layout/route.tsxe2e/react-router/basic-react-query-file-based/src/routeTree.gen.tspackages/router-generator/src/generator.tse2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/child.tsxe2e/react-router/basic-file-based/src/routes/__root.tsxe2e/react-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/index.tsxe2e/react-router/basic-file-based/tests/app.spec.tse2e/react-start/basic/src/routeTree.gen.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/route.tsxe2e/react-router/basic-file-based/src/routes/pathless-layout/route.tsxe2e/react-router/basic-react-query-file-based/src/routeTree.gen.tspackages/router-generator/src/generator.tse2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/child.tsxe2e/react-router/basic-file-based/src/routes/__root.tsxe2e/react-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/index.tsxe2e/react-router/basic-file-based/tests/app.spec.tse2e/react-start/basic/src/routeTree.gen.ts
🧠 Learnings (11)
📓 Common learnings
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.
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.
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:
e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/route.tsxe2e/react-router/basic-file-based/src/routes/pathless-layout/route.tsxe2e/react-router/basic-react-query-file-based/src/routeTree.gen.tspackages/router-generator/src/generator.tse2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/child.tsxe2e/react-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/index.tsxe2e/react-router/basic-file-based/tests/app.spec.tse2e/react-start/basic/src/routeTree.gen.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:
e2e/react-router/basic-file-based/src/routes/pathless-layout/route.tsxe2e/react-router/basic-react-query-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/child.tsxe2e/react-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/index.tsxe2e/react-start/basic/src/routeTree.gen.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:
e2e/react-router/basic-file-based/src/routes/pathless-layout/route.tsxe2e/react-router/basic-react-query-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/child.tsxe2e/react-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/index.tsxe2e/react-router/basic-file-based/tests/app.spec.tse2e/react-start/basic/src/routeTree.gen.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: Use file-based routing in `src/routes/` directories or code-based routing with route definitions
Applied to files:
e2e/react-router/basic-file-based/src/routes/pathless-layout/route.tsxe2e/react-router/basic-react-query-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routeTree.gen.tse2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/index.tsxe2e/react-start/basic/src/routeTree.gen.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:
e2e/react-router/basic-file-based/src/routeTree.gen.tse2e/react-start/basic/src/routeTree.gen.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:
e2e/react-router/basic-file-based/src/routeTree.gen.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:
e2e/react-router/basic-file-based/src/routeTree.gen.ts
📚 Learning: 2025-10-09T12:59:02.129Z
Learnt from: hokkyss
Repo: TanStack/router PR: 5418
File: e2e/react-start/custom-identifier-prefix/src/styles/app.css:19-21
Timestamp: 2025-10-09T12:59:02.129Z
Learning: In e2e test directories (paths containing `e2e/`), accessibility concerns like outline suppression patterns are less critical since the code is for testing purposes, not production use.
Applied to files:
e2e/react-router/basic-file-based/tests/app.spec.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:
e2e/react-start/basic/src/routeTree.gen.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:
e2e/react-start/basic/src/routeTree.gen.ts
🧬 Code graph analysis (6)
e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/route.tsx (4)
e2e/react-router/basic-file-based/src/routes/__root.tsx (1)
Route(12-22)e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/child.tsx (1)
Route(3-7)e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/index.tsx (1)
Route(3-7)e2e/react-router/basic-file-based/src/routes/pathless-layout/route.tsx (1)
Route(3-15)
e2e/react-router/basic-file-based/src/routes/pathless-layout/route.tsx (4)
e2e/react-router/basic-file-based/src/routes/__root.tsx (1)
Route(12-22)e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/child.tsx (1)
Route(3-7)e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/index.tsx (1)
Route(3-7)e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/route.tsx (1)
Route(3-15)
packages/router-generator/src/generator.ts (1)
packages/router-generator/src/utils.ts (5)
hasParentRoute(364-444)removeLastSegmentFromPath(358-362)removeGroups(320-322)removeUnderscores(192-194)removeLayoutSegments(332-336)
e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/child.tsx (4)
e2e/react-router/basic-file-based/src/routes/__root.tsx (1)
Route(12-22)e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/index.tsx (1)
Route(3-7)e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/route.tsx (1)
Route(3-15)e2e/react-router/basic-file-based/src/routes/pathless-layout/route.tsx (1)
Route(3-15)
e2e/react-router/basic-file-based/src/routes/__root.tsx (1)
packages/react-router/src/link.tsx (1)
Link(569-600)
e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/index.tsx (4)
e2e/react-router/basic-file-based/src/routes/__root.tsx (1)
Route(12-22)e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/child.tsx (1)
Route(3-7)e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/route.tsx (1)
Route(3-15)e2e/react-router/basic-file-based/src/routes/pathless-layout/route.tsx (1)
Route(3-15)
⏰ 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 (14)
e2e/react-router/basic-react-query-file-based/src/routeTree.gen.ts (1)
1-220: Skipping review of autogenerated file.This
routeTree.gen.tsfile is automatically generated by TanStack Router and should not be manually modified. The file header confirms this with directives like@ts-nocheckandeslint-disable. Based on learnings, autogenerated route tree files in this repository are excluded from review.e2e/react-start/basic/src/routeTree.gen.ts (1)
1-999: Skipping review of autogenerated file.This file is automatically generated by TanStack Router and should not be manually modified. The changes reflect the fix for pathless layout routes (issue #3843) being correctly applied by the router generator.
Based on learnings,
routeTree.gen.tsfiles in TanStack Router repositories are excluded from review.e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/index.tsx (1)
1-7: LGTM!The index route is correctly defined under the pathless layout with proper test ID for e2e validation.
e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/route.tsx (1)
1-15: LGTM!The pathless layout route is well-structured with proper navigation, Outlet for nested content, and test IDs for e2e validation.
e2e/react-router/basic-file-based/src/routes/__root.tsx (1)
158-167: LGTM!The new navigation link follows the established pattern and includes proper test ID for e2e validation.
e2e/react-router/basic-file-based/tests/app.spec.ts (1)
329-395: Comprehensive test coverage for the pathless layout fix.The tests effectively validate the core scenarios for issue #3843:
- Direct and client-side navigation render layout sections correctly.
- Layout preservation during child navigation.
- Not-found behavior for non-existent routes under the pathless layout (the key fix).
e2e/react-router/basic-file-based/src/routes/pathless-layout/_layout/child.tsx (1)
1-7: LGTM!The child route is correctly nested under the pathless layout with proper test ID.
e2e/react-router/basic-file-based/src/routes/pathless-layout/route.tsx (1)
1-15: Well-structured parent route with proper not-found handling.The
notFoundComponentis essential for the fix - it ensures non-existent routes under/pathless-layoutdisplay the not-found message instead of empty HTML, directly addressing issue #3843.e2e/react-router/basic-file-based/src/routeTree.gen.ts (1)
1-10: Skipping review of autogenerated file.This file is automatically generated by TanStack Router and should not be manually modified. Based on learnings.
packages/router-generator/src/generator.ts (5)
411-411: LGTM: Performance improvement.Adding
routeNodesByPathMap improves route node lookups from O(n) to O(1).
411-417: LGTM: Improved flexibility and performance.The dynamic
pieceKeyapproach and Map-basedanchorRoutelookup are solid improvements. The type assertion on line 414 assumesnode._fsRouteTypematches the expected key type for route pieces.
1461-1472: LGTM: Correct map population logic.The guard
!node.isVirtualon line 1470 correctly prevents virtual routes from being added torouteNodesByPath, ensuring only real route nodes are available for path-based lookups.
357-364: LGTM: Simplified parent resolution.Delegating parent route resolution to the
hasParentRouteutility simplifies the code and centralizes the parent-finding logic, which handles various edge cases including non-nested routes and nesting exclusions.
434-459: The special handling for pathless layouts with acleanedPathintroduces unusual path assignments that require verification:
Line 450:
node.path = node.routePathassigns the raw route path with underscores. Verify whether pathless layouts should use the unprocessed route path for identification.Lines 451-453:
node.cleanedPathis computed fromimmediateParentPathrather than the node's own path, potentially causing pathless layouts to inherit their parent's cleaned path instead of having their own. Confirm this is correct for pathless layout routing semantics.Line 448:
candidate.path !== '/'prevents root routes from being selected as parents. Verify whether top-level pathless layouts should be able to use root as their parent.
| !node.isNonPath || | ||
| (node._fsRouteType === 'pathless_layout' && node.cleanedPath) | ||
| ? `path: '${node.cleanedPath}'` | ||
| : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🧩 Analysis chain
🏁 Script executed:
cd /tmp && git clone https://github.com/TanStack/router.git --depth 1 2>/dev/null && cd router && find . -name "generator.ts" -path "*/router-generator/*"Repository: TanStack/router
Length of output: 103
🏁 Script executed:
find . -name "generator.ts" 2>/dev/null | head -20Repository: TanStack/router
Length of output: 103
🏁 Script executed:
cat packages/router-generator/src/generator.ts | sed -n '440,470p; 640,670p'Repository: TanStack/router
Length of output: 2251
Remove pathless layouts from path property generation.
The condition at lines 654-657 incorrectly includes path: '${node.cleanedPath}' for pathless_layout routes. This causes pathless layouts to be treated as routes with a path property, resulting in empty HTML pages instead of proper 404 responses (Issue #3843).
The current logic:
!node.isNonPath || (node._fsRouteType === 'pathless_layout' && node.cleanedPath)
? `path: '${node.cleanedPath}'`
: undefined,
should exclude pathless layouts entirely, ensuring they never receive a path property. Pathless layout routes do not require a matching path in the URL by design.
🤖 Prompt for AI Agents
packages/router-generator/src/generator.ts around lines 654 to 657: the current
ternary erroneously assigns a path for pathless_layout nodes; change the
condition so the path string is only generated for nodes that are not non-path
and are NOT pathless_layouts (i.e., require both that node.isNonPath is false
and node._fsRouteType !== 'pathless_layout'); update the ternary accordingly so
pathless_layout routes are excluded and never receive a path property.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
wrong, they need to get the path here so that child routes, the ones that are matched atruntime, have the correct and full path by inheriting its parent path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@schiller-manuel, thank you for the clarification! You're absolutely right—pathless layouts need the path property so their child routes can inherit and construct the correct full path at runtime. I appreciate you explaining the inheritance mechanism.
✏️ Learnings added
Learnt from: schiller-manuel
Repo: TanStack/router PR: 6120
File: packages/router-generator/src/generator.ts:654-657
Timestamp: 2025-12-17T02:17:47.423Z
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.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
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.
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
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.
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.
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.
fixes #3843
Summary by CodeRabbit
New Features
Improvements
Breaking / Public API Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.