perf(router): use URL.canParse for absolute URL checks - #8251
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Team Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughThe change centralizes absolute URL detection with ChangesURL validation
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to This change centralizes absolute URL detection across routing, links, redirects, and build configuration while preserving internal routing and dangerous-protocol protections. No concrete current-head merge-blocking risk remains. Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
View your CI Pipeline Execution ↗ for commit fcca22d
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview6 package(s) bumped directly, 18 bumped as dependents. 🟩 Patch bumps
|
There was a problem hiding this comment.
remove this test file
| if (typeof to === 'string' && to.indexOf(':') > -1) { | ||
| try { | ||
| new URL(to) | ||
| if (isAbsoluteUrl(to)) { |
There was a problem hiding this comment.
we don't need nested ifs here, isAbsoluteUrl can just be part of the condition
| if (typeof to !== 'string' || to.indexOf(':') === -1) { | ||
| return undefined | ||
| } | ||
| try { | ||
| new URL(to) | ||
| if (isAbsoluteUrl(to)) { |
There was a problem hiding this comment.
we can re-arrange this into a single if block here
| const safeInternal = isSafeInternal(to) | ||
| if (safeInternal) return undefined | ||
| if (typeof to !== 'string' || to.indexOf(':') === -1) return undefined | ||
| try { | ||
| new URL(to as any) | ||
| if (isAbsoluteUrl(to)) { |
There was a problem hiding this comment.
we can re-arrange this into a single if block
There was a problem hiding this comment.
is this testing something useful? or are we just basically testing the built-in implementation of URL.canParse?
There was a problem hiding this comment.
remove this, useful for the PR, but not to keep in the repo
Bundle Size Benchmarks
The following scenarios have bundle-size changes compared with the baseline:
Current gzip tracks all emitted client JS chunks. Initial gzip tracks only the entry/import graph. Trend sparkline is historical current gzip ending with this PR measurement; lower is better. |
Merging this PR will degrade performance by 7.4%
|
| Mode | Benchmark | BASE |
HEAD |
Efficiency | |
|---|---|---|---|---|---|
| ⚡ | Memory | mem client unique-location-churn (solid) |
492.4 KB | 235.8 KB | ×2.1 |
| ⚡ | Memory | mem server error-paths not-found (solid) |
897.8 KB | 551.3 KB | +62.85% |
| ⚡ | Memory | mem server peak-large-page (vue) |
1.1 MB | 1.1 MB | +4.45% |
| ⚡ | Memory | mem server server-fn-churn (vue) |
361.3 KB | 346 KB | +4.44% |
| ⚡ | Memory | mem server error-paths unmatched (solid) |
579.1 KB | 558.3 KB | +3.74% |
| ⚡ | Simulation | ssr request loop (vue) |
719.9 ms | 694.5 ms | +3.65% |
| ⚡ | Simulation | client-history navigation loop (vue) |
124.2 ms | 119.9 ms | +3.54% |
| ⚡ | Memory | mem server error-paths redirect (solid) |
370.9 KB | 358.4 KB | +3.49% |
| ⚡ | Simulation | client-rewrites navigation loop (vue) |
161.3 ms | 156.5 ms | +3.05% |
| 👁 | Memory | mem server error-paths not-found (vue) |
497.8 KB | 2,435.2 KB | -79.56% |
| 👁 | Memory | mem server peak-large-page (solid) |
1.1 MB | 1.2 MB | -5.73% |
| 👁 | Memory | mem server request-churn (solid) |
730.4 KB | 794 KB | -8.01% |
| 👁 | Memory | mem server aborted-requests (react) |
783.4 KB | 963 KB | -18.65% |
| 👁 | Memory | mem server error-paths not-found (react) |
402.1 KB | 440.7 KB | -8.76% |
| 👁 | Memory | mem client navigation-churn (vue) |
1.5 MB | 1.6 MB | -9.69% |
| 👁 | Memory | mem client unique-location-churn (vue) |
426.8 KB | 710.8 KB | -39.95% |
| 👁 | Memory | mem client mount-unmount (solid) |
534.1 KB | 620.4 KB | -13.9% |
Tip
Curious why performance improved? Comment @codspeedbot explain why performance improved on this PR, or directly use the CodSpeed MCP with your agent.
Comparing codex/url-can-parse (fcca22d) with main (09da932)
Footnotes
-
48 benchmarks were skipped, so the baseline results were used instead. If they were deleted from the codebase, click here and archive them to remove them from the performance reports. ↩
![nx-cloud[bot]](https://avatars.githubusercontent.com/in/80458?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
At least one additional CI pipeline execution has run since the conclusion below was written and it may no longer be applicable.
Nx Cloud has identified a possible root cause for your failed CI:
We investigated the failing tanstack-react-start-e2e-rsc:test:e2e--vite-ssr--shard-1-of-6 task and found it crashes with a SyntaxError because @tanstack/router-e2e-utils does not export getDummyServerPort — this happens before any tests run, at Playwright config load time. Our PR only touches URL-parsing logic across router packages and has no connection to this e2e utility package, so the failure is an environment/dependency configuration issue unrelated to these changes.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
2 participants
🎯 Changes
Use
URL.canParsefor checks that only need to know whether a string is an absolute URL. This avoids constructing a URL object on success and throwing on relative paths or malformed URLs.React, Solid, Vue, navigation, and redirects share an internal
isAbsoluteUrlhelper with a constructor fallback for older browsers. The Start and Nitro build plugins useURL.canParsedirectly because their minimum Node versions support it. Existing protocol checks and internal-link fast paths remain in place.React and Solid links use
!isSafeInternal(to) && isAbsoluteUrl(to): slash/dot paths skip parsing, and other destinations go directly to URL validation without separate type or colon checks. The internal helper accepts optionalundefinedand returns a boolean.Local Node 25.8.1 validation benchmark results, median of seven samples of 100,000 checks after warmup, in ns/check:
These are validation microbenchmarks, not whole-router speedups. React and Solid already skip parsing most internal links.
Validation:
pnpm test:unit(30 affected projects), including existing framework link/redirect coverage.pnpm test:eslint(33 affected projects) andpnpm test:types(37 affected projects).✅ Checklist
🚀 Release Impact
Summary by CodeRabbit
URL.canParsethrough a compatibility fallback.