fix(hover): Defer pointerleave during press to prevent whileHover flash#3540
fix(hover): Defer pointerleave during press to prevent whileHover flash#3540mattgperry merged 2 commits intomainfrom
Conversation
Code ReviewI've thoroughly reviewed this PR and the changes look excellent. This is a well-thought-out solution that properly addresses the hover-during-press issue that the previous pointer capture approach missed. ✅ Strengths1. Correct Architecture
2. Comprehensive Coverage
3. Excellent Test Coverage
4. Clean Code
🔍 Potential Issues1. Event Listener Memory Leak Risk (Medium Priority) The // In hover.ts:84-95
const onPointerDown = () => {
isPressed = true
window.addEventListener("pointerup", onPointerUp as EventListener, eventOptions)
window.addEventListener("pointercancel", onPointerUp as EventListener, eventOptions)
}If the However, looking at the code more closely, I see that 2. Pointer Event Type Casting (Low Priority) In // hover.ts:67-81
const onPointerUp = (event: Event) => {
// ...
endHover(event as PointerEvent) // Type assertion here
}This is safe in practice since const onPointerUp = (event: PointerEvent) => {
// ... no casting needed
}3. State Reset on Hover Re-entry The code correctly resets // Suggested test case
test("hover state resets correctly on re-entry during press", async () => {
// pointerEnter -> pointerDown -> pointerLeave -> pointerEnter -> pointerUp
// Expected: hover should remain active since pointer is over element on release
})🎯 Performance Considerations
🔒 Security ConsiderationsNo security concerns identified. The code:
📝 Minor Suggestions1. Code Comment for Clarity The // hover.ts:98-107
const onPointerLeave = (leaveEvent: PointerEvent) => {
if (leaveEvent.pointerType === "touch") return
// Defer hover end during press (covers pre-drag window) or active drag
// to prevent whileHover from flashing off during interaction
if (isPressed || isDragActive()) {
deferredHoverEnd = true
return
}
endHover(leaveEvent)
}2. Test Comment Enhancement The test comment on line 330 mentions "needs movement threshold" which is accurate but could be slightly clearer about why this matters for the test scenario. 📊 Test Coverage AssessmentExcellent coverage with 37 passing hover + press tests. The three new tests specifically cover:
All critical edge cases are covered. ✅ VerdictThis is a high-quality PR that correctly solves a tricky UX issue. The approach is sound, test coverage is comprehensive, and the code is clean. Strengths:
Minor suggestions:
Recommendation: APPROVED ✅ The code quality is excellent and follows the repository's style conventions from CLAUDE.md. The solution is production-ready. 🤖 Generated with Claude Code |
…g before drag starts The previous pointer capture approach in the drag system didn't cover the window between pointerdown and drag start. Move the protection into hover() itself by tracking press state per-element and deferring pointerleave processing while the pointer is pressed. Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
d277fd4 to
4830aba
Compare
|
Code Review Summary: This PR correctly moves hover-during-press protection from drag into hover itself, fixing whileHover deactivation during the gap between pointerdown and drag start. Strengths: (1) Correct architecture - handles press-only, press+drag, and press+tap scenarios (2) Excellent test coverage for edge cases (3) Clean per-element state management (4) Proper event cleanup. Critical Issue: Memory leak risk - the cancel function does not clean up window-level pointerup/pointercancel listeners if hover is canceled while pointer is pressed (line 142). These listeners will remain attached. Recommend: Track cleanup functions and call them on cancel. Medium Priority: Multi-pointer support - current implementation does not track pointerId, so multiple simultaneous pointers could confuse the isPressed state. Low Priority: Type safety for onPointerUp parameter, changelog should mention behavioral fix not just types. Overall: Approve with fix for memory leak before merge. Great solution to a tricky interaction problem! |
Detailed Code SuggestionsIssue 1: Memory Leak Fix (Required)The returned cancel function needs to clean up window listeners. Here is the pattern to follow: export function hover(
elementOrSelector: ElementOrSelector,
onHoverStart: OnHoverStartEvent,
options: EventOptions = {}
): VoidFunction {
const [elements, eventOptions, cancel] = setupGesture(
elementOrSelector,
options
)
const cleanupFunctions: VoidFunction[] = []
elements.forEach((element) => {
let isPressed = false
let deferredHoverEnd = false
let hoverEndCallback: OnHoverEndEvent | undefined
const removeWindowListeners = () => {
window.removeEventListener(
"pointerup",
onPointerUp as EventListener
)
window.removeEventListener(
"pointercancel",
onPointerUp as EventListener
)
}
const onPointerUp = (event: Event) => {
isPressed = false
removeWindowListeners()
if (deferredHoverEnd) {
deferredHoverEnd = false
endHover(event as PointerEvent)
}
}
// ... rest of implementation
cleanupFunctions.push(removeWindowListeners)
})
return () => {
cancel()
cleanupFunctions.forEach(fn => fn())
}
}Issue 2: Multi-Pointer Support (Optional Enhancement)For handling multiple simultaneous pointers: const pressedPointers = new Set<number>()
const onPointerDown = (event: PointerEvent) => {
pressedPointers.add(event.pointerId)
if (pressedPointers.size === 1) {
window.addEventListener("pointerup", onPointerUp as EventListener, eventOptions)
window.addEventListener("pointercancel", onPointerUp as EventListener, eventOptions)
}
}
const onPointerUp = (event: Event) => {
const pointerEvent = event as PointerEvent
pressedPointers.delete(pointerEvent.pointerId)
if (pressedPointers.size === 0) {
removeWindowListeners()
if (deferredHoverEnd) {
deferredHoverEnd = false
endHover(pointerEvent)
}
}
}
const onPointerLeave = (leaveEvent: PointerEvent) => {
if (leaveEvent.pointerType === "touch") return
if (pressedPointers.size > 0) {
deferredHoverEnd = true
return
}
endHover(leaveEvent)
} |
Summary
pointerdownand drag start, causingwhileHoverto deactivate when the pointer moved outside the element during that gaphover()itself — tracks press state per-element and deferspointerleaveprocessing while the pointer is pressed, covering all press scenarios (press-only, press+drag, press+tap)Test plan
whileHoverstays active during press and deactivates on release outside elementwhileHoverstays active during press when pointer leaves before drag starts🤖 Generated with Claude Code