From a910b3665648b49e4197b7cd809ef9d0d0c7ea17 Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 29 Jul 2026 14:41:19 -0700 Subject: [PATCH 1/2] fix(folders): stop archived rows skewing sortOrder, hide them from admin, and cover the two untested modules MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- .../workspaces/[id]/folders/route.test.ts | 89 ++++++++ .../v1/admin/workspaces/[id]/folders/route.ts | 25 ++- apps/sim/lib/folders/lifecycle.test.ts | 47 ++++ apps/sim/lib/folders/lifecycle.ts | 11 +- apps/sim/lib/folders/naming.test.ts | 133 ++++++++++++ apps/sim/lib/folders/queries.test.ts | 203 ++++++++++++++++++ 6 files changed, 497 insertions(+), 11 deletions(-) create mode 100644 apps/sim/app/api/v1/admin/workspaces/[id]/folders/route.test.ts create mode 100644 apps/sim/lib/folders/naming.test.ts create mode 100644 apps/sim/lib/folders/queries.test.ts diff --git a/apps/sim/app/api/v1/admin/workspaces/[id]/folders/route.test.ts b/apps/sim/app/api/v1/admin/workspaces/[id]/folders/route.test.ts new file mode 100644 index 00000000000..457db07c4a2 --- /dev/null +++ b/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> { + if (!condition || typeof condition !== 'object') return [] + const node = condition as Record + 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) + }) +}) diff --git a/apps/sim/app/api/v1/admin/workspaces/[id]/folders/route.ts b/apps/sim/app/api/v1/admin/workspaces/[id]/folders/route.ts index f2786433ce3..4bbf5171776 100644 --- a/apps/sim/app/api/v1/admin/workspaces/[id]/folders/route.ts +++ b/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), diff --git a/apps/sim/lib/folders/lifecycle.test.ts b/apps/sim/lib/folders/lifecycle.test.ts index 94074a09095..571a833d351 100644 --- a/apps/sim/lib/folders/lifecycle.test.ts +++ b/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> { + if (!condition || typeof condition !== 'object') return [] + const node = condition as Record + 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 = {}) { 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()]) diff --git a/apps/sim/lib/folders/lifecycle.ts b/apps/sim/lib/folders/lifecycle.ts index 7b02f7f4f1b..cc592df395c 100644 --- a/apps/sim/lib/folders/lifecycle.ts +++ b/apps/sim/lib/folders/lifecycle.ts @@ -128,6 +128,13 @@ 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) @@ -135,7 +142,8 @@ export async function nextFolderSortOrder( 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 ) ) diff --git a/apps/sim/lib/folders/naming.test.ts b/apps/sim/lib/folders/naming.test.ts new file mode 100644 index 00000000000..bd121398683 --- /dev/null +++ b/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> { + if (!condition || typeof condition !== 'object') return [] + const node = condition as Record + if (node.type === 'and' && Array.isArray(node.conditions)) { + return node.conditions.flatMap(flattenConditions) + } + return [node] +} + +function hasCondition( + condition: unknown, + predicate: (node: Record) => 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 `" (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) + }) + }) +}) diff --git a/apps/sim/lib/folders/queries.test.ts b/apps/sim/lib/folders/queries.test.ts new file mode 100644 index 00000000000..6b84c09f897 --- /dev/null +++ b/apps/sim/lib/folders/queries.test.ts @@ -0,0 +1,203 @@ +/** + * @vitest-environment node + */ +import { dbChainMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing' +import { beforeEach, describe, expect, it, vi } from 'vitest' +import { + findActiveFolder, + listFoldersForWorkspace, + resolveRestoredFolderId, + toFolderApi, + wouldCreateFolderCycle, +} from '@/lib/folders/queries' + +/** Flattens the nested `and(...)` objects the drizzle operator mocks produce. */ +function flattenConditions(condition: unknown): Array> { + if (!condition || typeof condition !== 'object') return [] + const node = condition as Record + if (node.type === 'and' && Array.isArray(node.conditions)) { + return node.conditions.flatMap(flattenConditions) + } + return [node] +} + +function hasCondition( + condition: unknown, + predicate: (node: Record) => boolean +): boolean { + return flattenConditions(condition).some(predicate) +} + +/** The condition passed to the Nth `.where()` of this test. */ +function whereAt(index: number): unknown { + return dbChainMockFns.where.mock.calls[index]?.[0] +} + +const ROW = { + id: 'f-1', + resourceType: 'workflow' as const, + name: 'Reports', + userId: 'u-1', + workspaceId: 'ws-1', + parentId: null, + locked: false, + sortOrder: 0, + createdAt: new Date('2026-01-01T00:00:00.000Z'), + updatedAt: new Date('2026-01-02T00:00:00.000Z'), + deletedAt: null, +} + +describe('folder queries', () => { + beforeEach(() => { + vi.clearAllMocks() + resetDbChainMock() + }) + + /** + * These are the id-keyed lookups. Every other query in the feature is already scoped by + * workspace + resourceType through a list filter, but these accept a caller-supplied id — so + * they are the one place a missing `resource_type` clause silently crosses resource trees, + * filing a knowledge base under a table folder where no page will ever render it. Nothing + * asserted this before: every caller mocks this module out, so deleting the clause left the + * whole suite green. + */ + describe('findActiveFolder', () => { + it('scopes by id, workspace, resourceType, and active state', async () => { + queueTableRows(schemaMock.folder, [ROW]) + + await findActiveFolder('f-1', 'ws-1', 'knowledge_base') + + const where = whereAt(0) + expect(hasCondition(where, (n) => n.type === 'eq' && n.right === 'f-1')).toBe(true) + expect(hasCondition(where, (n) => n.type === 'eq' && n.right === 'ws-1')).toBe(true) + expect(hasCondition(where, (n) => n.type === 'eq' && n.right === 'knowledge_base')).toBe(true) + // Archived folders are not valid destinations — a row filed under one is unreachable. + expect(hasCondition(where, (n) => n.type === 'isNull')).toBe(true) + }) + + it('returns null when no row matches', async () => { + queueTableRows(schemaMock.folder, []) + + expect(await findActiveFolder('f-1', 'ws-1', 'workflow')).toBeNull() + }) + }) + + describe('wouldCreateFolderCycle', () => { + it('detects the immediate self-parent case without querying', async () => { + expect(await wouldCreateFolderCycle('f-1', 'f-1', 'workflow')).toBe(true) + expect(dbChainMockFns.where).not.toHaveBeenCalled() + }) + + it('scopes every step of the upward walk to resourceType', async () => { + // Without the clause the walk can leave this resource's tree via a caller-supplied + // parent id and report "no cycle" from another tree's ancestry. + queueTableRows(schemaMock.folder, [{ parentId: 'grandparent' }]) + queueTableRows(schemaMock.folder, [{ parentId: null }]) + + await wouldCreateFolderCycle('f-1', 'parent-1', 'table') + + expect(dbChainMockFns.where.mock.calls.length).toBeGreaterThanOrEqual(2) + for (const [where] of dbChainMockFns.where.mock.calls) { + expect(hasCondition(where, (n) => n.type === 'eq' && n.right === 'table')).toBe(true) + } + }) + + it('reports a cycle when the walk reaches the folder being reparented', async () => { + queueTableRows(schemaMock.folder, [{ parentId: 'f-1' }]) + + expect(await wouldCreateFolderCycle('f-1', 'parent-1', 'workflow')).toBe(true) + }) + + it('terminates on a pre-existing cycle above the folder', async () => { + // `visited` is what stops this looping forever; optimistic client reparents can write one. + queueTableRows(schemaMock.folder, [{ parentId: 'b' }]) + queueTableRows(schemaMock.folder, [{ parentId: 'a' }]) + + expect(await wouldCreateFolderCycle('f-1', 'a', 'workflow')).toBe(true) + }) + + it('returns false when the walk reaches the root', async () => { + queueTableRows(schemaMock.folder, [{ parentId: null }]) + + expect(await wouldCreateFolderCycle('f-1', 'parent-1', 'workflow')).toBe(false) + }) + }) + + /** + * The `restoringFolderIds` short-circuit is load-bearing for cascade ordering: `restoreFolder` + * runs its `restoreChildren` hook BEFORE un-archiving the folder rows, so a plain "is my folder + * active?" check sees them still archived and dumps the entire subtree at the workspace root. + */ + describe('resolveRestoredFolderId', () => { + it('keeps the folder without querying when it is in the restoring set', async () => { + const result = await resolveRestoredFolderId('f-1', 'ws-1', 'workflow', new Set(['f-1'])) + + expect(result).toBe('f-1') + expect(dbChainMockFns.where).not.toHaveBeenCalled() + }) + + it('re-roots to null when the original folder is not active', async () => { + queueTableRows(schemaMock.folder, []) + + expect(await resolveRestoredFolderId('f-1', 'ws-1', 'workflow')).toBeNull() + }) + + it('keeps the folder when it is still active outside a cascade', async () => { + queueTableRows(schemaMock.folder, [ROW]) + + expect(await resolveRestoredFolderId('f-1', 'ws-1', 'workflow')).toBe('f-1') + }) + + it('re-roots when the resource has no folder or no workspace', async () => { + expect(await resolveRestoredFolderId(null, 'ws-1', 'workflow')).toBeNull() + expect(await resolveRestoredFolderId('f-1', null, 'workflow')).toBeNull() + expect(dbChainMockFns.where).not.toHaveBeenCalled() + }) + }) + + describe('listFoldersForWorkspace', () => { + it('scopes to workspace and resourceType, and to active rows by default', async () => { + queueTableRows(schemaMock.folder, [ROW]) + + await listFoldersForWorkspace('ws-1', 'active', 'table') + + const where = whereAt(0) + expect(hasCondition(where, (n) => n.type === 'eq' && n.right === 'ws-1')).toBe(true) + expect(hasCondition(where, (n) => n.type === 'eq' && n.right === 'table')).toBe(true) + expect(hasCondition(where, (n) => n.type === 'isNull')).toBe(true) + expect(hasCondition(where, (n) => n.type === 'isNotNull')).toBe(false) + }) + + it('inverts the soft-delete filter for the archived scope', async () => { + queueTableRows(schemaMock.folder, []) + + await listFoldersForWorkspace('ws-1', 'archived', 'workflow') + + const where = whereAt(0) + expect(hasCondition(where, (n) => n.type === 'isNotNull')).toBe(true) + expect(hasCondition(where, (n) => n.type === 'isNull')).toBe(false) + }) + }) + + /** + * `requestJson` validates responses against the contract, so a route returning a raw row fails + * client-side parse AFTER its write has already committed. This normalizer is the single point + * that keeps every folder route emitting the same wire shape. + */ + describe('toFolderApi', () => { + it('serializes timestamps to ISO strings and preserves a null deletedAt', () => { + expect(toFolderApi(ROW)).toMatchObject({ + id: 'f-1', + createdAt: '2026-01-01T00:00:00.000Z', + updatedAt: '2026-01-02T00:00:00.000Z', + deletedAt: null, + }) + }) + + it('serializes a present deletedAt rather than dropping it', () => { + const deleted = { ...ROW, deletedAt: new Date('2026-03-03T00:00:00.000Z') } + + expect(toFolderApi(deleted).deletedAt).toBe('2026-03-03T00:00:00.000Z') + }) + }) +}) From c029210882a50517cf6e34c04f3394955f7de32d Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 29 Jul 2026 14:48:17 -0700 Subject: [PATCH 2/2] refactor(testing): share the drizzle condition-tree helpers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Asserting on WHERE clauses is the only way to pin a filter the row-queue mocks cannot enforce — a mock returns whatever was queued regardless of the predicate — so this pattern spreads to every test that guards a query's scoping. It had reached five local copies of the same flatten/has pair, four of them added by the tests in this branch. Moved to @sim/testing beside createMockSqlOperators, whose output shape they parse, so the helper and the node types it depends on live together. --- .../workspaces/[id]/folders/route.test.ts | 15 ++---- apps/sim/lib/folders/cascade.test.ts | 36 +++++--------- apps/sim/lib/folders/lifecycle.test.ts | 15 ++---- apps/sim/lib/folders/naming.test.ts | 30 +++--------- apps/sim/lib/folders/queries.test.ts | 49 ++++++++----------- packages/testing/src/mocks/database.mock.ts | 30 ++++++++++++ packages/testing/src/mocks/index.ts | 3 ++ 7 files changed, 78 insertions(+), 100 deletions(-) diff --git a/apps/sim/app/api/v1/admin/workspaces/[id]/folders/route.test.ts b/apps/sim/app/api/v1/admin/workspaces/[id]/folders/route.test.ts index 457db07c4a2..c66b8492307 100644 --- a/apps/sim/app/api/v1/admin/workspaces/[id]/folders/route.test.ts +++ b/apps/sim/app/api/v1/admin/workspaces/[id]/folders/route.test.ts @@ -4,6 +4,7 @@ import { createMockRequest, dbChainMockFns, + flattenMockConditions, queueTableRows, resetDbChainMock, schemaMock, @@ -32,16 +33,6 @@ function listRequest() { ) } -/** Flattens the nested `and(...)` objects the drizzle operator mocks produce. */ -function flattenConditions(condition: unknown): Array> { - if (!condition || typeof condition !== 'object') return [] - const node = condition as Record - if (node.type === 'and' && Array.isArray(node.conditions)) { - return node.conditions.flatMap(flattenConditions) - } - return [node] -} - describe('admin workspace folders GET', () => { beforeEach(() => { vi.clearAllMocks() @@ -67,7 +58,7 @@ describe('admin workspace folders GET', () => { // 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( + flattenMockConditions(where).some( (node) => node.type === 'isNull' && node.column === schemaMock.folder.deletedAt ) ).toBe(true) @@ -82,7 +73,7 @@ describe('admin workspace folders GET', () => { await GET(listRequest(), routeContext) const where = dbChainMockFns.where.mock.calls[1]?.[0] - const nodes = flattenConditions(where) + const nodes = flattenMockConditions(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) }) diff --git a/apps/sim/lib/folders/cascade.test.ts b/apps/sim/lib/folders/cascade.test.ts index a99f683a42e..f354a9ded09 100644 --- a/apps/sim/lib/folders/cascade.test.ts +++ b/apps/sim/lib/folders/cascade.test.ts @@ -1,6 +1,7 @@ /** * @vitest-environment node */ +import { flattenMockConditions, hasMockCondition } from '@sim/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' import { archiveFolderCascade, @@ -81,23 +82,6 @@ function makeConfig(overrides: Partial = {}): FolderResour } } -/** Flattens the nested `and(...)` objects the drizzle operator mocks produce. */ -function flattenConditions(condition: unknown): Array> { - if (!condition || typeof condition !== 'object') return [] - const node = condition as Record - if (node.type === 'and' && Array.isArray(node.conditions)) { - return node.conditions.flatMap(flattenConditions) - } - return [node] -} - -function hasCondition( - condition: unknown, - predicate: (node: Record) => boolean -): boolean { - return flattenConditions(condition).some(predicate) -} - const TIMESTAMP = new Date('2026-01-01T00:00:00.000Z') const NOW = new Date('2026-02-02T00:00:00.000Z') @@ -138,7 +122,7 @@ describe('collectCascadeSubtreeIds', () => { expect(ids).toEqual(['root', 'child', 'grandchild']) // Either still active, or carrying this cascade's own stamp — never another snapshot's. - const clause = flattenConditions(selectCalls[0].where).find((node) => node.type === 'or') + const clause = flattenMockConditions(selectCalls[0].where).find((node) => node.type === 'or') expect(clause).toBeDefined() const branches = (clause?.conditions ?? []) as Array> expect(branches.some((node) => node.type === 'isNull')).toBe(true) @@ -150,8 +134,10 @@ describe('collectCascadeSubtreeIds', () => { await collectCascadeSubtreeIds(tx, 'ws-1', 'knowledge_base', 'root', TIMESTAMP) - expect(hasCondition(selectCalls[0].where, (node) => node.right === 'knowledge_base')).toBe(true) - expect(hasCondition(selectCalls[0].where, (node) => node.right === 'ws-1')).toBe(true) + expect(hasMockCondition(selectCalls[0].where, (node) => node.right === 'knowledge_base')).toBe( + true + ) + expect(hasMockCondition(selectCalls[0].where, (node) => node.right === 'ws-1')).toBe(true) }) }) @@ -169,7 +155,7 @@ describe('collectArchivedSubtreeIds', () => { const ids = await collectArchivedSubtreeIds(tx, 'ws-1', 'table', 'root', TIMESTAMP) expect(ids).toEqual(['root', 'child']) - expect(hasCondition(selectCalls[0].where, (node) => node.right === TIMESTAMP)).toBe(true) + expect(hasMockCondition(selectCalls[0].where, (node) => node.right === TIMESTAMP)).toBe(true) }) it('terminates on a parent cycle instead of recursing forever', async () => { @@ -230,7 +216,7 @@ describe('archiveFolderCascade', () => { await archiveFolderCascade(tx, makeConfig(), 'ws-1', ['root'], TIMESTAMP) for (const call of updateCalls) { - expect(hasCondition(call.where, (node) => node.type === 'isNull')).toBe(true) + expect(hasMockCondition(call.where, (node) => node.type === 'isNull')).toBe(true) } }) @@ -299,7 +285,7 @@ describe('restoreFolderCascade', () => { expect(updateCalls[1].set).toEqual({ archivedAt: null, updatedAt: NOW }) expect(updateCalls[2].table).toBe(DEPENDENT_TABLE) expect( - hasCondition(updateCalls[2].where, (node) => { + hasMockCondition(updateCalls[2].where, (node) => { return node.type === 'inArray' && Array.isArray(node.values) && node.values.length === 2 }) ).toBe(true) @@ -353,7 +339,7 @@ describe('restoreFolderCascade', () => { ) for (const call of updateCalls) { - expect(hasCondition(call.where, (node) => node.right === TIMESTAMP)).toBe(true) + expect(hasMockCondition(call.where, (node) => node.right === TIMESTAMP)).toBe(true) } }) }) @@ -373,7 +359,7 @@ describe('restoreFolderRows', () => { expect(folders).toBe(2) expect(updateCalls).toHaveLength(1) - expect(hasCondition(updateCalls[0].where, (node) => node.right === TIMESTAMP)).toBe(true) + expect(hasMockCondition(updateCalls[0].where, (node) => node.right === TIMESTAMP)).toBe(true) }) }) diff --git a/apps/sim/lib/folders/lifecycle.test.ts b/apps/sim/lib/folders/lifecycle.test.ts index 571a833d351..9694728bd12 100644 --- a/apps/sim/lib/folders/lifecycle.test.ts +++ b/apps/sim/lib/folders/lifecycle.test.ts @@ -5,6 +5,7 @@ import { auditMock, dbChainMock, dbChainMockFns, + flattenMockConditions, queueTableRows, resetDbChainMock, schemaMock, @@ -67,16 +68,6 @@ 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> { - if (!condition || typeof condition !== 'object') return [] - const node = condition as Record - 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 = {}) { resourceConfig.current = { @@ -266,12 +257,12 @@ describe('createFolder', () => { // 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( + flattenMockConditions(folderWhere).some( (node) => node.type === 'isNull' && node.column === schemaMock.folder.deletedAt ) ).toBe(true) expect( - flattenConditions(childWhere).some( + flattenMockConditions(childWhere).some( (node) => node.type === 'isNull' && node.column === 'child.archivedAt' ) ).toBe(true) diff --git a/apps/sim/lib/folders/naming.test.ts b/apps/sim/lib/folders/naming.test.ts index bd121398683..34b37b64929 100644 --- a/apps/sim/lib/folders/naming.test.ts +++ b/apps/sim/lib/folders/naming.test.ts @@ -1,6 +1,7 @@ /** * @vitest-environment node */ +import { flattenMockConditions, hasMockCondition } from '@sim/testing' import { describe, expect, it } from 'vitest' import { deduplicateFolderName } from '@/lib/folders/naming' @@ -27,23 +28,6 @@ function makeTx(siblingNames: string[]) { return { tx: tx as never, selectCalls } } -/** Flattens the nested `and(...)` objects the drizzle operator mocks produce. */ -function flattenConditions(condition: unknown): Array> { - if (!condition || typeof condition !== 'object') return [] - const node = condition as Record - if (node.type === 'and' && Array.isArray(node.conditions)) { - return node.conditions.flatMap(flattenConditions) - } - return [node] -} - -function hasCondition( - condition: unknown, - predicate: (node: Record) => 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 `" (N)"` starting at (1). A server-side drift @@ -101,11 +85,13 @@ describe('deduplicateFolderName', () => { expect(selectCalls).toHaveLength(1) const { where } = selectCalls[0] - expect(hasCondition(where, (n) => n.type === 'eq' && n.right === 'ws-1')).toBe(true) + expect(hasMockCondition(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) + expect(hasMockCondition(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) + expect(hasMockCondition(where, (n) => n.type === 'isNull')).toBe(true) }) it('scopes to the given parent when nested', async () => { @@ -114,7 +100,7 @@ describe('deduplicateFolderName', () => { await deduplicateFolderName(tx, 'ws-1', 'parent-1', 'Reports', 'workflow') expect( - hasCondition(selectCalls[0].where, (n) => n.type === 'eq' && n.right === 'parent-1') + hasMockCondition(selectCalls[0].where, (n) => n.type === 'eq' && n.right === 'parent-1') ).toBe(true) }) @@ -126,7 +112,7 @@ describe('deduplicateFolderName', () => { await deduplicateFolderName(tx, 'ws-1', null, 'Reports', 'workflow') expect( - flattenConditions(selectCalls[0].where).filter((n) => n.type === 'isNull') + flattenMockConditions(selectCalls[0].where).filter((n) => n.type === 'isNull') ).toHaveLength(2) }) }) diff --git a/apps/sim/lib/folders/queries.test.ts b/apps/sim/lib/folders/queries.test.ts index 6b84c09f897..b7678cc1b58 100644 --- a/apps/sim/lib/folders/queries.test.ts +++ b/apps/sim/lib/folders/queries.test.ts @@ -1,7 +1,13 @@ /** * @vitest-environment node */ -import { dbChainMockFns, queueTableRows, resetDbChainMock, schemaMock } from '@sim/testing' +import { + dbChainMockFns, + hasMockCondition, + queueTableRows, + resetDbChainMock, + schemaMock, +} from '@sim/testing' import { beforeEach, describe, expect, it, vi } from 'vitest' import { findActiveFolder, @@ -11,23 +17,6 @@ import { wouldCreateFolderCycle, } from '@/lib/folders/queries' -/** Flattens the nested `and(...)` objects the drizzle operator mocks produce. */ -function flattenConditions(condition: unknown): Array> { - if (!condition || typeof condition !== 'object') return [] - const node = condition as Record - if (node.type === 'and' && Array.isArray(node.conditions)) { - return node.conditions.flatMap(flattenConditions) - } - return [node] -} - -function hasCondition( - condition: unknown, - predicate: (node: Record) => boolean -): boolean { - return flattenConditions(condition).some(predicate) -} - /** The condition passed to the Nth `.where()` of this test. */ function whereAt(index: number): unknown { return dbChainMockFns.where.mock.calls[index]?.[0] @@ -68,11 +57,13 @@ describe('folder queries', () => { await findActiveFolder('f-1', 'ws-1', 'knowledge_base') const where = whereAt(0) - expect(hasCondition(where, (n) => n.type === 'eq' && n.right === 'f-1')).toBe(true) - expect(hasCondition(where, (n) => n.type === 'eq' && n.right === 'ws-1')).toBe(true) - expect(hasCondition(where, (n) => n.type === 'eq' && n.right === 'knowledge_base')).toBe(true) + expect(hasMockCondition(where, (n) => n.type === 'eq' && n.right === 'f-1')).toBe(true) + expect(hasMockCondition(where, (n) => n.type === 'eq' && n.right === 'ws-1')).toBe(true) + expect(hasMockCondition(where, (n) => n.type === 'eq' && n.right === 'knowledge_base')).toBe( + true + ) // Archived folders are not valid destinations — a row filed under one is unreachable. - expect(hasCondition(where, (n) => n.type === 'isNull')).toBe(true) + expect(hasMockCondition(where, (n) => n.type === 'isNull')).toBe(true) }) it('returns null when no row matches', async () => { @@ -98,7 +89,7 @@ describe('folder queries', () => { expect(dbChainMockFns.where.mock.calls.length).toBeGreaterThanOrEqual(2) for (const [where] of dbChainMockFns.where.mock.calls) { - expect(hasCondition(where, (n) => n.type === 'eq' && n.right === 'table')).toBe(true) + expect(hasMockCondition(where, (n) => n.type === 'eq' && n.right === 'table')).toBe(true) } }) @@ -162,10 +153,10 @@ describe('folder queries', () => { await listFoldersForWorkspace('ws-1', 'active', 'table') const where = whereAt(0) - expect(hasCondition(where, (n) => n.type === 'eq' && n.right === 'ws-1')).toBe(true) - expect(hasCondition(where, (n) => n.type === 'eq' && n.right === 'table')).toBe(true) - expect(hasCondition(where, (n) => n.type === 'isNull')).toBe(true) - expect(hasCondition(where, (n) => n.type === 'isNotNull')).toBe(false) + expect(hasMockCondition(where, (n) => n.type === 'eq' && n.right === 'ws-1')).toBe(true) + expect(hasMockCondition(where, (n) => n.type === 'eq' && n.right === 'table')).toBe(true) + expect(hasMockCondition(where, (n) => n.type === 'isNull')).toBe(true) + expect(hasMockCondition(where, (n) => n.type === 'isNotNull')).toBe(false) }) it('inverts the soft-delete filter for the archived scope', async () => { @@ -174,8 +165,8 @@ describe('folder queries', () => { await listFoldersForWorkspace('ws-1', 'archived', 'workflow') const where = whereAt(0) - expect(hasCondition(where, (n) => n.type === 'isNotNull')).toBe(true) - expect(hasCondition(where, (n) => n.type === 'isNull')).toBe(false) + expect(hasMockCondition(where, (n) => n.type === 'isNotNull')).toBe(true) + expect(hasMockCondition(where, (n) => n.type === 'isNull')).toBe(false) }) }) diff --git a/packages/testing/src/mocks/database.mock.ts b/packages/testing/src/mocks/database.mock.ts index e8a79a6bd0d..0ca0af53921 100644 --- a/packages/testing/src/mocks/database.mock.ts +++ b/packages/testing/src/mocks/database.mock.ts @@ -421,3 +421,33 @@ export const drizzleOrmMock = { getTableColumns: vi.fn((table: Record) => ({ ...table })), ...createMockSqlOperators(), } + +/** + * Condition nodes produced by `createMockSqlOperators` — `{ type: 'eq', left, right }`, + * `{ type: 'isNull', column }`, and so on. + */ +export type MockCondition = Record + +/** + * Flattens the nested `and(...)` trees `createMockSqlOperators` builds into a flat node list. + * + * Tests assert on WHERE clauses to pin filters the row-queue mocks cannot enforce — a mock + * returns whatever was queued regardless of the predicate, so "the query filters on X" is only + * testable by inspecting the condition tree. `and()` nests arbitrarily, hence the flatten. + */ +export function flattenMockConditions(condition: unknown): MockCondition[] { + if (!condition || typeof condition !== 'object') return [] + const node = condition as MockCondition + if (node.type === 'and' && Array.isArray(node.conditions)) { + return node.conditions.flatMap(flattenMockConditions) + } + return [node] +} + +/** True when any node in `condition` satisfies `predicate`. */ +export function hasMockCondition( + condition: unknown, + predicate: (node: MockCondition) => boolean +): boolean { + return flattenMockConditions(condition).some(predicate) +} diff --git a/packages/testing/src/mocks/index.ts b/packages/testing/src/mocks/index.ts index 92b192fc9fc..40de62426f4 100644 --- a/packages/testing/src/mocks/index.ts +++ b/packages/testing/src/mocks/index.ts @@ -45,6 +45,9 @@ export { dbChainMock, dbChainMockFns, drizzleOrmMock, + flattenMockConditions, + hasMockCondition, + type MockCondition, queueTableRows, resetDbChainMock, } from './database.mock'