fix: navigate() with href containing basepath #6335
Merged
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
📝 WalkthroughWalkthroughAdds href-based navigation and tests that exercise basepath-aware href handling (with and without full-document reload) across React/ Solid examples and updates router core to rewrite hrefs via the router’s rewrite pipeline before committing locations. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller as navigate()/click
participant RouterCore as Router (buildAndCommitLocation)
participant Rewriter as executeRewriteInput (rewrite pipeline)
participant LocationBuilder as buildLocation
participant Browser as Browser/navigation
Caller->>RouterCore: request navigation (to or href, reloadDocument?)
RouterCore->>Rewriter: rewrite(parsed.pathname) -- when href provided
Rewriter-->>RouterCore: rewritten pathname
RouterCore->>LocationBuilder: buildLocation(rest.with rewritten pathname)
LocationBuilder-->>RouterCore: Location (href, publicHref, pathname)
RouterCore->>Browser: commitLocation / pushState or full reload (uses publicHref when reload)
Browser-->>Caller: navigation completed / page loaded
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
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 |
|
View your CI Pipeline Execution ↗ for commit ce49fb9
☁️ Nx Cloud last updated this comment at |
schiller-manuel
changed the title
![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: 0
🧹 Nitpick comments (1)
packages/router-core/src/router.ts (1)
2054-2062: Consider usinglocation.publicHrefinstead oflocation.url.href.On lines 2056-2057, both
hrefandpublicHrefare assigned fromlocation.url.href, which includes the full origin (e.g.,http://localhost/app/posts).For
publicHref, it would be more semantically correct to uselocation.publicHrefwhich contains just the path portion (e.g.,/app/posts). This aligns with howpublicHrefis used elsewhere and avoids potential issues with full URLs in certain environments.♻️ Suggested change
if (to !== undefined || !href) { const location = this.buildLocation({ to, ...rest } as any) href = href ?? location.url.href - publicHref = publicHref ?? location.url.href + publicHref = publicHref ?? location.publicHref }Please verify whether using
location.url.href(full URL with origin) vslocation.publicHref(path only) causes any behavioral differences in the failing start framework tests.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/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:
packages/router-core/src/router.ts
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/router-core/src/router.ts
🧠 Learnings (6)
📓 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.
📚 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/router.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.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/router.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:
packages/router-core/src/router.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:
packages/router-core/src/router.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: Test
- GitHub Check: Preview
🔇 Additional comments (1)
packages/router-core/src/router.ts (1)
1980-1991: LGTM! Basepath-aware href rewriting.The logic correctly strips the basepath from the provided
hrefbefore assigning torest.to, preventing the basepath from being doubled whenbuildLocationadds it back. The comment clearly documents the intent.
nlynzaad
approved these changes
Jan 8, 2026
Merged
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
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.
Summary
This PR adds failing tests that reproduce an issue with
navigate({ href: '/basepath/...' })when using a router with a custom basepath.The Issue
When calling
navigate()with anhrefthat already includes the basepath:reloadDocument: The basepath gets doubled (e.g.,/app/app/postsinstead of/app/posts)reloadDocument: true: Works in plain routers (react-router, solid-router), but fails in start frameworks (react-start, solid-start)Test Results
navigate({ href: '/basepath/...' })navigate({ href: '/basepath/...', reloadDocument: true })Root Cause
The issue was introduced in
@tanstack/router-core@1.143.4. InbuildAndCommitLocation, whenhrefis provided, it parses the pathname and setsrest.to = parsed.pathname. Buttois expected to be without the basepath - the router adds the basepath automatically. So passinghref="/app/about"setsto="/app/about", which then becomes/app/app/about.Files Added
e2e/react-router/basepath-file-based/- test buttons and 2 testse2e/solid-router/basepath-file-based/- test buttons and 2 testse2e/react-start/custom-basepath/- navigate-test route and 2 testse2e/solid-start/custom-basepath/- navigate-test route and 2 testsSummary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.