From 128054ea51efbc77641d872f52ce484c4f2bb97f Mon Sep 17 00:00:00 2001 From: Waleed Date: Wed, 12 Aug 2026 13:26:23 -0700 Subject: [PATCH] fix(workflow): stop subflows resizing themselves after every load (#6639) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(workflow): stop subflows resizing themselves after every load A container sized itself from its children, and when a child had not yet reported a height it used `estimateBlockDimensions` in its place — a guess of `ceil(subBlockCount / 2)` rows, which read a 39-field Gmail card as 276px tall against the 112px it draws. The container painted that number, the real height arrived a frame later, and it visibly resized between the two. Nothing is persisted, so it happened on every refresh. A card's height depends on what it actually renders — which rows survive its conditions, whether it draws a summary sentence, and for a reactive field even a credential it has to fetch — so the card is the only thing that can know it. Size only from heights the children have themselves reported, and hold the container at its current size until they have. `getBlockDimensions` keeps the estimate for the callers that only need a rough box (clamping a drag, placing a paste) and is now that same lookup plus the fallback. Also stop `calculateContainerDimensions` counting the container's chrome twice. Child coordinates are relative to the container's own origin and are already held clear of the header by `clampPositionToContainer`, so a child's far edge is the distance to cover and only the trailing padding is owed on top. Adding the header and leading padding again left every container 66px taller and 16px wider than its contents. * fix(workflow): gate container sizing on this session's reported layout Two holes in the measurement gate, both from reading the wrong field. `height` is a persisted column and `data.width` / `data.height` persist a container's last size, so a block that has not reported yet can still carry last session's numbers — reachable through paste, import, and checkpoint restore. The gate treated those as reported and sized from them. Nested containers had it worse: an inner container with an unreported descendant handed back its 500x300 default as though it were measured, so the outer container sized to that and resized again once the descendant filled in — the same two-step this change exists to remove. `layout` is in-memory only and written by exactly the two places that know: a card through `updateBlockLayoutMetrics`, a container through `updateNodeDimensions`. Reading it means "reported during this session" and nothing else, and an inner container that is still waiting reports null, so the outer one waits with it. `getBlockDimensions` keeps the persisted height and the estimate as fallbacks — its callers only need a rough box, where a stale height still beats a guess. * Revert "fix(workflow): gate container sizing on this session's reported layout" This reverts commit 3cce724832608b3cca9645c15b46402dde3d2dfb. * fix(workflow): size containers from a state-aware child estimate The gate in the reverted commit held a container at its current size until its children reported. That is worse than it sounds: the size it holds is the persisted default of 300, the child needs 335, and so the child hung outside the container until something forced a resize. Estimate accurately instead of waiting. `getBlockMetrics` derives a card's height from the block's own state — the sub-blocks its values leave visible, the summary sentence, the error row — through the same `calculateWorkflowBlockDimensions` the card calls, and lands on the height the card goes on to render: 112px for the Gmail card the type-only estimate put at 276px. The pass before the cards report and the pass after now produce the same container, so there is nothing to gate and nothing to correct. This also fixes the guess everywhere else it was painted rather than only in the container path — `estimateBlockDimensions` fed React Flow's node height for unmeasured blocks, so selection bounds were 276px around a 112px card. * improvement(workflow): even out the gutter inside a container Left, top and bottom were 16 and the bottom read tighter than either, because the 50px header sits above the top gap and gives that edge visual weight the other two do not have. Taking them to 24 leaves the three gutter-only edges matching and the bottom no longer pinched. Right stays 80. The container's output handle sits on that edge, so a child needs clearance there it does not need anywhere else — chrome rather than gutter, now said so in the type. Only reachable as a single constant each because the paddings mean what they say: each is the gap between a child's edge and the container's, counted once. While the sizing math added the header and leading padding a second time, the effective bottom gap was spread across three constants and tuning it meant reasoning about all of them. * improvement(workflow): give a container one source for its own gutter The four paddings and the header height existed twice: as `CONTAINER_DIMENSIONS`, which sizes a container and clamps its children, and again as Tailwind literals in `subflow-node-view`, which draws the header and the content box. Nothing kept them in step and they had already drifted — the view rendering a 40px header against a constant claiming 50, so children were clamped 10px below where the header actually ends. The view now renders from the constants, and the constant follows the DOM at 40. Match the bottom gutter to the right at 80. The two edges that carry chrome are now the two that are wider: the container's output handle sits on the right, and the resize grip in the bottom-right corner spans 40px in from both, so a child at the 24px gutter width could sit underneath it. Left and top are only gutter and stay at 24. * test(workflow-renderer): assert the subflow header's height, not its class The header renders from `CONTAINER_DIMENSIONS.HEADER_HEIGHT` now, so the class it used to carry is gone. Assert the rendered height against the same constant the layout math measures against — the two drifting apart is what this whole change is about, and a utility-class assertion cannot catch that. Also set `IS_REACT_ACT_ENVIRONMENT`, which these tests have always needed. React only treats `act` as supported when it can see the flag, so every render logged "The current testing environment is not configured to support act(...)" — around forty lines of it per run, burying the actual failure output. * fix(workflow-renderer): declare the act-environment global `vitest.setup.ts` is inside the package's tsconfig, so assigning an undeclared property on `globalThis` failed type-check (TS7017) even though the tests ran. * fix(workflow): size a container from one snapshot of the store `calculateLoopDimensions` took child positions from the live store but child dimensions from the hook's render snapshot, so it was reading two ages of the same data. `resizeLoopNodes` walks deepest-first: an inner container resized earlier in the pass was already updated in the live snapshot and still stale in the closed-over one, so its parent sized against the old inner box and only caught up on a later render — a nested container visibly resizing twice, which is the symptom this branch set out to remove. Take both from the snapshot the function already reads. * fix(workflow): size an unmeasured note as a note Routing every non-container block through `getBlockMetrics` sent notes through the workflow-card estimate, which counts sub-block rows and an error row a note does not have. A note that had not reported a height yet got a card's box, so a container holding one sized itself around the wrong shape. Give a note its own branch, as the estimate it replaced did: measured height when there is one, and the height an empty note paints when there is not. --- .../[workflowId]/hooks/use-node-utilities.ts | 77 +++++++++++++------ .../utils/node-position-utils.test.ts | 60 +++++++++++++++ .../[workflowId]/utils/node-position-utils.ts | 16 ++-- packages/workflow-renderer/src/dimensions.ts | 28 ++++++- .../src/subflow/subflow-node-view.tsx | 17 ++-- .../workflow-block-border-mount.test.tsx | 6 +- packages/workflow-renderer/vitest.setup.ts | 15 ++++ 7 files changed, 181 insertions(+), 38 deletions(-) create mode 100644 apps/sim/app/workspace/[workspaceId]/w/[workflowId]/utils/node-position-utils.test.ts 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 85a4a7ab12..5b59431bd5 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,11 +1,11 @@ import { useCallback } from 'react' import { createLogger } from '@sim/logger' -import { BLOCK_DIMENSIONS, CONTAINER_DIMENSIONS } from '@sim/workflow-renderer' +import { BLOCK_DIMENSIONS, CONTAINER_DIMENSIONS, getNoteBlockHeight } from '@sim/workflow-renderer' import { useReactFlow } from 'reactflow' +import { getBlockMetrics } from '@/lib/workflows/autolayout/utils' import { calculateContainerDimensions, clampPositionToContainer, - estimateBlockDimensions, } from '@/app/workspace/[workspaceId]/w/[workflowId]/utils/node-position-utils' import { useWorkflowStore } from '@/stores/workflows/workflow/store' @@ -26,39 +26,57 @@ export function useNodeUtilities(blocks: Record) { /** * Get the dimensions of a block. - * For regular blocks, uses stored height or estimates based on block config. + * + * Before a card mounts there is no measurement, so the height comes from + * {@link getBlockMetrics}, which estimates it from the block's own state — + * the sub-blocks its values leave visible, the summary sentence it will draw, + * the error row — through the same `calculateWorkflowBlockDimensions` the + * card itself calls. It lands on the height the card goes on to render. + * + * The old estimate read the block's *type* alone and assumed + * `ceil(subBlockCount / 2)` rows, which put a 39-field Gmail card at 276px + * against the 112px it draws. A container sized from that, painted it, then + * got the real height a frame later and visibly resized — on every load, + * because measurements are not persisted. Estimating from state removes the + * gap rather than waiting it out: both passes now produce the same number. */ - const getBlockDimensions = useCallback( - (blockId: string): { width: number; height: number } => { - const block = blocks[blockId] + const dimensionsOfBlock = useCallback( + (block: any): { width: number; height: number } => { if (!block) { return { width: BLOCK_DIMENSIONS.FIXED_WIDTH, height: BLOCK_DIMENSIONS.MIN_HEIGHT } } if (isContainerType(block.type)) { return { - width: block.data?.width - ? Math.max(block.data.width, CONTAINER_DIMENSIONS.MIN_WIDTH) - : CONTAINER_DIMENSIONS.DEFAULT_WIDTH, - height: block.data?.height - ? Math.max(block.data.height, CONTAINER_DIMENSIONS.MIN_HEIGHT) - : CONTAINER_DIMENSIONS.DEFAULT_HEIGHT, + width: Math.max( + block.data?.width || CONTAINER_DIMENSIONS.DEFAULT_WIDTH, + CONTAINER_DIMENSIONS.MIN_WIDTH + ), + height: Math.max( + block.data?.height || CONTAINER_DIMENSIONS.DEFAULT_HEIGHT, + CONTAINER_DIMENSIONS.MIN_HEIGHT + ), } } - if (block.height) { + /* A note is not a card: it has no sub-block rows and no error row, so the + card estimate does not describe it. Its own height is what it was + measured at, or the height an empty one paints. */ + if (block.type === 'note') { return { - width: block.type === 'note' ? BLOCK_DIMENSIONS.NOTE_WIDTH : BLOCK_DIMENSIONS.FIXED_WIDTH, - height: - block.type === 'note' - ? block.height - : Math.max(block.height, BLOCK_DIMENSIONS.MIN_HEIGHT), + width: BLOCK_DIMENSIONS.NOTE_WIDTH, + height: block.height || getNoteBlockHeight(true), } } - return estimateBlockDimensions(block.type) + return getBlockMetrics(block) }, - [blocks, isContainerType] + [isContainerType] + ) + + const getBlockDimensions = useCallback( + (blockId: string): { width: number; height: number } => dimensionsOfBlock(blocks[blockId]), + [blocks, dimensionsOfBlock] ) /** @@ -270,6 +288,12 @@ export function useNodeUtilities(blocks: Record) { /** * Calculates appropriate dimensions for a loop or parallel node based on its children + * + * Child heights come from {@link getBlockDimensions}, which estimates from + * block state when a card has not mounted yet and lands on the height it will + * render — so the size computed before the cards report matches the one after, + * and the container does not resize behind the user. + * * @param nodeId ID of the container node * @returns Calculated width and height for the container */ @@ -284,14 +308,21 @@ export function useNodeUtilities(blocks: Record) { .map((childId) => { const child = currentBlocks[childId] if (!child?.position) return null - const { width, height } = getBlockDimensions(childId) + /* Sized from `currentBlocks`, the same snapshot the position came + from. Reading dimensions off the hook's render snapshot instead + mixed two ages of the same store: `resizeLoopNodes` walks + deepest-first, so an inner container resized earlier in the pass + was already updated here but still old there, and the parent sized + against a stale inner box — leaving nested containers to converge + over a second pass. */ + const { width, height } = dimensionsOfBlock(child) return { x: child.position.x, y: child.position.y, width, height } }) - .filter((p): p is NonNullable => p !== null) + .filter((position): position is NonNullable => position !== null) return calculateContainerDimensions(childPositions) }, - [getBlockDimensions] + [dimensionsOfBlock] ) /** diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/utils/node-position-utils.test.ts b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/utils/node-position-utils.test.ts new file mode 100644 index 0000000000..5cf044a929 --- /dev/null +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/utils/node-position-utils.test.ts @@ -0,0 +1,60 @@ +/** + * @vitest-environment node + */ +import { CONTAINER_DIMENSIONS } from '@sim/workflow-renderer' +import { describe, expect, it } from 'vitest' +import { + calculateContainerDimensions, + clampPositionToContainer, +} from '@/app/workspace/[workspaceId]/w/[workflowId]/utils/node-position-utils' + +describe('calculateContainerDimensions', () => { + it('covers the child it holds plus one trailing padding', () => { + /* Child coordinates are relative to the container's own origin, so their + far edge is already the distance to cover. */ + const child = { x: 273, y: 207.5, width: 250, height: 112 } + + expect(calculateContainerDimensions([child])).toEqual({ + width: child.x + child.width + CONTAINER_DIMENSIONS.RIGHT_PADDING, + height: child.y + child.height + CONTAINER_DIMENSIONS.BOTTOM_PADDING, + }) + }) + + it('leaves the same gap under a child wherever the child sits', () => { + const gapUnder = (y: number) => + calculateContainerDimensions([{ x: 600, y, width: 250, height: 112 }]).height - (y + 112) + + expect(gapUnder(400)).toBe(CONTAINER_DIMENSIONS.BOTTOM_PADDING) + expect(gapUnder(700)).toBe(CONTAINER_DIMENSIONS.BOTTOM_PADDING) + }) + + it('holds a child pinned to the top-left clear of the chrome', () => { + /* The floor `clampPositionToContainer` applies is what encodes the header + and leading padding into the child's own coordinates — the sizing math + reads them from there rather than adding them again. */ + const pinned = clampPositionToContainer( + { x: -999, y: -999 }, + { width: 900, height: 900 }, + { width: 250, height: 112 } + ) + + expect(pinned).toEqual({ + x: CONTAINER_DIMENSIONS.LEFT_PADDING, + y: CONTAINER_DIMENSIONS.HEADER_HEIGHT + CONTAINER_DIMENSIONS.TOP_PADDING, + }) + }) + + it('falls back to the default box when it holds nothing', () => { + expect(calculateContainerDimensions([])).toEqual({ + width: CONTAINER_DIMENSIONS.DEFAULT_WIDTH, + height: CONTAINER_DIMENSIONS.DEFAULT_HEIGHT, + }) + }) + + it('never sizes below the default box', () => { + expect(calculateContainerDimensions([{ x: 16, y: 66, width: 40, height: 20 }])).toEqual({ + width: CONTAINER_DIMENSIONS.DEFAULT_WIDTH, + height: CONTAINER_DIMENSIONS.DEFAULT_HEIGHT, + }) + }) +}) diff --git a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/utils/node-position-utils.ts b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/utils/node-position-utils.ts index 8b93e302e0..7430229df4 100644 --- a/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/utils/node-position-utils.ts +++ b/apps/sim/app/workspace/[workspaceId]/w/[workflowId]/utils/node-position-utils.ts @@ -70,6 +70,15 @@ export function clampPositionToContainer( * Single source of truth for container sizing - ensures consistency between * live drag updates and final dimension calculations. * + * Child coordinates are relative to the container's own origin — React Flow + * places a child at the parent's origin plus its position, and + * {@link clampPositionToContainer} keeps them clear of the chrome by flooring + * them at `LEFT_PADDING` and `HEADER_HEIGHT + TOP_PADDING`. A child's far edge + * is therefore already the distance the container has to cover, and only the + * trailing padding is owed on top. Adding the header and leading padding here + * as well counted them twice, leaving every container 66px taller and 16px + * wider than its contents. + * * @param childPositions - Array of child positions with their dimensions * @returns Calculated width and height for the container */ @@ -93,14 +102,11 @@ export function calculateContainerDimensions( const width = Math.max( CONTAINER_DIMENSIONS.DEFAULT_WIDTH, - CONTAINER_DIMENSIONS.LEFT_PADDING + maxRight + CONTAINER_DIMENSIONS.RIGHT_PADDING + maxRight + CONTAINER_DIMENSIONS.RIGHT_PADDING ) const height = Math.max( CONTAINER_DIMENSIONS.DEFAULT_HEIGHT, - CONTAINER_DIMENSIONS.HEADER_HEIGHT + - CONTAINER_DIMENSIONS.TOP_PADDING + - maxBottom + - CONTAINER_DIMENSIONS.BOTTOM_PADDING + maxBottom + CONTAINER_DIMENSIONS.BOTTOM_PADDING ) return { width, height } diff --git a/packages/workflow-renderer/src/dimensions.ts b/packages/workflow-renderer/src/dimensions.ts index 8efc509854..e35fb5cb08 100644 --- a/packages/workflow-renderer/src/dimensions.ts +++ b/packages/workflow-renderer/src/dimensions.ts @@ -80,16 +80,36 @@ export const estimateNoteBlockHeight = (content: string) => { ) } +/** + * A container's box, and the gutter it keeps around the blocks inside it. + * + * The single source for both halves of that: the layout math that sizes a + * container and clamps its children, and the card's own DOM. `subflow-node-view` + * renders its header and content box straight from these, so the gap the + * geometry reserves is the gap the container actually paints. They used to be + * separate — the same four numbers as Tailwind literals in the view — and had + * already drifted, the view drawing a 40px header against a constant that + * claimed 50. + * + * Each padding is the gap between a child's edge and the container's, counted + * once: the header is accounted for by the child's own position, which + * `clampPositionToContainer` floors at `HEADER_HEIGHT + TOP_PADDING`. + * + * The two edges that carry chrome are wider than the two that are only gutter. + * The container's output handle sits on the right, and the resize grip in the + * bottom-right corner spans 40px in from both — a child at the gutter width + * would sit underneath it. + */ export const CONTAINER_DIMENSIONS = { DEFAULT_WIDTH: 500, DEFAULT_HEIGHT: 300, MIN_WIDTH: 400, MIN_HEIGHT: 200, - HEADER_HEIGHT: 50, - LEFT_PADDING: 16, + HEADER_HEIGHT: 40, + LEFT_PADDING: 24, RIGHT_PADDING: 80, - TOP_PADDING: 16, - BOTTOM_PADDING: 16, + TOP_PADDING: 24, + BOTTOM_PADDING: 80, } as const /** diff --git a/packages/workflow-renderer/src/subflow/subflow-node-view.tsx b/packages/workflow-renderer/src/subflow/subflow-node-view.tsx index b7d25588ae..40fbafd140 100644 --- a/packages/workflow-renderer/src/subflow/subflow-node-view.tsx +++ b/packages/workflow-renderer/src/subflow/subflow-node-view.tsx @@ -8,7 +8,7 @@ import { useStoreApi as useReactFlowStoreApi, useUpdateNodeInternals, } from 'reactflow' -import { BLOCK_DIMENSIONS, HANDLE_POSITIONS } from '../dimensions' +import { BLOCK_DIMENSIONS, CONTAINER_DIMENSIONS, HANDLE_POSITIONS } from '../dimensions' import { OverflowSpan } from '../lib/overflow-span' import type { DiffStatus } from '../types' import { @@ -588,8 +588,8 @@ export function SubflowNodeView({ aria-label={`Select ${blockName}`} onClick={onSelect} onKeyDown={(event) => handleKeyboardActivation(event, onSelect)} - className='workflow-drag-handle relative z-20 flex h-[40px] cursor-grab items-center justify-between px-2 [&:active]:cursor-grabbing' - style={{ pointerEvents: 'auto' }} + className='workflow-drag-handle relative z-20 flex cursor-grab items-center justify-between px-2 [&:active]:cursor-grabbing' + style={{ pointerEvents: 'auto', height: CONTAINER_DIMENSIONS.HEADER_HEIGHT }} data-subflow-header='' >
{ ) const header = host.querySelector('[data-subflow-header]') - expect(header).toHaveClass('h-[40px]') + /* Height comes from `CONTAINER_DIMENSIONS`, which the layout math also + measures against — asserting the rendered value rather than a utility + class keeps the two from drifting apart again. */ + expect(header).toHaveStyle({ height: `${CONTAINER_DIMENSIONS.HEADER_HEIGHT}px` }) expect(header).not.toHaveClass('border-b') expect(header).not.toHaveClass('bg-[var(--surface-2)]') expect(host.querySelector('[data-subflow-type-tag="loop"]')).toHaveTextContent('Loop') diff --git a/packages/workflow-renderer/vitest.setup.ts b/packages/workflow-renderer/vitest.setup.ts index 0c78bee392..1468f99e72 100644 --- a/packages/workflow-renderer/vitest.setup.ts +++ b/packages/workflow-renderer/vitest.setup.ts @@ -1,3 +1,11 @@ +declare global { + /** + * React reads this to decide whether `act` is supported. It is not one of the + * ambient globals, so it has to be declared before it can be assigned. + */ + var IS_REACT_ACT_ENVIRONMENT: boolean +} + /** * jest-dom only registers DOM matchers (`toHaveStyle`, `toHaveClass`, …), so it is * dead weight outside a DOM environment. This package's mount tests opt into jsdom @@ -5,4 +13,11 @@ */ if (typeof document !== 'undefined') { await import('@testing-library/jest-dom/vitest') + /* + * Without this React treats `act` as unsupported, and every render in the + * mount tests logs "The current testing environment is not configured to + * support act(...)" — around forty lines a run, which buries the output that + * matters when one of them fails. + */ + globalThis.IS_REACT_ACT_ENVIRONMENT = true }