fix: compare rewritten url to decide if server needs to redirect #6072
Conversation
WalkthroughThe PR changes ParsedLocation.url from a string to a URL object in router-core, updates router logic to propagate and consume URL objects, and updates framework link components and tests to read Changes
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 (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 6a00d7b
☁️ Nx Cloud last updated this comment at |
![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 (1)
packages/router-core/src/location.ts (1)
41-43: Documentation change removes rewrite context.The updated JSDoc for
publicHrefno longer mentions "before any rewrites", which was helpful context for understanding the distinction betweenpublicHrefandhref. Consider preserving this clarification to help developers understand when rewrite transformations have been applied.Apply this diff to restore the rewrite context:
/** * @private - * @description The public href of the location. + * @description The public href of the location before any rewrite transformations. * If a rewrite is applied, the `href` property will be the rewritten URL. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/react-router/src/link.tsx(1 hunks)packages/router-core/src/location.ts(1 hunks)packages/router-core/src/router.ts(5 hunks)packages/solid-router/src/link.tsx(1 hunks)packages/vue-router/src/link.tsx(1 hunks)
🧰 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/solid-router/src/link.tsxpackages/router-core/src/router.tspackages/react-router/src/link.tsxpackages/router-core/src/location.tspackages/vue-router/src/link.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/solid-router/src/link.tsxpackages/router-core/src/router.tspackages/react-router/src/link.tsxpackages/router-core/src/location.tspackages/vue-router/src/link.tsx
🧠 Learnings (3)
📓 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/solid-router/src/link.tsxpackages/router-core/src/router.tspackages/react-router/src/link.tsxpackages/router-core/src/location.tspackages/vue-router/src/link.tsx
📚 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
⏰ 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 (10)
packages/router-core/src/location.ts (1)
50-50: Type change from string to URL object is correct.The change from
url: stringtourl: URLaligns with the PR objective to store URLs as objects rather than strings. Since this field is marked@private, the breaking change impact is contained to internal usage.packages/router-core/src/router.ts (6)
1184-1184: URL object storage is correctly implemented.Storing the URL object directly in the parsed location enables more robust URL manipulation and comparison throughout the router lifecycle.
1768-1768: Rewritten URL stored as object for consistency.Storing the rewritten URL as a URL object maintains consistency with the
parseLocationoutput and enables uniform handling across navigation flows.
2003-2003: Correct href extraction from URL object.Accessing
location.url.hrefproperly extracts the string href from the URL object for document navigation.
2385-2391: ThegetParsedLocationHrefhelper correctly implements origin stripping.The helper extracts the href from the URL object and strips the router's origin when they differ. This aligns with the learning that origin stripping is intentional for same-origin redirects while preserving full URLs for cross-origin cases.
Implementation is correct and consistent with the existing
resolveRedirectpattern.Based on learnings, this method correctly preserves the router's origin-stripping behavior for same-origin redirects.
2396-2396: Correct usage of the new helper in redirect resolution.Using
getParsedLocationHrefensures the redirect location header contains the properly normalized href with origin handling applied.
2060-2066: The condition logic is correct and intentional.The OR logic appropriately handles both scenarios independently:
publicHrefmismatch — Detects when the rewrite function changes the pathname, search, or hash (visible URL changes)originmismatch — Detects when the rewrite function returns a URL with a different origin (e.g., viarewrite.input()returning'https://other-origin.com/path')Both conditions warrant a server-side redirect independently. The rewrite function can modify the origin by returning a new URL string or object with a different origin, so the origin check is not redundant. This design ensures the server correctly handles cases where URL rewriting affects either the path or the origin.
packages/react-router/src/link.tsx (1)
130-143: Href extraction correctly adapted to URL objects.The change properly extracts string hrefs from the new URL objects (
maskedLocation.url.hrefandnext.url.href) while preserving the existing external link detection and origin handling logic.packages/vue-router/src/link.tsx (1)
433-437: Href extraction correctly adapted for Vue.The implementation consistently extracts string hrefs from URL objects, matching the pattern used in the React implementation. The optional chaining on line 436 safely handles cases where
nextLocationmight be undefined.packages/solid-router/src/link.tsx (1)
142-145: Href extraction correctly adapted for SolidJS.The implementation properly extracts string hrefs from URL objects using the SolidJS reactive pattern (
next().url.href), maintaining consistency with the React and Vue implementations.
schiller-manuel
force-pushed
the
fix-rewrite-redirect
branch
from
3b9b4c7 to
6a00d7b
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/router-core/src/router.ts (2)
1842-1930: Don’t leakURLinstances into history state viamaskedLocation(can breakpushState).
You’re strippingnext.url, but...maskedLocationreintroducesmaskedLocation.urlintonextHistory(and then into__tempLocation). IfURLisn’t structured-cloneable in some targets, this can throw at runtime.- if (maskedLocation) { + if (maskedLocation) { + // Omit `url` from maskedLocation before putting it into history/state payloads + // (URL may not be structured-cloneable everywhere) + // eslint-disable-next-line prefer-const + let { url: _maskedUrl, ...maskedLocationForHistory } = maskedLocation as any + nextHistory = { - ...maskedLocation, + ...maskedLocationForHistory, state: { - ...maskedLocation.state, + ...maskedLocationForHistory.state, __tempKey: undefined, __tempLocation: { ...nextHistory, search: nextHistory.searchStr, state: { ...nextHistory.state, __tempKey: undefined!, __tempLocation: undefined!, __TSR_key: undefined!, key: undefined!, // TODO: Remove in v2 - use __TSR_key instead }, }, }, }
2399-2410:resolveRedirectmust setredirect.options.hrefto the same value as the Location header (breaks cross-origin redirects otherwise).
Right now the header usesgetParsedLocationHref(location)butredirect.options.hrefis set tolocation.href(internal, origin-less). On the client,catch (redirect)spreadsredirect.optionsintonavigate(), so cross-origin redirects won’t becomereloadDocumentnavigations.resolveRedirect = (redirect: AnyRedirect): AnyRedirect => { if (!redirect.options.href) { const location = this.buildLocation(redirect.options) const href = this.getParsedLocationHref(location) - redirect.options.href = location.href + // Keep options.href consistent with the Location header so client-side + // redirect handling can correctly detect absolute/cross-origin navigations. + redirect.options.href = href redirect.headers.set('Location', href) }
🧹 Nitpick comments (6)
e2e/solid-start/custom-basepath/tests/navigation.spec.ts (1)
63-77: PreferAPIResponse.headerValue('location')+ avoidawait ... .then(...)in tests.
This keeps the test runtime-independent (no reliance on globalHeaders) and improves readability/debuggability.// do not follow redirects since we want to test the Location header // first go to the route WITHOUT the base path, this will just add the base path - await page.request - .get('/redirect/throw-it', { maxRedirects: 0 }) - .then((res) => { - const headers = new Headers(res.headers()) - expect(headers.get('location')).toBe('/custom/basepath/redirect/throw-it') - }) + const res1 = await page.request.get('/redirect/throw-it', { maxRedirects: 0 }) + expect(res1.headerValue('location')).toBe('/custom/basepath/redirect/throw-it') // now go to the route WITH the base path, this will redirect to the final destination - await page.request - .get('/custom/basepath/redirect/throw-it', { maxRedirects: 0 }) - .then((res) => { - const headers = new Headers(res.headers()) - expect(headers.get('location')).toBe('/custom/basepath/posts/1') - }) + const res2 = await page.request.get('/custom/basepath/redirect/throw-it', { + maxRedirects: 0, + }) + expect(res2.headerValue('location')).toBe('/custom/basepath/posts/1')packages/react-router/src/link.tsx (1)
126-143: Update looks correct forParsedLocation.url: URLmigration; consider guarding/typingmaskedLocation.urlas always-present.
The switch tonext.*.url.hrefaligns with the new location shape and keeps origin-stripping logic intact.packages/react-router/tests/redirect.test.tsx (1)
358-376: Avoid deeptoEqualonURLinstances; assert on.hreforexpect.any(URL)instead.
This expectation may be brittle depending on how the test runner comparesURLobjects.- expect(currentRedirect.options).toEqual({ + expect(currentRedirect.options).toEqual({ _fromLocation: { @@ - url: new URL('http://localhost/'), + url: expect.any(URL), }, @@ }) + expect(currentRedirect.options._fromLocation!.url.href).toBe('http://localhost/')packages/solid-router/tests/redirect.test.tsx (1)
352-370: Avoid deeptoEqualonURLinstances; assert on.hreforexpect.any(URL)instead.expect(currentRedirect.options).toEqual({ _fromLocation: { publicHref: '/', - url: new URL('http://localhost/'), + url: expect.any(URL), hash: '', href: '/', @@ }, @@ }) + expect(currentRedirect.options._fromLocation!.url.href).toBe('http://localhost/')packages/router-core/src/location.ts (1)
40-51: Docs/type change is fine, but call out serialization constraints forurl: URL.
SinceParsedLocationis widely consumed, it’s worth explicitly noting (in docs) thaturlis not JSON-serializable and may be unsafe to store in history/state payloads.packages/react-router/tests/router.test.tsx (1)
345-467: Test updates tolocation.url.hrefare consistent; consider removing redundantnew URL(location.url)usage.
Usingrouter.state.location.url.pathnameis clearer now thaturlis already aURL.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
e2e/react-start/custom-basepath/tests/navigation.spec.ts(1 hunks)e2e/solid-start/custom-basepath/tests/navigation.spec.ts(1 hunks)packages/react-router/src/link.tsx(1 hunks)packages/react-router/tests/redirect.test.tsx(1 hunks)packages/react-router/tests/router.test.tsx(13 hunks)packages/router-core/src/location.ts(1 hunks)packages/router-core/src/router.ts(6 hunks)packages/solid-router/src/link.tsx(1 hunks)packages/solid-router/tests/redirect.test.tsx(1 hunks)packages/vue-router/src/link.tsx(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/solid-router/src/link.tsx
- packages/vue-router/src/link.tsx
🧰 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/react-router/tests/redirect.test.tsxpackages/react-router/tests/router.test.tsxe2e/solid-start/custom-basepath/tests/navigation.spec.tspackages/router-core/src/location.tspackages/solid-router/tests/redirect.test.tsxe2e/react-start/custom-basepath/tests/navigation.spec.tspackages/router-core/src/router.tspackages/react-router/src/link.tsx
**/*.{js,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Implement ESLint rules for router best practices using the ESLint plugin router
Files:
packages/react-router/tests/redirect.test.tsxpackages/react-router/tests/router.test.tsxe2e/solid-start/custom-basepath/tests/navigation.spec.tspackages/router-core/src/location.tspackages/solid-router/tests/redirect.test.tsxe2e/react-start/custom-basepath/tests/navigation.spec.tspackages/router-core/src/router.tspackages/react-router/src/link.tsx
🧠 Learnings (4)
📓 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/react-router/tests/redirect.test.tsxpackages/react-router/tests/router.test.tsxe2e/solid-start/custom-basepath/tests/navigation.spec.tspackages/router-core/src/location.tspackages/solid-router/tests/redirect.test.tsxe2e/react-start/custom-basepath/tests/navigation.spec.tspackages/router-core/src/router.tspackages/react-router/src/link.tsx
📚 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/react-router/tests/redirect.test.tsxpackages/react-router/tests/router.test.tsxe2e/solid-start/custom-basepath/tests/navigation.spec.tspackages/solid-router/tests/redirect.test.tsxe2e/react-start/custom-basepath/tests/navigation.spec.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/react-router/tests/router.test.tsx
⏰ 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
🔇 Additional comments (2)
e2e/react-start/custom-basepath/tests/navigation.spec.ts (1)
63-77: LGTM! Excellent redirect header verification.The two-step header verification properly tests the basepath redirect logic:
- Requesting without basepath correctly expects a redirect to add it
- Requesting with basepath correctly expects a redirect to the final destination
This provides good coverage for the URL comparison fix and ensures the server correctly handles basepath application during redirects.
packages/router-core/src/router.ts (1)
2054-2073: Server redirect decision change matches the PR objective; looks good.
ComparingpublicHref(actual) vsnextLocation.publicHref(post-output-rewrite) plus origin mismatch is the right signal for “server must redirect”.
fixes #6065
Summary by CodeRabbit
Refactor
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.