refactor: simplify slot handling in base-popper component#128
refactor: simplify slot handling in base-popper component#128hexqi merged 2 commits intoopentiny:release/v0.2.xfrom
Conversation
- Removed unnecessary complexity in trigger VNode management by introducing a new `useSlotRefs` composable. - Updated the base-popper component to utilize the new composable for better readability and maintainability. - Enhanced the handling of trigger VNodes and their references, improving overall performance and clarity.
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughA new composable, Changes
Poem
✨ Finishing Touches🧪 Generate Unit Tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
packages/components/src/shared/composables/useSlotRefs.ts (2)
44-50: Consider usingimmediate: truefor the length watcher.The watcher adjusts the refs array length when the computed length changes. Using
flush: 'post'is good, but you might wantimmediate: trueto ensure the initial length is set correctly.watch( length, (len) => { refs.value.length = len }, - { flush: 'post' }, + { flush: 'post', immediate: true }, )
5-5: Add JSDoc documentation for the composable.The composable would benefit from comprehensive documentation explaining its purpose, parameters, and return values.
Add documentation:
+/** + * Composable for managing references to Vue slot VNodes + * @param slot - Optional slot function that returns VNodes + * @param renderAll - Whether to render all VNodes (true) or just the first one (false) + * @returns Object containing vnodes, refs, and helper functions + */ export function useSlotRefs(slot?: (...args: any[]) => VNode[], renderAll?: boolean) {
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/components/src/base-popper/index.vue(5 hunks)packages/components/src/shared/composables/index.ts(1 hunks)packages/components/src/shared/composables/useSlotRefs.ts(1 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: gene9831
PR: opentiny/tiny-robot#59
File: packages/components/src/suggestion-popover/index.vue:131-133
Timestamp: 2025-05-27T03:35:11.008Z
Learning: In the SuggestionPopover component (packages/components/src/suggestion-popover/index.vue), the click handler can be bound unconditionally because the `show` computed property has a custom setter that prevents state mutations when `props.trigger === 'manual'`. This design centralizes trigger mode logic in the computed property rather than requiring conditional checks in event handlers.
packages/components/src/base-popper/index.vue (3)
Learnt from: gene9831
PR: opentiny/tiny-robot#59
File: packages/components/src/suggestion-popover/index.vue:131-133
Timestamp: 2025-05-27T03:35:11.008Z
Learning: In the SuggestionPopover component (packages/components/src/suggestion-popover/index.vue), the click handler can be bound unconditionally because the `show` computed property has a custom setter that prevents state mutations when `props.trigger === 'manual'`. This design centralizes trigger mode logic in the computed property rather than requiring conditional checks in event handlers.
Learnt from: gene9831
PR: opentiny/tiny-robot#59
File: packages/components/src/suggestion-popover/index.vue:0-0
Timestamp: 2025-05-27T03:45:56.392Z
Learning: In Vue components using VueUse's onClickOutside, calling stopPropagation() in the onClickOutside handler can prevent subsequent click event handlers on the same element from being triggered. This means that clicking a trigger element while a popover is open can successfully close the popover without immediately reopening it, even when the onClickOutside only listens to the popover element and not the trigger element.
Learnt from: gene9831
PR: opentiny/tiny-robot#123
File: packages/components/src/bubble/message/Message.vue:22-40
Timestamp: 2025-06-25T07:04:18.791Z
Learning: In the Message component (packages/components/src/bubble/message/Message.vue), the renderer resolution is intentionally not reactive to type changes - the component is designed to work with a fixed type that doesn't change after initialization.
🔇 Additional comments (6)
packages/components/src/shared/composables/index.ts (1)
1-1: LGTM! Clean export addition.The export follows the existing pattern and properly exposes the new composable.
packages/components/src/base-popper/index.vue (5)
34-39: LGTM! Clean integration of the useSlotRefs composable.The destructuring assignment properly extracts all needed values from the composable, and the parameter passing for
renderAllTriggersis correct.
53-55: Good refactoring of event handler mapping.The event handler mapping now uses the composable's
triggerVNodesoutput, which is cleaner than the previous manual VNode management.
130-135: Improved template with safer key bindings.The template now uses
isVNode(vnode) ? vnode.key : indexfor safer key binding, which handles cases where the VNode might not have a key property.
113-117: Consider the impact of removing flush timing on the watcher.The watcher for
props.showhad its flush timing changed. This could affect when theupdate()function is called relative to DOM updates.Verify that the
update()timing still works correctly with the new flush behavior:#!/bin/bash # Search for other similar watchers in the codebase to understand the pattern rg -A 5 -B 2 "flush.*post.*show" packages/components/src/
4-5: I’ll search across the Vue package to locateisVNodein its type definitions:#!/bin/bash # Search for any export or definition of isVNode in Vue's shipped types rg "export.*isVNode" -n node_modules/vue rg "isVNode" -n node_modules/vue
- Simplified the logic for handling Fragment nodes in the `useSlotRefs` composable to return children of the first Fragment found in the VNode array. - Enhanced the type checking in the `resolveRef` function to ensure proper handling of elements and Vue component instances, improving error messaging for invalid inputs.
3 participants
useSlotRefscomposable.Summary by CodeRabbit
New Features
Refactor
Chores