Skip to content

Commit 602f0dd

Browse files
icecrasher321icecrasher321
committed
address comments
1 parent c85220c commit 602f0dd

2 files changed

Lines changed: 279 additions & 20 deletions

File tree

‎apps/sim/lib/workflows/persistence/duplicate.test.ts‎

Lines changed: 226 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -383,6 +383,232 @@ describe('duplicateWorkflow ordering', () => {
383383
expect(copiedSubBlocks.variables.value[0].variableName).toBe('customerName')
384384
})
385385

386+
it('preserves remap when an edge references an unknown source block (drops the edge with a warning)', async () => {
387+
let insertedEdges: Array<Record<string, unknown>> | null = null
388+
const tx = createMockTx(
389+
[
390+
[
391+
{
392+
id: 'source-workflow-id',
393+
workspaceId: 'workspace-123',
394+
folderId: null,
395+
description: 'source',
396+
color: '#000000',
397+
variables: {},
398+
},
399+
],
400+
[],
401+
[],
402+
[
403+
{
404+
id: 'block-a',
405+
workflowId: 'source-workflow-id',
406+
type: 'agent',
407+
name: 'Agent A',
408+
parentId: null,
409+
extent: null,
410+
data: {},
411+
subBlocks: {},
412+
position: { x: 0, y: 0 },
413+
enabled: true,
414+
horizontalHandles: true,
415+
isWide: false,
416+
height: 0,
417+
advancedMode: false,
418+
triggerMode: false,
419+
locked: false,
420+
createdAt: new Date(),
421+
updatedAt: new Date(),
422+
},
423+
{
424+
id: 'block-b',
425+
workflowId: 'source-workflow-id',
426+
type: 'agent',
427+
name: 'Agent B',
428+
parentId: null,
429+
extent: null,
430+
data: {},
431+
subBlocks: {},
432+
position: { x: 0, y: 0 },
433+
enabled: true,
434+
horizontalHandles: true,
435+
isWide: false,
436+
height: 0,
437+
advancedMode: false,
438+
triggerMode: false,
439+
locked: false,
440+
createdAt: new Date(),
441+
updatedAt: new Date(),
442+
},
443+
],
444+
[
445+
{
446+
id: 'edge-valid',
447+
workflowId: 'source-workflow-id',
448+
sourceBlockId: 'block-a',
449+
targetBlockId: 'block-b',
450+
sourceHandle: null,
451+
targetHandle: null,
452+
createdAt: new Date(),
453+
updatedAt: new Date(),
454+
},
455+
{
456+
id: 'edge-orphan',
457+
workflowId: 'source-workflow-id',
458+
sourceBlockId: 'unknown-source-block',
459+
targetBlockId: 'block-b',
460+
sourceHandle: null,
461+
targetHandle: null,
462+
createdAt: new Date(),
463+
updatedAt: new Date(),
464+
},
465+
],
466+
[],
467+
],
468+
undefined,
469+
(values) => {
470+
if (
471+
Array.isArray(values) &&
472+
values.length > 0 &&
473+
(values[0] as Record<string, unknown>)?.sourceBlockId !== undefined
474+
) {
475+
insertedEdges = values as Array<Record<string, unknown>>
476+
}
477+
}
478+
)
479+
480+
mockDb.transaction.mockImplementation(async (callback: (txArg: unknown) => Promise<unknown>) =>
481+
callback(tx)
482+
)
483+
484+
await expect(
485+
duplicateWorkflow({
486+
sourceWorkflowId: 'source-workflow-id',
487+
userId: 'user-123',
488+
name: 'Duplicated',
489+
workspaceId: 'workspace-123',
490+
folderId: null,
491+
requestId: 'req-orphan-edge',
492+
})
493+
).resolves.toBeDefined()
494+
495+
expect(insertedEdges).toHaveLength(1)
496+
const onlyEdge = insertedEdges?.[0]
497+
expect(onlyEdge?.sourceBlockId).not.toBe('unknown-source-block')
498+
expect(onlyEdge?.sourceBlockId).toEqual(expect.any(String))
499+
expect(onlyEdge?.targetBlockId).toEqual(expect.any(String))
500+
})
501+
502+
it('preserves remap when a subflow references an unknown node (drops the node with a warning)', async () => {
503+
let insertedSubflows: Array<Record<string, unknown>> | null = null
504+
const tx = createMockTx(
505+
[
506+
[
507+
{
508+
id: 'source-workflow-id',
509+
workspaceId: 'workspace-123',
510+
folderId: null,
511+
description: 'source',
512+
color: '#000000',
513+
variables: {},
514+
},
515+
],
516+
[],
517+
[],
518+
[
519+
{
520+
id: 'loop-block',
521+
workflowId: 'source-workflow-id',
522+
type: 'loop',
523+
name: 'Loop',
524+
parentId: null,
525+
extent: null,
526+
data: {},
527+
subBlocks: {},
528+
position: { x: 0, y: 0 },
529+
enabled: true,
530+
horizontalHandles: true,
531+
isWide: false,
532+
height: 0,
533+
advancedMode: false,
534+
triggerMode: false,
535+
locked: false,
536+
createdAt: new Date(),
537+
updatedAt: new Date(),
538+
},
539+
{
540+
id: 'known-node',
541+
workflowId: 'source-workflow-id',
542+
type: 'agent',
543+
name: 'Agent',
544+
parentId: null,
545+
extent: null,
546+
data: {},
547+
subBlocks: {},
548+
position: { x: 0, y: 0 },
549+
enabled: true,
550+
horizontalHandles: true,
551+
isWide: false,
552+
height: 0,
553+
advancedMode: false,
554+
triggerMode: false,
555+
locked: false,
556+
createdAt: new Date(),
557+
updatedAt: new Date(),
558+
},
559+
],
560+
[],
561+
[
562+
{
563+
id: 'loop-block',
564+
workflowId: 'source-workflow-id',
565+
type: 'loop',
566+
config: {
567+
id: 'loop-block',
568+
nodes: ['known-node', 'unknown-node'],
569+
iterations: 1,
570+
loopType: 'for',
571+
},
572+
createdAt: new Date(),
573+
updatedAt: new Date(),
574+
},
575+
],
576+
],
577+
undefined,
578+
(values) => {
579+
if (
580+
Array.isArray(values) &&
581+
values.length > 0 &&
582+
(values[0] as Record<string, unknown>)?.config !== undefined
583+
) {
584+
insertedSubflows = values as Array<Record<string, unknown>>
585+
}
586+
}
587+
)
588+
589+
mockDb.transaction.mockImplementation(async (callback: (txArg: unknown) => Promise<unknown>) =>
590+
callback(tx)
591+
)
592+
593+
await expect(
594+
duplicateWorkflow({
595+
sourceWorkflowId: 'source-workflow-id',
596+
userId: 'user-123',
597+
name: 'Duplicated',
598+
workspaceId: 'workspace-123',
599+
folderId: null,
600+
requestId: 'req-orphan-subflow',
601+
})
602+
).resolves.toBeDefined()
603+
604+
expect(insertedSubflows).toHaveLength(1)
605+
const remappedConfig = insertedSubflows?.[0].config as { nodes: string[] }
606+
expect(Array.isArray(remappedConfig.nodes)).toBe(true)
607+
expect(remappedConfig.nodes).toHaveLength(1)
608+
expect(remappedConfig.nodes[0]).not.toBe('unknown-node')
609+
expect(remappedConfig.nodes[0]).toEqual(expect.any(String))
610+
})
611+
386612
it('preserves stale variable references instead of failing the duplicate', async () => {
387613
let insertedBlocks: Array<Record<string, unknown>> | null = null
388614
const tx = createMockTx(

‎apps/sim/lib/workflows/persistence/duplicate.ts‎

Lines changed: 53 additions & 20 deletions
Original file line numberDiff line numberDiff line change
@@ -504,31 +504,52 @@ export async function duplicateWorkflow(
504504
.where(eq(workflowEdges.workflowId, sourceWorkflowId))
505505

506506
if (sourceEdges.length > 0) {
507-
const newEdges = sourceEdges.map((edge) => {
507+
/**
508+
* Edge remap is best-effort: when an edge points at a source/target block
509+
* that isn't in `blockIdMapping`, we drop the edge with a `logger.warn`
510+
* instead of throwing. This matches the pre-PR leniency (which silently
511+
* kept stale references) and avoids rolling back an entire folder-tree
512+
* duplicate transaction over a single orphaned reference. Inserting an
513+
* edge with a stale block id would create a dangling DB row, so we skip
514+
* the edge entirely rather than carry forward the unmapped id.
515+
*/
516+
const newEdges = sourceEdges.flatMap((edge) => {
508517
const newSourceBlockId = blockIdMapping.get(edge.sourceBlockId)
509518
const newTargetBlockId = blockIdMapping.get(edge.targetBlockId)
510519
if (!newSourceBlockId || !newTargetBlockId) {
511-
throw new Error(`Edge ${edge.id} references a block that could not be remapped`)
520+
logger.warn('Skipping edge with unmapped block reference during duplication', {
521+
requestId,
522+
edgeId: edge.id,
523+
sourceBlockId: edge.sourceBlockId,
524+
targetBlockId: edge.targetBlockId,
525+
})
526+
return []
512527
}
513528
const newSourceHandle =
514529
edge.sourceHandle && blockIdMapping.has(edge.sourceBlockId)
515530
? remapConditionEdgeHandle(edge.sourceHandle, edge.sourceBlockId, newSourceBlockId)
516531
: edge.sourceHandle
517532

518-
return {
519-
...edge,
520-
id: generateId(),
521-
workflowId: newWorkflowId,
522-
sourceBlockId: newSourceBlockId,
523-
targetBlockId: newTargetBlockId,
524-
sourceHandle: newSourceHandle,
525-
createdAt: now,
526-
updatedAt: now,
527-
}
533+
return [
534+
{
535+
...edge,
536+
id: generateId(),
537+
workflowId: newWorkflowId,
538+
sourceBlockId: newSourceBlockId,
539+
targetBlockId: newTargetBlockId,
540+
sourceHandle: newSourceHandle,
541+
createdAt: now,
542+
updatedAt: now,
543+
},
544+
]
528545
})
529546

530-
await tx.insert(workflowEdges).values(newEdges)
531-
logger.info(`[${requestId}] Copied ${sourceEdges.length} edges with updated block references`)
547+
if (newEdges.length > 0) {
548+
await tx.insert(workflowEdges).values(newEdges)
549+
}
550+
logger.info(
551+
`[${requestId}] Copied ${newEdges.length}/${sourceEdges.length} edges with updated block references`
552+
)
532553
}
533554

534555
// Copy all subflows from source workflow with new IDs and updated block references
@@ -567,16 +588,28 @@ export async function duplicateWorkflow(
567588

568589
;(updatedConfig as any).id = newSubflowId
569590

570-
// Update node references in config if they exist
591+
/**
592+
* Subflow node remap is best-effort: when `config.nodes` lists a
593+
* block id that isn't in `blockIdMapping`, we drop the entry with
594+
* a `logger.warn` instead of throwing. This matches the pre-PR
595+
* leniency (which silently carried the stale id forward) and
596+
* avoids rolling back an entire folder-tree duplicate transaction
597+
* over a single orphaned reference. Downstream consumers and
598+
* cleanup tolerate missing membership entries the same way they
599+
* tolerate other persisted drift.
600+
*/
571601
if ('nodes' in updatedConfig && Array.isArray(updatedConfig.nodes)) {
572-
updatedConfig.nodes = updatedConfig.nodes.map((nodeId: string) => {
602+
updatedConfig.nodes = updatedConfig.nodes.flatMap((nodeId: string) => {
573603
const newNodeId = blockIdMapping.get(nodeId)
574604
if (!newNodeId) {
575-
throw new Error(
576-
`Subflow ${subflow.id} references node ${nodeId} that could not be remapped`
577-
)
605+
logger.warn('Skipping unmapped subflow node reference during duplication', {
606+
requestId,
607+
subflowId: subflow.id,
608+
nodeId,
609+
})
610+
return []
578611
}
579-
return newNodeId
612+
return [newNodeId]
580613
})
581614
}
582615
}

0 commit comments

Comments
 (0)