fix(types): enforce intersection of relative to paths when from is a union #5729
Conversation
WalkthroughAdds two new test-only routes (postEditRoute, postPreviewRoute) and updates route trees and many type assertions; introduces type utilities Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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: 0
🧹 Nitpick comments (2)
packages/router-core/src/utils.ts (1)
148-150: Consider adding JSDoc comments to clarify the purpose of these utility types.These types implement a clever pattern to convert unions to intersections (when combined with
UnionToIntersection), but their purpose isn't immediately obvious from the names alone. A brief comment explaining the usage pattern would help maintainability.Example:
/** * Wraps a type T in an object with property 'foo'. * Used with UnObject and UnionToIntersection for union-to-intersection conversion. */ export type ToObject<T> = { foo: T } /** * Unwraps a type from an object with property 'foo'. * Used with ToObject and UnionToIntersection for union-to-intersection conversion. */ export type UnObject<T> = T extends { foo: any } ? T["foo"] : neverpackages/router-core/src/link.ts (1)
236-254: Implementation correctly enforces intersection semantics for union-based navigation.The type-level logic properly converts union-typed
frompaths into intersection-typedtopaths, ensuring relative navigation is valid for all route alternatives.Consider removing the commented-out code on line 255:
- // RelativeToPath<TRouter, TTo, TResolvedPath>While it provides context during review, commented-out code is generally avoided in production to reduce noise.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/react-router/tests/link.test-d.tsx(43 hunks)packages/router-core/src/link.ts(2 hunks)packages/router-core/src/utils.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/react-router/tests/link.test-d.tsxpackages/router-core/src/utils.tspackages/router-core/src/link.ts
packages/{react-router,solid-router}/**
📄 CodeRabbit inference engine (AGENTS.md)
Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Files:
packages/react-router/tests/link.test-d.tsx
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
Files:
packages/router-core/src/utils.tspackages/router-core/src/link.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/react-router/tests/link.test-d.tsx
📚 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/react-router/tests/link.test-d.tsx
📚 Learning: 2025-09-23T17:36:12.598Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to packages/{react-router,solid-router}/** : Implement React and Solid bindings/components only in packages/react-router/ and packages/solid-router/
Applied to files:
packages/react-router/tests/link.test-d.tsx
📚 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/react-router/tests/link.test-d.tsx
📚 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/link.ts
📚 Learning: 2025-09-23T17:36:12.598Z
Learnt from: CR
Repo: TanStack/router PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-23T17:36:12.598Z
Learning: Applies to packages/router-core/** : Keep framework-agnostic core router logic in packages/router-core/
Applied to files:
packages/router-core/src/link.ts
🧬 Code graph analysis (2)
packages/react-router/tests/link.test-d.tsx (1)
packages/react-router/src/link.tsx (1)
Link(567-598)
packages/router-core/src/link.ts (1)
packages/router-core/src/utils.ts (3)
ToObject(149-149)UnObject(148-148)UnionToIntersection(142-146)
🔇 Additional comments (4)
packages/router-core/src/link.ts (1)
26-28: LGTM! Imports are correct for the intersection-based type construction.These utilities are properly used in the new
WrapRelativeToPathForIntersectiontype and the updatedRelativeToParentPathtype.packages/react-router/tests/link.test-d.tsx (3)
43-52: LGTM! New test routes properly expand coverage for the intersection fix.The
postEditRouteandpostPreviewRouteprovide good test cases for validating that relative navigation from a union of routes enforces intersection semantics.
113-117: Route tree structures correctly updated.Both
routeTreeTuplesandrouteTreeObjectsproperly include the new routes with correct parent-child relationships.Also applies to: 134-141
2165-2177: Excellent test case! This directly validates the fix for issue #3011.This test ensures that when navigating with a relative path from a union of routes, only paths valid for ALL union members are allowed. The expected type
'..' | '../..' | '../edit' | undefinedrepresents the intersection of valid navigation targets from both/invoices/$invoiceId/detailsand/posts/$postId/preview.
packages/router-core/src/utils.ts
Outdated
| ? T | ||
| : never | ||
|
|
||
| export type UnObject<T> = T extends { foo: any } ? T["foo"] : never |
There was a problem hiding this comment.
Thanks for the review!
I’ve removed the use of the foo property.
2 participants
fixes: #3011
Summary by CodeRabbit
New Features
Tests
Refactor