Claude finished @Yeraze's task —— View job

PR Review: fix(meshcore): stop upsertNode from clobbering stored node data with nulls (#3504)

• Read changed files
• Analyze code quality and correctness
• Check test coverage
• Post review feedback

Summary

This is a solid, well-reasoned fix. The bug is genuine and reachable (confirmed at meshcoreManager.ts:1157-1165 where persistContact passes field ?? null for every optional field), and the approach is appropriate for the problem.

Code Review: src/db/repositories/meshcore.ts

The core fix (lines 217–251):

const CLEARABLE_VIA_NULL = new Set(['outPath', 'pathLen']);
const updateSet: Record<string, unknown> = { sourceId, updatedAt: now };
for (const [k, v] of Object.entries(node)) {
  if (v !== null && v !== undefined) {
    updateSet[k] = v;
  } else if (v === null && CLEARABLE_VIA_NULL.has(k)) {
    updateSet[k] = null;
  }
}

Positives:

• The merge-not-clobber pattern is correct and minimal — exactly the change needed
• Treating false/0/'' as real values (not skipping them) is correct; only null/undefined means "not observed"
• The CLEARABLE_VIA_NULL opt-in is the right design pattern for semantically meaningful nulls. outPath/pathLen being clearable is well-reasoned (CMD_RESET_PATH = explicit "clear the route", not "I didn't observe this field")
• Putting CLEARABLE_VIA_NULL as a const inside the branch avoids creating it on the insert path, though since it's a small constant Set, it could alternatively be module-level without performance concern

One minor observation:
The adminCredential field also has setAdminCredential(envelope: string | null) which intentionally clears via null (line 329–339), but adminCredential is not in CLEARABLE_VIA_NULL. This is correct because adminCredential is managed exclusively by setAdminCredential (per the doc comment at line 57–59) and should never be passed through upsertNode — so this is fine as-is. The comment at line 57 documents this contract clearly.

Potential concern — undefined keys in Object.entries:
Object.entries() only yields own enumerable properties, but the spread { ...node } on the insert path (line 246) could theoretically include prototype-chain properties if node is not a plain object. In practice, callers always pass object literals, so this is theoretical. The existing pattern for the insert path has the same characteristic, so this is not a regression.

Code Review: src/db/repositories/meshcore.test.ts

New test (lines 134–154):

it('upsertNode does NOT clobber stored name/position with incoming nulls (#3504)', async () => {
  await repo.upsertNode(
    { publicKey: 'pk-1', name: 'Repeater North', latitude: 43.65, longitude: -79.38 },
    'src-a',
  );
  await repo.upsertNode(
    { publicKey: 'pk-1', name: null, latitude: null, longitude: null, advType: 1 },
    'src-a',
  );
  // ...verifies name/lat/lon are preserved, advType is updated
});

• Test directly exercises the buggy scenario — a second upsert with explicit nulls for previously-stored fields
• Verifies that a provided non-null value (advType: 1) still takes effect (preventing a "never update anything" regression)
• Uses toBeCloseTo for float comparison — correct

What's not explicitly tested but is covered by existing tests:

• The false/0/'' behavior (not skipped) is not directly tested with a new case. This is probably fine since it's a property of the filter condition, but a future reader might appreciate a comment or test asserting batteryMv: 0 is written rather than skipped.

Correctness Validation

The fix correctly handles the two identified callers:

• persistContact (meshcoreManager.ts:1157–1165): passes field ?? null for all optional fields → these nulls now won't clobber stored values ✓
• The telemetry-config route (advType-stamping path): same pattern, same fix ✓
• meshcoreTelemetryPoller.ts:249: already safe (partial { publicKey, batteryMv } object, no nulls for other fields) ✓ — not broken by this change

Minor Suggestions (non-blocking)

1. CLEARABLE_VIA_NULL placement: Consider hoisting it to module-level as a const for clarity that it's a fixed policy, not computed per-call. Minor readability/intent signal only.

2. Test for falsy-but-valid values: A test like batteryMv: 0 being preserved (written, not skipped) would catch any future accidental change from != null to !v. Low priority.

Verdict

Approve — the fix is correct, minimal, well-documented, and the new test directly covers the regression. The CLEARABLE_VIA_NULL pattern is the right long-term design for semantically distinct null meanings. 6525 tests passing with no regressions is a strong signal.

Claude finished @Yeraze's task —— View job

PR Review: fix(meshcore): stop upsertNode from clobbering stored node data with nulls (#3504)

• Read changed files
• Analyze code quality and correctness
• Check test coverage
• Post review feedback

Summary

This is a well-executed fix for a genuine, reachable bug. The previous Claude review suggested two minor improvements; both have been addressed in the latest commit (c93bab0). Here's the updated assessment:

Previous Suggestions — Addressed

1. CLEARABLE_VIA_NULL hoisted to module scope ✅

CLEARABLE_VIA_NULL is now a module-level const at meshcore.ts:17, with a clear doc comment explaining its semantics. This is exactly the right location — it signals a fixed policy rather than a per-call computation, and makes the exception visible to anyone reading the file without needing to navigate into upsertNode.

2. batteryMv: 0 falsy-but-valid test added ✅

The new test at meshcore.test.ts:156–167 explicitly guards the !== null && !== undefined filter against future regression to a truthiness check. It also verifies that an unrelated field (name) is preserved in the same call — a nice belt-and-suspenders check.

Code Review: src/db/repositories/meshcore.ts

The core fix (meshcore.ts:225–249):

const updateSet: Record<string, unknown> = { sourceId, updatedAt: now };
for (const [k, v] of Object.entries(node)) {
  if (v !== null && v !== undefined) {
    updateSet[k] = v;
  } else if (v === null && CLEARABLE_VIA_NULL.has(k)) {
    updateSet[k] = null;
  }
  // else: preserve the existing stored value (don't write the field)
}

No concerns. The logic is correct, minimal, and well-commented. The inline comment // else: preserve the existing stored value (don't write the field) is a nice touch that makes the three-way branch immediately clear without requiring the reader to reason about the implicit no-op.

adminCredential / roomCredential exclusion — correctly excluded from CLEARABLE_VIA_NULL. Both are managed by dedicated setAdminCredential / setRoomCredential methods (meshcore.ts:334, meshcore.ts:370) and should never flow through upsertNode. The existing doc comment at meshcore.ts:62–67 documents this contract clearly.

Test Coverage: src/db/repositories/meshcore.test.ts

All 24+ meshcore tests pass per the PR description. The test suite now covers: null-clobber prevention, falsy-valid write, clearable-via-null opt-in, source scoping, cross-source isolation, and the stub-insert path.

No remaining concerns

The implementation is correct, the edge cases are covered, and both suggestions from the previous review have been applied cleanly. The CLEARABLE_VIA_NULL module-scope placement is idiomatic and the new batteryMv: 0 test adds real regression value.

Verdict: Approve. Ready to merge.