diff --git a/application/single_app/config.py b/application/single_app/config.py index 459b487f3..7f2400651 100644 --- a/application/single_app/config.py +++ b/application/single_app/config.py @@ -100,7 +100,7 @@ EXECUTOR_TYPE = 'thread' EXECUTOR_MAX_WORKERS = 30 SESSION_TYPE = 'filesystem' -VERSION = "0.261.204" +VERSION = "0.261.205" IS_DEVELOPMENT = is_development_env_enabled() # Opt-out for deployments where App Service Easy Auth is active but the platform diff --git a/application/v2_ui/src/components/workflows/WorkflowFlowCanvas.tsx b/application/v2_ui/src/components/workflows/WorkflowFlowCanvas.tsx index 20926432e..36d0518b8 100644 --- a/application/v2_ui/src/components/workflows/WorkflowFlowCanvas.tsx +++ b/application/v2_ui/src/components/workflows/WorkflowFlowCanvas.tsx @@ -1,13 +1,14 @@ // WorkflowFlowCanvas.tsx // A local presentation-only renderer. Semantic edits belong to the owning editor. -import { memo, useCallback, useEffect, useId, useMemo, useRef, useState, type Dispatch, type KeyboardEvent, type SetStateAction, type TouchEvent } from 'react'; +import { memo, useCallback, useEffect, useId, useLayoutEffect, useMemo, useRef, useState, type Dispatch, type KeyboardEvent, type SetStateAction, type TouchEvent } from 'react'; import { - Handle, MarkerType, Position, ReactFlow, + Handle, MarkerType, Position, ReactFlow, useUpdateNodeInternals, type Edge, type Node, type NodeChange, type NodeProps, type ReactFlowInstance, } from '@xyflow/react'; import { GlassButton } from '../ui/primitives'; import { layoutWorkflowFlow, visibleWorkflowEdges, visibleWorkflowNode, type WorkflowFlowStructure } from '../../lib/workflowFlowLayout'; +import { reuseUnchangedFlowNodes } from '../../lib/workflowFlowNodeReuse'; import { workflowInspectionBindings, type WorkflowInspectionDetails, type WorkflowInspectionNode } from '../../lib/workflowInspection'; import type { WorkflowExecutionRecord } from '../../lib/workflowExecutionHistory'; import '@xyflow/react/dist/style.css'; @@ -34,8 +35,16 @@ interface FlowNodeData extends Record { type FlowNode = Node; -const DefinitionNode = memo(function DefinitionNode({ data }: NodeProps) { +const DefinitionNode = memo(function DefinitionNode({ id, data }: NodeProps) { const node = data.record; + const updateNodeInternals = useUpdateNodeInternals(); + const chosen = useRef(data.chosen); + useEffect(() => { + if (chosen.current === data.chosen) return; + chosen.current = data.chosen; + // The selected border is wider, so it moves every handle without resizing the node. + updateNodeInternals(id); + }, [data.chosen, id, updateNodeInternals]); return
@@ -79,6 +88,7 @@ const DefinitionNode = memo(function DefinitionNode({ data }: NodeProps) { if (event.touches.length > 1) event.stopPropagation(); @@ -177,6 +187,19 @@ export function WorkflowFlowCanvas({ } }, [boxes, records, collapsed, onCollapse, onFocus]); + // Nodes call the latest handlers through stable wrappers, so a new callback identity alone + // never rebuilds a node (#1573). + const latest = useRef({ onSelect, onCollapse, onFocus, onNavigate }); + useLayoutEffect(() => { + latest.current = { onSelect, onCollapse, onFocus, onNavigate }; + }); + const handlers = useMemo(() => ({ + onSelect: (id: string) => latest.current.onSelect(id), + onCollapse: (id: string) => latest.current.onCollapse(id), + onFocus: (id: string, part?: 'collapse', reveal?: boolean) => latest.current.onFocus(id, part, reveal), + onNavigate: (id: string, event: KeyboardEvent) => latest.current.onNavigate(id, event), + }), []); + useEffect(() => { if (focusRequest) { buttons.current.get(focusRequest.id)?.focus({ preventScroll: true }); @@ -184,9 +207,15 @@ export function WorkflowFlowCanvas({ } }, [focusRequest]); - const nodes: FlowNode[] = useMemo(() => boxes.map((box) => { + // React Flow drops the handle positions of a node object it has not seen unless the object + // carries its measured size. Unchanged nodes keep their previous object; changed nodes carry + // the last size React Flow reported, so their edges stay drawn while it re-measures (#1573). + const measuredSizes = useRef(new Map()); + const previousNodes = useRef([]); + const nodes: FlowNode[] = useMemo(() => reuseUnchangedFlowNodes(previousNodes.current, boxes.map((box): FlowNode => { const record = records.get(box.id); if (!record) throw new Error('The Flow layout lost a canonical node.'); + const measured = measuredSizes.current.get(box.id); return { id: box.id, type: 'workflow', parentId: box.parentId, position: positions.get(box.id) ?? box.position, width: box.width, height: box.height, style: { width: box.width, height: box.height }, @@ -197,11 +226,20 @@ export function WorkflowFlowCanvas({ record, container: box.container, collapsed: collapsed.has(box.id), chosen: selectedId === box.id, tabStop: (focusedId && boxMap.has(focusedId) ? focusedId : boxes[0]?.id) === box.id, status: statuses.get(box.id) ?? (sourceKind === 'run' ? 'Not loaded' : 'Definition'), - onSelect, onFocus, onNavigate, onCollapse, registerButton, + ...handlers, registerButton, }, + ...(measured ? { measured } : {}), }; - }), [boxes, records, positions, collapsed, selectedId, focusedId, boxMap, statuses, dragPan, - sourceKind, onSelect, onFocus, onNavigate, onCollapse, registerButton]); + }), ['measured']), [boxes, records, positions, collapsed, selectedId, focusedId, boxMap, statuses, dragPan, + sourceKind, handlers, registerButton]); + useLayoutEffect(() => { + previousNodes.current = nodes; + }, [nodes]); + useEffect(() => { + for (const id of [...measuredSizes.current.keys()]) { + if (!boxMap.has(id)) measuredSizes.current.delete(id); + } + }, [boxMap]); const edges: Edge[] = useMemo(() => { const connections = new Map }>(); @@ -259,7 +297,10 @@ export function WorkflowFlowCanvas({ return [...control, ...[...dataConnections.values()].map(({ edge }) => edge)]; }, [projection, collapsed, records, observations, details, selectedId, bindingRelationships]); - const changePositions = useCallback((changes: NodeChange[]) => { + const changeNodes = useCallback((changes: NodeChange[]) => { + for (const change of changes) { + if (change.type === 'dimensions' && change.dimensions) measuredSizes.current.set(change.id, change.dimensions); + } const moved = changes.filter((change) => change.type === 'position' && change.position !== undefined); if (!moved.length) return; setPositions((current) => { @@ -270,6 +311,10 @@ export function WorkflowFlowCanvas({ return next; }); }, [boxMap, setPositions]); + const initFlow = useCallback((flow: ReactFlowInstance) => { + instance.current = flow; + }, []); + const reportGeometryError = useCallback(() => setError('Flow geometry is unavailable. Use List or reload the view.'), []); const moveSelected = (dx: number, dy: number) => { const box = selectedId ? boxMap.get(selectedId) : undefined; @@ -292,7 +337,7 @@ export function WorkflowFlowCanvas({ return
- void instance.current?.fitView({ padding: 0.15, maxZoom: 1, duration: 0 })}>Fit Flow + void instance.current?.fitView({ ...fitViewOptions, duration: 0 })}>Fit Flow void instance.current?.zoomIn({ duration: 0 })}>Zoom in void instance.current?.zoomOut({ duration: 0 })}>Zoom out pan(120, 0)}>Pan view left @@ -315,15 +360,15 @@ export function WorkflowFlowCanvas({ id={`workflow-flow-${helpId.replaceAll(':', '')}`} nodes={nodes} edges={edges} nodeTypes={nodeTypes} - onInit={(flow) => { instance.current = flow; }} - onNodesChange={changePositions} - onError={() => setError('Flow geometry is unavailable. Use List or reload the view.')} + onInit={initFlow} + onNodesChange={changeNodes} + onError={reportGeometryError} nodesConnectable={false} edgesReconnectable={false} nodesFocusable={false} edgesFocusable={false} elementsSelectable={false} selectNodesOnDrag={false} deleteKeyCode={null} selectionKeyCode={null} multiSelectionKeyCode={null} zoomActivationKeyCode={null} panActivationKeyCode={null} panOnDrag={dragPan} zoomOnScroll={false} zoomOnPinch={false} zoomOnDoubleClick={false} preventScrolling={false} - minZoom={0.005} maxZoom={2} fitView fitViewOptions={{ padding: 0.15, maxZoom: 1 }} + minZoom={0.005} maxZoom={2} fitView fitViewOptions={fitViewOptions} defaultMarkerColor={null} />
diff --git a/application/v2_ui/src/lib/workflowFlowNodeReuse.ts b/application/v2_ui/src/lib/workflowFlowNodeReuse.ts new file mode 100644 index 000000000..892bf4e17 --- /dev/null +++ b/application/v2_ui/src/lib/workflowFlowNodeReuse.ts @@ -0,0 +1,59 @@ +// workflowFlowNodeReuse.ts +// Keeps a React Flow node object when a rebuild produced an equal node. +// +// React Flow treats a node object it has not seen before as a new node: it throws away the +// handle positions it measured, and every edge to that node disappears until it is measured +// again. Rebuilding the node list therefore returns the previous object for each node that did +// not change, and the previous list itself when no node changed, so React Flow has nothing to +// adopt again. + +type PlainRecord = Record; + +function isPlainRecord(value: unknown): value is PlainRecord { + if (value === null || typeof value !== 'object') return false; + const prototype = Object.getPrototypeOf(value); + return prototype === Object.prototype || prototype === null; +} + +function hasKey(record: PlainRecord, key: string) { + return Object.prototype.hasOwnProperty.call(record, key); +} + +function sameEntries(left: PlainRecord, right: PlainRecord, same: (a: unknown, b: unknown) => boolean, + ignored: readonly string[] = []): boolean { + const keys = Object.keys(left).filter((key) => !ignored.includes(key)); + return keys.length === Object.keys(right).filter((key) => !ignored.includes(key)).length && + keys.every((key) => hasKey(right, key) && same(left[key], right[key])); +} + +function sameValue(left: unknown, right: unknown) { + if (Object.is(left, right)) return true; + return isPlainRecord(left) && isPlainRecord(right) && sameEntries(left, right, Object.is); +} + +/** + * Two nodes are equal when they have the same fields with identical values. A plain-object field, + * such as data, style or position, is compared one level down, so a literal rebuilt with the same + * values is still equal. Arrays, maps and deeper objects are compared by identity. Ignored fields + * never make two nodes different. + */ +export function sameFlowNode(previous: object, next: object, ignored: readonly string[] = []): boolean { + return previous === next || sameEntries(previous as PlainRecord, next as PlainRecord, sameValue, ignored); +} + +/** + * Returns the rebuilt nodes with every node that equals the previous node of the same ID replaced + * by that previous object. When every node is kept in the same order, returns the previous array. + */ +export function reuseUnchangedFlowNodes(previous: T[], next: T[], + ignored: readonly string[] = []): T[] { + const byId = new Map(previous.map((node) => [node.id, node])); + let unchanged = previous.length === next.length; + const nodes = next.map((node, index) => { + const prior = byId.get(node.id); + const kept = prior !== undefined && sameFlowNode(prior, node, ignored) ? prior : node; + if (kept !== previous[index]) unchanged = false; + return kept; + }); + return unchanged ? previous : nodes; +} diff --git a/docs/explanation/fixes/V2_WORKFLOW_FLOW_CANVAS_STABLE_RERENDERS_FIX.md b/docs/explanation/fixes/V2_WORKFLOW_FLOW_CANVAS_STABLE_RERENDERS_FIX.md new file mode 100644 index 000000000..114fab183 --- /dev/null +++ b/docs/explanation/fixes/V2_WORKFLOW_FLOW_CANVAS_STABLE_RERENDERS_FIX.md @@ -0,0 +1,228 @@ +# V2 Workflow Flow Canvas Stable Re-renders Fix (v0.261.205) + +**Fixed in version: 0.261.205** + +The application version is tracked in `application/single_app/config.py`. + +Fixes #1573. Found during the Chat Orchestration Workflows work (#1543). + +## Issue + +The V2 workflow editor's **Flow** surface and the read-only Flow view draw the +workflow with React Flow. Their execution arrows (control edges) disappeared +whenever the page re-rendered for a reason that didn't change the diagram: the +**Saving…** state, a failed save, typing in a field outside the diagram, or a +runtime poll of a running workflow that returned the same run. Usually the arrows +came back a frame or two later, once React Flow measured the blocks again. +Sometimes they didn't come back at all until something else made React Flow +measure the blocks. + +The UI test +`ui_tests/test_v2_workflow_flow_authoring.py::test_save_failures_retain_flow_fields_and_never_replace_the_original_cas_revision[400|409|503]` +edits two fields and then waits, with Playwright's default 5 seconds, for a control +edge (`expect_compiled`, line 282). Each variant timed out at least once in +full-suite runs, including on the V2 base, and passed when run alone. + +## Root cause + +### Every render rebuilt every node + +`WorkflowFlowCanvas` builds React Flow's node objects in a `useMemo`. Its +dependencies included the four handlers every node calls: `onSelect`, +`onCollapse`, and the canvas's own `onFocus` and `onNavigate`. The editor passes +new functions on every render: + +- `WorkflowEditorDialog` passes `onSelect` as an inline arrow. +- `WorkflowFlowAuthoring` passes `collapse`, a plain function, as `onCollapse`. + The canvas's `onNavigate` depends on `onCollapse`, so it changed as well. + +So every render of the editor built a new object for every node, even when the +diagram hadn't changed. Real changes did the same: selecting a block or typing in a +task rebuilt every node, not only the ones that changed. + +### A new node object loses its handle positions + +In `@xyflow/react` 12.11.6, `adoptUserNodes` keeps React Flow's internal node only +when it receives the same node object again. For any other object without a +`measured` size, `parseHandles` drops the node's handle bounds, and `EdgeWrapper` +draws no edge whose handles have no bounds. Each rebuild therefore removed every +arrow from the page, and the arrows came back only after the node's +ResizeObserver measured it again. On a diagram that shows 12 control edges, the +new regression spec counts 24 removed and added again while a save is pending. + +### A lost measurement leaves the arrows missing + +React Flow observes a node again only when the node's initialized state changes +(`useNodeObserver`). Two rebuilds in a row can lose that measurement: + +1. A rebuild drops the handle bounds, so React Flow observes the node again. +2. A second rebuild renders at default priority and commits. Its passive effect, + which gives React Flow the new node objects, hasn't run yet. +3. The ResizeObserver notification from step 1 arrives, and React Flow stores the + handle bounds. That store update renders synchronously, so React first runs the + pending passive effect, which drops the handle bounds again. +4. The node was uninitialized before and is uninitialized now, so React Flow + doesn't observe it again. Its arrows stay missing. + +In the flaky test, the second field edit renders the editor synchronously, which +rebuilds every node. The compiler preview effect then sets `loaded` to `null`, +which renders `WorkflowFlowAuthoring` again at default priority with a new +`collapse` and rebuilds every node a second time. A ResizeObserver notification +that lands between that render's commit and its passive effects leaves the arrows +missing, and `expect_compiled` times out at line 282. + +## Fix + +All changes are in `WorkflowFlowCanvas.tsx` and a new helper, +`lib/workflowFlowNodeReuse.ts`. The editor dialog, `WorkflowFlowAuthoring` and +`WorkflowFlowView` are unchanged, so the fix covers both the editor and the +read-only Flow view. + +### Stable handlers + +The canvas keeps the latest `onSelect`, `onCollapse`, `onFocus` and `onNavigate` +in a ref, updated after every commit, and gives the nodes stable wrappers that +call them. A new handler identity no longer rebuilds a node, and a click or a key +still reaches the editor's current state. + +### Unchanged nodes keep their object + +`reuseUnchangedFlowNodes` compares each rebuilt node with the previous node of the +same ID. Fields are compared by identity, and plain-object fields such as `data`, +`style` and `position` are compared one level down. A node that didn't change +keeps its previous object, and when no node changed the canvas passes React Flow +its previous array, so React Flow has nothing to adopt again. A change to one +block, such as its selection, now rebuilds only the nodes whose values changed. + +### Changed nodes keep their measured size + +The canvas records the size React Flow reports for each node (`dimensions` +changes in `onNodesChange`), and a rebuilt node carries it as `measured`. +React Flow then keeps the node's handle bounds instead of dropping them, so a real +change, such as typing in a task, a new compiler status or a collapse, no longer +removes the node's arrows while React Flow measures it again. Sizes of nodes that +leave the diagram are forgotten. `measured` is ignored when nodes are compared, so +recording a size never rebuilds a node. + +### Selection re-measures its node + +A selected block has a 2px border instead of 1px. That moves its handles without +changing its size, so no ResizeObserver notification follows. Before this fix, the +rebuild dropped the handle bounds, which forced a new measurement. Now each node +asks React Flow to measure it again (`useUpdateNodeInternals`) when its selection +changes, so its arrows follow the new border. + +### Stable React Flow props + +`onInit`, `onError` and `fitViewOptions` were new on every render. They are now a +`useCallback` each and a module constant, so a re-render gives `` no new +props when nothing changed. + +### What still rebuilds nodes + +A real change to a block's values still builds a new object for that block: its +record, label, status, selection, collapse, focus or position. Typing in a task +changes the draft, so every record is new and every node is rebuilt. Each rebuilt +node carries its measured size, so its arrows stay drawn. + +### Files modified + +| File | Change | +| --- | --- | +| `application/v2_ui/src/components/workflows/WorkflowFlowCanvas.tsx` | Stable handler wrappers, node reuse, recorded sizes carried as `measured`, the selection re-measure, and stable `onInit`, `onError` and `fitViewOptions` | +| `application/v2_ui/src/lib/workflowFlowNodeReuse.ts` | New: `sameFlowNode` and `reuseUnchangedFlowNodes` | +| `functional_tests/test_v2_workflow_flow_node_reuse_logic.mjs` | New: unit tests for the helper | +| `ui_tests/test_v2_workflow_flow_canvas_stability.py` | New: browser regression tests | +| `application/single_app/config.py` | Version 0.261.205 | + +## Testing + +### Regression spec + +`ui_tests/test_v2_workflow_flow_canvas_stability.py` runs the production bundle +against the closed Flow harnesses and the real compiler. It installs a +MutationObserver on the canvas and counts every control edge and React Flow node +removed from or added to the page, and the fewest control edges shown: + +- `test_non_diagram_rerenders_keep_every_flow_node_and_edge_mounted[400|409|503]` + holds a save open, lets it fail with 400, 409 or 503, and types in the decision + field builder with `page.keyboard`. Nothing may be removed or added, and typing + must not revalidate the draft. Afterwards, keyboard selection, collapse, arrow + key navigation and **Return to selected block** must still reach the latest + editor state, and every control edge must still start and end on its node's + handles. +- `test_focus_and_selection_changes_keep_control_edges_mounted_and_on_their_handles` + moves focus and selection with the keyboard. No control edge may leave the page, + and every control edge must end on its handles after the wider selected border. +- `test_rapid_focus_and_selection_changes_leave_every_control_edge_drawn` fits + the view, then clicks and presses keys faster than React Flow measures. Once the + canvas settles, every control edge must be drawn, on its handles, with no + further churn. +- `test_runtime_polls_keep_read_only_flow_nodes_and_edges_mounted` opens the + read-only Flow of a running workflow and waits for two runtime polls that return + the same run. Nothing may be removed or added. + +### Reproducing the flake + +The flake needs a ResizeObserver notification to land inside a short window, so +it rarely shows on a fast machine. A session-only pytest plugin, not committed, +repeated each test 20 times and made the window measurable with two options: + +- CPU throttling slows the page 4 times through the Chrome DevTools Protocol + (`Emulation.setCPUThrottlingRate`). +- A race amplifier, an init script, holds each ResizeObserver notification until a + microtask after the next React commit, or one more frame at most. A browser may + deliver a notification at that point, so it makes a rare but legal order common. + +Every failure of the save-failure test on the base, with or without the amplifier, +was the control edge wait at line 282, exactly as in the full-suite runs. + +The amplifier also holds React Flow's pane-size notifications. Under it, the first +fit showed the diagram at full size, with only the root block's button reachable, +on both the base and the fix. The rapid test therefore fits the view itself before +it clicks. + +### Unit tests + +`functional_tests/test_v2_workflow_flow_node_reuse_logic.mjs` checks, with +`node --test`, that equal rebuilds are reused, that a changed value, key or deeper +identity isn't, that ignored fields don't count, that the previous array is kept +only when every node is kept in order, and that neither input is changed. + +## Validation + +"Before" is the V2 bundle at `c33ad4d16`, and "after" is the bundle built from +this change. Both ran in Chromium against the offline Playwright harness. + +- The regression spec failed 5 of its 6 tests on the base, each on its churn + assertion. 24 control edges were removed and redrawn while a save was pending + and failing, 72 during focus and selection changes, and 24 across two runtime + polls, and each time there was a moment with no control edge drawn. The rapid + test passed on the base. With the fix, all 6 pass. +- With the fix, `ui_tests/test_v2_workflow_flow_authoring.py` passed all 30 tests + in each of two runs. The Flow inspection (53), authoring history (28), change + tracking (24), editor (9), calendar schedule (14), control flow (10) and M365 + runtime (8) specs pass. +- `node --test functional_tests/test_v2_workflow_flow_node_reuse_logic.mjs` + passes 10 of 10, and `functional_tests/test_v2_workflow_change_tracking.py` + passes 4 of 4. `npm run typecheck` and `npm run build` pass. + +The stress rows repeated each test 20 times with the session-only plugin described +under **Reproducing the flake**. + +| | Before | After | +| --- | --- | --- | +| Save, failed save (400, 409 or 503), typing outside the diagram | Every control edge removed and redrawn | Nothing removed | +| Runtime poll that returns the same run | Every control edge removed and redrawn | Nothing removed | +| Focus and selection changes | Every control edge removed and redrawn | Only the changed nodes are rebuilt; no control edge removed | +| Save-failure test, 4× CPU throttle and race amplifier, 60 runs | 60 failed, all at the control edge wait (line 282) | None failed | +| Save-failure test, 4× CPU throttle, 60 runs | 3 failed, all at the control edge wait (line 282) | None failed | +| Rapid selection test, 4× CPU throttle and race amplifier, 20 runs | 2 failed, both at the control edge wait (line 282) while opening the Flow | None failed | + +## Known limitations + +A node React Flow has never measured has no size to carry. If it is rebuilt at +default priority before its first measurement, and the measurement lands between +that render's commit and its passive effects, its arrows can still go missing, as +before. A new node is normally measured within a frame of being added, and none of +the stress runs showed this. diff --git a/docs/explanation/release_notes.md b/docs/explanation/release_notes.md index 1b45f983d..17a0f505d 100644 --- a/docs/explanation/release_notes.md +++ b/docs/explanation/release_notes.md @@ -2,6 +2,16 @@ For feature-focused and fix-focused drill-downs by version, see [Features by Version](https://github.com/microsoft/simplechat/tree/main/docs/explanation/features) and [Fixes by Version](https://github.com/microsoft/simplechat/tree/main/docs/explanation/fixes). +### **(v0.261.205)** + +#### Bug Fixes + +* **Workflow Flow Arrows Stay Drawn While The Page Updates** + * The execution arrows in the V2 workflow editor's **Flow** surface disappeared whenever the editor re-rendered without changing the diagram: while saving, after a failed save, or while typing in a field outside the diagram. The read-only Flow view did the same on every runtime poll of a running workflow. The arrows usually came back once React Flow measured the blocks again, but a measurement that arrived between two re-renders could leave them missing, which also made the Flow save-failure UI test flaky. + * The Flow canvas now calls the editor's latest handlers through stable wrappers and keeps each unchanged block's React Flow node between renders, so a re-render that doesn't change the diagram changes nothing on the canvas. A block that does change carries the size React Flow last measured, so its arrows stay drawn while it is measured again, and a block whose selection changes is measured again because its wider border moves its handles. + * Selection, collapse, keyboard navigation, focus requests, dragged positions and change tracking behave as before. + * (Ref: #1573, #1543, `WorkflowFlowCanvas.tsx`, `workflowFlowNodeReuse.ts`, [V2 Workflow Flow Canvas Stable Re-renders Fix](fixes/V2_WORKFLOW_FLOW_CANVAS_STABLE_RERENDERS_FIX.md)) + ### **(v0.261.204)** #### User Interface Enhancements diff --git a/functional_tests/test_v2_workflow_flow_node_reuse_logic.mjs b/functional_tests/test_v2_workflow_flow_node_reuse_logic.mjs new file mode 100644 index 000000000..3405c61c1 --- /dev/null +++ b/functional_tests/test_v2_workflow_flow_node_reuse_logic.mjs @@ -0,0 +1,132 @@ +// test_v2_workflow_flow_node_reuse_logic.mjs +// Version: 0.261.205 +// Implemented in: 0.261.205 +// Executes the node-reuse rule the workflow Flow canvas uses to keep React Flow node objects +// stable across renders that do not change the diagram (#1573). +// +// React Flow discards the measured handle positions of any node object it has not seen before, +// and every edge to that node disappears until it is measured again. The canvas therefore keeps +// the previous object for each node a rebuild did not change, and the previous array when no +// node changed. Run with: node --test functional_tests/test_v2_workflow_flow_node_reuse_logic.mjs + +import assert from 'node:assert/strict'; +import test from 'node:test'; +import './test_support/tsResolve.mjs'; + +// The repository resolver must be registered before TypeScript imports load. +const { reuseUnchangedFlowNodes, sameFlowNode } = await import( + '../application/v2_ui/src/lib/workflowFlowNodeReuse.ts' +); + +const record = { id: 'evaluate', kind: 'task', label: 'Evaluate', child_region_ids: [] }; +const onSelect = () => {}; +const registerButton = () => {}; + +function flowNode(id, fields = {}, data = {}) { + return { + id, type: 'workflow', parentId: undefined, position: { x: 10, y: 20 }, + width: 280, height: 96, style: { width: 280, height: 96 }, + draggable: true, extent: undefined, + data: { record, chosen: false, status: 'Definition', onSelect, registerButton, ...data }, + ...fields, + }; +} + +test('a node rebuilt with the same values equals the previous node', () => { + assert.equal(sameFlowNode(flowNode('evaluate'), flowNode('evaluate')), true); + const node = flowNode('evaluate'); + assert.equal(sameFlowNode(node, node), true); +}); + +test('a changed value, field or key makes the node different', () => { + const previous = flowNode('evaluate'); + assert.equal(sameFlowNode(previous, flowNode('evaluate', {}, { chosen: true })), false); + assert.equal(sameFlowNode(previous, flowNode('evaluate', { position: { x: 11, y: 20 } })), false); + assert.equal(sameFlowNode(previous, flowNode('evaluate', { style: { width: 300, height: 96 } })), false); + assert.equal(sameFlowNode(previous, flowNode('evaluate', { draggable: false })), false); + assert.equal(sameFlowNode(previous, flowNode('evaluate', { hidden: true })), false); + const { draggable, ...withoutDraggable } = previous; + assert.equal(draggable, true); + assert.equal(sameFlowNode(previous, withoutDraggable), false); + assert.equal(sameFlowNode(withoutDraggable, previous), false); +}); + +test('values below the first level and callbacks are compared by identity', () => { + const previous = flowNode('evaluate'); + assert.equal(sameFlowNode(previous, flowNode('evaluate', {}, { record: { ...record } })), false); + assert.equal(sameFlowNode(previous, flowNode('evaluate', {}, { onSelect: () => {} })), false); + assert.equal(sameFlowNode(flowNode('evaluate', {}, { tags: ['a'] }), flowNode('evaluate', {}, { tags: ['a'] })), false); + const tags = ['a']; + assert.equal(sameFlowNode(flowNode('evaluate', {}, { tags }), flowNode('evaluate', {}, { tags })), true); +}); + +test('ignored fields never make two nodes different', () => { + const previous = flowNode('evaluate'); + const measured = flowNode('evaluate', { measured: { width: 280, height: 96 } }); + assert.equal(sameFlowNode(previous, measured), false); + assert.equal(sameFlowNode(previous, measured, ['measured']), true); + assert.equal(sameFlowNode(measured, previous, ['measured']), true); + assert.equal(sameFlowNode(measured, flowNode('evaluate', { measured: { width: 300, height: 96 } }), ['measured']), true); + assert.equal(sameFlowNode(previous, flowNode('evaluate', { measured: { width: 280, height: 96 } }, { chosen: true }), + ['measured']), false); +}); + +test('an unchanged rebuild returns the previous array itself', () => { + const previous = [flowNode('root'), flowNode('evaluate'), flowNode('finish')]; + const next = [flowNode('root'), flowNode('evaluate'), flowNode('finish')]; + assert.equal(reuseUnchangedFlowNodes(previous, next), previous); + assert.equal(reuseUnchangedFlowNodes([], []).length, 0); +}); + +test('only changed nodes receive new objects', () => { + const previous = [flowNode('root'), flowNode('evaluate'), flowNode('finish')]; + const next = [flowNode('root'), flowNode('evaluate', {}, { chosen: true }), flowNode('finish')]; + const nodes = reuseUnchangedFlowNodes(previous, next); + assert.notEqual(nodes, previous); + assert.equal(nodes[0], previous[0]); + assert.equal(nodes[1], next[1]); + assert.equal(nodes[2], previous[2]); +}); + +test('a reorder keeps every object but returns a new array', () => { + const previous = [flowNode('root'), flowNode('evaluate'), flowNode('finish')]; + const nodes = reuseUnchangedFlowNodes(previous, [flowNode('finish'), flowNode('root'), flowNode('evaluate')]); + assert.notEqual(nodes, previous); + assert.deepEqual(nodes.map((node) => node.id), ['finish', 'root', 'evaluate']); + assert.equal(nodes[0], previous[2]); + assert.equal(nodes[1], previous[0]); + assert.equal(nodes[2], previous[1]); +}); + +test('added nodes are new and removed nodes are dropped', () => { + const previous = [flowNode('root'), flowNode('evaluate'), flowNode('finish')]; + const added = flowNode('review'); + const grown = reuseUnchangedFlowNodes(previous, [flowNode('root'), flowNode('evaluate'), added, flowNode('finish')]); + assert.deepEqual(grown.map((node) => node.id), ['root', 'evaluate', 'review', 'finish']); + assert.equal(grown[0], previous[0]); + assert.equal(grown[2], added); + assert.equal(grown[3], previous[2]); + const shrunk = reuseUnchangedFlowNodes(previous, [flowNode('root'), flowNode('finish')]); + assert.notEqual(shrunk, previous); + assert.deepEqual(shrunk, [previous[0], previous[2]]); + const prefix = reuseUnchangedFlowNodes(previous, [flowNode('root'), flowNode('evaluate')]); + assert.notEqual(prefix, previous); + assert.deepEqual(prefix, [previous[0], previous[1]]); +}); + +test('a newly measured node still reuses the previous object', () => { + const previous = [flowNode('root'), flowNode('evaluate')]; + const next = [flowNode('root', { measured: { width: 280, height: 96 } }), flowNode('evaluate')]; + assert.equal(reuseUnchangedFlowNodes(previous, next, ['measured']), previous); + assert.notEqual(reuseUnchangedFlowNodes(previous, next), previous); +}); + +test('the inputs are never modified', () => { + const previous = [flowNode('root'), flowNode('evaluate')]; + const next = [flowNode('evaluate', {}, { chosen: true }), flowNode('root')]; + const before = [previous.slice(), next.slice()]; + reuseUnchangedFlowNodes(previous, next); + assert.deepEqual([previous, next], before); + assert.equal(previous[0], before[0][0]); + assert.equal(next[0], before[1][0]); +}); diff --git a/ui_tests/test_v2_workflow_flow_canvas_stability.py b/ui_tests/test_v2_workflow_flow_canvas_stability.py new file mode 100644 index 000000000..e0e71cbda --- /dev/null +++ b/ui_tests/test_v2_workflow_flow_canvas_stability.py @@ -0,0 +1,333 @@ +# test_v2_workflow_flow_canvas_stability.py +""" +Offline real-bundle browser regressions for a stable workflow Flow canvas (#1573). +Version: 0.261.205 +Implemented in: 0.261.205 + +A re-render that doesn't change the diagram must not rebuild the React Flow nodes. New node +objects reset React Flow's measured handle bounds, so every edge is removed from the page +until the nodes are measured again, and a later rebuild can leave the edges missing. These +tests watch the canvas with a MutationObserver while the editor saves, fails a save and takes +typing outside the diagram, and while a read-only run Flow receives runtime polls. No node or +control edge may be removed or added. The canvas handlers must still reach the latest editor +state afterwards: pointer and keyboard selection, collapse, keyboard navigation and a focus +request. + +A focus or selection change rebuilds only the nodes it changes, and those keep their measured +handles, so no control edge leaves the page. Rapid focus and selection changes, which rebuild +the same nodes again before React Flow measures them, must leave every control edge drawn once +the canvas settles. + +Reuses the closed Flow harnesses, the local production assets and the real compiler. No live +app, model, workflow admission, publication or Azure browser is contacted. Run with +PLAYWRIGHT_SERVICE_URL='' and PYTHONPATH including ui_tests/fixtures. +""" + +import re +import sys +from pathlib import Path +from urllib.parse import urlsplit + +import pytest +from playwright.sync_api import expect + +ROOT = Path(__file__).resolve().parents[1] +sys.path.insert(0, str(ROOT)) +sys.path.insert(0, str(ROOT / "ui_tests")) +sys.path.insert(0, str(ROOT / "ui_tests" / "fixtures")) + +# The shared fixtures import pure application helpers after setting their paths. +from ui_tests import test_v2_workflow_flow_authoring as authoring +from ui_tests.fixtures.workflow_flow import FLOW_NAME, FLOW_RUN_ID, FLOW_WORKFLOW_ID, workflow_flow_ui # noqa: F401 +from ui_tests.test_v2_workflow_flow_authoring import authoring_ui, connect_options # noqa: F401 + + +pytestmark = pytest.mark.ui + +EDGE = ".workflow-flow-control-edge" + +# Counts every control edge and React Flow node removed from or added to the canvas, including +# those inside a removed or added element, and the fewest control edges the canvas showed. +INSTALL_PROBE = """ +(root) => { + window.__flowCanvasChurn?.observer.disconnect(); + const selectors = { edges: '.workflow-flow-control-edge', nodes: '.react-flow__node' }; + const count = (element, selector) => element.nodeType === Node.ELEMENT_NODE + ? Number(element.matches(selector)) + element.querySelectorAll(selector).length : 0; + const state = { + removedEdges: 0, addedEdges: 0, removedNodes: 0, addedNodes: 0, + fewestEdges: root.querySelectorAll(selectors.edges).length, + }; + const record = (mutations) => { + for (const mutation of mutations) { + for (const element of mutation.removedNodes) { + state.removedEdges += count(element, selectors.edges); + state.removedNodes += count(element, selectors.nodes); + } + for (const element of mutation.addedNodes) { + state.addedEdges += count(element, selectors.edges); + state.addedNodes += count(element, selectors.nodes); + } + } + state.fewestEdges = Math.min(state.fewestEdges, root.querySelectorAll(selectors.edges).length); + }; + const observer = new MutationObserver(record); + observer.observe(root, { childList: true, subtree: true }); + window.__flowCanvasChurn = { observer, record, state }; + return state.fewestEdges; +} +""" + +READ_PROBE = """ +() => { + const probe = window.__flowCanvasChurn; + probe.record(probe.observer.takeRecords()); + return { ...probe.state }; +} +""" + +NEXT_FRAMES = "() => new Promise((resolve) => requestAnimationFrame(() => requestAnimationFrame(resolve)))" + +# Returns every drawn control edge whose ends are not on a node handle, measured the way React +# Flow measures handles. An edge drawn from stale handle bounds misses its handle. +DETACHED_EDGES = """ +(root) => { + const viewport = root.querySelector('.react-flow__viewport'); + const zoom = new DOMMatrixReadOnly(getComputedStyle(viewport).transform).m22; + const anchors = { source: [], target: [] }; + for (const node of root.querySelectorAll('.react-flow__node')) { + const [x, y] = node.style.transform.match(/-?[\\d.]+/g).map(Number); + const bounds = node.getBoundingClientRect(); + for (const handle of node.querySelectorAll('.react-flow__handle')) { + const rect = handle.getBoundingClientRect(); + const left = x + (rect.left - bounds.left) / zoom; + const top = y + (rect.top - bounds.top) / zoom; + const width = handle.offsetWidth; + const height = handle.offsetHeight; + const side = handle.dataset.handlepos; + const point = side === 'top' ? [left + width / 2, top] + : side === 'bottom' ? [left + width / 2, top + height] + : side === 'left' ? [left, top + height / 2] : [left + width, top + height / 2]; + anchors[handle.classList.contains('source') ? 'source' : 'target'].push(point); + } + } + const near = (points, [x, y]) => points.some(([px, py]) => Math.abs(px - x) <= 0.5 && Math.abs(py - y) <= 0.5); + return [...root.querySelectorAll('.workflow-flow-control-edge')].flatMap((edge) => { + const path = edge.querySelector('.react-flow__edge-path').getAttribute('d'); + const numbers = path.match(/-?\\d*\\.?\\d+(?:e[-+]?\\d+)?/gi).map(Number); + const start = numbers.slice(0, 2); + const end = numbers.slice(-2); + return near(anchors.source, start) && near(anchors.target, end) ? [] : [{ edge: edge.dataset.id, start, end }]; + }); +} +""" + +# The node buttons a pointer can reach at their centers, in canvas order. +CLICKABLE_NODES = """ +(root) => [...root.querySelectorAll('button[data-workflow-node-id]')].flatMap((button) => { + const bounds = button.getBoundingClientRect(); + const x = bounds.left + bounds.width / 2; + const y = bounds.top + bounds.height / 2; + return button.contains(document.elementFromPoint(x, y)) ? [{ id: button.dataset.workflowNodeId, x, y }] : []; +}) +""" + + +def settle(page, delay=400): + """Leave time for React effects, a debounced preview and a ResizeObserver pass to happen.""" + page.evaluate(NEXT_FRAMES) + page.wait_for_timeout(delay) + page.evaluate(NEXT_FRAMES) + + +def install_probe(page, view): + settle(page) + edges = view.locator(".react-flow").evaluate(INSTALL_PROBE) + assert edges > 0, "The canvas must show control edges before it is watched." + return edges + + +def assert_no_churn(page, edges, moment): + settle(page) + churn = page.evaluate(READ_PROBE) + assert churn == { + "removedEdges": 0, "addedEdges": 0, "removedNodes": 0, "addedNodes": 0, "fewestEdges": edges, + }, f"The Flow canvas rebuilt its nodes {moment}: {churn}" + + +def assert_edges_meet_handles(view, moment): + settle(view.page) + detached = view.locator(".react-flow").evaluate(DETACHED_EDGES) + assert detached == [], f"Control edges miss their node handles {moment}: {detached}" + + +def is_save(request): + return request.method == "POST" and urlsplit(request.url).path == "/api/user/workflows" + + +def compiled_status(view): + return view.get_by_role("status").filter(has_text=re.compile(r"^Compiler-validated draft\.")) + + +def node_order(view): + return view.locator(".react-flow__node").evaluate_all("nodes => nodes.map((node) => node.dataset.id)") + + +def open_compiled_flow(ui): + editor = authoring.open_editor(ui) + view = authoring.switch_surface(editor, "Flow") + authoring.expect_compiled(view) + return view + + +@pytest.mark.parametrize("status", [400, 409, 503]) +def test_non_diagram_rerenders_keep_every_flow_node_and_edge_mounted(authoring_ui, status): + ui, page = authoring_ui, authoring_ui.page + editor = authoring.open_editor(ui) + view = authoring.switch_surface(editor, "Flow") + fields = authoring.select_node(view, "evaluate") + fields.get_by_label("Task name", exact=True).fill("Kept while the editor re-renders") + fields.get_by_label("Instructions", exact=True).fill("Keep this unsaved instruction while the editor re-renders.") + authoring.open_details(fields) + authoring.expect_compiled(view) + edges = install_probe(page, view) + previews = len(ui.preview_requests) + + # Saving and a failed save re-render the editor without changing the diagram. + if status == 409: + ui.mutate_revision(FLOW_WORKFLOW_ID) + else: + ui.reject_next("POST", "/api/user/workflows", status=status, error="Fictional authoring save was rejected.") + ui.hold_save_response = True + with page.expect_request(is_save): + editor.get_by_role("button", name="Save workflow", exact=True).click() + try: + expect(editor.get_by_role("button", name="Saving…", exact=True)).to_be_disabled() + assert_no_churn(page, edges, "while saving") + finally: + ui.release_save_responses() + expect(editor.get_by_role("alert").first).to_be_visible() + expect(editor.get_by_role("button", name="Save workflow", exact=True)).to_be_enabled() + assert_no_churn(page, edges, f"after the failed {status} save") + + # The decision field builder changes a field draft, not the previewed definition. + builder = fields.get_by_label("Decision field name", exact=True) + builder.click() + page.keyboard.type("extra") + expect(builder).to_have_value("extra") + expect(builder).to_be_focused() + assert_no_churn(page, edges, "while typing outside the diagram") + assert len(ui.preview_requests) == previews, "Typing in the field builder must not revalidate the draft." + expect(compiled_status(view)).to_be_visible() + expect(fields.get_by_label("Task name", exact=True)).to_have_value("Kept while the editor re-renders") + + # The canvas handlers still reach the latest editor state after those re-renders. + finish = authoring.node_button(view, "finish") + finish.focus() + page.keyboard.press("Enter") + expect(finish).to_have_attribute("aria-pressed", "true") + expect(authoring.configuration(view).get_by_text("Canonical ID: finish", exact=True)).to_be_visible() + choose = authoring.node_button(view, "choose") + toggle = view.locator(".react-flow__node[data-id='choose'] button.workflow-flow-collapse") + expect(toggle).to_have_attribute("aria-expanded", "true") + toggle.focus() + page.keyboard.press("Enter") + expect(toggle).to_have_attribute("aria-expanded", "false") + expect(choose).to_have_attribute("aria-pressed", "true") + expect(authoring.configuration(view).get_by_text("Canonical ID: choose", exact=True)).to_be_visible() + choose.focus() + page.keyboard.press("ArrowRight") + expect(toggle).to_have_attribute("aria-expanded", "true") + expect(choose).to_be_focused() + order = node_order(view) + following = order[order.index("choose") + 1] + assert following != "bypass-note", "An expanded If block must list its own regions next." + page.keyboard.press("ArrowDown") + expect(authoring.node_button(view, following)).to_be_focused() + authoring.configuration(view).get_by_role("button", name="Return to selected block", exact=True).click() + expect(choose).to_be_focused() + expect(view.locator(EDGE)).to_have_count(edges) + assert_edges_meet_handles(view, "after the canvas handlers ran") + assert not ui.workflow_writes + + +def test_focus_and_selection_changes_keep_control_edges_mounted_and_on_their_handles(authoring_ui): + ui, page = authoring_ui, authoring_ui.page + view = open_compiled_flow(ui) + order = node_order(view) + assert len(order) > 4 + edges = install_probe(page, view) + + # Focus moves the canvas tab stop, and Enter selects a node, which widens its border. + authoring.node_button(view, order[0]).focus() + for key in ("ArrowDown", "Enter", "ArrowDown", "ArrowDown", "Enter", "ArrowUp"): + page.keyboard.press(key) + settle(page, 100) + expect(authoring.node_button(view, order[3])).to_have_attribute("aria-pressed", "true") + expect(authoring.node_button(view, order[1])).to_have_attribute("aria-pressed", "false") + expect(authoring.node_button(view, order[2])).to_be_focused() + assert_no_churn(page, edges, "on focus and selection changes") + assert_edges_meet_handles(view, "after the selection changed") + assert not ui.workflow_writes + + +def test_rapid_focus_and_selection_changes_leave_every_control_edge_drawn(authoring_ui): + ui, page = authoring_ui, authoring_ui.page + view = open_compiled_flow(ui) + order = node_order(view) + # Fit the view again so the clicks below don't depend on when the initial fit ran. + view.get_by_role("button", name="Fit Flow", exact=True).click() + edges = install_probe(page, view) + + # Each click and key rebuilds nodes again before React Flow has measured the last rebuild. + # The clicks come first: keyboard focus reveals a node at full size, which hides the rest. + reachable = view.locator(".react-flow").evaluate(CLICKABLE_NODES) + clicks = (reachable[::2] + reachable[1::2])[:5] + assert len(clicks) >= 2, f"The fitted canvas must show at least two clickable nodes: {reachable}" + for click in clicks: + page.mouse.click(click["x"], click["y"]) + authoring.node_button(view, order[0]).focus() + for key in ["ArrowDown"] * 6 + ["ArrowUp"] * 6 + ["ArrowDown", "Enter", "ArrowUp", "Enter"] * 2: + page.keyboard.press(key) + expect(authoring.node_button(view, order[0])).to_have_attribute("aria-pressed", "true") + expect(authoring.node_button(view, order[0])).to_be_focused() + + settle(page) + expect(view.locator(EDGE)).to_have_count(edges) + assert install_probe(page, view) == edges + page.wait_for_timeout(1000) + assert_no_churn(page, edges, "after rapid focus and selection changes settled") + assert_edges_meet_handles(view, "after rapid focus and selection changes settled") + assert not ui.workflow_writes + + +def test_runtime_polls_keep_read_only_flow_nodes_and_edges_mounted(workflow_flow_ui): + ui, page = workflow_flow_ui, workflow_flow_ui.page + key = ("user", FLOW_WORKFLOW_ID, FLOW_RUN_ID) + # An active durable run makes the runtime panel poll, with an equal runtime every time. + ui.workflow_runs[FLOW_WORKFLOW_ID][0]["status"] = "running" + ui.workflow_runtimes[key]["state"] = "running" + ui.open("/workspace/workflows") + row = page.get_by_role("listitem").filter(has=page.get_by_role( + "button", name=f"View Flow for {FLOW_NAME}", exact=True, + )).first + row.get_by_role("button", name="Show run history", exact=True).click() + row.get_by_role("button", name="Show run task results", exact=True).click() + row.get_by_role("button", name="Show Flow for this run", exact=True).click() + view = page.get_by_role("region", name="Workflow Flow", exact=True) + expect(view.get_by_text("Run's frozen definition", exact=True)).to_be_visible() + expect(view.get_by_text("Loading bounded execution overlay...", exact=True)).to_have_count(0) + expect(view.locator(EDGE).first).to_be_attached() + edges = install_probe(page, view) + polls = ui.runtime_get_count[key] + + def is_runtime_poll(response): + return response.request.method == "GET" and urlsplit(response.url).path.endswith(f"/runs/{FLOW_RUN_ID}/runtime") + + for _ in range(2): + with page.expect_response(is_runtime_poll, timeout=10_000): + pass + assert ui.runtime_get_count[key] >= polls + 2 + assert_no_churn(page, edges, "on runtime polls that did not change the run") + expect(view.get_by_role("alert")).to_have_count(0) + ui.assert_read_only()