From 5e1aa23726ff3044092a044b9cba9ffcd799ae5a Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 12 Aug 2026 14:59:37 -0700 Subject: [PATCH 1/2] fix(workflow): stop a nested block jumping when it leaves its container MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .../hooks/use-node-utilities.test.tsx | 95 +++++++++++++++++++ .../[workflowId]/hooks/use-node-utilities.ts | 24 +++-- .../[workspaceId]/w/[workflowId]/workflow.tsx | 36 +++---- 3 files changed, 129 insertions(+), 26 deletions(-) create mode 100644 apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.test.tsx diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.test.tsx b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.test.tsx new file mode 100644 index 00000000000..c440cf300f5 --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.test.tsx @@ -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: Record) { + let api: ReturnType | null = null + function Probe() { + api = useNodeUtilities(blockMap as Record) + return null + } + const host = document.createElement('div') + document.body.appendChild(host) + act(() => { + createRoot(host).render() + }) + 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 = { + 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) + }) +}) diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.ts b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.ts index 5b59431bd5e..91833d7c79f 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.ts +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.ts @@ -140,8 +140,20 @@ export function useNodeUtilities(blocks: Record) { ) /** - * 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} */ @@ -184,13 +196,9 @@ export function useNodeUtilities(blocks: Record) { 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] diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/workflow.tsx b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/workflow.tsx index 4e972785a87..5ec22eca9d5 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/workflow.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/workflow.tsx @@ -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 From 1c7fc31f29d17e385c3f3341d243edb1909422cf Mon Sep 17 00:00:00 2001 From: Waleed Latif Date: Wed, 12 Aug 2026 15:07:31 -0700 Subject: [PATCH 2/2] refactor(workflow): type the node-utilities block map `useNodeUtilities` took `Record`, 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` 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. --- .../w/[workflowId]/hooks/use-node-utilities.test.tsx | 6 +++--- .../w/[workflowId]/hooks/use-node-utilities.ts | 10 ++++++---- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.test.tsx b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.test.tsx index c440cf300f5..0de4fa927ef 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.test.tsx +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.test.tsx @@ -18,10 +18,10 @@ vi.mock('reactflow', () => ({ 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: Record) { +function renderNodeUtilities(blockMap: Parameters[0]) { let api: ReturnType | null = null function Probe() { - api = useNodeUtilities(blockMap as Record) + api = useNodeUtilities(blockMap) return null } const host = document.createElement('div') @@ -43,7 +43,7 @@ const CHILD_POSITION = { y: CONTAINER_DIMENSIONS.HEADER_HEIGHT + CONTAINER_DIMENSIONS.TOP_PADDING, } -const blocks = { +const blocks: Parameters[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: {} }, diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.ts b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.ts index 91833d7c79f..4b9959ebc16 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.ts +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.ts @@ -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) { +export function useNodeUtilities(blocks: Record) { const { getNodes } = useReactFlow() /** @@ -180,9 +181,10 @@ export function useNodeUtilities(blocks: Record) { } const visited = new Set() - 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,