From 465c9928961fdb4d348b5eb4c03870ae614c33a1 Mon Sep 17 00:00:00 2001 From: karunkop Date: Tue, 27 Oct 2026 16:02:03 +0545 Subject: [PATCH] test(dnd): dragAndDropThought waits for its drop target's hover dragAndDropThought waited only for the drag alert, which a held drag that moves on to a second drop target has already shown, so a snapshot could be taken before the second drop hover rendered (#5879). Tests should not be responsible for drag-and-drop timing, so the helper now waits for the hover itself. - Each drop target element (thought rows, DropChild, DropEnd, DropUncle) renders data-drop-over, react-dnd's isOver({ shallow: true }). It names the target a drop lands on, is set in the same commit as the target's isHovering, and is true even on a target that rejects the drop. - Before its final move, the helper records when the drop target under the pointer renders that hover, and waits for the record once the drag is in progress. It records rather than polls because a Puppeteer drag dispatches dragover only when the pointer moves, so a target that moves under the stationary pointer, e.g. after hover expansion, is never hovered. It throws if the target is never hovered. - The own-position test no longer waits on .drop-hover itself. - Docs: drag-and-drop.md describes data-drop-over and corrects DropUncle's zone; testing.md lists the helper's window global. Fixes #5879 Claude-Session: https://claude.ai/code/session_01Fd4vvqGUWV42TDr5CEEzGf Co-Authored-By: Claude claude-opus-5-5 (xhigh) --- docs/drag-and-drop.md | 4 +- docs/testing.md | 3 +- src/components/DropChild.tsx | 3 +- src/components/DropEnd.tsx | 3 +- src/components/DropUncle.tsx | 3 +- src/components/Thought.tsx | 2 + src/e2e/puppeteer/__tests__/drag-and-drop.ts | 5 - .../puppeteer/helpers/dragAndDropThought.ts | 92 +++++++++++++++++-- src/hooks/useDragAndDropSubThought.tsx | 8 +- src/hooks/useDragAndDropThought.tsx | 5 +- 10 files changed, 107 insertions(+), 21 deletions(-) diff --git a/docs/drag-and-drop.md b/docs/drag-and-drop.md index e2cc31eb0a3..b96d880e0ac 100644 --- a/docs/drag-and-drop.md +++ b/docs/drag-and-drop.md @@ -68,6 +68,8 @@ Three companion fields ride alongside it: These are what `DropHover` and other UI components subscribe to. +Each drop target element (the thought rows, `DropChild`, `DropEnd` and `DropUncle`) renders `data-drop-over`: react-dnd's `isOver({ shallow: true })` for that target, which is true even on a target that rejects the drop. It names the target a drop lands on, and it is set in the same commit as the target's `isHovering`, from which `DropChild`, `DropEnd` and `DropUncle` draw their drop hover. A thought row's drop hover also needs `state.hoveringPath`, which the same hover writes first, but only when the pointer has moved (`throttleByMousePosition`). react-dnd itself moves its hover only on a `dragover`, and a drag driven by Puppeteer dispatches `dragover` only when the pointer moves. So when a hover changes the layout under a stationary pointer, e.g. by expanding a thought, the drop target that ends up under the pointer is not hovered until the pointer moves again. The Puppeteer helper [`dragAndDropThought`](../src/e2e/puppeteer/helpers/dragAndDropThought.ts) therefore records `data-drop-over` on the drop target under the pointer as it is rendered, and waits for that record before it holds or drops. + ### Drag sources There are four drag sources in the app, each implemented as a hook: @@ -94,7 +96,7 @@ The drop targets in the UI are: - [`DropChild`](../src/components/DropChild.tsx) — a `SubthoughtsDrop` zone rendered inside a thought when it's empty or collapsed. - [`DropEnd`](../src/components/DropEnd.tsx) — a `SubthoughtsDrop` zone at the end of a subthought list. - [`DropCliff`](../src/components/DropCliff.tsx) — renders one `DropEnd` per cliff level when depth decreases by more than 1, so a thought can be dropped at any of the intermediate levels exposed by the cliff. -- [`DropUncle`](../src/components/DropUncle.tsx) — a `ThoughtDrop` zone for inserting before the next hidden uncle, when the immediate parent is hidden by autofocus. +- [`DropUncle`](../src/components/DropUncle.tsx) — a `SubthoughtsDrop` zone for inserting before the next hidden uncle, when the immediate parent is hidden by autofocus. - [`Favorites`](../src/components/Favorites.tsx) — its own zone, handled by `useDragDropFavorites`. - [`DropGutter`](../src/components/DropGutter.tsx) — the right-edge invisible quick-delete panel; see [Quick drops](#quick-drops). diff --git a/docs/testing.md b/docs/testing.md index c8976874ce2..67fade7c95d 100644 --- a/docs/testing.md +++ b/docs/testing.md @@ -637,6 +637,7 @@ Integration tests are blackbox, but named helpers may take shortcuts during arra | Text paint assertion | Assert | [`getThoughtPixels`](../src/e2e/puppeteer/helpers/getThoughtPixels.ts) | Read pixels within a thought's text bounds through the shared stabilized screenshot helper and an offscreen canvas. Does not change the thought or app state. | | Structural assertion | Assert | [`exportThoughts`](../src/e2e/puppeteer/helpers/exportThoughts.ts) | Export the thought tree as plaintext. Do not make additional assertions on Redux state. | | Non-visual synchronization | Wait | [`waitForThoughtspaceIdle`](../src/e2e/puppeteer/helpers/waitForThoughtspaceIdle.ts) | Use only when persistence or another prerequisite has no immediate visual signal. `waitForThoughtspaceIdle` waits for every queued persistence write to commit and rejects if one failed; call it only to synchronize before a reload or before asserting that a write landed. `refresh` waits for persisted state to load after a reload, but shared setup and `resetApp` deliberately do not wait for database initialization. Assert the final user-visible result separately. | +| Drag-and-drop synchronization | Wait | [`dragAndDropThought`](../src/e2e/puppeteer/helpers/dragAndDropThought.ts) | Waits until react-dnd has hovered the drop target under the pointer, read from its `data-drop-over` attribute ([drag-and-drop.md](drag-and-drop.md#state-machine-statelongpress)). For the length of the move it keeps a `MutationObserver` on `window.__dropTargetHoverWatch`, since a Puppeteer drag dispatches `dragover` only on pointer moves, and a hover can change the layout under the stationary pointer, e.g. by hover expansion, before a poll would see it. Tests must not wait on drop hovers themselves. | | Flake diagnosis | Arrange | [`withDebugLog`](../src/e2e/puppeteer/helpers/withDebugLog.ts) | Wrap only a test under investigation for an intermittent failure. It records the [Debug Log](debug-log.md) and appends it to the error on failure, where the Puppeteer Flaky workflow's failure report carries it. Logging adds a little synchronous work to every action and input event, so it can make a race rarer; remove the wrapper once the flake is fixed. | DOM reads are different from backdoors: inline `page.evaluate`/`browser.execute` may read user-visible DOM when no helper exists, though a repeated read should become a named helper. It may not dispatch actions, mutate app state, set test flags, or write to the DOM. @@ -703,7 +704,7 @@ Puppeteer input is coordinated through the helpers in [`../src/e2e/puppeteer/hel | Press a key or shortcut | [`press`](../src/e2e/puppeteer/helpers/press.ts) | Presses a key with optional `alt`, `ctrl`, `meta`, and `shift` modifiers. | | Swipe/command gesture | [`gesture`](../src/e2e/puppeteer/helpers/gesture.ts) | Emits `touchStart`, stepped `touchMove` events, and `touchEnd` for the supplied direction path or command gesture. | | Long press | [`longPressThought`](../src/e2e/puppeteer/helpers/longPressThought.ts), [`longPressBullet`](../src/e2e/puppeteer/helpers/longPressBullet.ts), [`longPressWord`](../src/e2e/puppeteer/helpers/longPressWord.ts) | Holds a touch on the thought, or the mouse on its bullet, until the bullet reports the long-press highlight, then releases. Long press is bound to the bullet on every platform but to the thought itself only on touch, so `longPressBullet` is how a thought is long pressed on desktop, without emulating a device. `longPressWord` holds a touch on a word until the highlight, adds Chrome for Android's native long press response (it selects the word, then dispatches a touch `contextmenu`), and leaves the finger down for the test to move or lift. | -| Drag and drop | [`dragAndDropThought`](../src/e2e/puppeteer/helpers/dragAndDropThought.ts), [`dragAndDropFavorite`](../src/e2e/puppeteer/helpers/dragAndDropFavorite.ts), [`dragAndDrop`](../src/e2e/puppeteer/helpers/dragAndDrop.ts) | Drives real mouse down/move/up input and waits for drag-specific visible conditions. | +| Drag and drop | [`dragAndDropThought`](../src/e2e/puppeteer/helpers/dragAndDropThought.ts), [`dragAndDropFavorite`](../src/e2e/puppeteer/helpers/dragAndDropFavorite.ts), [`dragAndDrop`](../src/e2e/puppeteer/helpers/dragAndDrop.ts) | Drives real mouse down/move/up input and waits for drag-specific visible conditions. `dragAndDropThought` waits until react-dnd has hovered the drop target under the pointer before it holds or drops, so a test never waits on a drop hover itself. | | Scroll | [`scroll`](../src/e2e/puppeteer/helpers/scroll.ts), [`scrollBy`](../src/e2e/puppeteer/helpers/scrollBy.ts), [`scrollIntoView`](../src/e2e/puppeteer/helpers/scrollIntoView.ts), [`scrollTo`](../src/e2e/puppeteer/helpers/scrollTo.ts) | Scrolls the window or a named container; use the narrowest helper that expresses the intent. | | Emulate a mobile device | [`deviceEmulation.useForSuite`](../src/e2e/puppeteer/helpers/deviceEmulation.ts) | Selects a Puppeteer device profile at suite scope, which `setup` applies before navigation. There is no mid-session equivalent; see the emulation note above. | diff --git a/src/components/DropChild.tsx b/src/components/DropChild.tsx index 0c5cc265c44..20b7b96d9fa 100644 --- a/src/components/DropChild.tsx +++ b/src/components/DropChild.tsx @@ -33,7 +33,7 @@ const DropChild = ({ depth, path, simplePath, cliff, isLastVisible }: DropChildP const dropHoverColorValue = useSelector(state => dropHoverColor(state, depth || 0)) const fontSize = useSelector(state => state.fontSize) - const { isHovering, dropTarget } = useDragAndDropSubThought({ path, simplePath }) + const { isHovering, isOver, dropTarget } = useDragAndDropSubThought({ path, simplePath }) // Calculate the height for the child thought over cliff // If the thought is at a cliff (cliff < 0) or is the last visible thought, calculate the drop target height @@ -62,6 +62,7 @@ const DropChild = ({ depth, path, simplePath, cliff, isLastVisible }: DropChildP }), )} ref={dndRef(dropTarget)} + data-drop-over={isOver} style={{ height: `${0.5 + dropTargetHeight}em`, // Adjust position to compensate for cliff padding on parent thought diff --git a/src/components/DropEnd.tsx b/src/components/DropEnd.tsx index 00102be18b0..decbe08b8e5 100644 --- a/src/components/DropEnd.tsx +++ b/src/components/DropEnd.tsx @@ -58,7 +58,7 @@ const DropEnd = ({ attributeEquals(state, head(rootedParentOf(state, path)), '=view', 'Table'), ) - const { isHovering, dropTarget } = useDragAndDropSubThought({ path }) + const { isHovering, isOver, dropTarget } = useDragAndDropSubThought({ path }) // a boolean indicating if the drop-hover component is shown // true if hovering and the context is not sorted @@ -128,6 +128,7 @@ const DropEnd = ({ transform: `translateX(${DROPEND_FINGERSHIFT}em)`, }} ref={dndRef(dropTarget)} + data-drop-over={isOver} > {testFlags.simulateDrop && ( {testFlags.simulateDrop && ( dragPreview(dropTarget(node)))} aria-label='child' + data-drop-over={isOver} data-divider={isDivider(value)} data-editing={isEditing} onClick={isTouch ? undefined : handleMultiselect} diff --git a/src/e2e/puppeteer/__tests__/drag-and-drop.ts b/src/e2e/puppeteer/__tests__/drag-and-drop.ts index ee824aa10d1..f390eb55531 100644 --- a/src/e2e/puppeteer/__tests__/drag-and-drop.ts +++ b/src/e2e/puppeteer/__tests__/drag-and-drop.ts @@ -21,7 +21,6 @@ import setSelection from '../helpers/setSelection' import simulateDragAndDrop from '../helpers/simulateDragAndDrop' import waitForAlert from '../helpers/waitForAlert' import waitForEditable from '../helpers/waitForEditable' -import waitUntil from '../helpers/waitUntil' import withDebugLog from '../helpers/withDebugLog' import { page } from '../session' @@ -134,10 +133,6 @@ describe('drag', () => { // hover the drop target directly below aaa, i.e. aaa's own position await dragAndDropThought('aaa', 'aaa', { hold: true, position: 'after' }) - // the drop hover is still shown at the thought's own position; only the drop is cancelled - // (.drop-hover is the class that dropHoverRecipe gives every drop hover bar) - await waitUntil(() => !!document.querySelector('.drop-hover'), { timeout: 6000 }) - // release on the same drop target await dragAndDropThought('aaa', 'aaa', { position: 'after', skipMouseDown: true }) diff --git a/src/e2e/puppeteer/helpers/dragAndDropThought.ts b/src/e2e/puppeteer/helpers/dragAndDropThought.ts index fee8118f47b..77ec48d3bfd 100644 --- a/src/e2e/puppeteer/helpers/dragAndDropThought.ts +++ b/src/e2e/puppeteer/helpers/dragAndDropThought.ts @@ -5,7 +5,77 @@ import hide from './hide' import showMousePointer from './showMousePointer' import waitUntil from './waitUntil' -/** Performs Drag and Drop functionality on a thought in Puppeteer browser. */ +/** The page-side record of whether react-dnd has hovered the drop target under the drop point. */ +interface DropTargetHoverWatch { + /** Returns true if react-dnd hovers the drop target under the drop point now. */ + check: () => boolean + observer: MutationObserver + hovered: boolean +} + +declare global { + interface Window { + /** Set by dragAndDropThought for the length of a move. */ + __dropTargetHoverWatch?: DropTargetHoverWatch + } +} + +/** + * Starts recording whether react-dnd hovers the drop target under the given point: the innermost element with + * data-drop-over, which every drop target element renders from react-dnd's isOver({ shallow: true }). That is the + * target a drop there lands on. It is set in the same commit as the target's isHovering, from which DropChild, DropEnd + * and DropUncle draw their drop hover. A thought row's drop hover also needs state.hoveringPath, which the same hover + * writes before it whenever the pointer has moved. It is true even on a target that rejects the drop, so this also + * covers a drop that shows no drop hover. + * + * The hover is recorded as it is rendered rather than polled for. A drag driven by Puppeteer dispatches dragover only + * when the pointer moves, so when a hover changes the layout under the stationary pointer, e.g. by expanding a + * thought, nothing hovers the drop target that is under it afterwards, and a poll would wait for it forever. + */ +const watchDropTargetHover = (point: { x: number; y: number }) => + page.evaluate(({ x, y }) => { + window.__dropTargetHoverWatch?.observer.disconnect() + + /** Returns true if react-dnd hovers the drop target under the point. */ + const check = () => + document.elementFromPoint(x, y)?.closest('[data-drop-over]')?.getAttribute('data-drop-over') === 'true' + + const watch: DropTargetHoverWatch = { + check, + observer: new MutationObserver(() => { + watch.hovered = watch.hovered || check() + }), + hovered: false, + } + watch.observer.observe(document.body, { attributes: true, attributeFilter: ['data-drop-over'], subtree: true }) + window.__dropTargetHoverWatch = watch + }, point) + +/** Waits until react-dnd has hovered the drop target under the point since watchDropTargetHover, or hovers it now, which covers a target that was already hovered before the move. Throws if it never does. */ +const waitForDropTargetHover = async (point: { x: number; y: number }) => { + try { + await page.waitForFunction( + () => !!window.__dropTargetHoverWatch?.hovered || !!window.__dropTargetHoverWatch?.check(), + { timeout: 5000 }, + ) + } catch (e) { + const targetFound = await page.evaluate( + ({ x, y }) => !!document.elementFromPoint(x, y)?.closest('[data-drop-over]'), + point, + ) + throw new Error( + targetFound + ? 'The drop target under the pointer was never hovered.' + : `There is no drop target under the pointer at (${point.x}, ${point.y}).`, + { cause: e }, + ) + } +} + +/** Stops recording the hover. */ +const unwatchDropTargetHover = () => page.evaluate(() => window.__dropTargetHoverWatch?.observer.disconnect()) + +/** Performs Drag and Drop functionality on a thought in Puppeteer browser. Waits for the drop target under the pointer to render its hover before holding or dropping, so callers do not need to synchronize with drag-and-drop themselves. */ const dragAndDropThought = async ( sourceValue: string, /** The value of the thought to drop at. Use the position absolute to control whether to drop before, after, etc. If null, only initiates the drag and move the mouse. */ @@ -92,12 +162,22 @@ const dragAndDropThought = async ( y: dropUncle ? dragEnd.y + dragEnd.height : dragEnd.y + dragEnd.height / 2 + yOffset, } - // A single mousemove is not registered by react-dnd when continuing an existing drag (skipMouseDown), so issue a second move one pixel away first. - // Both events land on the drop target, so no other drop targets are hovered in between. - if (skipMouseDown) { - await page.mouse.move(dropPosition.x, dropPosition.y - 1) + await watchDropTargetHover(dropPosition) + try { + // A single mousemove is not registered by react-dnd when continuing an existing drag (skipMouseDown), so issue a second move one pixel away first. + // Both events land on the drop target, so no other drop targets are hovered in between. + if (skipMouseDown) { + await page.mouse.move(dropPosition.x, dropPosition.y - 1) + } + await page.mouse.move(dropPosition.x, dropPosition.y) + + // The drop targets other than the thought rows (DropChild, DropEnd, DropUncle) mount only once the drag is in + // progress, so the drop target under the pointer is only known after it. + await page.locator('[data-drag-in-progress="true"]').wait() + await waitForDropTargetHover(dropPosition) + } finally { + await unwatchDropTargetHover() } - await page.mouse.move(dropPosition.x, dropPosition.y) } else { // Trigger drag state when no explicit drop target is provided. // Without this move, drag-and-drop alert visibility is flaky in CI. diff --git a/src/hooks/useDragAndDropSubThought.tsx b/src/hooks/useDragAndDropSubThought.tsx index 34e80fe5ceb..726af3e4432 100644 --- a/src/hooks/useDragAndDropSubThought.tsx +++ b/src/hooks/useDragAndDropSubThought.tsx @@ -282,8 +282,8 @@ const drop = (props: DroppableSubthoughts, monitor: DropTargetMonitor) => { /** Creates the props for drop. */ const dropCollect = (monitor: DropTargetMonitor) => ({ isHovering: monitor.isOver({ shallow: true }) && monitor.canDrop(), - // is being hovered over current thought irrespective of whether the given item is droppable - isBeingHoveredOver: monitor.isOver({ shallow: true }), + // hovered irrespective of whether the dragged thought can be dropped here + isOver: monitor.isOver({ shallow: true }), isDeepHovering: monitor.isOver(), canDropThought: monitor.canDrop(), }) @@ -292,7 +292,7 @@ const dropCollect = (monitor: DropTargetMonitor) => ({ const useDragAndDropSubThought = (props: DroppableSubthoughts) => { const dispatch = useDispatch() - const [{ isHovering, isDeepHovering, canDropThought }, dropTarget] = useDrop({ + const [{ isHovering, isOver, isDeepHovering, canDropThought }, dropTarget] = useDrop({ accept: [DragAndDropType.Thought, NativeTypes.FILE], canDrop: (item, monitor) => !dropRejection(props, monitor), drop: (item, monitor) => drop(props, monitor), @@ -338,7 +338,7 @@ const useDragAndDropSubThought = (props: DroppableSubthoughts) => { const isHoveringPinned = usePinDropHover(isHovering) - return { isHovering: isHoveringPinned, isDeepHovering, dropTarget } + return { isHovering: isHoveringPinned, isOver, isDeepHovering, dropTarget } } export default useDragAndDropSubThought diff --git a/src/hooks/useDragAndDropThought.tsx b/src/hooks/useDragAndDropThought.tsx index b03bb85f994..2f5285c24b2 100644 --- a/src/hooks/useDragAndDropThought.tsx +++ b/src/hooks/useDragAndDropThought.tsx @@ -355,6 +355,8 @@ const dragCollect = (monitor: DragSourceMonitor) => ({ /** Collects props from the DropTarget. */ const dropCollect = (monitor: DropTargetMonitor) => ({ isHovering: monitor.isOver({ shallow: true }) && monitor.canDrop(), + // hovered irrespective of whether the dragged thought can be dropped here + isOver: monitor.isOver({ shallow: true }), isDeepHovering: monitor.isOver(), canDropThought: monitor.canDrop(), }) @@ -380,7 +382,7 @@ const useDragAndDropThought = (props: Partial & { hoverZo collect: dragCollect, }) - const [{ isHovering, isDeepHovering, canDropThought }, dropTarget] = useDrop({ + const [{ isHovering, isOver, isDeepHovering, canDropThought }, dropTarget] = useDrop({ accept: [DragAndDropType.Thought, NativeTypes.FILE], canDrop: (item, monitor) => !dropRejection(propsTypes, monitor), drop: (item, monitor) => drop(propsTypes, monitor), @@ -439,6 +441,7 @@ const useDragAndDropThought = (props: Partial & { hoverZo dragSourceEditable, dragPreview, isHovering: isHoveringPinned, + isOver, isDeepHovering, canDropThought, dropTarget,