fix(folders): cleanup pass + fix stale cycle-check race in workflow folder update

Cleanup pass over the remaining audit findings (dead code, stale comments,
duplicated logic, minor hygiene), plus two real bugs caught along the way:

- workflow folder-lifecycle.ts: performUpdateFolder had the same stale
  cycle-check race already fixed in the generic kb/table path -- the
  circular-reference check ran against an unlocked pre-transaction snapshot,
  so two concurrent moves (A under B, B under A) could each pass and commit
  a persisted cycle. Re-check inside the transaction now, matching the
  generic path. (Caught by Greptile on the post-audit re-review.)
- folders/orchestration.ts: performUpdateFileFolder was swallowing
  ResourceLockedError into a generic 500 instead of propagating it for the
  route's 423 handling, unlike its create/delete/restore siblings.
- knowledge/[id]/route.ts: the admin-lock-permission check for PUT only
  verified admin on the KB's *current* workspace, even though workspaceId
  and locked can change in the same request -- now checks both current and
  target workspace.

Other fixes: extracted duplicated lock+recheck-parent logic in
folders/orchestration.ts into a shared helper; removed dead `validUpdates`
aliases; removed dead `color` field from workflow folder params; fixed
stale comments claiming `locked`/file-folder locking is workflow-only or
unused (the generic lock engine treats all four resourceTypes uniformly);
added resourceType scoping to log folder-descendant expansion; fixed an
O(n*m) descendant-collection loop in the files-download route to build its
parent->children index once; added typed TableInvalidFolderError instead of
string-matching error messages; fixed a hardcoded SQL column literal in
pinned-items lookups; deduplicated FolderResourceType between the folders
contract and the folders store; derived a magic query-key array index from
the key factory instead of a literal; moved a render-time ref mutation in
files.tsx into an effect; memoized a per-render Object.fromEntries in
knowledge.tsx; fixed recently-deleted.tsx passing no resourceType on file
folder restore (silently defaulted to workflow); aligned a stray
eslint-disable in tables.tsx with its file/knowledge siblings.

Fixed a knowledge-route test whose hand-rolled db mock had no `transaction`
stub (needed after createKnowledgeBase was wrapped in a transaction to
close its own folder-lock TOCTOU window) and whose beforeEach was
clobbering that stub every test.

Author: waleedlatif1

apps/sim/app/api/folders/reorder/route.ts

@@ -59,17 +59,13 @@ export const PUT = withRouteHandler(async (req: NextRequest) => {
     if (hasInvalidId) {
       return NextResponse.json({ error: 'One or more folders were not found' }, { status: 400 })
     }
-    const validUpdates = updates
-
     // A single reorder call operates on one resourceType at a time (the UI never mixes
     // folder types in one drag-drop tree). Reject a mixed-type batch explicitly instead
     // of silently reordering only the first-seen type and reporting success — a caller
     // bug that sends folders from two resource types should surface as an error, not a
     // partial `updated` count with no indication some entries were skipped.
-    const resourceType = resourceTypeById.get(validUpdates[0].id)!
-    const hasMixedResourceTypes = validUpdates.some(
-      (u) => resourceTypeById.get(u.id) !== resourceType
-    )
+    const resourceType = resourceTypeById.get(updates[0].id)!
+    const hasMixedResourceTypes = updates.some((u) => resourceTypeById.get(u.id) !== resourceType)
     if (hasMixedResourceTypes) {
       return NextResponse.json(
         { error: 'All folders in a reorder batch must share the same resourceType' },
@@ -81,7 +77,7 @@ export const PUT = withRouteHandler(async (req: NextRequest) => {
     // `performReorderFolders` (via `assertFolderParentValid`) below — this route does
     // not duplicate that check, it only guards the self-parent case which is a
     // cheap synchronous comparison, not a DB round-trip.
-    for (const update of validUpdates) {
+    for (const update of updates) {
       if (update.parentId && update.parentId === update.id) {
         return NextResponse.json({ error: 'Folder cannot be its own parent' }, { status: 400 })
       }
@@ -102,13 +98,13 @@ export const PUT = withRouteHandler(async (req: NextRequest) => {
     for (const folderRow of workspaceFolders) {
       parentById.set(folderRow.id, folderRow.parentId)
     }
-    for (const update of validUpdates) {
+    for (const update of updates) {
       if (update.parentId !== undefined) {
         parentById.set(update.id, update.parentId || null)
       }
     }
 
-    for (const update of validUpdates) {
+    for (const update of updates) {
       const visited = new Set<string>()
       let cursor: string | null = update.id
       while (cursor) {
@@ -124,7 +120,7 @@ export const PUT = withRouteHandler(async (req: NextRequest) => {
     }
 
     const policy = FOLDER_RESOURCE_POLICIES[resourceType]
-    for (const update of validUpdates) {
+    for (const update of updates) {
       await policy.assertMutable(update.id)
       if (update.parentId !== undefined) {
         await policy.assertMutable(update.parentId)
@@ -134,7 +130,7 @@ export const PUT = withRouteHandler(async (req: NextRequest) => {
     const result = await performReorderFolders({
       resourceType,
       workspaceId,
-      updates: validUpdates,
+      updates,
     })
 
     if (!result.success) {

apps/sim/app/api/folders/route.ts

@@ -9,17 +9,11 @@ import { performCreateFolder } from '@/lib/folders/orchestration'
 import { FOLDER_RESOURCE_POLICIES } from '@/lib/folders/policy'
 import { listFoldersForWorkspace } from '@/lib/folders/queries'
 import { captureServerEvent } from '@/lib/posthog/server'
+import { statusForOrchestrationError } from '@/lib/workflows/orchestration/types'
 import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
 
 const logger = createLogger('FoldersAPI')
 
-function folderMutationStatus(errorCode: string | undefined): number {
-  if (errorCode === 'validation') return 400
-  if (errorCode === 'conflict') return 409
-  if (errorCode === 'not_found') return 404
-  return 500
-}
-
 export const GET = withRouteHandler(async (request: NextRequest) => {
   try {
     const session = await getSession()
@@ -96,7 +90,7 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
     if (!result.success || !result.folder) {
       return NextResponse.json(
         { error: result.error },
-        { status: folderMutationStatus(result.errorCode) }
+        { status: statusForOrchestrationError(result.errorCode) }
       )
     }
 

apps/sim/app/api/knowledge/[id]/route.ts

@@ -98,15 +98,19 @@ export const PUT = withRouteHandler(
       const validatedData = parsed.data.body
 
       if (validatedData.locked !== undefined) {
-        const workspaceId = accessCheck.knowledgeBase.workspaceId
-        const workspacePermission = workspaceId
-          ? await getUserEntityPermissions(userId, 'workspace', workspaceId)
-          : null
-        if (workspacePermission !== 'admin') {
-          return NextResponse.json(
-            { error: 'Admin access required to lock knowledge bases' },
-            { status: 403 }
-          )
+        const currentWorkspaceId = accessCheck.knowledgeBase.workspaceId
+        const targetWorkspaceId = validatedData.workspaceId ?? currentWorkspaceId
+        const workspaceIdsToCheck = new Set(
+          [currentWorkspaceId, targetWorkspaceId].filter((wsId): wsId is string => Boolean(wsId))
+        )
+        for (const wsId of workspaceIdsToCheck) {
+          const workspacePermission = await getUserEntityPermissions(userId, 'workspace', wsId)
+          if (workspacePermission !== 'admin') {
+            return NextResponse.json(
+              { error: 'Admin access required to lock knowledge bases' },
+              { status: 403 }
+            )
+          }
         }
       }
 

apps/sim/app/api/knowledge/route.test.ts

@@ -13,7 +13,7 @@ import {
 import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'
 
 const { mockDbChain } = vi.hoisted(() => {
-  const mockDbChain = {
+  const mockDbChain: Record<string, any> = {
     select: vi.fn().mockReturnThis(),
     from: vi.fn().mockReturnThis(),
     leftJoin: vi.fn().mockReturnThis(),
@@ -24,6 +24,7 @@ const { mockDbChain } = vi.hoisted(() => {
     insert: vi.fn().mockReturnThis(),
     values: vi.fn().mockResolvedValue(undefined),
   }
+  mockDbChain.transaction = vi.fn((cb: (tx: typeof mockDbChain) => unknown) => cb(mockDbChain))
   return { mockDbChain }
 })
 
@@ -44,11 +45,19 @@ describe('Knowledge Base API Route', () => {
     Object.values(mockDbChain).forEach((fn) => {
       if (typeof fn === 'function') {
         fn.mockClear()
-        if (fn !== mockDbChain.orderBy && fn !== mockDbChain.values && fn !== mockDbChain.limit) {
+        if (
+          fn !== mockDbChain.orderBy &&
+          fn !== mockDbChain.values &&
+          fn !== mockDbChain.limit &&
+          fn !== mockDbChain.transaction
+        ) {
           fn.mockReturnThis()
         }
       }
     })
+    mockDbChain.transaction.mockImplementation((cb: (tx: typeof mockDbChain) => unknown) =>
+      cb(mockDbChain)
+    )
 
     permissionsMockFns.mockGetUserEntityPermissions.mockResolvedValue('admin')
 

apps/sim/app/api/pinned-items/route.ts

@@ -10,7 +10,7 @@ import {
 import { createLogger } from '@sim/logger'
 import { getPostgresErrorCode } from '@sim/utils/errors'
 import { generateId } from '@sim/utils/id'
-import { and, eq, inArray, isNull, sql } from 'drizzle-orm'
+import { and, eq, inArray, isNull } from 'drizzle-orm'
 import type { PgColumn, PgTable } from 'drizzle-orm/pg-core'
 import { type NextRequest, NextResponse } from 'next/server'
 import type { PinnedResourceType } from '@/lib/api/contracts'
@@ -82,7 +82,7 @@ async function resourceExistsInWorkspace(
 ): Promise<boolean> {
   const config = PINNED_RESOURCE_LOOKUP[resourceType]
   const [row] = await db
-    .select({ id: sql<string>`id` })
+    .select({ id: config.idColumn })
     .from(config.resourceTable)
     .where(
       and(

apps/sim/app/api/table/[tableId]/route.ts

@@ -8,7 +8,13 @@ import { checkSessionOrInternalAuth } from '@/lib/auth/hybrid'
 import { generateRequestId } from '@/lib/core/utils/request'
 import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
 import { captureServerEvent } from '@/lib/posthog/server'
-import { deleteTable, renameTable, TableConflictError, type TableSchema } from '@/lib/table'
+import {
+  deleteTable,
+  renameTable,
+  TableConflictError,
+  TableInvalidFolderError,
+  type TableSchema,
+} from '@/lib/table'
 import { getWorkspaceTableLimits } from '@/lib/table/billing'
 import { getUserEntityPermissions } from '@/lib/workspaces/permissions/utils'
 import { accessError, checkAccess, normalizeColumn } from '@/app/api/table/utils'
@@ -168,7 +174,7 @@ export const PATCH = withRouteHandler(
       if (error instanceof TableConflictError) {
         return NextResponse.json({ error: error.message }, { status: 409 })
       }
-      if (error instanceof Error && error.message.includes('Invalid folderId')) {
+      if (error instanceof TableInvalidFolderError) {
         return NextResponse.json({ error: error.message }, { status: 400 })
       }
 

apps/sim/app/api/table/route.ts

@@ -12,6 +12,7 @@ import {
   createTable,
   getWorkspaceTableLimits,
   listTables,
+  TableInvalidFolderError,
   type TableSchema,
   type TableScope,
 } from '@/lib/table'
@@ -148,14 +149,17 @@ export const POST = withRouteHandler(async (request: NextRequest) => {
       return NextResponse.json({ error: error.message }, { status: error.status })
     }
 
+    if (error instanceof TableInvalidFolderError) {
+      return NextResponse.json({ error: error.message }, { status: 400 })
+    }
+
     if (error instanceof Error) {
       if (error.message.includes('maximum table limit')) {
         return NextResponse.json({ error: error.message }, { status: 403 })
       }
       if (
         error.message.includes('Invalid table name') ||
         error.message.includes('Invalid schema') ||
-        error.message.includes('Invalid folderId') ||
         error.message.includes('already exists')
       ) {
         return NextResponse.json({ error: error.message }, { status: 400 })

apps/sim/app/api/workspaces/[id]/files/download/route.ts

@@ -6,7 +6,7 @@ import { downloadWorkspaceFileItemsContract } from '@/lib/api/contracts/workspac
 import { parseRequest } from '@/lib/api/server'
 import { getSession } from '@/lib/auth'
 import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
-import { collectDescendantFolderIds } from '@/lib/folders/subtree'
+import type { FolderSubtreeRow } from '@/lib/folders/subtree'
 import { captureServerEvent } from '@/lib/posthog/server'
 import {
   buildWorkspaceFileFolderPathMap,
@@ -43,17 +43,32 @@ function withZipPathSuffix(path: string, suffix: number): string {
     : `${directory}${filename} (${suffix})`
 }
 
-/** Unions each root folder id with every descendant reachable from it. */
-function collectSelectedFolderIds(
-  rootIds: string[],
-  folders: Array<{ id: string; parentId: string | null }>
-): Set<string> {
+/**
+ * Unions each root folder id with every descendant reachable from it.
+ * Builds the parent→children index once and reuses it across all roots,
+ * instead of rebuilding it per root as a per-root `collectDescendantFolderIds`
+ * call would.
+ */
+function collectSelectedFolderIds(rootIds: string[], folders: FolderSubtreeRow[]): Set<string> {
+  const childrenByParent = new Map<string, string[]>()
+  for (const folder of folders) {
+    if (!folder.parentId) continue
+    const children = childrenByParent.get(folder.parentId) ?? []
+    children.push(folder.id)
+    childrenByParent.set(folder.parentId, children)
+  }
+
   const folderIds = new Set(rootIds)
-  for (const rootId of rootIds) {
-    for (const descendantId of collectDescendantFolderIds(folders, rootId)) {
-      folderIds.add(descendantId)
+  const visit = (id: string) => {
+    for (const childId of childrenByParent.get(id) ?? []) {
+      if (folderIds.has(childId)) continue
+      folderIds.add(childId)
+      visit(childId)
     }
   }
+  for (const rootId of rootIds) {
+    visit(rootId)
+  }
   return folderIds
 }
 

apps/sim/app/workspace/[workspaceId]/files/files.tsx

@@ -599,7 +599,6 @@ export function Files() {
   const [prevVisibleRowIds, setPrevVisibleRowIds] = useState(visibleRowIds)
   if (prevVisibleRowIds !== visibleRowIds) {
     setPrevVisibleRowIds(visibleRowIds)
-    lastSelectedIndexRef.current = -1
     const visible = new Set(visibleRowIds)
     setSelectedRowIds((prev) => {
       if (prev.size === 0) return prev
@@ -608,6 +607,14 @@ export function Files() {
     })
   }
 
+  // lastSelectedIndexRef is only read inside event handlers, never during
+  // render/JSX, so it stays a ref — but writing ref.current during render is
+  // forbidden (react.dev, useRef → "Do not write or read ref.current during
+  // rendering"), so the reset runs in an Effect keyed on the same transition.
+  useEffect(() => {
+    lastSelectedIndexRef.current = -1
+  }, [visibleRowIds])
+
   const isAllSelected =
     visibleRowIds.length > 0 && visibleRowIds.every((id) => selectedRowIds.has(id))
   const { selectedFileIds, selectedFolderIds } = useMemo(() => {

apps/sim/app/workspace/[workspaceId]/knowledge/knowledge.tsx

@@ -314,6 +314,7 @@ export function Knowledge() {
   )
 
   const folderById = useMemo(() => new Map(folders.map((folder) => [folder.id, folder])), [folders])
+  const folderByIdRecord = useMemo(() => Object.fromEntries(folderById), [folderById])
 
   const activeFolder = activeFolderId ? (folderById.get(activeFolderId) ?? null) : null
 
@@ -605,7 +606,7 @@ export function Knowledge() {
 
   const knowledgeBaseInheritedLocked = isFolderOrAncestorLocked(
     activeKnowledgeBase?.folderId ?? null,
-    Object.fromEntries(folderById)
+    folderByIdRecord
   )
 
   const handleToggleKnowledgeBaseLock = useCallback(() => {
@@ -636,7 +637,7 @@ export function Knowledge() {
 
   const folderInheritedLocked = isFolderOrAncestorLocked(
     activeFolder?.parentId ?? null,
-    Object.fromEntries(folderById)
+    folderByIdRecord
   )
 
   const handleToggleFolderLock = useCallback(() => {