fix(unified): merge nodes across sources so labels show in unified view (#3135)

[Yeraze]

Summary

The unified Nodes view (/api/poll and /api/nodes with no sourceId) was returning every per-source row from the composite-keyed nodes table. A node heard on both an RF source (with NodeInfo) and an MQTT-bridged source (with only a transit packet, so longName/shortName = null) appeared twice — once labeled, once as Node <nodeNum>. This PR collapses the duplicates into one merged row per node when no source is selected.

Changes

• src/server/utils/mergeNodesAcrossSources.ts — new helper. Newest row by lastHeard wins (tiebreaker: updatedAt); empty fields are back-filled from older rows; isFavorite / isIgnored / favoriteLocked are OR'd across sources; result is sorted by updatedAt desc.
• src/server/meshtasticManager.ts — getAllNodesAsync calls the merger when sourceId is undefined. Per-source queries (getAllNodesAsync(sourceId)) are untouched.
• src/server/utils/mergeNodesAcrossSources.test.ts — 11 regression tests covering back-fill, newer-row-wins, boolean OR, position merging, three-source coverage, ordering.
• CHANGELOG.md — entry under [Unreleased].

Issues Resolved

Fixes #3135.

Documentation Updates

• CHANGELOG.md — Unreleased fix entry. No other documentation changes needed; the dedup is an internal contract change in one helper and the behavior is what users intuitively expected from the unified view.

Testing

• Unit tests pass (10,345 tests, including the 11 new merge tests)
• TypeScript compiles cleanly
• No raw SQL added (merge happens in JS after the repository returns rows)
• Reviewer can manually verify by connecting two sources where one hears a node via MQTT bridge without a NodeInfo and the other has the NodeInfo, then checking the unified Nodes tab — the node should appear once, with the label from the source that has it

Design notes

• The alternative was an SQL GROUP BY / window function. Rejected because the merge rule isn't expressible as simple aggregates (it's "newest row wins but back-fill empties, then OR booleans"), so it would have required three dialect-specific implementations for marginal gain at our scale (a few thousand rows max).
• A similar cross-source merge already exists for unified messages (fix(unified): preserve tapback metadata across MQTT ingest + cross-source merge #3105). This brings the unified Nodes view in line with that pattern.

🤖 Generated with Claude Code

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review: Unified Node Deduplication

Completed comprehensive review of this pull request for code quality, potential issues, and best practices.

Todo List:

• Read repository guidelines and context
• Examine the merge utility implementation
• Review integration in meshtasticManager
• Analyze test coverage and quality
• Check for potential bugs and edge cases
• Evaluate performance considerations
• Assess security implications
• Provide comprehensive feedback

---

Overall Assessment: ✅ EXCELLENT

This is a well-implemented solution to the unified node deduplication problem. The code demonstrates strong software engineering practices with comprehensive testing and clear documentation.

Code Quality: 9.5/10

Strengths:

• Clear separation of concerns: The merge logic is isolated in a dedicated utility with pure functions
• Comprehensive documentation: Excellent JSDoc explaining the problem, solution, and merge rules
• Follows project conventions: Adheres to MeshMonitor's multi-source architecture patterns
• Type safety: Proper TypeScript usage with type imports and explicit typing
• Clean implementation: Readable code with descriptive variable names and logical flow

Minor Suggestions:

• Consider extracting magic numbers (line 25: rows.length <= 1) into named constants for better readability
• The Record<string, any> casting (lines 46, 49) could be more type-safe, though it's acceptable for this use case

Test Coverage: 10/10

The test suite is exemplary:

• 11 comprehensive test cases covering all major scenarios
• Edge cases handled: Empty arrays, single nodes, mixed data scenarios
• Regression-focused: Tests specifically target the reported issue [BUG] weird 'unified' data unification #3135
• Clear test structure: Well-named tests with descriptive scenarios
• Good test data factory: makeNode() helper reduces test boilerplate

Performance Considerations: 9/10

Efficient Implementation:

• O(n log n) time complexity: Dominated by the sorting operations, which is optimal for this problem
• Single pass grouping: Efficient Map-based grouping in O(n)
• Memory efficient: No unnecessary data copying, reuses existing objects where possible
• Smart early returns: Handles single-item groups without unnecessary processing

Scale Appropriateness:

• Perfect for MeshMonitor's expected scale (thousands of nodes max)
• The JavaScript-based merge is justified over SQL due to complex merge rules
• Consistent with existing unified message merging pattern

Security: 10/10

No security concerns identified:

• No user input processing: Works with trusted database data only
• No SQL injection risks: Pure JavaScript logic, no database queries
• No data leakage: Properly scoped to source boundaries
• Type safety: Prevents potential runtime errors

Bug Analysis: 9/10

Well Handled Edge Cases:

• Empty string handling via isEmpty() helper
• Null/undefined value safety throughout
• Proper fallback to 0 for timestamp comparisons
• Correct boolean OR logic for user preferences

Potential Concern:

• Type casting safety (line 73): The cast from Record<string, any> back to DbNode assumes the merge preserved all required fields. While this appears safe in practice, it could benefit from runtime validation in a future iteration.

Integration Quality: 10/10

Perfect integration with existing codebase:

• Minimal invasive change: Only 6 lines modified in meshtasticManager.ts
• Preserves existing behavior: Per-source queries unchanged
• Clear conditional logic: sourceId ? dbNodes : mergeNodesAcrossSources(dbNodes)
• Follows established patterns: Matches the unified messages approach

Architecture Compliance: 10/10

Fully compliant with MeshMonitor's architecture:

• ✅ Multi-source aware: Properly handles cross-source merging
• ✅ No raw SQL: Pure JavaScript implementation
• ✅ Repository pattern: Works with existing database service layer
• ✅ Testing standards: Comprehensive test coverage with proper mocking patterns

Documentation: 9/10

• Excellent code documentation: Clear JSDoc explaining the problem and solution
• Good changelog entry: Properly documents the fix under [Unreleased]
• Descriptive PR description: Well-structured with clear rationale

Recommendations:

1. Consider adding integration tests that verify the full getAllNodesAsync() flow, though the unit tests are excellent
2. Monitor performance in production if node counts grow significantly (current implementation should handle thousands efficiently)
3. Future enhancement: Consider adding a debug flag to log merge statistics for troubleshooting

Final Verdict: ✅ APPROVED

This PR excellently solves the unified node deduplication problem with:

• Clean, maintainable code
• Comprehensive test coverage
• Proper architectural integration
• Clear documentation
• No security or performance concerns

The implementation follows MeshMonitor's high-quality standards and should be merged confidently.

---