Skip to content

Commit fe4fd47

Browse files
committed
improvement(workflow): remove useEffect anti-patterns
1 parent 6b412c5 commit fe4fd47

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
@@ -307,9 +307,8 @@ const WorkflowContent = React.memo(() => {
307307

308308
const isAutoConnectEnabled = useAutoConnect()
309309
const autoConnectRef = useRef(isAutoConnectEnabled)
310-
useEffect(() => {
311-
autoConnectRef.current = isAutoConnectEnabled
312-
}, [isAutoConnectEnabled])
310+
// Keep ref in sync with latest value for use in callbacks (no effect needed)
311+
autoConnectRef.current = isAutoConnectEnabled
313312

314313
// Panel open states for context menu
315314
const isVariablesOpen = useVariablesStore((state) => state.isOpen)
@@ -448,11 +447,14 @@ const WorkflowContent = React.memo(() => {
448447
)
449448

450449
/** Re-applies diff markers when blocks change after socket rehydration. */
451-
const blocksRef = useRef(blocks)
450+
const diffBlocksRef = useRef(blocks)
452451
useEffect(() => {
453-
if (!isWorkflowReady) return
454-
if (hasActiveDiff && isDiffReady && blocks !== blocksRef.current) {
455-
blocksRef.current = blocks
452+
// Track if blocks actually changed (vs other deps triggering this effect)
453+
const blocksChanged = blocks !== diffBlocksRef.current
454+
diffBlocksRef.current = blocks
455+
456+
if (!isWorkflowReady || !blocksChanged) return
457+
if (hasActiveDiff && isDiffReady) {
456458
setTimeout(() => reapplyDiffMarkers(), 0)
457459
}
458460
}, [blocks, hasActiveDiff, isDiffReady, reapplyDiffMarkers, isWorkflowReady])
@@ -2160,6 +2162,8 @@ const WorkflowContent = React.memo(() => {
21602162
// Local state for nodes - allows smooth drag without store updates on every frame
21612163
const [displayNodes, setDisplayNodes] = useState<Node[]>([])
21622164

2165+
// Sync derivedNodes to displayNodes while preserving selection state
2166+
// This effect handles both normal sync and pending selection from paste/duplicate
21632167
useEffect(() => {
21642168
// Check for pending selection (from paste/duplicate), otherwise preserve existing selection
21652169
if (pendingSelection && pendingSelection.length > 0) {
@@ -2186,7 +2190,7 @@ const WorkflowContent = React.memo(() => {
21862190
selected: selectedIds.has(node.id),
21872191
}))
21882192
})
2189-
}, [derivedNodes, blocks, pendingSelection, clearPendingSelection])
2193+
}, [derivedNodes, blocks, pendingSelection, clearPendingSelection, syncPanelWithSelection])
21902194

21912195
// Phase 2: When displayNodes updates, check if pending zoom blocks are ready
21922196
// (Phase 1 is located earlier in the file where pendingZoomBlockIdsRef is defined)
@@ -2380,40 +2384,6 @@ const WorkflowContent = React.memo(() => {
23802384
resizeLoopNodesWrapper()
23812385
}, [derivedNodes, resizeLoopNodesWrapper, isWorkflowReady])
23822386

2383-
/** Cleans up orphaned nodes with invalid parent references after deletion. */
2384-
useEffect(() => {
2385-
if (!isWorkflowReady) return
2386-
2387-
// Create a mapping of node IDs to check for missing parent references
2388-
const nodeIds = new Set(Object.keys(blocks))
2389-
2390-
// Check for nodes with invalid parent references and collect updates
2391-
const orphanedUpdates: Array<{
2392-
id: string
2393-
position: { x: number; y: number }
2394-
parentId: string
2395-
}> = []
2396-
Object.entries(blocks).forEach(([id, block]) => {
2397-
const parentId = block.data?.parentId
2398-
2399-
// If block has a parent reference but parent no longer exists
2400-
if (parentId && !nodeIds.has(parentId)) {
2401-
logger.warn('Found orphaned node with invalid parent reference', {
2402-
nodeId: id,
2403-
missingParentId: parentId,
2404-
})
2405-
2406-
const absolutePosition = getNodeAbsolutePosition(id)
2407-
orphanedUpdates.push({ id, position: absolutePosition, parentId: '' })
2408-
}
2409-
})
2410-
2411-
// Batch update all orphaned nodes at once
2412-
if (orphanedUpdates.length > 0) {
2413-
batchUpdateBlocksWithParent(orphanedUpdates)
2414-
}
2415-
}, [blocks, batchUpdateBlocksWithParent, getNodeAbsolutePosition, isWorkflowReady])
2416-
24172387
/** Handles edge removal changes. */
24182388
const onEdgesChange = useCallback(
24192389
(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)