feat(hooks): useHoverToggle的入参refElement支持传入响应式数组 #6333
Conversation
broBinChen
requested review from
anncwb,
vince292007,
mynetfan and
jinmao88
as code owners
|
WalkthroughThe Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (8)
✨ Finishing Touches
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[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/effects/hooks/src/use-hover-toggle.ts (2)
11-16: Update documentation to reflect reactive array support.The JSDoc comment should be updated to mention that
refElementnow also accepts reactive references to arrays of elements./** * 监测鼠标是否在元素内部，如果在元素内部则返回 true，否则返回 false - * @param refElement 所有需要检测的元素。如果提供了一个数组，那么鼠标在任何一个元素内部都会返回 true + * @param refElement 所有需要检测的元素。支持单个元素、元素数组或响应式引用的元素数组。如果鼠标在任何一个元素内部都会返回 true * @param delay 延迟更新状态的时间 * @returns 返回一个数组，第一个元素是一个 ref，表示鼠标是否在元素内部，第二个元素是一个控制器，可以通过 enable 和 disable 方法来控制监听器的启用和禁用 */
47-47: Consider the performance impact of deep watching.The
deep: trueoption might cause unnecessary re-executions if array element references change without actual element changes. Sincerefsis already reactive, consider if deep watching is truly necessary.If the array elements themselves don't change internally (only the array composition changes), you could remove the deep option:
- watch(refs, updateHovers, { deep: true }); + watch(refs, updateHovers);Alternatively, if deep watching is necessary, consider using a more targeted approach to minimize performance impact.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/effects/hooks/src/use-hover-toggle.ts(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (2)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
🔇 Additional comments (2)
packages/effects/hooks/src/use-hover-toggle.ts (2)
17-20: Good type extension for reactive array support!The parameter type extension maintains backward compatibility while adding the requested reactive array functionality.
33-47:❓ Verification inconclusive
Critical: Memory leak - old hover listeners are not cleaned up.
When
updateHoversis called due torefschanges, newuseElementHoverinstances are created without cleaning up the old ones. This causes memory leaks as the old event listeners remain active.Each
useElementHovercall sets up event listeners that need to be properly disposed when no longer needed.Consider implementing a cleanup mechanism:
+ // Store cleanup functions + const cleanupFns = ref<Array<() => void>>([]); + // 更新 hover 监听的函数 function updateHovers() { + // Clean up old listeners + cleanupFns.value.forEach(fn => fn()); + cleanupFns.value = []; + isHovers.value = refs.value.map((refEle) => { const eleRef = computed(() => { const ele = unref(refEle); return ele instanceof Element ? ele : (ele?.$el as Element); }); - return useElementHover(eleRef); + const hoverRef = useElementHover(eleRef); + // Store cleanup function if the composable provides one + // Note: Check VueUse documentation for the actual cleanup mechanism + return hoverRef; }); } onUnmounted(() => { timer.value && clearTimeout(timer.value); + // Clean up all listeners on unmount + cleanupFns.value.forEach(fn => fn()); });Let me verify how VueUse handles cleanup for
useElementHover:
🏁 Script executed:
#!/bin/bash # Description: Check how useElementHover handles cleanup in VueUse # Search for useElementHover implementation to understand its cleanup mechanism fd -e ts -e js | xargs rg -A 10 "export.*function.*useElementHover" # Also check if it uses watchEffect or other Vue APIs that auto-cleanup fd -e ts -e js | xargs rg -A 5 "useElementHover.*watchEffect|useElementHover.*onScopeDispose"Length of output: 165
Critical: Memory leak in
updateHovers– old hover listeners are never disposedWhen
updateHoversruns (e.g. onrefschanges), it callsuseElementHoveragain without tearing down the previous instances. Because each call registers event listeners tied to the component’s effect scope, you’ll accumulate listeners until the component unmounts, leading to memory leaks and unexpected behavior.Proposed cleanup via isolated effect scopes:
• Add at the top of your hook:
const hoverScopes = ref<EffectScope[]>([])
• InupdateHovers(), before mapping new hovers:
– Stop and clear previous scopes:
hoverScopes.value.forEach(s => s.stop()); hoverScopes.value = []
• For each element:
–const scope = effectScope()
–const hoverRef = scope.run(() => useElementHover(eleRef))
–hoverScopes.value.push(scope)
– returnhoverRef
• InonUnmounted(), stop any remaining scopes:
hoverScopes.value.forEach(s => s.stop())Example diff:
+ import { ref, computed, watch, onUnmounted, unref, effectScope, EffectScope } from 'vue'; import { useElementHover } from '@vueuse/core'; + const hoverScopes = ref<EffectScope[]>([]); function updateHovers() { + // clean up old hover listeners + hoverScopes.value.forEach(scope => scope.stop()); + hoverScopes.value = []; isHovers.value = refs.value.map((refEle) => { + const scope = effectScope(); + const hoverRef = scope.run(() => { + const eleRef = computed(() => { + const ele = unref(refEle); + return ele instanceof Element ? ele : (ele?.$el as Element); + }); + return useElementHover(eleRef); + }); + hoverScopes.value.push(scope); + return hoverRef; }); } onUnmounted(() => { timer.value && clearTimeout(timer.value); + // ensure all listener scopes are stopped + hoverScopes.value.forEach(scope => scope.stop()); });Please verify whether
useElementHoveralready provides a built-in disposal (e.g. viaonScopeDispose) or if the above scope-based cleanup is required.
|
解决一下ai反馈的问题和冲突 |
…watch 监听深度，仅需浅层监听 refs 变化。 3、使用 effectScope 管理 useElementHover 实例，避免 refs 变化时事件监听器累积导致的内存泄漏问题
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
packages/effects/hooks/src/use-hover-toggle.ts (3)
53-53: Consider the ref structure complexity.While the nested ref structure (
ref<Array<Ref<boolean>>>) works functionally, it creates a complex reactive structure that might be harder to reason about. However, this appears necessary for the reactive behavior requirements.Consider adding a comment explaining why this nested ref structure is necessary for maintainability.
56-74: Enhance error handling in updateHovers function.The implementation correctly manages effect scopes and hover listeners, but could benefit from additional error handling for edge cases.
Consider adding error handling for potential issues:
function updateHovers() { // 停止并清理之前的作用域 hoverScopes.value.forEach((scope) => scope.stop()); hoverScopes.value = []; - isHovers.value = refs.value.map((refEle) => { + isHovers.value = refs.value.map((refEle) => { + if (!refEle) { + return ref(false); + } + const eleRef = computed(() => { const ele = unref(refEle); return ele instanceof Element ? ele : (ele?.$el as Element); }); // 为每个元素创建独立的作用域 const scope = effectScope(); const hoverRef = scope.run(() => useElementHover(eleRef)) || ref(false); hoverScopes.value.push(scope); return hoverRef; }); }
77-79: Optimize the watch configuration.The deep watch might be unnecessary and could impact performance. Consider if a shallow watch would suffice for your use case.
-watch(refs, updateHovers, { deep: 1 }); +watch(refs, updateHovers, { deep: false });Since
refsis a computed that returns a new array when dependencies change, a shallow watch should be sufficient. Test this change to ensure it still triggers appropriately for your use cases.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/effects/hooks/src/use-hover-toggle.ts(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (8)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Check (windows-latest)
- GitHub Check: Check (ubuntu-latest)
- GitHub Check: Lint (ubuntu-latest)
- GitHub Check: Test (windows-latest)
- GitHub Check: Lint (windows-latest)
- GitHub Check: post-update (ubuntu-latest)
- GitHub Check: post-update (windows-latest)
🔇 Additional comments (6)
packages/effects/hooks/src/use-hover-toggle.ts (6)
5-5: LGTM: Import addition is appropriate.The
effectScopeimport is correctly added to support the new reactive effect management functionality.
23-23: LGTM: Type signature and documentation updates are accurate.The function signature correctly extends support to
Ref<HTMLElement[] | null>and the JSDoc documentation accurately reflects the new reactive capabilities.Also applies to: 28-28
44-44: LGTM: Effect scopes tracking is properly initialized.Using a ref to store effect scopes is appropriate for tracking and cleanup purposes.
46-51: LGTM: Input normalization is well-implemented.The computed property properly handles the different input types (null, single element, array) and provides reactive unwrapping of the
refElementparameter.
81-81: LGTM: Computed property correctly uses reactive array.The
isOutsideAllcomputed property properly referencesisHovers.valueto maintain reactivity.
145-146: LGTM: Proper cleanup on unmount.The cleanup logic correctly stops all effect scopes to prevent memory leaks.
…化而不是整个数组变化，避免过度依赖收集
|
@coderabbitai I pushed a fix in commit 62726a0, please review it. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 69711 🏁 Script executed: Length of output: 4630 @broBinChen Great work on the fixes in commit 62726a0! I've reviewed the changes and they look excellent. Here's what I found: ✅ Improvements MadePerformance Optimization:
Enhanced Boundary Handling:
Code Quality:
✅ Issues Resolved
📝 Note on ChangesetThe changeset-bot mentioned no changeset was found. This might need to be added by a maintainer if this change should result in a version bump for the The optimization you've implemented (只监听元素数量变化而不是整个数组变化，避免过度依赖收集) is exactly the right approach for this use case. Well done! 🎉 🐇✨ The rabbit hops with joy seeing such clean, optimized code! If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit! |
Description
在使用 useHoverToggle hook 监听一组 DOM 元素的 hover 事件时，我遇到了一个问题：这些元素在运行时会动态更新，但新增的元素缺失了 hover 监听器，导致行为不一致。
PixPin_2025-06-04_16-18-04_before.mp4
解决方案
本PR通过为 useHoverToggle hook 增加对 refElement 参数的响应式支持来解决这个问题。主要改动包括：
PixPin_2025-06-04_16-18-50_after.mp4
Type of change
Please delete options that are not relevant.
Checklist
pnpm run docs:devcommand.pnpm test.feat:,fix:,perf:,docs:, orchore:.Summary by CodeRabbit