Claude finished @Yeraze's task —— View job

PR Review: fix(meshcore): seed in-memory contacts from DB

Tasks:

• Read changed files
• Analyze implementation
• Review tests
• Post feedback

Summary

This PR correctly addresses a real bug: the DM contact list (getContacts() → this.contacts) was in-memory only, while the map/node list used the DB-backed getAllNodes(). On a flaky companion, refreshContacts() intentionally refuses to wipe on an empty result (to avoid the "node list collapses to 1" regression), leaving this.contacts sparse. The fix adds seedContactsFromDb() as a pre-seed step, matching the existing message-seeding-from-DB pattern.

The implementation is clean and the logic is sound. A few observations:

Code Quality

meshcoreManager.ts — seedContactsFromDb() (line 1850)

The implementation is well-structured. Minor observations:

1. Mixed null semantics for outPath/pathLen — In refreshContacts() (line 1823-1824) these fields are set as c.out_path ?? null (explicitly null). In seedContactsFromDb() (line 1868-1869) they're also ?? null. This is consistent. However the MeshCoreContact interface (line 330-335) types them as number | null and string | null — the null values here are intentional sentinels (OUT_PATH_UNKNOWN), which is correct.

2. advType cast — Line 1862: (n.advType ?? undefined) as MeshCoreDeviceType | undefined. This cast is necessary since the DB column is typed as a raw string/number. The pattern is the same as getAllNodes() (line 3572), so it's consistent.

3. No event emission after seeding — seedContactsFromDb() does not call dataEventEmitter.emitMeshCoreContactUpdated() for each seeded contact (unlike the live advert path at lines 1022, 1122, 1147). This appears intentional — the method is called at connect time before the frontend has subscribed to SSE events for this source. Worth confirming this assumption holds in all reconnect scenarios (e.g. if the frontend is already connected during a reconnect).

4. seeded counter is logged but the 0-contact case is silently skipped — Fine as-is; this is a minor style preference.

Potential Issue: Reconnect Scenario

seedContactsFromDb() is called in connect() (line 663). Looking at disconnect() (line 840: this.contacts.clear()), on reconnect the sequence is:

1. contacts.clear()
2. connect() → seedContactsFromDb() → refreshContacts()

This works correctly. However, if a reconnect happens while seedContactsFromDb() is running (unlikely, since it awaits before refreshContacts()), the DB result could race with a clear. Given the sequential await chain this is not a real concern, but worth noting.

Test Coverage

meshcoreManager.seedContacts.test.ts

The test file covers the four key behaviors well:

One gap: no test for the DB error path (the catch block at line 1876). The method is non-fatal on DB error per the comment, but there's no test confirming the catch swallows the error and leaves contacts empty/untouched rather than propagating. This isn't a blocker but would strengthen the regression suite.

Test design note: Using (m as unknown as {...}) to access private state is pragmatic and consistent with the existing test pattern in this codebase. The seed() helper (line 50) is a clean way to call the private method without exposing it.

No Security or Performance Concerns

• The DB query (getNodesBySource) is already used by getAllNodes() on every node-list fetch; adding one more call at connect time is negligible.
• No raw SQL introduced (ESLint-compliant).
• No sensitive data handling changes.

Verdict

Approve. The fix is correct, minimal, and well-tested. The one recommended addition (error-path test) is optional but would be a nice follow-up.

Thanks for the review! On the optional DB-error-path test: I did write one initially, but it trips vitest's spy error-surfacing — a mockImplementation that throws (sync or async) is reported by @vitest/spy through the test runner even though seedContactsFromDb's own try/catch swallows it, so the test fails on a caught error. Rather than fight the tooling on an optional case, I'm leaving it out; the catch (warn-and-continue, leaving this.contacts untouched) is straightforward by inspection. Happy to add it later if we find a clean mock pattern that doesn't trigger the detector.