Claude finished @Yeraze's task —— View job

PR Review — fix(map): adaptive node-type filter per connected source protocol (#3610)

• Gather context and understand the request
• Read changed files
• Review code quality and best practices
• Check for potential bugs
• Assess test coverage
• Post review feedback

Summary

This is a well-structured fix for a real UX problem. The overall approach is sound: a single categorization function drives both the filter UI and map icons, the category set adapts to connected protocols, and the persisted state degrades gracefully when the source mix changes. Here are observations by area.

Code Quality

nodeTypeCategory.ts — clean and well-organized

The two-family design (MeshCoreCategory / MeshtasticCategory) is clear, and NODE_TYPE_CATEGORY_META with labelKey/label pairs is a good pattern for i18n with a safe English fallback.

Minor: categoryGlyphFamily has a redundant default branch

// src/utils/nodeTypeCategory.ts:148-174
switch (category) {
    case 'mtLostAndFound':
      return 'standard';
    default:
      return 'standard';   // ← unreachable; all 18 union members are covered
  }

MeshtasticCategory and MeshCoreCategory together cover all 18 members of NodeTypeCategory, so the default arm is dead code. TypeScript would flag this as an unreachable default with noImplicitOverride + exhaustive checks, but it's harmless. Consider removing it (or keeping it purely as a safety net with a comment).

Potential Bugs

1. isMeshCoreNode conflates advType: 0 with MeshCore identity

// src/utils/nodeTypeCategory.ts:199
export function isMeshCoreNode(node: CategorizableNode): boolean {
  return !!node.isMeshCore || typeof node.advType === 'number';
}

advType: 0 is explicitly checked in the test (line 29 in the test file) as a legitimate MeshCore node mapping to 'standard'. But if a Meshtastic node ever arrives with an advType: 0 field (e.g., from a schema migration or a noisy data source), it would be silently mis-categorized as MeshCore. Given the CategorizableNode interface already has isMeshCore as the canonical flag, it might be safer to gate purely on !!node.isMeshCore and use typeof node.advType === 'number' as a secondary signal only when isMeshCore is absent. The current approach is acceptable given actual data shapes, but worth a comment explaining why advType: 0 is still treated as MeshCore.

2. useVisibleNodeTypeCategories reacts to config.sources reference, not content

// src/components/MapAnalysis/useVisibleNodeTypeCategories.ts:39
}, [sources, config.sources]);

If config.sources is a new array reference on every render (common with Zustand slices or useState initializers that spread), this useMemo will re-run every render even when the IDs haven't changed. Worth checking how MapAnalysisContext produces config.sources; if it's an inline [], a shallow-equal comparison would silently cause excessive recalculation. This wouldn't cause incorrect behavior — just unnecessary work — but it's worth auditing.

3. createNodeIcon still has the legacy isRouter path that duplicates the repeater SVG

// src/utils/mapIcons.ts:202-219
} : isRouter ? `
    <svg ...>
      

Pre-#3610, isRouter drove the repeater glyph. Now roleCategory does for Meshtastic nodes. But isRouter is still evaluated as a fallback when roleCategory is falsy or 'standard'. If a caller passes isRouter: true without roleCategory, they get the repeater tower; if they also pass roleCategory: 'mtRouter', they get the glyph path. Callers that haven't been updated yet may produce the repeater tower through the old path, bypassing categoryGlyphFamily. Not a regression (it was like this before), but the two paths are now subtle to reason about. A follow-up to deprecate isRouter in favor of always supplying roleCategory would clean this up.

Design / Approach

sourceTypeProtocol — catch-all defaults to Meshtastic

// src/utils/nodeTypeCategory.ts:256-258
export function sourceTypeProtocol(type: string | undefined | null): 'meshcore' | 'meshtastic' {
  return type === 'meshcore' ? 'meshcore' : 'meshtastic';
}

This is intentional (unknown type → Meshtastic, the more common case), but a future 'serial', 'mqtt', or other non-meshcore source type would silently become Meshtastic. This is fine for now, but a comment noting the intent would help future maintainers.

categoriesForProtocols({ meshcore: false, meshtastic: false }) falls back to the full set

The rationale in the comment is correct (while loading, show everything so the control isn't empty), but this means a user whose sources are all disabled (not just loading) also sees the full set rather than an empty/disabled filter. This is the right UX trade-off given persisted-state semantics, but it's worth noting the ambiguity.

Performance

The useMemo in useVisibleNodeTypeCategories is well-placed — the categorization only recomputes when sources or config.sources changes. The filter in nodePassesTypeFilter is a simple map lookup (O(1)) and is safe to call per node per render. No performance concerns.

Security

No security concerns. SVG content rendered via dangerouslySetInnerHTML in MapLegend.tsx and MeshCoreMap.tsx is generated entirely from the static roleGlyphInnerSvg/roleGlyphMarkerSvg functions — no user data flows into it.

Test Coverage

Test coverage is excellent for the utility layer:

• All 13 Meshtastic roles, all 5 MeshCore advTypes, all edge cases for getNodeTypeCategory
• categoriesForProtocols for all four protocol combinations
• categoryGlyphFamily for all three glyph families
• Graceful degradation (stale toggle test at line 108)
• roleGlyphMarkerSvg now covers Meshtastic categories via their glyph family

Gap: no unit test for useVisibleNodeTypeCategories — the React hook is only indirectly tested through the component. A test that mocks useDashboardSources with sources of known types and asserts the returned category list would catch regressions in the source-scoping logic. This isn't blocking but would be a good follow-up.

Gap: NodeTypeFilterControl has no test that exercises the adaptive category list or the "Show all" reset behavior.

Minor Nits

• src/components/MapAnalysis/MapLegend.tsx:64-66: The legendCategories filter correctly excludes 'standard'-family glyphs from the legend, but the comment says "client/standard buckets render as the default pin" — mtTracker, mtTak, etc. also render as pins but aren't called out. The comment could be slightly more precise.
• src/utils/nodeTypeCategory.ts:119-139: The alignment-heavy record literal (key: 'repeater', labelKey: ...) with varying trailing spaces may cause noise in future diffs if editors normalize whitespace. Consider letting the formatter handle it.

Verdict

Solid, well-scoped fix. The architecture is clean, the tests are thorough for the utility layer, and the graceful-degradation story for persisted state is correct. The isRouter legacy path in createNodeIcon and the missing hook test are the most notable follow-up items but are not blocking for merge.