fix(react-router): Link isActive on competing optional segment routes#7303
fix(react-router): Link isActive on competing optional segment routes#7303
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughRefactors parameter inheritance in buildLocation to depend on explicit Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
View your CI Pipeline Execution ↗ for commit 6e79fff
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version PreviewNo changeset entries found. Merging this PR will not cause a version bump for any packages. |
Bundle Size Benchmarks
Trend sparkline is historical gzip bytes ending with this PR measurement; lower is better. |
Merging this PR will not alter performance
Comparing Footnotes
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/router-core/src/router.ts`:
- Around line 1863-1875: The bug is that baseParams is seeded from the active
match's params (fromParams/lightweightResult.params) even when dest.from is
specified; instead, when dest.from exists compute inherited params by
re-matching currentLocation.pathname against dest.from (using the same route
matching helper used elsewhere in router.ts) to produce the correct params
object before applying dest.params/functionalUpdate; update the logic in the
inheritParams/baseParams/nextParams block (referencing inheritParams,
baseParams, nextParams, dest.from, fromParams, functionalUpdate,
currentLocation.pathname) to use those re-matched params and add a regression
test that exercises competing optional-segment routes (e.g. /{-$foo}/bar vs
/foo/{-$bar}) to verify href and isActive behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 21e54d3f-c346-4943-834b-f3dc6a5ac203
📒 Files selected for processing (11)
packages/react-router/tests/link.test.tsxpackages/react-router/tests/navigate.test.tsxpackages/react-router/tests/useNavigate.test.tsxpackages/router-core/src/router.tspackages/router-core/tests/build-location.test.tspackages/solid-router/tests/link.test.tsxpackages/solid-router/tests/navigate.test.tsxpackages/solid-router/tests/useNavigate.test.tsxpackages/vue-router/tests/link.test.tsxpackages/vue-router/tests/navigate.test.tsxpackages/vue-router/tests/useNavigate.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/react-router/tests/link.test.tsx
There was a problem hiding this comment.
Nx Cloud has identified a possible root cause for your failed CI:
We classified this failure as an environment state issue rather than a code change. The tanstack-react-start-e2e-transform-asset-urls:test:e2e task failed due to port 44877 already being in use (EADDRINUSE), which is unrelated to our PR's changes to param inheritance in buildLocation. Since this project is not among the touched projects and the error has no connection to the router logic we modified, a re-run in a clean environment should resolve it.
No code changes were suggested for this issue.
Trigger a rerun:
🎓 Learn more about Self-Healing CI on nx.dev
Summary
Fixes param inheritance when building locations so links and navigations only inherit current params when explicitly requested.
Problem
Links to routes with optional params could accidentally reuse params from the current location when
paramswas omitted. This caused competing optional segment links like/foo/barand/barto both resolve as active from/foo/bar.Before this fix, the second link inherited
{ foo: 'foo' }from the current route even though it did not passparams, so it built/foo/barinstead of/barand incorrectly became active alongside the first link.Changes
buildLocationparam inheritance rules inrouter-core.params: true, and explicitfrom.params: trueorfrom.TODO
We should also change the types, so something like this would be allowed:
<Link to="/{-$foo}/bar">(i.e. no required forparamssince$foois optional)Summary by CodeRabbit
Bug Fixes
Tests