Claude finished @Yeraze's task —— View job

PR Review: fix(meshcore): preserve real Last Heard across reconnect

Tasks:

• Gather context and read changed files
• Review meshcoreManager.ts changes
• Review test file
• Post comprehensive feedback

Summary

This is a clean, minimal fix for a real bug. The root cause analysis is accurate, the fix is correct, and the test coverage is solid. No blocking issues.

Code Review

src/server/meshcoreManager.ts (+13 / -1)

The fix (meshcoreManager.ts:1833–1847):

const advertSec = typeof c.last_advert === 'number' ? c.last_advert : 0;
const advertMs = advertSec > 0
  ? (advertSec < 1e12 ? advertSec * 1000 : advertSec)
  : undefined;
// ...
lastAdvert: advertSec > 0 ? advertSec : undefined,
lastSeen: advertMs ?? nowMs,

Correctness: The seconds→ms conversion and < 1e12 guard correctly mirrors the logic already used in MeshCoreContactDetailPanel.tsx:803–804. Consistent guard thresholds are important here — good call to keep them aligned.

Minor observation — nowMs hoisted outside the loop: const nowMs = Date.now() is computed once before the for loop rather than per-iteration. This is the right approach — all contacts in the same get_contacts response share the same fallback wall-clock, which avoids tiny skew between contacts when the fallback fires.

Minor observation — lastAdvert field was already in MeshCoreContact: The lastAdvert?: number field at meshcoreManager.ts:338 existed before this PR but was only ever populated by the contact_path_updated / advert-push path (meshcoreManager.ts:1031). refreshContacts() was silently discarding it. This PR closes that gap. Worth noting there's no data-loss risk — the DB upsertNode call (persistContact) ultimately writes lastHeard ← lastSeen, so fixing lastSeen is what matters for persistence.

No issues found. The change is exactly the right size.

src/server/meshcoreManager.lastHeard.test.ts (+105 / -0)

Coverage:

Test quality is high. vi.setSystemTime is used correctly to lock the fallback clock, and the stability test cleanly demonstrates that the fix is monotone (device value doesn't get clobbered by subsequent reconnect wall-clocks).

Minor observation — missing edge cases (non-blocking):

• last_advert is undefined (field missing from device response entirely) vs. 0 — the current code handles this correctly (typeof c.last_advert === 'number' returns false for undefined, so advertSec = 0 → fallback), but a test for the undefined case would document that behavior explicitly.
• The < 1e12 guard (already-in-ms branch) is not tested — if a future device ever reports milliseconds directly, the guard is in place but is currently untested.

Neither gap is a blocker, just potential additions if you want belt-and-suspenders coverage.

sourceId scoping: upsertNode is asserted with 'src-a' (line 69) — good, this confirms the per-source scoping invariant is exercised.

Verdict

✅ Approve — Correct fix, clean implementation, well-tested. The two missing edge cases in tests are optional improvements, not blockers.