Skip to content

Commit 5e1aa23

Browse files
committed
fix(workflow): stop a nested block jumping when it leaves its container
`getNodeAbsolutePosition` added the container's header and padding to a child's position. Those are already in the position: React Flow places a child at its parent's origin plus its own coordinates, and `clampPositionToContainer` is what holds it clear of the chrome, flooring it at `LEFT_PADDING` and `HEADER_HEIGHT + TOP_PADDING`. Counting them twice put every nested node 16px right and 66px below where it actually renders. Visible as a block dropping down-right the moment it is dragged out of a Loop, and as a block landing off-target when dragged into one from the canvas. Also skewed container hit-testing during a drag and the bounds `fitView` focuses on. Two callers already knew: both subtracted the same three constants straight back off to recover a relative position. They now take the difference of two absolutes, which is what a relative position is. A third place, React Flow's child `extent`, had its own copy of the numbers — a fourth distinct header height, 42, against the 40 the card renders — and now reads the same constants as the clamp, so a drag stops where a drop would put it. `positionAbsolute ?? getNodeAbsolutePosition(...)` in the fit-view path can also stop disagreeing with itself: React Flow's own answer carries no offset, so the two branches returned points 66px apart for the same node.
1 parent 128054e commit 5e1aa23

3 files changed

Lines changed: 129 additions & 26 deletions

File tree

Lines changed: 95 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,95 @@
1+
/**
2+
* @vitest-environment jsdom
3+
*/
4+
5+
import { act } from 'react'
6+
import { CONTAINER_DIMENSIONS } from '@sim/workflow-renderer'
7+
import { createRoot } from 'react-dom/client'
8+
import { beforeEach, describe, expect, it, vi } from 'vitest'
9+
10+
const { mockGetNodes } = vi.hoisted(() => ({ mockGetNodes: vi.fn() }))
11+
12+
vi.mock('reactflow', () => ({
13+
useReactFlow: () => ({ getNodes: mockGetNodes }),
14+
Position: { Left: 'left', Right: 'right', Top: 'top', Bottom: 'bottom' },
15+
Handle: () => null,
16+
}))
17+
18+
import { useNodeUtilities } from '@/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities'
19+
20+
/** Renders the hook and hands back what it returned, without a test library. */
21+
function renderNodeUtilities(blockMap: Record<string, unknown>) {
22+
let api: ReturnType<typeof useNodeUtilities> | null = null
23+
function Probe() {
24+
api = useNodeUtilities(blockMap as Record<string, any>)
25+
return null
26+
}
27+
const host = document.createElement('div')
28+
document.body.appendChild(host)
29+
act(() => {
30+
createRoot(host).render(<Probe />)
31+
})
32+
if (!api) throw new Error('hook did not render')
33+
return api
34+
}
35+
36+
/**
37+
* A container at (1000, 500) holding one child placed at the top-left of its
38+
* body — exactly where `clampPositionToContainer` floors a child.
39+
*/
40+
const CONTAINER_POSITION = { x: 1000, y: 500 }
41+
const CHILD_POSITION = {
42+
x: CONTAINER_DIMENSIONS.LEFT_PADDING,
43+
y: CONTAINER_DIMENSIONS.HEADER_HEIGHT + CONTAINER_DIMENSIONS.TOP_PADDING,
44+
}
45+
46+
const blocks = {
47+
loop: { id: 'loop', type: 'loop', position: CONTAINER_POSITION, data: {} },
48+
child: { id: 'child', type: 'gmail_v2', position: CHILD_POSITION, data: { parentId: 'loop' } },
49+
root: { id: 'root', type: 'gmail_v2', position: { x: 10, y: 20 }, data: {} },
50+
}
51+
52+
const nodes = [
53+
{ id: 'loop', position: CONTAINER_POSITION },
54+
{ id: 'child', position: CHILD_POSITION, parentId: 'loop' },
55+
{ id: 'root', position: { x: 10, y: 20 } },
56+
]
57+
58+
describe('getNodeAbsolutePosition', () => {
59+
beforeEach(() => {
60+
vi.clearAllMocks()
61+
mockGetNodes.mockReturnValue(nodes)
62+
})
63+
64+
it('places a child at its parent plus its own position, as React Flow does', () => {
65+
/* A child's position is already relative to the container's origin — the
66+
header and padding live in the position itself, put there by
67+
`clampPositionToContainer`. Adding them again reported a nested node 16px
68+
right and 66px below where it actually renders, which is why callers had
69+
to subtract the same constants back off. */
70+
const api = renderNodeUtilities(blocks)
71+
72+
expect(api.getNodeAbsolutePosition('child')).toEqual({
73+
x: CONTAINER_POSITION.x + CHILD_POSITION.x,
74+
y: CONTAINER_POSITION.y + CHILD_POSITION.y,
75+
})
76+
})
77+
78+
it('leaves a root-level node exactly where it is', () => {
79+
const api = renderNodeUtilities(blocks)
80+
81+
expect(api.getNodeAbsolutePosition('root')).toEqual({ x: 10, y: 20 })
82+
expect(api.getNodeAbsolutePosition('loop')).toEqual(CONTAINER_POSITION)
83+
})
84+
85+
it('round-trips: a child popped out of its container does not move', () => {
86+
/* Removing a parent stores the node's absolute position verbatim, so any
87+
drift here is a visible jump — the block used to drop 66px down and 16px
88+
right the moment it left the container. */
89+
const api = renderNodeUtilities(blocks)
90+
const absolute = api.getNodeAbsolutePosition('child')
91+
const container = api.getNodeAbsolutePosition('loop')
92+
93+
expect({ x: absolute.x - container.x, y: absolute.y - container.y }).toEqual(CHILD_POSITION)
94+
})
95+
})

apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.ts

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -140,8 +140,20 @@ export function useNodeUtilities(blocks: Record<string, any>) {
140140
)
141141

142142
/**
143-
* Gets the absolute position of a node (accounting for nested parents).
144-
* For nodes inside containers, accounts for header and padding offsets.
143+
* Gets the absolute position of a node, walking up its parent chain.
144+
*
145+
* A child's position is relative to its container's own origin — React Flow
146+
* places it at the parent's origin plus its position, and
147+
* `clampPositionToContainer` is what holds it clear of the chrome, flooring it
148+
* at `LEFT_PADDING` and `HEADER_HEIGHT + TOP_PADDING`. The container's header
149+
* and padding are therefore already inside the child's coordinates, and
150+
* adding them again here counted them twice: a nested node reported 16px
151+
* right and 66px below where it actually is.
152+
*
153+
* That is why callers wanting a relative position had to subtract the same
154+
* three constants straight back off, and why `positionAbsolute` — React
155+
* Flow's own answer, which carries no offset — disagreed with this one.
156+
*
145157
* @param nodeId ID of the node to check
146158
* @returns Absolute position coordinates {x, y}
147159
*/
@@ -184,13 +196,9 @@ export function useNodeUtilities(blocks: Record<string, any>) {
184196

185197
const parentPos = getNodeAbsolutePosition(parentId)
186198

187-
const headerHeight = 50
188-
const leftPadding = 16
189-
const topPadding = 16
190-
191199
return {
192-
x: parentPos.x + leftPadding + node.position.x,
193-
y: parentPos.y + headerHeight + topPadding + node.position.y,
200+
x: parentPos.x + node.position.x,
201+
y: parentPos.y + node.position.y,
194202
}
195203
},
196204
[getNodes, blocks]

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

Lines changed: 18 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -969,14 +969,14 @@ const WorkflowContent = React.memo(
969969

970970
let newPosition = oldPosition
971971
if (newParentId) {
972+
/* Both absolutes are in the container's own coordinate space, so the
973+
difference is already the child's position within it — the header
974+
and padding are accounted for by the clamp, not subtracted here. */
972975
const nodeAbsPos = getNodeAbsolutePosition(nodeId)
973976
const parentAbsPos = getNodeAbsolutePosition(newParentId)
974-
const headerHeight = 50
975-
const leftPadding = 16
976-
const topPadding = 16
977977
newPosition = {
978-
x: nodeAbsPos.x - parentAbsPos.x - leftPadding,
979-
y: nodeAbsPos.y - parentAbsPos.y - headerHeight - topPadding,
978+
x: nodeAbsPos.x - parentAbsPos.x,
979+
y: nodeAbsPos.y - parentAbsPos.y,
980980
}
981981
} else if (oldParentId) {
982982
newPosition = getNodeAbsolutePosition(nodeId)
@@ -2711,12 +2711,13 @@ const WorkflowContent = React.memo(
27112711
const parentId = block.data?.parentId as string | undefined
27122712
if (!parentId) return block.data?.extent || undefined
27132713

2714-
// Constrain ONLY the top by header height (42px) and keep a small left padding.
2715-
// Do not clamp right/bottom so blocks can move freely within the body.
2716-
const headerHeight = 42
2717-
const leftPadding = 16
2718-
const minX = leftPadding
2719-
const minY = headerHeight
2714+
// Constrain the top and left to the container's own gutter, the same
2715+
// floor `clampPositionToContainer` applies everywhere else — a drag
2716+
// that stopped somewhere different from a drop was the whole reason
2717+
// these numbers were written out by hand and drifted. Right and
2718+
// bottom stay free so a block can move anywhere in the body.
2719+
const minX = CONTAINER_DIMENSIONS.LEFT_PADDING
2720+
const minY = CONTAINER_DIMENSIONS.HEADER_HEIGHT + CONTAINER_DIMENSIONS.TOP_PADDING
27202721
const maxX = Number.POSITIVE_INFINITY
27212722
const maxY = Number.POSITIVE_INFINITY
27222723

@@ -3767,17 +3768,16 @@ const WorkflowContent = React.memo(
37673768
})
37683769
}
37693770

3770-
// Compute relative position BEFORE updating parent to avoid stale state
3771-
// Account for header (50px), left padding (16px), and top padding (16px)
3771+
// Computed BEFORE updating the parent to avoid stale state. The two
3772+
// absolutes share the container's coordinate space, so their
3773+
// difference is the child's position within it — which is what the
3774+
// sibling positions this is compared against are measured in too.
37723775
const containerAbsPosBefore = getNodeAbsolutePosition(potentialParentId)
37733776
const nodeAbsPosBefore = getNodeAbsolutePosition(node.id)
3774-
const headerHeight = 50
3775-
const leftPadding = 16
3776-
const topPadding = 16
37773777

37783778
const relativePositionBefore = {
3779-
x: nodeAbsPosBefore.x - containerAbsPosBefore.x - leftPadding,
3780-
y: nodeAbsPosBefore.y - containerAbsPosBefore.y - headerHeight - topPadding,
3779+
x: nodeAbsPosBefore.x - containerAbsPosBefore.x,
3780+
y: nodeAbsPosBefore.y - containerAbsPosBefore.y,
37813781
}
37823782

37833783
// Auto-connect when moving an existing block into a container

0 commit comments

Comments
 (0)