Claude finished @Yeraze's task —— View job

PR Review: feat(map-analysis): rich multi-source node popups matching the Unified map

Tasks:

• Gather context and read changed files
• Analyze code quality and best practices
• Check for potential bugs or issues
• Review test coverage
• Post final review

Overview

This is a clean, well-scoped PR that achieves its stated goals. The approach — reusing DashboardNodePopup and useDashboardUnifiedData rather than duplicating logic — is the right architectural call. A few items worth flagging:

Bug / Correctness Issues

1. Source filter uses a shadowed variable (sourceIds) — NodeMarkersLayer.tsx:169

// Line 138 — outer scope:
const sourceIds = sourceList.map((s) => s.id);

// Line 169 — inside .filter(), SHADOWS the outer `sourceIds`:
const sourceIds = node.sources && node.sources.length > 0
  ? node.sources.map((s) => s.sourceId)
  : (node.sourceId ? [node.sourceId] : []);
return sourceIds.some((id) => config.sources.includes(id));

The inner const sourceIds shadows the outer one, which is fine at runtime but will trigger ESLint's no-shadow rule if it's enabled in this project (worth checking). A rename to nodeSourceIds (or similar) would eliminate any ambiguity and make the intent immediately obvious.

2. Hop-count lookup uses only the node's primary sourceId — NodeMarkersLayer.tsx:198

const sourceId = n.sourceId ?? '';
const hopVal = hopByKey.get(`${sourceId}:${Number(n.nodeNum)}`);

For a unified-merged node with multiple sources, n.sourceId is the primary source. If the hop-count data came back for one of the secondary sources instead, hopVal will be undefined and the popup will show no hops. This is a pre-existing gap (not introduced here), but since this PR now prominently exposes hops in the popup for all multi-source nodes it may become more noticeable. Worth a follow-up issue if not already tracked.

Code Quality

3. config.sources vs sourceIds naming could be clearer

Within filteredNodes the name sourceIds (inner) refers to the node's own reporting sources, while config.sources is the user's selection filter. A comment (or rename) would help the next reader.

4. Source-filter fallback for MeshCore nodes

MeshCore nodes have isMeshCore: true and typically no nodeId for Meshtastic. The filter at line 169 falls back to node.sourceId when node.sources is empty. If node.sourceId is also undefined (which is possible for some MeshCore entries), the node will always be filtered out when config.sources is non-empty — even though config.sources contains its source. Worth verifying against a real MeshCore node.

Test Coverage

The new test in MapAnalysisCanvas.test.tsx is solid. A few observations:

• Good: The test exercises the most important new behaviour — "Seen by 2 sources" renders inside the popup, and both source names appear.
• Missing: No test for the updated source filter logic (the node.sources.some(...) path). It would be straightforward to add a second test that mocks a node with sources: [{sourceId: 'b', ...}] but sourceId: 'a', sets config.sources = ['b'], and asserts the marker is still rendered.
• The useDisplaySettings mock added to SettingsContext is correctly placed — good catch that DashboardNodePopup pulls from it.

Minor Nits

• The comment on line 245–250 is helpful but a little long for inline JSX. A single-line summary would keep the JSX readable.
• useMapAnalysisCtx is destructured at line 52 but config.sources is referenced in several separate useMemo/filter blocks — no issue, just noting that a future reader may want to destructure config itself for clarity.

Summary

✅ Correct approach — reuses existing unified data hooks and popup component.
✅ Source-filter fix (checking node.sources instead of only primary sourceId) is the right behaviour.
✅ Test suite covers the main new rendering path.
⚠️ Shadowed sourceIds variable is a minor style/lint hazard worth renaming.
⚠️ Hop-count lookup is primary-source-only for multi-source nodes (pre-existing; may be more visible now).
⚠️ Filter coverage test for the multi-source path is missing.

Overall the PR is in good shape and ready to merge after considering the above.

Claude finished @Yeraze's task —— View job

PR Review: feat(map-analysis): rich multi-source node popups matching the Unified map

Tasks:

• Gather context and read changed files
• Analyze code quality and best practices
• Check for potential bugs or issues
• Review test coverage
• Post final review

Overview

This is a clean, well-scoped PR that achieves its stated goals. The approach — reusing DashboardNodePopup and useDashboardUnifiedData rather than duplicating logic — is the right architectural call. The second commit addresses the node.sources population issue from the prior review. Here is an updated assessment:

Bug / Correctness Issues

1. Shadowed sourceIds variable — NodeMarkersLayer.tsx:172

// Line 138 — outer scope:
const sourceIds = sourceList.map((s) => s.id);

// Line 172 — inside .filter(), SHADOWS the outer `sourceIds`:
const sourceIds = node.sources && node.sources.length > 0
  ? node.sources.map((s) => s.sourceId)
  : (node.sourceId ? [node.sourceId] : []);
return sourceIds.some((id) => config.sources.includes(id));

no-shadow is not in the ESLint config, so this won't fail CI — but it is a readability hazard. Renaming the inner variable to nodeSourceIds would make the intent obvious at a glance.

2. Hop-count lookup uses only the node's primary sourceId — NodeMarkersLayer.tsx:200-201

const sourceId = n.sourceId ?? '';
const hopVal = hopByKey.get(`${sourceId}:${Number(n.nodeNum)}`);

For unified-merged nodes, n.sourceId is the primary source. If hop data only arrived for a secondary source, hopVal will be undefined and the popup will show no hops. This is a pre-existing gap not introduced here, but the new rich popup makes it more visible. A follow-up issue would be appropriate if not already tracked.

3. MeshCore node filter fallback

The filter at line 172–175 falls back to node.sourceId when node.sources is empty. For MeshCore nodes where both node.sources and node.sourceId might be absent, the node will silently fail the filter even when its source is in config.sources. Worth verifying against a live MeshCore source.

Code Quality

The comment block at lines 248–253 (inside JSX) is a bit verbose — a single-line summary referencing the issue numbers would keep the JSX scannable. Minor nit.

Test Coverage

The new test at MapAnalysisCanvas.test.tsx:120-127 is solid and covers the most important new behaviour. Two observations:

• Good: Tests that DashboardNodePopup correctly renders "Seen by 2 sources" with both source names in a multi-source scenario. The useDisplaySettings mock is correctly added to satisfy DashboardNodePopup's hook dependency.
• Missing: No test for the updated source filter logic (the node.sources.some(...) path at line 175). A test that sets config.sources = ['b'] while node.sourceId = 'a' (primary not selected) but node.sources includes b would confirm the fix actually works. This is the core behavioral change in the PR and currently has no regression coverage.

Summary

✅ Correct approach — reuses unified data hooks and popup component.
✅ Sources now passed as full objects so node.sources is populated (fix from second commit).
✅ Source-filter correctly checks all node.sources rather than primary sourceId only.
✅ Test covers the new "Seen by N sources" rendering path.
⚠️ Inner sourceIds shadows outer — rename to nodeSourceIds for clarity (no lint impact currently).
⚠️ Hop-count lookup is primary-source-only for multi-source nodes (pre-existing gap, now more visible).
⚠️ Source-filter behavioral fix has no unit test coverage.

The PR is in good shape. The shadowed variable rename is trivial and could be done before merge; the hop-count gap and missing filter test are reasonable to track as follow-ups.