Uh oh!
There was an error while loading. Please reload this page.
refactor(router-core): redirect skips creating a URL object if href is not a string - #4986
Merged
Merged
Conversation
Contributor
WalkthroughThe redirect() function updates its URL handling to guard on string href and constructs URLs directly. parseRedirect() adds a null check before accessing properties. No exported/public signatures changed. Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant redirect
participant URL
Caller->>redirect: redirect(opts)
alt opts.href is string
redirect->>URL: new URL(opts.href)
URL-->>redirect: URL instance
redirect-->>Caller: proceed with reload logic
else
redirect-->>Caller: skip URL construction/reload check
end
sequenceDiagram
participant Caller
participant parseRedirect
Caller->>parseRedirect: parseRedirect(obj)
alt obj is non-null and has isSerializedRedirect
parseRedirect-->>Caller: parsed redirect
else
parseRedirect-->>Caller: safe fallback (no property access)
end
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested labels
Poem
✨ Finishing Touches
🧪 Generate unit tests
🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File ( |
Contributor
View your CI Pipeline Execution ↗ for commit 8ce3e79
☁️ Nx Cloud last updated this comment at |
More templates
@tanstack/arktype-adapter@tanstack/directive-functions-plugin@tanstack/eslint-plugin-router@tanstack/history@tanstack/react-router@tanstack/react-router-devtools@tanstack/react-router-ssr-query@tanstack/react-start@tanstack/react-start-client@tanstack/react-start-plugin@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-plugin@tanstack/solid-start-server@tanstack/start-client-core@tanstack/start-plugin-core@tanstack/start-server-core@tanstack/start-server-functions-client@tanstack/start-server-functions-fetcher@tanstack/start-server-functions-server@tanstack/start-storage-context@tanstack/valibot-adapter@tanstack/virtual-file-routes@tanstack/zod-adaptercommit: |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/router-core/src/redirect.ts (1)
68-73: Good guard on href type; consider avoiding try/catch cost with a scheme checkThe string guard addresses the runtime error and avoids constructing URL objects for non-strings. If you'd like to further reduce cost for string hrefs, you can skip new URL entirely by checking for an absolute scheme via a regex. This keeps behavior consistent (protocol-relative URLs like // still won’t set reloadDocument, same as the current try/catch path).
- if (!opts.reloadDocument && typeof opts.href === 'string') {- try {- new URL(opts.href)- opts.reloadDocument = true- } catch {}- }+ if (!opts.reloadDocument && typeof opts.href === 'string') {+ // Fast-path: mark as document reload only for absolute URLs (scheme:...).+ if (/^[a-zA-Z][a-zA-Z\d+.-]*:/.test(opts.href)) {+ opts.reloadDocument = true+ }+ }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
packages/router-core/src/redirect.ts(2 hunks)
⏰ 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 (1)
packages/router-core/src/redirect.ts (1)
106-111: Null/object guard prevents NPE in parseRedirectThe additional null check before accessing isSerializedRedirect is correct and avoids runtime errors when null is passed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Creating a
URLobject is a little expensive, and we know it's going to throw ifhrefis not a string. So we can skip it entirely in that case (which is probably most cases?).Summary by CodeRabbit