Uh oh!
There was an error while loading. Please reload this page.
Fix default route component remounting - #8002
Conversation
📝 WalkthroughWalkthroughSolid and Vue router match components now preserve mounted components during route parameter changes unless ChangesRouter runtime updates
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
🚥 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 5ec9a20
☁️ Nx Cloud last updated this comment at |
🚀 Changeset Version Preview2 package(s) bumped directly, 6 bumped as dependents. 🟩 Patch bumps
|
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 regress 0 benchmarks
|
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/vue-router/src/link.tsx (1)
104-185: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftMake external-link selection and props reactive.
type.valueis read only during setup, so changes totocannot switch between internal and external behavior. External props also capture the initialto,target,class, anddisabledvalues. Compute the branch and external props from a reactive source. Add tests for both transition directions and external URL updates.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vue-router/src/link.tsx` around lines 104 - 185, The link branch and external props in the setup flow must remain reactive when link options change. Update the logic around the computed type and externalProps so it derives the current options inside reactive computation, allowing `to` to transition between internal and external URLs in both directions and ensuring external `href`, `target`, `class`, and `disabled` values update; add tests covering both transitions and external URL changes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/vue-router/src/link.tsx`:
- Around line 116-117: Update the stable event-handler wrappers in the link
setup to resolve user handlers from the current options snapshot at event time
rather than from the setup-time initialOptions/eventHandlers. Preserve the
stable wrapper identity while ensuring replaced or removed handlers are
reflected for every event.
- Around line 898-901: Update LinkImpl’s allProps/linkPropsSource flow to
explicitly refresh the cached fallthrough attributes from attrs whenever the
parent updates, ensuring dynamic class, aria-label, and native listener changes
are observed. Add a regression test that updates a fallthrough attribute and
verifies LinkImpl reflects the new value.
---
Outside diff comments:
In `@packages/vue-router/src/link.tsx`:
- Around line 104-185: The link branch and external props in the setup flow must
remain reactive when link options change. Update the logic around the computed
type and externalProps so it derives the current options inside reactive
computation, allowing `to` to transition between internal and external URLs in
both directions and ensuring external `href`, `target`, `class`, and `disabled`
values update; add tests covering both transitions and external URL changes.
🪄 Autofix
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 Plus
Run ID: d8cd4609-8c8a-4f20-933d-0e0660ba7aee
📒 Files selected for processing (5)
packages/solid-router/src/Match.tsxpackages/solid-router/tests/remountDeps.test.tsxpackages/vue-router/src/Match.tsxpackages/vue-router/src/link.tsxpackages/vue-router/tests/remountDeps.test.tsx
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (7)
packages/vue-router/src/link.tsx (4)
565-581: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant
...staticEventHandlersspread.Line 565 spreads
staticEventHandlers. Lines 571-581 then assign each handler withstaticEventHandlers?.x ?? fallback. WhenstaticEventHandlersis defined, the explicit assignments repeat the spread. When it is undefined, the spread contributes nothing. Delete Line 565 to keep one source for each handler.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vue-router/src/link.tsx` around lines 565 - 581, Remove the redundant ...staticEventHandlers spread from the props object near the explicit event-handler assignments. Keep the existing staticEventHandlers?.x ?? fallback expressions for each handler so handler precedence and behavior remain unchanged.
169-192: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueClarify the duplicated
type.value === 'external'guards.The outer check at Line 178 evaluates once during setup. The inner check at Line 180 re-evaluates when
typechanges. The pair encodes a real behavior: a link that starts external and later becomes internal must begin subscribing to the location store, becauseuseStorewas skipped at setup. The code does not state this intent, so a later reader can remove one guard and break the external-to-internal transition.Add a short comment that records the intent.
♻️ Proposed clarification
+ // Links that start external skip the `useStore` subscription above. If `to`+ // later becomes internal, this effect re-runs and starts tracking location. if (type.value === 'external') { Vue.watchEffect((onCleanup) => { if (type.value === 'external') { return }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vue-router/src/link.tsx` around lines 169 - 192, Add a short comment around the duplicated external-type guards in the currentLocation/watchEffect setup, explicitly documenting that links changing from external to internal must start subscribing to router.stores.location because useStore was skipped initially. Preserve both guards and their existing transition behavior.
238-289: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDeclare
pendingPreloadbeforeenqueuePreload.
enqueuePreloadreads and writespendingPreloadat Lines 244, 251, and 254, but theletdeclaration appears at Line 283. The code works because no call occurs before the declaration. Moving the declaration above the function removes the temporal dead zone risk if a future change callsenqueuePreloadearlier.♻️ Proposed reordering
+ let pendingPreload: 'intent' | 'viewport' | undefined+ const enqueuePreload = ( e?: MouseEvent | FocusEvent | IntersectionObserverEntry, ) => {- let pendingPreload: 'intent' | 'viewport' | undefined- useIntersectionObserver(🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vue-router/src/link.tsx` around lines 238 - 289, Move the pendingPreload declaration above the enqueuePreload function so all reads and writes in enqueuePreload reference an initialized binding, preserving its existing union type and behavior.
291-301: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winUse
Vue.watchEffectfor render preloading.
Vue.effectruns synchronously whenpreloadornextchanges. This can calldoPreload()during component update processing.Vue.watchEffectuses pre-flush scheduling by default and follows the component effect scope.♻️ Proposed change
- Vue.effect(() => {+ Vue.watchEffect(() => {🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vue-router/src/link.tsx` around lines 291 - 301, Replace the Vue.effect used for render preloading with Vue.watchEffect, preserving the existing preload.value check, next.value.href comparison, renderFetchedHref update, and doPreload invocation. Keep the effect within the component’s existing effect scope so it uses watchEffect’s default pre-flush scheduling.packages/vue-router/tests/link.test.tsx (3)
541-543: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winMove the
data-statusassertion insidewaitFor.Line 542 waits for the history pathname. The DOM patch that adds
data-status="active"happens in a later Vue flush. Line 543 can run before that patch and cause a flaky failure.💚 Proposed change
await fireEvent.click(link) - await waitFor(() => expect(window.location.pathname).toBe('/posts'))- expect(link).toHaveAttribute('data-status', 'active')+ await waitFor(() => {+ expect(window.location.pathname).toBe('/posts')+ expect(link).toHaveAttribute('data-status', 'active')+ })🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vue-router/tests/link.test.tsx` around lines 541 - 543, Update the test’s waitFor callback around fireEvent.click(link) so it waits for both the /posts pathname and the link’s data-status="active" attribute. Remove the separate assertion after waitFor while preserving the existing navigation expectation.
5565-5583: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a positive control to this cancellation test.
All three assertions are negative. If intent preloading stops working completely, this test still passes. Add a final case where no input changes during the delay and assert that
preloadRouteSpyis called once.💚 Proposed addition
await fireEvent.mouseEnter(link) to.value = 'https://example.com' await Vue.nextTick() await vi.advanceTimersByTimeAsync(50) expect(preloadRouteSpy).not.toHaveBeenCalled() ++ to.value = '/about'+ await Vue.nextTick()+ await fireEvent.mouseEnter(link)+ await vi.advanceTimersByTimeAsync(50)+ expect(preloadRouteSpy).toHaveBeenCalledOnce()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vue-router/tests/link.test.tsx` around lines 5565 - 5583, Add a positive-control case to the existing link preloading cancellation test after the disabled, changed-target, and external-URL scenarios: restore a preloadable internal target, trigger mouseenter without changing inputs during the delay, advance the timers, and assert preloadRouteSpy is called once.
603-611: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSeparate handler removal from the
disabledchange.Lines 603-605 clear both handlers and set
disabledto true in one step. The assertions at Lines 610-611 then pass even if handler removal is broken, because a disabled link suppresses the outcome anyway. Clear the handlers first withdisabledstill false, assert the counts, then setdisabled.💚 Proposed change
clickHandler.value = undefined mouseEnterHandler.value = undefined - disabled.value = true await Vue.nextTick() await fireEvent.click(link) await fireEvent.mouseEnter(link) expect(secondClick).toHaveBeenCalledOnce() expect(secondMouseEnter).toHaveBeenCalledOnce() ++ disabled.value = true+ await Vue.nextTick()+ await fireEvent.click(link)+ await fireEvent.mouseEnter(link)++ expect(secondClick).toHaveBeenCalledOnce()+ expect(secondMouseEnter).toHaveBeenCalledOnce()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/vue-router/tests/link.test.tsx` around lines 603 - 611, Update the test around the disabled-link scenario to clear clickHandler and mouseEnterHandler while disabled remains false, await the update, and assert neither handler is invoked. Then set disabled to true in a separate update and retain the existing disabled-link assertions, ensuring handler removal and disabled behavior are tested independently.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/vue-router/src/link.tsx`:
- Around line 562-582: The external-link props assembled by getExternalLinkProps
must match combineResultProps for disabled links: when options.disabled is true,
omit href and emit role="link" with aria-disabled, rather than forwarding a raw
disabled attribute. Update packages/vue-router/src/link.tsx lines 562-582
accordingly; update packages/vue-router/tests/link.test.tsx lines 470-494 to
assert aria-disabled and role instead of disabled attributes.
In `@packages/vue-router/tests/link.test.tsx`:
- Line 470: Update the assertions in the link tests around the external-link and
false-disabled cases to stop expecting a native disabled attribute on <a>.
Assert the accessible contract instead: the element retains role="link" and
communicates disabled state through aria-disabled when applicable, while enabled
links do not expose disabled="false".
---
Nitpick comments:
In `@packages/vue-router/src/link.tsx`:
- Around line 565-581: Remove the redundant ...staticEventHandlers spread from
the props object near the explicit event-handler assignments. Keep the existing
staticEventHandlers?.x ?? fallback expressions for each handler so handler
precedence and behavior remain unchanged.
- Around line 169-192: Add a short comment around the duplicated external-type
guards in the currentLocation/watchEffect setup, explicitly documenting that
links changing from external to internal must start subscribing to
router.stores.location because useStore was skipped initially. Preserve both
guards and their existing transition behavior.
- Around line 238-289: Move the pendingPreload declaration above the
enqueuePreload function so all reads and writes in enqueuePreload reference an
initialized binding, preserving its existing union type and behavior.
- Around line 291-301: Replace the Vue.effect used for render preloading with
Vue.watchEffect, preserving the existing preload.value check, next.value.href
comparison, renderFetchedHref update, and doPreload invocation. Keep the effect
within the component’s existing effect scope so it uses watchEffect’s default
pre-flush scheduling.
In `@packages/vue-router/tests/link.test.tsx`:
- Around line 541-543: Update the test’s waitFor callback around
fireEvent.click(link) so it waits for both the /posts pathname and the link’s
data-status="active" attribute. Remove the separate assertion after waitFor
while preserving the existing navigation expectation.
- Around line 5565-5583: Add a positive-control case to the existing link
preloading cancellation test after the disabled, changed-target, and
external-URL scenarios: restore a preloadable internal target, trigger
mouseenter without changing inputs during the delay, advance the timers, and
assert preloadRouteSpy is called once.
- Around line 603-611: Update the test around the disabled-link scenario to
clear clickHandler and mouseEnterHandler while disabled remains false, await the
update, and assert neither handler is invoked. Then set disabled to true in a
separate update and retain the existing disabled-link assertions, ensuring
handler removal and disabled behavior are tested independently.
🪄 Autofix
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 Plus
Run ID: c266438a-d53a-4f5a-927d-9b438f5c92a6
📒 Files selected for processing (7)
.changeset/warm-routes-rest.mdpackages/solid-router/src/Match.tsxpackages/solid-router/tests/remountDeps.test.tsxpackages/vue-router/src/Match.tsxpackages/vue-router/src/link.tsxpackages/vue-router/tests/link.test.tsxpackages/vue-router/tests/remountDeps.test.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/vue-router/tests/remountDeps.test.tsx
- packages/solid-router/tests/remountDeps.test.tsx
- packages/solid-router/src/Match.tsx
Uh oh!
There was an error while loading. Please reload this page.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/vue-router/tests/link.test.tsx`:
- Around line 5599-5604: Update the preload assertion in the test around
preloadRouteSpy to verify the call argument includes { to: '/about' }, rather
than checking only that it was called once. Preserve the existing single-call
expectation while asserting the final preload target.
🪄 Autofix
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 Plus
Run ID: 41039de6-b289-483c-8020-640e8219dd09
📒 Files selected for processing (2)
packages/vue-router/src/link.tsxpackages/vue-router/tests/link.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/vue-router/src/link.tsx
| to.value = '/about' | ||
| await Vue.nextTick() | ||
| await fireEvent.mouseEnter(link) | ||
| await vi.advanceTimersByTimeAsync(50) | ||
| expect(preloadRouteSpy).toHaveBeenCalledOnce() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bashset -e
printf'%s\n''--- candidate files ---'
git ls-files | rg '(^|/)(link\.test\.tsx|.*router.*|.*link.*\.(ts|tsx|js|jsx))$'| head -200
printf'%s\n''--- test lines 5560-5625 ---'
sed -n '5560,5625p' packages/vue-router/tests/link.test.tsx
printf'%s\n''--- preloadRouteSpy references ---'
rg -n -C 5 'preloadRouteSpy' packages/vue-router/tests packages/vue-router/src packages/vue-router 2>/dev/null | head -300Repository: TanStack/router
Length of output: 35260
🏁 Script executed:
#!/bin/bashset -e
printf'%s\n''--- dynamic test setup ---'
sed -n '5541,5608p' packages/vue-router/tests/link.test.tsx
printf'%s\n''--- preloadRoute definitions and calls ---'
rg -n -C 4 'preloadRoute\s*[=:]|\bpreloadRoute\s*\(' packages/router-core packages/vue-router packages/react-router packages/solid-router 2>/dev/null | head -400
printf'%s\n''--- files declaring Router.preloadRoute ---'
rg -l 'preloadRoute' packages --glob '*.{ts,tsx}'| head -100Repository: TanStack/router
Length of output: 36136
🏁 Script executed:
#!/bin/bashset -e
printf'%s\n''--- Vue Link intent preload implementation ---'
rg -n -C 12 'preloadRoute|preloadDelay|intent' packages/vue-router/src/link.tsx | head -300
printf'%s\n''--- Router preloadRoute implementation ---'
rg -n -C 15 'preloadRoute\s*\(' packages/router-core/src/router.ts | head -300
printf'%s\n''--- Link tests with argument assertions ---'
rg -n -C 4 'preloadRouteSpy.*(CalledWith|mock\.calls)|mock\.calls.*preloadRoute|toHaveBeenLastCalledWith|toHaveBeenCalledWith' packages/vue-router/tests/link.test.tsx packages/react-router/tests/link.test.tsx packages/solid-router/tests/link.test.tsx | head -300Repository: TanStack/router
Length of output: 27099
Assert the final preload target.
toHaveBeenCalledOnce() checks only the call count. Assert that preloadRouteSpy was called with { to: '/about' } so a stale preload cannot satisfy the test.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/vue-router/tests/link.test.tsx` around lines 5599 - 5604, Update the
preload assertion in the test around preloadRouteSpy to verify the call argument
includes { to: '/about' }, rather than checking only that it was called once.
Preserve the existing single-call expectation while asserting the final preload
target.
Uh oh!
There was an error while loading. Please reload this page.
The #8002 fix itself is merged; its changeset stays on main for the 1.x release and would otherwise trigger a stray vue-router/solid-router bump in the rc pre-release. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Summary
Root cause
Solid used the changing match ID as its implicit component key when no
remountDepsfunction was configured. Vue independently included serialized strict params in the child Match key. Both identities changed during param-only navigation and forced active route components to unmount and mount again, contrary to the documented default.Removing Vue's implicit param key exposed a separate assumption in Link: it copied declared props during setup and therefore relied on its parent remounting to receive param-derived values. The first correctness fix kept those props live with
proxyRefs({ ...toRefs(props), ...attrs }).That proxy was also the source of the broad Vue CPU regressions. Link has several location-driven computed values, and every navigation caused each Link to repeatedly cross the proxy and unwrap refs. Link-heavy scenarios amplified that fixed per-Link cost, which is why otherwise unrelated Vue navigation simulations moved together.
The follow-up keeps correctness with a computed plain snapshot. The snapshot rebuilds only when Link inputs change; location-only updates reuse the cached object.
Impact
Param-only navigation now preserves component instances and local state by default in Solid and Vue, avoiding unnecessary lifecycle work and garbage collection. Applications can continue to opt into remounting with
remountDeps.The focused Vue links benchmark recovered from 31.04 ms with the proxy/ref implementation to 17.15 ms (±1.13%). A controlled run with the old non-reactive plain spread was 17.46 ms, so the cached reactive version is back at the prior CPU cost while retaining correct updates in reused components.
Validation
CI=1 NX_DAEMON=false pnpm nx run @benchmarks/client-nav:test:perf:vue --outputStyle=stream --skipRemoteCache -- scenarios/links/vue/speed.bench.tsgit diff --checkSummary by CodeRabbit
Bug Fixes
Tests