Skip to content

Commit d81eca7

Browse files
committed
improvement(workflow): remove useEffect anti-patterns
1 parent be7f3db commit d81eca7

File tree

2 files changed

+40
-42
lines changed

2 files changed

+40
-42
lines changed

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

Lines changed: 12 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -330,9 +330,8 @@ const WorkflowContent = React.memo(() => {
330330

331331
const isAutoConnectEnabled = useAutoConnect()
332332
const autoConnectRef = useRef(isAutoConnectEnabled)
333-
useEffect(() => {
334-
autoConnectRef.current = isAutoConnectEnabled
335-
}, [isAutoConnectEnabled])
333+
// Keep ref in sync with latest value for use in callbacks (no effect needed)
334+
autoConnectRef.current = isAutoConnectEnabled
336335

337336
// Panel open states for context menu
338337
const isVariablesOpen = useVariablesStore((state) => state.isOpen)
@@ -473,11 +472,14 @@ const WorkflowContent = React.memo(() => {
473472
)
474473

475474
/** Re-applies diff markers when blocks change after socket rehydration. */
476-
const blocksRef = useRef(blocks)
475+
const diffBlocksRef = useRef(blocks)
477476
useEffect(() => {
478-
if (!isWorkflowReady) return
479-
if (hasActiveDiff && isDiffReady && blocks !== blocksRef.current) {
480-
blocksRef.current = blocks
477+
// Track if blocks actually changed (vs other deps triggering this effect)
478+
const blocksChanged = blocks !== diffBlocksRef.current
479+
diffBlocksRef.current = blocks
480+
481+
if (!isWorkflowReady || !blocksChanged) return
482+
if (hasActiveDiff && isDiffReady) {
481483
setTimeout(() => reapplyDiffMarkers(), 0)
482484
}
483485
}, [blocks, hasActiveDiff, isDiffReady, reapplyDiffMarkers, isWorkflowReady])
@@ -2050,6 +2052,8 @@ const WorkflowContent = React.memo(() => {
20502052
// Local state for nodes - allows smooth drag without store updates on every frame
20512053
const [displayNodes, setDisplayNodes] = useState<Node[]>([])
20522054

2055+
// Sync derivedNodes to displayNodes while preserving selection state
2056+
// This effect handles both normal sync and pending selection from paste/duplicate
20532057
useEffect(() => {
20542058
// Check for pending selection (from paste/duplicate), otherwise preserve existing selection
20552059
if (pendingSelection && pendingSelection.length > 0) {
@@ -2076,7 +2080,7 @@ const WorkflowContent = React.memo(() => {
20762080
selected: selectedIds.has(node.id),
20772081
}))
20782082
})
2079-
}, [derivedNodes, blocks, pendingSelection, clearPendingSelection])
2083+
}, [derivedNodes, blocks, pendingSelection, clearPendingSelection, syncPanelWithSelection])
20802084

20812085
// Phase 2: When displayNodes updates, check if pending zoom blocks are ready
20822086
// (Phase 1 is located earlier in the file where pendingZoomBlockIdsRef is defined)
@@ -2270,40 +2274,6 @@ const WorkflowContent = React.memo(() => {
22702274
resizeLoopNodesWrapper()
22712275
}, [derivedNodes, resizeLoopNodesWrapper, isWorkflowReady])
22722276

2273-
/** Cleans up orphaned nodes with invalid parent references after deletion. */
2274-
useEffect(() => {
2275-
if (!isWorkflowReady) return
2276-
2277-
// Create a mapping of node IDs to check for missing parent references
2278-
const nodeIds = new Set(Object.keys(blocks))
2279-
2280-
// Check for nodes with invalid parent references and collect updates
2281-
const orphanedUpdates: Array<{
2282-
id: string
2283-
position: { x: number; y: number }
2284-
parentId: string
2285-
}> = []
2286-
Object.entries(blocks).forEach(([id, block]) => {
2287-
const parentId = block.data?.parentId
2288-
2289-
// If block has a parent reference but parent no longer exists
2290-
if (parentId && !nodeIds.has(parentId)) {
2291-
logger.warn('Found orphaned node with invalid parent reference', {
2292-
nodeId: id,
2293-
missingParentId: parentId,
2294-
})
2295-
2296-
const absolutePosition = getNodeAbsolutePosition(id)
2297-
orphanedUpdates.push({ id, position: absolutePosition, parentId: '' })
2298-
}
2299-
})
2300-
2301-
// Batch update all orphaned nodes at once
2302-
if (orphanedUpdates.length > 0) {
2303-
batchUpdateBlocksWithParent(orphanedUpdates)
2304-
}
2305-
}, [blocks, batchUpdateBlocksWithParent, getNodeAbsolutePosition, isWorkflowReady])
2306-
23072277
/** Handles edge removal changes. */
23082278
const onEdgesChange = useCallback(
23092279
(changes: any) => {

apps/sim/stores/workflows/workflow/store.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -448,6 +448,34 @@ export const useWorkflowStore = create<WorkflowStore>()(
448448
delete newBlocks[blockId]
449449
})
450450

451+
// Clean up orphaned nodes - blocks whose parent was removed but weren't descendants
452+
// This can happen in edge cases (e.g., data inconsistency, external modifications)
453+
const remainingBlockIds = new Set(Object.keys(newBlocks))
454+
Object.entries(newBlocks).forEach(([blockId, block]) => {
455+
const parentId = block.data?.parentId
456+
if (parentId && !remainingBlockIds.has(parentId)) {
457+
// Parent was removed - convert to absolute position and clear parentId
458+
// Calculate absolute position by traversing up the (now-deleted) parent chain
459+
let absoluteX = block.position.x
460+
let absoluteY = block.position.y
461+
462+
// Try to get parent's position from original blocks before deletion
463+
let currentParentId: string | undefined = parentId
464+
while (currentParentId && currentBlocks[currentParentId]) {
465+
const parent = currentBlocks[currentParentId]
466+
absoluteX += parent.position.x
467+
absoluteY += parent.position.y
468+
currentParentId = parent.data?.parentId
469+
}
470+
471+
newBlocks[blockId] = {
472+
...block,
473+
position: { x: absoluteX, y: absoluteY },
474+
data: { ...block.data, parentId: undefined },
475+
}
476+
}
477+
})
478+
451479
const activeWorkflowId = useWorkflowRegistry.getState().activeWorkflowId
452480
if (activeWorkflowId) {
453481
const subBlockStore = useSubBlockStore.getState()

0 commit comments

Comments
 (0)