fix(autolayout): targetted autolayout heuristic restored (#3536)

* fix(autolayout): targetted autolayout heuristic restored

* fix autolayout boundary cases

* more fixes

* address comments

* on conflict updates

* address more comments

* fix relative position scope

* fix tye omission

* address bugbot comment

Author: icecrasher321

apps/sim/lib/copilot/tools/server/workflow/edit-workflow/engine.ts

@@ -238,6 +238,12 @@ export function applyOperationsToWorkflowState(
       totalEdges: (modifiedState as any).edges?.length,
     })
   }
+  // Remove edges that cross scope boundaries. This runs after all operations
+  // and deferred connections are applied so that every block has its final
+  // parentId. Running it per-operation would incorrectly drop edges between
+  // blocks that are both being moved into the same subflow in one batch.
+  removeInvalidScopeEdges(modifiedState)
+
   // Regenerate loops and parallels after modifications
 
   ;(modifiedState as any).loops = generateLoopBlocks((modifiedState as any).blocks)
@@ -272,3 +278,46 @@ export function applyOperationsToWorkflowState(
 
   return { state: modifiedState, validationErrors, skippedItems }
 }
+
+/**
+ * Removes edges that cross scope boundaries after all operations are applied.
+ * An edge is invalid if:
+ * - Either endpoint no longer exists (dangling reference)
+ * - The source and target are in incompatible scopes
+ * - A child block connects to its own parent container (non-handle edge)
+ *
+ * Valid scope relationships:
+ * - Same scope: both blocks share the same parentId
+ * - Container→child: source is the parent container of the target (start handles)
+ * - Child→container: target is the parent container of the source (end handles)
+ */
+function removeInvalidScopeEdges(modifiedState: any): void {
+  const blocks = modifiedState.blocks || {}
+  const prevCount = modifiedState.edges?.length ?? 0
+
+  modifiedState.edges = (modifiedState.edges || []).filter((edge: any) => {
+    const sourceBlock = blocks[edge.source]
+    const targetBlock = blocks[edge.target]
+    if (!sourceBlock || !targetBlock) return false
+
+    const sourceParent = sourceBlock.data?.parentId ?? null
+    const targetParent = targetBlock.data?.parentId ?? null
+
+    // Same scope — always valid
+    if (sourceParent === targetParent) return true
+
+    // Container→child (e.g., loop-start-source → first child in loop)
+    if (targetParent === edge.source) return true
+
+    // Child→container (e.g., last child → loop-end via loop block edges)
+    if (sourceParent === edge.target) return true
+
+    // Different scopes with no parent-child relationship — invalid
+    return false
+  })
+
+  const removed = prevCount - (modifiedState.edges?.length ?? 0)
+  if (removed > 0) {
+    logger.info('Removed invalid scope-crossing edges', { removed })
+  }
+}

apps/sim/lib/copilot/tools/server/workflow/edit-workflow/index.ts

@@ -3,7 +3,11 @@ import { workflow as workflowTable } from '@sim/db/schema'
 import { createLogger } from '@sim/logger'
 import { eq } from 'drizzle-orm'
 import type { BaseServerTool } from '@/lib/copilot/tools/server/base-tool'
-import { applyAutoLayout } from '@/lib/workflows/autolayout'
+import { applyTargetedLayout } from '@/lib/workflows/autolayout'
+import {
+  DEFAULT_HORIZONTAL_SPACING,
+  DEFAULT_VERTICAL_SPACING,
+} from '@/lib/workflows/autolayout/constants'
 import { extractAndPersistCustomTools } from '@/lib/workflows/persistence/custom-tools-persistence'
 import {
   loadWorkflowFromNormalizedTables,
@@ -234,21 +238,38 @@ export const editWorkflowServerTool: BaseServerTool<EditWorkflowParams, unknown>
     // Persist the workflow state to the database
     const finalWorkflowState = validation.sanitizedState || modifiedWorkflowState
 
-    // Apply autolayout to position blocks properly
-    const layoutResult = applyAutoLayout(finalWorkflowState.blocks, finalWorkflowState.edges, {
-      horizontalSpacing: 250,
-      verticalSpacing: 100,
-      padding: { x: 100, y: 100 },
+    // Identify blocks that need layout by comparing against the pre-operation
+    // state. New blocks and blocks inserted into subflows (position reset to
+    // 0,0) need repositioning. Extracted blocks are excluded — their handler
+    // already computed valid absolute positions from the container offset.
+    const preOperationBlockIds = new Set(Object.keys(workflowState.blocks || {}))
+    const blocksNeedingLayout = Object.keys(finalWorkflowState.blocks).filter((id) => {
+      if (!preOperationBlockIds.has(id)) return true
+      const prevParent = workflowState.blocks[id]?.data?.parentId ?? null
+      const currParent = finalWorkflowState.blocks[id]?.data?.parentId ?? null
+      if (prevParent === currParent) return false
+      // Parent changed — only needs layout if position was reset to (0,0)
+      // by insert_into_subflow. extract_from_subflow computes absolute
+      // positions directly, so those blocks don't need repositioning.
+      const pos = finalWorkflowState.blocks[id]?.position
+      return pos?.x === 0 && pos?.y === 0
     })
 
-    const layoutedBlocks =
-      layoutResult.success && layoutResult.blocks ? layoutResult.blocks : finalWorkflowState.blocks
+    let layoutedBlocks = finalWorkflowState.blocks
 
-    if (!layoutResult.success) {
-      logger.warn('Autolayout failed, using default positions', {
-        workflowId,
-        error: layoutResult.error,
-      })
+    if (blocksNeedingLayout.length > 0) {
+      try {
+        layoutedBlocks = applyTargetedLayout(finalWorkflowState.blocks, finalWorkflowState.edges, {
+          changedBlockIds: blocksNeedingLayout,
+          horizontalSpacing: DEFAULT_HORIZONTAL_SPACING,
+          verticalSpacing: DEFAULT_VERTICAL_SPACING,
+        })
+      } catch (error) {
+        logger.warn('Targeted autolayout failed, using default positions', {
+          workflowId,
+          error: error instanceof Error ? error.message : String(error),
+        })
+      }
     }
 
     const workflowStateForDb = {

apps/sim/lib/copilot/tools/server/workflow/edit-workflow/operations.ts

@@ -798,12 +798,16 @@ export function handleInsertIntoSubflowOperation(
       return
     }
 
-    // Moving existing block into subflow - just update parent
+    // Moving existing block into subflow — update parent and reset position.
+    // Position must be reset because React Flow uses coordinates relative to
+    // the parent container; keeping the old absolute position would place the
+    // block far outside the container's bounds.
     existingBlock.data = {
       ...existingBlock.data,
       parentId: subflowId,
       extent: 'parent' as const,
     }
+    existingBlock.position = { x: 0, y: 0 }
 
     // Update inputs if provided (with validation)
     if (params.inputs) {
@@ -981,12 +985,25 @@ export function handleExtractFromSubflowOperation(
     })
   }
 
-  // Remove parent relationship
+  // Convert from relative (to container) to absolute position so the block
+  // appears at roughly the same visual location after extraction. This avoids
+  // needing targeted layout to reposition it — extracted blocks often lose
+  // their edges to siblings still in the container, making them disconnected
+  // and causing layout to stack them at layer 0.
+  const container = modifiedState.blocks[subflowId]
+  if (container?.position && block.position) {
+    block.position = {
+      x: (container.position.x ?? 0) + (block.position.x ?? 0),
+      y: (container.position.y ?? 0) + (block.position.y ?? 0),
+    }
+  } else {
+    // Fallback to (0,0) which signals to blocksNeedingLayout in index.ts
+    // that this block requires targeted layout repositioning.
+    block.position = { x: 0, y: 0 }
+  }
+
   if (block.data) {
     block.data.parentId = undefined
     block.data.extent = undefined
   }
-
-  // Note: We keep the block and its edges, just remove parent relationship
-  // The block becomes a root-level block
 }

apps/sim/lib/workflows/autolayout/constants.ts

@@ -73,6 +73,17 @@ export const AUTO_LAYOUT_EXCLUDED_TYPES = new Set(['note'])
  */
 export const CONTAINER_BLOCK_TYPES = new Set(['loop', 'parallel'])
 
+/**
+ * Estimated height per subblock when no measured height is available.
+ * Used as a heuristic for new blocks that haven't been rendered yet.
+ */
+export const ESTIMATED_SUBBLOCK_HEIGHT = 45
+
+/**
+ * Bottom padding added to estimated block height
+ */
+export const ESTIMATED_BLOCK_BOTTOM_PADDING = 20
+
 /**
  * Default layout options
  */

apps/sim/lib/workflows/autolayout/targeted.ts

@@ -10,7 +10,6 @@ import {
   filterLayoutEligibleBlockIds,
   getBlockMetrics,
   getBlocksByParent,
-  isContainerType,
   prepareContainerDimensions,
   shouldSkipAutoLayout,
   snapPositionToGrid,
@@ -90,7 +89,39 @@ export function applyTargetedLayout(
 }
 
 /**
- * Layouts a group of blocks (either root level or within a container)
+ * Selects the best anchor block for offset computation.
+ * Prefers an unchanged block that is a direct edge-neighbor of a block
+ * that needs layout, so the offset aligns new blocks relative to their
+ * actual graph neighbors rather than an arbitrary/outlier block.
+ */
+function selectBestAnchor(
+  eligibleIds: string[],
+  needsLayoutSet: Set<string>,
+  edges: Edge[],
+  layoutPositions: Map<string, { x: number; y: number }>
+): string | undefined {
+  const candidates = eligibleIds.filter((id) => !needsLayoutSet.has(id) && layoutPositions.has(id))
+  if (candidates.length === 0) return undefined
+  if (candidates.length === 1) return candidates[0]
+
+  const candidateSet = new Set(candidates)
+
+  for (const edge of edges) {
+    if (needsLayoutSet.has(edge.source) && candidateSet.has(edge.target)) {
+      return edge.target
+    }
+    if (needsLayoutSet.has(edge.target) && candidateSet.has(edge.source)) {
+      return edge.source
+    }
+  }
+
+  return candidates[0]
+}
+
+/**
+ * Layouts a group of blocks (either root level or within a container).
+ * Only repositions blocks in `changedSet` or those with invalid positions;
+ * all other blocks act as anchors.
  */
 function layoutGroup(
   parentId: string | null,
@@ -119,24 +150,20 @@ function layoutGroup(
   const requestedLayout = layoutEligibleChildIds.filter((id) => {
     const block = blocks[id]
     if (!block) return false
-    if (isContainerType(block.type)) {
-      return changedSet.has(id) && isDefaultPosition(block)
-    }
     return changedSet.has(id)
   })
-  const missingPositions = layoutEligibleChildIds.filter((id) => {
+  const invalidPositions = layoutEligibleChildIds.filter((id) => {
     const block = blocks[id]
     if (!block) return false
-    return !hasPosition(block) || isDefaultPosition(block)
+    return !hasPosition(block)
   })
-  const needsLayoutSet = new Set([...requestedLayout, ...missingPositions])
+  const needsLayoutSet = new Set([...requestedLayout, ...invalidPositions])
   const needsLayout = Array.from(needsLayoutSet)
 
-  if (parentBlock) {
-    updateContainerDimensions(parentBlock, childIds, blocks)
-  }
-
   if (needsLayout.length === 0) {
+    if (parentBlock) {
+      updateContainerDimensions(parentBlock, childIds, blocks)
+    }
     return
   }
 
@@ -168,9 +195,7 @@ function layoutGroup(
   let offsetX = 0
   let offsetY = 0
 
-  const anchorId = layoutEligibleChildIds.find(
-    (id) => !needsLayout.includes(id) && layoutPositions.has(id)
-  )
+  const anchorId = selectBestAnchor(layoutEligibleChildIds, needsLayoutSet, edges, layoutPositions)
 
   if (anchorId) {
     const oldPos = oldPositions.get(anchorId)
@@ -187,6 +212,10 @@ function layoutGroup(
     if (!block || !newPos) continue
     block.position = snapPositionToGrid({ x: newPos.x + offsetX, y: newPos.y + offsetY }, gridSize)
   }
+
+  if (parentBlock) {
+    updateContainerDimensions(parentBlock, childIds, blocks)
+  }
 }
 
 /**
@@ -304,20 +333,11 @@ function updateContainerDimensions(
 }
 
 /**
- * Checks if a block has a valid position
+ * Checks if a block has a valid, finite position.
+ * Returns false for missing, undefined, NaN, or Infinity coordinates.
  */
 function hasPosition(block: BlockState): boolean {
   if (!block.position) return false
   const { x, y } = block.position
   return Number.isFinite(x) && Number.isFinite(y)
 }
-
-/**
- * Checks if a block is at the default/uninitialized position (0, 0).
- * New blocks typically start at this position before being laid out.
- */
-function isDefaultPosition(block: BlockState): boolean {
-  if (!block.position) return true
-  const { x, y } = block.position
-  return x === 0 && y === 0
-}

apps/sim/lib/workflows/autolayout/utils.ts

@@ -4,6 +4,8 @@ import {
   CONTAINER_PADDING,
   CONTAINER_PADDING_X,
   CONTAINER_PADDING_Y,
+  ESTIMATED_BLOCK_BOTTOM_PADDING,
+  ESTIMATED_SUBBLOCK_HEIGHT,
   ROOT_PADDING_X,
   ROOT_PADDING_Y,
 } from '@/lib/workflows/autolayout/constants'
@@ -131,16 +133,36 @@ function getContainerMetrics(block: BlockState): BlockMetrics {
 }
 
 /**
- * Gets metrics for a regular (non-container) block
+ * Estimates block height from subblock count when no measurement is available.
+ * Provides a reasonable approximation to prevent overlaps in layout before
+ * the block has been rendered and measured by the browser.
+ */
+function estimateBlockHeight(block: BlockState): number {
+  const subBlockCount = Object.keys(block.subBlocks || {}).length
+  if (subBlockCount === 0) return BLOCK_DIMENSIONS.MIN_HEIGHT
+
+  return Math.max(
+    BLOCK_DIMENSIONS.HEADER_HEIGHT +
+      subBlockCount * ESTIMATED_SUBBLOCK_HEIGHT +
+      ESTIMATED_BLOCK_BOTTOM_PADDING,
+    BLOCK_DIMENSIONS.MIN_HEIGHT
+  )
+}
+
+/**
+ * Gets metrics for a regular (non-container) block.
+ * Falls back to subblock-based height estimation when no measurement exists,
+ * which prevents overlaps for newly added blocks that haven't been rendered.
  */
 function getRegularBlockMetrics(block: BlockState): BlockMetrics {
   const minWidth = BLOCK_DIMENSIONS.FIXED_WIDTH
   const minHeight = BLOCK_DIMENSIONS.MIN_HEIGHT
   const measuredH = block.layout?.measuredHeight ?? block.height
   const measuredW = block.layout?.measuredWidth
 
+  const hasMeasurement = typeof measuredH === 'number' && measuredH > 0
+  const height = hasMeasurement ? Math.max(measuredH, minHeight) : estimateBlockHeight(block)
   const width = Math.max(measuredW ?? minWidth, minWidth)
-  const height = Math.max(measuredH ?? minHeight, minHeight)
 
   return {
     width,

apps/sim/lib/workflows/diff/diff-engine.test.ts

@@ -26,7 +26,6 @@ vi.mock('@/lib/workflows/sanitization/key-validation', () => ({
 vi.mock('@/lib/workflows/autolayout', () => ({
   transferBlockHeights: vi.fn(),
   applyTargetedLayout: (blocks: Record<string, BlockState>) => blocks,
-  applyAutoLayout: () => ({ success: true, blocks: {} }),
 }))
 
 vi.mock('@/lib/workflows/autolayout/constants', () => ({