Skip to content

Commit a3a9918

Browse files
PlaneInABottlePlaneInABottle
committed
fix: preserve protected batch adds and co-moving reparent sync
1 parent d330694 commit a3a9918

File tree

2 files changed

+215
-21
lines changed

2 files changed

+215
-21
lines changed

‎apps/sim/socket/database/operations.test.ts‎

Lines changed: 163 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,163 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
5+
import { describe, expect, it, vi } from 'vitest'
6+
import {
7+
filterBatchAddBlocksByProtection,
8+
getBatchUpdateParentCoMovingKey,
9+
} from '@/socket/database/operations'
10+
11+
vi.mock('@sim/db', () => ({
12+
webhook: {},
13+
workflow: {},
14+
workflowBlocks: {},
15+
workflowEdges: {},
16+
workflowSubflows: {},
17+
}))
18+
19+
vi.mock('@sim/logger', () => ({
20+
createLogger: vi.fn(() => ({
21+
info: vi.fn(),
22+
warn: vi.fn(),
23+
error: vi.fn(),
24+
debug: vi.fn(),
25+
})),
26+
}))
27+
28+
vi.mock('drizzle-orm', () => ({
29+
and: vi.fn(),
30+
eq: vi.fn(),
31+
inArray: vi.fn(),
32+
isNull: vi.fn(),
33+
or: vi.fn(),
34+
sql: vi.fn(),
35+
}))
36+
37+
vi.mock('drizzle-orm/postgres-js', () => ({
38+
drizzle: vi.fn(() => ({ transaction: vi.fn() })),
39+
}))
40+
41+
vi.mock('postgres', () => ({
42+
default: vi.fn(() => ({})),
43+
}))
44+
45+
vi.mock('@/lib/audit/log', () => ({
46+
AuditAction: {},
47+
AuditResourceType: {},
48+
recordAudit: vi.fn(),
49+
}))
50+
51+
vi.mock('@/lib/core/config/env', () => ({
52+
env: {
53+
DATABASE_URL: 'postgresql://test:test@localhost:5432/test',
54+
},
55+
}))
56+
57+
vi.mock('@/lib/webhooks/provider-subscriptions', () => ({
58+
cleanupExternalWebhook: vi.fn(),
59+
}))
60+
61+
vi.mock('@/lib/workflows/active-context', () => ({
62+
getActiveWorkflowContext: vi.fn(),
63+
}))
64+
65+
vi.mock('@/lib/workflows/persistence/utils', () => ({
66+
loadWorkflowFromNormalizedTables: vi.fn(),
67+
}))
68+
69+
vi.mock('@/lib/workflows/subblocks', () => ({
70+
mergeSubBlockValues: vi.fn(),
71+
}))
72+
73+
vi.mock('@/blocks/registry', () => ({
74+
getBlock: vi.fn(),
75+
}))
76+
77+
vi.mock('@/triggers', () => ({
78+
getTrigger: vi.fn(),
79+
isTriggerValid: vi.fn(() => false),
80+
}))
81+
82+
describe('getBatchUpdateParentCoMovingKey', () => {
83+
it('groups blocks entering the same destination container together', () => {
84+
expect(getBatchUpdateParentCoMovingKey('old-parent-a', 'loop-1')).toBe(
85+
JSON.stringify(['destination', 'loop-1'])
86+
)
87+
expect(getBatchUpdateParentCoMovingKey('old-parent-b', 'loop-1')).toBe(
88+
JSON.stringify(['destination', 'loop-1'])
89+
)
90+
})
91+
92+
it('keeps removal transitions scoped by old parent when leaving containers', () => {
93+
expect(getBatchUpdateParentCoMovingKey('loop-1', null)).toBe(JSON.stringify(['loop-1', null]))
94+
expect(getBatchUpdateParentCoMovingKey('loop-2', null)).toBe(JSON.stringify(['loop-2', null]))
95+
})
96+
})
97+
98+
describe('filterBatchAddBlocksByProtection', () => {
99+
it('filters blocks added under locked ancestor containers', () => {
100+
const allowedBlocks = filterBatchAddBlocksByProtection(
101+
[
102+
{
103+
id: 'child-under-locked-descendant',
104+
data: { parentId: 'inner-container' },
105+
},
106+
{
107+
id: 'child-under-unlocked-parent',
108+
data: { parentId: 'free-container' },
109+
},
110+
],
111+
[
112+
{
113+
id: 'locked-root',
114+
locked: true,
115+
data: {},
116+
},
117+
{
118+
id: 'inner-container',
119+
locked: false,
120+
data: { parentId: 'locked-root' },
121+
},
122+
{
123+
id: 'free-container',
124+
locked: false,
125+
data: {},
126+
},
127+
]
128+
)
129+
130+
expect(allowedBlocks).toEqual([
131+
{
132+
id: 'child-under-unlocked-parent',
133+
data: { parentId: 'free-container' },
134+
},
135+
])
136+
})
137+
138+
it('allows co-added container subtrees when no ancestor is locked', () => {
139+
const allowedBlocks = filterBatchAddBlocksByProtection(
140+
[
141+
{
142+
id: 'new-container',
143+
locked: false,
144+
data: { parentId: 'free-root' },
145+
},
146+
{
147+
id: 'new-child',
148+
data: { parentId: 'new-container' },
149+
},
150+
],
151+
[
152+
{
153+
id: 'free-root',
154+
locked: false,
155+
data: {},
156+
},
157+
]
158+
)
159+
160+
expect(allowedBlocks).toHaveLength(2)
161+
expect(allowedBlocks.map((block) => block.id)).toEqual(['new-container', 'new-child'])
162+
})
163+
})

‎apps/sim/socket/database/operations.ts‎

Lines changed: 52 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -49,6 +49,31 @@ interface DbBlockRef {
4949
data: unknown
5050
}
5151

52+
export function filterBatchAddBlocksByProtection(
53+
blocks: Array<Record<string, unknown>>,
54+
existingBlocks: Array<{ id: string; locked?: boolean | null; data: unknown }>
55+
): Array<Record<string, unknown>> {
56+
const blocksById: Record<string, DbBlockRef> = Object.fromEntries(
57+
existingBlocks.map((block) => [block.id, block])
58+
)
59+
60+
for (const block of blocks) {
61+
const id = block.id as string | undefined
62+
if (!id) continue
63+
64+
blocksById[id] = {
65+
id,
66+
locked: (block.locked as boolean | null | undefined) ?? false,
67+
data: (block.data as Record<string, unknown> | null | undefined) ?? {},
68+
}
69+
}
70+
71+
return blocks.filter((block) => {
72+
const id = block.id as string | undefined
73+
return id !== undefined && !isDbBlockProtected(id, blocksById)
74+
})
75+
}
76+
5277
/**
5378
* Checks if a block is protected (locked or inside a locked ancestor).
5479
* Works with raw DB records.
@@ -127,6 +152,15 @@ function getParentTransitionKey(
127152
return JSON.stringify([oldParentId ?? null, newParentId ?? null])
128153
}
129154

155+
export function getBatchUpdateParentCoMovingKey(
156+
oldParentId: string | null | undefined,
157+
newParentId: string | null | undefined
158+
): string {
159+
return newParentId
160+
? JSON.stringify(['destination', newParentId])
161+
: getParentTransitionKey(oldParentId, newParentId)
162+
}
163+
130164
/**
131165
* Remove cross-boundary edges when a block is reparented into or out of a container.
132166
*
@@ -1099,29 +1133,25 @@ async function handleBlocksOperationTx(
10991133
if (blocks && blocks.length > 0) {
11001134
// Fetch existing blocks to check for locked parents
11011135
const existingBlocks = await tx
1102-
.select({ id: workflowBlocks.id, locked: workflowBlocks.locked })
1136+
.select({
1137+
id: workflowBlocks.id,
1138+
locked: workflowBlocks.locked,
1139+
data: workflowBlocks.data,
1140+
})
11031141
.from(workflowBlocks)
11041142
.where(eq(workflowBlocks.workflowId, workflowId))
11051143

11061144
type ExistingBlockRecord = (typeof existingBlocks)[number]
11071145
existingBlockIds = new Set(existingBlocks.map((block: ExistingBlockRecord) => block.id))
1108-
const lockedParentIds = new Set(
1146+
allowedBlocks = filterBatchAddBlocksByProtection(
1147+
blocks as Array<Record<string, unknown>>,
11091148
existingBlocks
1110-
.filter((b: ExistingBlockRecord) => b.locked)
1111-
.map((b: ExistingBlockRecord) => b.id)
11121149
)
11131150

1114-
// Filter out blocks being added to locked parents
1115-
allowedBlocks = (blocks as Array<Record<string, unknown>>).filter((block) => {
1116-
const parentId = (block.data as Record<string, unknown> | null)?.parentId as
1117-
| string
1118-
| undefined
1119-
if (parentId && lockedParentIds.has(parentId)) {
1120-
logger.info(`Skipping block ${block.id} - parent ${parentId} is locked`)
1121-
return false
1122-
}
1123-
return true
1124-
})
1151+
const skippedBlockCount = blocks.length - allowedBlocks.length
1152+
if (skippedBlockCount > 0) {
1153+
logger.info(`Filtered out ${skippedBlockCount} batch-added block(s) due to protection`)
1154+
}
11251155

11261156
if (allowedBlocks.length === 0) {
11271157
logger.info('All blocks filtered out due to locked parents, skipping add')
@@ -1683,7 +1713,7 @@ async function handleBlocksOperationTx(
16831713
const allRemovedEdgeIds: string[] = []
16841714
const allAddedEdges: NonNullable<OperationResult['addedEdges']> = []
16851715
const applicableUpdates: Array<(typeof updates)[number]> = []
1686-
const coMovingBlockIdsByTransition = new Map<string, Set<string>>()
1716+
const coMovingBlockIdsByDestination = new Map<string, Set<string>>()
16871717

16881718
for (const update of updates) {
16891719
const { id, parentId, position } = update
@@ -1714,11 +1744,10 @@ async function handleBlocksOperationTx(
17141744
const existingParentId = (existing.data as Record<string, unknown> | null)?.parentId as
17151745
| string
17161746
| undefined
1717-
const transitionKey = getParentTransitionKey(existingParentId, parentId)
1718-
const coMovingBlockIds =
1719-
coMovingBlockIdsByTransition.get(transitionKey) ?? new Set<string>()
1747+
const coMovingKey = getBatchUpdateParentCoMovingKey(existingParentId, parentId)
1748+
const coMovingBlockIds = coMovingBlockIdsByDestination.get(coMovingKey) ?? new Set<string>()
17201749
coMovingBlockIds.add(id)
1721-
coMovingBlockIdsByTransition.set(transitionKey, coMovingBlockIds)
1750+
coMovingBlockIdsByDestination.set(coMovingKey, coMovingBlockIds)
17221751
}
17231752

17241753
for (const update of applicableUpdates) {
@@ -1769,7 +1798,9 @@ async function handleBlocksOperationTx(
17691798
id,
17701799
parentId ?? null,
17711800
existingParentId ?? null,
1772-
coMovingBlockIdsByTransition.get(getParentTransitionKey(existingParentId, parentId))
1801+
coMovingBlockIdsByDestination.get(
1802+
getBatchUpdateParentCoMovingKey(existingParentId, parentId)
1803+
)
17731804
)
17741805
allRemovedEdgeIds.push(...removedEdgeIds)
17751806

0 commit comments

Comments
 (0)