From ad23359a783b8b7c8758f0fcdda4b01358222e18 Mon Sep 17 00:00:00 2001 From: Kodudula Ashish Reddy Date: Wed, 1 Jul 2026 18:58:39 +0530 Subject: [PATCH] fix(apollo-react): compute node height from handle count to stop flicker [MST-11677] --- .../AgentCanvas/nodes/AgentNode.test.tsx | 4 +- .../components/BaseNode/BaseNode.test.tsx | 146 +++++++++++------- .../canvas/components/BaseNode/BaseNode.tsx | 73 +++------ .../canvas/stores/animatedViewportManager.ts | 12 +- 4 files changed, 123 insertions(+), 112 deletions(-) diff --git a/packages/apollo-react/src/canvas/components/AgentCanvas/nodes/AgentNode.test.tsx b/packages/apollo-react/src/canvas/components/AgentCanvas/nodes/AgentNode.test.tsx index 1d1b65101..d440ff734 100644 --- a/packages/apollo-react/src/canvas/components/AgentCanvas/nodes/AgentNode.test.tsx +++ b/packages/apollo-react/src/canvas/components/AgentCanvas/nodes/AgentNode.test.tsx @@ -1,9 +1,9 @@ import { render, screen } from '@testing-library/react'; import type { ReactElement } from 'react'; import { describe, expect, it, vi } from 'vitest'; -import { BaseCanvasModeProvider } from '../../BaseCanvas/BaseCanvasModeProvider'; import { NodeRegistryProvider } from '../../../core/NodeRegistryProvider'; import type { AgentNodeTranslations } from '../../../types'; +import { BaseCanvasModeProvider } from '../../BaseCanvas/BaseCanvasModeProvider'; import { agentFlowManifest } from '../agent-flow.manifest'; import { AgentNodeElement } from './AgentNode'; @@ -26,6 +26,8 @@ vi.mock('@uipath/apollo-react/canvas/xyflow/react', () => ({ useReactFlow: () => ({ setNodes: vi.fn(), setEdges: vi.fn(), + updateNode: vi.fn(), + getNode: vi.fn(), }), })); diff --git a/packages/apollo-react/src/canvas/components/BaseNode/BaseNode.test.tsx b/packages/apollo-react/src/canvas/components/BaseNode/BaseNode.test.tsx index dbc6fdfa8..3dac23d85 100644 --- a/packages/apollo-react/src/canvas/components/BaseNode/BaseNode.test.tsx +++ b/packages/apollo-react/src/canvas/components/BaseNode/BaseNode.test.tsx @@ -12,6 +12,8 @@ const DEFAULT_MANIFEST = { // Hoisted mocks — available inside vi.mock factories const { mockUpdateNode, + mockGetNode, + mockNodeState, mockHandleConfigs, mockManifest, mockUseButtonHandles, @@ -22,6 +24,9 @@ const { mockNodeToolbar, } = vi.hoisted(() => ({ mockUpdateNode: vi.fn(), + mockGetNode: vi.fn(), + // Simulates React Flow's stored node: updateNode writes height here, getNode reads it. + mockNodeState: { current: { height: undefined as number | undefined } }, mockHandleConfigs: { current: undefined as HandleGroupManifest[] | undefined }, mockManifest: { current: { @@ -39,12 +44,25 @@ const { mockNodeToolbar: vi.fn() as any, })); +// getNode reflects whatever updateNode last wrote, so the height-sync effect converges +// exactly as it does against React Flow's real store. +mockGetNode.mockImplementation(() => mockNodeState.current); +mockUpdateNode.mockImplementation((_id: string, patch: { height?: number }) => { + if (patch?.height !== undefined) { + mockNodeState.current = { ...mockNodeState.current, height: patch.height }; + } +}); + // xyflow is globally mocked in canvas-mocks.ts; extend with test-specific overrides. vi.mock('@uipath/apollo-react/canvas/xyflow/react', async (importOriginal) => ({ ...(await importOriginal()), useStore: () => mockIsConnecting.current, useUpdateNodeInternals: () => vi.fn(), - useReactFlow: () => ({ updateNodeData: vi.fn(), updateNode: mockUpdateNode }), + useReactFlow: () => ({ + updateNodeData: vi.fn(), + updateNode: mockUpdateNode, + getNode: mockGetNode, + }), })); vi.mock('../../hooks', () => ({ @@ -155,6 +173,8 @@ const makeHandles = (position: Position, count: number): HandleGroupManifest[] = describe('BaseNode', () => { afterEach(() => { mockUpdateNode.mockClear(); + mockGetNode.mockClear(); + mockNodeState.current = { height: undefined }; mockUseButtonHandles.mockClear(); mockNodeToolbar.mockClear(); mockHandleConfigs.current = undefined; @@ -165,85 +185,95 @@ describe('BaseNode', () => { mockIsConnecting.current = false; }); - describe('Height computation', () => { - // Override the rAF mock from canvas-mocks to run synchronously - // so updateNode calls complete during the effect flush within act/render. - const savedRAF = window.requestAnimationFrame; - beforeEach(() => { - window.requestAnimationFrame = (cb) => { - cb(performance.now()); - return 0; - }; - }); - afterEach(() => { - window.requestAnimationFrame = savedRAF; + // The handle count sets `--node-h` (the node height), which is also written to + // React Flow's node.height so measured height, edges, and handle spacing agree. + // It is a pure function of handles/footer (never reads the measured height), so the + // write-back is idempotent and converges (no flicker loop). + // Height = max(96 default | footer height, (maxHandlesPerSide * 2 + 2) * 16). + describe('Height computation (floor + node.height sync)', () => { + // Read the `--node-h` CSS var off the outer wrapper (parent of base-container). + const floorVar = () => + screen.getByTestId('base-container').parentElement?.style.getPropertyValue('--node-h'); + + it('sets the floor to the 96px default when handles fit within it', () => { + // 2 handles → (2*2+2)*16 = 96 == default + mockHandleConfigs.current = makeHandles(Position.Left, 2); + render(); + expect(floorVar()).toBe('96px'); }); - it('expands height when handles exceed base height', () => { - // 4 handles × 40px = 160px > 96px default + it('expands the floor to fit the handle count', () => { + // 4 handles → (4*2+2)*16 = 160 > 96 default mockHandleConfigs.current = makeHandles(Position.Left, 4); - render(); - expect(mockUpdateNode).toHaveBeenCalledWith('test-node', { height: 160 }); + render(); + expect(floorVar()).toBe('160px'); }); - it('does not expand when handles fit within base height', () => { - // 2 handles × 40px = 80px < 96px default - mockHandleConfigs.current = makeHandles(Position.Left, 2); - render(); - expect(mockUpdateNode).not.toHaveBeenCalled(); + it('uses max of left and right handle counts', () => { + // 2 left, 4 right → uses 4 → 160px + mockHandleConfigs.current = [ + ...makeHandles(Position.Left, 2), + ...makeHandles(Position.Right, 4), + ]; + render(); + expect(floorVar()).toBe('160px'); }); - it('respects user-provided height as the base', () => { - // height=200, 2 handles need 80px < 200 → no expansion + it('ignores the measured height prop when computing the floor', () => { mockHandleConfigs.current = makeHandles(Position.Left, 2); - render(); - expect(mockUpdateNode).not.toHaveBeenCalled(); + const { rerender } = render(); + expect(floorVar()).toBe('96px'); + rerender(); + expect(floorVar()).toBe('96px'); }); - it('shrinks back to default when handles are removed', () => { - // Start with 4 handles (160px needed) - mockHandleConfigs.current = makeHandles(Position.Left, 4); - const { rerender } = render(); - expect(mockUpdateNode).toHaveBeenCalledWith('test-node', { height: 160 }); + it('writes the floor to node.height and converges without repeat writes', () => { + // 3 handles → 128px floor, written once to node.height. + mockHandleConfigs.current = makeHandles(Position.Left, 3); + const { rerender } = render(); + expect(mockUpdateNode).toHaveBeenCalledWith('test-node', { height: 128 }); + expect(mockUpdateNode).toHaveBeenCalledTimes(1); - // Simulate React Flow updating height to match our sync - // Note: new data={{}} ref on each rerender to break through React.memo + // Re-render with the handle set unchanged: node.height already equals the floor, + // so the guard skips the write — no oscillation. mockUpdateNode.mockClear(); - rerender(); + rerender(); expect(mockUpdateNode).not.toHaveBeenCalled(); + }); - // Remove handles — should shrink back to DEFAULT_NODE_SIZE (96) - mockHandleConfigs.current = []; - mockUpdateNode.mockClear(); - rerender(); - expect(mockUpdateNode).toHaveBeenCalledWith('test-node', { height: 96 }); + it('does not write when node.height already equals the floor', () => { + // Persisted node already at the correct height → no redundant write on mount. + mockNodeState.current = { height: 96 }; + mockHandleConfigs.current = makeHandles(Position.Left, 2); + render(); + expect(mockUpdateNode).not.toHaveBeenCalled(); }); - it('shrinks back to user-provided height after handle removal', () => { - // User height=200, 8 handles need 288px - mockHandleConfigs.current = makeHandles(Position.Left, 8); - const { rerender } = render(); - expect(mockUpdateNode).toHaveBeenCalledWith('test-node', { height: 288 }); + it('deflates the floor and settles when a handle is removed (MST-11677)', () => { + mockHandleConfigs.current = makeHandles(Position.Left, 3); + const { rerender } = render(); + expect(floorVar()).toBe('128px'); + expect(mockUpdateNode).toHaveBeenLastCalledWith('test-node', { height: 128 }); - // Simulate React Flow updating height + // Remove a handle → floor drops to 96, written once, then settles. mockUpdateNode.mockClear(); - rerender(); + mockHandleConfigs.current = makeHandles(Position.Left, 2); + rerender(); + expect(floorVar()).toBe('96px'); + expect(mockUpdateNode).toHaveBeenCalledWith('test-node', { height: 96 }); - // Remove handles — should return to 200, not DEFAULT_NODE_SIZE (96) - mockHandleConfigs.current = []; mockUpdateNode.mockClear(); - rerender(); - expect(mockUpdateNode).toHaveBeenCalledWith('test-node', { height: 200 }); + rerender(); + expect(floorVar()).toBe('96px'); + expect(mockUpdateNode).not.toHaveBeenCalledWith('test-node', { height: 128 }); + expect(mockUpdateNode).not.toHaveBeenCalled(); }); - it('uses max of left and right handle counts', () => { - // 2 left, 4 right → uses 4 → 160px - mockHandleConfigs.current = [ - ...makeHandles(Position.Left, 2), - ...makeHandles(Position.Right, 4), - ]; - render(); - expect(mockUpdateNode).toHaveBeenCalledWith('test-node', { height: 160 }); + it('uses the fixed footer height as the floor when it exceeds the handle floor', () => { + mockOverrideConfig.current = { footerComponent: 'footer', footerVariant: 'single' }; + mockHandleConfigs.current = makeHandles(Position.Left, 2); + render(); + expect(floorVar()).toBe('160px'); }); }); diff --git a/packages/apollo-react/src/canvas/components/BaseNode/BaseNode.tsx b/packages/apollo-react/src/canvas/components/BaseNode/BaseNode.tsx index 517f587d9..4b291c934 100644 --- a/packages/apollo-react/src/canvas/components/BaseNode/BaseNode.tsx +++ b/packages/apollo-react/src/canvas/components/BaseNode/BaseNode.tsx @@ -63,11 +63,11 @@ const getContainerWidth = (shape: NodeShape | undefined, width: number | undefin return defaultWidth; }; -const getContainerHeight = ( - height: number | undefined, +// Intrinsic height: the fixed footer height when a footer is present, else the default. +const getIntrinsicHeight = ( hasFooter: boolean, footerVariant: FooterVariant | undefined -): number | 'auto' => { +): number => { if (hasFooter) { switch (footerVariant) { case 'button': @@ -76,13 +76,9 @@ const getContainerHeight = ( return NODE_HEIGHT_FOOTER_SINGLE; case 'double': return NODE_HEIGHT_FOOTER_DOUBLE; - default: - return 'auto'; } } - // Use `||` rather than `??` — React Flow can pass `height: 0` before measurement, - // and we want that to fall through to the default rather than collapse the node. - return height || NODE_HEIGHT_DEFAULT; + return NODE_HEIGHT_DEFAULT; }; const BaseNodeComponent = (props: NodeProps>) => { @@ -111,22 +107,13 @@ const BaseNodeComponent = (props: NodeProps>) => { } = useBaseNodeOverrideConfig(); const updateNodeInternals = useUpdateNodeInternals(); - const { updateNodeData, updateNode } = useReactFlow(); + const { updateNodeData, updateNode, getNode } = useReactFlow(); const containerRef = useRef(null); const [isHovered, setIsHovered] = useState(false); const [isFocused, setIsFocused] = useState(false); - // Tracks the height we last synced to React Flow via updateNode, so we can - // distinguish our own handle-based writes from external changes (user-specified - // height, React Flow measurement, resize). - const syncedHeightRef = useRef(undefined); - // The "base" height — what the node height would be without handle inflation. - // Updated when height changes from an external source (not our sync). - const baseHeightRef = useRef(DEFAULT_NODE_SIZE); - - if (height && height !== syncedHeightRef.current) { - baseHeightRef.current = height; - } + // Height is driven by computedHeight (below), a pure function of handle count/footer. + // It never reads the measured height, so writing it back can't loop. // Get execution status from external source const executionState = useNodeExecutionState(id); @@ -260,10 +247,9 @@ const BaseNodeComponent = (props: NodeProps>) => { }; }, [adornmentsProp, statusContext]); - // Compute height: max of base height (user-specified or measured) and handle minimum. - // baseHeightRef is updated above from external height changes; handle inflation - // is computed from the current handleConfigurations. - // biome-ignore lint/correctness/useExhaustiveDependencies: height updates baseHeightRef above and intentionally retriggers this memo. + // Computed node height: max of the handle-count floor and the intrinsic default/footer + // height. Pure (never reads the measured `height`); written to node.height below as + // the authoritative size, so node content is expected to fit within it. const computedHeight = useMemo(() => { const leftHandles = handleConfigurations .filter((config) => config.position === Position.Left && config.visible !== false) @@ -282,31 +268,21 @@ const BaseNodeComponent = (props: NodeProps>) => { const leftRightHandles = Math.max(leftHandles, rightHandles); // Each handle gets a 2-grid-space lane (32px), plus 2-grid-space padding at top + bottom of node. - const minNodeHeight = (leftRightHandles * 2 + 2) * GRID_SPACING; - return Math.max(baseHeightRef.current, minNodeHeight); - }, [handleConfigurations, height]); + const handleFloor = (leftRightHandles * 2 + 2) * GRID_SPACING; - // biome-ignore lint/correctness/useExhaustiveDependencies: handle configuration changes require React Flow handle recalculation. - useEffect(() => { - updateNodeInternals(id); - }, [handleConfigurations, id, updateNodeInternals]); + return Math.max(getIntrinsicHeight(!!footerComponent, footerVariant), handleFloor); + }, [handleConfigurations, footerComponent, footerVariant]); - // Sync computed height to node when it differs from React Flow's current value + // 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 }); } - - return undefined; - }, [computedHeight, height, id, updateNode, updateNodeInternals]); + updateNodeInternals(id); + }, [handleConfigurations, computedHeight, id, getNode, updateNode, updateNodeInternals]); const displayLabel = display.label; const displayShape = display.shape ?? 'square'; @@ -320,7 +296,6 @@ const BaseNodeComponent = (props: NodeProps>) => { // Display customization from props (not data) const displayLabelTooltip = labelTooltip; const displayLabelBackgroundColor = labelBackgroundColor; - const displayFooterVariant = footerVariant; // SubLabel: Component prop takes precedence, then plain string from display. const displaySubLabel = useMemo(() => { @@ -343,10 +318,10 @@ const BaseNodeComponent = (props: NodeProps>) => { // --------------------------------------------------------------------------- const hasFooter = !!displayFooter; const containerWidth = getContainerWidth(displayShape, width); - const containerHeight = getContainerHeight(height, hasFooter, displayFooterVariant); + const containerHeight = computedHeight; const nodeVars = useMemo((): React.CSSProperties => { - const numH = typeof containerHeight === 'number' ? containerHeight : DEFAULT_NODE_SIZE; + const numH = containerHeight; const getRadius = (basis: number, ratio: number) => { return displayShape === 'circle' @@ -378,7 +353,7 @@ const BaseNodeComponent = (props: NodeProps>) => { return { '--node-w': `${containerWidth}px`, - '--node-h': typeof containerHeight === 'number' ? `${containerHeight}px` : 'auto', + '--node-h': `${containerHeight}px`, '--node-radius': nodeRadius, '--node-gap': `${(gap - NODE_BORDER_SIZE * 2) / 2}px`, '--inner-w': `${innerW}px`, diff --git a/packages/apollo-react/src/canvas/stores/animatedViewportManager.ts b/packages/apollo-react/src/canvas/stores/animatedViewportManager.ts index bd337b118..f20728b4f 100644 --- a/packages/apollo-react/src/canvas/stores/animatedViewportManager.ts +++ b/packages/apollo-react/src/canvas/stores/animatedViewportManager.ts @@ -237,8 +237,10 @@ class AnimatedViewportManager { const nodeBounds: NodeBounds = { x: node.position.x, y: node.position.y, - width: node.width || 200, - height: node.height || 100, + // Prefer measured dimensions: node width/height are content-driven and may be + // unset in the store until React Flow's ResizeObserver reports them. + width: node.measured?.width ?? node.width ?? 200, + height: node.measured?.height ?? node.height ?? 100, }; // Calculate the viewport that focuses on the node with padding @@ -335,8 +337,10 @@ class AnimatedViewportManager { return { x: node.position.x, y: node.position.y, - width: node.width || 200, - height: node.height || 100, + // Prefer measured dimensions: node width/height are content-driven and may be + // unset in the store until React Flow's ResizeObserver reports them. + width: node.measured?.width ?? node.width ?? 200, + height: node.measured?.height ?? node.height ?? 100, }; }