fix: correctly handle client-side vs server-side redirects with rewrites #5330
Conversation
WalkthroughAdds a redirect demo under custom basepath: new /redirect and /redirect/throw-it routes; throw-it throws a redirect to /posts/1 via beforeLoad. Updates generated route tree/types to expose the new routes. Adds E2E tests for client/server redirects and a test id on the post view. Adjusts router core resolveRedirect to use buildLocation()'s Location object when setting redirect href and the Location header. Changes
Sequence Diagram(s)sequenceDiagram
participant U as User
participant B as Browser
participant R as Router (match)
participant Rt as /redirect/throw-it (beforeLoad)
participant C as RouterCore.resolveRedirect
participant P as /posts/1 (PostComponent)
U->>B: Navigate to /base/redirect or /base/redirect/throw-it
B->>R: Router matches route
R->>Rt: call beforeLoad()
Rt-->>R: throw redirect({ to: '/posts/$postId', params:{postId:'1'} })
R->>C: resolveRedirect(redirect)
note right of C: buildLocation(redirect.options) -> Location\nset redirect.options.href = location.href\nuse trimmed location.url for Location header
C-->>R: normalized redirect info
R->>B: instruct navigation to /base/posts/1
B->>R: load /posts/1
R->>P: render PostComponent
P-->>U: displays post view (data-testid="post-view")
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests
Comment |
|
View your CI Pipeline Execution ↗ for commit 329e764
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter
@tanstack/directive-functions-plugin
@tanstack/eslint-plugin-router
@tanstack/history
@tanstack/nitro-v2-vite-plugin
@tanstack/react-router
@tanstack/react-router-devtools
@tanstack/react-router-ssr-query
@tanstack/react-start
@tanstack/react-start-client
@tanstack/react-start-server
@tanstack/router-cli
@tanstack/router-core
@tanstack/router-devtools
@tanstack/router-devtools-core
@tanstack/router-generator
@tanstack/router-plugin
@tanstack/router-ssr-query-core
@tanstack/router-utils
@tanstack/router-vite-plugin
@tanstack/server-functions-plugin
@tanstack/solid-router
@tanstack/solid-router-devtools
@tanstack/solid-start
@tanstack/solid-start-client
@tanstack/solid-start-server
@tanstack/start-client-core
@tanstack/start-plugin-core
@tanstack/start-server-core
@tanstack/start-static-server-functions
@tanstack/start-storage-context
@tanstack/valibot-adapter
@tanstack/virtual-file-routes
@tanstack/zod-adapter
commit: |
![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 (5)
packages/router-core/src/router.ts (1)
2233-2238: Remove unusedhrefcomputation and origin-stripping logic inresolveRedirect.The local
hrefvariable (assigned fromlocation.url) and the subsequent origin-stripping (lines 2234–2237) are never used—redirect.options.hrefis now set directly tolocation.href. Delete lines 2234–2237 to eliminate dead code and clarify the logic.e2e/react-start/custom-basepath/src/routes/redirect/throw-it.tsx (1)
5-8: Optional: avoid extra history entry on redirect.Use
replace: trueso the throw route doesn’t pollute history.throw redirect({ to: '/posts/$postId', params: { postId: '1' }, + replace: true, })e2e/react-start/custom-basepath/src/routes/redirect/index.tsx (1)
10-12: Minor: simplify markup and attach test id to Link.Reduces nesting and keeps the interactive element the target.
- <Link to="/redirect/throw-it"> - <div data-testid="link-to-throw-it">Throw It</div> - </Link> + <Link to="/redirect/throw-it" data-testid="link-to-throw-it"> + Throw It + </Link>e2e/react-start/custom-basepath/tests/navigation.spec.ts (2)
49-54: Use Playwright's built-in visibility matcher for consistency.The test logic is correct, but the assertion style differs from other tests in this file. Playwright provides
toBeVisible()which is more idiomatic and consistent.Apply this diff:
test('client-side redirect', async ({ page }) => { await page.goto('/redirect') await page.getByTestId('link-to-throw-it').click() - expect(await page.getByTestId('post-view').isVisible()).toBe(true) + await expect(page.getByTestId('post-view')).toBeVisible() })
56-59: Use Playwright's built-in visibility matcher for consistency.Same as the client-side test: prefer
toBeVisible()for consistency with the rest of the test suite.Apply this diff:
test('server-side redirect', async ({ page }) => { await page.goto('/redirect/throw-it') - expect(await page.getByTestId('post-view').isVisible()).toBe(true) + await expect(page.getByTestId('post-view')).toBeVisible() })
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e/react-start/custom-basepath/src/routeTree.gen.ts(13 hunks)e2e/react-start/custom-basepath/src/routes/posts.$postId.tsx(1 hunks)e2e/react-start/custom-basepath/src/routes/redirect/index.tsx(1 hunks)e2e/react-start/custom-basepath/src/routes/redirect/throw-it.tsx(1 hunks)e2e/react-start/custom-basepath/tests/navigation.spec.ts(1 hunks)packages/router-core/src/router.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{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.tse2e/react-start/custom-basepath/tests/navigation.spec.tse2e/react-start/custom-basepath/src/routeTree.gen.tse2e/react-start/custom-basepath/src/routes/posts.$postId.tsxe2e/react-start/custom-basepath/src/routes/redirect/throw-it.tsxe2e/react-start/custom-basepath/src/routes/redirect/index.tsx
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
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-start/custom-basepath/tests/navigation.spec.tse2e/react-start/custom-basepath/src/routeTree.gen.tse2e/react-start/custom-basepath/src/routes/posts.$postId.tsxe2e/react-start/custom-basepath/src/routes/redirect/throw-it.tsxe2e/react-start/custom-basepath/src/routes/redirect/index.tsx
**/src/routes/**
📄 CodeRabbit inference engine (AGENTS.md)
Place file-based routes under src/routes/ directories
Files:
e2e/react-start/custom-basepath/src/routes/posts.$postId.tsxe2e/react-start/custom-basepath/src/routes/redirect/throw-it.tsxe2e/react-start/custom-basepath/src/routes/redirect/index.tsx
🧬 Code graph analysis (4)
packages/router-core/src/router.ts (1)
packages/router-core/src/redirect.ts (1)
redirect(57-93)
e2e/react-start/custom-basepath/src/routeTree.gen.ts (1)
e2e/react-router/js-only-file-based/src/routeTree.gen.js (1)
PostsIndexRoute(35-39)
e2e/react-start/custom-basepath/src/routes/redirect/throw-it.tsx (2)
e2e/react-start/custom-basepath/src/routes/posts.$postId.tsx (1)
Route(7-14)e2e/react-start/custom-basepath/src/routes/redirect/index.tsx (1)
Route(3-5)
e2e/react-start/custom-basepath/src/routes/redirect/index.tsx (2)
e2e/react-start/custom-basepath/src/routes/posts.$postId.tsx (1)
Route(7-14)e2e/react-start/custom-basepath/src/routes/redirect/throw-it.tsx (1)
Route(3-10)
⏰ 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 (6)
e2e/react-start/custom-basepath/src/routes/redirect/throw-it.tsx (1)
3-10: LGTM: correct server/client redirect via beforeLoad.This covers SSR (direct hit) and CSR (link click) as intended.
e2e/react-start/custom-basepath/src/routes/redirect/index.tsx (1)
3-5: Route registration looks good.e2e/react-start/custom-basepath/src/routeTree.gen.ts (2)
18-21: Wiring is consistent for new redirect routes.Imports, route updates, module augmentation for
/redirect/throw-it, and root children all align.Also applies to: 57-61, 72-76, 201-204, 271-277, 352-354
250-256: Trailing-slash inconsistency in generated/redirect/index routeFileRoutesByPath['/redirect/'] currently has
path: '/redirect' fullPath: '/redirect'(without a trailing slash), whereas other index routes (e.g.
/users/,/posts/) usepath: '/' fullPath: '/users/' // or '/posts/'Please re-generate and confirm; if it still occurs, update the generator to emit
path: '/' fullPath: '/redirect/'e2e/react-start/custom-basepath/src/routes/posts.$postId.tsx (1)
24-24: LGTM! Test ID enables redirect verification.The addition of
data-testid="post-view"correctly enables the new E2E tests to verify that redirects successfully render the post view component.e2e/react-start/custom-basepath/tests/navigation.spec.ts (1)
49-59: Good test coverage for the basepath redirect fix.These tests appropriately validate both client-side and server-side redirect scenarios under a custom basepath, directly addressing the fix for issue #5324. The successful rendering of post-view implicitly confirms that basepath duplication is not occurring.
| let href = location.url | ||
| if (this.origin && href.startsWith(this.origin)) { | ||
| href = href.replace(this.origin, '') || '/' | ||
| } |
There was a problem hiding this comment.
🛠️ 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.
| 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.
schiller-manuel
force-pushed
the
fix-5324
branch
from
fd1cce7 to
329e764
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
e2e/react-start/custom-basepath/src/routeTree.gen.ts(13 hunks)e2e/react-start/custom-basepath/src/routes/posts.$postId.tsx(1 hunks)e2e/react-start/custom-basepath/src/routes/redirect/index.tsx(1 hunks)e2e/react-start/custom-basepath/src/routes/redirect/throw-it.tsx(1 hunks)e2e/react-start/custom-basepath/tests/navigation.spec.ts(1 hunks)packages/router-core/src/router.ts(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- e2e/react-start/custom-basepath/src/routes/posts.$postId.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- e2e/react-start/custom-basepath/tests/navigation.spec.ts
- e2e/react-start/custom-basepath/src/routes/redirect/index.tsx
- e2e/react-start/custom-basepath/src/routes/redirect/throw-it.tsx
🧰 Additional context used
📓 Path-based instructions (3)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use TypeScript in strict mode with extensive type safety across the codebase
Files:
e2e/react-start/custom-basepath/src/routeTree.gen.tspackages/router-core/src/router.ts
e2e/**
📄 CodeRabbit inference engine (AGENTS.md)
Store end-to-end tests under the e2e/ directory
Files:
e2e/react-start/custom-basepath/src/routeTree.gen.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
🧬 Code graph analysis (2)
e2e/react-start/custom-basepath/src/routeTree.gen.ts (1)
e2e/react-router/js-only-file-based/src/routeTree.gen.js (1)
PostsIndexRoute(35-39)
packages/router-core/src/router.ts (1)
packages/router-core/src/redirect.ts (1)
redirect(57-93)
⏰ 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
| id: '/redirect/', | ||
| path: '/redirect/', | ||
| getParentRoute: () => rootRouteImport, | ||
| } as any) |
There was a problem hiding this comment.
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.
| 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.
There was a problem hiding this comment.
dont ever again bother about the autogenerated file routeTree.gen.ts
There was a problem hiding this comment.
@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.
| 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 | ||
| } |
There was a problem hiding this comment.
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.hrefcontains 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.
| 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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@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/barremains 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.
Signed-off-by: leesb971204 <leesb971204@gmail.com>
fixes #5324
Summary by CodeRabbit