From c5dd5772d0ca124187dfdd38d72c8ff1a49efd5f Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Andr=C3=A9=20Mazoni?= <andre.mazoni@meandu.com>
Date: Thu, 9 Jul 2026 00:20:02 +0930
Subject: [PATCH] Keep run graph zoom/pan when switching tabs (#561)

Switching from a run's Overview tab to another tab and back reset the
graph zoom and position to the default. Now it holds.

## Why

The viewport (pan and zoom) lived in `RunOverview` component state.
Overview and Stages are sibling routes under `runs/:id`, so switching
tabs unmounts Overview and drops that state.

## Fix

`apps/fabro-web/app/routes/run-overview.tsx`: cache the viewport per run
outside the component so it survives the remount, and reset it when the
run id changes, since the route instance is reused when only the id
changes.

Added two tests: viewport restores on remount for the same run, and does
not carry across runs.

Does not persist across a full page reload (in-memory only).

---------

Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-authored-by: Bryan Helmkamp <bryan@brynary.com>
---
 .../app/components/graph-toolbar.tsx          | 25 +++---
 apps/fabro-web/app/hooks/effects.ts           | 82 +++++++++++--------
 .../app/hooks/use-remembered-graph-view.ts    | 39 +++++++++
 apps/fabro-web/app/lib/graph-viewport.test.ts |  8 ++
 apps/fabro-web/app/lib/graph-viewport.ts      | 19 ++++-
 .../app/routes/run-overview.test.tsx          | 64 +++++++++++++--
 apps/fabro-web/app/routes/run-overview.tsx    | 31 ++-----
 7 files changed, 190 insertions(+), 78 deletions(-)
 create mode 100644 apps/fabro-web/app/hooks/use-remembered-graph-view.ts

diff --git a/apps/fabro-web/app/components/graph-toolbar.tsx b/apps/fabro-web/app/components/graph-toolbar.tsx
index 282317214..03067059b 100644
--- a/apps/fabro-web/app/components/graph-toolbar.tsx
+++ b/apps/fabro-web/app/components/graph-toolbar.tsx
@@ -1,23 +1,24 @@
 import { ArrowDownIcon, ArrowRightIcon, MinusIcon, PlusIcon } from "@heroicons/react/20/solid";
 
+import { GRAPH_MAX_ZOOM, GRAPH_MIN_ZOOM } from "../lib/graph-viewport";
+
 type Direction = "LR" | "TB";
 
+// +/- button step. Zoom-out uses the reciprocal, keeping it symmetric with zoom-in.
+const ZOOM_STEP_FACTOR = 1.25;
+
 export function GraphToolbar({
   direction,
   setDirection,
   fitToWindow,
-  onZoomIn,
-  onZoomOut,
-  canZoomIn,
-  canZoomOut,
+  zoom,
+  onZoomBy,
 }: {
   direction: Direction;
   setDirection: (d: Direction) => void;
   fitToWindow: () => void;
-  onZoomIn: () => void;
-  onZoomOut: () => void;
-  canZoomIn: boolean;
-  canZoomOut: boolean;
+  zoom: number;
+  onZoomBy: (factor: number) => void;
 }) {
   const group =
     "flex items-center gap-0.5 px-0.5 [&:not(:first-child)]:border-l [&:not(:first-child)]:border-line-strong [&:not(:first-child)]:pl-1 [&:not(:first-child)]:ml-1";
@@ -70,8 +71,8 @@ export function GraphToolbar({
         <button
           type="button"
           title="Zoom out"
-          onClick={onZoomOut}
-          disabled={!canZoomOut}
+          onClick={() => onZoomBy(1 / ZOOM_STEP_FACTOR)}
+          disabled={zoom <= GRAPH_MIN_ZOOM}
           className={`${btn} ${btnDisabled}`}
         >
           <MinusIcon className="size-4" aria-hidden="true" />
@@ -79,8 +80,8 @@ export function GraphToolbar({
         <button
           type="button"
           title="Zoom in"
-          onClick={onZoomIn}
-          disabled={!canZoomIn}
+          onClick={() => onZoomBy(ZOOM_STEP_FACTOR)}
+          disabled={zoom >= GRAPH_MAX_ZOOM}
           className={`${btn} ${btnDisabled}`}
         >
           <PlusIcon className="size-4" aria-hidden="true" />
diff --git a/apps/fabro-web/app/hooks/effects.ts b/apps/fabro-web/app/hooks/effects.ts
index 2ff5260e4..8560bb908 100644
--- a/apps/fabro-web/app/hooks/effects.ts
+++ b/apps/fabro-web/app/hooks/effects.ts
@@ -71,6 +71,48 @@ export function useDebouncedValue<T>(value: T, delayMs: number): T {
   return debounced;
 }
 
+/**
+ * Shared body of the event-listener hooks below. The target is read when the
+ * effect runs (so `window`/`document` absence and not-yet-mounted elements are
+ * handled uniformly); the listener is removed before resubscribe and on unmount;
+ * the handler sees the latest render without forcing a resubscribe. Callers must
+ * pass a stable `options` value — a fresh object every render resubscribes the
+ * listener every render.
+ */
+function useEventTargetEvent<E extends Event>(
+  target: { readonly current: EventTarget | null },
+  type: string,
+  handler: (event: E) => void,
+  options: AddEventListenerOptions | boolean | undefined,
+  active: boolean,
+): void {
+  const handlerRef = useRef(handler);
+  handlerRef.current = handler;
+
+  useEffect(() => {
+    const el = target.current;
+    if (!active || !el) return undefined;
+    const listener: EventListener = (event) => handlerRef.current(event as E);
+    el.addEventListener(type, listener, options);
+    return () => {
+      el.removeEventListener(type, listener, options);
+    };
+  }, [active, options, target, type]);
+}
+
+// Lazy getters so `window`/`document` are only touched when an effect runs, never
+// at module load (matching the previous per-hook `typeof window` checks).
+const WINDOW_TARGET = {
+  get current(): EventTarget | null {
+    return typeof window === "undefined" ? null : window;
+  },
+};
+const DOCUMENT_TARGET = {
+  get current(): EventTarget | null {
+    return typeof document === "undefined" ? null : document;
+  },
+};
+
 /**
  * Synchronizes React with a browser `window` event listener. The listener is
  * removed before resubscribe and on unmount; the handler sees the latest render.
@@ -81,17 +123,7 @@ export function useWindowEvent<K extends keyof WindowEventMap>(
   options?: AddEventListenerOptions | boolean,
   active = true,
 ): void {
-  const handlerRef = useRef(handler);
-  handlerRef.current = handler;
-
-  useEffect(() => {
-    if (!active || typeof window === "undefined") return undefined;
-    const listener = (event: WindowEventMap[K]) => handlerRef.current(event);
-    window.addEventListener(type, listener as EventListener, options);
-    return () => {
-      window.removeEventListener(type, listener as EventListener, options);
-    };
-  }, [active, options, type]);
+  useEventTargetEvent(WINDOW_TARGET, type, handler, options, active);
 }
 
 /**
@@ -104,17 +136,7 @@ export function useDocumentEvent<K extends keyof DocumentEventMap>(
   options?: AddEventListenerOptions | boolean,
   active = true,
 ): void {
-  const handlerRef = useRef(handler);
-  handlerRef.current = handler;
-
-  useEffect(() => {
-    if (!active || typeof document === "undefined") return undefined;
-    const listener = (event: DocumentEventMap[K]) => handlerRef.current(event);
-    document.addEventListener(type, listener as EventListener, options);
-    return () => {
-      document.removeEventListener(type, listener as EventListener, options);
-    };
-  }, [active, options, type]);
+  useEventTargetEvent(DOCUMENT_TARGET, type, handler, options, active);
 }
 
 /**
@@ -122,6 +144,9 @@ export function useDocumentEvent<K extends keyof DocumentEventMap>(
  * removed before resubscribe and on unmount; the handler sees the latest render.
  * Unlike a JSX event prop, this can pass `{ passive: false }` so the handler may
  * `preventDefault()` (e.g. to own wheel/⌘-scroll instead of the browser).
+ *
+ * The element must exist when the effect runs: if it renders conditionally, gate
+ * with `active` on the same condition so the effect re-runs once it mounts.
  */
 export function useElementEvent<K extends keyof HTMLElementEventMap>(
   ref: RefObject<HTMLElement | null>,
@@ -130,18 +155,7 @@ export function useElementEvent<K extends keyof HTMLElementEventMap>(
   options?: AddEventListenerOptions | boolean,
   active = true,
 ): void {
-  const handlerRef = useRef(handler);
-  handlerRef.current = handler;
-
-  useEffect(() => {
-    const el = ref.current;
-    if (!active || !el) return undefined;
-    const listener = (event: HTMLElementEventMap[K]) => handlerRef.current(event);
-    el.addEventListener(type, listener as EventListener, options);
-    return () => {
-      el.removeEventListener(type, listener as EventListener, options);
-    };
-  }, [active, options, ref, type]);
+  useEventTargetEvent(ref, type, handler, options, active);
 }
 
 /**
diff --git a/apps/fabro-web/app/hooks/use-remembered-graph-view.ts b/apps/fabro-web/app/hooks/use-remembered-graph-view.ts
new file mode 100644
index 000000000..94a495eb2
--- /dev/null
+++ b/apps/fabro-web/app/hooks/use-remembered-graph-view.ts
@@ -0,0 +1,39 @@
+import { useEffect, useRef, useState } from "react";
+import type { Dispatch, SetStateAction } from "react";
+
+import { DEFAULT_GRAPH_VIEW, type GraphView } from "../lib/graph-viewport";
+
+// Remember each run's graph pan/zoom so it survives leaving and returning to the Overview
+// tab — that route unmounts when you switch to Stages/Files/etc. and remounts on return, so
+// the viewport can't live only in component state. In-memory for the session; a full reload
+// starts fresh. Tradeoff: unpruned Map, entries are three numbers each and a session views
+// few runs — swap for an LRU if that ever stops holding.
+const graphViewByRun = new Map<string, GraphView>();
+
+const loadGraphView = (runId: string | undefined): GraphView =>
+  (runId ? graphViewByRun.get(runId) : undefined) ?? DEFAULT_GRAPH_VIEW;
+
+/**
+ * Synchronizes a run's graph viewport with the session-scoped store above, so the
+ * viewport is remembered per run across unmounts. `runId` controls which entry the
+ * state binds to: when it changes, the returned view resets to that run's remembered
+ * (or default) viewport instead of carrying the previous run's over. Every update is
+ * written back to the store; the store itself needs no cleanup. Safe under Strict
+ * Mode — the write is idempotent and re-runs persist the same committed value.
+ */
+export function useRememberedGraphView(
+  runId: string | undefined,
+): [GraphView, Dispatch<SetStateAction<GraphView>>] {
+  const [view, setView] = useState<GraphView>(() => loadGraphView(runId));
+  // Callers keep their component instance when only the route param changes, so the
+  // rebind is a render-phase reset rather than a mount.
+  const viewedRunId = useRef(runId);
+  if (viewedRunId.current !== runId) {
+    viewedRunId.current = runId;
+    setView(loadGraphView(runId));
+  }
+  useEffect(() => {
+    if (runId) graphViewByRun.set(runId, view);
+  }, [runId, view]);
+  return [view, setView];
+}
diff --git a/apps/fabro-web/app/lib/graph-viewport.test.ts b/apps/fabro-web/app/lib/graph-viewport.test.ts
index 06bda0088..59fa3a3ca 100644
--- a/apps/fabro-web/app/lib/graph-viewport.test.ts
+++ b/apps/fabro-web/app/lib/graph-viewport.test.ts
@@ -4,6 +4,7 @@ import {
   GRAPH_MAX_ZOOM,
   GRAPH_MIN_ZOOM,
   clampZoom,
+  wheelZoomFactor,
   zoomAtPoint,
   type GraphView,
 } from "./graph-viewport";
@@ -55,3 +56,10 @@ test("clampZoom respects bounds", () => {
   expect(clampZoom(500)).toBe(GRAPH_MAX_ZOOM);
   expect(clampZoom(75)).toBe(75);
 });
+
+test("wheelZoomFactor is positive and symmetric: equal scrolls up and down cancel", () => {
+  expect(wheelZoomFactor(120)).toBeGreaterThan(0);
+  expect(wheelZoomFactor(120)).toBeLessThan(1); // scroll down zooms out
+  expect(wheelZoomFactor(-120)).toBeGreaterThan(1); // scroll up zooms in
+  expect(wheelZoomFactor(120) * wheelZoomFactor(-120)).toBeCloseTo(1);
+});
diff --git a/apps/fabro-web/app/lib/graph-viewport.ts b/apps/fabro-web/app/lib/graph-viewport.ts
index f346475a4..eefbd7c5d 100644
--- a/apps/fabro-web/app/lib/graph-viewport.ts
+++ b/apps/fabro-web/app/lib/graph-viewport.ts
@@ -11,14 +11,27 @@ export const GRAPH_MAX_ZOOM = 200;
 
 export type GraphView = { zoom: number; pan: { x: number; y: number } };
 
+// Initial viewport shown when a graph first loads: 75% zoom, centered.
+export const DEFAULT_GRAPH_VIEW: GraphView = { zoom: 75, pan: { x: 0, y: 0 } };
+
 export const clampZoom = (zoom: number): number =>
   Math.min(GRAPH_MAX_ZOOM, Math.max(GRAPH_MIN_ZOOM, zoom));
 
+// How fast wheel/pinch input zooms; tune to taste.
+const WHEEL_SENSITIVITY = 0.002;
+
+/**
+ * Zoom factor for a wheel event's `deltaY`, for feeding into `zoomAtPoint`.
+ * exp() keeps equal scrolls up and down exact inverses and the factor above 0.
+ */
+export const wheelZoomFactor = (deltaY: number): number =>
+  Math.exp(-deltaY * WHEEL_SENSITIVITY);
+
 /**
  * Scale `view.zoom` by `factor`, keeping the content point under `cursor` fixed on
  * screen. `cursor` is measured from the container CENTER (matching the graph's
- * `transform-origin: center center`); pass {x:0,y:0} to zoom toward the center, which
- * is what the toolbar +/- buttons want.
+ * `transform-origin: center center`) and defaults to it, which is what the toolbar
+ * +/- buttons want.
  *
  * Derivation: with `translate(pan) scale(s)` about the center, a content point at
  * pre-transform offset q sits at screen offset `pan + s*q`. Holding the point under
@@ -27,7 +40,7 @@ export const clampZoom = (zoom: number): number =>
 export function zoomAtPoint(
   view: GraphView,
   factor: number,
-  cursor: { x: number; y: number },
+  cursor: { x: number; y: number } = { x: 0, y: 0 },
 ): GraphView {
   const zoom = clampZoom(view.zoom * factor);
   const k = zoom / view.zoom; // applied ratio after clamping
diff --git a/apps/fabro-web/app/routes/run-overview.test.tsx b/apps/fabro-web/app/routes/run-overview.test.tsx
index 4d2e5c692..ae5f159b9 100644
--- a/apps/fabro-web/app/routes/run-overview.test.tsx
+++ b/apps/fabro-web/app/routes/run-overview.test.tsx
@@ -1,8 +1,9 @@
-import { afterEach, describe, expect, mock, test } from "bun:test";
+import { afterEach, beforeEach, describe, expect, mock, test } from "bun:test";
 import TestRenderer, { act } from "react-test-renderer";
 import { MemoryRouter, Route, Routes } from "react-router";
 
 import { ApiError } from "../lib/api-client";
+import { setupReactTestEnv } from "../lib/test-utils";
 
 let currentGraphData: string | null | undefined;
 let currentGraphError: Error | undefined;
@@ -44,12 +45,13 @@ function textFromNode(
   return (node.children ?? []).map(textFromNode).join(" ");
 }
 
-function render(): TestRenderer.ReactTestRenderer {
-  (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT = true;
+// Zooming writes to useRememberedGraphView's module-scoped store, so tests that
+// change the viewport must each use a run id no other test zooms.
+function renderAt(entry: string): TestRenderer.ReactTestRenderer {
   let renderer!: TestRenderer.ReactTestRenderer;
   act(() => {
     renderer = TestRenderer.create(
-      <MemoryRouter initialEntries={["/runs/run-1"]}>
+      <MemoryRouter initialEntries={[entry]}>
         <Routes>
           <Route path="/runs/:id" element={<RunOverview />} />
         </Routes>
@@ -60,6 +62,30 @@ function render(): TestRenderer.ReactTestRenderer {
   return renderer;
 }
 
+function render(): TestRenderer.ReactTestRenderer {
+  return renderAt("/runs/run-1");
+}
+
+// The inner graph node carries `transform: translate(...) scale(...)` reflecting view.zoom/pan.
+function graphTransform(renderer: TestRenderer.ReactTestRenderer): string {
+  const node = renderer.root.findAll(
+    (n) => (n.props?.style as { transformOrigin?: string })?.transformOrigin === "center center",
+  )[0];
+  return (node.props.style as { transform: string }).transform;
+}
+
+function clickTitle(renderer: TestRenderer.ReactTestRenderer, title: string): void {
+  act(() => {
+    renderer.root.findAll((n) => n.props?.title === title)[0].props.onClick();
+  });
+}
+
+let teardownReactTestEnv: () => void;
+
+beforeEach(() => {
+  teardownReactTestEnv = setupReactTestEnv();
+});
+
 afterEach(() => {
   for (const renderer of mountedRenderers.splice(0)) {
     act(() => renderer.unmount());
@@ -68,7 +94,7 @@ afterEach(() => {
   currentGraphError = undefined;
   currentGraphLoading = false;
   graphMutateMock.mockClear();
-  delete (globalThis as { IS_REACT_ACT_ENVIRONMENT?: boolean }).IS_REACT_ACT_ENVIRONMENT;
+  teardownReactTestEnv();
 });
 
 describe("RunOverview", () => {
@@ -87,4 +113,32 @@ describe("RunOverview", () => {
     expect(text).toContain("failed to parse DOT source");
     expect(text).not.toContain("No workflow graph");
   });
+
+  test("restores graph zoom/pan when the route remounts for the same run", () => {
+    currentGraphData = "<svg viewBox='0 0 100 100'></svg>";
+
+    const first = renderAt("/runs/zoom-persist");
+    const initial = graphTransform(first);
+    clickTitle(first, "Zoom in");
+    const zoomed = graphTransform(first);
+    expect(zoomed).not.toBe(initial);
+
+    // Switching tabs unmounts this route; returning remounts it for the same run.
+    act(() => first.unmount());
+    const second = renderAt("/runs/zoom-persist");
+    expect(graphTransform(second)).toBe(zoomed);
+  });
+
+  test("does not carry one run's zoom over to a different run", () => {
+    currentGraphData = "<svg viewBox='0 0 100 100'></svg>";
+
+    const renderer = renderAt("/runs/run-a");
+    const initial = graphTransform(renderer);
+    clickTitle(renderer, "Zoom in");
+    expect(graphTransform(renderer)).not.toBe(initial);
+
+    act(() => renderer.unmount());
+    const other = renderAt("/runs/run-b");
+    expect(graphTransform(other)).toBe(initial);
+  });
 });
diff --git a/apps/fabro-web/app/routes/run-overview.tsx b/apps/fabro-web/app/routes/run-overview.tsx
index 9c841cc24..4c22fa45e 100644
--- a/apps/fabro-web/app/routes/run-overview.tsx
+++ b/apps/fabro-web/app/routes/run-overview.tsx
@@ -6,14 +6,9 @@ import { FloatingTooltip } from "../components/floating-tooltip";
 import { RunSummaryPanel } from "../components/run-summary-panel";
 import { StagePopover } from "../components/stage-popover";
 import { StageSidebar } from "../components/stage-sidebar";
-import {
-  GRAPH_MAX_ZOOM,
-  GRAPH_MIN_ZOOM,
-  clampZoom,
-  zoomAtPoint,
-  type GraphView,
-} from "../lib/graph-viewport";
+import { clampZoom, wheelZoomFactor, zoomAtPoint } from "../lib/graph-viewport";
 import { useElementEvent } from "../hooks/effects";
+import { useRememberedGraphView } from "../hooks/use-remembered-graph-view";
 import { GraphToolbar } from "../components/graph-toolbar";
 import { EmptyState, ErrorState } from "../components/state";
 import {
@@ -37,14 +32,6 @@ function parseSourceDirection(source: string | undefined): Direction | undefined
   return value === "LR" || value === "TB" ? value : undefined;
 }
 
-// Initial zoom shown when the graph first loads, in percent.
-const GRAPH_DEFAULT_ZOOM = 75;
-// Toolbar +/- step. Using 1/1.25 for zoom-out keeps it symmetric with zoom-in.
-const GRAPH_ZOOM_BUTTON_FACTOR = 1.25;
-// How fast ⌘-scroll zooms; tune to taste. exp() keeps it symmetric and always above 0.
-const GRAPH_ZOOM_WHEEL_SENSITIVITY = 0.002;
-// Zoom toward the container center. The toolbar +/- buttons anchor here, not the cursor.
-const CENTER = { x: 0, y: 0 };
 // Non-passive so the wheel handler can call preventDefault on the browser's own ⌘-zoom.
 // Kept at module scope for a stable identity, since the effect resubscribes when its
 // options object changes.
@@ -80,7 +67,7 @@ export default function RunOverview() {
   const innerRef = useRef<HTMLDivElement>(null);
   const svgRef = useRef<SVGSVGElement | null>(null);
   const navigate = useNavigate();
-  const [view, setView] = useState<GraphView>({ zoom: GRAPH_DEFAULT_ZOOM, pan: { x: 0, y: 0 } });
+  const [view, setView] = useRememberedGraphView(id);
   const dragState = useRef<{ startX: number; startY: number; startPanX: number; startPanY: number } | null>(null);
   const [hoveredNode, setHoveredNode] = useState<RunGraphNodeHover | null>(null);
 
@@ -99,8 +86,7 @@ export default function RunOverview() {
   });
 
   const onPointerDown = useCallback((e: React.PointerEvent) => {
-    if ((e.target as HTMLElement).closest("button")) return;
-    if ((e.target as HTMLElement).closest(".node")) return;
+    if (e.target instanceof Element && e.target.closest("button, .node")) return;
     e.currentTarget.setPointerCapture(e.pointerId);
     dragState.current = { startX: e.clientX, startY: e.clientY, startPanX: view.pan.x, startPanY: view.pan.y };
   }, [view.pan]);
@@ -126,12 +112,11 @@ export default function RunOverview() {
   const onWheel = useCallback((e: WheelEvent) => {
     const el = containerRef.current;
     if (!el) return;
-    if ((e.target as HTMLElement).closest('[role="toolbar"]')) return;
     e.preventDefault();
     if (e.ctrlKey || e.metaKey) {
       const r = el.getBoundingClientRect();
       const cursor = { x: e.clientX - (r.left + r.width / 2), y: e.clientY - (r.top + r.height / 2) };
-      setView((v) => zoomAtPoint(v, Math.exp(-e.deltaY * GRAPH_ZOOM_WHEEL_SENSITIVITY), cursor));
+      setView((v) => zoomAtPoint(v, wheelZoomFactor(e.deltaY), cursor));
     } else {
       setView((v) => ({ ...v, pan: { x: v.pan.x - e.deltaX, y: v.pan.y - e.deltaY } }));
     }
@@ -173,10 +158,8 @@ export default function RunOverview() {
               direction={activeDirection}
               setDirection={setDirection}
               fitToWindow={fitToWindow}
-              onZoomIn={() => setView((v) => zoomAtPoint(v, GRAPH_ZOOM_BUTTON_FACTOR, CENTER))}
-              onZoomOut={() => setView((v) => zoomAtPoint(v, 1 / GRAPH_ZOOM_BUTTON_FACTOR, CENTER))}
-              canZoomIn={view.zoom < GRAPH_MAX_ZOOM}
-              canZoomOut={view.zoom > GRAPH_MIN_ZOOM}
+              zoom={view.zoom}
+              onZoomBy={(factor) => setView((v) => zoomAtPoint(v, factor))}
             />
 
             <div