fix(apollo-react): compute node height from handle count to stop flicker [MST-11677]#867
fix(apollo-react): compute node height from handle count to stop flicker [MST-11677]#867KodudulaAshishUiPath wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Dependency License Review
License distribution
Excluded packages
|
There was a problem hiding this comment.
Pull request overview
Fixes a React Flow node-height oscillation in apollo-react canvas BaseNode caused by a deferred requestAnimationFrame height sync briefly stamping a “synced” height before the write actually committed, allowing a stale in-flight height to be treated as an external resize.
Changes:
- Move
syncedHeightRefstamping into the rAF callback so it only records heights that were actually committed viaupdateNode. - Tighten comments around
syncedHeightRef/baseHeightRefsemantics to reflect commit-time stamping and the external-vs-internal height distinction. - Add a regression test that keeps rAF deferred and reproduces the stale interleaved render scenario, asserting the node deflates and remains stable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| packages/apollo-react/src/canvas/components/BaseNode/BaseNode.tsx | Stamps syncedHeightRef at rAF commit time (paired with updateNode) to prevent stale in-flight heights from poisoning baseHeightRef. |
| packages/apollo-react/src/canvas/components/BaseNode/BaseNode.test.tsx | Adds a deferred-rAF regression test to reproduce and prevent the 96px ↔ 128px flicker loop. |
📊 Coverage + size by packagePer-package coverage and bundle size on this PR. New-line coverage = of the source lines this PR adds or changes, the % hit by tests.
"Coverage" is each package's own |
04c3ca9 to
7bae0a7
Compare
| const intrinsic = getContainerHeight(undefined, !!footerComponent, footerVariant); | ||
| const intrinsicFloor = typeof intrinsic === 'number' ? intrinsic : NODE_HEIGHT_DEFAULT; | ||
|
|
||
| return Math.max(intrinsicFloor, handleFloor); |
There was a problem hiding this comment.
It looks like this somehow affects the handle spacing so some scenarios are breaking:
- Handles become offset from the grid
- Handles grow out of node bounds
You can compare the existing storybook with preview storybook to see behavior differences.
Screen.Recording.2026-07-01.at.8.46.47.AM.mov
There was a problem hiding this comment.
Thanks Ben — addressed in ed95b26.
Root cause: handle spacing is computed from the nodeHeight prop, which is React Flow's measured height. When the height write-back was removed, that value was no longer tied to the handle-count floor — and because the stories/nodes set an explicit node.height (e.g. the Dynamic Handles story's height: 96), the measured height never grew to the floor. So the handles were distributed over the wrong height (off-grid, and overflowing the node bounds on high-fan-out nodes like the 3-input/7-output switch).
Fix: computedHeight (a pure function of handle count + footer) is now written back to node.height, so React Flow's measured height matches the rendered body and the grid math (calculateGridAlignedHandlePositions) gets the correct height. No feedback loop is reintroduced, because computedHeight never reads the measured height.
Could you re-check against the preview storybook once the new build is up?
7bae0a7 to
ed95b26
Compare
| // Write computedHeight to node.height and recalculate handle positions. Compare | ||
| // against node.height (not the measured `height` prop) so a lagging measurement | ||
| // can't retrigger the write. | ||
| // biome-ignore lint/correctness/useExhaustiveDependencies: handleConfigurations triggers handle recalculation. | ||
| useEffect(() => { | ||
| if (computedHeight !== undefined && computedHeight !== height) { | ||
| syncedHeightRef.current = computedHeight; | ||
| const frameId = requestAnimationFrame(() => { | ||
| updateNode(id, { height: computedHeight }); | ||
| updateNodeInternals(id); | ||
| }); | ||
|
|
||
| return () => { | ||
| cancelAnimationFrame(frameId); | ||
| }; | ||
| if (getNode(id)?.height !== computedHeight) { | ||
| updateNode(id, { height: computedHeight }); | ||
| } |
There was a problem hiding this comment.
Good catch — the PR description was stale and is now updated. The write-back is intentional and required: React Flow's node model (measured height, edge anchoring, fit-view, selection bounds, and handle spacing) all read node.height, so we do need to keep it in sync.
The key change from the previous flickering version is that computedHeight is a pure function of handle count + footer and never reads the measured height prop — so writing it back cannot feed into its own input. That's what breaks the old feedback loop, not the removal of the write. The guard compares against the explicit node.height (via getNode), not the measured prop, so a lagging ResizeObserver measurement can't retrigger the write.
| @@ -378,7 +353,7 @@ const BaseNodeComponent = (props: NodeProps<Node<BaseNodeData>>) => { | |||
|
|
|||
| return { | |||
| '--node-w': `${containerWidth}px`, | |||
| '--node-h': typeof containerHeight === 'number' ? `${containerHeight}px` : 'auto', | |||
| '--node-h': `${containerHeight}px`, | |||
| '--node-radius': nodeRadius, | |||
There was a problem hiding this comment.
Correct, and the description is updated to match. Height is deliberately handle/footer-driven, not content-driven: computedHeight is written to node.height as the authoritative size, and BaseNodeContainer applies it as a fixed h-(--node-h). The earlier min-h-(--node-h) h-auto experiment was reverted for exactly the consistency reason you note — with an authoritative node.height, a fixed height keeps the body, the React Flow wrapper, and the measured height in agreement. Node content (icon/label, or fixed-height footer variants) is expected to fit within the computed height by construction.
ed95b26 to
ad23359
Compare
Summary
Fixes the Switch-node height flicker (MST-11677): deleting a case from an unconnected Switch made it oscillate between the 2-handle (96px) and 3-handle (128px) sizes. Root cause was a feedback loop where
BaseNodederived its height from the measuredheightprop and wrote it back viaupdateNodeinside arequestAnimationFrame.Changes
computedHeightis now a pure function of visible handle count + intrinsic footer/default height (getIntrinsicHeight). It never reads the measuredheightprop, so the value written back can't feed into its own input — the loop is structurally gone.useEffectwritescomputedHeighttonode.height(no rAF), only when it differs from the explicitgetNode(id)?.height, then callsupdateNodeInternals. Comparing against the explicit height (not the measured prop) means a lagging ResizeObserver measurement can't retrigger the write. Convergent and idempotent.node.heightauthoritative fixes the downstream model: measured height, edge anchoring, fit-view, selection bounds, and handle spacing all agree with the rendered body (fixes handles overflowing on high-fan-out nodes).animatedViewportManagernow readsnode.measured?.height ?? node.height ?? …, matching the package's dominant convention (previouslynode.height || 100, which went stale when height wasn't written).baseHeightRef/syncedHeightRefbookkeeping and the deadgetContainerHeightheight param.Flow
flowchart TD A[handleConfigurations / footer] --> B[computedHeight - pure] B --> C{getNode.height != computedHeight?} C -- yes --> D[updateNode height + updateNodeInternals] C -- no --> E[no-op: converged] D --> F[node.height authoritative] F --> G[measured height / edges / handle spacing agree]Testing
pnpm lint(Biome) passespnpm typecheckpassespnpm test:visual— runs in CIas any/ type suppressions added