fix(workflow): stop a nested block jumping when it leaves its container (#6644)

* 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.

* refactor(workflow): type the node-utilities block map

`useNodeUtilities` took `Record<string, any>`, so the test fixtures had to be
cast to reach it and nothing in the hook was checked against a real block.

Typing it as `Record<string, BlockState>` surfaced an unsafe read straight away:
the cycle walk re-read `blocks[currentId].data.parentId` after the `while`
condition had tested the same optional chain, on a map where both links are
optional. It now reads the value once and breaks on absence, which is what the
condition was trying to express.

The fixtures follow the hook's own parameter type, so they stay honest without a
cast on either side.
This commit is contained in:
Waleed
2026-08-12 15:16:12 -07:00
committed by GitHub
parent 1fa40b8118
commit c58a6427f7
3 changed files with 135 additions and 30 deletions
@@ -0,0 +1,95 @@
/**
* @vitest-environment jsdom
*/
import { act } from 'react'
import { CONTAINER_DIMENSIONS } from '@sim/workflow-renderer'
import { createRoot } from 'react-dom/client'
import { beforeEach, describe, expect, it, vi } from 'vitest'
const { mockGetNodes } = vi.hoisted(() => ({ mockGetNodes: vi.fn() }))
vi.mock('reactflow', () => ({
useReactFlow: () => ({ getNodes: mockGetNodes }),
Position: { Left: 'left', Right: 'right', Top: 'top', Bottom: 'bottom' },
Handle: () => null,
}))
import { useNodeUtilities } from '@/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities'
/** Renders the hook and hands back what it returned, without a test library. */
function renderNodeUtilities(blockMap: Parameters<typeof useNodeUtilities>[0]) {
let api: ReturnType<typeof useNodeUtilities> | null = null
function Probe() {
api = useNodeUtilities(blockMap)
return null
}
const host = document.createElement('div')
document.body.appendChild(host)
act(() => {
createRoot(host).render(<Probe />)
})
if (!api) throw new Error('hook did not render')
return api
}
/**
* A container at (1000, 500) holding one child placed at the top-left of its
* body — exactly where `clampPositionToContainer` floors a child.
*/
const CONTAINER_POSITION = { x: 1000, y: 500 }
const CHILD_POSITION = {
x: CONTAINER_DIMENSIONS.LEFT_PADDING,
y: CONTAINER_DIMENSIONS.HEADER_HEIGHT + CONTAINER_DIMENSIONS.TOP_PADDING,
}
const blocks: Parameters<typeof useNodeUtilities>[0] = {
loop: { id: 'loop', type: 'loop', position: CONTAINER_POSITION, data: {} },
child: { id: 'child', type: 'gmail_v2', position: CHILD_POSITION, data: { parentId: 'loop' } },
root: { id: 'root', type: 'gmail_v2', position: { x: 10, y: 20 }, data: {} },
}
const nodes = [
{ id: 'loop', position: CONTAINER_POSITION },
{ id: 'child', position: CHILD_POSITION, parentId: 'loop' },
{ id: 'root', position: { x: 10, y: 20 } },
]
describe('getNodeAbsolutePosition', () => {
beforeEach(() => {
vi.clearAllMocks()
mockGetNodes.mockReturnValue(nodes)
})
it('places a child at its parent plus its own position, as React Flow does', () => {
/* A child's position is already relative to the container's origin — the
header and padding live in the position itself, put there by
`clampPositionToContainer`. Adding them again reported a nested node 16px
right and 66px below where it actually renders, which is why callers had
to subtract the same constants back off. */
const api = renderNodeUtilities(blocks)
expect(api.getNodeAbsolutePosition('child')).toEqual({
x: CONTAINER_POSITION.x + CHILD_POSITION.x,
y: CONTAINER_POSITION.y + CHILD_POSITION.y,
})
})
it('leaves a root-level node exactly where it is', () => {
const api = renderNodeUtilities(blocks)
expect(api.getNodeAbsolutePosition('root')).toEqual({ x: 10, y: 20 })
expect(api.getNodeAbsolutePosition('loop')).toEqual(CONTAINER_POSITION)
})
it('round-trips: a child popped out of its container does not move', () => {
/* Removing a parent stores the node's absolute position verbatim, so any
drift here is a visible jump — the block used to drop 66px down and 16px
right the moment it left the container. */
const api = renderNodeUtilities(blocks)
const absolute = api.getNodeAbsolutePosition('child')
const container = api.getNodeAbsolutePosition('loop')
expect({ x: absolute.x - container.x, y: absolute.y - container.y }).toEqual(CHILD_POSITION)
})
})
@@ -1,6 +1,7 @@
import { useCallback } from 'react'
import { createLogger } from '@sim/logger'
import { BLOCK_DIMENSIONS, CONTAINER_DIMENSIONS, getNoteBlockHeight } from '@sim/workflow-renderer'
import type { BlockState } from '@sim/workflow-types/workflow'
import { useReactFlow } from 'reactflow'
import { getBlockMetrics } from '@/lib/workflows/autolayout/utils'
import {
@@ -14,7 +15,7 @@ const logger = createLogger('NodeUtilities')
/**
* Hook providing utilities for node position, hierarchy, and dimension calculations
*/
export function useNodeUtilities(blocks: Record<string, any>) {
export function useNodeUtilities(blocks: Record<string, BlockState>) {
const { getNodes } = useReactFlow()
/**
@@ -140,8 +141,20 @@ export function useNodeUtilities(blocks: Record<string, any>) {
)
/**
* Gets the absolute position of a node (accounting for nested parents).
* For nodes inside containers, accounts for header and padding offsets.
* Gets the absolute position of a node, walking up its parent chain.
*
* A child's position is relative to its container's own origin — React Flow
* places it at the parent's origin plus its position, and
* `clampPositionToContainer` is what holds it clear of the chrome, flooring it
* at `LEFT_PADDING` and `HEADER_HEIGHT + TOP_PADDING`. The container's header
* and padding are therefore already inside the child's coordinates, and
* adding them again here counted them twice: a nested node reported 16px
* right and 66px below where it actually is.
*
* That is why callers wanting a relative position had to subtract the same
* three constants straight back off, and why `positionAbsolute` — React
* Flow's own answer, which carries no offset — disagreed with this one.
*
* @param nodeId ID of the node to check
* @returns Absolute position coordinates {x, y}
*/
@@ -168,9 +181,10 @@ export function useNodeUtilities(blocks: Record<string, any>) {
}
const visited = new Set<string>()
let currentId = nodeId
while (currentId && blocks?.[currentId]?.data?.parentId) {
const currentParentId = blocks[currentId].data.parentId
let currentId: string | undefined = nodeId
while (currentId) {
const currentParentId: string | undefined = blocks[currentId]?.data?.parentId
if (!currentParentId) break
if (visited.has(currentParentId)) {
logger.error('Circular parent reference detected', {
nodeId,
@@ -184,13 +198,9 @@ export function useNodeUtilities(blocks: Record<string, any>) {
const parentPos = getNodeAbsolutePosition(parentId)
const headerHeight = 50
const leftPadding = 16
const topPadding = 16
return {
x: parentPos.x + leftPadding + node.position.x,
y: parentPos.y + headerHeight + topPadding + node.position.y,
x: parentPos.x + node.position.x,
y: parentPos.y + node.position.y,
}
},
[getNodes, blocks]
@@ -969,14 +969,14 @@ const WorkflowContent = React.memo(
let newPosition = oldPosition
if (newParentId) {
/* Both absolutes are in the container's own coordinate space, so the
difference is already the child's position within it — the header
and padding are accounted for by the clamp, not subtracted here. */
const nodeAbsPos = getNodeAbsolutePosition(nodeId)
const parentAbsPos = getNodeAbsolutePosition(newParentId)
const headerHeight = 50
const leftPadding = 16
const topPadding = 16
newPosition = {
x: nodeAbsPos.x - parentAbsPos.x - leftPadding,
y: nodeAbsPos.y - parentAbsPos.y - headerHeight - topPadding,
x: nodeAbsPos.x - parentAbsPos.x,
y: nodeAbsPos.y - parentAbsPos.y,
}
} else if (oldParentId) {
newPosition = getNodeAbsolutePosition(nodeId)
@@ -2711,12 +2711,13 @@ const WorkflowContent = React.memo(
const parentId = block.data?.parentId as string | undefined
if (!parentId) return block.data?.extent || undefined
// Constrain ONLY the top by header height (42px) and keep a small left padding.
// Do not clamp right/bottom so blocks can move freely within the body.
const headerHeight = 42
const leftPadding = 16
const minX = leftPadding
const minY = headerHeight
// Constrain the top and left to the container's own gutter, the same
// floor `clampPositionToContainer` applies everywhere else — a drag
// that stopped somewhere different from a drop was the whole reason
// these numbers were written out by hand and drifted. Right and
// bottom stay free so a block can move anywhere in the body.
const minX = CONTAINER_DIMENSIONS.LEFT_PADDING
const minY = CONTAINER_DIMENSIONS.HEADER_HEIGHT + CONTAINER_DIMENSIONS.TOP_PADDING
const maxX = Number.POSITIVE_INFINITY
const maxY = Number.POSITIVE_INFINITY
@@ -3767,17 +3768,16 @@ const WorkflowContent = React.memo(
})
}
// Compute relative position BEFORE updating parent to avoid stale state
// Account for header (50px), left padding (16px), and top padding (16px)
// Computed BEFORE updating the parent to avoid stale state. The two
// absolutes share the container's coordinate space, so their
// difference is the child's position within it — which is what the
// sibling positions this is compared against are measured in too.
const containerAbsPosBefore = getNodeAbsolutePosition(potentialParentId)
const nodeAbsPosBefore = getNodeAbsolutePosition(node.id)
const headerHeight = 50
const leftPadding = 16
const topPadding = 16
const relativePositionBefore = {
x: nodeAbsPosBefore.x - containerAbsPosBefore.x - leftPadding,
y: nodeAbsPosBefore.y - containerAbsPosBefore.y - headerHeight - topPadding,
x: nodeAbsPosBefore.x - containerAbsPosBefore.x,
y: nodeAbsPosBefore.y - containerAbsPosBefore.y,
}
// Auto-connect when moving an existing block into a container