Skip to content

Commit 50058d2

Browse files
committed
fix(review): exclude deleted folders from sort order; make the allowlist tests real
Three findings from cubic on #7062, all valid. 1. nextWorkflowSortOrder consulted the folder minimum without excluding soft-deleted folders. Because the helper returns min - 1, a deleted folder holding the lowest slot ratchets the floor down permanently — the same class of bug as the archived-workflow one this PR set out to fix, on the other half of the query. lib/folders/orchestration.ts already documents this exact rationale for the folder-creation side of the same algorithm, and the uploads folder manager filters it too; this was the outlier. 2. The two new allowlist tests did not call setEnterpriseOrgWorkspace(), so resolution never reached the group queries and validateBlockType returned early. They passed against the unfixed code when run in isolation and only appeared to fail in a full-file run, where mock state leaked from earlier tests. Verified with 'vitest -t': both now fail without the case-folding fix and pass with it. 3. The restore-resource comment this PR rewrote was itself wrong. A Partial map does not defer the failure into the cascade — the lookup widens to FolderResourceType | undefined and the error lands on the restoreFolder call site. Reworded to say that, and why keeping the check at the mapping matters.
1 parent 4befe38 commit 50058d2

3 files changed

Lines changed: 14 additions & 8 deletions

File tree

apps/sim/ee/access-control/utils/permission-check.test.ts

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -448,12 +448,14 @@ describe('validateBlockType', () => {
448448
})
449449

450450
it('case-folds a stored allowlist so a mixed-case entry still matches', async () => {
451+
setEnterpriseOrgWorkspace()
451452
queueGroupResolution([{ config: { allowedIntegrations: ['Slack'] } }])
452453

453454
await validateBlockType('user-123', 'workspace-1', 'slack')
454455
})
455456

456457
it('still rejects a block absent from a mixed-case stored allowlist', async () => {
458+
setEnterpriseOrgWorkspace()
457459
queueGroupResolution([{ config: { allowedIntegrations: ['Slack'] } }])
458460

459461
await expect(validateBlockType('user-123', 'workspace-1', 'discord')).rejects.toThrow(

apps/sim/lib/resources/orchestration/restore-resource.ts

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -47,10 +47,10 @@ type RestorableFolderType = 'folder' | 'knowledge_folder' | 'table_folder'
4747

4848
/**
4949
* Deliberately a total `Record` over the folder types, not a `Partial` one: adding a tree to
50-
* `RestorableFolderType` without a mapping here has to fail the build. With a partial map the
51-
* lookup would yield `undefined`, which `restoreFolder` types as a required
52-
* `FolderResourceType` — so the failure would surface as an undefined folder config deep in
53-
* the cascade rather than at the call site.
50+
* `RestorableFolderType` without a mapping has to fail the build *here*, at the mapping. A
51+
* `Partial` still compiles with the tree missing — the lookup widens to
52+
* `FolderResourceType | undefined`, so the error moves to the `restoreFolder` call site, and
53+
* suppressing it there leaves the cascade resolving an undefined folder config.
5454
*/
5555
const FOLDER_RESOURCE_TYPE_BY_RESTORABLE: Record<RestorableFolderType, FolderResourceType> = {
5656
folder: 'workflow',

apps/sim/lib/workflows/sort-order.ts

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,12 @@ import type { DbOrTx } from '@/lib/db/types'
66
/**
77
* Sort order placing a new workflow above everything already in its folder.
88
*
9-
* Workflows and folders share one ordering, so both minimums are consulted.
10-
* Archived workflows are excluded: a soft-deleted row must not hold a slot that
11-
* pushes new siblings further up each time one is created.
9+
* Workflows and folders share one ordering, so both minimums are consulted, and
10+
* *both* exclude soft-deleted rows. Because this returns `min - 1`, counting a
11+
* deleted row lets every delete ratchet the floor further negative and never
12+
* recover: a deleted sibling at -400 pins the next new workflow at -401 forever.
13+
* `lib/folders/orchestration.ts` documents the same rule for the folder-creation
14+
* side of this algorithm.
1215
*
1316
* Pass `tx` when the caller is inside a transaction, so the read sees that
1417
* transaction's uncommitted rows rather than the pre-transaction snapshot.
@@ -43,7 +46,8 @@ export async function nextWorkflowSortOrder(
4346
and(
4447
eq(folderTable.workspaceId, workspaceId),
4548
eq(folderTable.resourceType, 'workflow'),
46-
folderParentCondition
49+
folderParentCondition,
50+
isNull(folderTable.deletedAt)
4751
)
4852
),
4953
])

0 commit comments

Comments
 (0)