Skip to content

fix: correctly handle client-side vs server-side redirects with rewrites #5330

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Oct 1, 2025
Merged

fix: correctly handle client-side vs server-side redirects with rewrites #5330

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
42 changes: 42 additions & 0 deletions e2e/react-start/custom-basepath/src/routeTree.gen.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,10 @@ import { Route as LogoutRouteImport } from './routes/logout'
import { Route as DeferredRouteImport } from './routes/deferred'
import { Route as IndexRouteImport } from './routes/index'
import { Route as UsersIndexRouteImport } from './routes/users.index'
import { Route as RedirectIndexRouteImport } from './routes/redirect/index'
import { Route as PostsIndexRouteImport } from './routes/posts.index'
import { Route as UsersUserIdRouteImport } from './routes/users.$userId'
import { Route as RedirectThrowItRouteImport } from './routes/redirect/throw-it'
import { Route as PostsPostIdRouteImport } from './routes/posts.$postId'
import { Route as ApiUsersRouteImport } from './routes/api.users'
import { Route as PostsPostIdDeepRouteImport } from './routes/posts_.$postId.deep'
Expand Down Expand Up @@ -52,6 +54,11 @@ const UsersIndexRoute = UsersIndexRouteImport.update({
path: '/',
getParentRoute: () => UsersRoute,
} as any)
const RedirectIndexRoute = RedirectIndexRouteImport.update({
id: '/redirect/',
path: '/redirect/',
getParentRoute: () => rootRouteImport,
} as any)
Comment on lines +58 to +61
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Correct the RedirectIndexRoute path

Route.update currently sets path: '/redirect/', but the generated type metadata just below (see FileRoutesByPath['/redirect/']) expects /redirect without the trailing slash. That mismatch means the runtime route emits /redirect/ URLs while the type-safe APIs point to /redirect, which undermines the new redirect tests and can reintroduce the basepath duplication we’re fixing. Please align the runtime path with the metadata by dropping the trailing slash.

 const RedirectIndexRoute = RedirectIndexRouteImport.update({
   id: '/redirect/',
-  path: '/redirect/',
+  path: '/redirect',
   getParentRoute: () => rootRouteImport,
 } as any)
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
id: '/redirect/',
path: '/redirect/',
getParentRoute: () => rootRouteImport,
} as any)
const RedirectIndexRoute = RedirectIndexRouteImport.update({
id: '/redirect/',
path: '/redirect',
getParentRoute: () => rootRouteImport,
} as any)
🤖 Prompt for AI Agents 
In e2e/react-start/custom-basepath/src/routeTree.gen.ts around lines 58 to 61,
the RedirectIndexRoute is defined with path: '/redirect/' which includes a
trailing slash while the generated type metadata expects '/redirect' without it;
update the runtime route definition to use path: '/redirect' (drop the trailing
slash) so the emitted URLs match the type-safe FileRoutesByPath entries and
avoid basepath duplication.

Copy link
Contributor Author

@schiller-manuel schiller-manuel Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

dont ever again bother about the autogenerated file routeTree.gen.ts

Copy link
Contributor

@coderabbitai coderabbitai bot Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schiller-manuel Understood! I'll skip reviewing routeTree.gen.ts files in future reviews since they're autogenerated.

✏️ Learnings added 
Learnt from: schiller-manuel
PR: TanStack/router#5330
File: e2e/react-start/custom-basepath/src/routeTree.gen.ts:58-61
Timestamp: 2025-10-01T18:31:35.376Z
Learning: Do not review files named `routeTree.gen.ts` in TanStack Router repositories, as these are autogenerated files that should not be manually modified.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.

🧠 Learnings used 
Learnt from: schiller-manuel
PR: TanStack/router#5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.559Z
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.

const PostsIndexRoute = PostsIndexRouteImport.update({
id: '/',
path: '/',
Expand All @@ -62,6 +69,11 @@ const UsersUserIdRoute = UsersUserIdRouteImport.update({
path: '/$userId',
getParentRoute: () => UsersRoute,
} as any)
const RedirectThrowItRoute = RedirectThrowItRouteImport.update({
id: '/redirect/throw-it',
path: '/redirect/throw-it',
getParentRoute: () => rootRouteImport,
} as any)
const PostsPostIdRoute = PostsPostIdRouteImport.update({
id: '/$postId',
path: '/$postId',
Expand Down Expand Up @@ -91,8 +103,10 @@ export interface FileRoutesByFullPath {
'/users': typeof UsersRouteWithChildren
'/api/users': typeof ApiUsersRouteWithChildren
'/posts/$postId': typeof PostsPostIdRoute
'/redirect/throw-it': typeof RedirectThrowItRoute
'/users/$userId': typeof UsersUserIdRoute
'/posts/': typeof PostsIndexRoute
'/redirect': typeof RedirectIndexRoute
'/users/': typeof UsersIndexRoute
'/api/users/$id': typeof ApiUsersIdRoute
'/posts/$postId/deep': typeof PostsPostIdDeepRoute
Expand All @@ -103,8 +117,10 @@ export interface FileRoutesByTo {
'/logout': typeof LogoutRoute
'/api/users': typeof ApiUsersRouteWithChildren
'/posts/$postId': typeof PostsPostIdRoute
'/redirect/throw-it': typeof RedirectThrowItRoute
'/users/$userId': typeof UsersUserIdRoute
'/posts': typeof PostsIndexRoute
'/redirect': typeof RedirectIndexRoute
'/users': typeof UsersIndexRoute
'/api/users/$id': typeof ApiUsersIdRoute
'/posts/$postId/deep': typeof PostsPostIdDeepRoute
Expand All @@ -118,8 +134,10 @@ export interface FileRoutesById {
'/users': typeof UsersRouteWithChildren
'/api/users': typeof ApiUsersRouteWithChildren
'/posts/$postId': typeof PostsPostIdRoute
'/redirect/throw-it': typeof RedirectThrowItRoute
'/users/$userId': typeof UsersUserIdRoute
'/posts/': typeof PostsIndexRoute
'/redirect/': typeof RedirectIndexRoute
'/users/': typeof UsersIndexRoute
'/api/users/$id': typeof ApiUsersIdRoute
'/posts_/$postId/deep': typeof PostsPostIdDeepRoute
Expand All @@ -134,8 +152,10 @@ export interface FileRouteTypes {
| '/users'
| '/api/users'
| '/posts/$postId'
| '/redirect/throw-it'
| '/users/$userId'
| '/posts/'
| '/redirect'
| '/users/'
| '/api/users/$id'
| '/posts/$postId/deep'
Expand All @@ -146,8 +166,10 @@ export interface FileRouteTypes {
| '/logout'
| '/api/users'
| '/posts/$postId'
| '/redirect/throw-it'
| '/users/$userId'
| '/posts'
| '/redirect'
| '/users'
| '/api/users/$id'
| '/posts/$postId/deep'
Expand All @@ -160,8 +182,10 @@ export interface FileRouteTypes {
| '/users'
| '/api/users'
| '/posts/$postId'
| '/redirect/throw-it'
| '/users/$userId'
| '/posts/'
| '/redirect/'
| '/users/'
| '/api/users/$id'
| '/posts_/$postId/deep'
Expand All @@ -174,6 +198,8 @@ export interface RootRouteChildren {
PostsRoute: typeof PostsRouteWithChildren
UsersRoute: typeof UsersRouteWithChildren
ApiUsersRoute: typeof ApiUsersRouteWithChildren
RedirectThrowItRoute: typeof RedirectThrowItRoute
RedirectIndexRoute: typeof RedirectIndexRoute
PostsPostIdDeepRoute: typeof PostsPostIdDeepRoute
}

Expand Down Expand Up @@ -221,6 +247,13 @@ declare module '@tanstack/react-router' {
preLoaderRoute: typeof UsersIndexRouteImport
parentRoute: typeof UsersRoute
}
'/redirect/': {
id: '/redirect/'
path: '/redirect'
fullPath: '/redirect'
preLoaderRoute: typeof RedirectIndexRouteImport
parentRoute: typeof rootRouteImport
}
'/posts/': {
id: '/posts/'
path: '/'
Expand All @@ -235,6 +268,13 @@ declare module '@tanstack/react-router' {
preLoaderRoute: typeof UsersUserIdRouteImport
parentRoute: typeof UsersRoute
}
'/redirect/throw-it': {
id: '/redirect/throw-it'
path: '/redirect/throw-it'
fullPath: '/redirect/throw-it'
preLoaderRoute: typeof RedirectThrowItRouteImport
parentRoute: typeof rootRouteImport
}
'/posts/$postId': {
id: '/posts/$postId'
path: '/$postId'
Expand Down Expand Up @@ -309,6 +349,8 @@ const rootRouteChildren: RootRouteChildren = {
PostsRoute: PostsRouteWithChildren,
UsersRoute: UsersRouteWithChildren,
ApiUsersRoute: ApiUsersRouteWithChildren,
RedirectThrowItRoute: RedirectThrowItRoute,
RedirectIndexRoute: RedirectIndexRoute,
PostsPostIdDeepRoute: PostsPostIdDeepRoute,
}
export const routeTree = rootRouteImport
Expand Down
2 changes: 1 addition & 1 deletion e2e/react-start/custom-basepath/src/routes/posts.$postId.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ function PostComponent() {
const post = Route.useLoaderData()

return (
<div className="space-y-2">
<div className="space-y-2" data-testid="post-view">
<h4 className="text-xl font-bold underline">{post.title}</h4>
<div className="text-sm">{post.body}</div>
<Link
Expand Down
15 changes: 15 additions & 0 deletions e2e/react-start/custom-basepath/src/routes/redirect/index.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,15 @@
import { Link, createFileRoute } from '@tanstack/react-router'

export const Route = createFileRoute('/redirect/')({
component: RouteComponent,
})

function RouteComponent() {
return (
<div className="p-2 flex gap-2">
<Link to="/redirect/throw-it">
<div data-testid="link-to-throw-it">Throw It</div>
</Link>
</div>
)
}
10 changes: 10 additions & 0 deletions e2e/react-start/custom-basepath/src/routes/redirect/throw-it.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,10 @@
import { createFileRoute, redirect } from '@tanstack/react-router'

export const Route = createFileRoute('/redirect/throw-it')({
beforeLoad: () => {
throw redirect({
to: '/posts/$postId',
params: { postId: '1' },
})
},
})
25 changes: 25 additions & 0 deletions e2e/react-start/custom-basepath/tests/navigation.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,3 +45,28 @@ test('Server function URLs correctly include app basepath', async ({
'/custom/basepath/_serverFn/src_routes_logout_tsx--logoutFn_createServerFn_handler',
)
})

test('client-side redirect', async ({ page, baseURL }) => {
await page.goto('/redirect')
await page.getByTestId('link-to-throw-it').click()
await page.waitForLoadState('networkidle')

expect(await page.getByTestId('post-view').isVisible()).toBe(true)
expect(page.url()).toBe(`${baseURL}/posts/1`)
})

test('server-side redirect', async ({ page, baseURL }) => {
await page.goto('/redirect/throw-it')
await page.waitForLoadState('networkidle')

expect(await page.getByTestId('post-view').isVisible()).toBe(true)
expect(page.url()).toBe(`${baseURL}/posts/1`)

// do not follow redirects since we want to test the Location header
await page.request
.get('/redirect/throw-it', { maxRedirects: 0 })
.then((res) => {
const headers = new Headers(res.headers())
expect(headers.get('location')).toBe('/custom/basepath/posts/1')
})
})
9 changes: 4 additions & 5 deletions packages/router-core/src/router.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2230,18 +2230,17 @@ export class RouterCore<

resolveRedirect = (redirect: AnyRedirect): AnyRedirect => {
if (!redirect.options.href) {
let href = this.buildLocation(redirect.options).url
const location = this.buildLocation(redirect.options)
let href = location.url
if (this.origin && href.startsWith(this.origin)) {
href = href.replace(this.origin, '') || '/'
}
Comment on lines +2234 to 2237
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion | 🟠 Major

Remove unused origin-stripping code.

The local href variable (line 2234) is computed from location.url and then processed through origin-stripping logic (lines 2235-2237), but the result is never used. Line 2238 directly assigns location.href instead.

This dead code should be removed for clarity, or if the origin stripping is needed, the logic should be updated to use the processed href variable.

Apply this diff to remove the unused code:

   resolveRedirect = (redirect: AnyRedirect): AnyRedirect => {
     if (!redirect.options.href) {
       const location = this.buildLocation(redirect.options)
-      let href = location.url
-      if (this.origin && href.startsWith(this.origin)) {
-        href = href.replace(this.origin, '') || '/'
-      }
       redirect.options.href = location.href
       redirect.headers.set('Location', redirect.options.href)
     }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
let href = location.url
if (this.origin && href.startsWith(this.origin)) {
href = href.replace(this.origin, '') || '/'
}
resolveRedirect = (redirect: AnyRedirect): AnyRedirect => {
if (!redirect.options.href) {
const location = this.buildLocation(redirect.options)
- let href = location.url
- if (this.origin && href.startsWith(this.origin)) {
- href = href.replace(this.origin, '') || '/'
- }
redirect.options.href = location.href
redirect.headers.set('Location', redirect.options.href)
}
🤖 Prompt for AI Agents 
In packages/router-core/src/router.ts around lines 2234 to 2237, the local href
is computed from location.url and then origin-stripped but never used; remove
the dead origin-stripping logic (the let href = location.url and the subsequent
if that replaces this.origin) so the code uses location.href directly as
currently done, or if origin-stripping is required instead of removal, replace
the later assignment that uses location.href to use the processed href variable
and ensure '/' fallback remains.

redirect.options.href = href
redirect.headers.set('Location', redirect.options.href)
redirect.options.href = location.href
redirect.headers.set('Location', href)
}

if (!redirect.headers.get('Location')) {
redirect.headers.set('Location', redirect.options.href)
}

return redirect
}
Comment on lines 2231 to 2245
Copy link
Contributor

@coderabbitai coderabbitai bot Oct 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Use location.publicHref for the Location header and remove origin-stripping logic.

The past review comment correctly identified dead code at lines 2234-2237. The href variable is computed from location.url and then origin-stripped, but line 2238 assigns location.href (not the processed href) to redirect.options.href. Only the Location header uses the processed href.

Additionally, the manual origin-stripping is fragile—if location.url doesn't start with this.origin (e.g., due to misconfiguration or edge cases), the Location header would incorrectly contain the full URL with origin.

Since buildLocation returns publicHref (the rewritten public-facing path without origin, see lines 1689-1690), use that directly for the Location header.

Apply this diff:

 resolveRedirect = (redirect: AnyRedirect): AnyRedirect => {
   if (!redirect.options.href) {
     const location = this.buildLocation(redirect.options)
-    let href = location.url
-    if (this.origin && href.startsWith(this.origin)) {
-      href = href.replace(this.origin, '') || '/'
-    }
     redirect.options.href = location.href
-    redirect.headers.set('Location', href)
+    redirect.headers.set('Location', location.publicHref)
   }
   if (!redirect.headers.get('Location')) {
     redirect.headers.set('Location', redirect.options.href)
   }
   return redirect
 }

This ensures:

  • redirect.options.href contains the internal path (location.href)
  • The Location header contains the rewritten public path (location.publicHref), correctly handling basepath rewrites per the PR objective
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
resolveRedirect = (redirect: AnyRedirect): AnyRedirect => {
if (!redirect.options.href) {
let href = this.buildLocation(redirect.options).url
const location = this.buildLocation(redirect.options)
let href = location.url
if (this.origin && href.startsWith(this.origin)) {
href = href.replace(this.origin, '') || '/'
}
redirect.options.href = href
redirect.headers.set('Location', redirect.options.href)
redirect.options.href = location.href
redirect.headers.set('Location', href)
}
if (!redirect.headers.get('Location')) {
redirect.headers.set('Location', redirect.options.href)
}
return redirect
}
resolveRedirect = (redirect: AnyRedirect): AnyRedirect => {
if (!redirect.options.href) {
const location = this.buildLocation(redirect.options)
redirect.options.href = location.href
redirect.headers.set('Location', location.publicHref)
}
if (!redirect.headers.get('Location')) {
redirect.headers.set('Location', redirect.options.href)
}
return redirect
}
🤖 Prompt for AI Agents 
In packages/router-core/src/router.ts around lines 2231 to 2245, the code
computes and origin-strips an href from location.url and assigns location.href
to redirect.options.href while using the processed href for the Location header;
remove the origin-stripping and dead-code, keep redirect.options.href =
location.href, and set the Location header to location.publicHref (which already
contains the rewritten public-facing path) so headers use publicHref and
internal href stays as location.href.

Copy link
Contributor Author

@schiller-manuel schiller-manuel Oct 1, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

no, this is exactly what is wanted. in case the origin matches the router's origin (e.g. https://foo.com), then remove so we convert https://foo.com/bar to just /bar

only if the redirect points to a different origin, THEN we want a full url

Copy link
Contributor

@coderabbitai coderabbitai bot Oct 1, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@schiller-manuel, thank you for the clarification! I understand now—the origin-stripping logic is intentional to handle same-origin vs. cross-origin redirects correctly:

  • Same-origin redirects: Strip the origin to produce a relative path (e.g., https://foo.com/bar → /bar)
  • Cross-origin redirects: Keep the full URL (e.g., https://other.com/bar remains unchanged)

This ensures the Location header contains the appropriate format based on whether the redirect target matches the router's origin. I appreciate you explaining the reasoning behind this implementation!

✏️ Learnings added 
Learnt from: schiller-manuel
PR: TanStack/router#5330
File: packages/router-core/src/router.ts:2231-2245
Timestamp: 2025-10-01T18:30:26.559Z
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.

Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.


Expand Down
Loading