fix(popover): preserve wheel handler cancellation and ref cleanup

src/renderer/src/components/ui/popover-wheel-scroll.test.tsx

@@ -1,5 +1,12 @@
 // @vitest-environment happy-dom
-import { act } from 'react'
+import {
+  act,
+  createRef,
+  type Ref,
+  type RefCallback,
+  type WheelEvent as ReactWheelEvent,
+  type WheelEventHandler
+} from 'react'
 import { createRoot, type Root } from 'react-dom/client'
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
 import { Popover, PopoverContent, PopoverTrigger } from './popover'
@@ -21,7 +28,23 @@ function wheel(el: HTMLElement, deltaY: number): WheelEvent {
   return event
 }
 
-function renderPopover(contentClassName: string, nested: boolean): void {
+function renderPopover(
+  contentClassName: string,
+  nested: boolean,
+  {
+    onWheel,
+    onWheelCapture,
+    innerOnWheel,
+    contentRef,
+    portalContainer
+  }: {
+    onWheel?: WheelEventHandler<HTMLDivElement>
+    onWheelCapture?: WheelEventHandler<HTMLDivElement>
+    innerOnWheel?: WheelEventHandler<HTMLDivElement>
+    contentRef?: Ref<HTMLDivElement>
+    portalContainer?: HTMLElement
+  } = {}
+): void {
   container = document.createElement('div')
   document.body.appendChild(container)
   root = createRoot(container)
@@ -29,13 +52,23 @@ function renderPopover(contentClassName: string, nested: boolean): void {
     root!.render(
       <Popover open>
         <PopoverTrigger>open</PopoverTrigger>
-        <PopoverContent className={contentClassName}>
+        <PopoverContent
+          className={contentClassName}
+          onWheel={onWheel}
+          onWheelCapture={onWheelCapture}
+          ref={contentRef}
+          portalContainer={portalContainer}
+        >
           {nested ? (
             <div data-testid="viewport" style={{ overflowY: 'auto' }}>
-              <div data-testid="inner">tall</div>
+              <div data-testid="inner" onWheel={innerOnWheel}>
+                tall
+              </div>
             </div>
           ) : (
-            <div data-testid="inner">tall</div>
+            <div data-testid="inner" onWheel={innerOnWheel}>
+              tall
+            </div>
           )}
         </PopoverContent>
       </Popover>
@@ -147,4 +180,89 @@ describe('PopoverContent wheel shim', () => {
     expect(viewport.scrollTop).toBe(120)
     expect(event.defaultPrevented).toBe(true)
   })
+
+  it('lets a consumer prevent the shim scroll', () => {
+    const onWheel = vi.fn((event: ReactWheelEvent<HTMLDivElement>) => {
+      Object.defineProperty(event.nativeEvent, 'preventDefault', { value: vi.fn() })
+      event.preventDefault()
+    })
+    const portalContainer = document.createElement('div')
+    document.body.appendChild(portalContainer)
+    renderPopover('popover-wheel-scroll', true, { onWheel, portalContainer })
+    const viewport = document.querySelector<HTMLElement>('[data-testid="viewport"]')!
+    viewport.style.overflowY = 'auto'
+    makeScrollable(viewport, 1000, 400)
+
+    const event = wheel(document.querySelector<HTMLElement>('[data-testid="inner"]')!, 120)
+
+    expect(onWheel).toHaveBeenCalledOnce()
+    expect(event.defaultPrevented).toBe(false)
+    expect(viewport.scrollTop).toBe(0)
+  })
+
+  it('respects consumer capture cancellation', () => {
+    const onWheelCapture = vi.fn((event: ReactWheelEvent<HTMLDivElement>) => {
+      Object.defineProperty(event.nativeEvent, 'preventDefault', { value: vi.fn() })
+      event.preventDefault()
+    })
+    renderPopover('popover-wheel-scroll', true, { onWheelCapture })
+    const viewport = document.querySelector<HTMLElement>('[data-testid="viewport"]')!
+    viewport.style.overflowY = 'auto'
+    makeScrollable(viewport, 1000, 400)
+
+    const event = wheel(document.querySelector<HTMLElement>('[data-testid="inner"]')!, 120)
+
+    expect(onWheelCapture).toHaveBeenCalledOnce()
+    expect(event.defaultPrevented).toBe(false)
+    expect(viewport.scrollTop).toBe(0)
+  })
+
+  it('respects descendant cancellation', () => {
+    const innerOnWheel = vi.fn((event: ReactWheelEvent<HTMLDivElement>) => event.preventDefault())
+    renderPopover('popover-wheel-scroll', true, { innerOnWheel })
+    const viewport = document.querySelector<HTMLElement>('[data-testid="viewport"]')!
+    viewport.style.overflowY = 'auto'
+    makeScrollable(viewport, 1000, 400)
+
+    const event = wheel(document.querySelector<HTMLElement>('[data-testid="inner"]')!, 120)
+
+    expect(innerOnWheel).toHaveBeenCalledOnce()
+    expect(event.defaultPrevented).toBe(true)
+    expect(viewport.scrollTop).toBe(0)
+  })
+
+  it('preserves callback ref cleanup', () => {
+    const cleanup = vi.fn()
+    const contentRef: RefCallback<HTMLDivElement> = vi.fn(() => cleanup)
+    renderPopover('', false, { contentRef })
+
+    expect(contentRef).toHaveBeenCalledTimes(1)
+    expect(contentRef).toHaveBeenCalledWith(
+      document.querySelector<HTMLElement>('[data-slot="popover-content"]')
+    )
+
+    act(() => root!.unmount())
+    root = null
+    expect(cleanup).toHaveBeenCalledOnce()
+    expect(contentRef).toHaveBeenCalledTimes(1)
+  })
+
+  it('clears callback refs that do not return cleanup', () => {
+    const callbackRef = vi.fn((_node: HTMLDivElement | null): void => {})
+    renderPopover('', false, { contentRef: callbackRef })
+
+    act(() => root!.unmount())
+    root = null
+    expect(callbackRef).toHaveBeenLastCalledWith(null)
+  })
+
+  it('clears object refs on detach', () => {
+    const objectRef = createRef<HTMLDivElement>()
+    renderPopover('', false, { contentRef: objectRef })
+    expect(objectRef.current).toBeInstanceOf(HTMLDivElement)
+
+    act(() => root!.unmount())
+    root = null
+    expect(objectRef.current).toBeNull()
+  })
 })

src/renderer/src/components/ui/popover.tsx

@@ -5,6 +5,9 @@ import { Popover as PopoverPrimitive } from 'radix-ui'
 
 import { cn } from '@/lib/utils'
 
+// React delegates wheel passively, so native defaultPrevented may not reflect synthetic cancellation.
+const consumerPreventedWheelEvents = new WeakSet<WheelEvent>()
+
 function Popover(props: React.ComponentProps<typeof PopoverPrimitive.Root>) {
   return <PopoverPrimitive.Root data-slot="popover" {...props} />
 }
@@ -21,7 +24,10 @@ function PopoverAnchor(props: React.ComponentProps<typeof PopoverPrimitive.Ancho
  * Nearest scrollable element between the wheel target and the popover content,
  * inclusive of both. Returns null when nothing in that chain can scroll.
  */
-function resolvePopoverScroller(target: EventTarget | null, content: HTMLElement): HTMLElement | null {
+function resolvePopoverScroller(
+  target: EventTarget | null,
+  content: HTMLElement
+): HTMLElement | null {
   let node = target instanceof Node ? target : null
   while (node && node !== content.parentNode) {
     if (node instanceof HTMLElement && node.scrollHeight > node.clientHeight) {
@@ -35,6 +41,75 @@ function resolvePopoverScroller(target: EventTarget | null, content: HTMLElement
   return null
 }
 
+function handlePopoverWheel(event: WheelEvent, content: HTMLDivElement): void {
+  if (
+    event.defaultPrevented ||
+    consumerPreventedWheelEvents.has(event) ||
+    !(event.target instanceof Node) ||
+    !content.contains(event.target)
+  ) {
+    return
+  }
+
+  // Why two markers: `popover-scroll-content` also imposes a 15rem max-height and its
+  // own overflow, while `popover-wheel-scroll` opts into the shim alone.
+  if (
+    !content.classList.contains('popover-scroll-content') &&
+    !content.classList.contains('popover-wheel-scroll')
+  ) {
+    return
+  }
+
+  const el = resolvePopoverScroller(event.target, content)
+  if (!el) {
+    return
+  }
+
+  const delta =
+    event.deltaMode === WheelEvent.DOM_DELTA_LINE
+      ? event.deltaY * 16
+      : event.deltaMode === WheelEvent.DOM_DELTA_PAGE
+        ? event.deltaY * el.clientHeight
+        : event.deltaY
+  const maxScrollTop = el.scrollHeight - el.clientHeight
+  const nextScrollTop = Math.max(0, Math.min(maxScrollTop, el.scrollTop + delta))
+
+  // Why: Radix dialog scroll-lock swallows native wheel scrolling in portaled popovers.
+  if (nextScrollTop !== el.scrollTop) {
+    event.preventDefault()
+    el.scrollTop = nextScrollTop
+  }
+}
+
+type PopoverContentRef = React.ComponentProps<typeof PopoverPrimitive.Content>['ref']
+
+function attachPopoverContent(
+  content: HTMLDivElement,
+  portalContainer: HTMLElement | null | undefined,
+  forwardedRef: PopoverContentRef | undefined
+): () => void {
+  // React delegates portal events here first, preserving onWheel-before-shim ordering.
+  const wheelTarget = portalContainer ?? content.ownerDocument.body
+  const handleWheel = (event: WheelEvent): void => handlePopoverWheel(event, content)
+  wheelTarget.addEventListener('wheel', handleWheel, { passive: false })
+
+  const refCleanup = typeof forwardedRef === 'function' ? forwardedRef(content) : undefined
+  if (forwardedRef && typeof forwardedRef !== 'function') {
+    forwardedRef.current = content
+  }
+
+  return () => {
+    wheelTarget.removeEventListener('wheel', handleWheel)
+    if (typeof refCleanup === 'function') {
+      refCleanup()
+    } else if (typeof forwardedRef === 'function') {
+      forwardedRef(null)
+    } else if (forwardedRef) {
+      forwardedRef.current = null
+    }
+  }
+}
+
 function PopoverContent({
   className,
   align = 'center',
@@ -42,83 +117,46 @@ function PopoverContent({
   portalContainer,
   style,
   onWheel,
+  onWheelCapture,
   ref: forwardedRef,
   ...props
 }: React.ComponentProps<typeof PopoverPrimitive.Content> & {
   portalContainer?: HTMLElement | null
 }) {
-  const [content, setContent] = React.useState<HTMLDivElement | null>(null)
+  const handleConsumerWheel = React.useCallback(
+    (event: React.WheelEvent<HTMLDivElement>): void => {
+      onWheel?.(event)
+      if (event.defaultPrevented) {
+        consumerPreventedWheelEvents.add(event.nativeEvent)
+      }
+    },
+    [onWheel]
+  )
+  const handleConsumerWheelCapture = React.useCallback(
+    (event: React.WheelEvent<HTMLDivElement>): void => {
+      onWheelCapture?.(event)
+      if (event.defaultPrevented) {
+        consumerPreventedWheelEvents.add(event.nativeEvent)
+      }
+    },
+    [onWheelCapture]
+  )
 
   const setContentRef = React.useCallback(
     (node: HTMLDivElement | null) => {
-      setContent(node)
+      if (node) {
+        return attachPopoverContent(node, portalContainer, forwardedRef)
+      }
       if (typeof forwardedRef === 'function') {
-        const cleanup = forwardedRef(node)
-        if (node !== null && typeof cleanup === 'function') {
-          return () => {
-            setContent(null)
-            cleanup()
-          }
-        }
+        forwardedRef(null)
       } else if (forwardedRef) {
-        forwardedRef.current = node
+        forwardedRef.current = null
       }
       return undefined
     },
-    [forwardedRef]
+    [forwardedRef, portalContainer]
   )
 
-  React.useEffect(() => {
-    if (!content) {
-      return
-    }
-
-    const handleWheel = (event: WheelEvent): void => {
-      if (event.defaultPrevented) {
-        return
-      }
-
-      // Why two markers: `popover-scroll-content` also imposes a 15rem max-height and its
-      // own overflow, which a popover that manages its own layout (a fixed-height flex
-      // column over a pinned footer) must not inherit. `popover-wheel-scroll` opts into
-      // the shim alone.
-      if (
-        !content.classList.contains('popover-scroll-content') &&
-        !content.classList.contains('popover-wheel-scroll')
-      ) {
-        return
-      }
-
-      // Why resolve rather than use currentTarget: a popover whose content is a flex
-      // column with a nested viewport (a ScrollArea above a pinned footer) is itself
-      // overflow-hidden, so the scroller is a descendant under the pointer.
-      const el = resolvePopoverScroller(event.target, content)
-      if (!el) {
-        return
-      }
-
-      const delta =
-        event.deltaMode === WheelEvent.DOM_DELTA_LINE
-          ? event.deltaY * 16
-          : event.deltaMode === WheelEvent.DOM_DELTA_PAGE
-            ? event.deltaY * el.clientHeight
-            : event.deltaY
-      const maxScrollTop = el.scrollHeight - el.clientHeight
-      const nextScrollTop = Math.max(0, Math.min(maxScrollTop, el.scrollTop + delta))
-
-      // Why: issue drawers are Radix dialogs with scroll-lock. These popovers
-      // are portaled outside the dialog subtree, so native wheel scrolling is
-      // swallowed even though the scrollbar can be dragged.
-      if (nextScrollTop !== el.scrollTop) {
-        event.preventDefault()
-        el.scrollTop = nextScrollTop
-      }
-    }
-
-    content.addEventListener('wheel', handleWheel, { passive: false })
-    return () => content.removeEventListener('wheel', handleWheel)
-  }, [content])
-
   return (
     <PopoverPrimitive.Portal container={portalContainer ?? undefined}>
       <PopoverPrimitive.Content
@@ -143,7 +181,8 @@ function PopoverContent({
             WebkitAppRegion: 'no-drag'
           } as React.CSSProperties
         }
-        onWheel={onWheel}
+        onWheel={onWheel ? handleConsumerWheel : undefined}
+        onWheelCapture={onWheelCapture ? handleConsumerWheelCapture : undefined}
         {...props}
       />
     </PopoverPrimitive.Portal>