Claude finished @Yeraze's task —— View job

PR Review: fix(meshcore): node list collapsing to a single node with multiple sources

Tasks:

• Gather context and understand the PR
• Read changed files
• Review core logic in meshcoreManager.ts
• Review new test file
• Review repository and route changes
• Post comprehensive review

Summary

This is a well-diagnosed and clearly written fix. The root cause analysis is accurate, the solution is appropriately layered (DB as durable base + in-memory overlay), and the test coverage directly targets the two failure modes. A few observations follow.

Core Logic (meshcoreManager.ts)

refreshContacts() guard (src/server/meshcoreManager.ts:1741) — the fix (response.data.length > 0) is correct. One subtle edge case worth noting: if a companion legitimately has zero contacts (brand-new device, or after a full reset), a real empty get_contacts response will permanently be ignored and the in-memory map will never be cleared. This is probably acceptable in practice since it only matters in that narrow window before any advert is received, and the DB would also be empty in that case. But it's worth a comment so future maintainers don't treat the guard as "never clear on empty" without understanding the trade-off.

getAllNodes() merge (src/server/meshcoreManager.ts:3456–3512) — the logic is solid:

• DB rows form the durable base ✓
• In-memory overlay wins for freshness ✓
• DB-only fields (batteryMv, uptimeSecs, radio params) preserved via spread-then-overlay ✓
• Local node deduplicated correctly ✓
• DB failure falls back gracefully ✓

Minor observation (src/server/meshcoreManager.ts:3462): the local node is skipped from the DB base with if (n.isLocalNode) continue. This works, but it means if this.localNode is null at call time (e.g. before the initial handshake completes), the persisted local node row is silently dropped from the output entirely rather than being included as a fallback. Depending on the reconnect timing this could cause the local node to briefly disappear from the list. Consider including it from DB when this.localNode is null.

In-memory merge (src/server/meshcoreManager.ts:3492–3503): batteryMv and uptimeSecs are intentionally preserved from base (the DB row) since contacts don't carry them, but txPower, maxTxPower, and the radio params are NOT carried over from base in the in-memory overlay branch. They're set from DB in the first loop, then the spread-and-set in the overlay loop does ...base first, so they survive the spread — but only if the contact was already in the DB. If a fresh in-memory-only contact arrives that was never persisted, those fields are simply absent (undefined), which is correct. No bug here, but worth confirming the spread order is intentional.

Repository (src/db/repositories/meshcore.ts)

getNodesBySource() (src/db/repositories/meshcore.ts:168–176) — clean, properly scoped, matches the pattern of the existing getNodeByPublicKeyAndSource. Good addition.

getLocalNode() (src/db/repositories/meshcore.ts:216–224) — this existing method is not scoped by sourceId. It returns the first isLocalNode=true row it finds globally. With multiple MeshCore sources this would return an arbitrary source's local node. This is a pre-existing issue and out of scope for this PR, but worth flagging so it's tracked.

Tests (src/server/meshcoreManager.getAllNodes.test.ts)

The six tests are well-targeted and cover:

1. DB-backed list when in-memory is empty ✓
2. In-memory overlay without duplication + DB-only field preservation ✓
3. Live local node first + de-duped from DB ✓
4. DB failure falls back to in-memory ✓
5. Empty get_contacts does not wipe ✓
6. Non-empty get_contacts still replaces ✓

One gap: There is no test for the case where this.localNode is null but a isLocalNode=true row exists in the DB. Given the observation above about the local node being silently dropped in that case, a test documenting the current behavior (or the desired fallback) would be valuable.

flushMicrotasks (src/server/meshcoreManager.getAllNodes.test.ts:58–61) — the double-Promise.resolve() flush is a reasonable approach for settling async event handlers, but it's fragile if the contact persistence path chains more than two microtask hops. The tests pass currently, but this is worth a note if they become flaky.

Route callers

All four callers (meshcoreRoutes, sourceRoutes, unifiedRoutes, server.ts) correctly await the now-async getAllNodes(). No issues.

Performance note (src/server/routes/sourceRoutes.ts:819, src/server/routes/unifiedRoutes.ts:1245): getAllNodes() now issues a DB query on every call. The source-list endpoint calls it in a loop over all MeshCore sources to compute node counts. For deployments with many MeshCore sources (e.g. 10+) this becomes O(N) DB queries. Not a regression from the fix itself (the previous sync path was O(1)), but worth tracking. A getNodeCountBySource() query could make those count-only call sites cheaper if it ever becomes a bottleneck.

Overall

The fix is correct and well-tested. The two main observations (local node dropped when this.localNode is null; getLocalNode() is unscoped) are pre-existing or edge cases that are out of scope for this PR. Recommend addressing the local-node null case in a follow-up if reconnect behavior shows the node list flickering.

Verdict: Approve with the above non-blocking notes.