Skip to content

Commit c6bb7d4

Browse files
fix(autolayout): container overlap on insert and error-branch alignment
Lay out contained groups before their containers so a container carries the height it renders at before siblings are placed beside it. Targeted layout sized containers from a hypothetical child layout it never applied, so a container whose children stay frozen rendered taller than the space reserved for it and overlapped its neighbour. Also mark containers resized by a child edit so following blocks shift clear, and derive the error handle offset from the layout's own block height instead of the raw height field.
1 parent d71e6ba commit c6bb7d4

5 files changed

Lines changed: 245 additions & 44 deletions

File tree

apps/sim/lib/workflows/autolayout/core.test.ts

Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
/**
22
* @vitest-environment node
33
*/
4+
import { HANDLE_POSITIONS } from '@sim/workflow-renderer'
45
import { describe, expect, it, vi } from 'vitest'
56
import { layoutBlocksCore } from '@/lib/workflows/autolayout/core'
67
import type { Edge } from '@/lib/workflows/autolayout/types'
@@ -73,4 +74,35 @@ describe('layoutBlocksCore', () => {
7374
)
7475
}
7576
})
77+
78+
it('aligns an error branch to the source block bottom using layout metrics height', () => {
79+
// The source's height comes from layout.measuredHeight, not the raw height
80+
// field, so an error handle offset read off block.height would fall back to
81+
// MIN_HEIGHT and place the error branch near the block's top instead.
82+
const source = {
83+
...createBlock('source'),
84+
height: undefined,
85+
layout: { measuredWidth: 250, measuredHeight: 400 },
86+
} as BlockState
87+
88+
const blocks: Record<string, BlockState> = {
89+
source,
90+
ok: createBlock('ok'),
91+
failed: createBlock('failed'),
92+
}
93+
const edges: Edge[] = [
94+
{ id: 'e1', source: 'source', target: 'ok' },
95+
{ id: 'e2', source: 'source', target: 'failed', sourceHandle: 'error' },
96+
]
97+
98+
const { nodes } = layoutBlocksCore(blocks, edges, { isContainer: false })
99+
const sourceNode = nodes.get('source')!
100+
101+
// The error branch's target handle lines up with the source's error handle,
102+
// which sits ERROR_BOTTOM_OFFSET above the source's real bottom edge.
103+
const errorHandleY =
104+
sourceNode.position.y + sourceNode.metrics.height - HANDLE_POSITIONS.ERROR_BOTTOM_OFFSET
105+
expect(nodes.get('failed')!.position.y + HANDLE_POSITIONS.DEFAULT_Y_OFFSET).toBe(errorHandleY)
106+
expect(nodes.get('ok')!.position.y).toBeLessThan(nodes.get('failed')!.position.y)
107+
})
76108
})

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

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
import { createLogger } from '@sim/logger'
2-
import { BLOCK_DIMENSIONS, HANDLE_POSITIONS } from '@sim/workflow-renderer'
2+
import { HANDLE_POSITIONS } from '@sim/workflow-renderer'
33
import {
44
CONTAINER_LAYOUT_OPTIONS,
55
DEFAULT_LAYOUT_OPTIONS,
@@ -21,11 +21,18 @@ const SUBFLOW_START_HANDLES = new Set(['loop-start-source', 'parallel-start-sour
2121

2222
/**
2323
* Calculates the Y offset for a source handle based on block type and handle ID.
24+
*
25+
* The error handle sits relative to the block's bottom, so it must use the same
26+
* height the rest of the layout uses (`node.metrics.height`). Reading the raw
27+
* `block.height` field instead would mis-place the handle for any block sized
28+
* from `layout.measuredHeight` or from an estimate, pulling error branches far
29+
* above their real handle.
2430
*/
25-
function getSourceHandleYOffset(block: BlockState, sourceHandle?: string | null): number {
31+
function getSourceHandleYOffset(node: GraphNode, sourceHandle?: string | null): number {
32+
const block = node.block
33+
2634
if (sourceHandle === 'error') {
27-
const blockHeight = block.height || BLOCK_DIMENSIONS.MIN_HEIGHT
28-
return blockHeight - HANDLE_POSITIONS.ERROR_BOTTOM_OFFSET
35+
return node.metrics.height - HANDLE_POSITIONS.ERROR_BOTTOM_OFFSET
2936
}
3037

3138
if (sourceHandle && SUBFLOW_START_HANDLES.has(sourceHandle)) {
@@ -319,7 +326,7 @@ export function calculatePositions(
319326
for (const edge of incomingEdges) {
320327
const predecessor = allNodes.get(edge.source)
321328
if (predecessor) {
322-
const sourceHandleOffset = getSourceHandleYOffset(predecessor.block, edge.sourceHandle)
329+
const sourceHandleOffset = getSourceHandleYOffset(predecessor, edge.sourceHandle)
323330
const sourceHandleY = predecessor.position.y + sourceHandleOffset
324331

325332
if (sourceHandleY > bestSourceHandleY) {

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

Lines changed: 113 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -345,4 +345,117 @@ describe('applyTargetedLayout', () => {
345345
result.existing.position.y + existingMetrics.height
346346
)
347347
})
348+
349+
it('places a new sibling container below a frozen container at its rendered height', () => {
350+
// The frozen container's children sit further apart than a fresh layout would
351+
// place them (a stale layout from when those blocks measured taller). Its
352+
// rendered height therefore exceeds the height a hypothetical re-layout
353+
// predicts, and the new sibling must clear the rendered one.
354+
const spacedChild = (id: string, y: number, parentId: string) =>
355+
createBlock(id, {
356+
position: { x: 150, y },
357+
data: { parentId, extent: 'parent' },
358+
layout: { measuredWidth: 250, measuredHeight: 300 },
359+
height: 300,
360+
})
361+
362+
const blocks = {
363+
start: createBlock('start', {
364+
type: 'start_trigger',
365+
position: { x: 0, y: 0 },
366+
layout: { measuredWidth: 250, measuredHeight: 100 },
367+
height: 100,
368+
}),
369+
frozenLoop: createBlock('frozenLoop', {
370+
type: 'loop',
371+
position: { x: 400, y: 0 },
372+
data: { width: 900, height: 1200 },
373+
layout: { measuredWidth: 900, measuredHeight: 1200 },
374+
}),
375+
frozenTop: spacedChild('frozenTop', 100, 'frozenLoop'),
376+
frozenBottom: spacedChild('frozenBottom', 750, 'frozenLoop'),
377+
newLoop: createBlock('newLoop', {
378+
type: 'loop',
379+
position: { x: 0, y: 0 },
380+
data: { width: 500, height: 300 },
381+
}),
382+
newChild: createBlock('newChild', {
383+
position: { x: 0, y: 0 },
384+
data: { parentId: 'newLoop', extent: 'parent' },
385+
}),
386+
}
387+
388+
const edges: Edge[] = [
389+
{ id: 'e1', source: 'start', target: 'frozenLoop' },
390+
{ id: 'e2', source: 'start', target: 'newLoop' },
391+
{ id: 'e3', source: 'frozenTop', target: 'frozenBottom' },
392+
]
393+
394+
const result = applyTargetedLayout(blocks, edges, {
395+
changedBlockIds: ['newLoop', 'newChild'],
396+
})
397+
398+
// Frozen children must not move, so the frozen container keeps its tall size.
399+
expect(result.frozenTop.position).toEqual({ x: 150, y: 100 })
400+
expect(result.frozenBottom.position).toEqual({ x: 150, y: 750 })
401+
402+
const containers = [result.frozenLoop, result.newLoop].sort(
403+
(a, b) => a.position.y - b.position.y
404+
)
405+
const upperBottom = containers[0].position.y + getBlockMetrics(containers[0]).height
406+
expect(containers[1].position.y).toBeGreaterThanOrEqual(upperBottom)
407+
})
408+
409+
it('shifts downstream blocks right when a container grows from an inserted child', () => {
410+
// Inserting inside a container widens it, but that resize happens during
411+
// layout and so is absent from the caller's change set. Blocks after the
412+
// container must still move clear of its new right edge.
413+
const blocks = {
414+
loop: createBlock('loop', {
415+
type: 'loop',
416+
position: { x: 100, y: 100 },
417+
data: { width: 900, height: 400 },
418+
layout: { measuredWidth: 900, measuredHeight: 400 },
419+
}),
420+
first: createBlock('first', {
421+
position: { x: 150, y: 150 },
422+
data: { parentId: 'loop', extent: 'parent' },
423+
layout: { measuredWidth: 250, measuredHeight: 120 },
424+
height: 120,
425+
}),
426+
last: createBlock('last', {
427+
position: { x: 600, y: 150 },
428+
data: { parentId: 'loop', extent: 'parent' },
429+
layout: { measuredWidth: 250, measuredHeight: 120 },
430+
height: 120,
431+
}),
432+
inserted: createBlock('inserted', {
433+
position: { x: 0, y: 0 },
434+
data: { parentId: 'loop', extent: 'parent' },
435+
}),
436+
after: createBlock('after', {
437+
position: { x: 1100, y: 150 },
438+
layout: { measuredWidth: 250, measuredHeight: 120 },
439+
height: 120,
440+
}),
441+
}
442+
443+
const edges: Edge[] = [
444+
{ id: 'e1', source: 'first', target: 'inserted' },
445+
{ id: 'e2', source: 'inserted', target: 'last' },
446+
{ id: 'e3', source: 'loop', target: 'after', sourceHandle: 'loop-end-source' },
447+
]
448+
449+
const result = applyTargetedLayout(blocks, edges, {
450+
changedBlockIds: ['inserted'],
451+
shiftSourceBlockIds: ['inserted'],
452+
})
453+
454+
const loopMetrics = getBlockMetrics(result.loop)
455+
expect(loopMetrics.width).toBeGreaterThan(900)
456+
expect(result.loop.position).toEqual({ x: 100, y: 100 })
457+
expect(result.after.position.x).toBeGreaterThanOrEqual(
458+
result.loop.position.x + loopMetrics.width
459+
)
460+
})
348461
})

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

Lines changed: 58 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -13,10 +13,12 @@ import {
1313
getBlockMetrics,
1414
getBlocksByParent,
1515
hasFinitePosition,
16+
isContainerType,
1617
prepareContainerDimensions,
1718
resolveNoteOverlaps,
1819
shouldSkipAutoLayout,
1920
snapPositionToGrid,
21+
sortContainersDeepestFirst,
2022
} from '@/lib/workflows/autolayout/utils'
2123
import type { BlockState } from '@/stores/workflows/workflow/types'
2224

@@ -66,6 +68,8 @@ export function applyTargetedLayout(
6668
const shiftSourceSet = new Set(shiftSourceBlockIds)
6769
const blocksCopy: Record<string, BlockState> = structuredClone(blocks)
6870

71+
const containerSizesBeforeLayout = measureContainers(blocksCopy)
72+
6973
prepareContainerDimensions(
7074
blocksCopy,
7175
edges,
@@ -79,6 +83,43 @@ export function applyTargetedLayout(
7983

8084
const subflowDepths = calculateSubflowDepths(blocksCopy, edges, assignLayers)
8185

86+
// Lay out contained groups before the groups that contain them. `prepareContainerDimensions`
87+
// sizes a container from a hypothetical layout of its children, but targeted layout keeps
88+
// frozen children where they are — so that size only becomes real once the container's own
89+
// group has run. Finishing inner groups first means every container carries the size it will
90+
// actually render at before anything is positioned beside it.
91+
for (const containerId of sortContainersDeepestFirst(
92+
Array.from(groups.children.keys()),
93+
blocksCopy
94+
)) {
95+
layoutGroup(
96+
containerId,
97+
groups.children.get(containerId) ?? [],
98+
blocksCopy,
99+
edges,
100+
changedSet,
101+
resizedSet,
102+
shiftSourceSet,
103+
verticalSpacing,
104+
horizontalSpacing,
105+
subflowDepths,
106+
gridSize
107+
)
108+
109+
// A container resized by an edit to its *children* is invisible to the caller's change
110+
// set. Mark it resized now that its final size is known, so the enclosing group shifts
111+
// neighbours out of its way. Deepest-first ordering means a nested container is already
112+
// marked before the group that contains it runs.
113+
const sizeBefore = containerSizesBeforeLayout.get(containerId)
114+
const container = blocksCopy[containerId]
115+
if (!sizeBefore || !container) continue
116+
117+
const sizeAfter = getBlockMetrics(container)
118+
if (sizeAfter.width !== sizeBefore.width || sizeAfter.height !== sizeBefore.height) {
119+
resizedSet.add(containerId)
120+
}
121+
}
122+
82123
layoutGroup(
83124
null,
84125
groups.root,
@@ -93,29 +134,30 @@ export function applyTargetedLayout(
93134
gridSize
94135
)
95136

96-
for (const [parentId, childIds] of groups.children.entries()) {
97-
layoutGroup(
98-
parentId,
99-
childIds,
100-
blocksCopy,
101-
edges,
102-
changedSet,
103-
resizedSet,
104-
shiftSourceSet,
105-
verticalSpacing,
106-
horizontalSpacing,
107-
subflowDepths,
108-
gridSize
109-
)
110-
}
111-
112137
// Relocate notes only where this pass introduced an overlap, comparing against
113138
// the original positions so pre-existing note arrangements are preserved.
114139
resolveNoteOverlaps(blocksCopy, verticalSpacing, { previousBlocks: blocks })
115140

116141
return blocksCopy
117142
}
118143

144+
/**
145+
* Captures the current on-canvas size of every container block.
146+
*/
147+
function measureContainers(
148+
blocks: Record<string, BlockState>
149+
): Map<string, { width: number; height: number }> {
150+
const sizes = new Map<string, { width: number; height: number }>()
151+
152+
for (const [id, block] of Object.entries(blocks)) {
153+
if (!isContainerType(block.type)) continue
154+
const { width, height } = getBlockMetrics(block)
155+
sizes.set(id, { width, height })
156+
}
157+
158+
return sizes
159+
}
160+
119161
/**
120162
* Selects the best anchor block for offset computation.
121163
* Prefers an upstream (predecessor) anchor over a downstream one because

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

Lines changed: 30 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -622,6 +622,35 @@ export function calculateSubflowDepths(
622622
return depths
623623
}
624624

625+
/**
626+
* Orders container IDs by nesting depth, deepest first.
627+
*
628+
* A container's size depends on its children, and a nested container counts as
629+
* one of those children, so inner containers must be resolved before the outer
630+
* ones that measure them.
631+
*/
632+
export function sortContainersDeepestFirst(
633+
containerIds: string[],
634+
blocks: Record<string, BlockState>
635+
): string[] {
636+
const containerDepth = new Map<string, number>()
637+
638+
for (const containerId of containerIds) {
639+
let depth = 0
640+
let currentId: string | undefined = containerId
641+
while (currentId) {
642+
const parentId: string | undefined = blocks[currentId]?.data?.parentId
643+
currentId = parentId
644+
if (currentId) depth++
645+
}
646+
containerDepth.set(containerId, depth)
647+
}
648+
649+
return [...containerIds].sort(
650+
(a, b) => (containerDepth.get(b) ?? 0) - (containerDepth.get(a) ?? 0)
651+
)
652+
}
653+
625654
/**
626655
* Layout function type for preparing container dimensions.
627656
* Returns laid out nodes and bounding dimensions.
@@ -663,29 +692,7 @@ export function prepareContainerDimensions(
663692
): void {
664693
const { children } = getBlocksByParent(blocks)
665694

666-
// Build dependency graph to process nested containers bottom-up
667-
const containerIds = Array.from(children.keys())
668-
const containerDepth = new Map<string, number>()
669-
670-
// Calculate nesting depth for each container
671-
for (const containerId of containerIds) {
672-
let depth = 0
673-
let currentId: string | undefined = containerId
674-
while (currentId) {
675-
const block: BlockState | undefined = blocks[currentId]
676-
const parentId: string | undefined = block?.data?.parentId
677-
currentId = parentId
678-
if (currentId) depth++
679-
}
680-
containerDepth.set(containerId, depth)
681-
}
682-
683-
// Sort containers by depth (deepest first) for bottom-up processing
684-
const sortedContainerIds = containerIds.sort((a, b) => {
685-
const depthA = containerDepth.get(a) ?? 0
686-
const depthB = containerDepth.get(b) ?? 0
687-
return depthB - depthA
688-
})
695+
const sortedContainerIds = sortContainersDeepestFirst(Array.from(children.keys()), blocks)
689696

690697
// Process each container, laying out its children to determine dimensions
691698
for (const containerId of sortedContainerIds) {

0 commit comments

Comments
 (0)