fix(flows): preserve canvas viewport and skip redundant remount on save (#4999)

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<string> = new Set();
@@ -219,6 +248,8 @@ function EditableFlowCanvas(
     initialDirty = false,
     showPalette = true,
     onSaveMetaChange,
+    savedViewport = null,
+    onViewportChange,
   }: EditableFlowCanvasProps,
   ref: ForwardedRef<EditableFlowCanvasHandle>
 ) {
@@ -227,6 +258,23 @@ function EditableFlowCanvas(
   const [edges, setEdges, onEdgesChange] = useEdgesState<FlowEdge>(initialEdges);
   const rfRef = useRef<ReactFlowInstance<FlowNode, FlowEdge> | 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>
           <Background variant={BackgroundVariant.Dots} gap={16} size={1} />

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<EditableFlowCanvasHandle, FlowCanvasProps>(
       initialDirty,
       showPalette = true,
       onSaveMetaChange,
+      savedViewport,
+      onViewportChange,
     }: FlowCanvasProps,
     ref
   ) => {
@@ -150,6 +167,8 @@ const FlowCanvas = forwardRef<EditableFlowCanvasHandle, FlowCanvasProps>(
           initialDirty={initialDirty}
           showPalette={showPalette}
           onSaveMetaChange={onSaveMetaChange}
+          savedViewport={savedViewport}
+          onViewportChange={onViewportChange}
         />
       );
     }

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<EditableFlowCanvasHandle>(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<Viewport | null>(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<EditorSaveMeta>({
     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<void>`) 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 && (

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<void>)(
+          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();