From 1a6c496e30a5c54ce337d4ed20f36e83c6a63595 Mon Sep 17 00:00:00 2001 From: Raine Revere Date: Mon, 17 Aug 2026 12:14:15 -0700 Subject: [PATCH] Only adjust the drag hoverCount on an actual hover transition MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit useDragLeave shares a module-level hoverCount across every drop target, and debounces a clear of state.hoveringPath when it reaches zero. The effect treated every run as a hover transition: any run that was not a false→true change of isDeepHovering fell into the `else` branch and decremented the shared count. That branch is reached on mount, so any thought mounting mid-drag decremented the count — dropping it to zero and blanking the drop indicator while a target was still hovered. Its cleanup was commented "Cleanup on unmount" but listed four dependencies, so React ran it before every re-run rather than only on unmount. It also never decremented, so a drop target unmounted mid-drag leaked its count and hoveringPath was never cleared. Guard the count on `isDeepHovering !== isCountedRef.current` so only a real enter or leave adjusts it, and move the release into its own effect with empty deps, where it decrements this target's contribution. The empty deps are what make it an unmount handler. Adds hook tests for the two reproduced failures (an unrelated thought mounting mid-hover; a hovered target unmounting), a guard that the count still reaches zero on leave, and documents the counting rule in docs/drag-and-drop.md. Co-Authored-By: Claude Opus 5 (unknown context) --- docs/drag-and-drop.md | 2 + src/hooks/__tests__/useDragLeave.ts | 88 +++++++++++++++++++++++++++++ src/hooks/useDragLeave.ts | 58 +++++++++++-------- 3 files changed, 126 insertions(+), 22 deletions(-) create mode 100644 src/hooks/__tests__/useDragLeave.ts diff --git a/docs/drag-and-drop.md b/docs/drag-and-drop.md index 4b668c37564..cb8e31f61a8 100644 --- a/docs/drag-and-drop.md +++ b/docs/drag-and-drop.md @@ -135,6 +135,8 @@ When the press ends, `useLongPress` defers `onLongPressEnd` by 10 ms so that the [`useDragLeave`](../src/hooks/useDragLeave.ts) tracks how many drop targets are currently being deep-hovered (a module-level `hoverCount`). When the count drops to zero, it debounces a 50 ms clear of `state.hoveringPath`. This prevents flicker when the cursor briefly leaves one drop zone before entering an adjacent one. +Because `hoverCount` is shared across every drop target, only a change in `isDeepHovering` may adjust it. The hook's effect also re-runs on mount and when `canDropThought` or `hoverZone` change, and treating those as hover transitions would let a thought mounting mid-drag decrement the count to zero and blank the drop indicator while a target is still hovered. A separate unmount-only effect releases a target's contribution to the count, so a thought the layout unmounts mid-drag doesn't leak one. + ### `useDropHoverColor` [`useDropHoverColor`](../src/hooks/useDropHoverColor.ts) — small UI hook that maps the drop zone's depth to its hover color. Used by the various Drop* components. diff --git a/src/hooks/__tests__/useDragLeave.ts b/src/hooks/__tests__/useDragLeave.ts new file mode 100644 index 00000000000..cc1dc99a20b --- /dev/null +++ b/src/hooks/__tests__/useDragLeave.ts @@ -0,0 +1,88 @@ +import { renderHook } from '@testing-library/react' +import { act, createElement } from 'react' +import { Provider } from 'react-redux' +import { importTextActionCreator as importText } from '../../actions/importText' +import { updateHoveringPathActionCreator as updateHoveringPath } from '../../actions/updateHoveringPath' +import contextToPath from '../../selectors/contextToPath' +import store from '../../stores/app' +import initStore from '../../test-helpers/initStore' +import useDragLeave from '../useDragLeave' + +/** Renders useDragLeave against the app store. */ +const renderDragLeave = (props: { isDeepHovering: boolean; canDropThought: boolean }) => + renderHook((propsNew: { isDeepHovering: boolean; canDropThought: boolean }) => useDragLeave(propsNew), { + initialProps: props, + wrapper: ({ children }) => createElement(Provider, { store, children }), + }) + +/** Imports two thoughts and sets hoveringPath to the first, as if a drag were in progress over it. */ +const startHovering = () => { + store.dispatch(importText({ text: '- a\n- b' })) + store.dispatch(updateHoveringPath({ path: contextToPath(store.getState(), ['a'])! })) +} + +/** Advances past the hook's 50ms debounce. */ +const flushDebounce = () => act(() => vi.advanceTimersByTimeAsync(100)) + +beforeEach(() => { + initStore() + vi.useFakeTimers() +}) + +afterEach(() => { + vi.useRealTimers() +}) + +it('keeps hoveringPath while a drop target is hovered and an unrelated thought mounts', async () => { + startHovering() + + // the cursor enters a drop target + const target = renderDragLeave({ isDeepHovering: false, canDropThought: true }) + target.rerender({ isDeepHovering: true, canDropThought: true }) + + // an unrelated thought mounts while the drag is still over the target + renderDragLeave({ isDeepHovering: false, canDropThought: true }) + + await flushDebounce() + + expect(store.getState().hoveringPath).toBeDefined() +}) + +// Guards against over-correcting the hover count: ignoring mounts must not leave the count stuck above zero, or +// hoveringPath would never be cleared. +it('still clears hoveringPath after an unrelated thought mounts and the cursor leaves', async () => { + startHovering() + + const target = renderDragLeave({ isDeepHovering: false, canDropThought: true }) + target.rerender({ isDeepHovering: true, canDropThought: true }) + renderDragLeave({ isDeepHovering: false, canDropThought: true }) + target.rerender({ isDeepHovering: false, canDropThought: true }) + + await flushDebounce() + + expect(store.getState().hoveringPath).toBeUndefined() +}) + +it('clears hoveringPath once the cursor leaves the drop target', async () => { + startHovering() + + const target = renderDragLeave({ isDeepHovering: false, canDropThought: true }) + target.rerender({ isDeepHovering: true, canDropThought: true }) + target.rerender({ isDeepHovering: false, canDropThought: true }) + + await flushDebounce() + + expect(store.getState().hoveringPath).toBeUndefined() +}) + +it('clears hoveringPath when a hovered drop target unmounts', async () => { + startHovering() + + const target = renderDragLeave({ isDeepHovering: false, canDropThought: true }) + target.rerender({ isDeepHovering: true, canDropThought: true }) + target.unmount() + + await flushDebounce() + + expect(store.getState().hoveringPath).toBeUndefined() +}) diff --git a/src/hooks/useDragLeave.ts b/src/hooks/useDragLeave.ts index c8fd2964429..9600246f3a1 100644 --- a/src/hooks/useDragLeave.ts +++ b/src/hooks/useDragLeave.ts @@ -21,7 +21,9 @@ const clearHoveringPath: Thunk = (dispatch, getState) => { const useDragLeave = ({ isDeepHovering, canDropThought }: { isDeepHovering: boolean; canDropThought: boolean }) => { const dispatch = useDispatch() const hoverZone = useSelector(state => state.hoverZone) - const prevIsDeepHoveringRef = useRef(isDeepHovering) + // Whether this drop target is currently counted in hoverCount. Starts false so that mounting contributes nothing + // until the cursor actually enters. + const isCountedRef = useRef(false) const prevHoverZone = useRef(hoverZone) // Initialize the debounced function if it hasn't been already @@ -46,34 +48,46 @@ const useDragLeave = ({ isDeepHovering, canDropThought }: { isDeepHovering: bool return } - if (isDeepHovering && !prevIsDeepHoveringRef.current) { - // Cursor has entered a drop target, increase hover count - hoverCount += 1 - - // Cancel any pending debounce since we're over a drop target - debouncedSetHoveringPath?.cancel() - } else { - // Cursor has left a drop target, decrease hover count - hoverCount = Math.max(hoverCount - 1, 0) - if (hoverCount === 0) { - // No drop targets are being hovered over; start debounce - debouncedSetHoveringPath?.() - } - } - - prevIsDeepHoveringRef.current = isDeepHovering - prevHoverZone.current = hoverZone - - return () => { - // Cleanup on unmount + // Only a change in isDeepHovering means the cursor entered or left this drop target. The effect also re-runs when + // the component mounts and when canDropThought changes, and those must not touch the shared hoverCount: otherwise + // any thought mounting mid-drag decrements the count to zero and clears hoveringPath while a target is still + // hovered, dropping the drop indicator. + if (isDeepHovering !== isCountedRef.current) { + isCountedRef.current = isDeepHovering if (isDeepHovering) { + // Cursor has entered a drop target, increase hover count + hoverCount += 1 + + // Cancel any pending debounce since we're over a drop target + debouncedSetHoveringPath?.cancel() + } else { + // Cursor has left a drop target, decrease hover count + hoverCount = Math.max(hoverCount - 1, 0) if (hoverCount === 0) { - // Start debounce when unmounting and no more drop targets are hovered + // No drop targets are being hovered over; start debounce debouncedSetHoveringPath?.() } } } + + prevHoverZone.current = hoverZone }, [isDeepHovering, dispatch, hoverZone, canDropThought]) + + // Release this drop target's contribution to hoverCount when it unmounts mid-drag, e.g. when the layout unmounts a + // thought that the cursor is over. Empty deps are load-bearing: React runs an effect's cleanup before every re-run, + // not only on unmount, so this cannot be folded into the effect above. It closes over refs and module state only, + // so it never goes stale. + useEffect( + () => () => { + if (!isCountedRef.current) return + isCountedRef.current = false + hoverCount = Math.max(hoverCount - 1, 0) + if (hoverCount === 0) { + debouncedSetHoveringPath?.() + } + }, + [], + ) } export default useDragLeave