fix(workflow): stop subflows resizing themselves after every load - #6639
Merged
Conversation
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.
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
PR SummaryMedium Risk Overview
Reviewed by Cursor Bugbot for commit a32bbe5. Configure here. |
Contributor
Greptile SummaryThe PR replaces type-only child-height estimates with state-aware metrics, makes nested container sizing consume a fresh store snapshot, and aligns container geometry with rendered chrome.
Confidence Score: 4/5The PR is not yet safe to merge because MCP cards with runtime-expanded arguments still cause their parent containers to resize after mounting. The new state-based estimate counts Files Needing Attention: apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.ts
|
| Filename | Overview |
|---|---|
| apps/sim/app/workspace/[workspaceId]/w/[workflowId]/hooks/use-node-utilities.ts | Introduces state-aware, fresh-snapshot sizing, but runtime-expanded MCP arguments still make pre-mount and measured card heights diverge. |
| apps/sim/app/workspace/[workspaceId]/w/[workflowId]/utils/node-position-utils.ts | Correctly removes duplicated leading chrome from container bounds while retaining trailing padding and minimum dimensions. |
| packages/workflow-renderer/src/dimensions.ts | Centralizes the container chrome and gutter dimensions shared by layout and rendering. |
| packages/workflow-renderer/src/subflow/subflow-node-view.tsx | Replaces duplicated Tailwind geometry literals with shared container-dimension constants. |
| apps/sim/app/workspace/[workspaceId]/w/[workflowId]/utils/node-position-utils.test.ts | Covers the corrected coordinate convention, trailing gaps, defaults, and minimum container bounds. |
Reviews (5): Last reviewed commit: "fix(workflow): size an unmeasured note a..." | Re-trigger Greptile
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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.
Collaborator
Author
Collaborator
Author
|
@cursor review |
…ed layout" This reverts commit 3cce724.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
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.
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.
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.
Collaborator
Author
Collaborator
Author
|
@cursor review |
…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.
`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.
`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.
Collaborator
Author
Collaborator
Author
|
@cursor review |
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.
Collaborator
Author
Collaborator
Author
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit a32bbe5. Configure here.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
estimateBlockDimensionsinstead — a guess ofceil(subBlockCount / 2)rows clamped to[3,7], which reads a 39-field Gmail card as 276px tall against the 112px it actually draws. The container painted that, the real height arrived a frame later, and it resized between the two.useReactiveConditions→useWorkspaceCredential). The card is the only thing that can know it. So: size only from heights children have themselves reported, and hold the container at its current size until they have.getBlockDimensionskeeps the estimate for callers that only need a rough box (clamping a drag, placing a paste) and is now that same lookup plus the fallback, rather than a second copy of the policy.layout(in-memory, written byupdateBlockLayoutMetricsfor cards andupdateNodeDimensionsfor containers), notheight/data.height. Those are persisted, so an unreported block can still carry last session's numbers and an unsized inner container hands back its 500x300 default — both of which the gate would otherwise treat as measured, and the second of which made nested containers resize twice.calculateContainerDimensionscounted the container's chrome twice. Child coordinates are relative to the container's own origin and already held clear of the header byclampPositionToContainer, so a child's far edge is the distance to cover and only the trailing padding is owed. Adding the header and leading padding again left every container 66px taller and 16px wider than its contents.For the reported workflow, the Loop went
300 → 565.5 → 401.5on load. It now settles once, at603 × 335.5— the child's far edge plus exactly one padding.Type of Change
Testing
New unit tests pin the sizing contract; two of them fail on the old math (
expected { width: 619, height: 401.5 } to deeply equal { width: 603, height: 335.5 }, andexpected 82 to be 16for the gap under a child). Fullutilssuite green (62), type-check and lint clean.The two sizes were derived by running the real functions against the exported workflow state, and cross-checked against the reported screenshots: scaling both by the Gmail card's known 112px height (both at 0.58) gives container heights of ~412 and ~579 against the computed 401.5 and 565.5, with the child's offset staying ~207.5 in both. That also confirms the coordinate convention the padding fix depends on — children are positioned from the container's outer origin.
Not verified in the running app. I brought up
dev:fullbut could not sign in to the local instance, so this is verified by unit test and arithmetic only.Follow-ups (not in this PR)
estimateWorkflowBlockDimensionsis exact on the sentence path but deliberately biased high on the field-row path, because amcp-dynamic-argsfield expands into one row per schema property when the card mounts. The bias is correct for autolayout (a gap beats an overlap) but still moves a container. It is fixable — contrary to that function's doc, the count is derivable from block state, since_toolSchemais an ordinary subblock value — but removing the slack changes autolayout spacing, so it wants its own change.estimateWorkflowBlockDimensions(lib/workflows/autolayout/utils.ts) already estimates from block state rather than type alone, and is far more accurate thanestimateBlockDimensions. Routing the remaining estimate callers through it would fix the guess everywhere it is still painted (workflow.tsx:2747feeds it into React Flow node height, so selection bounds are 276px on a 112px card).preview-workflow.tsxre-implementscalculateContainerDimensions— and already uses the corrected formula, independent corroboration that the double-count was a bug.getNodeAbsolutePositionhardcodesheaderHeight = 50 / leftPadding = 16 / topPadding = 16and adds them to a child's position, which contradicts the outer-origin convention the rest of the code uses. It looks like a latent bug for nested containers, but it feeds reparenting math and deserves its own verification.block.heightand containerdata.width/data.heightare persisted — what is missing is the write-back.updateNodeDimensionsis a raw store action with no collaborative sync, and the realtime subflow-config handler re-floorsdatato 500x300, so a container paints the default on load and grows once when its children report. A tall loop still visibly grows from 300px each refresh. Removing that frame means writing the derived size back, which is a bigger change than this one.updateContainerDimensionsDuringMove(the drag path) still feedscalculateContainerDimensionsestimates. Plausibly correct — a best-effort box while dragging — but it duplicates the gather loop.Checklist