diff --git a/apps/sim/app/workspace/[workspaceId]/components/folders/use-folder-row-drag-drop.ts b/apps/sim/app/workspace/[workspaceId]/components/folders/use-folder-row-drag-drop.ts index 30769d0c264..4ac69bb78b8 100644 --- a/apps/sim/app/workspace/[workspaceId]/components/folders/use-folder-row-drag-drop.ts +++ b/apps/sim/app/workspace/[workspaceId]/components/folders/use-folder-row-drag-drop.ts @@ -12,12 +12,60 @@ import type { SpringOpenOptions } from '@/app/workspace/[workspaceId]/components import { useSpringNavigation } from '@/app/workspace/[workspaceId]/components/folders/use-spring-navigation' import type { RowDragDropConfig } from '@/app/workspace/[workspaceId]/components/resource/resource' -/** The foldered-list drag MIME — see {@link writeRowDragPayload} for why each surface owns one. */ -const DRAG_ROW_MIME = 'application/x-sim-foldered-row' +/** + * What the hook hands back: the render contract `Resource` consumes, plus the one signal that is + * not a rendering concern. + */ +export interface FolderRowDragDrop extends RowDragDropConfig { + /** + * Reports that a page-level overlay consumed an external drop, so spring navigation keeps the + * folder it opened instead of returning to where the drag started. Only a surface that owns a + * whole-page drop target needs this; row, body, and breadcrumb drops report themselves. + */ + externalDropHandled: () => void +} /** Shared empty set so an idle drag state keeps a stable identity across renders. */ const EMPTY_ROW_IDS = new Set() +/** + * The one surface currently reading as "release here". + * + * A union rather than three booleans because the three targets are mutually exclusive: a row, + * the list body, and a breadcrumb crumb can never be armed together. As separate flags every + * handler had to hand-clear the other two, and where a `dragleave` does not fire — a row lives + * inside the scroll container, so moving onto it leaves that container with a contained + * `relatedTarget` its handler ignores — two affordances could paint at once. Here exactly one + * is armed by construction. + */ +type ActiveDropTarget = + | { kind: 'row'; rowId: string } + | { kind: 'body' } + | { kind: 'crumb'; index: number } + +/** + * Arms `next`, reusing the current value when it already names the same target. + * + * `dragover` fires continuously — several times a second even with the pointer still — so a + * fresh object per event would re-render the whole list and rebuild the memoized config every + * time. Returning `current` unchanged lets React bail on `Object.is`, which is what the plain + * string this union replaced used to get for free. + */ +function armDropTarget( + current: ActiveDropTarget | null, + next: ActiveDropTarget +): ActiveDropTarget | null { + if (current?.kind !== next.kind) return next + switch (next.kind) { + case 'row': + return current.kind === 'row' && current.rowId === next.rowId ? current : next + case 'crumb': + return current.kind === 'crumb' && current.index === next.index ? current : next + default: + return current + } +} + /** Rows carried by one drag, already split by kind and stripped of no-op moves. */ export interface FolderedRowMove { folderIds: string[] @@ -25,6 +73,11 @@ export interface FolderedRowMove { } export interface UseFolderRowDragDropOptions { + /** + * This list's private drag MIME. Each surface owns one so a drag started in another list is + * never mistaken for one of these rows — see {@link writeRowDragPayload}. + */ + dragMime: string /** Drag and drop are edits; a reader gets neither draggable rows nor drop targets. */ canEdit: boolean /** Row currently being renamed inline, which must stay editable rather than draggable. */ @@ -68,6 +121,19 @@ export interface UseFolderRowDragDropOptions { * empty folder, which has no row to drop on. */ currentFolderId?: string | null + /** + * OS file drops, which Files accepts and the other lists do not. + * + * When `matches` recognises the drag, folder rows still highlight and still spring open — the + * gesture is the same, only the payload differs — but the internal move-validity gate is + * skipped, and the body and breadcrumb decline so a page-level upload overlay owns those + * regions rather than competing with it. + */ + externalDrop?: { + matches: (dataTransfer: DataTransfer) => boolean + /** Files released on a folder row, to be uploaded into it. */ + onDropIntoFolder: (dataTransfer: DataTransfer, targetFolderId: string) => void + } } /** @@ -76,10 +142,11 @@ export interface UseFolderRowDragDropOptions { * itself or its own subtree, and a row already sitting directly in the target is a no-op. * * Carries a whole checkbox selection when `selection` is supplied, and a single row otherwise. - * The Files page keeps its own configuration because it additionally accepts external OS file - * drops, which need a second drag protocol this hook deliberately does not know about. + * Files layers OS file drops on top through `externalDrop`; the gesture is identical, only the + * payload differs. */ export function useFolderRowDragDrop({ + dragMime, canEdit, editingRowId, descendantsByFolderId, @@ -90,10 +157,9 @@ export function useFolderRowDragDrop({ selection, onSpringOpenFolder, currentFolderId = null, -}: UseFolderRowDragDropOptions): RowDragDropConfig { - const [activeDropTargetId, setActiveDropTargetId] = useState(null) - const [isBodyDropActive, setIsBodyDropActive] = useState(false) - const [activeBreadcrumbIndex, setActiveBreadcrumbIndex] = useState(null) + externalDrop, +}: UseFolderRowDragDropOptions): FolderRowDragDrop { + const [activeDropTarget, setActiveDropTarget] = useState(null) const [draggedRowIds, setDraggedRowIds] = useState>(() => EMPTY_ROW_IDS) /** * The in-flight drag source, mirrored outside React state because `onDragOver` fires far @@ -109,6 +175,7 @@ export function useFolderRowDragDrop({ getRowLabel, onMoveRows, selection, + externalDrop, }) optionsRef.current = { descendantsByFolderId, @@ -117,6 +184,7 @@ export function useFolderRowDragDrop({ getRowLabel, onMoveRows, selection, + externalDrop, } const springNav = useSpringNavigation({ currentFolderId, onNavigate: onSpringOpenFolder }) @@ -132,9 +200,7 @@ export function useFolderRowDragDrop({ dragGhost.remove() draggedRowIdsRef.current = [] setDraggedRowIds(EMPTY_ROW_IDS) - setActiveDropTargetId(null) - setIsBodyDropActive(false) - setActiveBreadcrumbIndex(null) + setActiveDropTarget(null) }, [dragGhost, springNav]) useDragTeardown(endDrag) @@ -185,9 +251,9 @@ export function useFolderRowDragDrop({ [resolveMoveToFolder] ) - return useMemo( + return useMemo( () => ({ - activeDropTargetId, + activeDropTargetId: activeDropTarget?.kind === 'row' ? activeDropTarget.rowId : null, draggedRowIds, isAnyDragActive: draggedRowIds.size > 0, isRowDraggable: (rowId) => canEdit && editingRowId !== rowId, @@ -215,15 +281,29 @@ export function useFolderRowDragDrop({ setDraggedRowIds(new Set(sourceRowIds)) e.dataTransfer.effectAllowed = 'move' - writeRowDragPayload(e.dataTransfer, DRAG_ROW_MIME, sourceRowIds) + writeRowDragPayload(e.dataTransfer, dragMime, sourceRowIds) dragGhost.attach(e, optionsRef.current.getRowLabel(sourceRowIds[0]), sourceRowIds.length) }, onDragOver: (e: DragEvent, rowId) => { const sourceRowIds = draggedRowIdsRef.current + const isExternal = optionsRef.current.externalDrop?.matches(e.dataTransfer) ?? false + if (isExternal) { + /** + * An upload into a nested folder is the same gesture as a move into one, so the row + * highlights and springs open exactly the same way. Only the move-validity gate is + * skipped — there are no source rows to validate. + */ + e.preventDefault() + e.stopPropagation() + e.dataTransfer.dropEffect = 'copy' + setActiveDropTarget((current) => armDropTarget(current, { kind: 'row', rowId })) + springNav.arm(parseFolderedRowId(rowId).id) + return + } if (sourceRowIds.length > 0) { if (!resolveMove(rowId, sourceRowIds)) return - } else if (!e.dataTransfer.types.includes(DRAG_ROW_MIME)) { + } else if (!e.dataTransfer.types.includes(dragMime)) { /** * No local source and no payload of ours — an external or foreign drag. Returning * without `preventDefault` leaves the browser's default handling in place, which is @@ -245,14 +325,7 @@ export function useFolderRowDragDrop({ * descendants — and the drop would then silently do nothing. */ if (sourceRowIds.length > 0) { - setActiveDropTargetId(rowId) - /** - * The row is inside the scroll container, so moving onto it fires `dragleave` there - * with a contained `relatedTarget` — which that handler deliberately ignores. Without - * clearing here the row and the body would both render as the target at once. - */ - setIsBodyDropActive(false) - setActiveBreadcrumbIndex(null) + setActiveDropTarget((current) => armDropTarget(current, { kind: 'row', rowId })) /** * Armed on the same condition as the highlight, so a folder only springs open where a * drop was already possible. A folder the drag cannot legally enter never opens. @@ -264,17 +337,31 @@ export function useFolderRowDragDrop({ const relatedTarget = e.relatedTarget if (relatedTarget instanceof Node && e.currentTarget.contains(relatedTarget)) return springNav.disarm() - setActiveDropTargetId((current) => (current === rowId ? null : current)) + setActiveDropTarget((current) => + current?.kind === 'row' && current.rowId === rowId ? null : current + ) }, onDrop: (e: DragEvent, rowId) => { e.preventDefault() e.stopPropagation() const target = parseFolderedRowId(rowId) + const { externalDrop } = optionsRef.current + if (externalDrop?.matches(e.dataTransfer)) { + const { dataTransfer } = e + /** + * Marked before `endDrag`, which consumes the flag: an upload lands in the folder the + * drag opened, so the view has to stay there rather than springing back to the origin. + */ + if (target.kind === 'folder') springNav.markDropHandled() + endDrag() + if (target.kind === 'folder') externalDrop.onDropIntoFolder(dataTransfer, target.id) + return + } // Prefer the dataTransfer payload over the ref so a drag that started in another // mount of this page still resolves to real row ids. const sourceRowIds = - readRowDragPayload(e.dataTransfer, DRAG_ROW_MIME) ?? draggedRowIdsRef.current + readRowDragPayload(e.dataTransfer, dragMime) ?? draggedRowIdsRef.current const move = target.kind === 'folder' && sourceRowIds.length > 0 ? resolveMove(rowId, sourceRowIds) @@ -291,6 +378,7 @@ export function useFolderRowDragDrop({ if (move) optionsRef.current.onMoveRows(move, target.id) }, onDragEnd: endDrag, + externalDropHandled: springNav.markDropHandled, /** * The breadcrumb is how a drag walks back UP. Spring-loading only ever goes deeper, so * without this a drag that entered a folder can only leave it by being abandoned. @@ -298,21 +386,22 @@ export function useFolderRowDragDrop({ * one files the drag there directly. */ breadcrumb: { - activeIndex: activeBreadcrumbIndex, + activeIndex: activeDropTarget?.kind === 'crumb' ? activeDropTarget.index : null, onDragOver: (e: DragEvent, folderId: string | null, index: number) => { + if (optionsRef.current.externalDrop?.matches(e.dataTransfer)) return const sourceRowIds = draggedRowIdsRef.current const canDrop = sourceRowIds.length > 0 && resolveMoveToFolder(folderId, sourceRowIds) !== null /** * Armed even when the drop itself would be a no-op — walking back through a crumb the * rows already live in is exactly how a user returns to where they started, and - * refusing to navigate there would strand them. + * refusing to navigate there would strand them. The crumb for the folder already on + * screen is declined by {@link useSpringNavigation}, not here. */ - if (sourceRowIds.length > 0 && folderId !== currentFolderIdRef.current) { - springNav.arm(folderId) - } - setActiveBreadcrumbIndex(canDrop ? index : null) - setIsBodyDropActive(false) + if (sourceRowIds.length > 0) springNav.arm(folderId) + setActiveDropTarget((current) => + canDrop ? armDropTarget(current, { kind: 'crumb', index }) : null + ) if (!canDrop) return e.preventDefault() e.stopPropagation() @@ -320,13 +409,16 @@ export function useFolderRowDragDrop({ }, onDragLeave: (_e: DragEvent, index: number) => { springNav.disarm() - setActiveBreadcrumbIndex((current) => (current === index ? null : current)) + setActiveDropTarget((current) => + current?.kind === 'crumb' && current.index === index ? null : current + ) }, onDrop: (e: DragEvent, folderId: string | null) => { + if (optionsRef.current.externalDrop?.matches(e.dataTransfer)) return e.preventDefault() e.stopPropagation() const sourceRowIds = - readRowDragPayload(e.dataTransfer, DRAG_ROW_MIME) ?? draggedRowIdsRef.current + readRowDragPayload(e.dataTransfer, dragMime) ?? draggedRowIdsRef.current const move = sourceRowIds.length > 0 ? resolveMoveToFolder(folderId, sourceRowIds) : null if (move) springNav.markDropHandled() endDrag() @@ -334,8 +426,10 @@ export function useFolderRowDragDrop({ }, }, body: { - isActive: isBodyDropActive, + isActive: activeDropTarget?.kind === 'body', onDragOver: (e: DragEvent) => { + /** Declined: a page-level upload overlay owns the whole region for an OS file drag. */ + if (optionsRef.current.externalDrop?.matches(e.dataTransfer)) return const sourceRowIds = draggedRowIdsRef.current /** * Recomputed on every event rather than latched, because a spring-open changes the @@ -346,7 +440,9 @@ export function useFolderRowDragDrop({ const canDrop = sourceRowIds.length > 0 && resolveMoveToFolder(currentFolderIdRef.current, sourceRowIds) !== null - setIsBodyDropActive(canDrop) + setActiveDropTarget((current) => + canDrop ? armDropTarget(current, { kind: 'body' }) : null + ) if (!canDrop) return e.preventDefault() e.dataTransfer.dropEffect = 'move' @@ -354,13 +450,14 @@ export function useFolderRowDragDrop({ onDragLeave: (e: DragEvent) => { const relatedTarget = e.relatedTarget if (relatedTarget instanceof Node && e.currentTarget.contains(relatedTarget)) return - setIsBodyDropActive(false) + setActiveDropTarget((current) => (current?.kind === 'body' ? null : current)) }, onDrop: (e: DragEvent) => { + if (optionsRef.current.externalDrop?.matches(e.dataTransfer)) return e.preventDefault() - const sourceRowIds = - readRowDragPayload(e.dataTransfer, DRAG_ROW_MIME) ?? draggedRowIdsRef.current e.stopPropagation() + const sourceRowIds = + readRowDragPayload(e.dataTransfer, dragMime) ?? draggedRowIdsRef.current /** * Read from the ref, not the closure. This config is memoized, and during a drag the * only dep that routinely changes is the hovered row — so after a spring-open into an @@ -377,11 +474,10 @@ export function useFolderRowDragDrop({ }, }), [ - activeDropTargetId, - isBodyDropActive, - activeBreadcrumbIndex, + activeDropTarget, draggedRowIds, canEdit, + dragMime, editingRowId, resolveMove, resolveMoveToFolder, diff --git a/apps/sim/app/workspace/[workspaceId]/components/folders/use-spring-loaded-folder.test.tsx b/apps/sim/app/workspace/[workspaceId]/components/folders/use-spring-loaded-folder.test.tsx index fd20ab72c35..e2b3b9d5278 100644 --- a/apps/sim/app/workspace/[workspaceId]/components/folders/use-spring-loaded-folder.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/components/folders/use-spring-loaded-folder.test.tsx @@ -121,19 +121,25 @@ describe('useSpringLoadedFolder', () => { expect(onSpringOpen).not.toHaveBeenCalled() }) - it('opens a folder at most once per drag', () => { + it('opens a folder again when the drag comes back to it', () => { + // Descend, walk back out through the breadcrumb, change your mind and descend again — one + // gesture, and the second entry has to work. Re-entry costs another full delay, and + // `useSpringNavigation` refuses the folder already on screen, so nothing oscillates. const onSpringOpen = vi.fn() const harness = renderSpringLoad(onSpringOpen) act(() => harness.get().arm('folder-a')) rest() - expect(onSpringOpen).toHaveBeenCalledTimes(1) - - // Dragging back out and returning must not re-open it, which would loop at a boundary. - act(() => harness.get().arm('folder-b')) + act(() => harness.get().arm(null)) + rest() act(() => harness.get().arm('folder-a')) rest() - expect(onSpringOpen).toHaveBeenCalledTimes(1) + + expect(onSpringOpen.mock.calls).toEqual([ + ['folder-a', { history: 'push' }], + [null, { history: 'replace' }], + ['folder-a', { history: 'replace' }], + ]) }) it('pushes the first spring-open of a drag and replaces the rest', () => { @@ -197,17 +203,21 @@ describe('useSpringLoadedFolder', () => { expect(onSpringOpen).toHaveBeenCalledExactlyOnceWith(null, { history: 'push' }) }) - it('opens the root at most once per drag, like any other folder', () => { + it('re-opens the root like any other folder, and only after a full rest', () => { const onSpringOpen = vi.fn() const harness = renderSpringLoad(onSpringOpen) act(() => harness.get().arm(null)) rest() + + // Passing over another row cancels the countdown, so returning to the root has to wait out + // the delay again rather than firing on whatever was left of the previous one. act(() => harness.get().arm('folder-a')) act(() => harness.get().arm(null)) + expect(onSpringOpen).toHaveBeenCalledTimes(1) rest() - expect(onSpringOpen).toHaveBeenCalledTimes(1) + expect(onSpringOpen).toHaveBeenCalledTimes(2) }) it('never opens a folder after unmount', () => { diff --git a/apps/sim/app/workspace/[workspaceId]/components/folders/use-spring-loaded-folder.ts b/apps/sim/app/workspace/[workspaceId]/components/folders/use-spring-loaded-folder.ts index c4d07122d7f..3f39c951fc0 100644 --- a/apps/sim/app/workspace/[workspaceId]/components/folders/use-spring-loaded-folder.ts +++ b/apps/sim/app/workspace/[workspaceId]/components/folders/use-spring-loaded-folder.ts @@ -40,7 +40,7 @@ export interface SpringLoadedFolder { arm: (folderId: string | null) => void /** Cancels the pending open — the drag left the row, or the row stopped being a valid target. */ disarm: () => void - /** Cancels the pending open and forgets which folders already opened. Call when the drag ends. */ + /** Cancels the pending open and forgets that this drag opened anything. Call when the drag ends. */ reset: () => void } @@ -51,9 +51,11 @@ export interface SpringLoadedFolder { * The dragged rows unmount when the list re-renders into the newly opened folder, which is why * the drag payload has to live in `dataTransfer` rather than only in the source row's state. * - * A folder opens at most once per drag. Without that, dragging back out to a parent and - * returning would re-open it on a loop, and a drag that rests near a boundary would flicker - * between two levels. + * A folder may open more than once in a single drag: walking back out through the breadcrumb and + * descending again is a normal way to change your mind mid-gesture, and refusing the second entry + * strands the drag one level up. Nothing oscillates, because every open costs another full + * {@link SPRING_LOAD_DELAY_MS} of the drag holding still, and {@link useSpringNavigation} refuses + * to arm the folder already on screen. */ export function useSpringLoadedFolder({ onSpringOpen, @@ -65,9 +67,8 @@ export function useSpringLoadedFolder({ * nothing is armed — `null` is a real destination here, the workspace root. */ const armedFolderIdRef = useRef(undefined) - /** Folders already opened during this drag; each may only spring once. */ - const openedFolderIdsRef = useRef | null>(null) - const openedFolderIds = (openedFolderIdsRef.current ??= new Set()) + /** Whether this drag has already sprung a folder open, which decides push vs. replace. */ + const hasOpenedRef = useRef(false) const onSpringOpenRef = useRef(onSpringOpen) onSpringOpenRef.current = onSpringOpen @@ -91,27 +92,24 @@ export function useSpringLoadedFolder({ * this would let the folder the drag just left open behind the cursor. */ clearTimer() - if (openedFolderIds.has(folderId)) return - armedFolderIdRef.current = folderId timerRef.current = setTimeout(() => { timerRef.current = null armedFolderIdRef.current = undefined - /** Read before the add: an empty set means nothing has opened in this drag yet. */ - const isFirstOpenOfDrag = openedFolderIds.size === 0 - openedFolderIds.add(folderId) + const isFirstOpenOfDrag = !hasOpenedRef.current + hasOpenedRef.current = true onSpringOpenRef.current(folderId, { history: isFirstOpenOfDrag ? 'push' : 'replace', }) }, delayMs) }, - [clearTimer, delayMs, openedFolderIds] + [clearTimer, delayMs] ) const reset = useCallback(() => { clearTimer() - openedFolderIds.clear() - }, [clearTimer, openedFolderIds]) + hasOpenedRef.current = false + }, [clearTimer]) /** * Stable identity, not a fresh object per render. Consumers feed this handle into a diff --git a/apps/sim/app/workspace/[workspaceId]/components/folders/use-spring-navigation.test.tsx b/apps/sim/app/workspace/[workspaceId]/components/folders/use-spring-navigation.test.tsx index 8b49b7fc3c2..6e3590e2d48 100644 --- a/apps/sim/app/workspace/[workspaceId]/components/folders/use-spring-navigation.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/components/folders/use-spring-navigation.test.tsx @@ -68,6 +68,12 @@ function rest(harness: { rerender: () => void }) { harness.rerender() } +/** One spring-open: rest the drag on `folderId` until the timer fires and the list follows. */ +function descend(nav: ReturnType, folderId: string | null) { + act(() => nav.get().arm(folderId)) + rest(nav) +} + beforeEach(() => { vi.useFakeTimers() }) @@ -155,6 +161,130 @@ describe('useSpringNavigation', () => { expect(nav.navigate).not.toHaveBeenCalled() }) + describe('walking a drag back out and in again', () => { + it('re-enters a folder it already left through the breadcrumb', () => { + // The whole point of the breadcrumb accepting a drag: descend, think better of it, walk + // back up, then descend again — all inside one gesture without releasing the mouse. + const nav = renderSpringNavigation(null) + + act(() => nav.get().rememberOrigin()) + descend(nav, 'folder-a') + expect(nav.currentFolderId()).toBe('folder-a') + + descend(nav, null) + expect(nav.currentFolderId()).toBeNull() + + descend(nav, 'folder-a') + expect(nav.currentFolderId()).toBe('folder-a') + + expect(nav.navigate.mock.calls).toEqual([ + ['folder-a', 'push'], + [null, 'replace'], + ['folder-a', 'replace'], + ]) + }) + + it('never re-opens the folder already on screen', () => { + // The crumb for the current folder is a legal drop target but not a navigation. Arming it + // would re-enter the folder the drag is already standing in, on a loop. + const nav = renderSpringNavigation(null) + + act(() => nav.get().rememberOrigin()) + descend(nav, 'folder-a') + + descend(nav, 'folder-a') + + expect(nav.navigate).toHaveBeenCalledExactlyOnceWith('folder-a', 'push') + }) + + it('cancels a pending open when the drag moves onto the current folder', () => { + // Hovering a sibling folder starts its countdown; sliding onto the crumb of the folder + // you are already in has to call that off, not let it fire from under the cursor. + const nav = renderSpringNavigation('folder-a') + + act(() => nav.get().rememberOrigin()) + act(() => nav.get().arm('folder-b')) + act(() => { + vi.advanceTimersByTime(SPRING_LOAD_DELAY_MS - 1) + }) + descend(nav, 'folder-a') + + expect(nav.navigate).not.toHaveBeenCalled() + }) + + it('returns to the origin in one hop after a round trip that dropped nothing', () => { + const nav = renderSpringNavigation('origin') + + act(() => nav.get().rememberOrigin()) + descend(nav, 'folder-a') + descend(nav, 'folder-b') + descend(nav, 'folder-a') + + nav.navigate.mockClear() + act(() => nav.get().end()) + + expect(nav.navigate).toHaveBeenCalledExactlyOnceWith('origin', 'replace') + expect(nav.currentFolderId()).toBe('origin') + }) + + it('stays put when the round trip ends in a real drop', () => { + const nav = renderSpringNavigation('origin') + + act(() => nav.get().rememberOrigin()) + descend(nav, 'folder-a') + descend(nav, null) + descend(nav, 'folder-a') + + nav.navigate.mockClear() + act(() => { + nav.get().markDropHandled() + nav.get().end() + }) + + expect(nav.navigate).not.toHaveBeenCalled() + expect(nav.currentFolderId()).toBe('folder-a') + }) + + it('walks back to the origin folder itself without then bouncing away from it', () => { + // Ending a drag whose spring-opens happen to land back on the origin must not navigate + // again — the guard is origin-vs-current, not "did anything open". + const nav = renderSpringNavigation('origin') + + act(() => nav.get().rememberOrigin()) + descend(nav, 'folder-a') + descend(nav, 'origin') + expect(nav.currentFolderId()).toBe('origin') + + nav.navigate.mockClear() + act(() => nav.get().end()) + + expect(nav.navigate).not.toHaveBeenCalled() + }) + + it('starts the next drag from where the previous one left the user', () => { + // A drag that ended on a new folder is the new origin. Reusing the old one would yank the + // list back several folders on the next unrelated drag. + const nav = renderSpringNavigation('origin') + + act(() => nav.get().rememberOrigin()) + descend(nav, 'folder-a') + act(() => { + nav.get().markDropHandled() + nav.get().end() + }) + + nav.navigate.mockClear() + act(() => nav.get().rememberOrigin()) + descend(nav, 'folder-b') + act(() => nav.get().end()) + + expect(nav.navigate.mock.calls).toEqual([ + ['folder-b', 'push'], + ['folder-a', 'replace'], + ]) + }) + }) + it('does not carry drop state into the next drag', () => { const nav = renderSpringNavigation(null) diff --git a/apps/sim/app/workspace/[workspaceId]/components/folders/use-spring-navigation.ts b/apps/sim/app/workspace/[workspaceId]/components/folders/use-spring-navigation.ts index adbe048d26c..1e0e208f4ed 100644 --- a/apps/sim/app/workspace/[workspaceId]/components/folders/use-spring-navigation.ts +++ b/apps/sim/app/workspace/[workspaceId]/components/folders/use-spring-navigation.ts @@ -38,8 +38,7 @@ export interface SpringNavigation { * treated as part of the drag: unless a drop actually landed, ending the drag returns to where * it started. The workflow sidebar collapses its own spring-opened folders for the same reason. * - * Shared by every foldered list. Files keeps its own drag configuration for OS file drops, but - * this lifecycle is identical everywhere. + * Shared by every foldered list, including a drag of OS files onto the Files page. */ export function useSpringNavigation({ currentFolderId, @@ -73,16 +72,25 @@ export function useSpringNavigation({ * Seeds the origin for a drag that never reached {@link SpringNavigation.rememberOrigin} — a * drag of OS files starts outside the page, so there is no `dragstart` of ours to record it. * Without this the return lands on whatever folder the PREVIOUS drag began in. + * + * Refuses the folder already on screen. That target is not a navigation, and arming it is how + * a drag resting on one spot would re-open the same folder over and over: the underlying timer + * lets a folder spring more than once per drag so the user can descend, back out through the + * breadcrumb, and descend again. */ const arm = useCallback( (folderId: string | null) => { + if (folderId === currentFolderIdRef.current) { + springLoad.disarm() + return + } if (!hasOriginRef.current) { originFolderIdRef.current = currentFolderIdRef.current hasOriginRef.current = true } springLoad.arm(folderId) }, - [springLoad.arm] + [springLoad.arm, springLoad.disarm] ) const markDropHandled = useCallback(() => { diff --git a/apps/sim/app/workspace/[workspaceId]/components/resource/resource.tsx b/apps/sim/app/workspace/[workspaceId]/components/resource/resource.tsx index 0d52e9afc35..31ca47ae1bc 100644 --- a/apps/sim/app/workspace/[workspaceId]/components/resource/resource.tsx +++ b/apps/sim/app/workspace/[workspaceId]/components/resource/resource.tsx @@ -712,13 +712,7 @@ const DataRow = memo(function DataRow({ onRowClick && 'cursor-pointer', isDraggable && 'cursor-grab active:cursor-grabbing', isRowActive && chipActiveSurfaceClass, - /** - * Neutral, matching the workflow sidebar's own drop-inside affordance - * (`bg-[var(--text-subtle)] opacity-10` there, and `--text-subtle` for its reorder - * line). A brand colour here would be the only place in the app that signals "release - * here" with hue rather than weight. Drawn inside the row's own box - * (`outline-offset-[-1px]`) so the ring never overlaps the rows above and below. - */ + /** See {@link chipDropTargetSurfaceClass} for why this is neutral and drawn inset. */ isActiveDropTarget && chipDropTargetSurfaceClass, (isDragging || (isAnyDragActive && isSelected && !isActiveDropTarget)) && 'opacity-50' )} diff --git a/apps/sim/app/workspace/[workspaceId]/files/files.tsx b/apps/sim/app/workspace/[workspaceId]/files/files.tsx index 6e43ec3d53f..b29e169914b 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/files.tsx +++ b/apps/sim/app/workspace/[workspaceId]/files/files.tsx @@ -1,6 +1,6 @@ 'use client' -import { type DragEvent, useCallback, useEffect, useMemo, useRef, useState } from 'react' +import { useCallback, useEffect, useMemo, useRef, useState } from 'react' import { Button, ChipCombobox, @@ -51,7 +51,6 @@ import type { ResourceAction, ResourceColumn, ResourceRow, - RowDragDropConfig, SearchConfig, SortConfig, } from '@/app/workspace/[workspaceId]/components' @@ -76,13 +75,12 @@ import { FOLDERED_RESOURCE_HEADERS, folderBreadcrumbItems, folderedResourceListHref, + folderRowId, + parseFolderedRowId, parseMoveOptionValue, - readRowDragPayload, sortResources, - useDragTeardown, - useRowDragGhost, - useSpringNavigation, - writeRowDragPayload, + splitFolderedRowIds, + useFolderRowDragDrop, } from '@/app/workspace/[workspaceId]/components/folders' import { ResourceActionBar } from '@/app/workspace/[workspaceId]/components/resource/components/action-bar' import { DeleteConfirmModal } from '@/app/workspace/[workspaceId]/files/components/delete-confirm-modal' @@ -152,14 +150,11 @@ type FileListEntry = const logger = createLogger('Files') /** - * Private drag payload for file rows, kept distinct from the foldered-list MIME so a drag - * started on Tables or Knowledge is never mistaken for one of these rows. + * This list's private drag MIME, so a drag started on another list is never mistaken for one of + * these rows. */ const FILE_ROW_DRAG_MIME = 'application/x-sim-workspace-file-rows' -/** Shared empty set so an idle drag state keeps a stable identity across renders. */ -const EMPTY_DRAGGED_ROW_IDS = new Set() - const FILES_HEADER = FOLDERED_RESOURCE_HEADERS.file const FOLDER_ICON = @@ -211,14 +206,6 @@ const MIME_TYPE_LABELS: Record = { const EMPTY_WORKSPACE_FILES: WorkspaceFileRecord[] = [] const EMPTY_WORKSPACE_FILE_FOLDERS: WorkspaceFileFolderApi[] = [] -const fileRowId = (id: string) => `file:${id}` -const folderRowId = (id: string) => `folder:${id}` -const parseRowId = (rowId: string): { kind: 'file' | 'folder'; id: string } => { - if (rowId.startsWith('folder:')) return { kind: 'folder', id: rowId.slice('folder:'.length) } - if (rowId.startsWith('file:')) return { kind: 'file', id: rowId.slice('file:'.length) } - return { kind: 'file', id: rowId } -} - const hasExternalFiles = (dataTransfer: DataTransfer): boolean => dataTransfer.types.includes('Files') @@ -316,9 +303,8 @@ export function Files() { const filesRef = useRef(files) filesRef.current = files /** - * Indexed once. `isInvalidFolderTarget` resolves each dragged row's placement inside - * `dragover`, which fires continuously — a linear scan there is O(selection x resources) - * per event. + * Indexed once. The drag hook resolves each dragged row's placement inside `dragover`, which + * fires continuously — a linear scan there is O(selection x resources) per event. */ const fileById = useMemo(() => { const byId = new Map() @@ -327,8 +313,6 @@ export function Files() { }, [files]) const fileByIdRef = useRef(fileById) fileByIdRef.current = fileById - const foldersRef = useRef(folders) - foldersRef.current = folders const [uploadProgress, setUploadProgress] = useState({ completed: 0, @@ -339,6 +323,18 @@ export function Files() { const uploading = uploadProgress.total > 0 const [isDraggingOver, setIsDraggingOver] = useState(false) const dragCounterRef = useRef(0) + /** + * Takes down the "Drop to upload" overlay. + * + * Every path that consumes an OS file drag has to call this, including the one that never + * reaches the page-level handler: a drop on a folder row is handled by the drag hook, which + * stops propagation, so `handleDrop` below never runs and the counter it would have zeroed + * keeps the overlay on screen over the finished upload. + */ + const dismissUploadOverlay = useCallback(() => { + dragCounterRef.current = 0 + setIsDraggingOver(false) + }, []) const [ { search: urlSearchTerm, type: typeFilter, size: sizeFilter, uploadedBy: uploadedByFilter }, setFileFilters, @@ -379,10 +375,6 @@ export function Files() { const [creatingFile, setCreatingFile] = useState(false) const [isDirty, setIsDirty] = useState(false) const [saveStatus, setSaveStatus] = useState('idle') - const [activeDropTargetId, setActiveDropTargetId] = useState(null) - const [isBodyDropActive, setIsBodyDropActive] = useState(false) - const [activeBreadcrumbIndex, setActiveBreadcrumbIndex] = useState(null) - const [draggedRowIds, setDraggedRowIds] = useState>(() => EMPTY_DRAGGED_ROW_IDS) const [previewMode, setPreviewMode] = useState(() => { if (isNewFile) return 'editor' if (fileIdFromRoute) { @@ -395,7 +387,6 @@ export function Files() { const [showUnsavedChangesAlert, setShowUnsavedChangesAlert] = useState(false) const [showDeleteConfirm, setShowDeleteConfirm] = useState(false) const contextMenuItemRef = useRef(null) - const draggedRowIdsRef = useRef([]) const [deleteTarget, setDeleteTarget] = useState<{ fileIds: string[] folderIds: string[] @@ -404,7 +395,7 @@ export function Files() { const listRename = useInlineRename({ onSave: (rowId, name) => { - const parsed = parseRowId(rowId) + const parsed = parseFolderedRowId(rowId) if (parsed.kind === 'folder') { return updateFolder.mutateAsync({ workspaceId, folderId: parsed.id, updates: { name } }) } @@ -661,7 +652,7 @@ export function Files() { const { file } = item const Icon = getDocumentIcon(file.type || '', file.name) return { - id: fileRowId(file.id), + id: file.id, cells: { name: { icon: , @@ -720,66 +711,12 @@ export function Files() { onDeleteSelected: () => handleBulkDelete(), }) - const { selectedFileIds, selectedFolderIds } = useMemo(() => { - const fileIds: string[] = [] - const folderIds: string[] = [] - for (const rowId of selectedRowIds) { - const item = parseRowId(rowId) - if (item.kind === 'file') fileIds.push(item.id) - else folderIds.push(item.id) - } - return { selectedFileIds: fileIds, selectedFolderIds: folderIds } - }, [selectedRowIds]) - - const descendantFolderIdsByFolderId = useMemo(() => buildDescendantIndex(folders), [folders]) - - /** - * Whether dropping `sourceRowIds` into `targetFolderId` would move anything. - * - * Takes a folder id rather than a row id because the destination is not always a row: the - * list body files into the folder currently open, which has no row of its own, and a drag - * that spring-opened into an empty folder has nothing else to land on. - */ - const isInvalidFolderTarget = useCallback( - (targetFolderId: string | null, sourceRowIds: string[]) => { - for (const sourceRowId of sourceRowIds) { - const source = parseRowId(sourceRowId) - if (source.kind !== 'folder') continue - if (source.id === targetFolderId) return true - if ( - targetFolderId !== null && - descendantFolderIdsByFolderId.get(source.id)?.has(targetFolderId) - ) - return true - } - - const allAlreadyInTarget = sourceRowIds.every((sourceRowId) => { - const source = parseRowId(sourceRowId) - if (source.kind === 'file') { - return ( - (filesRef.current.find((f) => f.id === source.id)?.folderId ?? null) === targetFolderId - ) - } - return (folderByIdRef.current.get(source.id)?.parentId ?? null) === targetFolderId - }) - return allAlreadyInTarget - }, - [descendantFolderIdsByFolderId] + const { folderIds: selectedFolderIds, resourceIds: selectedFileIds } = useMemo( + () => splitFolderedRowIds(selectedRowIds), + [selectedRowIds] ) - /** - * Row-targeted drop: only a folder row can receive one. Delegates so the cycle and - * already-there rules live in exactly one place — the two had already drifted on whether a - * file's `folderId` was normalised with `?? null` before comparing. - */ - const isInvalidDropTarget = useCallback( - (targetRowId: string, sourceRowIds: string[]) => { - const target = parseRowId(targetRowId) - if (target.kind !== 'folder') return true - return isInvalidFolderTarget(target.id, sourceRowIds) - }, - [isInvalidFolderTarget] - ) + const descendantFolderIdsByFolderId = useMemo(() => buildDescendantIndex(folders), [folders]) const uploadFiles = useCallback( async (filesToUpload: File[], targetFolderId = currentFolderId) => { @@ -852,271 +789,45 @@ export function Files() { [workspaceId, canEdit, currentFolderId, notifyLimit] ) - const dragGhost = useRowDragGhost() - - const springNav = useSpringNavigation({ - currentFolderId, - onNavigate: (folderId, options) => { + const rowDragDropConfig = useFolderRowDragDrop({ + dragMime: FILE_ROW_DRAG_MIME, + canEdit, + editingRowId: listRename.editingId, + descendantsByFolderId: descendantFolderIdsByFolderId, + getFolderParentId: (folderId) => folderByIdRef.current.get(folderId)?.parentId ?? null, + getResourceFolderId: (fileId) => fileByIdRef.current.get(fileId)?.folderId ?? null, + getRowLabel: (rowId) => { + const parsed = parseFolderedRowId(rowId) + return parsed.kind === 'folder' + ? (folderByIdRef.current.get(parsed.id)?.name ?? 'Folder') + : (fileByIdRef.current.get(parsed.id)?.name ?? 'File') + }, + onMoveRows: ({ folderIds, resourceIds }, targetFolderId) => { + void moveItems + .mutateAsync({ workspaceId, fileIds: resourceIds, folderIds, targetFolderId }) + .then(() => clearSelection()) + .catch((error) => logger.error('Failed to move items:', error)) + }, + selection: { selectedRowIds, visibleRowIds, replaceSelection }, + onSpringOpenFolder: (folderId, options) => { void setFilesParams({ folderId, new: null }, options) }, - }) - - /** Returns the list to its resting state once a drag is over, however it ended. */ - const endDrag = useCallback(() => { - springNav.end() - dragGhost.remove() - dragCounterRef.current = 0 - draggedRowIdsRef.current = [] - setDraggedRowIds(EMPTY_DRAGGED_ROW_IDS) - setIsDraggingOver(false) - setActiveDropTargetId(null) - setIsBodyDropActive(false) - setActiveBreadcrumbIndex(null) - }, [dragGhost, springNav]) - - useDragTeardown(endDrag) - - const rowDragDropConfig = useMemo( - () => ({ - activeDropTargetId, - draggedRowIds, - isAnyDragActive: draggedRowIds.size > 0, - isRowDraggable: (rowId) => canEdit && listRename.editingId !== rowId, - isRowDropTarget: (rowId) => canEdit && parseRowId(rowId).kind === 'folder', - onDragStart: (e: DragEvent, rowId) => { - if (!canEdit || listRename.editingId === rowId) { - e.preventDefault() - return - } - - springNav.rememberOrigin() - const sourceRowIds = selectedRowIds.has(rowId) - ? visibleRowIds.filter((visibleRowId) => selectedRowIds.has(visibleRowId)) - : [rowId] - - draggedRowIdsRef.current = sourceRowIds - setDraggedRowIds(new Set(sourceRowIds)) - if (!selectedRowIds.has(rowId)) { - replaceSelection([rowId]) - } - - e.dataTransfer.effectAllowed = 'move' - writeRowDragPayload(e.dataTransfer, FILE_ROW_DRAG_MIME, sourceRowIds) - - const firstParsed = parseRowId(sourceRowIds[0]) - const firstName = - firstParsed.kind === 'file' - ? filesRef.current.find((f) => f.id === firstParsed.id)?.name - : foldersRef.current.find((f) => f.id === firstParsed.id)?.name - dragGhost.attach(e, firstName ?? 'Item', sourceRowIds.length) - }, - onDragOver: (e: DragEvent, rowId) => { - const sourceRowIds = draggedRowIdsRef.current - const isExternalFileDrag = hasExternalFiles(e.dataTransfer) - if (!isExternalFileDrag && isInvalidDropTarget(rowId, sourceRowIds)) return - - e.preventDefault() - e.stopPropagation() - e.dataTransfer.dropEffect = isExternalFileDrag ? 'copy' : 'move' - setActiveDropTargetId(rowId) - // The row sits inside the scroll container, whose `dragleave` ignores contained - // targets — clear it here so the row and the body never both read as the target. - setIsBodyDropActive(false) - setActiveBreadcrumbIndex(null) - /** - * Armed for OS file drags too: dropping an upload into a nested folder is the same - * gesture, and `onDragOver` only fires on folder rows. - */ - springNav.arm(parseRowId(rowId).id) - }, - onDragLeave: (e: DragEvent, rowId) => { - const relatedTarget = e.relatedTarget - if (relatedTarget instanceof Node && e.currentTarget.contains(relatedTarget)) return - springNav.disarm() - setActiveDropTargetId((current) => (current === rowId ? null : current)) - }, - onDrop: (e: DragEvent, rowId) => { - e.preventDefault() - e.stopPropagation() - - const target = parseRowId(rowId) - const droppedFiles = Array.from(e.dataTransfer.files ?? []) - const sourceRowIds = - readRowDragPayload(e.dataTransfer, FILE_ROW_DRAG_MIME) ?? draggedRowIdsRef.current - - const isFolderDrop = target.kind === 'folder' - const canUpload = isFolderDrop && droppedFiles.length > 0 - const canMove = - isFolderDrop && droppedFiles.length === 0 && !isInvalidDropTarget(rowId, sourceRowIds) - - /** - * Marked BEFORE `endDrag`, which is what consumes it. Ending the drag first runs the - * return navigation, bouncing the list out of the folder the drop just landed in — and - * because `end` clears the flag, setting it afterwards leaves it armed for the NEXT - * drag, whose return then never happens. The upload branch marks it too: a file dropped - * into a spring-opened folder must leave the view in that folder, not snap away from it. - */ - if (canUpload || canMove) springNav.markDropHandled() - - /** - * Ends the drag before dispatching, but only after the payload has been read off the - * event and the source ref. This handler stops propagation, so the window-level - * backstop never sees this drop, and the source row may already have unmounted — after - * a spring-open it always has. - */ - endDrag() - - if (canUpload) { - void uploadFiles(droppedFiles, target.id) - return - } - - if (!canMove) return - - const fileIds = sourceRowIds - .map(parseRowId) - .filter((source) => source.kind === 'file') - .map((source) => source.id) - const folderIds = sourceRowIds - .map(parseRowId) - .filter((source) => source.kind === 'folder') - .map((source) => source.id) - - if (fileIds.length === 0 && folderIds.length === 0) return - - void moveItems - .mutateAsync({ - workspaceId, - fileIds, - folderIds, - targetFolderId: target.id, - }) - .then(() => { - clearSelection() - }) - .catch((error) => { - logger.error('Failed to move items via drag and drop:', error) - }) - }, - onDragEnd: endDrag, - /** - * The breadcrumb is how a drag walks back UP; spring-loading only ever goes deeper. - * Hovering a crumb navigates to it on the same timer a folder row uses, and releasing on - * one files the drag there directly. - */ - breadcrumb: { - activeIndex: activeBreadcrumbIndex, - onDragOver: (e: DragEvent, folderId: string | null, index: number) => { - if (hasExternalFiles(e.dataTransfer)) return - const sourceRowIds = draggedRowIdsRef.current - if (sourceRowIds.length === 0) return - /** Armed even for a no-op drop: walking back to where the drag started is the point. */ - if (folderId !== currentFolderId) springNav.arm(folderId) - const canDrop = !isInvalidFolderTarget(folderId, sourceRowIds) - setActiveBreadcrumbIndex(canDrop ? index : null) - setIsBodyDropActive(false) - if (!canDrop) return - e.preventDefault() - e.stopPropagation() - e.dataTransfer.dropEffect = 'move' - }, - onDragLeave: (_e: DragEvent, index: number) => { - springNav.disarm() - setActiveBreadcrumbIndex((current) => (current === index ? null : current)) - }, - onDrop: (e: DragEvent, folderId: string | null) => { - if (hasExternalFiles(e.dataTransfer)) return - e.preventDefault() - e.stopPropagation() - const sourceRowIds = - readRowDragPayload(e.dataTransfer, FILE_ROW_DRAG_MIME) ?? draggedRowIdsRef.current - const canMove = sourceRowIds.length > 0 && !isInvalidFolderTarget(folderId, sourceRowIds) - if (canMove) springNav.markDropHandled() - endDrag() - if (!canMove) return - - const fileIds: string[] = [] - const folderIds: string[] = [] - for (const sourceRowId of sourceRowIds) { - const source = parseRowId(sourceRowId) - if (source.kind === 'file') fileIds.push(source.id) - else folderIds.push(source.id) - } - void moveItems - .mutateAsync({ workspaceId, fileIds, folderIds, targetFolderId: folderId }) - .then(() => clearSelection()) - .catch((error) => logger.error('Failed to move items via the breadcrumb:', error)) - }, - }, - body: { - isActive: isBodyDropActive, - onDragOver: (e: DragEvent) => { - /** - * Internal row drags only. An OS file drag is already owned by the page-level - * handler, which paints the full "Drop to upload" overlay and uploads into this same - * folder — claiming it here would double the affordance and, without stopping - * propagation, upload every dropped file twice. - */ - if (hasExternalFiles(e.dataTransfer)) return - const sourceRowIds = draggedRowIdsRef.current - // Recomputed every event: a spring-open changes the destination mid-drag. - const canDrop = - sourceRowIds.length > 0 && !isInvalidFolderTarget(currentFolderId, sourceRowIds) - setIsBodyDropActive(canDrop) - if (!canDrop) return - e.preventDefault() - e.dataTransfer.dropEffect = 'move' - }, - onDragLeave: (e: DragEvent) => { - const relatedTarget = e.relatedTarget - if (relatedTarget instanceof Node && e.currentTarget.contains(relatedTarget)) return - setIsBodyDropActive(false) - }, - onDrop: (e: DragEvent) => { - // Left to the page-level handler, which uploads into this folder already. - if (hasExternalFiles(e.dataTransfer)) return - e.preventDefault() - e.stopPropagation() - const sourceRowIds = - readRowDragPayload(e.dataTransfer, FILE_ROW_DRAG_MIME) ?? draggedRowIdsRef.current - const canMove = - sourceRowIds.length > 0 && !isInvalidFolderTarget(currentFolderId, sourceRowIds) - - if (canMove) springNav.markDropHandled() - endDrag() - if (!canMove) return - - const fileIds: string[] = [] - const folderIds: string[] = [] - for (const sourceRowId of sourceRowIds) { - const source = parseRowId(sourceRowId) - if (source.kind === 'file') fileIds.push(source.id) - else folderIds.push(source.id) - } - void moveItems - .mutateAsync({ workspaceId, fileIds, folderIds, targetFolderId: currentFolderId }) - .then(() => clearSelection()) - .catch((error) => logger.error('Failed to move items into the open folder:', error)) - }, + currentFolderId, + /** + * The one thing this list does that the others do not. Folder rows still highlight and + * spring open for an OS file drag — filing an upload into a nested folder is the same + * gesture — while the body and breadcrumb decline so the page-level "Drop to upload" + * overlay owns those regions instead of competing with them. + */ + externalDrop: { + matches: hasExternalFiles, + onDropIntoFolder: (dataTransfer, targetFolderId) => { + dismissUploadOverlay() + const dropped = Array.from(dataTransfer.files ?? []) + if (dropped.length > 0) void uploadFiles(dropped, targetFolderId) }, - }), - [ - activeDropTargetId, - draggedRowIds, - canEdit, - listRename.editingId, - selectedRowIds, - visibleRowIds, - isInvalidDropTarget, - isInvalidFolderTarget, - isBodyDropActive, - activeBreadcrumbIndex, - currentFolderId, - clearSelection, - uploadFiles, - workspaceId, - ] - ) + }, + }) const handleFileChange = async (e: React.ChangeEvent) => { const list = e.target.files @@ -1152,9 +863,8 @@ export function Files() { * the window-level teardown treats the drag as unconsumed and returns to the folder it * began in — pulling the user out of the folder they just spring-opened to receive it. */ - springNav.markDropHandled() - dragCounterRef.current = 0 - setIsDraggingOver(false) + rowDragDropConfig.externalDropHandled() + dismissUploadOverlay() const dropped = Array.from(e.dataTransfer.files) if (dropped.length > 0) await uploadFiles(dropped) } @@ -1449,7 +1159,7 @@ export function Files() { const handleRowContextMenu = useCallback( (e: React.MouseEvent, rowId: string) => { - const parsed = parseRowId(rowId) + const parsed = parseFolderedRowId(rowId) const item = parsed.kind === 'folder' ? folders.find((folder) => folder.id === parsed.id) @@ -1486,7 +1196,7 @@ export function Files() { const handleContextMenuDownload = useCallback(() => { const item = contextMenuItemRef.current if (!item) return - const rowId = item.kind === 'file' ? fileRowId(item.file.id) : folderRowId(item.folder.id) + const rowId = item.kind === 'file' ? item.file.id : folderRowId(item.folder.id) if (selectedRowIds.has(rowId) && selectedRowIds.size > 1) { void handleBulkDownload() closeContextMenu() @@ -1504,7 +1214,7 @@ export function Files() { const handleContextMenuRename = useCallback(() => { const item = contextMenuItemRef.current - if (item?.kind === 'file') listRename.startRename(fileRowId(item.file.id), item.file.name) + if (item?.kind === 'file') listRename.startRename(item.file.id, item.file.name) if (item?.kind === 'folder') listRename.startRename(folderRowId(item.folder.id), item.folder.name) closeContextMenu() @@ -1519,7 +1229,7 @@ export function Files() { const handleContextMenuDelete = useCallback(() => { const item = contextMenuItemRef.current if (!item) return - const rowId = item.kind === 'file' ? fileRowId(item.file.id) : folderRowId(item.folder.id) + const rowId = item.kind === 'file' ? item.file.id : folderRowId(item.folder.id) if (selectedRowIds.has(rowId) && selectedRowIds.size > 1) { handleBulkDelete() closeContextMenu() @@ -1715,7 +1425,7 @@ export function Files() { const handleRowClick = useCallback( (rowId: string) => { if (listRenameRef.current.editingId !== rowId && !headerRenameRef.current.editingId) { - const parsed = parseRowId(rowId) + const parsed = parseFolderedRowId(rowId) if (parsed.kind === 'folder') { void setFilesParams({ folderId: parsed.id, new: null }) return diff --git a/apps/sim/app/workspace/[workspaceId]/knowledge/knowledge.tsx b/apps/sim/app/workspace/[workspaceId]/knowledge/knowledge.tsx index b9a4518127a..0cb354d4c45 100644 --- a/apps/sim/app/workspace/[workspaceId]/knowledge/knowledge.tsx +++ b/apps/sim/app/workspace/[workspaceId]/knowledge/knowledge.tsx @@ -124,6 +124,10 @@ const CONTENT_FILTER_OPTIONS: ChipDropdownOption[] = [ { value: 'empty', label: 'Empty' }, ] +/** This list's private drag MIME, so a drag started on another list is never mistaken for one + * of these rows. */ +const KNOWLEDGE_ROW_DRAG_MIME = 'application/x-sim-workspace-knowledge-rows' + const FOLDER_RESOURCE_TYPE = 'knowledge_base' as const const ROOT_BREADCRUMB_LABEL = FOLDERED_RESOURCE_HEADERS[FOLDER_RESOURCE_TYPE].rootLabel @@ -1078,6 +1082,7 @@ export function Knowledge() { ) const rowDragDropConfig = useFolderRowDragDrop({ + dragMime: KNOWLEDGE_ROW_DRAG_MIME, canEdit, editingRowId: listRename.editingId, descendantsByFolderId, diff --git a/apps/sim/app/workspace/[workspaceId]/tables/tables.tsx b/apps/sim/app/workspace/[workspaceId]/tables/tables.tsx index a1672cd0b19..dc5e89a8742 100644 --- a/apps/sim/app/workspace/[workspaceId]/tables/tables.tsx +++ b/apps/sim/app/workspace/[workspaceId]/tables/tables.tsx @@ -102,6 +102,10 @@ const COLUMNS: ResourceColumn[] = [ { id: 'updated', header: 'Last Updated' }, ] +/** This list's private drag MIME, so a drag started on another list is never mistaken for one + * of these rows. */ +const TABLE_ROW_DRAG_MIME = 'application/x-sim-workspace-table-rows' + /** Root label for breadcrumbs and the "move to workspace root" destination. */ const ROOT_LABEL = FOLDERED_RESOURCE_HEADERS.table.rootLabel @@ -984,6 +988,7 @@ export function Tables() { }, [handleBulkDelete]) const rowDragDropConfig = useFolderRowDragDrop({ + dragMime: TABLE_ROW_DRAG_MIME, canEdit, editingRowId: listRename.editingId, descendantsByFolderId: descendantFolderIds, diff --git a/apps/sim/lib/knowledge/application/bulk.test.ts b/apps/sim/lib/knowledge/application/bulk.test.ts index 077a9fd8f14..a7965424441 100644 --- a/apps/sim/lib/knowledge/application/bulk.test.ts +++ b/apps/sim/lib/knowledge/application/bulk.test.ts @@ -60,7 +60,7 @@ vi.mock('@/lib/folders/bulk', () => ({ vi.mock('@/lib/folders/queries', () => ({ findActiveFolder: mocks.findActiveFolder })) vi.mock('@/lib/knowledge/application/contexts', () => ({ resolveKnowledgeWorkspaceContext: mocks.resolveWorkspace, - resolveActiveKnowledgeBaseContext: mocks.resolveKnowledgeBase, + resolveActiveKnowledgeBaseInWorkspace: mocks.resolveKnowledgeBase, })) vi.mock('@/lib/knowledge/service', () => ({ updateKnowledgeBase: mocks.updateRecord, @@ -96,8 +96,8 @@ describe('knowledge bulk application use cases', () => { mocks.resolvePermission.mockResolvedValue('write') mocks.planFolderSelection.mockResolvedValue(emptyPlan) mocks.findActiveFolder.mockResolvedValue({ id: 'folder-1' }) - mocks.resolveKnowledgeBase.mockImplementation( - async ({ knowledgeBaseId }: { knowledgeBaseId: string }) => knowledgeContext(knowledgeBaseId) + mocks.resolveKnowledgeBase.mockImplementation(async (knowledgeBaseId: string) => + knowledgeContext(knowledgeBaseId) ) mocks.updateRecord.mockImplementation(async (id: string) => ({ id, name: `Base ${id}` })) mocks.deleteRecord.mockResolvedValue(undefined) @@ -189,9 +189,8 @@ describe('knowledge bulk application use cases', () => { contained: [], covered: new Set(['folder-1', 'folder-child']), }) - mocks.resolveKnowledgeBase.mockImplementation( - async ({ knowledgeBaseId }: { knowledgeBaseId: string }) => - knowledgeContext(knowledgeBaseId, 'folder-child') + mocks.resolveKnowledgeBase.mockImplementation(async (knowledgeBaseId: string) => + knowledgeContext(knowledgeBaseId, 'folder-child') ) const result = await bulkDeleteKnowledgeItems.execute({ diff --git a/apps/sim/lib/knowledge/application/bulk.ts b/apps/sim/lib/knowledge/application/bulk.ts index 3f8aaf2d776..f838260ad78 100644 --- a/apps/sim/lib/knowledge/application/bulk.ts +++ b/apps/sim/lib/knowledge/application/bulk.ts @@ -26,7 +26,7 @@ import { resolveKnowledgeAttributedUserId } from '@/lib/knowledge/application/bi import { type ActiveKnowledgeBaseContext, type KnowledgeWorkspaceContext, - resolveActiveKnowledgeBaseContext, + resolveActiveKnowledgeBaseInWorkspace, resolveKnowledgeWorkspaceContext, } from '@/lib/knowledge/application/contexts' import { knowledgeOperations } from '@/lib/knowledge/application/operations' @@ -119,7 +119,7 @@ async function resolveBulkKnowledgeContext( */ async function runKnowledgeItems( knowledgeBaseIds: readonly string[], - workspaceId: string, + workspace: KnowledgeWorkspaceContext, covered: ReadonlySet, authorize: (canonical: ActiveKnowledgeBaseContext) => Promise, apply: (canonical: ActiveKnowledgeBaseContext) => Promise, @@ -129,10 +129,7 @@ async function runKnowledgeItems( for (const knowledgeBaseId of knowledgeBaseIds) { let knowledgeBaseName = knowledgeBaseId try { - const canonical = await resolveActiveKnowledgeBaseContext({ - knowledgeBaseId, - assertedWorkspaceId: workspaceId, - }) + const canonical = await resolveActiveKnowledgeBaseInWorkspace(knowledgeBaseId, workspace) knowledgeBaseName = canonical.knowledgeBase.name const folderId = canonical.knowledgeBase.folderId if (folderId && covered.has(folderId)) { @@ -221,7 +218,7 @@ export const bulkMoveKnowledgeItems = defineAuthorizedKnowledgeUseCase({ const terminalError = await runKnowledgeItems( context.knowledgeBaseIds, - context.workspaceId, + context, plan.covered, (canonical) => authorizeWorkspaceOperation(principal, knowledgeOperations.bulkMoveItems, canonical, { @@ -323,7 +320,7 @@ export const bulkDeleteKnowledgeItems = defineAuthorizedKnowledgeUseCase({ const terminalError = await runKnowledgeItems( context.knowledgeBaseIds, - context.workspaceId, + context, plan.covered, (canonical) => authorizeWorkspaceOperation(principal, knowledgeOperations.bulkDeleteItems, canonical, { diff --git a/apps/sim/lib/knowledge/application/contexts.ts b/apps/sim/lib/knowledge/application/contexts.ts index cf7ea274243..5219c3f2082 100644 --- a/apps/sim/lib/knowledge/application/contexts.ts +++ b/apps/sim/lib/knowledge/application/contexts.ts @@ -82,18 +82,30 @@ export async function resolveKnowledgeWorkspaceContext(input: { return context } -export async function resolveActiveKnowledgeBaseContext(input: { - knowledgeBaseId: string - assertedWorkspaceId?: string -}): Promise { - const knowledgeBase = await getKnowledgeBaseById(input.knowledgeBaseId) +/** + * Loads a knowledge base and asserts it lives in `workspaceId` when the caller named one. + * + * Shared by both resolvers below so the not-found concealment — a base outside the asserted + * workspace is reported as missing, never as forbidden — and the nullable-`workspaceId` guard + * that legacy personal bases need are written once, and cannot be dropped from one path only. + */ +async function requireKnowledgeBase(knowledgeBaseId: string, workspaceId: string | undefined) { + const knowledgeBase = await getKnowledgeBaseById(knowledgeBaseId) if ( !knowledgeBase?.workspaceId || - (input.assertedWorkspaceId !== undefined && - knowledgeBase.workspaceId !== input.assertedWorkspaceId) + (workspaceId !== undefined && knowledgeBase.workspaceId !== workspaceId) ) { throw new OrchestrationError('not_found', 'Knowledge base not found') } + /** The guard above proves `workspaceId` is set; carry that into the type so callers see it. */ + return knowledgeBase as typeof knowledgeBase & { workspaceId: string } +} + +export async function resolveActiveKnowledgeBaseContext(input: { + knowledgeBaseId: string + assertedWorkspaceId?: string +}): Promise { + const knowledgeBase = await requireKnowledgeBase(input.knowledgeBaseId, input.assertedWorkspaceId) const workspaceContext = await loadKnowledgeWorkspaceContext(knowledgeBase.workspaceId) if (!workspaceContext) throw new OrchestrationError('not_found', 'Knowledge base not found') return { @@ -103,6 +115,21 @@ export async function resolveActiveKnowledgeBaseContext(input: { } } +/** + * Resolves one knowledge base against a workspace context the caller already loaded. + * + * Same result as {@link resolveActiveKnowledgeBaseContext}, minus its workspace load. A batch has + * that context in hand before the first item — it is what bounded and authorized the request — + * and it cannot differ per item, so re-resolving it once per base is a whole extra query each. + */ +export async function resolveActiveKnowledgeBaseInWorkspace( + knowledgeBaseId: string, + workspaceContext: KnowledgeWorkspaceContext +): Promise { + const knowledgeBase = await requireKnowledgeBase(knowledgeBaseId, workspaceContext.workspaceId) + return { ...workspaceContext, knowledgeBaseId: knowledgeBase.id, knowledgeBase } +} + export async function resolveActiveKnowledgeResourceContext(input: { knowledgeBaseId: string assertedWorkspaceId?: string diff --git a/apps/sim/lib/table/application/bulk.test.ts b/apps/sim/lib/table/application/bulk.test.ts index 221ef7be4b0..ddfa56d7ad0 100644 --- a/apps/sim/lib/table/application/bulk.test.ts +++ b/apps/sim/lib/table/application/bulk.test.ts @@ -64,7 +64,7 @@ vi.mock('@/lib/table', () => ({ moveTableToFolder: mocks.moveTableToFolder, })) vi.mock('@/lib/table/application/context', () => ({ - resolveActiveTableContext: mocks.resolveTableContext, + resolveActiveTableInWorkspace: mocks.resolveTableContext, resolveTableWorkspaceContext: mocks.resolveWorkspaceContext, })) vi.mock('@/lib/table/events', () => ({ signalTableSchemaChanged: mocks.signal })) @@ -99,9 +99,7 @@ describe('table bulk application use cases', () => { mocks.resolvePermission.mockResolvedValue('write') mocks.planFolderSelection.mockResolvedValue(emptyPlan) mocks.findActiveFolder.mockResolvedValue({ id: 'folder-1' }) - mocks.resolveTableContext.mockImplementation(async ({ tableId }: { tableId: string }) => - tableContext(tableId) - ) + mocks.resolveTableContext.mockImplementation(async (tableId: string) => tableContext(tableId)) mocks.moveTableToFolder.mockResolvedValue({ name: 'Moved' }) mocks.deleteTable.mockResolvedValue({ archived: { name: 'Archived', workspaceId: 'workspace-1' }, @@ -191,7 +189,7 @@ describe('table bulk application use cases', () => { contained: [], covered: new Set(['folder-1', 'folder-child']), }) - mocks.resolveTableContext.mockImplementation(async ({ tableId }: { tableId: string }) => + mocks.resolveTableContext.mockImplementation(async (tableId: string) => tableContext(tableId, 'folder-child') ) @@ -293,6 +291,58 @@ describe('table bulk application use cases', () => { expect(mocks.bulkMoveFolders).not.toHaveBeenCalled() }) + /** + * The canonical workspace context is what bounded and authorized the request; it cannot differ + * per item, so the batch resolves it once and composes each table onto it. Resolving it per + * item was a whole extra load each. + * + * Note this deliberately does NOT memoize the per-item permission check: each item commits + * independently, so every one of them re-reads the caller's current permission and a + * revocation part-way through a batch stops the rest. + */ + it('loads the workspace context once however many items the batch carries', async () => { + const move = (tableIds: string[]) => + bulkMoveTables.execute({ + principal, + input: { + assertedWorkspaceId: 'workspace-1', + tableIds, + folderIds: [], + targetFolderId: 'folder-1', + }, + }) + + const small = await move(['table-1', 'table-2', 'table-3']) + expect(small.moved).toHaveLength(3) + expect(mocks.resolveWorkspaceContext).toHaveBeenCalledTimes(1) + + mocks.resolveWorkspaceContext.mockClear() + const large = await move(Array.from({ length: 25 }, (_, index) => `table-${index}`)) + expect(large.moved).toHaveLength(25) + expect(mocks.resolveWorkspaceContext).toHaveBeenCalledTimes(1) + }) + + /** A revocation part-way through a batch must stop the items that have not run yet. */ + it('re-checks the caller permission for every item', async () => { + mocks.resolvePermission.mockResolvedValueOnce('write').mockResolvedValueOnce('write') + mocks.resolvePermission.mockResolvedValue(null) + + const result = await bulkMoveTables.execute({ + principal, + input: { + assertedWorkspaceId: 'workspace-1', + tableIds: ['table-1', 'table-2', 'table-3'], + folderIds: [], + targetFolderId: 'folder-1', + }, + }) + + expect(result.moved).toHaveLength(1) + expect(result.failed.concat(result.notFound as never[])).toHaveLength(2) + /** One for the operation itself, then one per item — no memo may collapse these. */ + expect(mocks.resolvePermission).toHaveBeenCalledTimes(4) + }) + it('moves tables and folders in one operation', async () => { mocks.planFolderSelection.mockResolvedValue({ selected: [{ id: 'folder-2', name: 'Archive' }], diff --git a/apps/sim/lib/table/application/bulk.ts b/apps/sim/lib/table/application/bulk.ts index 5b895d92662..ba046391444 100644 --- a/apps/sim/lib/table/application/bulk.ts +++ b/apps/sim/lib/table/application/bulk.ts @@ -25,7 +25,7 @@ import { } from '@/lib/table/application/batch-policy' import { type ActiveTableContext, - resolveActiveTableContext, + resolveActiveTableInWorkspace, resolveTableWorkspaceContext, type TableWorkspaceContext, } from '@/lib/table/application/context' @@ -159,7 +159,7 @@ async function notifyBatchedTableChanges( */ async function runTableItems( tableIds: readonly string[], - workspaceId: string, + workspace: TableWorkspaceContext, covered: ReadonlySet, authorize: (canonical: ActiveTableContext) => Promise, /** Runs against an already-authorized canonical table. Returns its authoritative name. */ @@ -170,10 +170,7 @@ async function runTableItems( for (const tableId of tableIds) { let tableName = tableId try { - const canonical = await resolveActiveTableContext({ - tableId, - assertedWorkspaceId: workspaceId, - }) + const canonical = await resolveActiveTableInWorkspace(tableId, workspace) tableName = canonical.table.name if (canonical.table.folderId && covered.has(canonical.table.folderId)) { outcome.skipped.push({ kind: 'table', id: canonical.table.id, name: tableName }) @@ -249,7 +246,7 @@ export const bulkMoveTables = defineAuthorizedTableUseCase({ try { const terminalError = await runTableItems( context.tableIds, - context.workspaceId, + context, plan.covered, (canonical) => authorizeTableOperation(principal, tableOperations.bulkMove, canonical), async (canonical) => @@ -355,7 +352,7 @@ export const bulkDeleteTables = defineAuthorizedTableUseCase({ try { const terminalError = await runTableItems( context.tableIds, - context.workspaceId, + context, plan.covered, (canonical) => authorizeTableOperation(principal, tableOperations.bulkDelete, canonical), async (canonical) => { diff --git a/apps/sim/lib/table/application/context.ts b/apps/sim/lib/table/application/context.ts index d87150c0f50..71e71de49b9 100644 --- a/apps/sim/lib/table/application/context.ts +++ b/apps/sim/lib/table/application/context.ts @@ -18,17 +18,40 @@ export async function resolveTableWorkspaceContext( return canonical } +/** + * Loads a table and asserts it lives in `workspaceId` when the caller named one. + * + * Shared by both resolvers below so the not-found concealment — a table outside the asserted + * workspace is reported as missing, never as forbidden — is written once. + */ +async function requireTable(tableId: string, workspaceId: string | undefined) { + const table = await getTableById(tableId) + if (!table || (workspaceId !== undefined && table.workspaceId !== workspaceId)) { + throw new OrchestrationError('not_found', 'Table not found') + } + return table +} + export async function resolveActiveTableContext(input: { tableId: string assertedWorkspaceId?: string }): Promise { - const table = await getTableById(input.tableId) - if ( - !table || - (input.assertedWorkspaceId !== undefined && table.workspaceId !== input.assertedWorkspaceId) - ) { - throw new OrchestrationError('not_found', 'Table not found') - } + const table = await requireTable(input.tableId, input.assertedWorkspaceId) const workspaceContext = await resolveTableWorkspaceContext(table.workspaceId) return { ...workspaceContext, tableId: table.id, table } } + +/** + * Resolves one table against a workspace context the caller already loaded. + * + * Same result as {@link resolveActiveTableContext}, minus its workspace load. A batch has that + * context in hand before the first item — it is what bounded and authorized the request — and it + * cannot differ per item, so re-resolving it once per table is a whole extra query each. + */ +export async function resolveActiveTableInWorkspace( + tableId: string, + workspaceContext: TableWorkspaceContext +): Promise { + const table = await requireTable(tableId, workspaceContext.workspaceId) + return { ...workspaceContext, tableId: table.id, table } +} diff --git a/packages/emcn/src/components/chip/chip-chrome.ts b/packages/emcn/src/components/chip/chip-chrome.ts index e8318f98343..f15f351fbdd 100644 --- a/packages/emcn/src/components/chip/chip-chrome.ts +++ b/packages/emcn/src/components/chip/chip-chrome.ts @@ -80,9 +80,13 @@ export const chipActiveSurfaceClass = 'bg-[var(--surface-active)]' * own drop affordance is a `--text-subtle` tint. Drawn inside the element's own box so the ring * never overlaps its neighbours. Hand-rolled rows and breadcrumb crumbs import this rather than * restating the literal, so every drop destination reads identically. + * + * Fills to `--surface-active`, the same weight as a selected row, and leans on the ring to tell + * the two apart. Not `--surface-4`: that is the button-base token, and in light mode it is + * *lighter* than `--surface-hover`, so the row under the cursor read weaker the moment it became + * a drop target — the strongest state painting the faintest fill. */ -export const chipDropTargetSurfaceClass = - 'bg-[var(--surface-4)] outline outline-1 outline-[var(--text-subtle)] outline-offset-[-1px]' +export const chipDropTargetSurfaceClass = `${chipActiveSurfaceClass} outline outline-1 outline-[var(--text-subtle)] outline-offset-[-1px]` /** * The disclosure chevron that rotates to expand or collapse a sidebar section or a * tree row: 14px at `--text-icon`, animating on the same 150ms curve the section