Skip to content

Conversation

@schiller-manuel
Copy link
Contributor

@schiller-manuel schiller-manuel commented Oct 18, 2025

fixes #5477

Summary by CodeRabbit

  • Bug Fixes
    • Redirect targets now derive from standardized URL data, reducing incorrect or unexpected redirects.
    • Redirect destinations are normalized to remove redundant origin parts so redirects behave consistently across routing scenarios.

@coderabbitai
Copy link
Contributor

coderabbitai bot commented Oct 18, 2025

Walkthrough

beforeLoad now derives the redirect target from nextLocation.url (not nextLocation.href), strips the origin when present, and calls redirect({ href }) with the normalized value.

Changes

Cohort / File(s) Summary
Before-load redirect normalization
packages/router-core/src/router.ts
Use nextLocation.url as the source for redirect href, remove origin if present, and pass the stripped value to redirect({ href }) instead of using nextLocation.href.

Sequence Diagram

sequenceDiagram
    participant Router
    participant beforeLoad as beforeLoad
    participant Redirect as redirect()

    rect rgb(220, 255, 220)
    Note over Router,beforeLoad: New redirect resolution
    Router->>beforeLoad: determine nextLocation
    beforeLoad->>beforeLoad: read nextLocation.url (not .href)
    beforeLoad->>beforeLoad: if origin present -> strip origin
    beforeLoad->>Redirect: redirect({ href: sanitizedHref })
    Redirect->>Router: perform navigation
    end
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Poem

🐰
I hopped along the route one day,
swapped href for url along the way,
I trimmed the origin, clean and neat,
sent the redirect home — no repeat.
Tiny paws, big fixes — hooray! 🥕✨

Pre-merge checks and finishing touches

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Title Check ⚠️ Warning The title contains a typo ("causd" instead of "caused") and uses vague phrasing ("respect rewrite") that makes the intent unclear. While it does reference actual elements of the change—server-side redirects and default search params—the awkward wording and spelling error prevent it from being clear and specific enough for a teammate to quickly understand the primary change from the title alone. The title attempts to address the core issue but falls short of being concise and readable. The title should be revised to correct the typo and clarify the change. A clearer alternative might be "fix: server-side redirects for default search params with base paths" or "fix: default search params redirect with base path configuration". This would maintain the connection to the linked issue while being more concise and grammatically correct.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues Check ✅ Passed Issue #5477 describes a bug where default search params defined via a zod schema fail to redirect correctly when base paths are configured. The expected behavior is that visiting a path like /base/foo should redirect to /base/foo?search=query. The PR modifies the redirect computation in beforeLoad to compute the redirect href from nextLocation.url instead of nextLocation.href, which aligns with URL-based location data and addresses the redirect handling issue described in the linked issue.
Out of Scope Changes Check ✅ Passed The changeset only modifies packages/router-core/src/router.ts with focused changes to the redirect computation logic in the beforeLoad function. This change is directly related to fixing the server-side redirect behavior when default search params are configured with base paths, which is the core objective of linked issue #5477. No unrelated or extraneous changes are present in the PR.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch fix-5477

📜 Recent review details

Configuration used: CodeRabbit UI

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between d00f2d0 and d40c54c.

📒 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
🧠 Learnings (2)
📓 Common learnings
Learnt from: schiller-manuel
PR: TanStack/router#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
PR: TanStack/router#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/router-core/src/router.ts
🧬 Code graph analysis (1)
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: Test
  • GitHub Check: Preview
🔇 Additional comments (1)
packages/router-core/src/router.ts (1)

1968-1973: LGTM! Consistent origin-stripping pattern for server-side redirects.

The change correctly uses nextLocation.url (the full rewritten URL) instead of nextLocation.href (internal path-only representation), then strips the origin. This ensures server-side redirects respect base path configuration by using the public URL after rewrite processing.

The implementation follows the same pattern as the existing resolveRedirect method (lines 2260-2268), maintaining consistency across the codebase. Based on learnings, this origin-stripping logic is intentional to preserve same-origin redirects as paths while allowing cross-origin redirects to remain full URLs.


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

@nx-cloud
Copy link

nx-cloud bot commented Oct 18, 2025

View your CI Pipeline Execution ↗ for commit d40c54c

Command Status Duration Result
nx affected --targets=test:eslint,test:unit,tes... ✅ Succeeded 5m 24s View ↗
nx run-many --target=build --exclude=examples/*... ✅ Succeeded 1m 22s View ↗

☁️ Nx Cloud last updated this comment at 2025-10-18 00:30:30 UTC

@pkg-pr-new
Copy link

pkg-pr-new bot commented Oct 18, 2025

More templates

@tanstack/arktype-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/arktype-adapter@5523

@tanstack/directive-functions-plugin

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

@tanstack/eslint-plugin-router

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

@tanstack/history

npm i https://pkg.pr.new/TanStack/router/@tanstack/history@5523

@tanstack/nitro-v2-vite-plugin

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

@tanstack/react-router

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

@tanstack/react-router-devtools

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

@tanstack/react-router-ssr-query

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

@tanstack/react-start

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

@tanstack/react-start-client

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

@tanstack/react-start-server

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

@tanstack/router-cli

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

@tanstack/router-core

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

@tanstack/router-devtools

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

@tanstack/router-devtools-core

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

@tanstack/router-generator

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

@tanstack/router-plugin

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

@tanstack/router-ssr-query-core

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

@tanstack/router-utils

npm i https://pkg.pr.new/TanStack/router/@tanstack/router-utils@5523

@tanstack/router-vite-plugin

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

@tanstack/server-functions-plugin

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

@tanstack/solid-router

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

@tanstack/solid-router-devtools

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

@tanstack/solid-start

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

@tanstack/solid-start-client

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

@tanstack/solid-start-server

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

@tanstack/start-client-core

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

@tanstack/start-plugin-core

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

@tanstack/start-server-core

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

@tanstack/start-static-server-functions

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

@tanstack/start-storage-context

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

@tanstack/valibot-adapter

npm i https://pkg.pr.new/TanStack/router/@tanstack/valibot-adapter@5523

@tanstack/virtual-file-routes

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

@tanstack/zod-adapter

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

commit: d40c54c

@schiller-manuel schiller-manuel merged commit de16769 into main Oct 18, 2025
6 checks passed
@schiller-manuel schiller-manuel deleted the fix-5477 branch October 18, 2025 00:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Start: default search params not working properly with base paths configured

2 participants