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..0de4fa927ef --- /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: Parameters[0]) { + let api: ReturnType | null = null + function Probe() { + api = useNodeUtilities(blockMap) + 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: 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: {} }, +} + +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..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() /** @@ -140,8 +141,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} */ @@ -168,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, @@ -184,13 +198,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