From def9eab2b50e12d17322997e537840f0f01c9bae Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 13 Aug 2026 14:31:09 -0700 Subject: [PATCH 1/3] fix(sidebar): close folders a drag spring-opened, and surface reorder failures --- .../sidebar/hooks/use-drag-drop.test.tsx | 136 +++++++++++++- .../components/sidebar/hooks/use-drag-drop.ts | 170 +++++++++++++----- 2 files changed, 256 insertions(+), 50 deletions(-) diff --git a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.test.tsx b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.test.tsx index 9a84d924f5f..cdf4393a9ee 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.test.tsx @@ -9,6 +9,9 @@ vi.mock('next/navigation', () => ({ useParams: () => ({ workspaceId: 'ws-1' }), })) +/** Kept out of the module graph so this suite does not pull emcn's CSS modules through postcss. */ +vi.mock('@sim/emcn', () => ({ toast: { error: vi.fn() } })) + vi.mock('@/hooks/queries/folders', () => ({ useReorderFolders: () => ({ mutateAsync: vi.fn() }), })) @@ -29,13 +32,23 @@ vi.mock('@/lib/folders/tree', () => ({ getFolderPath: () => [], })) -const { mockUseFolderStore } = vi.hoisted(() => { - const folderState = { setExpanded: () => {}, expandedFolders: new Set() } +const { mockUseFolderStore, mockSetExpanded, expandedFolders } = vi.hoisted(() => { + const expanded = new Set() + const setExpanded = vi.fn((folderId: string, isExpanded: boolean) => { + if (isExpanded) expanded.add(folderId) + else expanded.delete(folderId) + }) + const folderState = { + setExpanded, + expandedFolders: expanded, + clearSelection: () => {}, + clearFolderSelection: () => {}, + } const store = Object.assign( (selector: (state: typeof folderState) => unknown) => selector(folderState), { getState: () => folderState } ) - return { mockUseFolderStore: store } + return { mockUseFolderStore: store, mockSetExpanded: setExpanded, expandedFolders: expanded } }) vi.mock('@/stores/folders/store', () => ({ useFolderStore: mockUseFolderStore })) @@ -63,6 +76,32 @@ function fakeDragOverEvent(): unknown { } } +/** + * A `dragover` on a folder row. `clientY` sits in the middle band of the 100px rect, which is what + * `calculateFolderDropPosition` reads as "inside" — the position that arms the spring-open timer. + */ +function fakeFolderDragOverEvent(): unknown { + const currentTarget = { + getBoundingClientRect: () => ({ top: 0, bottom: 100, height: 100 }), + } + return { + preventDefault: () => {}, + stopPropagation: () => {}, + clientY: 50, + target: {}, + currentTarget, + } +} + +/** A `drop` carrying no selection payload: enough to record the destination, then bail. */ +function fakeDropEvent(): unknown { + return { + preventDefault: () => {}, + stopPropagation: () => {}, + dataTransfer: { getData: () => '' }, + } +} + let container: HTMLDivElement let root: Root @@ -93,6 +132,7 @@ describe('useDragDrop stranded-drag reset', () => { container.remove() vi.unstubAllGlobals() vi.clearAllMocks() + expandedFolders.clear() }) it('clears isDragging on a window dragend when no drop fired', () => { @@ -122,3 +162,93 @@ describe('useDragDrop stranded-drag reset', () => { expect(latest.isDragging).toBe(true) }) }) + +/** + * Hovering a collapsed folder mid-drag spring-opens it so you can drop inside. Every folder opened + * that way that the drop did NOT land in has to close again, or dragging past a folder silently + * leaves it open and the sidebar grows rows the user never asked to see. + */ +describe('useDragDrop spring-open revert', () => { + beforeEach(() => { + vi.useFakeTimers() + vi.stubGlobal( + 'requestAnimationFrame', + () => 0 as unknown as ReturnType + ) + vi.stubGlobal('cancelAnimationFrame', () => {}) + container = document.createElement('div') + document.body.appendChild(container) + root = createRoot(container) + act(() => { + root.render() + }) + }) + + afterEach(() => { + act(() => { + root.unmount() + }) + container.remove() + vi.unstubAllGlobals() + vi.useRealTimers() + vi.clearAllMocks() + expandedFolders.clear() + }) + + /** Drives a drag that lingers over `folder-1` long enough to spring it open. */ + function dragOverFolderUntilExpanded() { + act(() => { + latest.handleDragStart(null) + }) + act(() => { + latest + .createFolderDragHandlers('folder-1', null) + .onDragOver(fakeFolderDragOverEvent() as never) + }) + act(() => { + vi.advanceTimersByTime(500) + }) + } + + it('closes a folder it spring-opened when the drag ends without dropping into it', () => { + dragOverFolderUntilExpanded() + expect(mockSetExpanded).toHaveBeenCalledWith('folder-1', true) + + // Esc-cancel / release outside: `dragend` fires with no drop recorded. + act(() => { + latest.handleDragEnd() + }) + + expect(mockSetExpanded).toHaveBeenCalledWith('folder-1', false) + expect(expandedFolders.has('folder-1')).toBe(false) + }) + + it('leaves a folder open when the drop landed inside it', () => { + dragOverFolderUntilExpanded() + mockSetExpanded.mockClear() + + // Drop inside folder-1, then the drag ends as it always does. + act(() => { + void latest.createFolderDragHandlers('folder-1', null).onDrop(fakeDropEvent() as never) + }) + act(() => { + latest.handleDragEnd() + }) + + expect(mockSetExpanded).not.toHaveBeenCalledWith('folder-1', false) + expect(expandedFolders.has('folder-1')).toBe(true) + }) + + it('never closes a folder the user had already opened themselves', () => { + expandedFolders.add('folder-1') + + dragOverFolderUntilExpanded() + act(() => { + latest.handleDragEnd() + }) + + // Already-expanded folders are skipped by the spring-open effect, so nothing to revert. + expect(mockSetExpanded).not.toHaveBeenCalledWith('folder-1', false) + expect(expandedFolders.has('folder-1')).toBe(true) + }) +}) diff --git a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.ts b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.ts index 3b97ca3caf5..cf71888d6b6 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.ts +++ b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.ts @@ -1,5 +1,7 @@ import { useCallback, useEffect, useRef, useState } from 'react' +import { toast } from '@sim/emcn' import { createLogger } from '@sim/logger' +import { getErrorMessage } from '@sim/utils/errors' import { noop } from '@sim/utils/helpers' import { useParams } from 'next/navigation' import { getFolderPath } from '@/lib/folders/tree' @@ -49,6 +51,18 @@ function isSameFolderScope( return (parentOrFolderId ?? null) === (scope ?? null) } +/** + * Sibling order within one folder scope. Matches `compareByOrder` in the sidebar's own utils so the + * indices this hook computes address the same slots the list renders. + */ +function compareSiblingItems(a: SiblingItem, b: SiblingItem): number { + if (a.sortOrder !== b.sortOrder) return a.sortOrder - b.sortOrder + const timeA = a.createdAt.getTime() + const timeB = b.createdAt.getTime() + if (timeA !== timeB) return timeA - timeB + return a.id.localeCompare(b.id) +} + export function useDragDrop(options: UseDragDropOptions = {}) { const { disabled = false } = options const [dropIndicator, setDropIndicator] = useState(null) @@ -64,8 +78,15 @@ export function useDragDrop(options: UseDragDropOptions = {}) { const hoverExpandTimerRef = useRef(null) const lastDragYRef = useRef(0) const draggedSourceFolderRef = useRef(null) - const siblingsCacheRef = useRef | null>(null) const isDraggingRef = useRef(false) + /** + * Folders this drag spring-opened, so {@link handleDragEnd} can close the ones the drop did not + * land in. Only ever holds folders that were collapsed when the drag reached them, so a folder + * the user opened themselves is never touched. + */ + const autoExpandedRef = useRef | null>(null) + /** Destination of the drop that just happened, read once by the drag-end cleanup. */ + const dropDestinationRef = useRef(null) const params = useParams() const workspaceId = params.workspaceId as string | undefined @@ -139,6 +160,7 @@ export function useDragDrop(options: UseDragDropOptions = {}) { if (expandedFolders.has(hoverFolderId)) return hoverExpandTimerRef.current = window.setTimeout(() => { + ;(autoExpandedRef.current ??= new Set()).add(hoverFolderId) setExpanded(hoverFolderId, true) }, HOVER_EXPAND_DELAY) @@ -150,10 +172,6 @@ export function useDragDrop(options: UseDragDropOptions = {}) { } }, [hoverFolderId, isDragging, expandedFolders, setExpanded]) - useEffect(() => { - siblingsCacheRef.current?.clear() - }, [workspaceId]) - const calculateDropPosition = useCallback( (e: React.DragEvent, element: HTMLElement): 'before' | 'after' => { const rect = element.getBoundingClientRect() @@ -175,14 +193,6 @@ export function useDragDrop(options: UseDragDropOptions = {}) { [] ) - const compareSiblingItems = (a: SiblingItem, b: SiblingItem): number => { - if (a.sortOrder !== b.sortOrder) return a.sortOrder - b.sortOrder - const timeA = a.createdAt.getTime() - const timeB = b.createdAt.getTime() - if (timeA !== timeB) return timeA - timeB - return a.id.localeCompare(b.id) - } - const getDestinationFolderId = useCallback((indicator: DropIndicator): string | null => { return indicator.position === 'inside' ? indicator.targetId === 'root' @@ -218,7 +228,11 @@ export function useDragDrop(options: UseDragDropOptions = {}) { ) const buildAndSubmitUpdates = useCallback( - async (newOrder: SiblingItem[], destinationFolderId: string | null) => { + async ( + targetWorkspaceId: string, + newOrder: SiblingItem[], + destinationFolderId: string | null + ) => { const indexed = newOrder.map((item, i) => ({ ...item, sortOrder: i })) const folderUpdates = indexed @@ -229,22 +243,42 @@ export function useDragDrop(options: UseDragDropOptions = {}) { .filter((item) => item.type === 'workflow') .map((item) => ({ id: item.id, sortOrder: item.sortOrder, folderId: destinationFolderId })) - await Promise.all( - [ - folderUpdates.length > 0 && - reorderFoldersMutation.mutateAsync({ - workspaceId: workspaceId!, - updates: folderUpdates, - }), - workflowUpdates.length > 0 && - reorderWorkflowsMutation.mutateAsync({ - workspaceId: workspaceId!, - updates: workflowUpdates, - }), - ].filter(Boolean) - ) + /** + * Folders and workflows share one index space but commit through separate endpoints, so a + * drop that moves both issues two requests. `allSettled` rather than `all`: with `all`, one + * rejection abandons the other request while it is still in flight and commits anyway, + * leaving the caller unable to tell a total failure from a half-applied one. + */ + const results = await Promise.allSettled([ + ...(folderUpdates.length > 0 + ? [ + reorderFoldersMutation.mutateAsync({ + workspaceId: targetWorkspaceId, + updates: folderUpdates, + }), + ] + : []), + ...(workflowUpdates.length > 0 + ? [ + reorderWorkflowsMutation.mutateAsync({ + workspaceId: targetWorkspaceId, + updates: workflowUpdates, + }), + ] + : []), + ]) + + const rejected = results.filter((r) => r.status === 'rejected') + if (rejected.length > 0) { + throw new AggregateError( + rejected.map((r) => r.reason), + rejected.length === results.length + ? 'Reorder failed' + : 'Reorder partially failed; some items kept their previous position' + ) + } }, - [workspaceId, reorderFoldersMutation, reorderWorkflowsMutation] + [reorderFoldersMutation, reorderWorkflowsMutation] ) const isLeavingElement = useCallback((e: React.DragEvent): boolean => { @@ -259,7 +293,12 @@ export function useDragDrop(options: UseDragDropOptions = {}) { if (stopPropagation) e.stopPropagation() lastDragYRef.current = e.clientY - if (!isDragging) { + /** + * Read from the ref, not the `isDragging` state: several `dragover` events fire before React + * commits the first `setIsDragging`, and on those frames the state still reads false while + * the drag is already live — which would skip restarting the auto-scroll loop below. + */ + if (!isDraggingRef.current) { isDraggingRef.current = true setIsDragging(true) } else if (scrollAnimationRef.current === null) { @@ -268,17 +307,16 @@ export function useDragDrop(options: UseDragDropOptions = {}) { return true }, - [isDragging, handleAutoScroll] + [handleAutoScroll] ) + /** + * Siblings in one folder scope, read live from the query cache on every call. Deliberately + * uncached: the only callers are the drag-over indicator and the drop itself, and both must see + * the optimistic order the previous drop already wrote. + */ const getSiblingItems = useCallback( (folderId: string | null): SiblingItem[] => { - const cacheKey = folderId ?? 'root' - if (!isDraggingRef.current) { - const cached = siblingsCacheRef.current?.get(cacheKey) - if (cached) return cached - } - const currentFolders = workspaceId ? getFolderMap(workspaceId) : {} const currentWorkflows = workspaceId ? getWorkflows(workspaceId) : [] const siblings = [ @@ -300,10 +338,6 @@ export function useDragDrop(options: UseDragDropOptions = {}) { })), ].sort(compareSiblingItems) - if (!isDraggingRef.current) { - const cache = (siblingsCacheRef.current ??= new Map()) - cache.set(cacheKey, siblings) - } return siblings }, [workspaceId] @@ -444,13 +478,19 @@ export function useDragDrop(options: UseDragDropOptions = {}) { ...remaining.slice(insertAt), ] - await buildAndSubmitUpdates(newOrder, destinationFolderId) + await buildAndSubmitUpdates(workspaceId, newOrder, destinationFolderId) const { clearSelection, clearFolderSelection } = useFolderStore.getState() clearSelection() clearFolderSelection() } catch (error) { logger.error('Failed to drop selection:', error) + /** + * Each mutation rolls its own slice of the cache back, so a failure is visible only as the + * rows silently returning to where they were. Say why, or it reads as the sidebar + * spontaneously undoing the move. + */ + toast.error(getErrorMessage(error, 'Failed to move items')) } }, [ @@ -474,7 +514,11 @@ export function useDragDrop(options: UseDragDropOptions = {}) { setDropIndicator(null) isDraggingRef.current = false setIsDragging(false) - siblingsCacheRef.current?.clear() + /** + * Recorded synchronously, before any `await`: `dragend` fires as soon as this handler + * returns to the event loop, and the cleanup there needs to know which folder to leave open. + */ + dropDestinationRef.current = indicator ? getDestinationFolderId(indicator) : null if (!indicator) return @@ -491,7 +535,7 @@ export function useDragDrop(options: UseDragDropOptions = {}) { logger.error('Failed to handle drop:', error) } }, - [handleSelectionDrop] + [handleSelectionDrop, getDestinationFolderId] ) const createWorkflowDragHandlers = useCallback( @@ -614,11 +658,43 @@ export function useDragDrop(options: UseDragDropOptions = {}) { const handleDragStart = useCallback((sourceFolderId: string | null) => { draggedSourceFolderRef.current = sourceFolderId - siblingsCacheRef.current?.clear() isDraggingRef.current = true setIsDragging(true) }, []) + /** + * Closes the folders this drag spring-opened, keeping the drop destination and its ancestors + * open so the moved rows stay visible. Runs on every drag end — drop, Esc-cancel, or a release + * outside the list — because `dragend` always fires on the source, so a cancelled drag cannot + * leave folders open that the user never chose to open. + */ + const collapseAutoExpandedFolders = useCallback(() => { + const autoExpanded = autoExpandedRef.current + const destination = dropDestinationRef.current + dropDestinationRef.current = null + if (!autoExpanded?.size) return + + /** + * The destination is seeded directly rather than relying on the ancestor walk to include it: + * `getFolderPath` returns an empty chain for an id missing from the folder map, which would + * otherwise close the very folder the drop just landed in. + */ + const keepOpen = new Set() + if (destination) { + keepOpen.add(destination) + if (workspaceId) { + for (const folder of getFolderPath(getFolderMap(workspaceId), destination)) { + keepOpen.add(folder.id) + } + } + } + + for (const folderId of autoExpanded) { + if (!keepOpen.has(folderId)) setExpanded(folderId, false) + } + autoExpanded.clear() + }, [workspaceId, setExpanded]) + const handleDragEnd = useCallback(() => { isDraggingRef.current = false setIsDragging(false) @@ -626,8 +702,8 @@ export function useDragDrop(options: UseDragDropOptions = {}) { setDropIndicator(null) draggedSourceFolderRef.current = null setHoverFolderId(null) - siblingsCacheRef.current?.clear() - }, []) + collapseAutoExpandedFolders() + }, [collapseAutoExpandedFolders]) useEffect(() => { if (!isDragging) return From 8b28504897fd6a06d3dd0aac7bccf542103c70a4 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 13 Aug 2026 14:53:54 -0700 Subject: [PATCH 2/3] fix(sidebar): stop bubbled dragleave events cancelling an in-progress drag --- .../sidebar/hooks/use-drag-drop.test.tsx | 94 ++++- .../components/sidebar/hooks/use-drag-drop.ts | 321 ++++++++++-------- 2 files changed, 266 insertions(+), 149 deletions(-) diff --git a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.test.tsx b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.test.tsx index cdf4393a9ee..dfe3bb37c71 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.test.tsx @@ -102,6 +102,38 @@ function fakeDropEvent(): unknown { } } +/** + * Registers a scroll container spanning x 0-200, then arms a drop indicator on it. Registration has + * to precede the first dragOver: the listener effect reads the container ref when `isDragging` + * flips, and `setScrollContainer` is a plain ref setter that triggers no re-render of its own. + */ +function armDragOverScrollContainer(): HTMLDivElement { + const scrollContainer = document.createElement('div') + scrollContainer.getBoundingClientRect = () => + ({ left: 0, right: 200, top: 0, bottom: 400 }) as DOMRect + document.body.appendChild(scrollContainer) + act(() => { + latest.setScrollContainer(scrollContainer) + }) + act(() => { + latest.createEdgeDropZone('workflow-1', 'before').onDragOver(fakeDragOverEvent() as never) + }) + return scrollContainer +} + +/** Chrome's `dragleave` shape: bubbles, and always reports a null `relatedTarget`. */ +function dispatchBubbledDragLeave(element: HTMLElement, clientX: number) { + act(() => { + const leave = new Event('dragleave', { bubbles: true }) as DragEvent + Object.defineProperties(leave, { + relatedTarget: { value: null }, + clientX: { value: clientX }, + clientY: { value: 200 }, + }) + element.dispatchEvent(leave) + }) +} + let container: HTMLDivElement let root: Root @@ -136,7 +168,6 @@ describe('useDragDrop stranded-drag reset', () => { }) it('clears isDragging on a window dragend when no drop fired', () => { - // A drag entering the list flips isDragging on via initDragOver. act(() => { latest.createRootDropZone().onDragOver(fakeDragOverEvent() as never) }) @@ -149,13 +180,70 @@ describe('useDragDrop stranded-drag reset', () => { expect(latest.isDragging).toBe(false) }) + /** + * `dragleave` bubbles and Chrome nulls its `relatedTarget`, so the container listener sees one + * for every descendant boundary the pointer crosses. Treating those as "left the list" wiped the + * drop indicator mid-drag, and `handleDrop` bails on a null indicator — so a release just after + * crossing a boundary did nothing at all. Nested rows in an expanded folder cross the most + * boundaries, which is why open folders looked like they broke dragging outright. + */ + it('keeps the drop indicator when a bubbled dragleave has no relatedTarget but the pointer is still inside', () => { + const scrollContainer = armDragOverScrollContainer() + expect(latest.dropIndicator).toEqual({ + targetId: 'workflow-1', + position: 'before', + folderId: null, + }) + + // A child row handing off to its sibling: pointer still well inside the list's 0-200 x-range. + dispatchBubbledDragLeave(scrollContainer, 100) + + expect(latest.dropIndicator).not.toBeNull() + scrollContainer.remove() + }) + + /** + * The root drop zone's own `onDragLeave` clears the indicator through `isLeavingElement`, which + * made the same null-`relatedTarget` assumption. Fixing only the container listener would have + * left this second path clearing the indicator on every internal crossing. + */ + it('keeps the drop indicator when the root drop zone sees a relatedTarget-less dragleave inside itself', () => { + const zone = document.createElement('div') + zone.getBoundingClientRect = () => ({ left: 0, right: 200, top: 0, bottom: 400 }) as DOMRect + + act(() => { + latest.createEdgeDropZone('workflow-1', 'before').onDragOver(fakeDragOverEvent() as never) + }) + expect(latest.dropIndicator).not.toBeNull() + + act(() => { + latest.createRootDropZone().onDragLeave({ + relatedTarget: null, + currentTarget: zone, + clientX: 100, + clientY: 200, + } as never) + }) + + expect(latest.dropIndicator).not.toBeNull() + }) + + it('clears the drop indicator when the pointer genuinely leaves the list', () => { + const scrollContainer = armDragOverScrollContainer() + expect(latest.dropIndicator).not.toBeNull() + + dispatchBubbledDragLeave(scrollContainer, 900) + + expect(latest.dropIndicator).toBeNull() + scrollContainer.remove() + }) + it('keeps isDragging active across dragOver updates until the drag ends', () => { act(() => { latest.createRootDropZone().onDragOver(fakeDragOverEvent() as never) }) expect(latest.isDragging).toBe(true) - // A subsequent dragOver must not tear down the active drag. act(() => { latest.createRootDropZone().onDragOver(fakeDragOverEvent() as never) }) @@ -227,7 +315,7 @@ describe('useDragDrop spring-open revert', () => { dragOverFolderUntilExpanded() mockSetExpanded.mockClear() - // Drop inside folder-1, then the drag ends as it always does. + // `dragend` fires after every drop, so the revert path runs here too. act(() => { void latest.createFolderDragHandlers('folder-1', null).onDrop(fakeDropEvent() as never) }) diff --git a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.ts b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.ts index cf71888d6b6..d75793042d9 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.ts +++ b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.ts @@ -1,10 +1,10 @@ import { useCallback, useEffect, useRef, useState } from 'react' import { toast } from '@sim/emcn' import { createLogger } from '@sim/logger' -import { getErrorMessage } from '@sim/utils/errors' import { noop } from '@sim/utils/helpers' import { useParams } from 'next/navigation' import { getFolderPath } from '@/lib/folders/tree' +import { compareByOrder } from '@/app/workspace/[workspaceId]/w/components/sidebar/utils' import { useReorderFolders } from '@/hooks/queries/folders' import { getFolderMap } from '@/hooks/queries/utils/folder-cache' import { getWorkflows } from '@/hooks/queries/utils/workflow-cache' @@ -52,17 +52,111 @@ function isSameFolderScope( } /** - * Sibling order within one folder scope. Matches `compareByOrder` in the sidebar's own utils so the - * indices this hook computes address the same slots the list renders. + * A reorder that did not fully commit. Carries whether the failure was partial rather than a + * ready-made sentence, so the copy is chosen where it is presented and raw transport errors from + * the underlying requests never reach the user. */ -function compareSiblingItems(a: SiblingItem, b: SiblingItem): number { - if (a.sortOrder !== b.sortOrder) return a.sortOrder - b.sortOrder - const timeA = a.createdAt.getTime() - const timeB = b.createdAt.getTime() - if (timeA !== timeB) return timeA - timeB - return a.id.localeCompare(b.id) +class ReorderFailedError extends Error { + readonly partial: boolean + + constructor(partial: boolean, causes: unknown[]) { + super(partial ? 'Reorder partially failed' : 'Reorder failed', { cause: causes }) + this.name = 'ReorderFailedError' + this.partial = partial + } +} + +/** The parts of a drag event this module needs, shared by React's synthetic event and the native one. */ +type DragLeaveLike = Pick + +/** + * Whether a drag has genuinely left `element`, rather than crossing one of its internal boundaries. + * + * `relatedTarget` alone cannot answer this. `dragleave` bubbles, so a listener sees one for every + * descendant the pointer leaves, and Chrome reports `relatedTarget` as `null` on all of them + * (Firefox populates it). Reading "no related node" as "left the element" therefore treated every + * internal crossing as an exit — which cleared the drop indicator mid-drag, and `handleDrop` bails + * on a null indicator, so releasing just after a crossing did nothing at all. Rows nested inside an + * expanded folder cross the most boundaries, which is why it looked like open folders broke + * dragging outright. + * + * The pointer position is the reliable signal on every engine, so it backs the null case. + */ +function hasDragLeftElement(element: HTMLElement, e: DragLeaveLike): boolean { + const related = e.relatedTarget as Node | null + if (related) return !element.contains(related) + const rect = element.getBoundingClientRect() + return !( + e.clientX >= rect.left && + e.clientX <= rect.right && + e.clientY >= rect.top && + e.clientY <= rect.bottom + ) +} + +/** Which half of a workflow row the pointer is in, deciding whether the drop line sits above or below. */ +function calculateDropPosition(e: React.DragEvent, element: HTMLElement): 'before' | 'after' { + const rect = element.getBoundingClientRect() + const midY = rect.top + rect.height / 2 + return e.clientY < midY ? 'before' : 'after' +} + +/** + * Folder rows take a third outcome: the middle band drops *into* the folder, while the outer + * quarters reorder around it. + */ +function calculateFolderDropPosition( + e: React.DragEvent, + element: HTMLElement +): 'before' | 'inside' | 'after' { + const rect = element.getBoundingClientRect() + const relativeY = e.clientY - rect.top + const height = rect.height + if (relativeY < height * 0.25) return 'before' + if (relativeY > height * 0.75) return 'after' + return 'inside' +} + +/** The folder a drop lands in: the target itself when dropping inside one, otherwise its scope. */ +function getDestinationFolderId(indicator: DropIndicator): string | null { + return indicator.position === 'inside' + ? indicator.targetId === 'root' + ? null + : indicator.targetId + : indicator.folderId +} + +/** + * Insert index into the list of siblings **excluding** moving items. Must use the full + * `siblingItems` list for lookup: when the drop line targets the dragged row, + * `indicator.targetId` is not present in `remaining`, so indexing `remaining` alone + * returns -1 and corrupts the splice. + */ +function getInsertIndexInRemaining( + siblingItems: SiblingItem[], + movingIds: Set, + indicator: DropIndicator +): number { + if (indicator.position === 'inside') { + return siblingItems.filter((s) => !movingIds.has(s.id)).length + } + + const targetIdx = siblingItems.findIndex((s) => s.id === indicator.targetId) + if (targetIdx === -1) { + return siblingItems.filter((s) => !movingIds.has(s.id)).length + } + + if (indicator.position === 'before') { + return siblingItems.slice(0, targetIdx).filter((s) => !movingIds.has(s.id)).length + } + + return siblingItems.slice(0, targetIdx + 1).filter((s) => !movingIds.has(s.id)).length } +/** Whether a drag has left the element the handler is bound to. See {@link hasDragLeftElement}. */ +const isLeavingElement = (e: React.DragEvent): boolean => + hasDragLeftElement(e.currentTarget, e) + export function useDragDrop(options: UseDragDropOptions = {}) { const { disabled = false } = options const [dropIndicator, setDropIndicator] = useState(null) @@ -85,14 +179,17 @@ export function useDragDrop(options: UseDragDropOptions = {}) { * the user opened themselves is never touched. */ const autoExpandedRef = useRef | null>(null) - /** Destination of the drop that just happened, read once by the drag-end cleanup. */ - const dropDestinationRef = useRef(null) const params = useParams() const workspaceId = params.workspaceId as string | undefined - const reorderWorkflowsMutation = useReorderWorkflows() - const reorderFoldersMutation = useReorderFolders() + /** + * Destructured because the mutation objects take a new identity on every state transition, so + * depending on them would re-create this hook's handler factories mid-drop and hand every sidebar + * row a fresh handler object. `mutateAsync` is stable in TanStack Query v5. + */ + const { mutateAsync: reorderWorkflows } = useReorderWorkflows() + const { mutateAsync: reorderFolders } = useReorderFolders() const setExpanded = useFolderStore((s) => s.setExpanded) const expandedFolders = useFolderStore((s) => s.expandedFolders) @@ -160,7 +257,8 @@ export function useDragDrop(options: UseDragDropOptions = {}) { if (expandedFolders.has(hoverFolderId)) return hoverExpandTimerRef.current = window.setTimeout(() => { - ;(autoExpandedRef.current ??= new Set()).add(hoverFolderId) + autoExpandedRef.current ??= new Set() + autoExpandedRef.current.add(hoverFolderId) setExpanded(hoverFolderId, true) }, HOVER_EXPAND_DELAY) @@ -172,61 +270,6 @@ export function useDragDrop(options: UseDragDropOptions = {}) { } }, [hoverFolderId, isDragging, expandedFolders, setExpanded]) - const calculateDropPosition = useCallback( - (e: React.DragEvent, element: HTMLElement): 'before' | 'after' => { - const rect = element.getBoundingClientRect() - const midY = rect.top + rect.height / 2 - return e.clientY < midY ? 'before' : 'after' - }, - [] - ) - - const calculateFolderDropPosition = useCallback( - (e: React.DragEvent, element: HTMLElement): 'before' | 'inside' | 'after' => { - const rect = element.getBoundingClientRect() - const relativeY = e.clientY - rect.top - const height = rect.height - if (relativeY < height * 0.25) return 'before' - if (relativeY > height * 0.75) return 'after' - return 'inside' - }, - [] - ) - - const getDestinationFolderId = useCallback((indicator: DropIndicator): string | null => { - return indicator.position === 'inside' - ? indicator.targetId === 'root' - ? null - : indicator.targetId - : indicator.folderId - }, []) - - /** - * Insert index into the list of siblings **excluding** moving items. Must use the full - * `siblingItems` list for lookup: when the drop line targets the dragged row, - * `indicator.targetId` is not present in `remaining`, so indexing `remaining` alone - * returns -1 and corrupts the splice. - */ - const getInsertIndexInRemaining = useCallback( - (siblingItems: SiblingItem[], movingIds: Set, indicator: DropIndicator): number => { - if (indicator.position === 'inside') { - return siblingItems.filter((s) => !movingIds.has(s.id)).length - } - - const targetIdx = siblingItems.findIndex((s) => s.id === indicator.targetId) - if (targetIdx === -1) { - return siblingItems.filter((s) => !movingIds.has(s.id)).length - } - - if (indicator.position === 'before') { - return siblingItems.slice(0, targetIdx).filter((s) => !movingIds.has(s.id)).length - } - - return siblingItems.slice(0, targetIdx + 1).filter((s) => !movingIds.has(s.id)).length - }, - [] - ) - const buildAndSubmitUpdates = useCallback( async ( targetWorkspaceId: string, @@ -249,44 +292,41 @@ export function useDragDrop(options: UseDragDropOptions = {}) { * rejection abandons the other request while it is still in flight and commits anyway, * leaving the caller unable to tell a total failure from a half-applied one. */ - const results = await Promise.allSettled([ - ...(folderUpdates.length > 0 - ? [ - reorderFoldersMutation.mutateAsync({ - workspaceId: targetWorkspaceId, - updates: folderUpdates, - }), - ] - : []), - ...(workflowUpdates.length > 0 - ? [ - reorderWorkflowsMutation.mutateAsync({ - workspaceId: targetWorkspaceId, - updates: workflowUpdates, - }), - ] - : []), - ]) - - const rejected = results.filter((r) => r.status === 'rejected') - if (rejected.length > 0) { - throw new AggregateError( - rejected.map((r) => r.reason), - rejected.length === results.length - ? 'Reorder failed' - : 'Reorder partially failed; some items kept their previous position' + const pending: Promise[] = [] + if (folderUpdates.length > 0) { + pending.push( + reorderFolders({ + workspaceId: targetWorkspaceId, + updates: folderUpdates, + }) ) } + if (workflowUpdates.length > 0) { + pending.push( + reorderWorkflows({ + workspaceId: targetWorkspaceId, + updates: workflowUpdates, + }) + ) + } + + const results = await Promise.allSettled(pending) + const rejected = results.filter((result) => result.status === 'rejected') + if (rejected.length === 0) return + + /** + * Whether the failure was partial only selects the message. Convergence needs nothing here: + * each mutation's own `onSettled` invalidates its list on error as well as success, so the + * committed side and the rolled-back side both refetch to server truth on their own. + */ + throw new ReorderFailedError( + rejected.length < results.length, + rejected.map((result) => result.reason) + ) }, - [reorderFoldersMutation, reorderWorkflowsMutation] + [reorderFolders, reorderWorkflows] ) - const isLeavingElement = useCallback((e: React.DragEvent): boolean => { - const relatedTarget = e.relatedTarget as HTMLElement | null - const currentTarget = e.currentTarget as HTMLElement - return !relatedTarget || !currentTarget.contains(relatedTarget) - }, []) - const initDragOver = useCallback( (e: React.DragEvent, stopPropagation = true): boolean => { e.preventDefault() @@ -336,7 +376,7 @@ export function useDragDrop(options: UseDragDropOptions = {}) { sortOrder: w.sortOrder, createdAt: w.createdAt, })), - ].sort(compareSiblingItems) + ].sort(compareByOrder) return siblings }, @@ -438,8 +478,8 @@ export function useDragDrop(options: UseDragDropOptions = {}) { } } - fromDestination.sort(compareSiblingItems) - fromOther.sort(compareSiblingItems) + fromDestination.sort(compareByOrder) + fromOther.sort(compareByOrder) return { fromDestination, fromOther } }, @@ -486,11 +526,16 @@ export function useDragDrop(options: UseDragDropOptions = {}) { } catch (error) { logger.error('Failed to drop selection:', error) /** - * Each mutation rolls its own slice of the cache back, so a failure is visible only as the - * rows silently returning to where they were. Say why, or it reads as the sidebar - * spontaneously undoing the move. + * Each mutation rolls its own slice of the cache back, so a failure is otherwise visible + * only as the rows silently returning to where they were — which reads as the sidebar + * spontaneously undoing the move. Copy is chosen here rather than carried on the error so + * transport-level text never reaches the user. */ - toast.error(getErrorMessage(error, 'Failed to move items')) + toast.error( + error instanceof ReorderFailedError && error.partial + ? 'Only some items moved' + : 'Failed to move items' + ) } }, [ @@ -515,10 +560,21 @@ export function useDragDrop(options: UseDragDropOptions = {}) { isDraggingRef.current = false setIsDragging(false) /** - * Recorded synchronously, before any `await`: `dragend` fires as soon as this handler - * returns to the event loop, and the cleanup there needs to know which folder to leave open. + * The destination and its ancestors stop being candidates for the drag-end collapse, so the + * folders holding the moved rows stay open. Done synchronously, before any `await`, because + * `dragend` fires as soon as this handler yields. Discarding ids rather than recording one to + * keep also means a destination missing from the folder map simply survives untouched. */ - dropDestinationRef.current = indicator ? getDestinationFolderId(indicator) : null + const destination = indicator ? getDestinationFolderId(indicator) : null + const autoExpanded = autoExpandedRef.current + if (destination && autoExpanded?.size) { + autoExpanded.delete(destination) + if (workspaceId) { + for (const folder of getFolderPath(getFolderMap(workspaceId), destination)) { + autoExpanded.delete(folder.id) + } + } + } if (!indicator) return @@ -535,7 +591,7 @@ export function useDragDrop(options: UseDragDropOptions = {}) { logger.error('Failed to handle drop:', error) } }, - [handleSelectionDrop, getDestinationFolderId] + [handleSelectionDrop, workspaceId] ) const createWorkflowDragHandlers = useCallback( @@ -557,7 +613,7 @@ export function useDragDrop(options: UseDragDropOptions = {}) { onDragLeave: () => {}, onDrop: handleDrop, }), - [initDragOver, calculateDropPosition, setNormalizedDropIndicator, handleDrop] + [initDragOver, setNormalizedDropIndicator, handleDrop] ) const createFolderDragHandlers = useCallback( @@ -587,13 +643,7 @@ export function useDragDrop(options: UseDragDropOptions = {}) { }, onDrop: handleDrop, }), - [ - initDragOver, - calculateFolderDropPosition, - setNormalizedDropIndicator, - isLeavingElement, - handleDrop, - ] + [initDragOver, setNormalizedDropIndicator, handleDrop] ) const createEmptyFolderDropZone = useCallback( @@ -635,7 +685,7 @@ export function useDragDrop(options: UseDragDropOptions = {}) { }, onDrop: handleDrop, }), - [initDragOver, setNormalizedDropIndicator, isLeavingElement, handleDrop] + [initDragOver, setNormalizedDropIndicator, handleDrop] ) const createEdgeDropZone = useCallback( @@ -670,30 +720,10 @@ export function useDragDrop(options: UseDragDropOptions = {}) { */ const collapseAutoExpandedFolders = useCallback(() => { const autoExpanded = autoExpandedRef.current - const destination = dropDestinationRef.current - dropDestinationRef.current = null if (!autoExpanded?.size) return - - /** - * The destination is seeded directly rather than relying on the ancestor walk to include it: - * `getFolderPath` returns an empty chain for an id missing from the folder map, which would - * otherwise close the very folder the drop just landed in. - */ - const keepOpen = new Set() - if (destination) { - keepOpen.add(destination) - if (workspaceId) { - for (const folder of getFolderPath(getFolderMap(workspaceId), destination)) { - keepOpen.add(folder.id) - } - } - } - - for (const folderId of autoExpanded) { - if (!keepOpen.has(folderId)) setExpanded(folderId, false) - } + for (const folderId of autoExpanded) setExpanded(folderId, false) autoExpanded.clear() - }, [workspaceId, setExpanded]) + }, [setExpanded]) const handleDragEnd = useCallback(() => { isDraggingRef.current = false @@ -710,8 +740,7 @@ export function useDragDrop(options: UseDragDropOptions = {}) { const container = scrollContainerRef.current if (!container) return const onLeave = (e: DragEvent) => { - const related = e.relatedTarget as Node | null - if (related && container.contains(related)) return + if (!hasDragLeftElement(container, e)) return if (scrollAnimationRef.current !== null) { cancelAnimationFrame(scrollAnimationRef.current) scrollAnimationRef.current = null From 3250c575190e8f294aebb41be87c79e5d0c202d7 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Thu, 13 Aug 2026 15:05:04 -0700 Subject: [PATCH 3/3] fix(sidebar): disarm the spring-open timer when a drag ends --- .../sidebar/hooks/use-drag-drop.test.tsx | 37 +++++++++++++++++++ .../components/sidebar/hooks/use-drag-drop.ts | 10 +++++ 2 files changed, 47 insertions(+) diff --git a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.test.tsx b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.test.tsx index dfe3bb37c71..f93b1d9b93a 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.test.tsx @@ -327,6 +327,43 @@ describe('useDragDrop spring-open revert', () => { expect(expandedFolders.has('folder-1')).toBe(true) }) + /** + * The spring-open timer is armed for 400ms, so a drag ending just before it fires leaves it + * pending. Relying on the effect cleanup to cancel it would let it land after the drag-end + * collapse had already emptied the set — re-adding the folder for the *next* drag to close, by + * which point the user had opened it themselves. + */ + it('does not spring-open a folder when the drag ends before the timer fires', () => { + act(() => { + latest.handleDragStart(null) + }) + act(() => { + latest + .createFolderDragHandlers('folder-1', null) + .onDragOver(fakeFolderDragOverEvent() as never) + }) + + /** + * Deliberately outside `act`: the race only exists while React has scheduled the drag-end state + * changes but not yet committed them, so the effect cleanup has not run and the timer is still + * armed. Wrapping this in `act` would flush the commit first and cancel the timer via the + * cleanup, hiding the very gap under test. + */ + latest.handleDragEnd() + act(() => { + vi.advanceTimersByTime(500) + }) + + expect(mockSetExpanded).not.toHaveBeenCalledWith('folder-1', true) + expect(expandedFolders.has('folder-1')).toBe(false) + + // Nothing was left behind for a later drag to collapse. + act(() => { + latest.handleDragEnd() + }) + expect(mockSetExpanded).not.toHaveBeenCalledWith('folder-1', false) + }) + it('never closes a folder the user had already opened themselves', () => { expandedFolders.add('folder-1') diff --git a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.ts b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.ts index d75793042d9..af8cb9b5731 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.ts +++ b/apps/sim/app/workspace/[workspaceId]/w/components/sidebar/hooks/use-drag-drop.ts @@ -732,6 +732,16 @@ export function useDragDrop(options: UseDragDropOptions = {}) { setDropIndicator(null) draggedSourceFolderRef.current = null setHoverFolderId(null) + /** + * Disarmed here rather than left to the effect cleanup, which only runs once React commits the + * state changes above. A timer due within that gap would otherwise fire after the collapse + * below, spring-opening a folder for a drag that already ended and leaving it in the set for + * the next drag to close — a folder the user, by then, opened themselves. + */ + if (hoverExpandTimerRef.current) { + clearTimeout(hoverExpandTimerRef.current) + hoverExpandTimerRef.current = null + } collapseAutoExpandedFolders() }, [collapseAutoExpandedFolders])