fix(router-core): enhance basePath handling #6157
Closed
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Contributor
WalkthroughAdds a file-based "/redirect" route that performs a beforeLoad redirect to "/about", updates route tree and UI to trigger it, adds an e2e test asserting redirects respect the basepath, and refactors router-core rewrite/buildLocation logic to centralize basepath handling and unify rewritten hrefs. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
⏰ 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)
Comment |
|
View your CI Pipeline Execution ↗ for commit 9ea0f93
☁️ Nx Cloud last updated this comment at |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/router-core/src/rewrite.ts (1)
4-5: Duplicate comment lines.Lines 4-5, 23-24, 69-70, and 86-87 contain duplicated JSDoc comments. This appears to be an unintentional copy-paste artifact.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e/react-router/basepath-file-based/src/routeTree.gen.ts(3 hunks)e2e/react-router/basepath-file-based/src/routes/index.tsx(1 hunks)e2e/react-router/basepath-file-based/src/routes/redirect.tsx(1 hunks)e2e/react-router/basepath-file-based/tests/reload-document.test.ts(1 hunks)e2e/react-start/custom-basepath/tests/navigation.spec.ts(1 hunks)packages/router-core/src/rewrite.ts(1 hunks)packages/router-core/src/router.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:
e2e/react-router/basepath-file-based/src/routes/redirect.tsxe2e/react-router/basepath-file-based/tests/reload-document.test.tse2e/react-router/basepath-file-based/src/routes/index.tsxpackages/router-core/src/rewrite.tse2e/react-start/custom-basepath/tests/navigation.spec.tspackages/router-core/src/router.tse2e/react-router/basepath-file-based/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/basepath-file-based/src/routes/redirect.tsxe2e/react-router/basepath-file-based/tests/reload-document.test.tse2e/react-router/basepath-file-based/src/routes/index.tsxpackages/router-core/src/rewrite.tse2e/react-start/custom-basepath/tests/navigation.spec.tspackages/router-core/src/router.tse2e/react-router/basepath-file-based/src/routeTree.gen.ts
🧠 Learnings (11)
📓 Common learnings
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: 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.
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-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-router/basepath-file-based/src/routes/redirect.tsxe2e/react-router/basepath-file-based/tests/reload-document.test.tse2e/react-router/basepath-file-based/src/routes/index.tsxpackages/router-core/src/rewrite.tspackages/router-core/src/router.tse2e/react-router/basepath-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: Applies to **/*.{js,ts,tsx} : Implement ESLint rules for router best practices using the ESLint plugin router
Applied to files:
e2e/react-router/basepath-file-based/src/routes/redirect.tsxe2e/react-router/basepath-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: Use file-based routing in `src/routes/` directories or code-based routing with route definitions
Applied to files:
e2e/react-router/basepath-file-based/src/routes/redirect.tsxe2e/react-router/basepath-file-based/src/routeTree.gen.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:
e2e/react-router/basepath-file-based/src/routes/redirect.tsxpackages/router-core/src/rewrite.tspackages/router-core/src/router.tse2e/react-router/basepath-file-based/src/routeTree.gen.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/rewrite.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/rewrite.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-start/custom-basepath/tests/navigation.spec.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:
e2e/react-start/custom-basepath/tests/navigation.spec.tspackages/router-core/src/router.tse2e/react-router/basepath-file-based/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/basepath-file-based/src/routeTree.gen.ts
📚 Learning: 2025-11-02T16:16:24.898Z
Learnt from: nlynzaad
Repo: TanStack/router PR: 5732
File: packages/start-client-core/src/client/hydrateStart.ts:6-9
Timestamp: 2025-11-02T16:16:24.898Z
Learning: In packages/start-client-core/src/client/hydrateStart.ts, the `import/no-duplicates` ESLint disable is necessary for imports from `#tanstack-router-entry` and `#tanstack-start-entry` because both aliases resolve to the same placeholder file (`fake-start-entry.js`) in package.json during static analysis, even though they resolve to different files at runtime.
Applied to files:
e2e/react-router/basepath-file-based/src/routeTree.gen.ts
🧬 Code graph analysis (2)
e2e/react-router/basepath-file-based/src/routes/redirect.tsx (1)
e2e/react-router/basepath-file-based/src/routes/index.tsx (1)
Route(3-5)
packages/router-core/src/rewrite.ts (1)
packages/router-core/src/path.ts (1)
joinPaths(12-20)
⏰ 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 (5)
e2e/react-start/custom-basepath/tests/navigation.spec.ts (1)
50-52: LGTM - improved test assertions.Using
waitForURLandtoBeInViewport()is more reliable thanwaitForLoadState('networkidle')and manualisVisible()checks. These changes follow Playwright best practices for navigation testing.e2e/react-router/basepath-file-based/src/routes/redirect.tsx (1)
1-12: LGTM - well-structured redirect route for testing.The route correctly uses
throw redirect()inbeforeLoadto trigger a redirect to/about. This follows TanStack Router's recommended pattern for route-level redirects.packages/router-core/src/router.ts (1)
1761-1771: LGTM - correct basepath handling for href construction.The change correctly derives both
publicHrefandhreffrom the rewritten URL path (which includes the basepath). The internalpathname(line 1767) correctly remains the non-rewritten path for route matching purposes.This aligns with the PR objective to fix href construction by using the rewritten URL path.
e2e/react-router/basepath-file-based/src/routeTree.gen.ts (1)
1-95: Autogenerated file - no review required.This file is autogenerated by TanStack Router and should not be manually modified. The changes correctly reflect the addition of the new
/redirectroute to the route tree structure.Based on learnings,
routeTree.gen.tsfiles are not reviewed as they are automatically generated.e2e/react-router/basepath-file-based/tests/reload-document.test.ts (1)
20-28: LGTM! Solid test coverage for basepath-aware redirects.The new test case correctly verifies that programmatic redirects (via
beforeLoad) respect the configured basepath. The test flow is clear: clicking the redirect button should navigate through/redirect(which redirects to/about) and end up at/app/aboutwith the basepath intact.This complements the existing test nicely by covering the redirect scenario alongside the
reloadDocument=truenavigation case.
Contributor
Author
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR addresses issues raised in #5200 and #5202.
It correctly sets the href using the rewrite url path and adds additional logic in the output section of the rewrite to check for paths that is already prefixed with the base path.
We also add an e2e test on router to test for redirects without reloadDocument = true.
This PR replaces #5202 and resolves #5200
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests
✏️ Tip: You can customize this high-level summary in your review settings.