fix(Link): ensure single-root rendering for v-show and $el resolution#6310
fix(Link): ensure single-root rendering for v-show and $el resolution#6310benjamincanac merged 1 commit intov4from
v-show and $el resolution#6310Conversation
📝 WalkthroughWalkthroughThe Link component logic was changed to explicitly determine internal vs external links via a new Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
|
commit: |
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
test/components/Link.spec.ts (1)
7-26: Consider adding tests forv-showand template ref behavior.The PR's primary objective is fixing
v-showdirectives and$elresolution. The current tests verify rendering but don't explicitly test these behaviors. Consider adding integration tests that:
- Apply
v-show="false"and verify the element is hidden (display: none)- Use a template ref and verify
$elresolves to the DOM element💡 Example test for v-show behavior
it('supports v-show directive', async () => { const wrapper = await mountSuspended({ components: { Link }, template: '<Link v-show="visible" to="/" custom v-slot="props"><a v-bind="props">Test</a></Link>', data: () => ({ visible: false }) }) expect(wrapper.find('a').isVisible()).toBe(false) })Would you like me to help generate more comprehensive tests for the
v-showand template ref scenarios that this PR addresses?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/components/Link.spec.ts` around lines 7 - 26, Add integration tests to cover v-show and template ref/$el resolution for the Link component: create one test that mounts Link via mountSuspended (or the existing renderEach pattern) with v-show="visible" (initially false) and a slot that renders an <a> element bound to slot props, then assert the rendered anchor has display: none or isVisible() === false; create another test that renders Link using a template ref (e.g., ref="linkRef") and after mount assert that wrapper.vm.$refs.linkRef.$el (or the ref value returned) resolves to the actual DOM element for the underlying anchor or root element; locate tests near other Link specs using renderEach and name them like "supports v-show directive" and "resolves template ref to DOM element" to keep coverage for the v-show/$el fixes.src/runtime/components/Link.vue (1)
207-243: Consider addingdata-slotattributes for slot identification.Per the coding guidelines for
src/runtime/components/**/*.vue, template elements should havedata-slot="name"attributes for slot identification. TheSlotandULinkBaseelements in this file don't have these attributes.💡 Example addition
<NuxtLink v-if="isInternalLink" v-slot="{ href, navigate, route: linkRoute, isActive, isExactActive, ...rest }" v-bind="nuxtLinkProps" :to="to" custom> - <Slot v-if="custom"> + <Slot v-if="custom" data-slot="link"> <slot ... /> </Slot> <ULinkBase v-else + data-slot="link" ... >As per coding guidelines: "Add
data-slot="name"attributes on all template elements for slot identification"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/runtime/components/Link.vue` around lines 207 - 243, The Slot and ULinkBase template elements inside the NuxtLink custom rendering need data-slot attributes for slot identification; add data-slot="custom" to the <Slot> element used when custom is true and add data-slot="default" to the <ULinkBase> element used for the non-custom rendering, keeping the rest of the v-bind and props unchanged (locate the <Slot> and <ULinkBase> usages within the NuxtLink block to apply).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/runtime/vue/overrides/vue-router/Link.vue`:
- Around line 222-236: The custom external Slot branch in Link.vue currently
doesn't provide a navigate handler; update the Slot v-else-if="custom" block to
include a navigate function (matching the behavior used in the "none" override)
in the v-bind object so custom slots receive navigate for external links;
implement navigate to call the same external navigation logic used elsewhere in
this component (respecting isExternal, href/to, rel, target and disabled) and
pass it as navigate alongside active, isExternal, href, etc., so custom-injected
handlers can trigger navigation consistently.
---
Nitpick comments:
In `@src/runtime/components/Link.vue`:
- Around line 207-243: The Slot and ULinkBase template elements inside the
NuxtLink custom rendering need data-slot attributes for slot identification; add
data-slot="custom" to the <Slot> element used when custom is true and add
data-slot="default" to the <ULinkBase> element used for the non-custom
rendering, keeping the rest of the v-bind and props unchanged (locate the <Slot>
and <ULinkBase> usages within the NuxtLink block to apply).
In `@test/components/Link.spec.ts`:
- Around line 7-26: Add integration tests to cover v-show and template ref/$el
resolution for the Link component: create one test that mounts Link via
mountSuspended (or the existing renderEach pattern) with v-show="visible"
(initially false) and a slot that renders an <a> element bound to slot props,
then assert the rendered anchor has display: none or isVisible() === false;
create another test that renders Link using a template ref (e.g., ref="linkRef")
and after mount assert that wrapper.vm.$refs.linkRef.$el (or the ref value
returned) resolves to the actual DOM element for the underlying anchor or root
element; locate tests near other Link specs using renderEach and name them like
"supports v-show directive" and "resolves template ref to DOM element" to keep
coverage for the v-show/$el fixes.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: aeaf6b35-1e1c-454e-921e-4583df1e0905
⛔ Files ignored due to path filters (2)
test/components/__snapshots__/Link-vue.spec.ts.snapis excluded by!**/*.snaptest/components/__snapshots__/Link.spec.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (5)
src/runtime/components/Link.vuesrc/runtime/vue/overrides/inertia/Link.vuesrc/runtime/vue/overrides/none/Link.vuesrc/runtime/vue/overrides/vue-router/Link.vuetest/components/Link.spec.ts
…l` resolution (nuxt#6310)" This reverts commit 20804d103c9d803137149ef1be312f9c3a29ecc9.
…l` resolution (nuxt#6310)" This reverts commit 54af6ebdf61aa66ef3480ddb07d0c9fe1ad6935a.
…l` resolution (nuxt#6310)" This reverts commit 20804d103c9d803137149ef1be312f9c3a29ecc9.
…l` resolution (nuxt#6310)" This reverts commit 54af6ebdf61aa66ef3480ddb07d0c9fe1ad6935a.
🔗 Linked issue
Resolves #5987, resolves #5108, resolves #4154
❓ Type of change
📚 Description
When
customis true,<template>wrappers around<slot>and NuxtLink's directslots.default()return produce Fragment root nodes. Vue cannot apply runtime directives (v-show) or resolve$elthrough Fragments, causing #5987 and #5108.Fix:
<template v-if="custom">with<Slot v-if="custom">(from Reka UI) in all 4 Link components to flatten slot Fragments into single elements.isInternalLinkcomputed) since NuxtLink's external+custom path always returns a Fragment. External link props (href,rel,target) are computed directly.!to) now skip NuxtLink entirely, removing unnecessary route tracking that caused allUButtoninstances to re-render on route changes (Performance Issue Rendering Large List of UButtons – flushJobs Extremely Slow #4154).📝 Checklist