fix(map-analysis): give the 3D view the view-state, follow, and layer plumbing 2D has (#4371)

[Yeraze]

Closes #4371.

Reported by Wilhelm via Discord as three bugs in the Map Analysis 3D view. They all trace to one gap — the 3D branch of MapAnalysisCanvas was never given the plumbing the 2D branch has — so this fixes them together rather than as three point patches, as the issue asked. A fourth bug surfaced during browser testing (see D).

A. Switching to 3D reset the view to a fixed fallback

FALLBACK_CENTER = [30, -90] fed static center/zoom into <Base3DMap>, with no mechanism to carry the 2D view across.

Both surfaces now publish their live camera into a shared mapViewRef on MapAnalysisContext — 2D via a new MapViewStateController, 3D via a new onViewChange prop on Base3DMap. Whichever branch mounts next seeds center/zoom from it, falling back to the Default Map Center only when no map has reported yet.

A ref rather than state on purpose: moveend fires continuously while panning and nothing renders from this value, so re-rendering every context consumer (all the map layers) per frame would be pure cost.

Base3DMap also takes pitch/bearing now, so a 3D → 2D → 3D round-trip restores the camera angle Leaflet has no way to hold.

B. The Follow button did nothing in 3D

<FollowController /> is a useMap() controller, rendered only inside <BaseMap>'s children. New Follow3DController mirrors it case for case against the MapLibre camera, and FollowResumeButton is mounted in the 3D branch too. Base3DMap gained an onMapReady handle for it — the 3D analogue of BaseMap's mapRef, since MapLibre has no useMap() child context.

Auto-zoom deliberately does not call map.fitBounds. Two MapLibre behaviors make the obvious port wrong:

• fitBounds routes through flyTo, which ignores animate: false — only prefers-reduced-motion makes it jump — so every follow update would animate.
• Its cameraForBounds call defaults bearing to 0, so a rotated 3D camera would be yanked north-up on every update.

Resolving the camera explicitly with the current bearing and jumpTo-ing it keeps bearing and pitch, and fires moveend synchronously the way the 2D controller's animate: false does — which is what the shared programmatic-move/pause guard depends on.

C. Map layers couldn't be switched in 3D

• The tileset selector is now mounted in the 3D branch. Base3DMap takes an already-resolved basemap and already swaps its raster source in place, so the selector goes in as a sibling rather than behind a prop.
• 3D node markers now honor config.layers.markers.enabled (they were unconditional).
• Layers a MapLibre canvas genuinely cannot draw — heatmap, trails, hop shading, SNR overlay, accuracy regions, polar grid, waypoints, ATAK contacts — get their toggle disabled with a "2D view only" tooltip instead of appearing to work and drawing nothing. Their stored state is untouched, so each returns exactly as it was on the switch back. This matches what the Feature consideration: 3D map mode for elevation display and indoor venue support #3826 spec already listed as epic-level non-goals; the bug was that the UI didn't say so.

D. Blank 3D basemap on some tilesets (CSP) — found in browser testing

With C making the tileset selector reachable from 3D, testing on the dev container surfaced a console flood and a blank basemap:

Fetch API cannot load https://a.tile.openstreetmap.fr/hot/5/8/13.png.
Refused to connect because it violates the document's Content Security Policy.

The connect-src list of built-in tile hosts in dynamicCsp.ts had drifted from the TILESETS catalog — tile.openstreetmap.fr (the "OSM Humanitarian" tileset) was never added.

This stayed invisible because the two map surfaces fetch tiles through different CSP directives:

	how tiles load	directive	value
2D (Leaflet)	<img src>	img-src	'self' data: http: https: — allows anything
3D (MapLibre)	fetch()	connect-src	narrow allowlist

So that tileset has worked in 2D and been blank in 3D since the 3D view shipped in #3826. This is a pre-existing bug, not a regression from this branch — folded in here because it is the same user-visible complaint (a layer you "switched to" that renders nothing), and because C is what makes it reachable. Happy to split it into its own issue if you'd rather keep the PR narrow.

Also fixed in the same commit: extractHostFromUrl now rewrites a leading {s} subdomain placeholder to a * wildcard. new URL() parses https://{s}.example.com/… and returns the literal hostname {s}.example.com, which as a CSP source matches nothing — the same blank-basemap-in-3D failure for anyone on a custom tileset with a subdomain placeholder.

dynamicCsp.test.ts now imports the real TILESETS catalog and asserts every built-in tileset's host is reachable under connect-src, so adding a tileset without its CSP entry fails CI rather than someone's browser. The guard was mutation-tested: removing the .fr entry fails exactly the osmHot case.

Tests

New suites for both controllers (Follow3DController.test.tsx, MapViewStateController.test.tsx), plus new blocks in Base3DMap.test.tsx (camera reporting, pitch/bearing defaults, the map handle's load/unmount lifecycle), MapAnalysisCanvas.test.tsx (view carryover both directions, follow wiring, tileset selector, markers gating), MapAnalysisToolbar.test.tsx (2D-only gating, and that it leaves persisted state alone), and dynamicCsp.test.ts (catalog-driven host coverage + {s} handling).

Follow3DController.test.tsx mirrors FollowController.test.tsx case for case, so the two stay comparable — including pinning the same 15%-padded bounds box, transposed into MapLibre's lng-first order.

Verification

• Full Vitest suite green. The skipped tests are the PostgreSQL/MySQL suites (containers not running locally); CI runs both as service containers.
• tsc --noEmit clean on both tsconfig.json and tsconfig.server.json; lint:ci clean.
• Browser-verified on the dev container (--no-cache build from an isolated worktree, bundle grepped to confirm this code was actually served). That run is what surfaced D; the served connect-src now includes https://*.tile.openstreetmap.fr and the 3D basemap renders.

🤖 Generated with Claude Code

https://claude.ai/code/session_01JyRkje9qCerXExJMxjeBRT

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: fix(map-analysis) — 3D view plumbing

Tasks:

• Gather context from PR description and changed files list
• Read core implementation files
• Review test coverage
• Analyze for bugs, performance, and quality issues
• Post final review

---

Summary

This is a well-structured PR that correctly addresses three related gaps in the 3D map branch — view-state continuity, Follow/Auto-zoom, and layer toggling. The architecture choices are sound and well-documented inline. I found a few items worth attention, but nothing blocking.

---

Observations

A. View-state carryover (MapViewStateController + MapAnalysisContext)

MapViewStateController.tsx:22-41 — The controller does a map.on('moveend', publish) after a synchronous publish() at mount. This is correct. One subtle note: if the Leaflet map fires moveend during its own mount/initialization (before this effect runs), that event is missed; but since publish() is called unconditionally at mount this is harmless — the seed is already there.

MapAnalysisContext.tsx:116 — mapViewRef is initialized as useRef<MapViewState | null>(null). The RefObject<MapViewState | null> type in CtxShape (line 87) is correct for this. No issue.

MapAnalysisCanvas.tsx:118 — liveView is read during render (mapViewRef.current). The PR's inline comment acknowledges this is deliberate (seed, not reactive state). This is fine because center/zoom/pitch/bearing are mount-only props on both BaseMap and Base3DMap, meaning stale reads after a re-render caused by something else won't cause a camera jump. The pattern is safe given those semantics.

B. Follow3DController

Follow3DController.tsx:36-50 — applyView / RAF safety net: The RAF fallback for environments without requestAnimationFrame (setTimeout(cb, 0)) is a nice defensive touch. The comment "safety net for a move MapLibre treats as a no-op" is accurate — jumpTo to the same position fires no moveend, which would leave programmaticRef.current = true permanently. The RAF clears it on the next frame. This is correct.

Follow3DController.tsx:88-93 — cameraForBounds bearing passthrough: Explicitly passing the current bearing to cameraForBounds is the right call — the PR description explains exactly why (cameraForBounds defaults bearing to 0, which would snap a rotated camera north-up on every update). The !camera guard on line 91 is correctly placed.

Follow3DController.tsx:103 — eslint-disable comment: The suppression is correctly scoped to react-hooks/exhaustive-deps with an issue reference and rationale, matching the repo's established pattern. The excluded deps (applyView, map, followPaused, config.followMode, config.autoZoom) are intentionally not in the array — the comment accurately describes why (sig is the key). This mirrors FollowController.

Minor: selKey at line 34 isn't memoized, unlike sig (line 32). config.selectedNodeIds.join('|') is computed on every render. Since this is a pure render-null component and selectedNodeIds is a stable array from config (not recreated each render), this is unlikely to cause perf issues in practice — but it's a slight asymmetry with sig.

C. Base3DMap changes

Base3DMap.tsx:355-363 — moveend listener registered before load: This is correct and the comment explains it. Camera getters are valid immediately after construction; not gating on load means a pan during tile loading still gets reported.

Base3DMap.tsx:447-452 — cleanup order: setLoaded(false) before onMapReadyRef.current?.(null) before map.remove(). This is a good sequence: clearing loaded first prevents the sync effects from trying to access the being-removed map, onMapReady(null) signals consumers (Follow3DController will receive null and bail), then the map is torn down.

Base3DMap.tsx:239-242 — ref pattern for callbacks: All callback props (onViewChange, onMapReady, onNodeClick, etc.) are stored in refs updated each render. This is the idiomatic pattern for stable callbacks inside effects that have empty deps arrays — correct.

D. Toolbar 2D-only gating

MapAnalysisToolbar.tsx:46:

const THREE_D_CAPABLE_LAYERS = new Set<LayerKey>(['markers', 'traceroutes', 'neighbors']);

The polarGrid layer has its own twoDOnly check on line 289 (!hasOwnNode || twoDOnly('polarGrid')) but polarGrid is not in THREE_D_CAPABLE_LAYERS, so twoDOnly('polarGrid') returns true in 3D. The disabled title logic on lines 290-294 correctly chooses twoDOnlyTitle over the own-node message when in 3D. This is correct; the test at line 221 pins it.

MapAnalysisToolbar.tsx:110-111:

const in3D = config.viewMode === '3d';
const twoDOnly = (key: LayerKey) => in3D && !THREE_D_CAPABLE_LAYERS.has(key);

Note that in3D reads from config.viewMode, not effectiveViewMode (which lives in MapAnalysisCanvas, not here). This means if the force-2D guard has already corrected the canvas to 2D, the toolbar still reads the persisted (but-about-to-be-corrected) '3d' config for one render cycle. However, since the correction effect in MapAnalysisCanvas fires synchronously on the same render cycle as the capabilities becoming available, and because the toolbar's disabled state on 2D-only layers is harmless (it just adds an extra disabled state that corrects on the next tick), this is an acceptable and common React pattern. No user-visible issue.

E. MapAnalysisCanvas.tsx — 3D branch missing TimeSliderControl and MapLegend

The 2D branch (lines 317-319) renders <TimeSliderControl />, <MapLegend />, and <FollowResumeButton />. The 3D branch (lines 212-244) only has <FollowResumeButton /> (newly added). TimeSliderControl and MapLegend are absent from the 3D branch. This may be intentional (if these are 2D-only features), but this isn't called out in the PR description. If the time slider should still work in 3D (it controls a time window used by the data hooks, which run in both branches), its absence could be a gap. Worth a conscious decision.

F. Test coverage

Test coverage is excellent and mirrors the 2D controller suite case-for-case as stated:

• Follow3DController.test.tsx: all 10 cases cover the key behaviors — single point, multi-point auto-zoom, null cameraForBounds, both modes, empty selection, null map, programmatic-move guard, genuine user pause, pause blocking, EPS no-op guard, unmount cleanup.
• MapViewStateController.test.tsx: 6 cases including the pitch/bearing preservation, null map, and renders-nothing check.
• MapAnalysisCanvas.test.tsx: new sections cover view carryover both directions, pitch/bearing round-trip, follow wiring, tileset selector presence, and markers gating.
• MapAnalysisToolbar.test.tsx: the 2D-only gating matrix (each disabled layer, each enabled layer, and the persisted-state-untouched case) is complete.

One gap: there's no test that the FollowResumeButton is mounted in the 3D branch. The PR description says it is (and the code confirms it at line 237), but the follow plumbing describe block tests the controller but not the resume button's presence in 3D.

G. Minor / Nits

• Base3DMap.tsx:533: className ?? '' with .trim() is a common pattern but worth noting — if className is an empty string the trim is a no-op. No issue.
• The FALLBACK_CENTER / FALLBACK_ZOOM constants in MapAnalysisCanvas (lines 42-43) remain, even though the 3D branch no longer uses them directly. They're still used by the 2D center derivation on line 119-123, so they're not dead code.

---

Verdict

The logic is sound, the fitBounds-avoidance rationale is correct and well-commented, the cleanup lifecycle is correct under StrictMode double-mount, and the test coverage is thorough. The main item worth a quick look before merge:

1. TimeSliderControl / MapLegend missing from 3D branch (item E) — intentional or oversight?
2. FollowResumeButton test presence in 3D — minor gap.

The PR description flags that browser validation (WebGL, DEM tiles, tileset selector overlay position) was not done locally. That visual confirm on the dev container before merge is worthwhile, as noted.

---

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: fix(map-analysis) — 3D view plumbing

Tasks:

• Gather context from PR description and changed files list
• Read core implementation files
• Review test coverage
• Analyze for bugs, performance, and quality issues
• Post final review

---

Summary

This PR correctly addresses three interconnected gaps in the 3D map branch (view-state carryover, Follow/Auto-zoom, and layer toggling). The architecture is sound: shared ref for camera state, a mirrored 3D follow controller, and explicit 2D-only gating with explanatory tooltips. Test coverage is thorough. A few items worth attention below, none blocking.

---

Code Quality

MapViewStateController.tsx:28-34 — Pitch/bearing preservation across a 3D→2D→3D round-trip

The controller reads the previous ref value before overwriting it, so pitch/bearing set by a 3D session survive the 2D leg intact. This is correct and the test at MapViewStateController.test.tsx:62-65 pins the exact behavior. Well done.

Follow3DController.tsx:34 — selKey not memoized

const selKey = config.selectedNodeIds.join('|');

sig (line 32) is wrapped in useMemo keyed on points, but selKey is recomputed inline every render. Since this is a render-null component and selectedNodeIds is a stable array from config, the practical impact is negligible — but the asymmetry is worth noting if the component ever gains more weight.

MapAnalysisCanvas.tsx:118 — Ref read during render

Reading mapViewRef.current during render is deliberate (the PR description and inline comment explain why). It works correctly because both BaseMap and Base3DMap consume center/zoom/pitch/bearing only at mount — stale reads on subsequent re-renders from unrelated causes don't cause camera snaps. This is the right trade-off, just worth knowing it's intentional.

---

Potential Bugs / Issues

MapAnalysisCanvas.tsx:210-244 — TimeSliderControl and MapLegend absent from the 3D branch

The 2D branch (lines 317-319) renders <TimeSliderControl /> and <MapLegend />. The 3D branch does not. The time slider controls config.timeSlider, and the time-window it sets is consumed by the data hooks (use3DNeighborLines, use3DTracerouteLines) that run in both branches. If the user enables the time slider in 2D then switches to 3D, the filter is still active but no UI to adjust or disable it is visible. Whether this is intentional (time slider deferred to a later 3D epic) should be explicitly confirmed — the PR description doesn't address it, and it wasn't in the original three-bug report.

Base3DMap.tsx:447-452 — Cleanup order under StrictMode

return () => {
  setLoaded(false);
  onMapReadyRef.current?.(null);
  map.remove();
  mapRef.current = null;
};

The sequence is: clear loaded → signal consumers (null map) → remove map. Follow3DController receives null via onMapReady(null) propagating through setMap3D, which triggers a re-render setting map3D = null, which causes the follow effect to bail at its if (!map) return guard. This is correct, but note that under StrictMode's dev double-mount the cleanup runs while the component is still mounted in the tree — the setLoaded(false) call during cleanup can cause a "state update on unmounted component" warning in dev for the inner state (React 18+ suppressed this warning, React 19 reintroduced variant forms). Not a production issue, just worth knowing if dev console warnings appear.

Follow3DController.tsx:43-49 — RAF flag stuck on no-op moves

const raf = typeof requestAnimationFrame !== 'undefined'
  ? requestAnimationFrame
  : (cb: () => void) => setTimeout(cb, 0);
raf(() => { programmaticRef.current = false; });

The comment ("safety net for a move MapLibre treats as a no-op") is accurate. However there's a subtle timing window: a user pan that starts within the same animation frame as a programmatic no-op move would see programmaticRef.current === true from the RAF that hasn't cleared yet, and would be swallowed (treated as programmatic). In practice this requires sub-frame timing on user interaction, making it vanishingly unlikely. The 2D FollowController doesn't have this window at all because animate: false on setView always fires moveend synchronously. Documenting the trade-off would be helpful if this is ever debugged.

---

Performance

MapAnalysisCanvas.tsx:200-204 — handle3DViewChange callback

const handle3DViewChange = useCallback(
  (view: Map3DViewState) => {
    mapViewRef.current = view;
  },
  [mapViewRef],
);

mapViewRef is a RefObject whose .current changes but the ref object itself is stable — this useCallback effectively never invalidates. Using [mapViewRef] as the dep is fine (stable identity), but [] would express the intent more clearly and avoid the reader wondering if mapViewRef can change.

Base3DMap.tsx:355-363 — moveend fires on every programmatic and user camera change

This includes follow-mode updates from Follow3DController, which call jumpTo → moveend → onViewChange → handle3DViewChange → ref write. The ref write is O(1) and triggers no re-render (per design), so this chain is cheap. Correct pattern.

---

Security

No security concerns. The CSP changes in dynamicCsp.ts add legitimate tileset hosts to connect-src, which is the correct place. No new user-controlled data flows to sensitive sinks.

---

Test Coverage

Coverage is strong across the board. Specific observations:

Well-covered:

• Follow3DController.test.tsx: all 10 behavioral cases including the cameraForBounds null guard, programmatic-move self-pause guard, EPS no-op guard, and unmount cleanup.
• MapViewStateController.test.tsx: pitch/bearing preservation (the tricky 3D→2D→3D case), null map guard, and renders-nothing assertion.
• MapAnalysisToolbar.test.tsx: the full 2D-only gating matrix (8 disabled layers, 3 enabled layers, persisted-state-untouched case, polar grid dual-gate).
• Base3DMap.test.tsx: pre-load moveend reporting, onMapReady lifecycle (before load, after load, on unmount, WebGL unavailable path).

Gaps:

1. FollowResumeButton presence in the 3D branch: MapAnalysisCanvas.test.tsx:646-665 ("follow plumbing") tests that Follow3DController mounts, but there's no assertion that FollowResumeButton is also present in the 3D branch (line 237 in the canvas). Low risk since the component is unconditional, but the test suite documents the controller without documenting the resume affordance.
2. Time slider / legend missing from 3D: no test asserts their absence (whether intentional or not) — which means a future add would also have no coverage guard.

---

Verdict

Logic is correct. The fitBounds-avoidance rationale is well-reasoned and the bearing passthrough to cameraForBounds is the right fix for the north-snap issue. The cleanup lifecycle handles StrictMode correctly. Two items worth a quick decision before merge:

1. TimeSliderControl / MapLegend absent from 3D — if the time slider's filter should be visible/adjustable in 3D, it's missing. If this is a deliberate 3D epic deferral, a comment noting it would prevent the next reviewer from asking the same question.
2. Browser validation — the PR description flags this wasn't done locally (no WebGL/DEM/real tileset selector overlay check). Worth a visual confirm on the dev container before merge, as noted.

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: fix(map-analysis) — 3D view plumbing

Tasks:

• Gather context from PR description and changed files list
• Read core implementation files
• Review test coverage
• Analyze for bugs, performance, and quality issues
• Post final review

---

Summary

This is a well-structured PR that correctly addresses three bugs in the 3D map branch (view-state carryover, Follow/Auto-zoom, layer toggling) plus a pre-existing CSP regression. The architecture choices are sound and well-reasoned. I found several items worth attention, none of which are blocking.

---

Code Quality

MapViewStateController.tsx:28-33 — Pitch/bearing preservation across 3D→2D→3D round-trips

The controller reads prev?.pitch / prev?.bearing before overwriting the ref, so a 3D camera angle survives the 2D leg intact. This is the right approach — Leaflet has no pitch/bearing concepts, and losing them on every 2D visit would be confusing. The test at MapViewStateController.test.tsx:62-65 pins the exact behavior.

Follow3DController.tsx:38-50 — applyView / RAF safety net

Setting programmaticRef.current = true, calling the camera action (which fires moveend synchronously for jumpTo), then scheduling a RAF to clear the flag is a correct and defensive pattern. The comment explains the non-obvious case (a no-op jumpTo fires no moveend, which would leave the flag stuck). This mirrors the intent of the 2D controller's animate: false behavior.

Follow3DController.tsx:88-90 — Bearing passthrough to cameraForBounds

const camera = map.cameraForBounds([[west, south], [east, north]], {
  bearing: map.getBearing(),
});

Explicitly threading the current bearing is the correct fix for the north-snap issue (MapLibre's default is bearing: 0). The test at Follow3DController.test.tsx:121-131 pins the exact argument shape, including the lng-first coordinate order.

Base3DMap.tsx:447-452 — Cleanup order

return () => {
  setLoaded(false);
  onMapReadyRef.current?.(null);
  map.remove();
  mapRef.current = null;
};

The sequence is correct: disabling loaded first prevents sync effects from accessing a being-removed map; onMapReady(null) signals Follow3DController (which bails at its if (!map) return guard); then the map is torn down. This handles StrictMode's dev double-mount cleanly.

dynamicCsp.ts:34-36 — {s} placeholder rewriting

const hostname = parsed.hostname.startsWith('{s}.')
  ? `*.${parsed.hostname.slice('{s}.'.length)}`
  : parsed.hostname;

This is narrowly correct for a leading {s}. pattern. It does not handle {s} appearing elsewhere in the hostname (e.g., mid-string like tiles.{s}.example.com), but that pattern doesn't exist in real-world tile URL conventions, so the guard is appropriately scoped.

---

Potential Issues

MapAnalysisCanvas.tsx:210-244 — TimeSliderControl and MapLegend absent from the 3D branch

The 2D branch (lines 317-319) renders <TimeSliderControl /> and <MapLegend />. The 3D branch does not. TimeSliderControl controls config.timeSlider, and the time window it sets is consumed by use3DNeighborLines and use3DTracerouteLines — hooks that run in the 3D branch. If a user enables the time slider in 2D and switches to 3D, the filter is still active but there is no way to see or adjust it. This may be an intentional deferral to a future epic, but:

1. It is not called out in the PR description.
2. It was not in the original three-bug report.
3. No test asserts the absence (whether intentional or not).

A comment in the code (or a note in the PR description) would prevent the next reviewer from asking the same question.

Follow3DController.tsx:34 — selKey computed on every render

const selKey = config.selectedNodeIds.join('|');

sig (line 32) is wrapped in useMemo keyed on points, but selKey is recomputed on every render. Since this is a render-null component the practical impact is negligible, but the asymmetry with sig is worth noting. A useMemo keyed on config.selectedNodeIds would be consistent with the existing pattern.

handle3DViewChange dep array — minor clarity issue

const handle3DViewChange = useCallback(
  (view: Map3DViewState) => { mapViewRef.current = view; },
  [mapViewRef],  // mapViewRef is a stable RefObject — [] would be clearer
);

mapViewRef is a useRef object whose identity never changes. Using [mapViewRef] as the dep array is functionally equivalent to [], but [] would express the intent ("this callback never needs to be recreated") more clearly and avoid a reader wondering whether mapViewRef can change.

MapAnalysisToolbar.tsx:109 — reads config.viewMode, not effectiveViewMode

const in3D = config.viewMode === '3d';

effectiveViewMode lives in MapAnalysisCanvas, not in the toolbar, so the toolbar cannot access it. If the force-2D guard in MapAnalysisCanvas has corrected the view to 2D (caps unavailable), the toolbar still reads the persisted '3d' value for one render cycle and shows the 2D-only disabled states on layer buttons. This self-corrects once the effect fires, and since disabled-extra is visually harmless, it's an acceptable trade-off — but worth documenting.

---

Performance

Base3DMap.tsx:355-363 — moveend fires on every follow update

Follow-mode jumpTo → moveend → onViewChange → handle3DViewChange → ref write. The ref write is O(1) and triggers no re-render, so this is cheap by design. The PR's decision to use a ref rather than state here is correct.

lines3D useMemo in MapAnalysisCanvas.tsx:174-177

const lines3D: Line3DFeature[] = useMemo(
  () => [...neighborLines3D.lines, ...tracerouteLines3D.selectionByKey],
  [neighborLines3D.lines, tracerouteLines3D.lines],
);

This is correct — both hook results are already memoized inside their respective hooks, so this spread is cheap.

---

Security

No concerns. The CSP changes add legitimate tile hosts to connect-src. The extractHostFromUrl function is appropriately defensive (returns null on bad input, uses URL parsing rather than regex). No new user-controlled data flows to sensitive sinks.

The catalog-driven test in dynamicCsp.test.ts:176-185 is an excellent guard — it imports the real TILESETS object and fails CI when any built-in tileset's host is not in connect-src, preventing a repeat of the tile.openstreetmap.fr gap.

---

Test Coverage

Well-covered:

• Follow3DController.test.tsx — 10 cases covering all behavioral branches: single-point follow, multi-point auto-zoom, cameraForBounds null guard, both modes active, empty selection, null map, programmatic-move guard, genuine user pause, paused blocking, EPS no-op guard, and unmount cleanup.
• MapViewStateController.test.tsx — 6 cases including the pitch/bearing round-trip and renders-nothing check.
• MapAnalysisToolbar.test.tsx — the full 2D-only gating matrix (8 disabled, 3 enabled, persisted-state-untouched, polar grid dual-gate).
• Base3DMap.test.tsx — pre-load moveend reporting, onMapReady lifecycle under WebGL unavailable / before load / after load / on unmount.
• dynamicCsp.test.ts — catalog-driven per-tileset coverage, {s} wildcard rewriting, custom tileset round-trip.

Gaps worth noting:

1. FollowResumeButton in the 3D branch — MapAnalysisCanvas.test.tsx tests that Follow3DController mounts, but not that FollowResumeButton is also rendered in the 3D branch (MapAnalysisCanvas.tsx:237). Low risk since it's unconditional, but a quick screen.getByTestId('follow-resume-button') assertion in the 3D follow section would document the intent.
2. TimeSliderControl / MapLegend absence — no test asserts their absence from the 3D branch, so a future accidental add or intentional add would have no coverage gate.

---

Verdict

Logic is correct. The fitBounds-avoidance rationale is well-reasoned and correctly implemented. The cleanup lifecycle is correct under StrictMode. The catalog-driven CSP test is an excellent regression guard. Two items worth a quick decision before merge:

1. TimeSliderControl / MapLegend missing from 3D — intentional deferral? If so, a comment noting it would prevent future confusion.
2. Browser validation — the PR description notes browser testing was done on the dev container, which is good. Confirming the tileset selector overlay position and the FollowResumeButton visibility in the 3D branch are the main things worth a quick visual check.

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: fix(map-analysis) — 3D view plumbing

Tasks:

• Gather context from PR description and changed files list
• Read core implementation files
• Review test coverage
• Analyze for bugs, performance, and quality issues
• Post final review

---

Summary

Well-structured fix that correctly addresses three connected gaps in the 3D map branch (view-state carryover, Follow/Auto-zoom, layer toggling) plus a pre-existing CSP regression. The architecture choices are sound, implementation is careful, and test coverage is thorough. No blocking issues found. A few observations below.

---

Code Quality

MapViewStateController.tsx:28-34 — Pitch/bearing round-trip preservation

The controller reads prev?.pitch/prev?.bearing before overwriting the ref, so a 3D camera angle survives the 2D leg intact. This is exactly right — Leaflet has no pitch/bearing and they'd otherwise be lost on every 2D visit. The MapViewStateController.test.tsx:62-65 test pins the exact behavior.

Follow3DController.tsx:79-93 — fitBounds avoidance rationale

The comment explaining why map.fitBounds() isn't used (animates regardless of animate:false, resets bearing to 0) is accurate and important. Explicitly passing bearing: map.getBearing() to cameraForBounds is the correct fix for the north-snap issue. The test at Follow3DController.test.tsx:121-131 pins the argument shape including the lng-first coordinate order — good regression guard.

useEffectiveViewMode.ts — Extracted shared hook

Extracting this into a shared hook was the right call. The toolbar previously read raw config.viewMode, which could momentarily show 2D-only disabled states while the canvas had already fallen back to 2D. Both consumers now agree on which surface is actually rendered.

Base3DMap.tsx:447-452 — Cleanup order

setLoaded(false);          // prevent sync effects accessing being-removed map
onMapReadyRef.current?.(null); // signal Follow3DController to bail
map.remove();              // tear down
mapRef.current = null;

The sequence is correct for StrictMode's dev double-mount. Follow3DController receives null via setMap3D and bails at its if (!map) return guard before the map object is removed.

---

Observations

MapAnalysisCanvas.tsx:168-170 — lines3D useMemo has a stale dep

const lines3D: Line3DFeature[] = useMemo(
  () => [...neighborLines3D.lines, ...tracerouteLines3D.lines],
  [neighborLines3D.lines, tracerouteLines3D.lines],  // ← correct
);

This is correct. But note the comment at line 168 in the original code was updated to say tracerouteLines3D.selectionByKey — the actual dep array correctly uses .lines for both, and there's a separate line3DSelectionByKey memo (line 172-178). No issue, just a previously flagged comment inconsistency that appears already resolved in this PR.

Follow3DController.tsx:34 — selKey not memoized

const selKey = config.selectedNodeIds.join('|');

sig on line 32 is wrapped in useMemo, but selKey is recomputed inline on every render. Since this is a render-null component the practical impact is negligible, but it's a mild asymmetry. A useMemo keyed on config.selectedNodeIds would be consistent.

MapAnalysisCanvas.tsx:193-198 — handle3DViewChange dep array

const handle3DViewChange = useCallback(
  (view: Map3DViewState) => { mapViewRef.current = view; },
  [mapViewRef],  // mapViewRef is a stable RefObject — [] expresses intent more clearly
);

Functionally correct since mapViewRef is a stable useRef object, but [] would make the "never-recreate" intent explicit to future readers.

MapAnalysisCanvas.tsx:232-238 — TimeSliderControl/MapLegend absence now documented and tested

The inline comment explaining this is an intentional 3D epic non-goal (not a gap), and the test at MapAnalysisCanvas.test.tsx:709-718 that asserts their absence, address the question raised by previous reviews. Both are now properly handled.

dynamicCsp.ts:34-36 — {s} placeholder rewriting scope

const hostname = parsed.hostname.startsWith('{s}.')
  ? `*.${parsed.hostname.slice('{s}.'.length)}`
  : parsed.hostname;

Handles the leading {s}. pattern correctly. Does not handle {s} in mid-string positions (e.g., tiles.{s}.example.com), but this pattern doesn't exist in real-world tile URL conventions, so the guard is appropriately scoped. new URL() would parse that as a literal hostname {s}.example.com which CSP would treat as a host match for that literal string — same failure mode, but vanishingly rare.

---

Test Coverage

Coverage is excellent. Key suites added:

• Follow3DController.test.tsx — 10 cases mirroring the 2D suite, including the cameraForBounds null guard, programmatic-move guard with RAF timing, EPS no-op avoidance, and unmount cleanup.
• MapViewStateController.test.tsx — pitch/bearing preservation across 3D→2D→3D, null map guard, renders-nothing assertion.
• MapAnalysisCanvas.test.tsx — view carryover both directions, follow wiring, FollowResumeButton visibility (driven into its visible state rather than just asserting mount — the right approach), TimeSliderControl/MapLegend absence test.
• MapAnalysisToolbar.test.tsx — full 2D-only gating matrix (8 disabled, 3 enabled, persisted-state-untouched).
• dynamicCsp.test.ts — catalog-driven per-tileset host coverage and {s} wildcard rewriting. The it.each(Object.values(TILESETS)) guard is an excellent future-proofing choice.

---

Security

No concerns. The CSP changes add legitimate tile hosts to connect-src. extractHostFromUrl is defensive (returns null on parse failure, uses URL rather than regex). No new user-controlled data flows to sensitive sinks. The catalog-driven test in dynamicCsp.test.ts is a good regression guard for future tileset additions.

---

Performance

• moveend → onViewChange → ref write chain is O(1) with no re-render — the ref-not-state choice is correct and pays off here.
• Follow3DController's applyView RAF pattern correctly handles the "no-op jumpTo fires no moveend" edge case. The RAF clear runs on the next frame, which has an extremely narrow race window for a user gesture on the same frame as a programmatic no-op — but this is vanishingly unlikely in practice and mirrors no issue in the 2D controller.

---

Verdict

The logic is correct, the architecture choices (ref for camera state, useEffectiveViewMode shared hook, RAF guard, bearing passthrough to cameraForBounds) are well-reasoned and well-commented, and test coverage is thorough with a good regression guard for the CSP catalog. The PR is in good shape to merge.

The two minor items (selKey memoization asymmetry, handle3DViewChange dep array clarity) are nits rather than bugs — worth considering for a follow-up if this file is touched again, but not blocking.

[Yeraze]

Thanks — all five items triaged, addressed in 855380e.

1. MapAnalysisToolbar.tsx reads config.viewMode, not effectiveViewMode — real bug, fixed.

You're right that these can disagree. The window is between the canvas falling back (immediate, on capsUnavailable) and its effect writing viewMode:'2d' back to the persisted config a render later — during which a 2D map is on screen with the 2D-only toggles greyed out.

Rather than duplicating the capabilities expression into a second file, I extracted it into useEffectiveViewMode and had both read it. The canvas keeps the correcting write-back (it owns the branch); the toolbar is a read-only consumer. Mid-flight capabilities still report '3d', so an available 3D view doesn't flicker its toggles on and back while the fetch lands.

Mutation-tested: reverting the toolbar to config.viewMode fails the new re-enables the 2D-only toggles when a persisted 3d is force-corrected to 2D case.

2. FollowResumeButton unasserted in the 3D branch — added, but deliberately stronger than suggested. getByTestId on the component would pass even if the button could never actually show, since it self-hides unless a mode is on and paused. The test now drives it into that state (follow on + setFollowPaused(true)) and asserts the button by role.

3. TimeSliderControl / MapLegend absence — intentional deferral, now documented and gated. Both are non-goals of the #3826 3D epic (the persisted time-slider window is still honored by the 3D data hooks, so 2D↔3D never changes which links show — you just can't drag the handles from the 3D canvas). Added a comment at the 3D branch explaining that, plus tests asserting absence in 3D and presence in 2D so an accidental add is caught.

4. handle3DViewChange dep array [mapViewRef] → [] — not taken, and worth recording why.

The reasoning holds generally, but not in this codebase. ESLint's exhaustive-deps only treats a ref as stable when it can see the useRef() call in the same component; mapViewRef arrives via context destructuring, so it's an ordinary value. I applied the change and ran ESLint:

203:5  error  React Hook useCallback has a missing dependency: 'mapViewRef'.
              Either include it or remove the dependency array  react-hooks/exhaustive-deps

That's a new violation in a baselined file, so lint:ci fails. Kept [mapViewRef].

5. Browser validation — the tileset selector overlay is confirmed on the dev container; that session is what surfaced the CSP bug now covered in section D of the description. The FollowResumeButton visibility I've covered by test rather than by eye — it uses the same wrapper and class as the 2D branch, so I don't expect a positioning surprise, but flagging that I verified it by test and not visually.

Full suite 11,876 passed / 0 failed; both typechecks and lint:ci clean.