fix(charts): preserve map pan/zoom on re-render + reflect graph node selection

claude_code_docs/component-review/graph-map-functional-findings.md

@@ -0,0 +1,68 @@
+# Graph + Map — deep functional review (2026-06-23)
+
+Continuation of the component design review. After the per-chart design/a11y pass
+(all 14 charts), this is a deep **functional** review of the two most complex
+charts — `graph-chart.tsx` (NVL) and `map-chart.tsx` (Leaflet) — where remaining
+bugs were most likely to hide. Branch: `fix/graph-map-functional-review` → `release/1.1`.
+
+## 🔴 P1 — Map: pan/zoom resets + all markers rebuild on every parent re-render
+
+**File:** `component/src/charts/map-chart.tsx`
+
+The marker-build `useEffect` re-ran on **every** render because two of its
+dependencies got a fresh identity each render:
+
+- the plugin passes an **inline `onMarkerClick` arrow** (`app/src/plugins/map/component.tsx`),
+- `fitBoundsPadding` **defaulted to an inline `[20, 20]` literal** (new array per render).
+
+Each re-run tore down the marker `LayerGroup`, rebuilt every `circleMarker`, **and
+called `map.fitBounds(...)`** — snapping the user's pan/zoom back to the data bounds.
+`autoFitBounds` defaults to `true` in the map plugin, so this was the live path: any
+unrelated re-render (parameter change, auto-refresh poll, hover state) yanked the map
+back to fit-bounds and rebuilt all markers.
+
+**Fix:**
+- Latest-ref the click handler (`onMarkerClickRef`) and always bind the marker click
+  through the ref — `onMarkerClick` is no longer an effect dependency.
+- Hoist the padding default to a module constant `DEFAULT_FIT_PADDING` (stable identity).
+- **Split** auto-fit into its own effect keyed on `[markers, autoFitBounds, fitBoundsPadding]`
+  only, so rebuilding markers (for a style/handler change) never re-fits. Re-fitting now
+  happens only when the markers themselves change → user pan/zoom is preserved.
+
+**Tests (jsdom, mocked Leaflet):** does-not-re-fit-on-rerender, does-not-rebuild-markers
+on handler-identity change, still-re-fits when markers actually change, invokes the
+latest handler after a re-render without rebuilding. (+4)
+
+## 🟠 P2 — Graph: `selectedNodeIds` had no visual effect
+
+**File:** `component/src/charts/graph-chart.tsx`
+
+`graph-exploration-wrapper.tsx` passes `selectedNodeIds={exploration.selectedNodeIds}`
+and the click handler toggles it, but `toNvlNode` never mapped it to NVL's `selected`
+field (`GraphElement.selected`, fully supported by NVL). So controlled selection was
+**one-way-out only** — a restored or programmatic selection never highlighted the node;
+only NVL's transient in-canvas click highlight showed.
+
+**Fix:** build a `Set` from `selectedNodeIds` and set `selected: selectedIds.has(node.id)`
+in `toNvlNode`; add `selectedIds` to the `nvlNodes` memo deps.
+
+**No reshuffle risk:** `BasicNvlWrapper` diffs node attributes and calls
+`addAndUpdateElementsInGraph` for just the changed nodes, so toggling `selected` updates
+incrementally **without** re-running the force layout or resetting positions.
+
+**Tests:** marks selected node, leaves others unselected, omitted prop → all unselected,
+updated prop reflects on re-render. (+3)
+
+## ⚪ Verified — NOT bugs (left as-is)
+
+- **Raw `popup` HTML binding** (`bindPopup(m.popup)` without escaping) is a trusted
+  public-API choice (stories pass HTML). The app's `transformToMapData` never sets
+  `popup`; the data-driven `properties` tooltip path **is** escaped via `escapeHtml`.
+  No app XSS path.
+- Standard graph toolbar/zoom/layout/caption wiring, division-by-zero guards, and the
+  empty/loading/error states were all reviewed and are correct.
+
+## Verification
+
+- `component` unit suite: **1474 pass** (+7 new); `tsc --noEmit` clean; `npm run lint` 0 errors.
+- E2E: targeted `charts`, `heavy-widgets`, `widget-states`, `styling-rules` (graph + map).

component/src/charts/__tests__/graph-chart.test.tsx

@@ -168,6 +168,51 @@ describe("GraphChart", () => {
     expect(nvlNodes[0].caption).toBeUndefined();
   });
 
+  // --- Selection ---
+
+  it("marks nodes in selectedNodeIds as selected on the NVL node", () => {
+    render(
+      <GraphChart
+        nodes={sampleNodes}
+        edges={sampleEdges}
+        selectedNodeIds={["2"]}
+      />,
+    );
+    const nvlNodes = capturedProps.nodes as NvlNode[];
+    expect(nvlNodes.find((n) => n.id === "1")?.selected).toBe(false);
+    expect(nvlNodes.find((n) => n.id === "2")?.selected).toBe(true);
+    expect(nvlNodes.find((n) => n.id === "3")?.selected).toBe(false);
+  });
+
+  it("leaves nodes unselected when selectedNodeIds is omitted", () => {
+    render(<GraphChart nodes={sampleNodes} edges={sampleEdges} />);
+    const nvlNodes = capturedProps.nodes as NvlNode[];
+    expect(nvlNodes.every((n) => n.selected === false)).toBe(true);
+  });
+
+  it("reflects an updated selectedNodeIds prop on the NVL nodes", () => {
+    const { rerender } = render(
+      <GraphChart
+        nodes={sampleNodes}
+        edges={sampleEdges}
+        selectedNodeIds={["1"]}
+      />,
+    );
+    expect(
+      (capturedProps.nodes as NvlNode[]).find((n) => n.id === "1")?.selected,
+    ).toBe(true);
+    rerender(
+      <GraphChart
+        nodes={sampleNodes}
+        edges={sampleEdges}
+        selectedNodeIds={["3"]}
+      />,
+    );
+    const nvlNodes = capturedProps.nodes as NvlNode[];
+    expect(nvlNodes.find((n) => n.id === "1")?.selected).toBe(false);
+    expect(nvlNodes.find((n) => n.id === "3")?.selected).toBe(true);
+  });
+
   // --- Node color ---
 
   it("passes explicit node color to NVL node", () => {

component/src/charts/__tests__/map-chart.test.tsx

@@ -216,6 +216,74 @@ describe("MapChart", () => {
     expect(mockFitBounds).not.toHaveBeenCalled();
   });
 
+  // --- Pan/zoom preservation on re-render ---
+  // Regression: any parent re-render used to re-run the marker effect (its deps
+  // included the inline onMarkerClick identity and the default fitBoundsPadding
+  // array), which re-called fitBounds and snapped the user's pan/zoom back.
+
+  it("does not re-fit bounds on re-render when markers are unchanged (new click-handler identity)", () => {
+    const markers = [
+      { id: "1", lat: 10, lng: 20 },
+      { id: "2", lat: 30, lng: 40 },
+    ];
+    const { rerender } = render(
+      <MapChart markers={markers} autoFitBounds onMarkerClick={() => {}} />,
+    );
+    expect(mockFitBounds).toHaveBeenCalledTimes(1);
+    mockFitBounds.mockClear();
+    // Re-render with the same markers but a fresh inline handler identity.
+    rerender(
+      <MapChart markers={markers} autoFitBounds onMarkerClick={() => {}} />,
+    );
+    expect(mockFitBounds).not.toHaveBeenCalled();
+  });
+
+  it("does not rebuild markers on re-render when only the click handler identity changes", () => {
+    const markers = [{ id: "1", lat: 10, lng: 20 }];
+    const { rerender } = render(
+      <MapChart markers={markers} onMarkerClick={() => {}} />,
+    );
+    expect(L.circleMarker).toHaveBeenCalledTimes(1);
+    (L.circleMarker as unknown as ReturnType<typeof vi.fn>).mockClear();
+    rerender(<MapChart markers={markers} onMarkerClick={() => {}} />);
+    expect(L.circleMarker).not.toHaveBeenCalled();
+  });
+
+  it("still re-fits bounds when the markers actually change", () => {
+    const markers = [{ id: "1", lat: 10, lng: 20 }];
+    const { rerender } = render(<MapChart markers={markers} autoFitBounds />);
+    expect(mockFitBounds).toHaveBeenCalledTimes(1);
+    mockFitBounds.mockClear();
+    rerender(
+      <MapChart
+        markers={[
+          { id: "1", lat: 10, lng: 20 },
+          { id: "2", lat: 50, lng: 60 },
+        ]}
+        autoFitBounds
+      />,
+    );
+    expect(mockFitBounds).toHaveBeenCalledTimes(1);
+  });
+
+  it("invokes the latest onMarkerClick after a re-render without rebuilding markers", () => {
+    const first = vi.fn();
+    const second = vi.fn();
+    const markers = [{ id: "1", lat: 10, lng: 20 }];
+    const { rerender } = render(
+      <MapChart markers={markers} onMarkerClick={first} />,
+    );
+    // Grab the click handler registered on the marker.
+    const clickHandler = mockOn.mock.calls.find(
+      (c) => c[0] === "click",
+    )?.[1] as (() => void) | undefined;
+    expect(clickHandler).toBeDefined();
+    rerender(<MapChart markers={markers} onMarkerClick={second} />);
+    clickHandler?.();
+    expect(first).not.toHaveBeenCalled();
+    expect(second).toHaveBeenCalledWith(markers[0]);
+  });
+
   // --- New options ---
 
   it("uses markerSize as default radius when marker has no value", () => {

component/src/charts/graph-chart.tsx

@@ -218,6 +218,7 @@ function toNvlNode(
   captionMap: Record<string, string>,
   labelColorMap: Map<string, string>,
   nodeSizeScale: number,
+  selectedIds: Set<string>,
   stylingRules?: StylingRule[],
   paramValues?: Record<string, unknown>,
 ): NvlNode {
@@ -256,6 +257,9 @@ function toNvlNode(
     size:
       baseSize !== undefined ? Math.round(baseSize * nodeSizeScale) : undefined,
     pinned: node.fixed,
+    // Reflect controlled selection so NVL highlights selected nodes (e.g. a
+    // restored or programmatic selection, not just the last in-canvas click).
+    selected: selectedIds.has(node.id),
     x,
     y,
   };
@@ -380,6 +384,11 @@ function GraphChartInner({
 
   const nodeSizeScale = NODE_SIZE_SCALE[nodeSize] ?? 1.0;
 
+  const selectedIds = useMemo(
+    () => new Set(selectedNodeIds ?? []),
+    [selectedNodeIds],
+  );
+
   const nvlNodes = useMemo(
     () =>
       nodes.map((n, i) =>
@@ -391,6 +400,7 @@ function GraphChartInner({
           captionMap,
           labelColorMap,
           nodeSizeScale,
+          selectedIds,
           stylingRules,
           paramValues,
         ),
@@ -401,6 +411,7 @@ function GraphChartInner({
       captionMap,
       labelColorMap,
       nodeSizeScale,
+      selectedIds,
       stylingRules,
       paramValues,
     ],

component/src/charts/map-chart.tsx

@@ -76,6 +76,13 @@ const TILE_PRESETS: Record<
 
 const DEFAULT_CENTER: [number, number] = [40, -3];
 const DEFAULT_ZOOM = 3;
+/**
+ * Module-level default so an omitted `fitBoundsPadding` prop keeps a stable
+ * identity across renders. An inline `[20, 20]` default literal would be a
+ * fresh array every render, re-triggering the fit-bounds effect and snapping
+ * the user's pan/zoom back on any unrelated re-render.
+ */
+const DEFAULT_FIT_PADDING: [number, number] = [20, 20];
 
 /** Resolve tile layer, auto-selecting carto-light/carto-dark when no explicit preset is given. */
 function resolveTileLayer(
@@ -117,7 +124,7 @@ function MapChart({
   tileLayer,
   attribution,
   autoFitBounds = false,
-  fitBoundsPadding = [20, 20],
+  fitBoundsPadding = DEFAULT_FIT_PADDING,
   markerSize = 6,
   clusterMarkers = false,
   showPopup = true,
@@ -135,6 +142,12 @@ function MapChart({
 
   const dark = useDarkMode();
 
+  // Latest-ref for the click callback so a fresh inline `onMarkerClick`
+  // identity (the common case — parents pass an arrow) does not re-run the
+  // marker-build effect and tear down/rebuild every marker on each render.
+  const onMarkerClickRef = useRef(onMarkerClick);
+  onMarkerClickRef.current = onMarkerClick;
+
   const tile = resolveTileLayer(tileLayer, dark, attribution);
 
   // Initialize map
@@ -237,32 +250,33 @@ function MapChart({
         circleMarker.bindPopup(m.popup);
       }
 
-      if (onMarkerClick) {
-        circleMarker.on("click", () => onMarkerClick(m));
-      }
+      // Always bind via the ref wrapper so the latest handler is invoked
+      // without making `onMarkerClick` an effect dependency.
+      circleMarker.on("click", () => onMarkerClickRef.current?.(m));
 
       circleMarker.addTo(layer);
     });
-
-    // Auto-fit bounds
-    if (autoFitBounds && map && markers.length > 0) {
-      const bounds = L.latLngBounds(
-        markers.map((m) => [m.lat, m.lng] as [number, number]),
-      );
-      map.fitBounds(bounds, { padding: fitBoundsPadding });
-    }
   }, [
     markers,
-    onMarkerClick,
-    autoFitBounds,
-    fitBoundsPadding,
     markerSize,
     clusterMarkers,
     showPopup,
     stylingRules,
     paramValues,
   ]);
 
+  // Auto-fit bounds — kept in its own effect so that rebuilding markers (for a
+  // style/handler change) never re-fits the map. Re-fitting only happens when
+  // the markers themselves change, preserving the user's pan/zoom otherwise.
+  useEffect(() => {
+    const map = mapRef.current;
+    if (!map || !autoFitBounds || markers.length === 0) return;
+    const bounds = L.latLngBounds(
+      markers.map((m) => [m.lat, m.lng] as [number, number]),
+    );
+    map.fitBounds(bounds, { padding: fitBoundsPadding });
+  }, [markers, autoFitBounds, fitBoundsPadding]);
+
   if (error) {
     return (
       <div