Skip to content

Commit bab5221

Browse files
committed
fix(sidebar): stop bubbled dragleave events cancelling an in-progress drag
1 parent affafe2 commit bab5221

2 files changed

Lines changed: 266 additions & 149 deletions

File tree

apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.test.tsx

Lines changed: 91 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -102,6 +102,38 @@ function fakeDropEvent(): unknown {
102102
}
103103
}
104104

105+
/**
106+
* Registers a scroll container spanning x 0-200, then arms a drop indicator on it. Registration has
107+
* to precede the first dragOver: the listener effect reads the container ref when `isDragging`
108+
* flips, and `setScrollContainer` is a plain ref setter that triggers no re-render of its own.
109+
*/
110+
function armDragOverScrollContainer(): HTMLDivElement {
111+
const scrollContainer = document.createElement('div')
112+
scrollContainer.getBoundingClientRect = () =>
113+
({ left: 0, right: 200, top: 0, bottom: 400 }) as DOMRect
114+
document.body.appendChild(scrollContainer)
115+
act(() => {
116+
latest.setScrollContainer(scrollContainer)
117+
})
118+
act(() => {
119+
latest.createEdgeDropZone('workflow-1', 'before').onDragOver(fakeDragOverEvent() as never)
120+
})
121+
return scrollContainer
122+
}
123+
124+
/** Chrome's `dragleave` shape: bubbles, and always reports a null `relatedTarget`. */
125+
function dispatchBubbledDragLeave(element: HTMLElement, clientX: number) {
126+
act(() => {
127+
const leave = new Event('dragleave', { bubbles: true }) as DragEvent
128+
Object.defineProperties(leave, {
129+
relatedTarget: { value: null },
130+
clientX: { value: clientX },
131+
clientY: { value: 200 },
132+
})
133+
element.dispatchEvent(leave)
134+
})
135+
}
136+
105137
let container: HTMLDivElement
106138
let root: Root
107139

@@ -136,7 +168,6 @@ describe('useDragDrop stranded-drag reset', () => {
136168
})
137169

138170
it('clears isDragging on a window dragend when no drop fired', () => {
139-
// A drag entering the list flips isDragging on via initDragOver.
140171
act(() => {
141172
latest.createRootDropZone().onDragOver(fakeDragOverEvent() as never)
142173
})
@@ -149,13 +180,70 @@ describe('useDragDrop stranded-drag reset', () => {
149180
expect(latest.isDragging).toBe(false)
150181
})
151182

183+
/**
184+
* `dragleave` bubbles and Chrome nulls its `relatedTarget`, so the container listener sees one
185+
* for every descendant boundary the pointer crosses. Treating those as "left the list" wiped the
186+
* drop indicator mid-drag, and `handleDrop` bails on a null indicator — so a release just after
187+
* crossing a boundary did nothing at all. Nested rows in an expanded folder cross the most
188+
* boundaries, which is why open folders looked like they broke dragging outright.
189+
*/
190+
it('keeps the drop indicator when a bubbled dragleave has no relatedTarget but the pointer is still inside', () => {
191+
const scrollContainer = armDragOverScrollContainer()
192+
expect(latest.dropIndicator).toEqual({
193+
targetId: 'workflow-1',
194+
position: 'before',
195+
folderId: null,
196+
})
197+
198+
// A child row handing off to its sibling: pointer still well inside the list's 0-200 x-range.
199+
dispatchBubbledDragLeave(scrollContainer, 100)
200+
201+
expect(latest.dropIndicator).not.toBeNull()
202+
scrollContainer.remove()
203+
})
204+
205+
/**
206+
* The root drop zone's own `onDragLeave` clears the indicator through `isLeavingElement`, which
207+
* made the same null-`relatedTarget` assumption. Fixing only the container listener would have
208+
* left this second path clearing the indicator on every internal crossing.
209+
*/
210+
it('keeps the drop indicator when the root drop zone sees a relatedTarget-less dragleave inside itself', () => {
211+
const zone = document.createElement('div')
212+
zone.getBoundingClientRect = () => ({ left: 0, right: 200, top: 0, bottom: 400 }) as DOMRect
213+
214+
act(() => {
215+
latest.createEdgeDropZone('workflow-1', 'before').onDragOver(fakeDragOverEvent() as never)
216+
})
217+
expect(latest.dropIndicator).not.toBeNull()
218+
219+
act(() => {
220+
latest.createRootDropZone().onDragLeave({
221+
relatedTarget: null,
222+
currentTarget: zone,
223+
clientX: 100,
224+
clientY: 200,
225+
} as never)
226+
})
227+
228+
expect(latest.dropIndicator).not.toBeNull()
229+
})
230+
231+
it('clears the drop indicator when the pointer genuinely leaves the list', () => {
232+
const scrollContainer = armDragOverScrollContainer()
233+
expect(latest.dropIndicator).not.toBeNull()
234+
235+
dispatchBubbledDragLeave(scrollContainer, 900)
236+
237+
expect(latest.dropIndicator).toBeNull()
238+
scrollContainer.remove()
239+
})
240+
152241
it('keeps isDragging active across dragOver updates until the drag ends', () => {
153242
act(() => {
154243
latest.createRootDropZone().onDragOver(fakeDragOverEvent() as never)
155244
})
156245
expect(latest.isDragging).toBe(true)
157246

158-
// A subsequent dragOver must not tear down the active drag.
159247
act(() => {
160248
latest.createRootDropZone().onDragOver(fakeDragOverEvent() as never)
161249
})
@@ -227,7 +315,7 @@ describe('useDragDrop spring-open revert', () => {
227315
dragOverFolderUntilExpanded()
228316
mockSetExpanded.mockClear()
229317

230-
// Drop inside folder-1, then the drag ends as it always does.
318+
// `dragend` fires after every drop, so the revert path runs here too.
231319
act(() => {
232320
void latest.createFolderDragHandlers('folder-1', null).onDrop(fakeDropEvent() as never)
233321
})

0 commit comments

Comments
 (0)