Claude finished @Yeraze's task —— View job

PR Review: feat(meshcore-map): node-type icons, filter & legend

• Read changed files
• Analyze code quality
• Check for bugs/issues
• Review test coverage
• Post final review

Overall this is a clean, well-scoped PR. The implementation is consistent with the pattern established in #3546 and the code is easy to follow. A few observations below.

src/utils/mapIcons.ts

roleGlyphMarkerSvg (lines 60–70) — well-designed. Delegating to roleGlyphInnerSvg and returning '' for unknown/standard keeps callers' fallback logic simple and avoids divergence between the marker and the legend swatch. No issues.

Potential minor issue — default: in roleGlyphInnerSvg (mapIcons.ts:47): The default branch already returns '', but the TypeScript type system should catch any unhandled NodeTypeCategory variants. If the union ever grows (e.g. a new firmware advert type), roleGlyphInnerSvg silently returns '' instead of surfacing a compile-time exhaustiveness error. Consider using a satisfies check or a typed exhaustive switch if this is expected to evolve. Not a blocker, but worth noting.

src/components/MeshCore/MeshCoreMap.tsx

getNodeTypeCategory called twice per contact (MeshCoreMap.tsx:197 and MeshCoreMap.tsx:287): visibleContacts filters using nodePassesTypeFilter({ advType: c.advType }, ...), which internally calls getNodeTypeCategory. Then, inside the visibleContacts.map(...) render loop, makeIcon is called with getNodeTypeCategory({ advType: c.advType }) again. With 97 nodes this is negligible, but if the contact list ever grows significantly the category could be derived once during the visibleContacts memo. Minor perf observation, not a bug.

makeIcon is a plain function recreated on every render (MeshCoreMap.tsx:71): makeIcon itself isn't memoized, but since it's a pure function with no closures over component state this is fine — L.divIcon is cheap. No issue.

localStorage.getItem / JSON.parse in useState initializer (MeshCoreMap.tsx:126–132): The try/catch around JSON.parse is correct. One nit: if JSON.parse returns a non-object (e.g. someone manually set the key to "null" or a string), the state would be typed as Partial<Record<...>> but be null at runtime, causing nodeTypeFilter[category] to throw. A typeof raw === 'object' && raw !== null check after parsing would be more defensive. Low-probability issue.

Filter applies only to markers, not paths/neighbor lines (MeshCoreMap.tsx:193–198): This matches the documented intent and the analysis map's behavior, so it's correct. The inline comment makes it clear.

MapContainer key resets the map on center/zoom changes (MeshCoreMap.tsx:269): This is existing behavior, not introduced by this PR.

src/components/MapLegend.tsx

dangerouslySetInnerHTML with roleGlyphMarkerSvg output (MapLegend.tsx:151): The SVG string is constructed entirely from the color constant (NODE_TYPE_LEGEND_COLOR = '#cba6f7') and hard-coded geometry from roleGlyphInnerSvg. There is no user-supplied data flowing into the HTML, so XSS risk is negligible here. Still, worth noting for future maintainers: if color ever comes from user input, this would need sanitization.

NODE_TYPE_LEGEND_COLOR is a module-level constant (MapLegend.tsx:12): Hard-coded to match the MeshCore map's accent. If a future feature lets users customize the MeshCore marker color, this constant would drift. Consider deriving it from overlayColors or a prop if that's ever needed — low priority.

showNodeTypes opt-in prop — well-designed. The Meshtastic maps are unaffected by default.

src/utils/mapIcons.test.ts

Coverage is good for the new function (mapIcons.test.ts:172–194): Tests cover all four glyph categories, the size parameter, and the standard early-return.

Missing edge case — unknown/arbitrary category value: roleGlyphMarkerSvg calls roleGlyphInnerSvg which returns '' for any unrecognized value via the default branch. A test with an out-of-range category (e.g. cast to NodeTypeCategory) would close that loop, though it's not critical given TypeScript's type safety.

No test for the color parameter being reflected in the output: The existing test asserts svg.toContain(COLOR) (line 181), which is correct.

Summary

No blocking issues. The PR is ready to merge as-is. The items above are suggestions for future hardening.