From 4eab04a7559321f34255cfc7011676ef50d69667 Mon Sep 17 00:00:00 2001 From: Cyrus Gray <144336577+graycyrus@users.noreply.github.com> Date: Fri, 17 Jul 2026 02:04:36 +0530 Subject: [PATCH] fix(flows): preserve canvas viewport and skip redundant remount on save (#4999) --- .../flows/canvas/EditableFlowCanvas.tsx | 60 ++++++++- .../components/flows/canvas/FlowCanvas.tsx | 21 ++- app/src/pages/FlowCanvasPage.tsx | 96 ++++++++++++-- .../pages/__tests__/FlowCanvasPage.test.tsx | 122 ++++++++++++++++++ 4 files changed, 285 insertions(+), 14 deletions(-) diff --git a/app/src/components/flows/canvas/EditableFlowCanvas.tsx b/app/src/components/flows/canvas/EditableFlowCanvas.tsx index 16ca19c73..05855c456 100644 --- a/app/src/components/flows/canvas/EditableFlowCanvas.tsx +++ b/app/src/components/flows/canvas/EditableFlowCanvas.tsx @@ -29,6 +29,7 @@ import { type ReactFlowInstance, useEdgesState, useNodesState, + type Viewport, } from '@xyflow/react'; import '@xyflow/react/dist/style.css'; import createDebug from 'debug'; @@ -186,6 +187,20 @@ export interface EditableFlowCanvasProps { * header (the canvas keeps only undo/redo). Fires whenever any field changes. */ onSaveMetaChange?: (meta: EditorSaveMeta) => void; + /** + * The viewport (pan/zoom) to restore on mount, captured from a previous + * mount of this same logical canvas via `onViewportChange` (F4/F5 fix). The + * host (`FlowCanvasPage`) keeps this in a ref that survives the `canvasVersion` + * remounts Save/Accept/Reject trigger, so a remount can restore the user's + * pan/zoom instead of `fitView` silently resetting it. `null`/absent means no + * prior viewport is known (first-ever mount) — `fitView` runs normally. + */ + savedViewport?: Viewport | null; + /** + * Fired on every viewport change (pan/zoom) so the host can capture the + * latest value for `savedViewport` on the next remount (F4/F5 fix). + */ + onViewportChange?: (viewport: Viewport) => void; } /** Save/Discard state the host header needs to render + gate its own buttons. */ @@ -199,6 +214,20 @@ export interface EditorSaveMeta { export interface EditableFlowCanvasHandle { save: () => void; discard: () => void; + /** + * Clear the forced-dirty flag and advance the baseline to the CURRENT live + * graph, without going through `save()` / `onSave` (the host has already + * persisted the graph itself — see `handleAcceptProposal` in + * `FlowCanvasPage.tsx`). Needed for the Accept path: it calls the page's + * `handleSave` directly (bypassing this canvas's own `save()`, whose ref is + * stale mid-remount) and only remounts a SECOND time when the server + * actually normalized the graph. When it doesn't (the common "echoed back + * unchanged" case), this already-mounted instance's `forcedDirty` — seeded + * `true` by Accept's own remount, before the persist resolved — would + * otherwise never clear, since only this canvas's own `save()`/`discard()` + * do that. Call this after such an out-of-band persist succeeds to sync it. + */ + clearForcedDirty: () => void; } const EMPTY_ID_SET: ReadonlySet = new Set(); @@ -219,6 +248,8 @@ function EditableFlowCanvas( initialDirty = false, showPalette = true, onSaveMetaChange, + savedViewport = null, + onViewportChange, }: EditableFlowCanvasProps, ref: ForwardedRef ) { @@ -227,6 +258,23 @@ function EditableFlowCanvas( const [edges, setEdges, onEdgesChange] = useEdgesState(initialEdges); const rfRef = useRef | null>(null); + // F4/F5 fix diagnostic: log once per mount whether this instance restores a + // caller-supplied viewport (`savedViewport`, threaded through from + // `FlowCanvasPage`'s `viewportRef`) or falls back to React Flow's own + // `fitView` (first-ever mount, or a host that doesn't track viewport). + useEffect(() => { + log( + 'mount: viewport %s x=%s y=%s zoom=%s', + savedViewport ? 'restored' : 'fitView-fallback', + savedViewport?.x, + savedViewport?.y, + savedViewport?.zoom + ); + // Mount-only — a later `savedViewport` prop change (from panning) must + // not re-log; only a fresh mount (new instance) should. + // eslint-disable-next-line react-hooks/exhaustive-deps + }, []); + // ── Undo / redo history ─────────────────────────────────────────────────── // A bounded past/future stack of {nodes, edges} snapshots so structural edits // (add / connect / delete / move) and config edits are recoverable without @@ -598,8 +646,13 @@ function EditableFlowCanvas( if (!dirty || saving) return; handleDiscard(); }, + clearForcedDirty: () => { + log('clearForcedDirty: host persisted externally — syncing baseline'); + setBaseline({ nodes, edges }); + setForcedDirty(false); + }, }), - [dirty, hasErrors, saving, saveDisabled, handleSave, handleDiscard] + [dirty, hasErrors, saving, saveDisabled, handleSave, handleDiscard, nodes, edges] ); // Mirror the Save-button state up so the header can render + gate its buttons. @@ -712,6 +765,7 @@ function EditableFlowCanvas( className="flow-canvas relative h-full w-full" data-testid="flow-canvas" data-editable="true" + data-viewport-restored={savedViewport ? 'true' : 'false'} onDrop={handleDrop} onDragOver={handleDragOver} onKeyDown={handleCanvasKeyDown}> @@ -790,7 +844,9 @@ function EditableFlowCanvas( nodesDraggable nodesConnectable elementsSelectable - fitView + fitView={!savedViewport} + defaultViewport={savedViewport ?? undefined} + onViewportChange={onViewportChange} panOnScroll zoomOnScroll> diff --git a/app/src/components/flows/canvas/FlowCanvas.tsx b/app/src/components/flows/canvas/FlowCanvas.tsx index 47d31029c..baf645840 100644 --- a/app/src/components/flows/canvas/FlowCanvas.tsx +++ b/app/src/components/flows/canvas/FlowCanvas.tsx @@ -12,7 +12,14 @@ * The `editable` prop defaults to `false` so every existing read-only consumer * (the `/flows/:id` viewer) keeps its exact behavior — only the builder opts in. */ -import { Background, BackgroundVariant, Controls, MiniMap, ReactFlow } from '@xyflow/react'; +import { + Background, + BackgroundVariant, + Controls, + MiniMap, + ReactFlow, + type Viewport, +} from '@xyflow/react'; import '@xyflow/react/dist/style.css'; import { forwardRef, memo, useMemo } from 'react'; @@ -70,6 +77,14 @@ export interface FlowCanvasProps { showPalette?: boolean; /** Reports Save-button state up so the host header can render Save/Discard. */ onSaveMetaChange?: (meta: EditorSaveMeta) => void; + /** + * The viewport (pan/zoom) to restore on mount (editable only, F4/F5 fix) — + * see `EditableFlowCanvas`'s `savedViewport` doc comment. Not threaded to + * the read-only viewer, which keeps its unconditional `fitView`. + */ + savedViewport?: Viewport | null; + /** Fired on every viewport change (editable only, F4/F5 fix). */ + onViewportChange?: (viewport: Viewport) => void; } const NODE_TYPES = { [FLOW_NODE_TYPE]: FlowNodeComponent }; @@ -130,6 +145,8 @@ const FlowCanvas = forwardRef( initialDirty, showPalette = true, onSaveMetaChange, + savedViewport, + onViewportChange, }: FlowCanvasProps, ref ) => { @@ -150,6 +167,8 @@ const FlowCanvas = forwardRef( initialDirty={initialDirty} showPalette={showPalette} onSaveMetaChange={onSaveMetaChange} + savedViewport={savedViewport} + onViewportChange={onViewportChange} /> ); } diff --git a/app/src/pages/FlowCanvasPage.tsx b/app/src/pages/FlowCanvasPage.tsx index e12d12e8e..183990196 100644 --- a/app/src/pages/FlowCanvasPage.tsx +++ b/app/src/pages/FlowCanvasPage.tsx @@ -17,6 +17,7 @@ * mounts a `HashRouter`, so full `useBlocker` interception isn't available — * the Back button is this page's only in-app navigation affordance.) */ +import type { Viewport } from '@xyflow/react'; import createDebug from 'debug'; import { useCallback, useEffect, useMemo, useRef, useState } from 'react'; import { useLocation, useNavigate, useParams } from 'react-router-dom'; @@ -277,6 +278,22 @@ function FlowEditor({ // Save/Discard now live in the page header; the canvas reports its Save state // up and exposes save()/discard() via this handle. const canvasRef = useRef(null); + // F4/F5 fix: the editable canvas is remounted on every Save/Accept/Reject + // (keyed on `canvasVersion` below), which previously wiped BOTH the + // pan/zoom viewport (React Flow's `fitView` refits on every mount) and the + // undo history. This ref lives on `FlowEditor` — keyed on `flow.id`, not + // `canvasVersion` — so it survives those remounts and can seed the fresh + // canvas's `defaultViewport`, skipping `fitView` and preserving pan/zoom + // across a Save. (The undo-history wipe is a known, accepted limitation — + // Save/Accept are commit points, so resetting undo there is semantically + // fine; see the PR description.) + const viewportRef = useRef(null); + const handleViewportChange = useCallback((vp: Viewport) => { + viewportRef.current = vp; + // Grep-friendly, numeric-only (no PII) — fires on every pan/zoom, so kept + // to plain numbers rather than the full `Viewport` object. + log('viewport capture: x=%d y=%d zoom=%d', vp.x, vp.y, vp.zoom); + }, []); const [saveMeta, setSaveMeta] = useState({ dirty: false, hasErrors: false, @@ -429,14 +446,21 @@ function FlowEditor({ // (schema migration, id defaults, port normalization, etc.) before // persisting, so the canonical saved shape can differ from what the client // sent. Re-sync the canvas draft from the RESPONSE (not just the just-sent - // `next`) and bump `canvasVersion` so the editable canvas re-seeds from the - // canonical persisted graph immediately — matching what a navigate-away- - // and-back remount would show, without requiring one. + // `next`) always; only bump `canvasVersion` — remounting the editable canvas + // to re-seed from the canonical persisted graph — when that response + // actually DIFFERS from what was sent (F4/F5 fix: an unconditional bump + // here wiped the canvas's undo history and viewport on every Save even when + // the server echoed the graph back unchanged, and double-remounted on + // Accept, which already bumps once for its own preview→draft transition). // // Declared ahead of `handleAcceptProposal` (below), which calls it directly // to persist an accepted proposal immediately. const handleSave = useCallback( - async (next: WorkflowGraph, overrideName?: string, overrideRequireApproval?: boolean) => { + async ( + next: WorkflowGraph, + overrideName?: string, + overrideRequireApproval?: boolean + ): Promise<{ remounted: boolean }> => { // `overrideName` covers the copilot-Accept call site: it calls // `setName(proposal.name)` and `handleSave(...)` in the same handler, // but `name` in THIS closure is still the pre-update value — React @@ -463,7 +487,9 @@ function FlowEditor({ const created = await createFlow(effectiveName, next, effectiveRequireApproval); log('save: draft persisted as flow id=%s', created.id); navigate(`/flows/${created.id}`, { replace: true }); - return; + // Navigating replaces this whole page (new `flowId` route param), so + // "remounted" is moot for a draft-create — no caller branches on it. + return { remounted: false }; } // Only include `name` / `requireApproval` in the update payload when // they actually diverge from what's already persisted (a manual @@ -486,6 +512,10 @@ function FlowEditor({ ...(requireApprovalChanged ? { requireApproval: effectiveRequireApproval } : {}), }); const persisted = updated.graph as WorkflowGraph; + // `persistedGraphRef`/`setDraftGraph` run UNCONDITIONALLY regardless of + // whether a remount fires below — the dirty diff (`initialDirty`, B21) + // is computed against `persistedGraphRef`, not the canvas's own mount + // baseline, so it stays correct either way. Only the remount is gated. persistedGraphRef.current = persisted; persistedNameRef.current = updated.name; setDraftGraph(persisted); @@ -496,17 +526,42 @@ function FlowEditor({ setName(updated.name); setTitleDraft(updated.name); } - setCanvasVersion(v => v + 1); + // F4/F5 fix: only remount the canvas (wiping its undo history and, + // pre-Fix-A, its viewport) when the server actually normalized the + // graph into something different from what was sent (issue B21 — schema + // migration, id defaults, port normalization, etc.). When the response + // is a byte-for-byte echo of `next` (the common case), re-seeding from + // it would be a no-op remount — most visibly on Accept, whose own + // preview→draft transition already bumps `canvasVersion` once, so a + // guaranteed-normalized-away Save bump right after it was a pure + // double-remount (undo wipe + dirty flash) with no behavioral upside. + const graphChanged = JSON.stringify(persisted) !== JSON.stringify(next); + if (graphChanged) { + setCanvasVersion(v => v + 1); + } log( - 'save: flow id=%s persisted — canvas re-synced from response nodes=%d edges=%d', + 'save: flow id=%s persisted — canvas re-synced from response nodes=%d edges=%d graphChanged=%s', flowId, persisted.nodes.length, - persisted.edges.length + persisted.edges.length, + graphChanged ); + return { remounted: graphChanged }; }, [isDraft, flowId, name, requireApproval, navigate] ); + // Adapter for the canvas's own `onSave` prop, whose type (`void | + // Promise`) is shared with the read-only viewer and every other + // consumer — `handleSave`'s richer `{ remounted }` return (needed by + // `handleAcceptProposal` below) isn't part of that contract. + const onCanvasSave = useCallback( + async (next: WorkflowGraph) => { + await handleSave(next); + }, + [handleSave] + ); + const handleGraphChange = useCallback( (next: WorkflowGraph) => { // Freeze the draft while a proposal is under review — the preview graph @@ -590,8 +645,25 @@ function FlowEditor({ // `canvasVersion` bump above) so the ref's imperative handle is stale; // call `handleSave` directly with the known-good proposed graph. try { - await handleSave(proposedGraph, overrideName, proposal.requireApproval); - log('copilot proposal accepted: persisted'); + const { remounted } = await handleSave( + proposedGraph, + overrideName, + proposal.requireApproval + ); + // The canvas remounted once already (this handler's own bump above) + // with `forcedDirty` seeded `true` — correct pre-persist, but that + // instance's `forcedDirty` is only ever cleared by ITS OWN save()/ + // discard(), neither of which fires here (we persisted directly, + // above). A second remount (`remounted === true`, server actually + // normalized the graph) reseeds a fresh instance with the now-correct + // `initialDirty`, so nothing else to do. Otherwise (the common + // echoed-back-unchanged case) explicitly sync the still-current + // instance so it doesn't read dirty forever (see + // `EditableFlowCanvasHandle.clearForcedDirty`'s doc comment). + if (!remounted) { + canvasRef.current?.clearForcedDirty(); + } + log('copilot proposal accepted: persisted remounted=%s', remounted); } catch (err) { log('copilot proposal accepted: save failed err=%o', err); // Rethrow: the draft above is already applied unconditionally, so no @@ -881,7 +953,7 @@ function FlowEditor({ nodes={nodes} edges={edges} meta={meta} - onSave={handleSave} + onSave={onCanvasSave} onDirtyChange={setDirty} onSaveMetaChange={setSaveMeta} activeRunId={activeRunId} @@ -891,6 +963,8 @@ function FlowEditor({ saveDisabled={preview !== null} initialDirty={initialDirty} showPalette={sidePanel === 'legend'} + savedViewport={viewportRef.current} + onViewportChange={handleViewportChange} /> {runError && ( diff --git a/app/src/pages/__tests__/FlowCanvasPage.test.tsx b/app/src/pages/__tests__/FlowCanvasPage.test.tsx index b46c3bd8b..6ef32d83d 100644 --- a/app/src/pages/__tests__/FlowCanvasPage.test.tsx +++ b/app/src/pages/__tests__/FlowCanvasPage.test.tsx @@ -288,6 +288,128 @@ describe('FlowCanvasPage', () => { expect(screen.getByTestId('flow-canvas-page')).toBeInTheDocument(); }); + // --------------------------------------------------------------------------- + // F4/F5 fix: Save/Accept/Reject bump `canvasVersion`, remounting the editable + // canvas — which previously always reset both the pan/zoom viewport + // (`fitView` refits on every mount) and the undo history. Fix A threads a + // `savedViewport` ref (captured via `onViewportChange`, survives the + // remount) through so a remount can restore pan/zoom instead of losing it; + // `EditableFlowCanvas` exposes `data-viewport-restored` on its root so this + // is observable without reaching into React Flow internals. Fix B stops the + // *redundant* second bump `handleSave` fired on top of Accept's own bump + // whenever the server echoed the graph back unchanged. + // --------------------------------------------------------------------------- + describe('F4/F5: canvas viewport preserved + no redundant remount', () => { + it('reads as no-viewport-restored on the very first mount', async () => { + getFlow.mockResolvedValue(makeFlow()); + renderEditor(); + await waitFor(() => expect(screen.getByTestId('flow-canvas')).toBeInTheDocument()); + + expect(screen.getByTestId('flow-canvas')).toHaveAttribute('data-viewport-restored', 'false'); + }); + + it('restores the captured viewport across a remount triggered by a server-normalized save (B21)', async () => { + getFlow.mockResolvedValue(makeFlow()); + // Same server-normalization shape as the B21 test above: the response + // legitimately differs from what was sent, so Fix B still lets the + // remount-triggering bump through — this test asserts that when a + // remount DOES happen, the captured viewport survives it via Fix A. + updateFlow.mockResolvedValue( + makeFlow({ + graph: { + schema_version: 1, + id: 'test-id', + name: 'Daily digest', + nodes: [ + { + id: 't', + kind: 'trigger', + name: 'Start (normalized)', + config: {}, + ports: [], + position: { x: 0, y: 0 }, + }, + { + id: 'new-agent-0', + kind: 'agent', + name: 'New agent', + config: {}, + ports: [], + position: { x: 80, y: 80 }, + }, + { + id: 'server-added', + kind: 'transform', + name: 'Server-added node', + config: {}, + ports: [], + position: { x: 160, y: 160 }, + }, + ], + edges: [], + }, + }) + ); + renderEditor(); + await waitFor(() => expect(screen.getByTestId('flow-canvas')).toBeInTheDocument()); + expect(screen.getByTestId('flow-canvas')).toHaveAttribute('data-viewport-restored', 'false'); + + // Pan the canvas — React Flow's `onViewportChange` fires off a real + // wheel event on the pane (panOnScroll is on), which is what the host + // page's `handleViewportChange` captures into the ref that survives the + // upcoming remount. Wait for the pane's own `.react-flow__viewport` + // transform to actually change rather than a fixed sleep (scheduler- + // dependent — React Flow's viewport update isn't synchronous with the + // wheel event) so the test isn't flaky under slow CI runners. + const pane = document.querySelector('.react-flow__pane'); + expect(pane).not.toBeNull(); + const viewportEl = document.querySelector('.react-flow__viewport') as HTMLElement | null; + expect(viewportEl).not.toBeNull(); + const transformBeforePan = viewportEl?.style.transform; + fireEvent.wheel(pane as Element, { deltaY: -50, deltaX: 0, clientX: 200, clientY: 200 }); + await waitFor(() => { + expect(viewportEl?.style.transform).not.toBe(transformBeforePan); + }); + + fireEvent.click(screen.getByTestId('flow-canvas-legend-toggle')); + fireEvent.click(screen.getByTestId('flow-palette-item-agent')); + fireEvent.click(screen.getByTestId('flow-editor-save')); + fireEvent.click(screen.getByTestId('flow-action-confirm-accept')); + await waitFor(() => expect(updateFlow).toHaveBeenCalledTimes(1)); + + // The server-normalized response differs from what was sent, so the + // canvas remounts (3 nodes, matching the B21 behavior) — and the + // freshly mounted canvas reads the captured viewport back. + await waitFor(() => expect(screen.getAllByTestId('flow-node')).toHaveLength(3)); + expect(screen.getByTestId('flow-canvas')).toHaveAttribute('data-viewport-restored', 'true'); + }); + + it('accepting a proposal the server echoes back unchanged does not double-remount (no redundant Save-triggered bump)', async () => { + getFlow.mockResolvedValue(makeFlow({ name: 'Daily digest' })); + const proposal = makeProposal(); + // The server persists the proposed graph verbatim — no normalization — + // so `handleSave`'s own bump must be skipped; only Accept's single bump + // (preview → draft) should remount the canvas. + updateFlow.mockResolvedValue(makeFlow({ name: 'Daily digest', graph: proposal.graph })); + copilotPanelProps.current = null; + renderEditor(); + await waitFor(() => expect(screen.getByTestId('flow-canvas')).toBeInTheDocument()); + await waitFor(() => expect(listFlowConnections).toHaveBeenCalledTimes(1)); + + await act(async () => { + await (copilotPanelProps.current?.onAccept as (p: WorkflowProposal) => Promise)( + proposal + ); + }); + + await waitFor(() => expect(updateFlow).toHaveBeenCalledTimes(1)); + // Exactly one extra mount for Accept's own preview→draft bump — Fix B's + // gate on `handleSave`'s bump means the accept-triggered save did NOT + // fire a second one on top of it. + expect(listFlowConnections).toHaveBeenCalledTimes(2); + }); + }); + it('does not prompt when navigating Back with no unsaved changes', async () => { getFlow.mockResolvedValue(makeFlow()); renderEditor();