Skip to content

Commit 5bcd02f

Browse files
waleedlatif1waleedlatif1
committed
fix(folders): address round-1 Greptile/Cursor review feedback
- Reorder route now rejects a batch spanning more than one resourceType instead of silently reordering only the first-inferred type (Greptile P1, Cursor Bugbot) - Duplicate route's ancestor-walk now re-checks deletedAt on every hop, not just the immediate parent -- a regression from an earlier simplify pass that only checked the first iteration (Cursor Bugbot). Exported assertTargetParentFolderMutable and added focused regression tests, since this route had zero prior coverage - GET /api/pinned-items now filters out pins whose underlying resource has since been deleted/archived, batched one existence query per resourceType present rather than per row (Greptile P2) Three other Greptile findings investigated and confirmed false positives (documented in review replies): file-folder creation writing to "legacy storage" (same table via a local alias), folder metadata silently dropped (the only affected fields, color/isExpanded, were already removed in the prior commit), and archived-workspace mutation (already blocked upstream by the shared getUserEntityPermissions check on every folder route). One more (deleted intermediates hiding descendants) is structurally unreachable given the existing parent-validation + restore-detach invariants.
1 parent 4f79bee commit 5bcd02f

6 files changed

Lines changed: 291 additions & 16 deletions

File tree

‎apps/sim/app/api/folders/[id]/duplicate/route.test.ts‎

Lines changed: 162 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,162 @@
1+
/**
2+
* Tests for the folder-duplicate route's target-parent ancestor walk.
3+
*
4+
* @vitest-environment node
5+
*/
6+
import { FolderLockedError } from '@sim/platform-authz/workflow'
7+
import { describe, expect, it, vi } from 'vitest'
8+
import { assertTargetParentFolderMutable } from '@/app/api/folders/[id]/duplicate/route'
9+
10+
describe('assertTargetParentFolderMutable', () => {
11+
const workspaceId = 'workspace-123'
12+
const sourceFolderId = 'source-folder'
13+
14+
/**
15+
* `assertTargetParentFolderMutable` issues one `select().from().where().limit()`
16+
* per ancestor hop, in walk order (target parent, then its parent, ...).
17+
* `eq(folderTable.id, currentFolderId)` isn't inspectable without mocking
18+
* drizzle-orm, so this mock returns each `chain` entry in call order instead.
19+
*/
20+
function buildTx(chain: Array<Record<string, unknown> | undefined>) {
21+
let call = 0
22+
return {
23+
select: vi.fn().mockReturnValue({
24+
from: vi.fn().mockReturnValue({
25+
where: vi.fn().mockReturnValue({
26+
limit: vi.fn().mockImplementation(() => {
27+
const row = chain[call]
28+
call += 1
29+
return row ? [row] : []
30+
}),
31+
}),
32+
}),
33+
}),
34+
} as unknown as Parameters<typeof assertTargetParentFolderMutable>[0]
35+
}
36+
37+
it('allows a target parent whose full ancestor chain is active', async () => {
38+
const tx = buildTx([
39+
{
40+
id: 'parent',
41+
parentId: 'grandparent',
42+
locked: false,
43+
workspaceId,
44+
resourceType: 'workflow',
45+
deletedAt: null,
46+
},
47+
{
48+
id: 'grandparent',
49+
parentId: null,
50+
locked: false,
51+
workspaceId,
52+
resourceType: 'workflow',
53+
deletedAt: null,
54+
},
55+
])
56+
57+
await expect(
58+
assertTargetParentFolderMutable(tx, 'parent', workspaceId, sourceFolderId)
59+
).resolves.toBeUndefined()
60+
})
61+
62+
it('rejects when the immediate target parent is soft-deleted', async () => {
63+
const tx = buildTx([
64+
{
65+
id: 'parent',
66+
parentId: null,
67+
locked: false,
68+
workspaceId,
69+
resourceType: 'workflow',
70+
deletedAt: new Date(),
71+
},
72+
])
73+
74+
await expect(
75+
assertTargetParentFolderMutable(tx, 'parent', workspaceId, sourceFolderId)
76+
).rejects.toThrow('Target parent folder not found')
77+
})
78+
79+
it('rejects when an ANCESTOR beyond the immediate parent is soft-deleted (regression: previously only checked the first hop)', async () => {
80+
const tx = buildTx([
81+
{
82+
id: 'parent',
83+
parentId: 'grandparent',
84+
locked: false,
85+
workspaceId,
86+
resourceType: 'workflow',
87+
deletedAt: null,
88+
},
89+
{
90+
id: 'grandparent',
91+
parentId: null,
92+
locked: false,
93+
workspaceId,
94+
resourceType: 'workflow',
95+
deletedAt: new Date(),
96+
},
97+
])
98+
99+
await expect(
100+
assertTargetParentFolderMutable(tx, 'parent', workspaceId, sourceFolderId)
101+
).rejects.toThrow('Target parent folder not found')
102+
})
103+
104+
it('rejects when the target parent belongs to a different workspace', async () => {
105+
const tx = buildTx([
106+
{
107+
id: 'parent',
108+
parentId: null,
109+
locked: false,
110+
workspaceId: 'other-workspace',
111+
resourceType: 'workflow',
112+
deletedAt: null,
113+
},
114+
])
115+
116+
await expect(
117+
assertTargetParentFolderMutable(tx, 'parent', workspaceId, sourceFolderId)
118+
).rejects.toThrow('Target parent folder not found')
119+
})
120+
121+
it('rejects duplicating into the source folder itself', async () => {
122+
const tx = buildTx([
123+
{
124+
id: sourceFolderId,
125+
parentId: null,
126+
locked: false,
127+
workspaceId,
128+
resourceType: 'workflow',
129+
deletedAt: null,
130+
},
131+
])
132+
133+
await expect(
134+
assertTargetParentFolderMutable(tx, sourceFolderId, workspaceId, sourceFolderId)
135+
).rejects.toThrow('Cannot duplicate folder into itself or one of its descendants')
136+
})
137+
138+
it('rejects when the target parent is locked', async () => {
139+
const tx = buildTx([
140+
{
141+
id: 'parent',
142+
parentId: null,
143+
locked: true,
144+
workspaceId,
145+
resourceType: 'workflow',
146+
deletedAt: null,
147+
},
148+
])
149+
150+
await expect(
151+
assertTargetParentFolderMutable(tx, 'parent', workspaceId, sourceFolderId)
152+
).rejects.toThrow(FolderLockedError)
153+
})
154+
155+
it('is a no-op for a null parentId (duplicating to root)', async () => {
156+
const tx = buildTx([])
157+
158+
await expect(
159+
assertTargetParentFolderMutable(tx, null, workspaceId, sourceFolderId)
160+
).resolves.toBeUndefined()
161+
})
162+
})

‎apps/sim/app/api/folders/[id]/duplicate/route.ts‎

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -244,15 +244,18 @@ export const POST = withRouteHandler(
244244
* write path checks via `assertFolderParentValid` is applied inline to the
245245
* immediate parent's row (the first loop iteration below), then the walk
246246
* continues up the full ancestor chain applying the workflow-specific
247-
* lock-cascade and self-descendant checks. Workspace/resourceType
248-
* consistency for ancestors beyond the immediate parent is guaranteed by the
249-
* `folder_parent_resource_type_match` DB trigger, so later iterations only
250-
* need to re-fetch `parentId`/`locked`. Checking inline on the one row
251-
* already being fetched for the walk avoids a redundant second SELECT of
252-
* the same immediate-parent row that a separate `assertFolderParentValid`
253-
* call would issue.
247+
* lock-cascade and self-descendant checks. `deletedAt` is re-checked on
248+
* EVERY hop, not just the immediate parent — the `folder_parent_resource_type_match`
249+
* trigger only fires when a `parent_id` edge is written, so it can't catch an
250+
* ancestor further up the chain being soft-deleted independently afterwards
251+
* (e.g. via a bulk admin import that writes `folder` rows directly). Only
252+
* workspace/resourceType consistency is safe to check first-iteration-only,
253+
* since the trigger enforces those on every write, including admin paths.
254+
* Checking inline on the one row already being fetched for the walk avoids a
255+
* redundant second SELECT of the same immediate-parent row that a separate
256+
* `assertFolderParentValid` call would issue.
254257
*/
255-
async function assertTargetParentFolderMutable(
258+
export async function assertTargetParentFolderMutable(
256259
tx: DbOrTx,
257260
parentId: string | null,
258261
targetWorkspaceId: string,
@@ -281,10 +284,9 @@ async function assertTargetParentFolderMutable(
281284

282285
if (
283286
!folder ||
287+
folder.deletedAt ||
284288
(isFirstIteration &&
285-
(folder.workspaceId !== targetWorkspaceId ||
286-
folder.resourceType !== 'workflow' ||
287-
folder.deletedAt))
289+
(folder.workspaceId !== targetWorkspaceId || folder.resourceType !== 'workflow'))
288290
) {
289291
throw new Error('Target parent folder not found')
290292
}

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

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -107,6 +107,28 @@ describe('PUT /api/folders/reorder', () => {
107107
expect(mockDb.transaction).not.toHaveBeenCalled()
108108
})
109109

110+
it('rejects a batch spanning more than one resourceType', async () => {
111+
mockWhere.mockReturnValueOnce([
112+
{ id: 'folder-1', workspaceId: 'workspace-123', resourceType: 'workflow' },
113+
{ id: 'folder-2', workspaceId: 'workspace-123', resourceType: 'file' },
114+
])
115+
116+
const req = createMockRequest('PUT', {
117+
workspaceId: 'workspace-123',
118+
updates: [
119+
{ id: 'folder-1', sortOrder: 0, parentId: null },
120+
{ id: 'folder-2', sortOrder: 0, parentId: null },
121+
],
122+
})
123+
124+
const response = await PUT(req)
125+
126+
expect(response.status).toBe(400)
127+
const data = await response.json()
128+
expect(data.error).toBe('All folders in a reorder batch must share the same resourceType')
129+
expect(mockDb.transaction).not.toHaveBeenCalled()
130+
})
131+
110132
it('rejects a batch that would form a cycle', async () => {
111133
mockWhere
112134
.mockReturnValueOnce([

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

Lines changed: 13 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,8 +58,20 @@ export const PUT = withRouteHandler(async (req: NextRequest) => {
5858
}
5959

6060
// A single reorder call operates on one resourceType at a time (the UI never mixes
61-
// folder types in one drag-drop tree); infer it from the first valid row.
61+
// folder types in one drag-drop tree). Reject a mixed-type batch explicitly instead
62+
// of silently reordering only the first-seen type and reporting success — a caller
63+
// bug that sends folders from two resource types should surface as an error, not a
64+
// partial `updated` count with no indication some entries were skipped.
6265
const resourceType = resourceTypeById.get(validUpdates[0].id)!
66+
const hasMixedResourceTypes = validUpdates.some(
67+
(u) => resourceTypeById.get(u.id) !== resourceType
68+
)
69+
if (hasMixedResourceTypes) {
70+
return NextResponse.json(
71+
{ error: 'All folders in a reorder batch must share the same resourceType' },
72+
{ status: 400 }
73+
)
74+
}
6375

6476
// Parent-id existence/workspace/resourceType/deleted validity is re-checked by
6577
// `performReorderFolders` (via `assertFolderParentValid`) below — this route does

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

Lines changed: 35 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -112,7 +112,13 @@ describe('Pinned Items API Route', () => {
112112
describe('GET /api/pinned-items', () => {
113113
it('should list pinned items for a workspace', async () => {
114114
mockAuthenticatedUser()
115-
mockWhere.mockReturnValueOnce([mockPinnedWorkflowRow, mockPinnedFolderRow])
115+
// 1st .where(): the pinned_item list query. 2nd/3rd: the per-resourceType
116+
// active-resource existence check filterActivePinnedItems() batches (one
117+
// per distinct type present — workflow, then folder, in row order).
118+
mockWhere
119+
.mockReturnValueOnce([mockPinnedWorkflowRow, mockPinnedFolderRow])
120+
.mockReturnValueOnce([{ id: 'workflow-1' }])
121+
.mockReturnValueOnce([{ id: 'folder-1' }])
116122

117123
const req = createMockRequest(
118124
'GET',
@@ -136,7 +142,9 @@ describe('Pinned Items API Route', () => {
136142

137143
it('should filter by resourceType when provided', async () => {
138144
mockAuthenticatedUser()
139-
mockWhere.mockReturnValueOnce([mockPinnedFolderRow])
145+
// 1st .where(): the pinned_item list query (folder-only). 2nd: the
146+
// filterActivePinnedItems() existence check for the single type present.
147+
mockWhere.mockReturnValueOnce([mockPinnedFolderRow]).mockReturnValueOnce([{ id: 'folder-1' }])
140148

141149
const req = createMockRequest(
142150
'GET',
@@ -153,6 +161,31 @@ describe('Pinned Items API Route', () => {
153161
expect(data.pinnedItems[0]).toMatchObject({ resourceType: 'folder', resourceId: 'folder-1' })
154162
})
155163

164+
it('excludes a pin whose underlying resource has since been deleted/archived', async () => {
165+
mockAuthenticatedUser()
166+
// The pinned_item row for the workflow pin still exists, but the
167+
// workflow itself is gone (empty existence-check result) — the stale
168+
// pin must not be returned. The folder pin's resource is still active.
169+
mockWhere
170+
.mockReturnValueOnce([mockPinnedWorkflowRow, mockPinnedFolderRow])
171+
.mockReturnValueOnce([])
172+
.mockReturnValueOnce([{ id: 'folder-1' }])
173+
174+
const req = createMockRequest(
175+
'GET',
176+
undefined,
177+
{},
178+
'http://localhost:3000/api/pinned-items?workspaceId=workspace-123'
179+
)
180+
181+
const response = await GET(req)
182+
183+
expect(response.status).toBe(200)
184+
const data = await response.json()
185+
expect(data.pinnedItems).toHaveLength(1)
186+
expect(data.pinnedItems[0]).toMatchObject({ resourceType: 'folder', resourceId: 'folder-1' })
187+
})
188+
156189
it('should return 401 for unauthenticated requests', async () => {
157190
mockUnauthenticated()
158191

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

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ import {
1010
import { createLogger } from '@sim/logger'
1111
import { getPostgresErrorCode } from '@sim/utils/errors'
1212
import { generateId } from '@sim/utils/id'
13-
import { and, eq, isNull, sql } from 'drizzle-orm'
13+
import { and, eq, inArray, isNull, sql } from 'drizzle-orm'
1414
import type { PgColumn, PgTable } from 'drizzle-orm/pg-core'
1515
import { type NextRequest, NextResponse } from 'next/server'
1616
import type { PinnedResourceType } from '@/lib/api/contracts'
@@ -99,6 +99,49 @@ function toPinnedItemApi(row: typeof pinnedItem.$inferSelect) {
9999
return { ...row, pinnedAt: row.pinnedAt.toISOString() }
100100
}
101101

102+
/**
103+
* Drops pins whose underlying resource has since been deleted/archived —
104+
* without this, a pin outlives its resource forever (the resource's own
105+
* delete never touches `pinned_item`), and a consumer that renders pins
106+
* without cross-referencing the live resource list would show a phantom
107+
* entry. Batches one existence query per distinct resourceType present in
108+
* `rows` (not one per row) to stay O(types) instead of O(n).
109+
*/
110+
async function filterActivePinnedItems(
111+
rows: (typeof pinnedItem.$inferSelect)[],
112+
workspaceId: string
113+
): Promise<(typeof pinnedItem.$inferSelect)[]> {
114+
const idsByType = new Map<PinnedResourceType, string[]>()
115+
for (const row of rows) {
116+
const type = row.resourceType as PinnedResourceType
117+
const ids = idsByType.get(type) ?? []
118+
ids.push(row.resourceId)
119+
idsByType.set(type, ids)
120+
}
121+
122+
const activeIdsByType = new Map<PinnedResourceType, Set<string>>()
123+
await Promise.all(
124+
Array.from(idsByType.entries()).map(async ([type, ids]) => {
125+
const config = PINNED_RESOURCE_LOOKUP[type]
126+
const activeRows = await db
127+
.select({ id: config.idColumn })
128+
.from(config.resourceTable)
129+
.where(
130+
and(
131+
inArray(config.idColumn, ids),
132+
eq(config.workspaceColumn, workspaceId),
133+
isNull(config.deletedColumn)
134+
)
135+
)
136+
activeIdsByType.set(type, new Set(activeRows.map((r) => r.id as string)))
137+
})
138+
)
139+
140+
return rows.filter((row) =>
141+
activeIdsByType.get(row.resourceType as PinnedResourceType)?.has(row.resourceId)
142+
)
143+
}
144+
102145
/** Lists pinned items for a workspace, optionally filtered to a single `resourceType`. */
103146
export const GET = withRouteHandler(async (request: NextRequest) => {
104147
const session = await getSession()
@@ -128,7 +171,8 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
128171
: and(eq(pinnedItem.userId, session.user.id), eq(pinnedItem.workspaceId, workspaceId))
129172
)
130173

131-
return NextResponse.json({ pinnedItems: rows.map(toPinnedItemApi) })
174+
const activeRows = await filterActivePinnedItems(rows, workspaceId)
175+
return NextResponse.json({ pinnedItems: activeRows.map(toPinnedItemApi) })
132176
})
133177

134178
export const POST = withRouteHandler(async (request: NextRequest) => {

0 commit comments

Comments
 (0)