diff --git a/claude_code_docs/component-review/graph-map-functional-findings.md b/claude_code_docs/component-review/graph-map-functional-findings.md new file mode 100644 index 00000000..ca6535ac --- /dev/null +++ b/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). diff --git a/component/src/charts/__tests__/graph-chart.test.tsx b/component/src/charts/__tests__/graph-chart.test.tsx index 2f5203f2..7b08ffaa 100644 --- a/component/src/charts/__tests__/graph-chart.test.tsx +++ b/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( + , + ); + 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(); + 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( + , + ); + expect( + (capturedProps.nodes as NvlNode[]).find((n) => n.id === "1")?.selected, + ).toBe(true); + rerender( + , + ); + 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", () => { diff --git a/component/src/charts/__tests__/map-chart.test.tsx b/component/src/charts/__tests__/map-chart.test.tsx index 442666cb..96c18b61 100644 --- a/component/src/charts/__tests__/map-chart.test.tsx +++ b/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( + {}} />, + ); + expect(mockFitBounds).toHaveBeenCalledTimes(1); + mockFitBounds.mockClear(); + // Re-render with the same markers but a fresh inline handler identity. + rerender( + {}} />, + ); + 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( + {}} />, + ); + expect(L.circleMarker).toHaveBeenCalledTimes(1); + (L.circleMarker as unknown as ReturnType).mockClear(); + rerender( {}} />); + 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(); + expect(mockFitBounds).toHaveBeenCalledTimes(1); + mockFitBounds.mockClear(); + rerender( + , + ); + 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( + , + ); + // 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(); + clickHandler?.(); + expect(first).not.toHaveBeenCalled(); + expect(second).toHaveBeenCalledWith(markers[0]); + }); + // --- New options --- it("uses markerSize as default radius when marker has no value", () => { diff --git a/component/src/charts/graph-chart.tsx b/component/src/charts/graph-chart.tsx index 89c6072b..75110f69 100644 --- a/component/src/charts/graph-chart.tsx +++ b/component/src/charts/graph-chart.tsx @@ -218,6 +218,7 @@ function toNvlNode( captionMap: Record, labelColorMap: Map, nodeSizeScale: number, + selectedIds: Set, stylingRules?: StylingRule[], paramValues?: Record, ): 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, ], diff --git a/component/src/charts/map-chart.tsx b/component/src/charts/map-chart.tsx index fd61da18..b8511603 100644 --- a/component/src/charts/map-chart.tsx +++ b/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,25 +250,14 @@ 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, @@ -263,6 +265,18 @@ function MapChart({ 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 (