fix(folders): stop archived rows skewing sortOrder, hide them from admin, and cover the two untested modules

nextFolderSortOrder returned min - 1 over ALL rows including soft-deleted ones,
so every delete ratcheted the floor further negative and never recovered — an
archived folder at -400 forced the next new folder to -401 forever. Both minima
(folders and child resources) now see only rows a user can still see, which is
how the Files path has always worked.

The admin workspace-folders endpoint counted and paginated soft-deleted folders,
so an operator saw phantom folders and an inflated total, disagreeing with every
user-facing list.

Adds naming.test.ts and queries.test.ts. Both modules had zero tests and are
mocked at every call site, so their bodies executed in no test anywhere. That
left unasserted the two bug classes that caused real defects in the folder
migration: the suffix sequence (must start at (1) and skip taken suffixes) and
resourceType scoping on the id-keyed lookups, where a missing clause silently
files a knowledge base under a table folder.

Every assertion is mutation-checked. The first version of the sortOrder test was
vacuous — for a root folder the parent condition is itself an isNull node, so a
presence-only check passed with the soft-delete filter deleted; it now asserts
the specific column.

Author: waleedlatif1

apps/sim/app/api/v1/admin/workspaces/[id]/folders/route.test.ts

@@ -0,0 +1,89 @@
+/**
+ * @vitest-environment node
+ */
+import {
+  createMockRequest,
+  dbChainMockFns,
+  queueTableRows,
+  resetDbChainMock,
+  schemaMock,
+} from '@sim/testing'
+import { beforeEach, describe, expect, it, vi } from 'vitest'
+
+/**
+ * The route composes `withAdminAuthParams`, so auth is bypassed by making that wrapper a
+ * passthrough — the assertions here are about query construction, not the auth gate.
+ */
+vi.mock('@/app/api/v1/admin/middleware', () => ({
+  withAdminAuthParams: (handler: unknown) => handler,
+}))
+
+import { GET } from '@/app/api/v1/admin/workspaces/[id]/folders/route'
+
+const WORKSPACE_ID = 'ws-1'
+const routeContext = { params: Promise.resolve({ id: WORKSPACE_ID }) }
+
+function listRequest() {
+  return createMockRequest(
+    'GET',
+    undefined,
+    {},
+    `http://localhost:3000/api/v1/admin/workspaces/${WORKSPACE_ID}/folders?limit=50&offset=0`
+  )
+}
+
+/** Flattens the nested `and(...)` objects the drizzle operator mocks produce. */
+function flattenConditions(condition: unknown): Array<Record<string, unknown>> {
+  if (!condition || typeof condition !== 'object') return []
+  const node = condition as Record<string, unknown>
+  if (node.type === 'and' && Array.isArray(node.conditions)) {
+    return node.conditions.flatMap(flattenConditions)
+  }
+  return [node]
+}
+
+describe('admin workspace folders GET', () => {
+  beforeEach(() => {
+    vi.clearAllMocks()
+    resetDbChainMock()
+  })
+
+  /**
+   * Both the count and the page must exclude soft-deleted folders. Without the filter an operator
+   * inspecting a workspace sees folders that live in Recently Deleted and an inflated total, and
+   * this endpoint disagrees with every user-facing folder list — all of which filter `deletedAt`.
+   */
+  it('excludes soft-deleted folders from both the count and the page', async () => {
+    queueTableRows(schemaMock.workspace, [{ id: WORKSPACE_ID }])
+    queueTableRows(schemaMock.folder, [{ total: 0 }])
+    queueTableRows(schemaMock.folder, [])
+
+    await GET(listRequest(), routeContext)
+
+    // Calls: [0] workspace lookup, then the count and page share one prebuilt condition.
+    const folderWheres = dbChainMockFns.where.mock.calls.slice(1).map(([where]) => where)
+    expect(folderWheres.length).toBeGreaterThanOrEqual(2)
+    for (const where of folderWheres) {
+      // Asserted on the COLUMN: `resourceType`/`workspaceId` are eq nodes, so a bare
+      // "some isNull exists" check could pass on an unrelated clause.
+      expect(
+        flattenConditions(where).some(
+          (node) => node.type === 'isNull' && node.column === schemaMock.folder.deletedAt
+        )
+      ).toBe(true)
+    }
+  })
+
+  it('still scopes to the workspace and to workflow folders', async () => {
+    queueTableRows(schemaMock.workspace, [{ id: WORKSPACE_ID }])
+    queueTableRows(schemaMock.folder, [{ total: 0 }])
+    queueTableRows(schemaMock.folder, [])
+
+    await GET(listRequest(), routeContext)
+
+    const where = dbChainMockFns.where.mock.calls[1]?.[0]
+    const nodes = flattenConditions(where)
+    expect(nodes.some((n) => n.type === 'eq' && n.right === WORKSPACE_ID)).toBe(true)
+    expect(nodes.some((n) => n.type === 'eq' && n.right === 'workflow')).toBe(true)
+  })
+})

apps/sim/app/api/v1/admin/workspaces/[id]/folders/route.ts

@@ -13,7 +13,7 @@
 import { db } from '@sim/db'
 import { folder as folderTable, workspace } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
-import { and, count, eq } from 'drizzle-orm'
+import { and, count, eq, isNull } from 'drizzle-orm'
 import { adminV1ListWorkspaceFoldersContract } from '@/lib/api/contracts/v1/admin'
 import { parseRequest } from '@/lib/api/server'
 import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
@@ -46,19 +46,24 @@ export const GET = withRouteHandler(
         return notFoundResponse('Workspace')
       }
 
+      /**
+       * Soft-deleted folders are excluded. Without this the count and the page both include rows
+       * sitting in Recently Deleted, so an operator inspecting a workspace sees phantom folders
+       * and an inflated total — and the two disagree with every user-facing folder list, all of
+       * which filter on `deletedAt`.
+       */
+      const activeWorkflowFolders = and(
+        eq(folderTable.workspaceId, workspaceId),
+        eq(folderTable.resourceType, 'workflow'),
+        isNull(folderTable.deletedAt)
+      )
+
       const [countResult, folders] = await Promise.all([
-        db
-          .select({ total: count() })
-          .from(folderTable)
-          .where(
-            and(eq(folderTable.workspaceId, workspaceId), eq(folderTable.resourceType, 'workflow'))
-          ),
+        db.select({ total: count() }).from(folderTable).where(activeWorkflowFolders),
         db
           .select()
           .from(folderTable)
-          .where(
-            and(eq(folderTable.workspaceId, workspaceId), eq(folderTable.resourceType, 'workflow'))
-          )
+          .where(activeWorkflowFolders)
           .orderBy(folderTable.sortOrder, folderTable.name)
           .limit(limit)
           .offset(offset),

apps/sim/lib/folders/lifecycle.test.ts

@@ -67,6 +67,16 @@ import { createFolder, deleteFolder, restoreFolder, updateFolder } from '@/lib/f
 
 const CHILD_TABLE = { name: 'child_table' }
 
+/** Flattens the nested `and(...)` objects the drizzle operator mocks produce. */
+function flattenConditions(condition: unknown): Array<Record<string, unknown>> {
+  if (!condition || typeof condition !== 'object') return []
+  const node = condition as Record<string, unknown>
+  if (node.type === 'and' && Array.isArray(node.conditions)) {
+    return node.conditions.flatMap(flattenConditions)
+  }
+  return [node]
+}
+
 /** Stand-in for the per-resource config; each test declares only the deltas it exercises. */
 function setConfig(overrides: Record<string, unknown> = {}) {
   resourceConfig.current = {
@@ -230,6 +240,43 @@ describe('createFolder', () => {
     expect(dbChainMockFns.values).toHaveBeenCalledWith(expect.objectContaining({ sortOrder: -3 }))
   })
 
+  it('ignores soft-deleted folders and resources when picking the new sortOrder', async () => {
+    /**
+     * `min - 1` means archived rows would ratchet the floor further negative on every delete and
+     * never recover. Both minima must therefore see only rows a user can still see. Asserted on
+     * the WHERE clauses because the mock returns whatever is queued regardless of the filter, so
+     * an assertion on the resulting sortOrder alone would pass without either clause.
+     */
+    setConfig({
+      resourceType: 'workflow',
+      countKey: 'workflows',
+      sortOrderColumn: 'child.sortOrder',
+    })
+    queueTableRows(schemaMock.folder, [{ minSortOrder: 0 }])
+    queueTableRows(CHILD_TABLE, [{ minSortOrder: 0 }])
+    dbChainMockFns.returning.mockResolvedValueOnce([folderRow({ sortOrder: -1 })])
+
+    await createFolder({ ...baseCreate, resourceType: 'workflow' })
+
+    const [folderWhere, childWhere] = dbChainMockFns.where.mock.calls
+      .slice(0, 2)
+      .map(([where]) => where)
+
+    // Assert on the specific COLUMN, not merely that some isNull exists: for a root folder the
+    // parent condition is itself `isNull(parentId)`, so a presence-only check passes with the
+    // soft-delete filter deleted. That made the first version of this test vacuous.
+    expect(
+      flattenConditions(folderWhere).some(
+        (node) => node.type === 'isNull' && node.column === schemaMock.folder.deletedAt
+      )
+    ).toBe(true)
+    expect(
+      flattenConditions(childWhere).some(
+        (node) => node.type === 'isNull' && node.column === 'child.archivedAt'
+      )
+    ).toBe(true)
+  })
+
   it('starts at zero when the folder is the first thing in its location', async () => {
     queueTableRows(schemaMock.folder, [{ minSortOrder: null }])
     dbChainMockFns.returning.mockResolvedValueOnce([folderRow()])

apps/sim/lib/folders/lifecycle.ts

@@ -128,14 +128,22 @@ export async function nextFolderSortOrder(
     ? eq(folderTable.parentId, parentId)
     : isNull(folderTable.parentId)
 
+  /**
+   * Soft-deleted rows are excluded from both minima. This function returns `min - 1` to put a
+   * new folder at the top, so counting archived rows lets every delete ratchet the floor further
+   * negative and never recover — an archived folder at -400 forces the next new folder to -401
+   * forever. Only rows a user can actually see should influence the ordering. The Files path
+   * (`workspace-file-folder-manager`) has always filtered this way.
+   */
   const folderMinPromise = tx
     .select({ minSortOrder: min(folderTable.sortOrder) })
     .from(folderTable)
     .where(
       and(
         eq(folderTable.workspaceId, workspaceId),
         eq(folderTable.resourceType, resourceType),
-        folderParentCondition
+        folderParentCondition,
+        isNull(folderTable.deletedAt)
       )
     )
 
@@ -147,6 +155,7 @@ export async function nextFolderSortOrder(
           and(
             eq(config.workspaceColumn, workspaceId),
             parentId ? eq(config.folderIdColumn, parentId) : isNull(config.folderIdColumn),
+            isNull(config.deletedColumn),
             config.scope
           )
         )

apps/sim/lib/folders/naming.test.ts

@@ -0,0 +1,133 @@
+/**
+ * @vitest-environment node
+ */
+import { describe, expect, it } from 'vitest'
+import { deduplicateFolderName } from '@/lib/folders/naming'
+
+interface SelectCall {
+  where: unknown
+}
+
+/**
+ * Chainable stand-in for the injectable `tx`. `deduplicateFolderName` awaits after `.where()`,
+ * so the sibling rows are returned there and the condition captured for inspection.
+ */
+function makeTx(siblingNames: string[]) {
+  const selectCalls: SelectCall[] = []
+  const tx = {
+    select: () => ({
+      from: () => ({
+        where: (where: unknown) => {
+          selectCalls.push({ where })
+          return Promise.resolve(siblingNames.map((name) => ({ name })))
+        },
+      }),
+    }),
+  }
+  return { tx: tx as never, selectCalls }
+}
+
+/** Flattens the nested `and(...)` objects the drizzle operator mocks produce. */
+function flattenConditions(condition: unknown): Array<Record<string, unknown>> {
+  if (!condition || typeof condition !== 'object') return []
+  const node = condition as Record<string, unknown>
+  if (node.type === 'and' && Array.isArray(node.conditions)) {
+    return node.conditions.flatMap(flattenConditions)
+  }
+  return [node]
+}
+
+function hasCondition(
+  condition: unknown,
+  predicate: (node: Record<string, unknown>) => boolean
+): boolean {
+  return flattenConditions(condition).some(predicate)
+}
+
+/**
+ * The suffix shape is a cross-surface contract: the client's `nextUntitledFolderName` and
+ * migration 0272's backfill both produce `"<name> (N)"` starting at (1). A server-side drift
+ * either collides on `folder_workspace_resource_parent_name_active_unique` (23505 on a path the
+ * user cannot retry) or renders a folder named differently depending on how it was created.
+ * Nothing asserted this before — every caller mocks this module out.
+ */
+describe('deduplicateFolderName', () => {
+  it('returns the requested name untouched when no sibling holds it', async () => {
+    const { tx } = makeTx(['Other', 'Reports (1)'])
+
+    expect(await deduplicateFolderName(tx, 'ws-1', null, 'Reports', 'workflow')).toBe('Reports')
+  })
+
+  it('starts the suffix at (1), not (2)', async () => {
+    // A loop seeded at 2 — the shape of a bug already fixed twice in this feature — yields
+    // "Reports (2)" here and silently diverges from the client and the migration.
+    const { tx } = makeTx(['Reports'])
+
+    expect(await deduplicateFolderName(tx, 'ws-1', null, 'Reports', 'workflow')).toBe('Reports (1)')
+  })
+
+  it('skips suffixes already taken rather than returning a colliding name', async () => {
+    const { tx } = makeTx(['Reports', 'Reports (1)', 'Reports (2)'])
+
+    expect(await deduplicateFolderName(tx, 'ws-1', null, 'Reports', 'workflow')).toBe('Reports (3)')
+  })
+
+  it('fills a gap in the suffix sequence instead of appending past it', async () => {
+    const { tx } = makeTx(['Reports', 'Reports (2)'])
+
+    expect(await deduplicateFolderName(tx, 'ws-1', null, 'Reports', 'workflow')).toBe('Reports (1)')
+  })
+
+  it('treats a name that only differs by suffix as a distinct base', async () => {
+    // 'Reports (1)' is taken, but the request is for 'Reports (1)' itself — its first free
+    // variant is 'Reports (1) (1)', not 'Reports (2)'.
+    const { tx } = makeTx(['Reports (1)'])
+
+    expect(await deduplicateFolderName(tx, 'ws-1', null, 'Reports (1)', 'workflow')).toBe(
+      'Reports (1) (1)'
+    )
+  })
+
+  /**
+   * The sibling query defines the namespace the suffix is chosen within. Every clause below
+   * mirrors one column of the partial unique index — dropping any of them counts the wrong rows
+   * and either inflates the suffix or picks a name that is already taken.
+   */
+  describe('sibling scoping', () => {
+    it('scopes to workspace, resourceType, root parent, and active rows', async () => {
+      const { tx, selectCalls } = makeTx([])
+
+      await deduplicateFolderName(tx, 'ws-1', null, 'Reports', 'knowledge_base')
+
+      expect(selectCalls).toHaveLength(1)
+      const { where } = selectCalls[0]
+      expect(hasCondition(where, (n) => n.type === 'eq' && n.right === 'ws-1')).toBe(true)
+      // Without this a knowledge-base folder would count table folders as siblings.
+      expect(hasCondition(where, (n) => n.type === 'eq' && n.right === 'knowledge_base')).toBe(true)
+      // Root scope must be IS NULL, not eq(null), which matches nothing in SQL.
+      expect(hasCondition(where, (n) => n.type === 'isNull')).toBe(true)
+    })
+
+    it('scopes to the given parent when nested', async () => {
+      const { tx, selectCalls } = makeTx([])
+
+      await deduplicateFolderName(tx, 'ws-1', 'parent-1', 'Reports', 'workflow')
+
+      expect(
+        hasCondition(selectCalls[0].where, (n) => n.type === 'eq' && n.right === 'parent-1')
+      ).toBe(true)
+    })
+
+    it('excludes soft-deleted siblings so an archived name is reusable', async () => {
+      // The unique index is partial (WHERE deleted_at IS NULL), so counting archived siblings
+      // would suffix a name that is actually free.
+      const { tx, selectCalls } = makeTx([])
+
+      await deduplicateFolderName(tx, 'ws-1', null, 'Reports', 'workflow')
+
+      expect(
+        flattenConditions(selectCalls[0].where).filter((n) => n.type === 'isNull')
+      ).toHaveLength(2)
+    })
+  })
+})