Skip to content

Commit a2c08e1

Browse files
fix(subflows): subflow-child selection issues, subflow error logs (#3656)
* fix(subflows): subflow-child selection issues, subflow error logs * address comments * make selection context more reliable * fix more multiselect issues * fix shift selection ordering to work correctly * fix more comments * address more comments * reuse helper
1 parent 5332614 commit a2c08e1

File tree

9 files changed

+286
-75
lines changed

9 files changed

+286
-75
lines changed

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/subflows/subflow-node.tsx

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -168,7 +168,7 @@ export const SubflowNodeComponent = memo(({ data, id, selected }: NodeProps<Subf
168168
<ActionBar blockId={id} blockType={data.kind} disabled={!userPermissions.canEdit} />
169169
)}
170170

171-
{/* Header Section — only interactive area for dragging */}
171+
{/* Header Section */}
172172
<div
173173
onClick={() => setCurrentBlockId(id)}
174174
className='workflow-drag-handle flex cursor-grab items-center justify-between rounded-t-[8px] border-[var(--border)] border-b bg-[var(--surface-2)] py-[8px] pr-[12px] pl-[8px] [&:active]:cursor-grabbing'
@@ -198,14 +198,15 @@ export const SubflowNodeComponent = memo(({ data, id, selected }: NodeProps<Subf
198198
</div>
199199

200200
{/*
201-
* Subflow body background. Uses pointer-events: none so that edges rendered
202-
* inside the subflow remain clickable. The subflow node wrapper also has
203-
* pointer-events: none (set in workflow.tsx), so body-area clicks pass
204-
* through to the pane. Subflow selection is done via the header above.
201+
* Subflow body background. Captures clicks to select the subflow in the
202+
* panel editor, matching the header click behavior. Child nodes and edges
203+
* are rendered as sibling divs at the viewport level by ReactFlow (not as
204+
* DOM children), so enabling pointer events here doesn't block them.
205205
*/}
206206
<div
207-
className='absolute inset-0 top-[44px] rounded-b-[8px]'
208-
style={{ pointerEvents: 'none' }}
207+
className='workflow-drag-handle absolute inset-0 top-[44px] cursor-grab rounded-b-[8px] [&:active]:cursor-grabbing'
208+
style={{ pointerEvents: isPreview ? 'none' : 'auto' }}
209+
onClick={() => setCurrentBlockId(id)}
209210
/>
210211

211212
{!isPreview && (

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/utils/workflow-canvas-helpers.ts

Lines changed: 65 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -221,3 +221,68 @@ export function resolveParentChildSelectionConflicts(
221221

222222
return hasConflict ? resolved : nodes
223223
}
224+
225+
export function getNodeSelectionContextId(
226+
node: Pick<Node, 'id' | 'parentId'>,
227+
blocks: Record<string, { data?: { parentId?: string } }>
228+
): string | null {
229+
return node.parentId || blocks[node.id]?.data?.parentId || null
230+
}
231+
232+
export function getEdgeSelectionContextId(
233+
edge: Pick<Edge, 'source' | 'target'>,
234+
nodes: Array<Pick<Node, 'id' | 'parentId'>>,
235+
blocks: Record<string, { data?: { parentId?: string } }>
236+
): string | null {
237+
const sourceNode = nodes.find((node) => node.id === edge.source)
238+
const targetNode = nodes.find((node) => node.id === edge.target)
239+
const sourceContextId = sourceNode ? getNodeSelectionContextId(sourceNode, blocks) : null
240+
const targetContextId = targetNode ? getNodeSelectionContextId(targetNode, blocks) : null
241+
if (sourceContextId) return sourceContextId
242+
if (targetContextId) return targetContextId
243+
return null
244+
}
245+
246+
export function resolveSelectionContextConflicts(
247+
nodes: Node[],
248+
blocks: Record<string, { data?: { parentId?: string } }>,
249+
preferredContextId?: string | null
250+
): Node[] {
251+
const selectedNodes = nodes.filter((node) => node.selected)
252+
if (selectedNodes.length <= 1) return nodes
253+
254+
const allowedContextId =
255+
preferredContextId !== undefined
256+
? preferredContextId
257+
: getNodeSelectionContextId(selectedNodes[0], blocks)
258+
let hasConflict = false
259+
260+
const resolved = nodes.map((node) => {
261+
if (!node.selected) return node
262+
const contextId = getNodeSelectionContextId(node, blocks)
263+
if (contextId !== allowedContextId) {
264+
hasConflict = true
265+
return { ...node, selected: false }
266+
}
267+
return node
268+
})
269+
270+
return hasConflict ? resolved : nodes
271+
}
272+
273+
export function resolveSelectionConflicts(
274+
nodes: Node[],
275+
blocks: Record<string, { data?: { parentId?: string } }>,
276+
preferredNodeId?: string
277+
): Node[] {
278+
const afterParentChild = resolveParentChildSelectionConflicts(nodes, blocks)
279+
280+
const preferredContextId =
281+
preferredNodeId !== undefined
282+
? afterParentChild.find((n) => n.id === preferredNodeId && n.selected)
283+
? getNodeSelectionContextId(afterParentChild.find((n) => n.id === preferredNodeId)!, blocks)
284+
: undefined
285+
: undefined
286+
287+
return resolveSelectionContextConflicts(afterParentChild, blocks, preferredContextId)
288+
}

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

Lines changed: 84 additions & 43 deletions
Original file line numberDiff line numberDiff line change
@@ -59,11 +59,13 @@ import {
5959
filterProtectedBlocks,
6060
getClampedPositionForNode,
6161
getDescendantBlockIds,
62+
getEdgeSelectionContextId,
63+
getNodeSelectionContextId,
6264
getWorkflowLockToggleIds,
6365
isBlockProtected,
6466
isEdgeProtected,
6567
isInEditableElement,
66-
resolveParentChildSelectionConflicts,
68+
resolveSelectionConflicts,
6769
validateTriggerPaste,
6870
} from '@/app/workspace/[workspaceId]/w/[workflowId]/utils'
6971
import { useSocket } from '@/app/workspace/providers/socket-provider'
@@ -168,16 +170,17 @@ function mapEdgesByNode(edges: Edge[], nodeIds: Set<string>): Map<string, Edge[]
168170

169171
/**
170172
* Syncs the panel editor with the current selection state.
171-
* Shows block details when exactly one block is selected, clears otherwise.
173+
* Shows the last selected block in the panel. Clears when nothing is selected.
172174
*/
173175
function syncPanelWithSelection(selectedIds: string[]) {
174176
const { currentBlockId, clearCurrentBlock, setCurrentBlockId } = usePanelEditorStore.getState()
175-
if (selectedIds.length === 1 && selectedIds[0] !== currentBlockId) {
176-
setCurrentBlockId(selectedIds[0])
177-
} else if (selectedIds.length === 0 && currentBlockId) {
178-
clearCurrentBlock()
179-
} else if (selectedIds.length > 1 && currentBlockId) {
180-
clearCurrentBlock()
177+
if (selectedIds.length === 0) {
178+
if (currentBlockId) clearCurrentBlock()
179+
} else {
180+
const lastSelectedId = selectedIds[selectedIds.length - 1]
181+
if (lastSelectedId !== currentBlockId) {
182+
setCurrentBlockId(lastSelectedId)
183+
}
181184
}
182185
}
183186

@@ -246,7 +249,6 @@ const WorkflowContent = React.memo(
246249
const [selectedEdges, setSelectedEdges] = useState<SelectedEdgesMap>(new Map())
247250
const [isErrorConnectionDrag, setIsErrorConnectionDrag] = useState(false)
248251
const canvasContainerRef = useRef<HTMLDivElement>(null)
249-
const selectedIdsRef = useRef<string[] | null>(null)
250252
const embeddedFitFrameRef = useRef<number | null>(null)
251253
const hasCompletedInitialEmbeddedFitRef = useRef(false)
252254
const canvasMode = useCanvasModeStore((state) => state.mode)
@@ -2477,6 +2479,16 @@ const WorkflowContent = React.memo(
24772479
// Local state for nodes - allows smooth drag without store updates on every frame
24782480
const [displayNodes, setDisplayNodes] = useState<Node[]>([])
24792481

2482+
const selectedNodeIds = useMemo(
2483+
() => displayNodes.filter((node) => node.selected).map((node) => node.id),
2484+
[displayNodes]
2485+
)
2486+
const selectedNodeIdsKey = selectedNodeIds.join(',')
2487+
2488+
useEffect(() => {
2489+
syncPanelWithSelection(selectedNodeIds)
2490+
}, [selectedNodeIdsKey])
2491+
24802492
useEffect(() => {
24812493
// Check for pending selection (from paste/duplicate), otherwise preserve existing selection
24822494
if (pendingSelection && pendingSelection.length > 0) {
@@ -2488,10 +2500,8 @@ const WorkflowContent = React.memo(
24882500
...node,
24892501
selected: pendingSet.has(node.id),
24902502
}))
2491-
const resolved = resolveParentChildSelectionConflicts(withSelection, blocks)
2503+
const resolved = resolveSelectionConflicts(withSelection, blocks)
24922504
setDisplayNodes(resolved)
2493-
const selectedIds = resolved.filter((node) => node.selected).map((node) => node.id)
2494-
syncPanelWithSelection(selectedIds)
24952505
return
24962506
}
24972507

@@ -2709,19 +2719,20 @@ const WorkflowContent = React.memo(
27092719
/** Handles node changes - applies changes and resolves parent-child selection conflicts. */
27102720
const onNodesChange = useCallback(
27112721
(changes: NodeChange[]) => {
2712-
selectedIdsRef.current = null
2713-
setDisplayNodes((nds) => {
2714-
const updated = applyNodeChanges(changes, nds)
2715-
const hasSelectionChange = changes.some((c) => c.type === 'select')
2722+
const hasSelectionChange = changes.some((c) => c.type === 'select')
2723+
setDisplayNodes((currentNodes) => {
2724+
const updated = applyNodeChanges(changes, currentNodes)
27162725
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
2726+
2727+
const preferredNodeId = [...changes]
2728+
.reverse()
2729+
.find(
2730+
(change): change is NodeChange & { id: string; selected: boolean } =>
2731+
change.type === 'select' && 'selected' in change && change.selected === true
2732+
)?.id
2733+
2734+
return resolveSelectionConflicts(updated, blocks, preferredNodeId)
27202735
})
2721-
const selectedIds = selectedIdsRef.current as string[] | null
2722-
if (selectedIds !== null) {
2723-
syncPanelWithSelection(selectedIds)
2724-
}
27252736

27262737
// Handle position changes (e.g., from keyboard arrow key movement)
27272738
// Update container dimensions when child nodes are moved and persist to backend
@@ -3160,7 +3171,10 @@ const WorkflowContent = React.memo(
31603171
parentId: currentParentId,
31613172
})
31623173

3163-
// Capture all selected nodes' positions for multi-node undo/redo
3174+
// Capture all selected nodes' positions for multi-node undo/redo.
3175+
// Also include the dragged node itself — during shift+click+drag, ReactFlow
3176+
// may have toggled (deselected) the node before drag starts, so it might not
3177+
// appear in the selected set yet.
31643178
const allNodes = getNodes()
31653179
const selectedNodes = allNodes.filter((n) => n.selected)
31663180
multiNodeDragStartRef.current.clear()
@@ -3174,6 +3188,33 @@ const WorkflowContent = React.memo(
31743188
})
31753189
}
31763190
})
3191+
if (!multiNodeDragStartRef.current.has(node.id)) {
3192+
multiNodeDragStartRef.current.set(node.id, {
3193+
x: node.position.x,
3194+
y: node.position.y,
3195+
parentId: currentParentId ?? undefined,
3196+
})
3197+
}
3198+
3199+
// When shift+clicking an already-selected node, ReactFlow toggles (deselects)
3200+
// it via onNodesChange before drag starts. Re-select the dragged node so all
3201+
// previously selected nodes move together as a group — but only if the
3202+
// deselection wasn't from a parent-child conflict (e.g. dragging a child
3203+
// when its parent subflow is selected).
3204+
const draggedNodeInSelected = allNodes.find((n) => n.id === node.id)
3205+
if (draggedNodeInSelected && !draggedNodeInSelected.selected && selectedNodes.length > 0) {
3206+
const draggedParentId = blocks[node.id]?.data?.parentId
3207+
const parentIsSelected =
3208+
draggedParentId && selectedNodes.some((n) => n.id === draggedParentId)
3209+
const contextMismatch =
3210+
getNodeSelectionContextId(draggedNodeInSelected, blocks) !==
3211+
getNodeSelectionContextId(selectedNodes[0], blocks)
3212+
if (!parentIsSelected && !contextMismatch) {
3213+
setDisplayNodes((currentNodes) =>
3214+
currentNodes.map((n) => (n.id === node.id ? { ...n, selected: true } : n))
3215+
)
3216+
}
3217+
}
31773218
},
31783219
[blocks, setDragStartPosition, getNodes, setPotentialParentId]
31793220
)
@@ -3453,7 +3494,7 @@ const WorkflowContent = React.memo(
34533494
})
34543495

34553496
// Apply visual deselection of children
3456-
setDisplayNodes((allNodes) => resolveParentChildSelectionConflicts(allNodes, blocks))
3497+
setDisplayNodes((allNodes) => resolveSelectionConflicts(allNodes, blocks))
34573498
},
34583499
[blocks]
34593500
)
@@ -3604,36 +3645,36 @@ const WorkflowContent = React.memo(
36043645

36053646
/**
36063647
* Handles node click to select the node in ReactFlow.
3607-
* Parent-child conflict resolution happens automatically in onNodesChange.
3648+
* Uses the controlled display node state so parent-child conflicts are resolved
3649+
* consistently for click, shift-click, and marquee selection.
36083650
*/
36093651
const handleNodeClick = useCallback(
36103652
(event: React.MouseEvent, node: Node) => {
36113653
const isMultiSelect = event.shiftKey || event.metaKey || event.ctrlKey
3612-
setNodes((nodes) =>
3613-
nodes.map((n) => ({
3614-
...n,
3615-
selected: isMultiSelect ? (n.id === node.id ? true : n.selected) : n.id === node.id,
3654+
setDisplayNodes((currentNodes) => {
3655+
const updated = currentNodes.map((currentNode) => ({
3656+
...currentNode,
3657+
selected: isMultiSelect
3658+
? currentNode.id === node.id
3659+
? true
3660+
: currentNode.selected
3661+
: currentNode.id === node.id,
36163662
}))
3617-
)
3663+
return resolveSelectionConflicts(updated, blocks, isMultiSelect ? node.id : undefined)
3664+
})
36183665
},
3619-
[setNodes]
3666+
[blocks]
36203667
)
36213668

36223669
/** Handles edge selection with container context tracking and Shift-click multi-selection. */
36233670
const onEdgeClick = useCallback(
36243671
(event: React.MouseEvent, edge: any) => {
36253672
event.stopPropagation() // Prevent bubbling
36263673

3627-
// Determine if edge is inside a loop by checking its source/target nodes
3628-
const sourceNode = getNodes().find((n) => n.id === edge.source)
3629-
const targetNode = getNodes().find((n) => n.id === edge.target)
3630-
3631-
// An edge is inside a loop if either source or target has a parent
3632-
// If source and target have different parents, prioritize source's parent
3633-
const parentLoopId = sourceNode?.parentId || targetNode?.parentId
3634-
3635-
// Create a unique identifier that combines edge ID and parent context
3636-
const contextId = `${edge.id}${parentLoopId ? `-${parentLoopId}` : ''}`
3674+
const contextId = `${edge.id}${(() => {
3675+
const selectionContextId = getEdgeSelectionContextId(edge, getNodes(), blocks)
3676+
return selectionContextId ? `-${selectionContextId}` : ''
3677+
})()}`
36373678

36383679
if (event.shiftKey) {
36393680
// Shift-click: toggle edge in selection
@@ -3651,7 +3692,7 @@ const WorkflowContent = React.memo(
36513692
setSelectedEdges(new Map([[contextId, edge.id]]))
36523693
}
36533694
},
3654-
[getNodes]
3695+
[blocks, getNodes]
36553696
)
36563697

36573698
/** Stable delete handler to avoid creating new function references per edge. */

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', () => {

0 commit comments

Comments
 (0)