Claude finished @Yeraze's task —— View job

PR Review: feat(map): MeshCore node-type icons + map filtering (#3546)

Tasks:

• Gather context
• Read changed files
• Analyze code quality and potential issues
• Post review feedback

Overall Assessment

This is a well-structured, well-tested PR. The "single source of truth" design principle in nodeTypeCategory.ts is the right call — it prevents icon/filter disagreement. A few observations below, ranging from a potential bug to minor style suggestions.

Issues

🐛 Potential Bug: isMeshCore detection heuristic can misclassify Meshtastic nodes

src/utils/nodeTypeCategory.ts:71

const isMeshCore = !!node.isMeshCore || typeof node.advType === 'number';

If a Meshtastic node somehow arrives with an advType field set to a numeric value (e.g., 0 from a future schema migration, or a join artifact), it will be wrongly treated as MeshCore. The fallback in sourceRoutes.ts:971 shows that MeshCore nodes are always emitted with advType: typeof n.advType === 'number' ? n.advType : 0 — meaning they always emit advType: 0 even when the true type is unknown, and 0 is also the default Meshtastic node role. This could be a problem if both protocols share a node store.

The check typeof node.advType === 'number' effectively means "any node with advType=0 gets treated as MeshCore with category standard" rather than as a Meshtastic node with a role. The explicit isMeshCore: true flag should be the primary guard, with advType only used as a hint when isMeshCore is missing. Consider:

const isMeshCore = !!node.isMeshCore;

and derive advType separately, only when isMeshCore is true. The current test at line 26 explicitly tests and validates the advType-only-heuristic, so this is a documented behavior — but it's worth flagging if MeshCore and Meshtastic nodes ever share a data path.

🐛 someHidden pluralization is brittle

src/components/MapAnalysis/NodeTypeFilterControl.tsx:23

t('map.nodeType.someHidden', '{{count}} type hidden', { count: hidden.length })

When hidden.length > 1, the label reads "2 type hidden" (singular). The en.json key at map.nodeType.someHidden doesn't use i18next's _plural suffix or count-based interpolation for pluralization. This should read "{{count}} types hidden" for the plural, or use i18next's count pluralization feature (map.nodeType.someHidden_one, map.nodeType.someHidden_other).

Code Quality Notes

mapIcons.ts — duplicated tower SVG

src/utils/mapIcons.ts:179–196

The repeater/tower SVG in roleGlyphInnerSvg() (lines 16–24) is duplicated verbatim in the isRouter branch of the MeshMonitor-style markerSvg (lines 183–195), minus the wrapping comment. Now that the router icon goes through roleGlyphInnerSvg('repeater', color), the isRouter code path is only reached when roleCategory === undefined or roleCategory === 'standard' on a Meshtastic ROUTER node.

This means a Meshtastic ROUTER node gets roleCategory = 'repeater' (from getNodeTypeCategory), so the roleInner path fires and the isRouter path is dead code. The isRouter parameter on createNodeIcon is now only reached when roleCategory is 'standard' and isRouter is true — which can't happen because ROUTER maps to 'repeater'. Consider removing isRouter from createNodeIcon's signature (or marking it @deprecated).

MapLegend.tsx — legend shows node-type section when markers are on, regardless of filter state

src/components/MapAnalysis/MapLegend.tsx:95–104

The Node Types legend section is always shown when showMarkers is true, even if all categories are standard (no MeshCore nodes are present). This is a minor UX issue — operators on pure-Meshtastic setups will see a "Node Types" legend section with four icons they'll never see on the map. Consider gating this section on whether any non-standard category is actually present in the visible node set, or accept the always-visible behavior as a feature.

MapAnalysisConfig.nodeTypes type vs. usage

src/hooks/useMapAnalysisConfig.ts:43

nodeTypes: Record<NodeTypeCategory, boolean>;

The type says every key is always present, but nodePassesTypeFilter in nodeTypeCategory.ts:92 uses enabledByCategory[getNodeTypeCategory(node)] !== false which safely handles missing keys. The Partial<Record<NodeTypeCategory, boolean>> parameter type on nodePassesTypeFilter also mismatches the MapAnalysisConfig.nodeTypes non-partial type. This is harmless (TS accepts Record<K,V> where Partial<Record<K,V>> is expected), but the design intent around "missing = visible" is worth documenting on the MapAnalysisConfig interface — Partial<Record<NodeTypeCategory, boolean>> would more accurately represent the intent.

dangerouslySetInnerHTML with dynamic color in MapLegend

src/components/MapAnalysis/MapLegend.tsx:18–24

The color variable is a hardcoded constant ('#6698f5'), so there is no XSS risk here. However, roleGlyphInnerSvg accepts a color parameter which elsewhere accepts the hop-color (a hex value generated in-process). This is fine. Worth noting for future callers that roleGlyphInnerSvg output is embedded in SVG via dangerouslySetInnerHTML — if color ever comes from user input, this would need sanitization.

Positive Highlights

• Single source of truth — getNodeTypeCategory drives both the visible icon and the filter, so they can never disagree. Clean design.
• load() merges stored nodeTypes with defaults (src/hooks/useMapAnalysisConfig.ts:95) — adding a new category in a future version won't break existing persisted configs (new category defaults to visible).
• Pure function + 36 test cases — getNodeTypeCategory is pure and thoroughly tested including edge cases (null role, garbage strings, missing isMeshCore).
• i18n fallbacks — meta.label English fallbacks on every category meta entry means untranslated locales don't break.
• Backend change is minimal and safe — just exposing advType on the existing MeshCore node shape in sourceRoutes.ts:971.

Summary

Two items worth addressing before merge:

1. The pluralization bug on "{{count}} type hidden" (cosmetic but user-visible).
2. The dead-code isRouter path in createNodeIcon — not a bug, but the duplication should be resolved to prevent confusion.

The isMeshCore heuristic concern is low-risk given the current data paths, but worth a code comment explaining why the typeof advType === 'number' secondary check is intentional.