Conversation
…t` to `SelectItemIndicator`
Contributor
📝 WalkthroughWalkthroughExpose selection-state slot props across multiple item components and make item indicator components optionally force-mounted with ref forwarding; several components now declare typed/default slot signatures and forward underlying scoped slot props to their own default slots. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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: |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
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/core/src/Select/SelectItem.vue`:
- Line 180: The slot in SelectItem.vue is passing the ComputedRef isSelected
instead of the raw boolean, so update the template to pass the unwrapped value
(use isSelected.value) to satisfy the slot type contract; locate the <slot
:selected="isSelected" /> usage in SelectItem.vue and replace it so consumers
receive a plain boolean, and audit similar patterns in Radio.vue and
TooltipRoot.vue where slot props are declared as unwrapped values but the Ref is
passed—change those to pass .value as well.
🪄 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: 2db09e47-5527-4261-8bc2-81f8365c7af1
📒 Files selected for processing (2)
packages/core/src/Select/SelectItem.vuepackages/core/src/Select/SelectItemIndicator.vue
Contributor
|
@kricsleo thank you 💙 do you think we can apply this to combobox too (and any other select-like components I might have missed where such a feature makes sense)? |
Contributor
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/core/src/Listbox/ListboxItemIndicator.vue`:
- Line 28: The Presence prop binding mixes forced and state-driven mounting;
change the Presence usage in ListboxItemIndicator.vue to pass present and
forceMount separately by setting :present="itemContext.isSelected.value" and
:forceMount="forceMount" so Presence can correctly handle transitions; locate
the Presence element (currently using :present="forceMount ||
itemContext.isSelected.value") and replace the expression with the two props,
and apply the same pattern to other components using the same combined
expression (e.g., Toast, Tooltip, Tabs, RadioGroup, Popover, Menu, HoverCard,
Dialog, Select, Checkbox, Collapsible, Combobox).
🪄 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: c446a63b-d950-423f-8e1b-56898d052469
📒 Files selected for processing (11)
packages/core/src/ColorSwatchPicker/ColorSwatchPickerItem.vuepackages/core/src/Combobox/ComboboxItem.vuepackages/core/src/ContextMenu/ContextMenuCheckboxItem.vuepackages/core/src/ContextMenu/ContextMenuRadioItem.vuepackages/core/src/DropdownMenu/DropdownMenuCheckboxItem.vuepackages/core/src/DropdownMenu/DropdownMenuRadioItem.vuepackages/core/src/Listbox/ListboxItem.vuepackages/core/src/Listbox/ListboxItemIndicator.vuepackages/core/src/Menu/MenuRadioItem.vuepackages/core/src/Menubar/MenubarCheckboxItem.vuepackages/core/src/Menubar/MenubarRadioItem.vue
✅ Files skipped from review due to trivial changes (1)
- packages/core/src/Menubar/MenubarCheckboxItem.vue
| > | ||
| <slot /> | ||
| </Primitive> | ||
| <Presence :present="forceMount || itemContext.isSelected.value"> |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Confirm Presence accepts separate 'present' and 'forceMount' props:"
sed -n '1,120p' packages/core/src/Presence/Presence.ts
echo
echo "2) Find indicator usages that fold forceMount into :present:"
rg -n --type=vue ':present="[^"]*forceMount[^"]*"' packages/core/src
echo
echo "3) Find usages that pass :force-mount explicitly:"
rg -n --type=vue ':force-mount=' packages/core/srcRepository: unovue/reka-ui
Length of output: 3102
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "1) Read ListboxItemIndicator.vue to verify line 28:"
cat -n packages/core/src/Listbox/ListboxItemIndicator.vue | sed -n '20,35p'
echo
echo "2) Search for :present with forceMount pattern (without file type restriction):"
rg -n ':present="[^"]*forceMount' packages/core/src --type-add 'vue:*.vue'
echo
echo "3) Check if forceMount prop is available in ListboxItemIndicator:"
rg -n 'forceMount' packages/core/src/Listbox/ListboxItemIndicator.vue
echo
echo "4) Check other :present usages in Listbox components:"
rg -n ':present=' packages/core/src/Listbox/ --type-add 'vue:*.vue'Repository: unovue/reka-ui
Length of output: 3353
Pass forceMount to Presence as a separate prop.
The Presence component explicitly supports separate present and forceMount props to distinguish between conditional rendering (state-based) and forced rendering (programmatic control). Combining them into a single :present="forceMount || itemContext.isSelected.value" masks this distinction and prevents Presence from handling animations/transitions correctly.
Proposed fix
- <Presence :present="forceMount || itemContext.isSelected.value">
+ <Presence
+ :present="itemContext.isSelected.value"
+ :force-mount="forceMount"
+ >Note: This pattern appears across 23+ components in the codebase (Toast, Tooltip, Tabs, ScrollArea, RadioGroup, Popover, Menu, NavigationMenu, HoverCard, Dialog, Select, Checkbox, Collapsible, Combobox, etc.). Consider applying this fix systematically across all affected components.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Presence :present="forceMount || itemContext.isSelected.value"> | |
| <Presence | |
| :present="itemContext.isSelected.value" | |
| :force-mount="forceMount" | |
| > |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/core/src/Listbox/ListboxItemIndicator.vue` at line 28, The Presence
prop binding mixes forced and state-driven mounting; change the Presence usage
in ListboxItemIndicator.vue to pass present and forceMount separately by setting
:present="itemContext.isSelected.value" and :forceMount="forceMount" so Presence
can correctly handle transitions; locate the Presence element (currently using
:present="forceMount || itemContext.isSelected.value") and replace the
expression with the two props, and apply the same pattern to other components
using the same combined expression (e.g., Toast, Tooltip, Tabs, RadioGroup,
Popover, Menu, HoverCard, Dialog, Select, Checkbox, Collapsible, Combobox).
kricsleo
changed the title
selected slot prop to SelectItem and forceMount to SelectItemIndicator*Item and forceMount to *ItemIndicator
Collaborator
Author
|
Added relevant features to more components. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🔗 Linked issue
resolves #2547
❓ Type of change
📚 Description
📸 Screenshots (if appropriate)
📝 Checklist
Summary by CodeRabbit