From b02c146f5c9cc1a31c2b59a665fb707b1e4f5781 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 15 Aug 2026 16:17:22 -0700 Subject: [PATCH 1/2] refactor(resources): converge Files onto the shared drag hook and batch bulk authorization Files kept a 280-line copy of the foldered-list drag logic because it also accepts OS file drops. The copies had already drifted, so the external drop becomes an option on the shared hook and the copy goes away. - Add `externalDrop` to `useFolderRowDragDrop`: folder rows highlight and spring open for an OS file drag exactly as for a move, while the body and breadcrumb decline so the page-level upload overlay owns those regions - Collapse the three drop-active booleans into one `ActiveDropTarget` union, so exactly one affordance is armed by construction rather than by hand-clearing - Keep drop-target writes identity-stable so `dragover` does not re-render the list on every event - Give each list its own drag MIME again, restoring the cross-surface isolation `drag-payload.ts` documents - Let a folder spring open more than once per drag, so a drag can walk back out through the breadcrumb and descend again; the guard against re-entering the folder already on screen moves to `useSpringNavigation`, the only layer that can state it - Resolve each bulk item against the workspace context the batch already holds, and memoize the effective-permission lookup for the batch, replacing two invariant queries per item - Fill the drop target at `--surface-active`: `--surface-4` is the button-base token and is lighter than hover in light mode, so the strongest row state read the faintest --- .../folders/use-folder-row-drag-drop.ts | 180 ++++++-- .../folders/use-spring-loaded-folder.test.tsx | 26 +- .../folders/use-spring-loaded-folder.ts | 28 +- .../folders/use-spring-navigation.test.tsx | 130 ++++++ .../folders/use-spring-navigation.ts | 14 +- .../components/resource/resource.tsx | 8 +- .../workspace/[workspaceId]/files/files.tsx | 414 +++--------------- .../[workspaceId]/knowledge/knowledge.tsx | 5 + .../workspace/[workspaceId]/tables/tables.tsx | 5 + apps/sim/lib/core/application/index.ts | 2 + .../application/workspace-authorization.ts | 76 +++- .../workspace-permission-cache.test.ts | 80 ++++ .../lib/knowledge/application/bulk.test.ts | 11 +- apps/sim/lib/knowledge/application/bulk.ts | 40 +- .../sim/lib/knowledge/application/contexts.ts | 41 +- .../lib/table/application/authorization.ts | 12 +- apps/sim/lib/table/application/bulk.test.ts | 41 +- apps/sim/lib/table/application/bulk.ts | 36 +- apps/sim/lib/table/application/context.ts | 37 +- .../emcn/src/components/chip/chip-chrome.ts | 8 +- 20 files changed, 699 insertions(+), 495 deletions(-) create mode 100644 apps/sim/lib/core/application/workspace-permission-cache.test.ts 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..9b511b4b500 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, @@ -379,10 +363,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 +375,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 +383,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 +640,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 +699,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 +777,44 @@ 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) => { + 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,7 +850,7 @@ 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() + rowDragDropConfig.externalDropHandled() dragCounterRef.current = 0 setIsDraggingOver(false) const dropped = Array.from(e.dataTransfer.files) @@ -1449,7 +1147,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 +1184,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 +1202,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 +1217,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 +1413,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/core/application/index.ts b/apps/sim/lib/core/application/index.ts index 6ac885759ba..7f9b1dd7475 100644 --- a/apps/sim/lib/core/application/index.ts +++ b/apps/sim/lib/core/application/index.ts @@ -21,9 +21,11 @@ export type { WorkspaceAuthorizationContext, WorkspaceAuthorizationOptions, WorkspaceDelegationPolicy, + WorkspacePermissionCache, } from '@/lib/core/application/workspace-authorization' export { authorizeWorkspaceOperation, + createWorkspacePermissionCache, DelegatedServiceAuthorizationError, DelegatedWorkspaceAuthorizationError, InsufficientWorkspacePermissionsError, diff --git a/apps/sim/lib/core/application/workspace-authorization.ts b/apps/sim/lib/core/application/workspace-authorization.ts index 4014afb158f..a8ce8115552 100644 --- a/apps/sim/lib/core/application/workspace-authorization.ts +++ b/apps/sim/lib/core/application/workspace-authorization.ts @@ -27,6 +27,65 @@ export interface WorkspaceAuthorizationOptions forUpdate?: boolean delegation?: WorkspaceDelegationPolicy + /** + * Memo for the human-permission lookup, supplied by a caller that authorizes many items in one + * operation. Ignored alongside `executor` or `forUpdate` — see + * {@link createWorkspacePermissionCache}. + */ + permissionCache?: WorkspacePermissionCache +} + +export interface WorkspacePermissionCache { + resolve( + userId: string, + workspaceId: string, + workspaceOrganizationId: string | null + ): Promise +} + +/** + * Memoizes the effective-permission lookup across the items of one bulk operation. + * + * A batch authorizes every item separately — delegation scope is per-resource, so the check + * cannot simply be hoisted out of the loop — but the human-permission half of it reads the same + * `(user, workspace, organization)` triple every time, two queries deep. On a hundred-item + * request that is two hundred round trips for a value that cannot change within the batch. + * + * Caller-owned and request-scoped on purpose: nothing here outlives the operation that created + * it, so a permission changed between requests is always seen by the next one. Skipped entirely + * when the caller passes its own `executor` (a transaction has its own snapshot to honour) or + * `forUpdate` (that lookup takes a row lock, which is a side effect, not a read). + * + * Neither of the repo's two existing memo idioms fits. `coalesceLocally` evicts on settle, so a + * sequential per-item loop would re-query every item. React `cache()` cannot be skipped per call + * for the `executor`/`forUpdate` paths and has no request scope in the worker runtime. An + * implicit process-wide memo on an authorization read is a lifetime worth refusing outright. + */ +export function createWorkspacePermissionCache(): WorkspacePermissionCache { + const entries = new Map>() + return { + resolve(userId, workspaceId, workspaceOrganizationId) { + /** Structural, so no id can run into the next and answer another workspace's question. */ + const key = JSON.stringify([userId, workspaceId, workspaceOrganizationId]) + const cached = entries.get(key) + if (cached) return cached + /** + * The in-flight promise is what gets stored, so concurrent items share one query rather + * than racing to start their own. Evicted if it rejects: a transient database failure must + * not become the permanent answer for the rest of the batch. + */ + const pending = resolveEffectiveWorkspacePermission( + userId, + workspaceId, + workspaceOrganizationId + ).catch((error) => { + entries.delete(key) + throw error + }) + entries.set(key, pending) + return pending + }, + } } export class InsufficientWorkspacePermissionsError extends ForbiddenOperationError { @@ -151,13 +210,16 @@ async function requireCurrentHumanPermission ): Promise { - const permission = await resolveEffectiveWorkspacePermission( - userId, - context.workspaceId, - context.workspaceOrganizationId, - options?.executor, - { forUpdate: options?.forUpdate } - ) + const memo = options?.executor || options?.forUpdate ? undefined : options?.permissionCache + const permission = memo + ? await memo.resolve(userId, context.workspaceId, context.workspaceOrganizationId) + : await resolveEffectiveWorkspacePermission( + userId, + context.workspaceId, + context.workspaceOrganizationId, + options?.executor, + { forUpdate: options?.forUpdate } + ) requirePermission(permission, required) } diff --git a/apps/sim/lib/core/application/workspace-permission-cache.test.ts b/apps/sim/lib/core/application/workspace-permission-cache.test.ts new file mode 100644 index 00000000000..7f0f8e88c8c --- /dev/null +++ b/apps/sim/lib/core/application/workspace-permission-cache.test.ts @@ -0,0 +1,80 @@ +/** + * @vitest-environment node + */ + +import { beforeEach, describe, expect, it, vi } from 'vitest' + +const mocks = vi.hoisted(() => ({ resolvePermission: vi.fn() })) + +vi.mock('@sim/platform-authz/workspace', () => ({ + permissionSatisfies: () => true, + resolveEffectiveWorkspacePermission: mocks.resolvePermission, +})) + +import { createWorkspacePermissionCache } from '@/lib/core/application/workspace-authorization' + +describe('createWorkspacePermissionCache', () => { + beforeEach(() => { + vi.clearAllMocks() + mocks.resolvePermission.mockResolvedValue('write') + }) + + it('queries once for a repeated triple', async () => { + const cache = createWorkspacePermissionCache() + + const results = await Promise.all([ + cache.resolve('user-1', 'workspace-1', 'organization-1'), + cache.resolve('user-1', 'workspace-1', 'organization-1'), + cache.resolve('user-1', 'workspace-1', 'organization-1'), + ]) + + expect(results).toEqual(['write', 'write', 'write']) + expect(mocks.resolvePermission).toHaveBeenCalledExactlyOnceWith( + 'user-1', + 'workspace-1', + 'organization-1' + ) + }) + + it('keeps a null answer, which is a real verdict rather than a cache miss', async () => { + mocks.resolvePermission.mockResolvedValue(null) + const cache = createWorkspacePermissionCache() + + expect(await cache.resolve('user-1', 'workspace-1', null)).toBeNull() + expect(await cache.resolve('user-1', 'workspace-1', null)).toBeNull() + + expect(mocks.resolvePermission).toHaveBeenCalledTimes(1) + }) + + it('separates entries that differ in any part of the triple', async () => { + const cache = createWorkspacePermissionCache() + + await cache.resolve('user-1', 'workspace-1', 'organization-1') + await cache.resolve('user-2', 'workspace-1', 'organization-1') + await cache.resolve('user-1', 'workspace-2', 'organization-1') + await cache.resolve('user-1', 'workspace-1', null) + + expect(mocks.resolvePermission).toHaveBeenCalledTimes(4) + }) + + it('does not let a workspace id run into an organization id', async () => { + // A naive concatenation makes ('u', 'a', 'bc') and ('u', 'ab', 'c') the same entry, which + // would answer one workspace's authorization question with another's permission row. + const cache = createWorkspacePermissionCache() + + await cache.resolve('u', 'a', 'bc') + await cache.resolve('u', 'ab', 'c') + + expect(mocks.resolvePermission).toHaveBeenCalledTimes(2) + }) + + it('re-queries after a rejection instead of caching the failure', async () => { + mocks.resolvePermission.mockRejectedValueOnce(new Error('connection reset')) + const cache = createWorkspacePermissionCache() + + await expect(cache.resolve('user-1', 'workspace-1', null)).rejects.toThrow('connection reset') + expect(await cache.resolve('user-1', 'workspace-1', null)).toBe('write') + + expect(mocks.resolvePermission).toHaveBeenCalledTimes(2) + }) +}) 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..b49502fc40e 100644 --- a/apps/sim/lib/knowledge/application/bulk.ts +++ b/apps/sim/lib/knowledge/application/bulk.ts @@ -1,6 +1,6 @@ import { AuditAction, AuditResourceType } from '@sim/audit' import { createLogger } from '@sim/logger' -import { authorizeWorkspaceOperation } from '@/lib/core/application' +import { authorizeWorkspaceOperation, createWorkspacePermissionCache } from '@/lib/core/application' import { classifyBulkItemError } from '@/lib/core/application/bulk-items' import { OrchestrationError } from '@/lib/core/orchestration/types' import { PlatformEvents } from '@/lib/core/telemetry' @@ -12,7 +12,10 @@ import { planFolderSelection, } from '@/lib/folders/bulk' import { findActiveFolder } from '@/lib/folders/queries' -import { knowledgeDelegationPolicy } from '@/lib/knowledge/application/authorization' +import { + type KnowledgeAuthorizationOptions, + knowledgeDelegationPolicy, +} from '@/lib/knowledge/application/authorization' import { defineAuthorizedKnowledgeUseCase } from '@/lib/knowledge/application/authorized-knowledge-use-case' import { type BoundedKnowledgeSelection, @@ -26,7 +29,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,20 +122,27 @@ async function resolveBulkKnowledgeContext( */ async function runKnowledgeItems( knowledgeBaseIds: readonly string[], - workspaceId: string, + workspace: KnowledgeWorkspaceContext, covered: ReadonlySet, - authorize: (canonical: ActiveKnowledgeBaseContext) => Promise, + authorize: ( + canonical: ActiveKnowledgeBaseContext, + options: KnowledgeAuthorizationOptions + ) => Promise, apply: (canonical: ActiveKnowledgeBaseContext) => Promise, succeeded: BulkKnowledgeItem[], outcome: BulkKnowledgeOutcome ): Promise { + /** + * Built here rather than by each caller so a bulk loop cannot forget it: every item authorizes + * against the same `(user, workspace, organization)` triple, and without the memo that is two + * identical queries per item. + */ + const permissionCache = createWorkspacePermissionCache() + 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)) { @@ -143,7 +153,7 @@ async function runKnowledgeItems( }) continue } - await authorize(canonical) + await authorize(canonical, { permissionCache }) succeeded.push({ kind: 'knowledgeBase', id: canonical.knowledgeBaseId, @@ -221,10 +231,11 @@ export const bulkMoveKnowledgeItems = defineAuthorizedKnowledgeUseCase({ const terminalError = await runKnowledgeItems( context.knowledgeBaseIds, - context.workspaceId, + context, plan.covered, - (canonical) => + (canonical, options) => authorizeWorkspaceOperation(principal, knowledgeOperations.bulkMoveItems, canonical, { + ...options, delegation: knowledgeDelegationPolicy, }), async (canonical) => @@ -323,10 +334,11 @@ export const bulkDeleteKnowledgeItems = defineAuthorizedKnowledgeUseCase({ const terminalError = await runKnowledgeItems( context.knowledgeBaseIds, - context.workspaceId, + context, plan.covered, - (canonical) => + (canonical, options) => authorizeWorkspaceOperation(principal, knowledgeOperations.bulkDeleteItems, canonical, { + ...options, delegation: knowledgeDelegationPolicy, }), async (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/authorization.ts b/apps/sim/lib/table/application/authorization.ts index 85330ac85c9..4cb35db9ddd 100644 --- a/apps/sim/lib/table/application/authorization.ts +++ b/apps/sim/lib/table/application/authorization.ts @@ -2,6 +2,7 @@ import type { Principal } from '@sim/auth/principal' import { authorizeWorkspaceOperation, type WorkspaceAuthorizationContext, + type WorkspaceAuthorizationOptions, type WorkspaceDelegationPolicy, } from '@/lib/core/application' import type { TableOperation } from '@/lib/table/application/operations' @@ -30,12 +31,21 @@ export const tableDelegationPolicy: WorkspaceDelegationPolicy, + 'delegation' +> + export function authorizeTableOperation( principal: Principal, operation: TableOperation, - context: TableAuthorizationContext + context: TableAuthorizationContext, + options?: TableAuthorizationOptions ) { return authorizeWorkspaceOperation(principal, operation, context, { + ...options, delegation: tableDelegationPolicy, }) } diff --git a/apps/sim/lib/table/application/bulk.test.ts b/apps/sim/lib/table/application/bulk.test.ts index 221ef7be4b0..a9e0e645be6 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,39 @@ describe('table bulk application use cases', () => { expect(mocks.bulkMoveFolders).not.toHaveBeenCalled() }) + /** + * The batch authorizes every table separately — delegation scope is per-resource — but the + * human-permission half of that check reads the same row every time. Without the shared memo a + * hundred-table request is a hundred identical lookups, two queries deep. + * + * Two calls, not one: the use case authorizes the operation itself before the loop starts, and + * that check is outside the batch memo. What matters is that the count does not grow with the + * selection. + */ + it('resolves the caller permission once for the whole batch, however many items it 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) + const afterSmall = mocks.resolvePermission.mock.calls.length + + mocks.resolvePermission.mockClear() + const large = await move(Array.from({ length: 25 }, (_, index) => `table-${index}`)) + expect(large.moved).toHaveLength(25) + + expect(mocks.resolvePermission).toHaveBeenCalledTimes(afterSmall) + expect(afterSmall).toBe(2) + }) + 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..64c4261de8d 100644 --- a/apps/sim/lib/table/application/bulk.ts +++ b/apps/sim/lib/table/application/bulk.ts @@ -1,6 +1,7 @@ import { AuditAction, AuditResourceType } from '@sim/audit' import { resolvePrincipalAttribution } from '@sim/auth/principal' import { createLogger } from '@sim/logger' +import { createWorkspacePermissionCache } from '@/lib/core/application' import { type BulkItemDisposition, classifyBulkItemError } from '@/lib/core/application/bulk-items' import { OrchestrationError } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' @@ -13,7 +14,10 @@ import { import { findActiveFolder } from '@/lib/folders/queries' import { notifyWorkspaceTablesChanged } from '@/lib/realtime/notify' import { deleteTable, moveTableToFolder } from '@/lib/table' -import { authorizeTableOperation } from '@/lib/table/application/authorization' +import { + authorizeTableOperation, + type TableAuthorizationOptions, +} from '@/lib/table/application/authorization' import { defineAuthorizedTableUseCase } from '@/lib/table/application/authorized-table-use-case' import { type BoundedTableSelection, @@ -25,7 +29,7 @@ import { } from '@/lib/table/application/batch-policy' import { type ActiveTableContext, - resolveActiveTableContext, + resolveActiveTableInWorkspace, resolveTableWorkspaceContext, type TableWorkspaceContext, } from '@/lib/table/application/context' @@ -159,27 +163,31 @@ async function notifyBatchedTableChanges( */ async function runTableItems( tableIds: readonly string[], - workspaceId: string, + workspace: TableWorkspaceContext, covered: ReadonlySet, - authorize: (canonical: ActiveTableContext) => Promise, + authorize: (canonical: ActiveTableContext, options: TableAuthorizationOptions) => Promise, /** Runs against an already-authorized canonical table. Returns its authoritative name. */ apply: (canonical: ActiveTableContext) => Promise, succeeded: BulkTableItem[], outcome: BulkTablesOutcome ): Promise { + /** + * Built here rather than by each caller so a bulk loop cannot forget it: every item authorizes + * against the same `(user, workspace, organization)` triple, and without the memo that is two + * identical queries per item. + */ + const permissionCache = createWorkspacePermissionCache() + 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 }) continue } - await authorize(canonical) + await authorize(canonical, { permissionCache }) succeeded.push({ kind: 'table', id: canonical.table.id, @@ -249,9 +257,10 @@ export const bulkMoveTables = defineAuthorizedTableUseCase({ try { const terminalError = await runTableItems( context.tableIds, - context.workspaceId, + context, plan.covered, - (canonical) => authorizeTableOperation(principal, tableOperations.bulkMove, canonical), + (canonical, options) => + authorizeTableOperation(principal, tableOperations.bulkMove, canonical, options), async (canonical) => ( await moveTableToFolder( @@ -355,9 +364,10 @@ export const bulkDeleteTables = defineAuthorizedTableUseCase({ try { const terminalError = await runTableItems( context.tableIds, - context.workspaceId, + context, plan.covered, - (canonical) => authorizeTableOperation(principal, tableOperations.bulkDelete, canonical), + (canonical, options) => + authorizeTableOperation(principal, tableOperations.bulkDelete, canonical, options), async (canonical) => { const { archived } = await deleteTable(canonical.table.id, generateRequestId(), { expectedWorkspaceId: context.workspaceId, 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 From 6ab3c6f7c59eebe2c5f73237df6486f3d024e406 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Sat, 15 Aug 2026 16:27:34 -0700 Subject: [PATCH 2/2] fix(resources): dismiss the upload overlay on a folder drop and re-check permission per item The drag hook stops propagation on a drop it handles, so the page-level handler that cleared the upload overlay never ran and the chrome stayed up over the finished upload. Both consuming paths now share one dismissal. Drop the batch permission memo: each item in a bulk move or delete commits independently, so reusing one allow verdict let a revocation part-way through a batch go unseen by the remaining items. The workspace context is still resolved once per batch, which was the larger saving. --- .../workspace/[workspaceId]/files/files.tsx | 16 +++- apps/sim/lib/core/application/index.ts | 2 - .../application/workspace-authorization.ts | 76 ++---------------- .../workspace-permission-cache.test.ts | 80 ------------------- apps/sim/lib/knowledge/application/bulk.ts | 27 ++----- .../lib/table/application/authorization.ts | 12 +-- apps/sim/lib/table/application/bulk.test.ts | 41 +++++++--- apps/sim/lib/table/application/bulk.ts | 23 ++---- 8 files changed, 63 insertions(+), 214 deletions(-) delete mode 100644 apps/sim/lib/core/application/workspace-permission-cache.test.ts diff --git a/apps/sim/app/workspace/[workspaceId]/files/files.tsx b/apps/sim/app/workspace/[workspaceId]/files/files.tsx index 9b511b4b500..b29e169914b 100644 --- a/apps/sim/app/workspace/[workspaceId]/files/files.tsx +++ b/apps/sim/app/workspace/[workspaceId]/files/files.tsx @@ -323,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, @@ -810,6 +822,7 @@ export function Files() { externalDrop: { matches: hasExternalFiles, onDropIntoFolder: (dataTransfer, targetFolderId) => { + dismissUploadOverlay() const dropped = Array.from(dataTransfer.files ?? []) if (dropped.length > 0) void uploadFiles(dropped, targetFolderId) }, @@ -851,8 +864,7 @@ export function Files() { * began in — pulling the user out of the folder they just spring-opened to receive it. */ rowDragDropConfig.externalDropHandled() - dragCounterRef.current = 0 - setIsDraggingOver(false) + dismissUploadOverlay() const dropped = Array.from(e.dataTransfer.files) if (dropped.length > 0) await uploadFiles(dropped) } diff --git a/apps/sim/lib/core/application/index.ts b/apps/sim/lib/core/application/index.ts index 7f9b1dd7475..6ac885759ba 100644 --- a/apps/sim/lib/core/application/index.ts +++ b/apps/sim/lib/core/application/index.ts @@ -21,11 +21,9 @@ export type { WorkspaceAuthorizationContext, WorkspaceAuthorizationOptions, WorkspaceDelegationPolicy, - WorkspacePermissionCache, } from '@/lib/core/application/workspace-authorization' export { authorizeWorkspaceOperation, - createWorkspacePermissionCache, DelegatedServiceAuthorizationError, DelegatedWorkspaceAuthorizationError, InsufficientWorkspacePermissionsError, diff --git a/apps/sim/lib/core/application/workspace-authorization.ts b/apps/sim/lib/core/application/workspace-authorization.ts index a8ce8115552..4014afb158f 100644 --- a/apps/sim/lib/core/application/workspace-authorization.ts +++ b/apps/sim/lib/core/application/workspace-authorization.ts @@ -27,65 +27,6 @@ export interface WorkspaceAuthorizationOptions forUpdate?: boolean delegation?: WorkspaceDelegationPolicy - /** - * Memo for the human-permission lookup, supplied by a caller that authorizes many items in one - * operation. Ignored alongside `executor` or `forUpdate` — see - * {@link createWorkspacePermissionCache}. - */ - permissionCache?: WorkspacePermissionCache -} - -export interface WorkspacePermissionCache { - resolve( - userId: string, - workspaceId: string, - workspaceOrganizationId: string | null - ): Promise -} - -/** - * Memoizes the effective-permission lookup across the items of one bulk operation. - * - * A batch authorizes every item separately — delegation scope is per-resource, so the check - * cannot simply be hoisted out of the loop — but the human-permission half of it reads the same - * `(user, workspace, organization)` triple every time, two queries deep. On a hundred-item - * request that is two hundred round trips for a value that cannot change within the batch. - * - * Caller-owned and request-scoped on purpose: nothing here outlives the operation that created - * it, so a permission changed between requests is always seen by the next one. Skipped entirely - * when the caller passes its own `executor` (a transaction has its own snapshot to honour) or - * `forUpdate` (that lookup takes a row lock, which is a side effect, not a read). - * - * Neither of the repo's two existing memo idioms fits. `coalesceLocally` evicts on settle, so a - * sequential per-item loop would re-query every item. React `cache()` cannot be skipped per call - * for the `executor`/`forUpdate` paths and has no request scope in the worker runtime. An - * implicit process-wide memo on an authorization read is a lifetime worth refusing outright. - */ -export function createWorkspacePermissionCache(): WorkspacePermissionCache { - const entries = new Map>() - return { - resolve(userId, workspaceId, workspaceOrganizationId) { - /** Structural, so no id can run into the next and answer another workspace's question. */ - const key = JSON.stringify([userId, workspaceId, workspaceOrganizationId]) - const cached = entries.get(key) - if (cached) return cached - /** - * The in-flight promise is what gets stored, so concurrent items share one query rather - * than racing to start their own. Evicted if it rejects: a transient database failure must - * not become the permanent answer for the rest of the batch. - */ - const pending = resolveEffectiveWorkspacePermission( - userId, - workspaceId, - workspaceOrganizationId - ).catch((error) => { - entries.delete(key) - throw error - }) - entries.set(key, pending) - return pending - }, - } } export class InsufficientWorkspacePermissionsError extends ForbiddenOperationError { @@ -210,16 +151,13 @@ async function requireCurrentHumanPermission ): Promise { - const memo = options?.executor || options?.forUpdate ? undefined : options?.permissionCache - const permission = memo - ? await memo.resolve(userId, context.workspaceId, context.workspaceOrganizationId) - : await resolveEffectiveWorkspacePermission( - userId, - context.workspaceId, - context.workspaceOrganizationId, - options?.executor, - { forUpdate: options?.forUpdate } - ) + const permission = await resolveEffectiveWorkspacePermission( + userId, + context.workspaceId, + context.workspaceOrganizationId, + options?.executor, + { forUpdate: options?.forUpdate } + ) requirePermission(permission, required) } diff --git a/apps/sim/lib/core/application/workspace-permission-cache.test.ts b/apps/sim/lib/core/application/workspace-permission-cache.test.ts deleted file mode 100644 index 7f0f8e88c8c..00000000000 --- a/apps/sim/lib/core/application/workspace-permission-cache.test.ts +++ /dev/null @@ -1,80 +0,0 @@ -/** - * @vitest-environment node - */ - -import { beforeEach, describe, expect, it, vi } from 'vitest' - -const mocks = vi.hoisted(() => ({ resolvePermission: vi.fn() })) - -vi.mock('@sim/platform-authz/workspace', () => ({ - permissionSatisfies: () => true, - resolveEffectiveWorkspacePermission: mocks.resolvePermission, -})) - -import { createWorkspacePermissionCache } from '@/lib/core/application/workspace-authorization' - -describe('createWorkspacePermissionCache', () => { - beforeEach(() => { - vi.clearAllMocks() - mocks.resolvePermission.mockResolvedValue('write') - }) - - it('queries once for a repeated triple', async () => { - const cache = createWorkspacePermissionCache() - - const results = await Promise.all([ - cache.resolve('user-1', 'workspace-1', 'organization-1'), - cache.resolve('user-1', 'workspace-1', 'organization-1'), - cache.resolve('user-1', 'workspace-1', 'organization-1'), - ]) - - expect(results).toEqual(['write', 'write', 'write']) - expect(mocks.resolvePermission).toHaveBeenCalledExactlyOnceWith( - 'user-1', - 'workspace-1', - 'organization-1' - ) - }) - - it('keeps a null answer, which is a real verdict rather than a cache miss', async () => { - mocks.resolvePermission.mockResolvedValue(null) - const cache = createWorkspacePermissionCache() - - expect(await cache.resolve('user-1', 'workspace-1', null)).toBeNull() - expect(await cache.resolve('user-1', 'workspace-1', null)).toBeNull() - - expect(mocks.resolvePermission).toHaveBeenCalledTimes(1) - }) - - it('separates entries that differ in any part of the triple', async () => { - const cache = createWorkspacePermissionCache() - - await cache.resolve('user-1', 'workspace-1', 'organization-1') - await cache.resolve('user-2', 'workspace-1', 'organization-1') - await cache.resolve('user-1', 'workspace-2', 'organization-1') - await cache.resolve('user-1', 'workspace-1', null) - - expect(mocks.resolvePermission).toHaveBeenCalledTimes(4) - }) - - it('does not let a workspace id run into an organization id', async () => { - // A naive concatenation makes ('u', 'a', 'bc') and ('u', 'ab', 'c') the same entry, which - // would answer one workspace's authorization question with another's permission row. - const cache = createWorkspacePermissionCache() - - await cache.resolve('u', 'a', 'bc') - await cache.resolve('u', 'ab', 'c') - - expect(mocks.resolvePermission).toHaveBeenCalledTimes(2) - }) - - it('re-queries after a rejection instead of caching the failure', async () => { - mocks.resolvePermission.mockRejectedValueOnce(new Error('connection reset')) - const cache = createWorkspacePermissionCache() - - await expect(cache.resolve('user-1', 'workspace-1', null)).rejects.toThrow('connection reset') - expect(await cache.resolve('user-1', 'workspace-1', null)).toBe('write') - - expect(mocks.resolvePermission).toHaveBeenCalledTimes(2) - }) -}) diff --git a/apps/sim/lib/knowledge/application/bulk.ts b/apps/sim/lib/knowledge/application/bulk.ts index b49502fc40e..f838260ad78 100644 --- a/apps/sim/lib/knowledge/application/bulk.ts +++ b/apps/sim/lib/knowledge/application/bulk.ts @@ -1,6 +1,6 @@ import { AuditAction, AuditResourceType } from '@sim/audit' import { createLogger } from '@sim/logger' -import { authorizeWorkspaceOperation, createWorkspacePermissionCache } from '@/lib/core/application' +import { authorizeWorkspaceOperation } from '@/lib/core/application' import { classifyBulkItemError } from '@/lib/core/application/bulk-items' import { OrchestrationError } from '@/lib/core/orchestration/types' import { PlatformEvents } from '@/lib/core/telemetry' @@ -12,10 +12,7 @@ import { planFolderSelection, } from '@/lib/folders/bulk' import { findActiveFolder } from '@/lib/folders/queries' -import { - type KnowledgeAuthorizationOptions, - knowledgeDelegationPolicy, -} from '@/lib/knowledge/application/authorization' +import { knowledgeDelegationPolicy } from '@/lib/knowledge/application/authorization' import { defineAuthorizedKnowledgeUseCase } from '@/lib/knowledge/application/authorized-knowledge-use-case' import { type BoundedKnowledgeSelection, @@ -124,21 +121,11 @@ async function runKnowledgeItems( knowledgeBaseIds: readonly string[], workspace: KnowledgeWorkspaceContext, covered: ReadonlySet, - authorize: ( - canonical: ActiveKnowledgeBaseContext, - options: KnowledgeAuthorizationOptions - ) => Promise, + authorize: (canonical: ActiveKnowledgeBaseContext) => Promise, apply: (canonical: ActiveKnowledgeBaseContext) => Promise, succeeded: BulkKnowledgeItem[], outcome: BulkKnowledgeOutcome ): Promise { - /** - * Built here rather than by each caller so a bulk loop cannot forget it: every item authorizes - * against the same `(user, workspace, organization)` triple, and without the memo that is two - * identical queries per item. - */ - const permissionCache = createWorkspacePermissionCache() - for (const knowledgeBaseId of knowledgeBaseIds) { let knowledgeBaseName = knowledgeBaseId try { @@ -153,7 +140,7 @@ async function runKnowledgeItems( }) continue } - await authorize(canonical, { permissionCache }) + await authorize(canonical) succeeded.push({ kind: 'knowledgeBase', id: canonical.knowledgeBaseId, @@ -233,9 +220,8 @@ export const bulkMoveKnowledgeItems = defineAuthorizedKnowledgeUseCase({ context.knowledgeBaseIds, context, plan.covered, - (canonical, options) => + (canonical) => authorizeWorkspaceOperation(principal, knowledgeOperations.bulkMoveItems, canonical, { - ...options, delegation: knowledgeDelegationPolicy, }), async (canonical) => @@ -336,9 +322,8 @@ export const bulkDeleteKnowledgeItems = defineAuthorizedKnowledgeUseCase({ context.knowledgeBaseIds, context, plan.covered, - (canonical, options) => + (canonical) => authorizeWorkspaceOperation(principal, knowledgeOperations.bulkDeleteItems, canonical, { - ...options, delegation: knowledgeDelegationPolicy, }), async (canonical) => { diff --git a/apps/sim/lib/table/application/authorization.ts b/apps/sim/lib/table/application/authorization.ts index 4cb35db9ddd..85330ac85c9 100644 --- a/apps/sim/lib/table/application/authorization.ts +++ b/apps/sim/lib/table/application/authorization.ts @@ -2,7 +2,6 @@ import type { Principal } from '@sim/auth/principal' import { authorizeWorkspaceOperation, type WorkspaceAuthorizationContext, - type WorkspaceAuthorizationOptions, type WorkspaceDelegationPolicy, } from '@/lib/core/application' import type { TableOperation } from '@/lib/table/application/operations' @@ -31,21 +30,12 @@ export const tableDelegationPolicy: WorkspaceDelegationPolicy, - 'delegation' -> - export function authorizeTableOperation( principal: Principal, operation: TableOperation, - context: TableAuthorizationContext, - options?: TableAuthorizationOptions + context: TableAuthorizationContext ) { return authorizeWorkspaceOperation(principal, operation, context, { - ...options, delegation: tableDelegationPolicy, }) } diff --git a/apps/sim/lib/table/application/bulk.test.ts b/apps/sim/lib/table/application/bulk.test.ts index a9e0e645be6..ddfa56d7ad0 100644 --- a/apps/sim/lib/table/application/bulk.test.ts +++ b/apps/sim/lib/table/application/bulk.test.ts @@ -292,15 +292,15 @@ describe('table bulk application use cases', () => { }) /** - * The batch authorizes every table separately — delegation scope is per-resource — but the - * human-permission half of that check reads the same row every time. Without the shared memo a - * hundred-table request is a hundred identical lookups, two queries deep. + * 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. * - * Two calls, not one: the use case authorizes the operation itself before the loop starts, and - * that check is outside the batch memo. What matters is that the count does not grow with the - * selection. + * 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('resolves the caller permission once for the whole batch, however many items it carries', async () => { + it('loads the workspace context once however many items the batch carries', async () => { const move = (tableIds: string[]) => bulkMoveTables.execute({ principal, @@ -314,14 +314,33 @@ describe('table bulk application use cases', () => { const small = await move(['table-1', 'table-2', 'table-3']) expect(small.moved).toHaveLength(3) - const afterSmall = mocks.resolvePermission.mock.calls.length + expect(mocks.resolveWorkspaceContext).toHaveBeenCalledTimes(1) - mocks.resolvePermission.mockClear() + 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(mocks.resolvePermission).toHaveBeenCalledTimes(afterSmall) - expect(afterSmall).toBe(2) + 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 () => { diff --git a/apps/sim/lib/table/application/bulk.ts b/apps/sim/lib/table/application/bulk.ts index 64c4261de8d..ba046391444 100644 --- a/apps/sim/lib/table/application/bulk.ts +++ b/apps/sim/lib/table/application/bulk.ts @@ -1,7 +1,6 @@ import { AuditAction, AuditResourceType } from '@sim/audit' import { resolvePrincipalAttribution } from '@sim/auth/principal' import { createLogger } from '@sim/logger' -import { createWorkspacePermissionCache } from '@/lib/core/application' import { type BulkItemDisposition, classifyBulkItemError } from '@/lib/core/application/bulk-items' import { OrchestrationError } from '@/lib/core/orchestration/types' import { generateRequestId } from '@/lib/core/utils/request' @@ -14,10 +13,7 @@ import { import { findActiveFolder } from '@/lib/folders/queries' import { notifyWorkspaceTablesChanged } from '@/lib/realtime/notify' import { deleteTable, moveTableToFolder } from '@/lib/table' -import { - authorizeTableOperation, - type TableAuthorizationOptions, -} from '@/lib/table/application/authorization' +import { authorizeTableOperation } from '@/lib/table/application/authorization' import { defineAuthorizedTableUseCase } from '@/lib/table/application/authorized-table-use-case' import { type BoundedTableSelection, @@ -165,19 +161,12 @@ async function runTableItems( tableIds: readonly string[], workspace: TableWorkspaceContext, covered: ReadonlySet, - authorize: (canonical: ActiveTableContext, options: TableAuthorizationOptions) => Promise, + authorize: (canonical: ActiveTableContext) => Promise, /** Runs against an already-authorized canonical table. Returns its authoritative name. */ apply: (canonical: ActiveTableContext) => Promise, succeeded: BulkTableItem[], outcome: BulkTablesOutcome ): Promise { - /** - * Built here rather than by each caller so a bulk loop cannot forget it: every item authorizes - * against the same `(user, workspace, organization)` triple, and without the memo that is two - * identical queries per item. - */ - const permissionCache = createWorkspacePermissionCache() - for (const tableId of tableIds) { let tableName = tableId try { @@ -187,7 +176,7 @@ async function runTableItems( outcome.skipped.push({ kind: 'table', id: canonical.table.id, name: tableName }) continue } - await authorize(canonical, { permissionCache }) + await authorize(canonical) succeeded.push({ kind: 'table', id: canonical.table.id, @@ -259,8 +248,7 @@ export const bulkMoveTables = defineAuthorizedTableUseCase({ context.tableIds, context, plan.covered, - (canonical, options) => - authorizeTableOperation(principal, tableOperations.bulkMove, canonical, options), + (canonical) => authorizeTableOperation(principal, tableOperations.bulkMove, canonical), async (canonical) => ( await moveTableToFolder( @@ -366,8 +354,7 @@ export const bulkDeleteTables = defineAuthorizedTableUseCase({ context.tableIds, context, plan.covered, - (canonical, options) => - authorizeTableOperation(principal, tableOperations.bulkDelete, canonical, options), + (canonical) => authorizeTableOperation(principal, tableOperations.bulkDelete, canonical), async (canonical) => { const { archived } = await deleteTable(canonical.table.id, generateRequestId(), { expectedWorkspaceId: context.workspaceId,