fix(ListboxVirtualizer): ignore non-element VNodes in slot children#2578
Conversation
When comment or text nodes are present in the virtualizer slot (e.g. `<!-- @vue-expect-error -->`), the first Fragment child may not be the actual component VNode. Filter by `typeof type !== 'symbol'` to skip Comment, Text, Static, and Fragment VNodes. Closes #2563 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughUpdated the virtualized slot's node selection logic to skip comment nodes and other non-element children, selecting the first valid child node instead of always using the first array element for virtualization metadata application. Changes
Estimated Code Review Effort🎯 1 (Trivial) | ⏱️ ~5 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)
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 |
commit: |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/Listbox/ListboxVirtualizer.vue (1)
85-91:⚠️ Potential issue | 🟡 MinorAdd a safe fallback when
.find()returns no matching child.Line 86 can return
undefinedwhen Fragment children are all symbol-typed nodes, which will breakcloneVNodeon line 91. Add a fallback to guarantee a valid VNode is always selected.Suggested fix
-import { cloneVNode, computed, Fragment, useSlots } from 'vue' +import { cloneVNode, computed, Fragment, isVNode, useSlots } from 'vue' @@ - const targetNode = defaultNode.type === Fragment && Array.isArray(defaultNode.children) - ? defaultNode.children.find(child => typeof (child as VNode).type !== 'symbol') as VNode - : defaultNode + const targetNode = defaultNode.type === Fragment && Array.isArray(defaultNode.children) + ? (defaultNode.children.find( + child => isVNode(child) && typeof child.type !== 'symbol', + ) as VNode | undefined) ?? defaultNode + : defaultNodeConsider adding a regression test for a Fragment slot containing only filtered nodes before the actual option node.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Listbox/ListboxVirtualizer.vue` around lines 85 - 91, The targetNode resolution can be undefined when Fragment children are all symbol-typed; update the logic that sets targetNode (based on defaultNode, Fragment and .find) to use a safe fallback (e.g., fall back to defaultNode or a minimal VNode) before calling cloneVNode, so cloneVNode always receives a valid VNode; update the code path that declares targetNode and ensure cloneVNode(targetNode, {...}) is guarded accordingly and add a regression test that renders a Fragment slot containing only filtered (symbol) nodes to verify the fallback behavior.
🧹 Nitpick comments (1)
packages/core/src/Listbox/ListboxVirtualizer.vue (1)
85-87: Consider extracting VNode-selection into a shared helper for virtualizers.
ListboxVirtualizernow has robust selection logic, whilepackages/core/src/Tree/TreeVirtualizer.vue:93-111still uses direct[0]access. A shared helper would keep behavior consistent and prevent divergence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/core/src/Listbox/ListboxVirtualizer.vue` around lines 85 - 87, Extract the VNode-selection logic currently in ListboxVirtualizer (the code that computes targetNode from defaultNode using the Fragment check and child.find where typeof child.type !== 'symbol') into a shared helper function (e.g., getPrimaryVNode or selectVisibleVNode) in a common utilities module used by virtualizers; then replace the inline logic in ListboxVirtualizer and change TreeVirtualizer (which currently uses direct [0] access) to call this helper so both use the same robust selection behavior and avoid divergence.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/core/src/Listbox/ListboxVirtualizer.vue`:
- Around line 85-91: The targetNode resolution can be undefined when Fragment
children are all symbol-typed; update the logic that sets targetNode (based on
defaultNode, Fragment and .find) to use a safe fallback (e.g., fall back to
defaultNode or a minimal VNode) before calling cloneVNode, so cloneVNode always
receives a valid VNode; update the code path that declares targetNode and ensure
cloneVNode(targetNode, {...}) is guarded accordingly and add a regression test
that renders a Fragment slot containing only filtered (symbol) nodes to verify
the fallback behavior.
---
Nitpick comments:
In `@packages/core/src/Listbox/ListboxVirtualizer.vue`:
- Around line 85-87: Extract the VNode-selection logic currently in
ListboxVirtualizer (the code that computes targetNode from defaultNode using the
Fragment check and child.find where typeof child.type !== 'symbol') into a
shared helper function (e.g., getPrimaryVNode or selectVisibleVNode) in a common
utilities module used by virtualizers; then replace the inline logic in
ListboxVirtualizer and change TreeVirtualizer (which currently uses direct [0]
access) to call this helper so both use the same robust selection behavior and
avoid divergence.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 82500c49-ab0f-4b82-a9ea-b85d60a3fc3a
📒 Files selected for processing (1)
packages/core/src/Listbox/ListboxVirtualizer.vue
Summary
ListboxVirtualizerslot rendering when comment or text nodes are present (e.g.<!-- @vue-expect-error -->)children[0]with a.find()that skips non-element VNodes (Comment,Text,Static,Fragment) by checkingtypeof type !== 'symbol'Closes #2563
Test plan
ComboboxVirtualizer/ListboxVirtualizerrenders correctly when slot contains comment nodes🤖 Generated with Claude Code
Summary by CodeRabbit