Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -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')
Comment thread
waleedlatif1 marked this conversation as resolved.
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)
})
})
Original file line number Diff line number Diff line change
@@ -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 {
Expand All @@ -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()

/**
Expand Down Expand Up @@ -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}
*/
Expand All @@ -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,
Expand All @@ -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]
Expand Down
36 changes: 18 additions & 18 deletions apps/sim/app/workspace/[workspaceId]/w/[workflowId]/workflow.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down Expand Up @@ -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

Expand Down Expand Up @@ -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
Expand Down
Loading