Skip to content

Commit 0f12799

Browse files
authored
fix(folders): stop archived rows skewing sortOrder, hide them from admin, and cover the two untested modules (#6062)
* 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. * refactor(testing): share the drizzle condition-tree helpers 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.
1 parent 4793607 commit 0f12799

9 files changed

Lines changed: 500 additions & 36 deletions

File tree

Lines changed: 80 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,80 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import {
5+
createMockRequest,
6+
dbChainMockFns,
7+
flattenMockConditions,
8+
queueTableRows,
9+
resetDbChainMock,
10+
schemaMock,
11+
} from '@sim/testing'
12+
import { beforeEach, describe, expect, it, vi } from 'vitest'
13+
14+
/**
15+
* The route composes `withAdminAuthParams`, so auth is bypassed by making that wrapper a
16+
* passthrough — the assertions here are about query construction, not the auth gate.
17+
*/
18+
vi.mock('@/app/api/v1/admin/middleware', () => ({
19+
withAdminAuthParams: (handler: unknown) => handler,
20+
}))
21+
22+
import { GET } from '@/app/api/v1/admin/workspaces/[id]/folders/route'
23+
24+
const WORKSPACE_ID = 'ws-1'
25+
const routeContext = { params: Promise.resolve({ id: WORKSPACE_ID }) }
26+
27+
function listRequest() {
28+
return createMockRequest(
29+
'GET',
30+
undefined,
31+
{},
32+
`http://localhost:3000/api/v1/admin/workspaces/${WORKSPACE_ID}/folders?limit=50&offset=0`
33+
)
34+
}
35+
36+
describe('admin workspace folders GET', () => {
37+
beforeEach(() => {
38+
vi.clearAllMocks()
39+
resetDbChainMock()
40+
})
41+
42+
/**
43+
* Both the count and the page must exclude soft-deleted folders. Without the filter an operator
44+
* inspecting a workspace sees folders that live in Recently Deleted and an inflated total, and
45+
* this endpoint disagrees with every user-facing folder list — all of which filter `deletedAt`.
46+
*/
47+
it('excludes soft-deleted folders from both the count and the page', async () => {
48+
queueTableRows(schemaMock.workspace, [{ id: WORKSPACE_ID }])
49+
queueTableRows(schemaMock.folder, [{ total: 0 }])
50+
queueTableRows(schemaMock.folder, [])
51+
52+
await GET(listRequest(), routeContext)
53+
54+
// Calls: [0] workspace lookup, then the count and page share one prebuilt condition.
55+
const folderWheres = dbChainMockFns.where.mock.calls.slice(1).map(([where]) => where)
56+
expect(folderWheres.length).toBeGreaterThanOrEqual(2)
57+
for (const where of folderWheres) {
58+
// Asserted on the COLUMN: `resourceType`/`workspaceId` are eq nodes, so a bare
59+
// "some isNull exists" check could pass on an unrelated clause.
60+
expect(
61+
flattenMockConditions(where).some(
62+
(node) => node.type === 'isNull' && node.column === schemaMock.folder.deletedAt
63+
)
64+
).toBe(true)
65+
}
66+
})
67+
68+
it('still scopes to the workspace and to workflow folders', async () => {
69+
queueTableRows(schemaMock.workspace, [{ id: WORKSPACE_ID }])
70+
queueTableRows(schemaMock.folder, [{ total: 0 }])
71+
queueTableRows(schemaMock.folder, [])
72+
73+
await GET(listRequest(), routeContext)
74+
75+
const where = dbChainMockFns.where.mock.calls[1]?.[0]
76+
const nodes = flattenMockConditions(where)
77+
expect(nodes.some((n) => n.type === 'eq' && n.right === WORKSPACE_ID)).toBe(true)
78+
expect(nodes.some((n) => n.type === 'eq' && n.right === 'workflow')).toBe(true)
79+
})
80+
})

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

Lines changed: 15 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -13,7 +13,7 @@
1313
import { db } from '@sim/db'
1414
import { folder as folderTable, workspace } from '@sim/db/schema'
1515
import { createLogger } from '@sim/logger'
16-
import { and, count, eq } from 'drizzle-orm'
16+
import { and, count, eq, isNull } from 'drizzle-orm'
1717
import { adminV1ListWorkspaceFoldersContract } from '@/lib/api/contracts/v1/admin'
1818
import { parseRequest } from '@/lib/api/server'
1919
import { withRouteHandler } from '@/lib/core/utils/with-route-handler'
@@ -46,19 +46,24 @@ export const GET = withRouteHandler(
4646
return notFoundResponse('Workspace')
4747
}
4848

49+
/**
50+
* Soft-deleted folders are excluded. Without this the count and the page both include rows
51+
* sitting in Recently Deleted, so an operator inspecting a workspace sees phantom folders
52+
* and an inflated total — and the two disagree with every user-facing folder list, all of
53+
* which filter on `deletedAt`.
54+
*/
55+
const activeWorkflowFolders = and(
56+
eq(folderTable.workspaceId, workspaceId),
57+
eq(folderTable.resourceType, 'workflow'),
58+
isNull(folderTable.deletedAt)
59+
)
60+
4961
const [countResult, folders] = await Promise.all([
50-
db
51-
.select({ total: count() })
52-
.from(folderTable)
53-
.where(
54-
and(eq(folderTable.workspaceId, workspaceId), eq(folderTable.resourceType, 'workflow'))
55-
),
62+
db.select({ total: count() }).from(folderTable).where(activeWorkflowFolders),
5663
db
5764
.select()
5865
.from(folderTable)
59-
.where(
60-
and(eq(folderTable.workspaceId, workspaceId), eq(folderTable.resourceType, 'workflow'))
61-
)
66+
.where(activeWorkflowFolders)
6267
.orderBy(folderTable.sortOrder, folderTable.name)
6368
.limit(limit)
6469
.offset(offset),

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

Lines changed: 11 additions & 25 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/**
22
* @vitest-environment node
33
*/
4+
import { flattenMockConditions, hasMockCondition } from '@sim/testing'
45
import { beforeEach, describe, expect, it, vi } from 'vitest'
56
import {
67
archiveFolderCascade,
@@ -81,23 +82,6 @@ function makeConfig(overrides: Partial<FolderResourceConfig> = {}): FolderResour
8182
}
8283
}
8384

84-
/** Flattens the nested `and(...)` objects the drizzle operator mocks produce. */
85-
function flattenConditions(condition: unknown): Array<Record<string, unknown>> {
86-
if (!condition || typeof condition !== 'object') return []
87-
const node = condition as Record<string, unknown>
88-
if (node.type === 'and' && Array.isArray(node.conditions)) {
89-
return node.conditions.flatMap(flattenConditions)
90-
}
91-
return [node]
92-
}
93-
94-
function hasCondition(
95-
condition: unknown,
96-
predicate: (node: Record<string, unknown>) => boolean
97-
): boolean {
98-
return flattenConditions(condition).some(predicate)
99-
}
100-
10185
const TIMESTAMP = new Date('2026-01-01T00:00:00.000Z')
10286
const NOW = new Date('2026-02-02T00:00:00.000Z')
10387

@@ -138,7 +122,7 @@ describe('collectCascadeSubtreeIds', () => {
138122

139123
expect(ids).toEqual(['root', 'child', 'grandchild'])
140124
// Either still active, or carrying this cascade's own stamp — never another snapshot's.
141-
const clause = flattenConditions(selectCalls[0].where).find((node) => node.type === 'or')
125+
const clause = flattenMockConditions(selectCalls[0].where).find((node) => node.type === 'or')
142126
expect(clause).toBeDefined()
143127
const branches = (clause?.conditions ?? []) as Array<Record<string, unknown>>
144128
expect(branches.some((node) => node.type === 'isNull')).toBe(true)
@@ -150,8 +134,10 @@ describe('collectCascadeSubtreeIds', () => {
150134

151135
await collectCascadeSubtreeIds(tx, 'ws-1', 'knowledge_base', 'root', TIMESTAMP)
152136

153-
expect(hasCondition(selectCalls[0].where, (node) => node.right === 'knowledge_base')).toBe(true)
154-
expect(hasCondition(selectCalls[0].where, (node) => node.right === 'ws-1')).toBe(true)
137+
expect(hasMockCondition(selectCalls[0].where, (node) => node.right === 'knowledge_base')).toBe(
138+
true
139+
)
140+
expect(hasMockCondition(selectCalls[0].where, (node) => node.right === 'ws-1')).toBe(true)
155141
})
156142
})
157143

@@ -169,7 +155,7 @@ describe('collectArchivedSubtreeIds', () => {
169155
const ids = await collectArchivedSubtreeIds(tx, 'ws-1', 'table', 'root', TIMESTAMP)
170156

171157
expect(ids).toEqual(['root', 'child'])
172-
expect(hasCondition(selectCalls[0].where, (node) => node.right === TIMESTAMP)).toBe(true)
158+
expect(hasMockCondition(selectCalls[0].where, (node) => node.right === TIMESTAMP)).toBe(true)
173159
})
174160

175161
it('terminates on a parent cycle instead of recursing forever', async () => {
@@ -230,7 +216,7 @@ describe('archiveFolderCascade', () => {
230216
await archiveFolderCascade(tx, makeConfig(), 'ws-1', ['root'], TIMESTAMP)
231217

232218
for (const call of updateCalls) {
233-
expect(hasCondition(call.where, (node) => node.type === 'isNull')).toBe(true)
219+
expect(hasMockCondition(call.where, (node) => node.type === 'isNull')).toBe(true)
234220
}
235221
})
236222

@@ -299,7 +285,7 @@ describe('restoreFolderCascade', () => {
299285
expect(updateCalls[1].set).toEqual({ archivedAt: null, updatedAt: NOW })
300286
expect(updateCalls[2].table).toBe(DEPENDENT_TABLE)
301287
expect(
302-
hasCondition(updateCalls[2].where, (node) => {
288+
hasMockCondition(updateCalls[2].where, (node) => {
303289
return node.type === 'inArray' && Array.isArray(node.values) && node.values.length === 2
304290
})
305291
).toBe(true)
@@ -353,7 +339,7 @@ describe('restoreFolderCascade', () => {
353339
)
354340

355341
for (const call of updateCalls) {
356-
expect(hasCondition(call.where, (node) => node.right === TIMESTAMP)).toBe(true)
342+
expect(hasMockCondition(call.where, (node) => node.right === TIMESTAMP)).toBe(true)
357343
}
358344
})
359345
})
@@ -373,7 +359,7 @@ describe('restoreFolderRows', () => {
373359

374360
expect(folders).toBe(2)
375361
expect(updateCalls).toHaveLength(1)
376-
expect(hasCondition(updateCalls[0].where, (node) => node.right === TIMESTAMP)).toBe(true)
362+
expect(hasMockCondition(updateCalls[0].where, (node) => node.right === TIMESTAMP)).toBe(true)
377363
})
378364
})
379365

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

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ import {
55
auditMock,
66
dbChainMock,
77
dbChainMockFns,
8+
flattenMockConditions,
89
queueTableRows,
910
resetDbChainMock,
1011
schemaMock,
@@ -230,6 +231,43 @@ describe('createFolder', () => {
230231
expect(dbChainMockFns.values).toHaveBeenCalledWith(expect.objectContaining({ sortOrder: -3 }))
231232
})
232233

234+
it('ignores soft-deleted folders and resources when picking the new sortOrder', async () => {
235+
/**
236+
* `min - 1` means archived rows would ratchet the floor further negative on every delete and
237+
* never recover. Both minima must therefore see only rows a user can still see. Asserted on
238+
* the WHERE clauses because the mock returns whatever is queued regardless of the filter, so
239+
* an assertion on the resulting sortOrder alone would pass without either clause.
240+
*/
241+
setConfig({
242+
resourceType: 'workflow',
243+
countKey: 'workflows',
244+
sortOrderColumn: 'child.sortOrder',
245+
})
246+
queueTableRows(schemaMock.folder, [{ minSortOrder: 0 }])
247+
queueTableRows(CHILD_TABLE, [{ minSortOrder: 0 }])
248+
dbChainMockFns.returning.mockResolvedValueOnce([folderRow({ sortOrder: -1 })])
249+
250+
await createFolder({ ...baseCreate, resourceType: 'workflow' })
251+
252+
const [folderWhere, childWhere] = dbChainMockFns.where.mock.calls
253+
.slice(0, 2)
254+
.map(([where]) => where)
255+
256+
// Assert on the specific COLUMN, not merely that some isNull exists: for a root folder the
257+
// parent condition is itself `isNull(parentId)`, so a presence-only check passes with the
258+
// soft-delete filter deleted. That made the first version of this test vacuous.
259+
expect(
260+
flattenMockConditions(folderWhere).some(
261+
(node) => node.type === 'isNull' && node.column === schemaMock.folder.deletedAt
262+
)
263+
).toBe(true)
264+
expect(
265+
flattenMockConditions(childWhere).some(
266+
(node) => node.type === 'isNull' && node.column === 'child.archivedAt'
267+
)
268+
).toBe(true)
269+
})
270+
233271
it('starts at zero when the folder is the first thing in its location', async () => {
234272
queueTableRows(schemaMock.folder, [{ minSortOrder: null }])
235273
dbChainMockFns.returning.mockResolvedValueOnce([folderRow()])

apps/sim/lib/folders/lifecycle.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,14 +128,22 @@ export async function nextFolderSortOrder(
128128
? eq(folderTable.parentId, parentId)
129129
: isNull(folderTable.parentId)
130130

131+
/**
132+
* Soft-deleted rows are excluded from both minima. This function returns `min - 1` to put a
133+
* new folder at the top, so counting archived rows lets every delete ratchet the floor further
134+
* negative and never recover — an archived folder at -400 forces the next new folder to -401
135+
* forever. Only rows a user can actually see should influence the ordering. The Files path
136+
* (`workspace-file-folder-manager`) has always filtered this way.
137+
*/
131138
const folderMinPromise = tx
132139
.select({ minSortOrder: min(folderTable.sortOrder) })
133140
.from(folderTable)
134141
.where(
135142
and(
136143
eq(folderTable.workspaceId, workspaceId),
137144
eq(folderTable.resourceType, resourceType),
138-
folderParentCondition
145+
folderParentCondition,
146+
isNull(folderTable.deletedAt)
139147
)
140148
)
141149

@@ -147,6 +155,7 @@ export async function nextFolderSortOrder(
147155
and(
148156
eq(config.workspaceColumn, workspaceId),
149157
parentId ? eq(config.folderIdColumn, parentId) : isNull(config.folderIdColumn),
158+
isNull(config.deletedColumn),
150159
config.scope
151160
)
152161
)

0 commit comments

Comments
 (0)