Skip to content

Commit 8c608ff

Browse files
committed
fix(folders): address round-2 review feedback on locking + reorder
- File-folder locked was never persisted: toFileFolder() hardcoded locked: false, and updateWorkspaceFileFolder() didn't accept/forward the field at all. Threaded locked through WorkspaceFileFolderRecord, updateWorkspaceFileFolder(), and the orchestration call site (Cursor) - Generic knowledge_base/table folder update branch silently dropped locked (only wrote name/parentId/sortOrder) -- a route could report success while the lock never took effect (Greptile P1) - Folder reorder now fails the WHOLE batch on any invalid parentId instead of silently skipping just that entry and reporting a partial success -- restores this endpoint's pre-generalization all-or-nothing guarantee, which an earlier round-1 fix regressed (Cursor)
1 parent d8c2558 commit 8c608ff

4 files changed

Lines changed: 38 additions & 36 deletions

File tree

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

Lines changed: 5 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -77,12 +77,15 @@ describe('PUT /api/folders/reorder', () => {
7777
expect(data).toMatchObject({ success: true, updated: 1 })
7878
})
7979

80-
it('rejects a parentId that belongs to another workspace', async () => {
80+
it('rejects a parentId that belongs to another workspace, failing the whole batch', async () => {
8181
// Parent-id validity is checked once, inside `performReorderFolders`
8282
// (via `assertFolderParentValid`) — the route no longer re-implements
8383
// this check, so the mock sequence covers: (1) route's own-id lookup,
8484
// (2) route's workspace-folders lookup for the circular-reference walk,
8585
// (3) orchestration's own-id lookup, (4) orchestration's parentId lookup.
86+
// A single invalid parentId fails the ENTIRE batch (matching this
87+
// endpoint's pre-generalization all-or-nothing behavior) rather than
88+
// silently skipping just that one entry.
8689
mockWhere
8790
.mockReturnValueOnce([
8891
{ id: 'folder-1', workspaceId: 'workspace-123', resourceType: 'workflow' },
@@ -103,7 +106,7 @@ describe('PUT /api/folders/reorder', () => {
103106

104107
expect(response.status).toBe(400)
105108
const data = await response.json()
106-
expect(data.error).toBe('No valid folders to update')
109+
expect(data.error).toBe('Parent folder not found')
107110
expect(mockDb.transaction).not.toHaveBeenCalled()
108111
})
109112

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

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -128,7 +128,10 @@ export const PUT = withRouteHandler(async (req: NextRequest) => {
128128
})
129129

130130
if (!result.success) {
131-
return NextResponse.json({ error: 'No valid folders to update' }, { status: 400 })
131+
return NextResponse.json(
132+
{ error: result.error ?? 'No valid folders to update' },
133+
{ status: 400 }
134+
)
132135
}
133136

134137
logger.info(`[${requestId}] Reordered ${result.updated} folders in workspace ${workspaceId}`)

apps/sim/lib/folders/orchestration.ts

Lines changed: 20 additions & 33 deletions
Original file line numberDiff line numberDiff line change
@@ -120,7 +120,7 @@ function toFileFolder(record: WorkspaceFileFolderRecord): Folder {
120120
userId: record.userId,
121121
workspaceId: record.workspaceId,
122122
parentId: record.parentId,
123-
locked: false,
123+
locked: record.locked,
124124
sortOrder: record.sortOrder,
125125
createdAt: record.createdAt,
126126
updatedAt: record.updatedAt,
@@ -204,6 +204,7 @@ async function performUpdateFileFolder(
204204
name: params.name,
205205
parentId: params.parentId,
206206
sortOrder: params.sortOrder,
207+
locked: params.locked,
207208
})
208209

209210
recordAudit({
@@ -684,6 +685,7 @@ export async function performUpdateFolder(
684685
if (params.name !== undefined) updates.name = params.name.trim()
685686
if (params.parentId !== undefined) updates.parentId = params.parentId || null
686687
if (params.sortOrder !== undefined) updates.sortOrder = params.sortOrder
688+
if (params.locked !== undefined) updates.locked = params.locked
687689

688690
const [updated] = await db
689691
.update(folderTable)
@@ -739,7 +741,7 @@ export async function performRestoreFolder(
739741

740742
export async function performReorderFolders(
741743
params: PerformReorderFoldersParams
742-
): Promise<{ success: boolean; updated: number }> {
744+
): Promise<{ success: boolean; updated: number; error?: string }> {
743745
const { resourceType, workspaceId, updates } = params
744746

745747
const folderIds = updates.map((u) => u.id)
@@ -751,48 +753,33 @@ export async function performReorderFolders(
751753
const validIds = new Set(
752754
existingFolders.filter((f) => f.workspaceId === workspaceId).map((f) => f.id)
753755
)
754-
const idValidUpdates = updates.filter((u) => validIds.has(u.id))
756+
const validUpdates = updates.filter((u) => validIds.has(u.id))
757+
if (validUpdates.length === 0) return { success: false, updated: 0 }
755758

756759
// A reparent (non-null parentId) must independently be validated against
757-
// `assertFolderParentValid` — `validIds` above only checks that `id` itself
758-
// belongs to this workspace/resourceType, not that the *new parent* does.
759-
// Without this, a reorder call could silently re-parent a folder under a
760-
// folder from a different workspace or resourceType. Invalid reparents are
761-
// skipped (not failed outright), matching this function's existing
762-
// skip-invalid-entries behavior for `validIds`.
760+
// `assertFolderParentValid` — the `validIds` check above only confirms `id`
761+
// itself belongs to this workspace/resourceType, not that the *new parent*
762+
// does. This is an all-or-nothing check: ANY invalid parentId fails the
763+
// whole batch before the transaction runs (rather than silently skipping
764+
// that one entry and reporting a partial `updated` count as success) —
765+
// matching this endpoint's pre-generalization behavior, where a bad
766+
// reparent surfaced as `Parent folder not found` up front.
763767
//
764768
// A subtree drag-drop can reparent many folders in one call, so the
765769
// distinct target parentIds are validated concurrently (one round-trip per
766770
// distinct parent, not one per update) rather than sequentially.
767771
const targetParentIds = Array.from(
768-
new Set(idValidUpdates.map((u) => u.parentId).filter((id): id is string => Boolean(id)))
772+
new Set(validUpdates.map((u) => u.parentId).filter((id): id is string => Boolean(id)))
769773
)
770-
const parentErrorEntries = await Promise.all(
771-
targetParentIds.map(
772-
async (parentId) =>
773-
[parentId, await assertFolderParentValid(parentId, { workspaceId, resourceType })] as const
774+
const parentErrors = await Promise.all(
775+
targetParentIds.map((parentId) =>
776+
assertFolderParentValid(parentId, { workspaceId, resourceType })
774777
)
775778
)
776-
const parentErrorById = new Map(parentErrorEntries)
777-
778-
const validUpdates: typeof idValidUpdates = []
779-
for (const update of idValidUpdates) {
780-
if (update.parentId) {
781-
const parentError = parentErrorById.get(update.parentId)
782-
if (parentError) {
783-
logger.warn('Skipping folder reorder entry with invalid parentId', {
784-
folderId: update.id,
785-
parentId: update.parentId,
786-
workspaceId,
787-
resourceType,
788-
error: parentError.error,
789-
})
790-
continue
791-
}
792-
}
793-
validUpdates.push(update)
779+
const firstParentError = parentErrors.find((error) => error !== null)
780+
if (firstParentError) {
781+
return { success: false, updated: 0, error: firstParentError.error }
794782
}
795-
if (validUpdates.length === 0) return { success: false, updated: 0 }
796783

797784
await db.transaction(async (tx) => {
798785
for (const update of validUpdates) {

apps/sim/lib/uploads/contexts/workspace/workspace-file-folder-manager.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -55,6 +55,7 @@ export interface WorkspaceFileFolderRecord {
5555
parentId: string | null
5656
path: string
5757
sortOrder: number
58+
locked: boolean
5859
deletedAt: Date | null
5960
createdAt: Date
6061
updatedAt: Date
@@ -67,6 +68,7 @@ interface RawWorkspaceFileFolder {
6768
name: string
6869
parentId: string | null
6970
sortOrder: number
71+
locked: boolean
7072
deletedAt: Date | null
7173
createdAt: Date
7274
updatedAt: Date
@@ -165,6 +167,7 @@ function mapFolder(
165167
parentId: folder.parentId,
166168
path: paths.get(folder.id) ?? folder.name,
167169
sortOrder: folder.sortOrder,
170+
locked: folder.locked,
168171
deletedAt: folder.deletedAt,
169172
createdAt: folder.createdAt,
170173
updatedAt: folder.updatedAt,
@@ -517,6 +520,7 @@ export async function ensureWorkspaceFileFolderPath(params: {
517520
name: created.name,
518521
parentId: created.parentId,
519522
sortOrder: created.sortOrder,
523+
locked: created.locked,
520524
deletedAt: created.deletedAt,
521525
createdAt: created.createdAt,
522526
updatedAt: created.updatedAt,
@@ -556,6 +560,7 @@ export async function updateWorkspaceFileFolder(params: {
556560
name?: string
557561
parentId?: string | null
558562
sortOrder?: number
563+
locked?: boolean
559564
}): Promise<WorkspaceFileFolderRecord> {
560565
const folder = await db.transaction(async (tx) => {
561566
await acquireWorkspaceFileFolderMutationLock(tx, params.workspaceId)
@@ -654,6 +659,10 @@ export async function updateWorkspaceFileFolder(params: {
654659
updates.sortOrder = params.sortOrder
655660
}
656661

662+
if (params.locked !== undefined) {
663+
updates.locked = params.locked
664+
}
665+
657666
try {
658667
const [updatedFolder] = await tx
659668
.update(workspaceFileFolder)

0 commit comments

Comments
 (0)