Skip to content

feat(map): add polar grid overlay to Map Analysis and Unified maps (#3971)#3983

Merged
Yeraze merged 1 commit into
mainfrom
feat/3971-polar-grid-maps
Jul 7, 2026
Merged

feat(map): add polar grid overlay to Map Analysis and Unified maps (#3971)#3983
Yeraze merged 1 commit into
mainfrom
feat/3971-polar-grid-maps

Conversation

@Yeraze

@Yeraze Yeraze commented Jul 7, 2026

Copy link
Copy Markdown
Owner

Closes #3971.

Extends the polar grid overlay (range rings + azimuth sectors centered on the own-node position, from #2307) — previously only in NodesTab — to two more map surfaces. Reuses the existing PolarGridOverlay component and getPolarGridRings() utility rather than duplicating logic.

1. Map Analysis view

  • New PolarGridLayer (src/components/MapAnalysis/layers/PolarGridLayer.tsx) draws a polar grid centered on each active source's own-node position, resolved per sourceId. Uses the theme-aware overlayColors.polarGrid palette (matching NodesTab).
  • New Polar Grid toggle in MapAnalysisToolbar, disabled when no active source has a known own-node position (LayerToggleButton gained disabled/title props).
  • Rendered in a dedicated low-z Pane in MapAnalysisCanvas so its labels sit below node markers.
  • Persisted as a new polarGrid layer in the localStorage-backed MapAnalysisConfig.

2. Unified / Dashboard map

  • Since the Dashboard map aggregates several sources, it renders one grid per source that has an own-node position, each in that source's per-source color (resolveSourceColor), with a legend so overlapping grids aren't confused.
  • Toggle reuses the existing MapContext.showPolarGrid (already shared across map surfaces and round-tripped to /api/user/map-preferences), disabled when no source has a position.
  • Works for both the single-source and Unified dashboard views.

Shared plumbing

  • getOwnNodePositions util + useOwnNodePositions hook resolve each source's own node from its local nodeNum (surfaced by GET /api/sources/:id/status via the manager's getStatus()) paired with that node's position in the shared node list. MeshCore sources (no meshtastic nodeNum) naturally yield no grid.
  • PolarGridOverlay gains an optional color prop; when set it recolors the whole grid at reduced opacity. Omitted ⇒ unchanged theme behavior (NodesTab / Map Analysis). The Dashboard passes a resolved literal color because Leaflet paints SVG strokes via the presentation attribute, which does not evaluate CSS var().

Tests

  • New ownNodePositions.test.ts (8 cases: nested/flat position, multi-source, null/MeshCore skip, Null Island, string nodeNum coercion).
  • Extended PolarGridOverlay.test.tsx with a color-override case.
  • New Map Analysis toolbar test asserting the Polar Grid toggle is disabled with no own-node position.
  • Full Vitest suite green (0 failures), tsc adds no new errors, lint clean.

🤖 Generated with Claude Code

https://claude.ai/code/session_015AFBA76hsjqhsXe1BdnYub

…3971)

Extend the polar grid overlay (range rings + azimuth sectors centered on the
own-node position, #2307) from NodesTab to two more map surfaces, reusing the
existing `PolarGridOverlay` component and `getPolarGridRings()` utility.

Map Analysis:
- New `PolarGridLayer` draws a theme-colored grid centered on each active
  source's own-node position (resolved per `sourceId`).
- Toggle added to the toolbar, disabled when no active source has a known
  own-node position.

Unified / Dashboard map:
- Renders one grid per source that has an own-node position, each colored to
  match that source's per-source color (via `resolveSourceColor`), with a
  legend so overlapping grids stay distinguishable.
- Toggle reuses the existing `MapContext.showPolarGrid` (unified across map
  surfaces), disabled when no source has a position.

Own-node positions are resolved from each source's local `nodeNum` (surfaced by
`GET /api/sources/:id/status`) paired with that node's position in the shared
node list — see new `getOwnNodePositions` util and `useOwnNodePositions` hook.
MeshCore sources (no meshtastic nodeNum) naturally yield no grid.

`PolarGridOverlay` gains an optional `color` prop; when set it recolors the whole
grid at reduced opacity (theme palette otherwise, unchanged for NodesTab).

Tests: new ownNodePositions unit tests, PolarGridOverlay color-override test,
and a Map Analysis toolbar disabled-state test. Full suite green (0 failures).

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015AFBA76hsjqhsXe1BdnYub
@github-actions

github-actions Bot commented Jul 7, 2026

Copy link
Copy Markdown

Claude finished @Yeraze's task —— View job


PR Review: feat(map): add polar grid overlay to Map Analysis and Unified maps (#3971)

  • Read all changed files
  • Analyze code quality and best practices
  • Check for potential bugs/issues
  • Review performance considerations
  • Check security concerns
  • Assess test coverage
  • Post review feedback

Overall this is a clean, well-architected feature addition. The reuse of PolarGridOverlay, careful separation of the pure getOwnNodePositions utility from the React hook, and the Null Island / MeshCore exclusion handling are all good. A few issues worth addressing:

Potential Bugs

1. Stale showPolarGrid in MapContext after disabling (src/components/Dashboard/DashboardMap.tsx:841)

checked={showPolarGrid && hasOwnNode}

The checkbox visual state is correctly gated on hasOwnNode, but showPolarGrid in MapContext (and on the server via /api/user/map-preferences) can remain true even when no own-node is available. If a node's GPS goes stale and hasOwnNode briefly becomes false while showPolarGrid is true, the grid disappears silently without updating the stored preference. When a position comes back, the grid immediately re-appears without user action — likely intentional, but worth a comment so future readers don't "fix" it.

The same pattern is used identically in MapAnalysisToolbar.tsx:131 (enabled={config.layers.polarGrid.enabled && hasOwnNode}), so at least the behavior is consistent.

2. useOwnNodePositions called twice per render in Map Analysis (src/hooks/useOwnNodePositions.ts)

MapAnalysisToolbar.tsx calls useOwnNodePositions(config.sources) at line 41 and PolarGridLayer.tsx calls it again at line 19 — both with config.sources as the argument. Because both components are children of MapAnalysisProvider, TanStack Query will deduplicate the underlying network fetches via the shared cache, but getOwnNodePositions itself runs twice per polling cycle with identical inputs, creating two Map objects and two linear scans of the node list. This is unlikely to be measurable in practice, but if the node list is large (e.g. unified view with thousands of nodes) it is wasted work. Consider surfacing ownNodePositions via MapAnalysisContext or a selector so it is computed once.

3. eslint-disable-next-line suppressing exhaustive-deps in useOwnNodePositions (src/hooks/useOwnNodePositions.ts:41-42)

// eslint-disable-next-line react-hooks/exhaustive-deps
}, [nodes, activeSourceIds.join(','), allSourceIds.join(','), statusKey(statuses)]);

activeSourceIds.join(',') and allSourceIds.join(',') are called inside the dependency array — this works (functions in dep arrays are called to get their value), but it creates a new string on every render regardless of whether the memo runs. The statusKey(statuses) call also allocates on every render. These are minor but worth noting: the memo body only runs when the keys change, but the dep array expressions still execute every render.

Code Quality

4. DashboardMap does not use useOwnNodePositions hook (src/components/Dashboard/DashboardMap.tsx:349-353)

DashboardMap imports and calls getOwnNodePositions (the raw utility) directly rather than using the useOwnNodePositions hook. This is fine — the component already has sourceStatuses from its own useSourceStatuses call and the nodes prop in scope — but the inconsistency means the two call sites use different code paths for the same operation. A brief comment explaining why the hook is not used here would help the next reader.

5. Legend z-index: 1000 may conflict (src/styles/dashboard.css:606)

The polar grid legend is positioned at z-index: 1000, which is the same z-index Leaflet uses for its control container. On some Leaflet builds this means the legend sits behind Leaflet's zoom controls. If the map controls panel (already rendered at a higher z via its own CSS) or the TilesetSelector overlay ever overlap the bottom-left corner, the legend may be obscured. The value 1001 or using z-index: var(--z-overlay, 1000) would be safer.

6. resolveSourceColor / resolveCssColor falls back silently (src/utils/sourceColors.ts:41-50)

If getComputedStyle returns an empty string for a --ctp-* variable (e.g. in a test environment without the Catppuccin theme applied), resolveCssColor returns the raw var(--ctp-blue) string. Leaflet will then receive a literal var(...) string as a stroke, which it cannot render and will default to black. The function correctly notes this as "no-op fallback when there is no document" but doesn't document the empty-string-computed case. Tests that exercise colored grids in jsdom (without CSS variables defined) will produce black grids silently. Consider logging a warning in development.

Performance

7. Color sort re-computes on every render (src/components/Dashboard/DashboardMap.tsx:180)

const colorSourceIds = [...allSourceIds].sort();

This spread-and-sort runs on every render of DashboardMap. Since allSources comes from TanStack Query and allSourceIds is derived inline (also on every render at line 177), neither is memoized. For typical source counts (2-10) this is negligible, but wrapping these two with useMemo would be consistent with the perf-conscious patterns elsewhere in this file (e.g. positionByNodeNum, positionByPublicKey, tracerouteSegments).

Test Coverage

8. No test for the legend rendering or the polar grid toggle in DashboardMap.test.tsx

The new polar-grid legend HTML (dashboard-polar-grid-legend) and the showPolarGrid checkbox are added in DashboardMap.tsx but have no corresponding test assertions in DashboardMap.test.tsx. The new test cases only mock showPolarGrid: false (line 47 in the mock) and never exercise the branch where showPolarGrid && ownNodePositions.length > 0. A test verifying the legend renders when showPolarGrid: true and a source with a matching nodeNum/position is provided would be valuable.

9. DashboardMap.test.tsx polar-grid mock returns empty statuses (src/components/Dashboard/DashboardMap.test.tsx:57)

useSourceStatuses: () => new Map(),

This is the right choice to avoid touching existing assertions, but it means ownNodePositions is always empty in tests. If you add a legend test (point 8), you'll need to provide both a non-empty nodeNum in the status map and a matching node in the nodes prop, plus showPolarGrid: true in the MapContext mock.

Minor / Nits

  • src/components/MapAnalysis/layers/PolarGridLayer.tsx:27 — the comment says "Map Analysis is scoped per source, so one grid is drawn per active source" but config.sources can be empty (= all sources). The comment is correct but could be clearer: "one grid per active source that has a resolvable own-node position; config.sources empty means all configured sources."
  • src/utils/ownNodePositions.ts:57typeof n?.nodeNum === 'number' ? n.nodeNum : Number(n?.nodeNum) is slightly awkward. The non-number branch already handles the number case correctly since Number(n) of a number is identity. Could simplify to const num = Number(n?.nodeNum) with the existing !Number.isFinite(num) guard below covering NaN.

Summary

  • Must address before merge: None — no correctness bugs that would break the feature.
  • Recommended: Points 1 (add comment), 3 (dep array style), 7 (memoize colorSourceIds), 8 (add a legend smoke test).
  • Nice to have: Points 2 (context dedup), 5 (z-index), 6 (warn on unresolved CSS var), 9 (test with a real position).

Code quality is high, the architecture correctly avoids the singleton anti-pattern, multi-source scoping is respected, and the isNullIsland / string-nodeNum coercion edge cases are well tested.

@Yeraze
Yeraze merged commit ece1ada into main Jul 7, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Feature: Add polar grid overlay to Map Analysis and Unified map (per source)

1 participant