fix(router-core): ensure rewrite basepath applies consistently #5202
Conversation
…ation Signed-off-by: leesb971204 <[email protected]>
WalkthroughparseLocation now sources internal href from the rewritten URL (origin removed) and sets url to the full rewritten href; publicHref remains pathname+search+hash. Documentation examples updated to use rewriteBasepath({ basepath }) and composeRewrites. Two new tests validate basepath rewrite and backward-compat behavior. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Router
participant Rewriter
Client->>Router: parseLocation(rawLocation)
Router->>Rewriter: apply rewrite rules(rawLocation)
Rewriter-->>Router: rewrittenUrl { href, origin, pathname, search, hash }
Router->>Router: href = rewrittenUrl.href.replace(rewrittenUrl.origin, '')
Router->>Router: url = rewrittenUrl.href
Router->>Router: publicHref = rewrittenUrl.pathname + rewrittenUrl.search + rewrittenUrl.hash
Router-->>Client: Location { href, url, publicHref, ... }
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
| Command | Status | Duration | Result |
|---|---|---|---|
nx affected --targets=test:eslint,test:unit,tes... |
❌ Failed | 10m 6s | View ↗ |
nx run-many --target=build --exclude=examples/*... |
✅ Succeeded | 1m 39s | View ↗ |
☁️ Nx Cloud last updated this comment at 2025-12-08 04:27:29 UTC
![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 (3)
packages/router-core/src/router.ts (3)
1683-1690: Minor: Buildhreffrom URL parts instead of stripping originSafer and clearer to concatenate
pathname + search + hashthan usingreplaceonhref(avoids accidental substring removal if origin appears elsewhere).- href: rewrittenUrl.href.replace(rewrittenUrl.origin, ''), + href: rewrittenUrl.pathname + rewrittenUrl.search + rewrittenUrl.hash,
1781-1783: Align equality check with newhrefsemantics (comparepublicHref)
buildLocation.hrefis now output‑rewritten whilelatestLocation.href(from parse) remains input‑rewritten. Comparing them will false‑negative under basepath. ComparepublicHrefon both sides to retain the “no-op if nothing changed” optimization.- const isSameUrl = - trimPathRight(this.latestLocation.href) === trimPathRight(next.href) + const isSameUrl = + trimPathRight(this.latestLocation.publicHref) === + trimPathRight(next.publicHref)
1939-1945: SSR parity: compare and redirect usingpublicHrefSame reasoning as above: avoid comparing input‑rewritten
hrefto output‑rewrittenhref. UsepublicHreffor both the comparison and redirect target to prevent unnecessary redirects (or loops) when a basepath rewrite is active.- if ( - trimPath(normalizeUrl(this.latestLocation.href)) !== - trimPath(normalizeUrl(nextLocation.href)) - ) { - throw redirect({ href: nextLocation.href }) - } + if ( + trimPath(normalizeUrl(this.latestLocation.publicHref)) !== + trimPath(normalizeUrl(nextLocation.publicHref)) + ) { + throw redirect({ href: nextLocation.publicHref }) + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/router-core/src/router.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/router-core/src/router.ts
packages/router-core/**
📄 CodeRabbit inference engine (AGENTS.md)
Keep framework-agnostic core router logic in packages/router-core/
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: Preview
- GitHub Check: Test
Signed-off-by: leesb971204 <[email protected]>
|
thanks for the PR! also, we should add tests for this (ideally some unit tests) |
I’m not really sure — honestly, I’m not very familiar with TanStack Start. I’ll go ahead and add a unit test in the meantime! |
…ot route Signed-off-by: leesb971204 <[email protected]>
…ref-rewrite - Typecheck: `tsc -b`
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/react-router/tests/router.test.tsx (2)
2701-2732: Good coverage of basepath on root; add a test for “/” → basepath redirectThis validates internal “/” vs browser “/my-app/” when landing on “/my-app/”. To guard against regressions for the original bug (missing redirect from “/” to basepath), add a companion test starting at “/” and asserting history ends at “/my-app/” while router stays at “/”.
Apply this diff near this test:
+ it('should redirect from "/" to basepath and keep internal pathname "/"', async () => { + const rootRoute = createRootRoute({ component: () => <Outlet /> }) + const indexRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/', + component: () => <div data-testid="home">Home</div>, + }) + const routeTree = rootRoute.addChildren([indexRoute]) + + const history = createMemoryHistory({ initialEntries: ['/'] }) + const router = createRouter({ + routeTree, + history, + rewrite: rewriteBasepath({ basepath: '/my-app' }), + }) + + render(<RouterProvider router={router} />) + + await waitFor(() => { + expect(screen.getByTestId('home')).toBeInTheDocument() + }) + + expect(router.state.location.pathname).toBe('/') + // Allow either trailing or non-trailing slash if normalization changes: + expect(['/my-app', '/my-app/']).toContain(history.location.pathname) + })
2734-2765: Back-compat test looks good; also assert navigation applies output rewrite with basepath optionThis confirms basepath option parity on initial load. Add a navigation test mirroring the Link-based case to ensure output rewrite occurs with basepath option too.
Apply this diff near this block:
+ it('basepath option: Link navigation applies output rewrite', async () => { + const rootRoute = createRootRoute({ component: () => <Outlet /> }) + const indexRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/', + component: () => ( + <div> + <Link to="/about" data-testid="about-link">About</Link> + </div> + ), + }) + const aboutRoute = createRoute({ + getParentRoute: () => rootRoute, + path: '/about', + component: () => <div data-testid="about">About</div>, + }) + const routeTree = rootRoute.addChildren([indexRoute, aboutRoute]) + + const history = createMemoryHistory({ initialEntries: ['/my-app/'] }) + const router = createRouter({ routeTree, history, basepath: '/my-app' }) + + render(<RouterProvider router={router} />) + const aboutLink = await screen.findByTestId('about-link') + fireEvent.click(aboutLink) + + await waitFor(() => { + expect(screen.getByTestId('about')).toBeInTheDocument() + }) + expect(router.state.location.pathname).toBe('/about') + expect(history.location.pathname).toBe('/my-app/about') + })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/react-router/tests/router.test.tsx(1 hunks)packages/router-core/src/router.ts(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/router-core/src/router.ts
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
packages/react-router/tests/router.test.tsx
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/router.test.tsx
🧬 Code graph analysis (1)
packages/react-router/tests/router.test.tsx (5)
packages/react-router/src/route.tsx (2)
createRootRoute(566-618)createRoute(310-390)packages/react-router/src/Match.tsx (1)
Outlet(309-359)packages/history/src/index.ts (1)
createMemoryHistory(568-614)packages/react-router/src/router.ts (1)
createRouter(80-82)packages/react-router/src/RouterProvider.tsx (1)
RouterProvider(47-56)
⏰ 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
leesb971204
changed the title
Signed-off-by: leesb971204 <[email protected]>
Signed-off-by: leesb971204 <[email protected]>
…tly instead of rewrite Signed-off-by: leesb971204 <[email protected]>
…ponent Signed-off-by: leesb971204 <[email protected]>
…ponent Signed-off-by: leesb971204 <[email protected]>
leesb971204
force-pushed
the
fix/buildlocation-href-rewrite
branch
from
58ff465 to
6aa38f1
Compare
Signed-off-by: leesb971204 <[email protected]>
Signed-off-by: leesb971204 <[email protected]>
| const nextLocation = this.buildLocation({ | ||
| to, | ||
| ...rest, | ||
| _includeValidateSearch: true, | ||
| _isNavigate: true, | ||
| } as any) |
There was a problem hiding this comment.
Call buildLocation only once and reuse the result.
| return this.buildAndCommitLocation({ | ||
| ...rest, | ||
| href, | ||
| to: to as string, | ||
| _isNavigate: true, | ||
| return this.commitLocation({ | ||
| ...nextLocation, | ||
| replace: rest.replace, | ||
| resetScroll: rest.resetScroll, | ||
| hashScrollIntoView: rest.hashScrollIntoView, | ||
| viewTransition: rest.viewTransition, | ||
| ignoreBlocker: rest.ignoreBlocker, |
There was a problem hiding this comment.
Since buildLocation is always called above, only commitLocation is executed here.
| const currentOrigin = new URL(this.latestLocation.url).origin | ||
| const nextOrigin = new URL(nextLocation.url).origin | ||
|
|
||
| if (currentOrigin !== nextOrigin) { | ||
| reloadDocument = true | ||
| href = nextLocation.url | ||
| } |
There was a problem hiding this comment.
Compare the origins to perform a hard navigation.
leesb971204
force-pushed
the
fix/buildlocation-href-rewrite
branch
from
6a014f1 to
bc36e9d
Compare
…ref-rewrite - Typecheck: `tsc -b`
|
Are there any other things we should consider? |
|
@leesb971204 Do you need help pushing this over the finish line? |
I’ve handled the issue in what I believe is the appropriate way and am currently waiting for a review. |
Maybe Manuel lost track of this PR. Have you thought about asking him on Discord? |
Thank you for the feedback. |
|
Heard something? @leesb971204 |
noting 🙁 |
|
Hi @leesb971204 there was quite a rewrite that was completed on router-core recently. Would you be able to merge this with the latest main? I just ran the tests you included, and they passed on the latest builds without any changes so not too sure if the tests aren't built from a failing initially point or if the problem has been resolved? Also took the original issue's repro, set the base in vite (to ensure its initially mounted in the correct path during dev) and did not see any specific issues. I'm probably missing something here, so if you could merge with the latest main and ensure failing tests before your changes are applied and then obviously succeeding after your changes. |
|
As you suggested, I just ran the tests on both the latest main branch and a PR branch that merges in the latest main. In both cases, the tests passed successfully. However, when I merge main into the PR branch again, the issue reappears — even though it was previously resolved and working as expected. I suspect that recent changes in the core are invalidating the fix I applied earlier. Also, when you said that setting Vite’s base solves the issue — does that mean this work is unnecessary? To be honest, I don’t clearly remember whether setting only the router’s basePath (without Vite’s base) was enough to make things work. |
|
@leesb971204 thanks for merging main into this. When the app is served, through vite dev or another bundler or server in prod this will load at the route it is served at. In vite dev's case this is by defualt at Router needs to know if the app is served at any path other than the default While it can deduce this in vite dev's case, when served from another server or bundler there is no way for Router to know what the base path is. We therefore need to do this by configuration which is done by setting the basePath property in createRouter. In other words, these needs to match. When updating the PR in the original issue and ensuring the app is served at the correct location I was no longer able to see a problem. So before any more work is done on this PR I think we need to ensure the issue this PR is trying to solve is in actual fact still occuring. @reihwald seeing as you are experiencing a similar issue could you please add your own reproducer onto the original issue, ensure its updated to the latest main, the app is served at the correct custom path and the basePath in router is set to match this custom path. |
I’m really sorry for asking again, but just to confirm — |
|
On initial render yes. Router has not executed any navigations yet to actually update the url with the set basePath |
In that case, this PR might not be necessary — I tested it with version 1.129.2, and it does behave that way: Below is the test case I ran using the latest version: Shouldn't both versions behave the same way? |
|
Thank you for looking into this @nlynzaad. I'm not facing the exact same issue as @leesb971204 it seems. Please take a look at #5200 (comment) for a reproducer for my issue. Would you agree that this is worth a different issue? |
|
@reihwald yes agreed, please create a new issue. From the description it seems related but different enough for a seperate issue |
5 participants
fixes #5200
Problem
When using the new rewrite system with basepath, redirects from
/to/basepathwere not working in client-side navigation.Root Cause
The
buildLocationfunction was returning the originalfullPathfor thehrefproperty instead of applying the output rewrite. This caused the Transitioner component to compare non-rewritten URLs, preventing proper redirect detection.Summary by CodeRabbit
Bug Fixes
Documentation
Tests