fix(router-core): make navigate publicHref Aware #6205
Conversation
📝 WalkthroughWalkthroughAdds two file-based redirect routes and tests; and extends RouterCore.navigate with an optional Changes
Sequence Diagram(s)sequenceDiagram
participant Client as App (UI)
participant Router as RouterCore
participant History as History/Blockers
participant Window as window.location
Client->>Router: navigate({ to, href?, publicHref?, reloadDocument? })
Router->>Router: parse href → isAbsolute?
alt isAbsolute OR reloadDocument true
Router->>History: run blockers for external navigation
alt blockers allow
Router->>Window: set location.href/replace = (publicHref ?? href)
else blockers block
Router-->>Client: navigation blocked
end
else internal SPA navigation
Router->>Router: update internal state/history (no full reload)
Router-->>Client: navigation complete
end
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 (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: defaults Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (2)**/*.{ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
**/*.{js,ts,tsx}📄 CodeRabbit inference engine (AGENTS.md)
Files:
🧠 Learnings (2)📓 Common learnings📚 Learning: 2025-10-01T18:30:26.591ZApplied to files:
⏰ 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)
🔇 Additional comments (1)
Comment |
|
View your CI Pipeline Execution ↗ for commit 386177d
☁️ 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: 1
🧹 Nitpick comments (2)
packages/router-core/src/link.ts (1)
356-356: Add JSDoc documentation for the newpublicHrefproperty.For consistency with other properties in the
NavigateOptionPropsinterface (e.g.,hashScrollIntoView,replace,resetScroll), please add JSDoc comments explaining:
- What
publicHrefis used for- When it should be provided
- How it differs from the regular
hrefproperty📝 Suggested documentation
reloadDocument?: boolean /** + * The public-facing URL to use when performing a full document navigation. + * This is used when the internal routing path differs from the public URL (e.g., when using URL rewriting or basepath transformations). + * If not provided, the router will compute the public URL from the current location. + * @link [API Docs](https://tanstack.com/router/latest/docs/framework/react/api/router/NavigateOptionsType#publichref) + */ + publicHref?: string + /** * This can be used instead of `to` to navigate to a fully built href, e.g. pointing to an external target. * @link [API Docs](https://tanstack.com/router/latest/docs/framework/react/api/router/NavigateOptionsType#href) */ href?: string - publicHref?: stringpackages/router-core/src/router.ts (1)
2053-2059: Consider clarifying the conditional logic for building location.The condition on line 2053
if (!href || (!publicHref && !hrefIsUrl))could be more explicit. It's building a location when:
- No href is provided, OR
- No publicHref is provided AND href is not a URL
This logic works correctly but might benefit from a comment explaining the intent.
💡 Suggested clarification
if (reloadDocument) { + // Build location to compute href/publicHref if either: + // 1. No href provided, or + // 2. We have a relative href but no explicit publicHref if (!href || (!publicHref && !hrefIsUrl)) { const location = this.buildLocation({ to, ...rest } as any) href = href ?? location.url.href publicHref = publicHref ?? location.url.href } const reloadHref = !hrefIsUrl && publicHref ? publicHref : href
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
e2e/react-router/basepath-file-based/src/routeTree.gen.tse2e/react-router/basepath-file-based/src/routes/index.tsxe2e/react-router/basepath-file-based/src/routes/redirect.tsxe2e/react-router/basepath-file-based/src/routes/redirectReload.tsxe2e/react-router/basepath-file-based/tests/reload-document.test.tspackages/router-core/src/link.tspackages/router-core/src/router.ts
🧰 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.tsxpackages/router-core/src/link.tse2e/react-router/basepath-file-based/src/routes/redirectReload.tsxe2e/react-router/basepath-file-based/src/routes/index.tsxpackages/router-core/src/router.tse2e/react-router/basepath-file-based/src/routeTree.gen.tse2e/react-router/basepath-file-based/tests/reload-document.test.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.tsxpackages/router-core/src/link.tse2e/react-router/basepath-file-based/src/routes/redirectReload.tsxe2e/react-router/basepath-file-based/src/routes/index.tsxpackages/router-core/src/router.tse2e/react-router/basepath-file-based/src/routeTree.gen.tse2e/react-router/basepath-file-based/tests/reload-document.test.ts
🧠 Learnings (9)
📓 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/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: 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: 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.tsxpackages/router-core/src/link.tse2e/react-router/basepath-file-based/src/routes/redirectReload.tsxe2e/react-router/basepath-file-based/src/routes/index.tsxpackages/router-core/src/router.tse2e/react-router/basepath-file-based/src/routeTree.gen.tse2e/react-router/basepath-file-based/tests/reload-document.test.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/link.tse2e/react-router/basepath-file-based/src/routeTree.gen.ts
📚 Learning: 2025-12-21T12:52:35.231Z
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:35.231Z
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/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-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/basepath-file-based/src/routeTree.gen.tse2e/react-router/basepath-file-based/tests/reload-document.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:
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
📚 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/routeTree.gen.ts
🧬 Code graph analysis (2)
e2e/react-router/basepath-file-based/src/routes/redirect.tsx (2)
e2e/react-router/basepath-file-based/src/routes/index.tsx (1)
Route(3-5)e2e/react-router/basepath-file-based/src/routes/redirectReload.tsx (1)
Route(3-8)
e2e/react-router/basepath-file-based/src/routes/redirectReload.tsx (2)
e2e/react-router/basepath-file-based/src/routes/index.tsx (1)
Route(3-5)e2e/react-router/basepath-file-based/src/routes/redirect.tsx (1)
Route(3-8)
⏰ 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)
e2e/react-router/basepath-file-based/tests/reload-document.test.ts (2)
20-28: LGTM! Well-structured test for redirect with basepath.The test correctly verifies that redirects respect the basepath configuration, following the same pattern as the existing navigate test.
30-40: LGTM! Comprehensive coverage for reload document scenario.This test ensures that redirects with
reloadDocument: truealso respect the basepath, which is critical for the publicHref functionality introduced in this PR.e2e/react-router/basepath-file-based/src/routes/redirect.tsx (1)
1-12: LGTM! Standard redirect route implementation.The route correctly uses
beforeLoadto redirect to/aboutbefore rendering. The component definition is appropriate even though it won't be rendered, as it's required by the route definition.e2e/react-router/basepath-file-based/src/routes/redirectReload.tsx (1)
1-12: LGTM! Correctly implements redirect with document reload.This route properly tests the scenario where a redirect occurs with
reloadDocument: true, which is essential for verifying the publicHref handling introduced in this PR.packages/router-core/src/router.ts (3)
2032-2050: LGTM! Good handling of absolute URLs and reloadDocument.The logic correctly:
- Detects absolute URLs using URL constructor
- Automatically sets
reloadDocument: truefor absolute URLs- Handles external navigation scenarios properly
2061-2078: LGTM! Proper blocker handling for external navigation.The implementation correctly checks navigation blockers even for external URLs (unless
ignoreBlockeris set), which maintains consistent blocker behavior across all navigation types.
2080-2084: LGTM! Correct usage of publicHref for document reload.The code properly uses
reloadHref(which preferspublicHrefwhen available) for bothwindow.location.replace()andwindow.location.hrefassignments, ensuring the public-facing URL is used for full page navigations.e2e/react-router/basepath-file-based/src/routeTree.gen.ts (1)
1-113: Autogenerated file - no review needed.This file is automatically generated by TanStack Router (as indicated by the header comment). The changes correctly reflect the new
/redirectand/redirectReloadroutes added in this PR.
This PR addresses issues raised in #5200 and #5202.
Following #6201 this replaces #6157 as this PR makes better use of the distinctions between href and publicHref.
We also add an e2e test on router to test for redirects with and without reloadDocument = true.
Summary by CodeRabbit
New Features
Tests
✏️ Tip: You can customize this high-level summary in your review settings.