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 c66b8492307..e9b6f21d202 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 @@ -39,11 +39,6 @@ describe('admin workspace folders GET', () => { 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 }]) @@ -55,8 +50,8 @@ describe('admin workspace folders GET', () => { 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. + // Pinned to the column so the assertion stays meaningful if another nullable filter + // (e.g. a parent scope) is ever added to this condition. expect( flattenMockConditions(where).some( (node) => node.type === 'isNull' && node.column === schemaMock.folder.deletedAt 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 4bbf5171776..5192325ec8e 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 @@ -47,10 +47,9 @@ export const GET = withRouteHandler( } /** - * 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`. + * Without the `deletedAt` filter the count and the page both include rows sitting in + * Recently Deleted, so an operator sees phantom folders and an inflated total, and this + * endpoint disagrees with every user-facing folder list. */ const activeWorkflowFolders = and( eq(folderTable.workspaceId, workspaceId), diff --git a/apps/sim/lib/folders/lifecycle.test.ts b/apps/sim/lib/folders/lifecycle.test.ts index 9694728bd12..8da406145db 100644 --- a/apps/sim/lib/folders/lifecycle.test.ts +++ b/apps/sim/lib/folders/lifecycle.test.ts @@ -231,13 +231,9 @@ describe('createFolder', () => { expect(dbChainMockFns.values).toHaveBeenCalledWith(expect.objectContaining({ sortOrder: -3 })) }) + // Asserted on the WHERE clauses, not the resulting sortOrder: the mock returns whatever was + // queued regardless of the filter, so a sortOrder assertion passes without either clause. 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', @@ -254,8 +250,8 @@ describe('createFolder', () => { .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. + // parent condition is itself `isNull(parentId)`, so a presence-only check stays green with + // the soft-delete filter deleted. expect( flattenMockConditions(folderWhere).some( (node) => node.type === 'isNull' && node.column === schemaMock.folder.deletedAt diff --git a/apps/sim/lib/folders/lifecycle.ts b/apps/sim/lib/folders/lifecycle.ts index cc592df395c..af8e002be0f 100644 --- a/apps/sim/lib/folders/lifecycle.ts +++ b/apps/sim/lib/folders/lifecycle.ts @@ -129,11 +129,9 @@ export async function nextFolderSortOrder( : 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. + * Both minima exclude soft-deleted rows. This 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 pins the next new folder at -401 forever. */ const folderMinPromise = tx .select({ minSortOrder: min(folderTable.sortOrder) }) diff --git a/apps/sim/lib/folders/naming.test.ts b/apps/sim/lib/folders/naming.test.ts index 34b37b64929..5952775a4c1 100644 --- a/apps/sim/lib/folders/naming.test.ts +++ b/apps/sim/lib/folders/naming.test.ts @@ -29,11 +29,8 @@ function makeTx(siblingNames: string[]) { } /** - * 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. + * Pins the `" (N)"` shape the client and migration 0272 also produce (see `naming.ts`). + * Every caller mocks this module out, so nothing else in the suite covers it. */ describe('deduplicateFolderName', () => { it('returns the requested name untouched when no sibling holds it', async () => { @@ -43,8 +40,6 @@ describe('deduplicateFolderName', () => { }) 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)') @@ -73,9 +68,9 @@ describe('deduplicateFolderName', () => { }) /** - * 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. + * The sibling query defines the namespace the suffix is chosen within, and must match the + * scope of the partial unique index — workspace, resourceType, parent, active-only. Drop any + * clause and it counts the wrong rows, inflating the suffix or picking a taken name. */ describe('sibling scoping', () => { it('scopes to workspace, resourceType, root parent, and active rows', async () => { @@ -111,6 +106,7 @@ describe('deduplicateFolderName', () => { await deduplicateFolderName(tx, 'ws-1', null, 'Reports', 'workflow') + // Two isNull nodes: the root parent scope and the soft-delete filter. expect( 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 b7678cc1b58..d6210ed53b5 100644 --- a/apps/sim/lib/folders/queries.test.ts +++ b/apps/sim/lib/folders/queries.test.ts @@ -43,12 +43,9 @@ describe('folder queries', () => { }) /** - * 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. + * The only lookup keyed on a caller-supplied folder id, so the `resourceType` clause is the + * sole guard against crossing resource trees (rationale in `findActiveFolder`). Every caller + * mocks this module out, so deleting that clause used to leave the whole suite green. */ describe('findActiveFolder', () => { it('scopes by id, workspace, resourceType, and active state', async () => { @@ -63,7 +60,13 @@ describe('folder queries', () => { true ) // Archived folders are not valid destinations — a row filed under one is unreachable. - expect(hasMockCondition(where, (n) => n.type === 'isNull')).toBe(true) + // Pinned to the column so the check cannot be satisfied by some other nullable filter. + expect( + hasMockCondition( + where, + (n) => n.type === 'isNull' && n.column === schemaMock.folder.deletedAt + ) + ).toBe(true) }) it('returns null when no row matches', async () => { @@ -100,7 +103,8 @@ describe('folder queries', () => { }) it('terminates on a pre-existing cycle above the folder', async () => { - // `visited` is what stops this looping forever; optimistic client reparents can write one. + // `visited` is what stops this looping forever; concurrent reparents can each pass the + // check and land a cycle. queueTableRows(schemaMock.folder, [{ parentId: 'b' }]) queueTableRows(schemaMock.folder, [{ parentId: 'a' }]) @@ -115,9 +119,9 @@ describe('folder queries', () => { }) /** - * 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. + * The `restoringFolderIds` short-circuit is load-bearing for cascade ordering: the + * `config.restoreChildren` hook path runs before the un-archive transaction, so without the + * set a plain "is my folder active?" check dumps the subtree at the workspace root. */ describe('resolveRestoredFolderId', () => { it('keeps the folder without querying when it is in the restoring set', async () => { @@ -155,7 +159,12 @@ describe('folder queries', () => { const where = whereAt(0) 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 === 'isNull' && n.column === schemaMock.folder.deletedAt + ) + ).toBe(true) expect(hasMockCondition(where, (n) => n.type === 'isNotNull')).toBe(false) }) @@ -165,16 +174,16 @@ describe('folder queries', () => { await listFoldersForWorkspace('ws-1', 'archived', 'workflow') const where = whereAt(0) - expect(hasMockCondition(where, (n) => n.type === 'isNotNull')).toBe(true) + expect( + hasMockCondition( + where, + (n) => n.type === 'isNotNull' && n.column === schemaMock.folder.deletedAt + ) + ).toBe(true) expect(hasMockCondition(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({