Skip to content

Commit b110966

Browse files
committed
address comments
1 parent ebd570a commit b110966

File tree

4 files changed

+95
-31
lines changed

4 files changed

+95
-31
lines changed

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/workflow.tsx

Lines changed: 32 additions & 28 deletions
Original file line numberDiff line numberDiff line change
@@ -246,7 +246,7 @@ const WorkflowContent = React.memo(
246246
const [selectedEdges, setSelectedEdges] = useState<SelectedEdgesMap>(new Map())
247247
const [isErrorConnectionDrag, setIsErrorConnectionDrag] = useState(false)
248248
const canvasContainerRef = useRef<HTMLDivElement>(null)
249-
const selectedIdsRef = useRef<string[] | null>(null)
249+
const displayNodesRef = useRef<Node[]>([])
250250
const embeddedFitFrameRef = useRef<number | null>(null)
251251
const hasCompletedInitialEmbeddedFitRef = useRef(false)
252252
const canvasMode = useCanvasModeStore((state) => state.mode)
@@ -2477,6 +2477,10 @@ const WorkflowContent = React.memo(
24772477
// Local state for nodes - allows smooth drag without store updates on every frame
24782478
const [displayNodes, setDisplayNodes] = useState<Node[]>([])
24792479

2480+
useEffect(() => {
2481+
displayNodesRef.current = displayNodes
2482+
}, [displayNodes])
2483+
24802484
useEffect(() => {
24812485
// Check for pending selection (from paste/duplicate), otherwise preserve existing selection
24822486
if (pendingSelection && pendingSelection.length > 0) {
@@ -2709,17 +2713,17 @@ const WorkflowContent = React.memo(
27092713
/** Handles node changes - applies changes and resolves parent-child selection conflicts. */
27102714
const onNodesChange = useCallback(
27112715
(changes: NodeChange[]) => {
2712-
selectedIdsRef.current = null
2713-
setDisplayNodes((nds) => {
2714-
const updated = applyNodeChanges(changes, nds)
2715-
const hasSelectionChange = changes.some((c) => c.type === 'select')
2716-
if (!hasSelectionChange) return updated
2717-
const resolved = resolveParentChildSelectionConflicts(updated, blocks)
2718-
selectedIdsRef.current = resolved.filter((node) => node.selected).map((node) => node.id)
2719-
return resolved
2720-
})
2721-
const selectedIds = selectedIdsRef.current as string[] | null
2722-
if (selectedIds !== null) {
2716+
const updated = applyNodeChanges(changes, displayNodesRef.current)
2717+
const hasSelectionChange = changes.some((c) => c.type === 'select')
2718+
const nextNodes = hasSelectionChange
2719+
? resolveParentChildSelectionConflicts(updated, blocks)
2720+
: updated
2721+
2722+
displayNodesRef.current = nextNodes
2723+
setDisplayNodes(nextNodes)
2724+
2725+
if (hasSelectionChange) {
2726+
const selectedIds = nextNodes.filter((node) => node.selected).map((node) => node.id)
27232727
syncPanelWithSelection(selectedIds)
27242728
}
27252729

@@ -3610,22 +3614,22 @@ const WorkflowContent = React.memo(
36103614
const handleNodeClick = useCallback(
36113615
(event: React.MouseEvent, node: Node) => {
36123616
const isMultiSelect = event.shiftKey || event.metaKey || event.ctrlKey
3613-
selectedIdsRef.current = null
3614-
setDisplayNodes((nodes) => {
3615-
const updated = nodes.map((n) => ({
3616-
...n,
3617-
selected: isMultiSelect ? (n.id === node.id ? true : n.selected) : n.id === node.id,
3618-
}))
3619-
const resolved = resolveParentChildSelectionConflicts(updated, blocks)
3620-
selectedIdsRef.current = resolved
3621-
.filter((selectedNode) => selectedNode.selected)
3622-
.map((selectedNode) => selectedNode.id)
3623-
return resolved
3624-
})
3625-
const selectedIds = selectedIdsRef.current as string[] | null
3626-
if (selectedIds !== null) {
3627-
syncPanelWithSelection(selectedIds)
3628-
}
3617+
const updated = displayNodesRef.current.map((currentNode) => ({
3618+
...currentNode,
3619+
selected: isMultiSelect
3620+
? currentNode.id === node.id
3621+
? true
3622+
: currentNode.selected
3623+
: currentNode.id === node.id,
3624+
}))
3625+
const resolved = resolveParentChildSelectionConflicts(updated, blocks)
3626+
const selectedIds = resolved
3627+
.filter((selectedNode) => selectedNode.selected)
3628+
.map((selectedNode) => selectedNode.id)
3629+
3630+
displayNodesRef.current = resolved
3631+
setDisplayNodes(resolved)
3632+
syncPanelWithSelection(selectedIds)
36293633
},
36303634
[blocks]
36313635
)

apps/sim/executor/dag/builder.test.ts

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,6 +92,57 @@ describe('DAGBuilder disabled subflow validation', () => {
9292
// Should not throw - loop is effectively disabled since all inner blocks are disabled
9393
expect(() => builder.build(workflow)).not.toThrow()
9494
})
95+
96+
it('does not mutate serialized loop config nodes during DAG build', () => {
97+
const workflow: SerializedWorkflow = {
98+
version: '1',
99+
blocks: [
100+
createBlock('start', BlockType.STARTER),
101+
createBlock('loop-1', BlockType.LOOP),
102+
{ ...createBlock('inner-block', BlockType.FUNCTION), enabled: false },
103+
],
104+
connections: [{ source: 'start', target: 'loop-1' }],
105+
loops: {
106+
'loop-1': {
107+
id: 'loop-1',
108+
nodes: ['inner-block'],
109+
iterations: 3,
110+
},
111+
},
112+
parallels: {},
113+
}
114+
115+
const builder = new DAGBuilder()
116+
builder.build(workflow)
117+
118+
expect(workflow.loops?.['loop-1']?.nodes).toEqual(['inner-block'])
119+
})
120+
121+
it('does not mutate serialized parallel config nodes during DAG build', () => {
122+
const workflow: SerializedWorkflow = {
123+
version: '1',
124+
blocks: [
125+
createBlock('start', BlockType.STARTER),
126+
createBlock('parallel-1', BlockType.PARALLEL),
127+
{ ...createBlock('inner-block', BlockType.FUNCTION), enabled: false },
128+
],
129+
connections: [{ source: 'start', target: 'parallel-1' }],
130+
loops: {},
131+
parallels: {
132+
'parallel-1': {
133+
id: 'parallel-1',
134+
nodes: ['inner-block'],
135+
count: 2,
136+
parallelType: 'count',
137+
},
138+
},
139+
}
140+
141+
const builder = new DAGBuilder()
142+
builder.build(workflow)
143+
144+
expect(workflow.parallels?.['parallel-1']?.nodes).toEqual(['inner-block'])
145+
})
95146
})
96147

97148
describe('DAGBuilder nested parallel support', () => {

apps/sim/executor/dag/builder.ts

Lines changed: 8 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -113,13 +113,19 @@ export class DAGBuilder {
113113
private initializeConfigs(workflow: SerializedWorkflow, dag: DAG): void {
114114
if (workflow.loops) {
115115
for (const [loopId, loopConfig] of Object.entries(workflow.loops)) {
116-
dag.loopConfigs.set(loopId, loopConfig)
116+
dag.loopConfigs.set(loopId, {
117+
...loopConfig,
118+
nodes: [...(loopConfig.nodes ?? [])],
119+
})
117120
}
118121
}
119122

120123
if (workflow.parallels) {
121124
for (const [parallelId, parallelConfig] of Object.entries(workflow.parallels)) {
122-
dag.parallelConfigs.set(parallelId, parallelConfig)
125+
dag.parallelConfigs.set(parallelId, {
126+
...parallelConfig,
127+
nodes: [...(parallelConfig.nodes ?? [])],
128+
})
123129
}
124130
}
125131
}

apps/sim/executor/orchestrators/parallel.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -76,7 +76,10 @@ export class ParallelOrchestrator {
7676
items = resolved.items
7777
isEmpty = resolved.isEmpty ?? false
7878
} catch (error) {
79-
const errorMessage = `Parallel Items did not resolve: ${error instanceof Error ? error.message : String(error)}`
79+
const baseErrorMessage = error instanceof Error ? error.message : String(error)
80+
const errorMessage = baseErrorMessage.startsWith('Parallel collection distribution is empty')
81+
? baseErrorMessage
82+
: `Parallel Items did not resolve: ${baseErrorMessage}`
8083
logger.error(errorMessage, { parallelId, distribution: parallelConfig.distribution })
8184
await this.addParallelErrorLog(ctx, parallelId, errorMessage, {
8285
distribution: parallelConfig.distribution,

0 commit comments

Comments
 (0)