router icon indicating copy to clipboard operation
router copied to clipboard

fix(router-core): ensure rewrite basepath applies consistently

Open leesb971204 opened this issue 2 months ago • 21 comments

fixes #5200

Problem

When using the new rewrite system with basepath, redirects from / to /basepath were not working in client-side navigation.

Root Cause

The buildLocation function was returning the original fullPath for the href property 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

    • Links/href generation now uses the rewritten path (without origin) to prevent mixed-origin or malformed links while keeping the full rewritten URL for navigation and deep links.
  • Documentation

    • Examples updated to demonstrate the new rewrite API shape and composing rewrites; public interfaces/signatures unchanged.
  • Tests

    • Added tests validating basepath rewrite behavior and backward compatibility with basepath option.

leesb971204 avatar Sep 24 '25 05:09 leesb971204

Walkthrough

parseLocation 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

Cohort / File(s) Summary
Router href & docs update
packages/router-core/src/router.ts
parseLocation now derives href from rewrittenUrl.href.replace(rewrittenUrl.origin, '') instead of fullPath; url uses rewrittenUrl.href; publicHref remains rewrittenUrl.pathname + rewrittenUrl.search + rewrittenUrl.hash. Examples/docs switch to rewriteBasepath({ basepath: '/basepath' }) and composeRewrites([...]). No public API signature changes.
Tests: basepath / rewrite behavior
packages/react-router/tests/router.test.tsx
Added two tests verifying: (1) rewriteBasepath serves app root while browser URL retains basepath, and (2) backward-compatible basepath option produces same behavior. Tests assert rendered route and sync between router state and history pathname.

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

  • Insik-Han

Poem

A rabbit hops through rewritten ways,
I trim the origin, brighten the blaze.
href now neat, url keeps the whole,
publicHref shows the visible stroll.
🥕✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues Check ✅ Passed The changes implement the output rewrite of the href property in buildLocation to ensure basepath behavior and include new unit tests that verify redirects from “/” to the basepath, directly addressing the objectives of issue #5200.
Out of Scope Changes Check ✅ Passed All modifications are confined to applying the rewrite logic in router-core and adding related tests for basepath handling, and there are no unrelated or extraneous code changes present.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
Title Check ✅ Passed The title adopts a conventional commit style with the scope router-core and clearly signals a bug fix. It specifies that basepath rewrites should apply consistently, directly reflecting the PR’s change to apply output rewrite to the href property in buildLocation. This mapping precisely addresses the redirect issue from '/' to '/basepath' described in issue #5200.
✨ Finishing touches
  • [ ] 📝 Generate Docstrings
🧪 Generate unit tests
  • [ ] Create PR with unit tests
  • [ ] Post copyable unit tests in a comment

Comment @coderabbitai help to get the list of available commands and usage tips.

coderabbitai[bot] avatar Sep 24 '25 05:09 coderabbitai[bot]

🤖 Nx Cloud AI Fix Eligible

An automatically generated fix could have helped fix failing tasks for this run, but Self-healing CI is disabled for this workspace. Visit workspace settings to enable it and get automatic fixes in future runs.

To disable these notifications, a workspace admin can disable them in workspace settings.


View your CI Pipeline Execution ↗ for commit 48702a462d1e53889afa51ae5d479c0e493b0c7d

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

nx-cloud[bot] avatar Sep 24 '25 05:09 nx-cloud[bot]

More templates

@tanstack/arktype-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/arktype-adapter@5202
@tanstack/directive-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/directive-functions-plugin@5202
@tanstack/eslint-plugin-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/eslint-plugin-router@5202
@tanstack/history

npm i https://pkg.pr.new/TanStack/router/@tanstack/history@5202
@tanstack/nitro-v2-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/nitro-v2-vite-plugin@5202
@tanstack/react-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router@5202
@tanstack/react-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-devtools@5202
@tanstack/react-router-ssr-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-router-ssr-query@5202
@tanstack/react-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start@5202
@tanstack/react-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-client@5202
@tanstack/react-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/react-start-server@5202
@tanstack/router-cli

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-cli@5202
@tanstack/router-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-core@5202
@tanstack/router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools@5202
@tanstack/router-devtools-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-devtools-core@5202
@tanstack/router-generator

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-generator@5202
@tanstack/router-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-plugin@5202
@tanstack/router-ssr-query-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-ssr-query-core@5202
@tanstack/router-utils

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-utils@5202
@tanstack/router-vite-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-vite-plugin@5202
@tanstack/server-functions-plugin

npm i https://pkg.pr.new/TanStack/router/@tanstack/server-functions-plugin@5202
@tanstack/solid-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router@5202
@tanstack/solid-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router-devtools@5202
@tanstack/solid-router-ssr-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-router-ssr-query@5202
@tanstack/solid-start

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start@5202
@tanstack/solid-start-client

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-client@5202
@tanstack/solid-start-server

npm i https://pkg.pr.new/TanStack/router/@tanstack/solid-start-server@5202
@tanstack/start-client-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-client-core@5202
@tanstack/start-plugin-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-plugin-core@5202
@tanstack/start-server-core

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-server-core@5202
@tanstack/start-static-server-functions

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-static-server-functions@5202
@tanstack/start-storage-context

npm i https://pkg.pr.new/TanStack/router/@tanstack/start-storage-context@5202
@tanstack/valibot-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/valibot-adapter@5202
@tanstack/virtual-file-routes

npm i https://pkg.pr.new/TanStack/router/@tanstack/virtual-file-routes@5202
@tanstack/vue-router

npm i https://pkg.pr.new/TanStack/router/@tanstack/vue-router@5202
@tanstack/vue-router-devtools

npm i https://pkg.pr.new/TanStack/router/@tanstack/vue-router-devtools@5202
@tanstack/vue-router-ssr-query

npm i https://pkg.pr.new/TanStack/router/@tanstack/vue-router-ssr-query@5202
@tanstack/zod-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/zod-adapter@5202

commit: 48702a4

pkg-pr-new[bot] avatar Sep 24 '25 05:09 pkg-pr-new[bot]

thanks for the PR! any idea why the e2e tests fail?

also, we should add tests for this (ideally some unit tests)

schiller-manuel avatar Sep 26 '25 04:09 schiller-manuel

thanks for the PR! any idea why the e2e tests fail?

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. When I run the Start example on my branch, the app gets stuck due to excessive redirects caused by the basepath. Do you have any idea what might be happening?

I’ll go ahead and add a unit test in the meantime!

leesb971204 avatar Sep 26 '25 04:09 leesb971204

@schiller-manuel

Are there any other things we should consider?

leesb971204 avatar Oct 20 '25 00:10 leesb971204

@leesb971204 Do you need help pushing this over the finish line?

reihwald avatar Nov 12 '25 10:11 reihwald

@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. If there’s any additional way I can contribute, I’d be more than happy to help!

leesb971204 avatar Nov 13 '25 00:11 leesb971204

@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. If there’s any additional way I can contribute, I’d be more than happy to help!

Maybe Manuel lost track of this PR. Have you thought about asking him on Discord?

reihwald avatar Nov 13 '25 14:11 reihwald

@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. If there’s any additional way I can contribute, I’d be more than happy to help!

Maybe Manuel lost track of this PR. Have you thought about asking him on Discord?

Thank you for the feedback. I’ve sent a Discord message to Manuel and will wait for his response.

leesb971204 avatar Nov 13 '25 23:11 leesb971204

Heard something? @leesb971204

reihwald avatar Nov 21 '25 16:11 reihwald

Heard something? @leesb971204

noting 🙁

leesb971204 avatar Nov 24 '25 00:11 leesb971204

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.

nlynzaad avatar Dec 05 '25 22:12 nlynzaad

@nlynzaad

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. I’ll need to look into it more closely, but I believe I’ll have to find a new solution and rewrite the test accordingly.

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. This issue has been going on for quite some time, so I’m not sure anymore whether the problem occurred because the base wasn’t set at all.

leesb971204 avatar Dec 08 '25 04:12 leesb971204

@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 /, but can be set to /custom by configuring the base property in the vite config.

Router needs to know if the app is served at any path other than the default /. This is to ensure navigation happens correctly at that base path.

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.

nlynzaad avatar Dec 08 '25 07:12 nlynzaad

@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 /, but can be set to /custom by configuring the base property in the vite config.

Router needs to know if the app is served at any path other than the default /. This is to ensure navigation happens correctly at that base path.

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 — If we don’t set the base in vite.config and only set basePath in the router, is it expected for useLocation().pathname or window.location.pathname to return '/' when accessed from the root path?

leesb971204 avatar Dec 08 '25 07:12 leesb971204

On initial render yes. Router has not executed any navigations yet to actually update the url with the set basePath

nlynzaad avatar Dec 08 '25 07:12 nlynzaad

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 — but from what I recall, before rewrite was added, the initial render would show the basePath in pathname regardless of the base setting in vite.config.

I tested it with version 1.129.2, and it does behave that way: https://stackblitz.com/edit/github-lebazlff-9dj5638p?file=package.json,src%2Froutes%2F__root.tsx,src%2Fmain.tsx

Below is the test case I ran using the latest version: https://stackblitz.com/edit/github-szphp94v?file=src%2Froutes%2Fabout.tsx,src%2Froutes%2F__root.tsx,src%2Froutes%2Findex.tsx

Shouldn't both versions behave the same way?

leesb971204 avatar Dec 08 '25 08:12 leesb971204

Thank you for looking into this @nlynzaad. I'm not facing the exact same issue as @leesb971204 it seems. Please take a look at https://github.com/TanStack/router/issues/5200#issuecomment-3627658496 for a reproducer for my issue. Would you agree that this is worth a different issue?

reihwald avatar Dec 08 '25 20:12 reihwald

@reihwald yes agreed, please create a new issue. From the description it seems related but different enough for a seperate issue

nlynzaad avatar Dec 08 '25 20:12 nlynzaad

@nlynzaad

How do you think this issue(#5200) should be handled in this PR?

leesb971204 avatar Dec 08 '25 22:12 leesb971204

@leesb971204 see the updated stackblitz apart from the bump in versions I added the base to the vite cconfig

nlynzaad avatar Dec 20 '25 12:12 nlynzaad

@leesb971204 I'm going to close this in favour of #6157 which is updated with the latest main and addresses the issues in rewrite and buildLocation.

nlynzaad avatar Dec 20 '25 15:12 nlynzaad