fix(nodes): don't clobber learned name/macaddr/hwModel with blanks (#3505)

[Yeraze]

Closes #3505. The durable form of the #3456 fix.

Problem (latent hole)

The node upsert merge guard treated '' and 0 as "provided":

• SQLite upsertNodeSqlite: setIfProvided only checks !== null && !== undefined, so ''/0 overwrite.
• Async PG/MySQL: nodeData.x ?? existingNode.x — ?? treats ''/0 as present.

#3456 (MQTT-sourced nodes appearing nameless) was fixed only at the call site (mqttIngestion stopped passing blanks), not the guard. So any future caller passing a blank name or hwModel: 0 re-introduces it. (No live caller triggers it today — this is hardening.)

Fix — per-column policy (not a blanket skip-empty)

0 is legitimate for most numerics (snr/battery/lat-lon, and role 0 = CLIENT), so only the genuinely-blank-meaningless fields are guarded:

• longName / shortName / macaddr: '' now preserves the stored value (new setIfNonBlank guard on SQLite; a nameOrExisting() helper on the async path).
• hwModel: 0 = HardwareModel.UNSET (unknown) → preserve a known model.
• Everything else unchanged — snr/rssi/battery/voltage/lat-lon/role still write 0.

Test

New nodes.upsertGuard.test.ts:

• A blank re-upsert (longName: '', hwModel: 0, …) preserves the learned name/macaddr/hwModel.
• A genuine snr: 0 / batteryLevel: 0 re-upsert still writes 0 (guards against an over-aggressive truthiness check).

(New file rather than editing nodes.test.ts, to avoid colliding with the in-flight #3501 test-DB-helper migration.)

Validation

• tsc --noEmit: clean
• Full Vitest suite: 6563 pass, 0 fail

🤖 Generated with Claude Code

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review

• Read changed files
• Review code quality and best practices
• Review test coverage
• Check for potential bugs or issues
• Post final review

---

Overall Assessment

This is a well-reasoned hardening fix. The per-column policy (blank strings and hwModel=0 are meaningless, but snr=0 / batteryLevel=0 are valid) is the correct approach. The PR description clearly explains both the latent hole and the fix strategy.

---

Code Quality

upsertNode (async path) — nodes.ts:356-374

The nameOrExisting helper is defined inline inside the if (existingNode) block:

const nameOrExisting = (incoming: any, existing: any) =>
  (incoming === null || incoming === undefined || incoming === '') ? existing : incoming;

Minor concerns:

1. any typing — both parameters could be typed string | null | undefined since this is only used for string identity fields. Not a blocking issue but weakens the type contract.
2. Inline definition — the helper is defined inside the if block on every call to upsertNode. It's a trivial closure but a private class method (or a module-level pure function) would be consistent with setIfProvided / setIfNonBlank in the SQLite path.
3. upsertSet in the insert path (nodes.ts:467-508) doesn't apply the same guard. If the race-condition path fires (concurrent getNode() both return null → both try INSERT, and the second hits the conflict), the upsertSet block writes longName: nodeData.longName ?? null. An '' longName would be persisted here on conflict. In practice this path is a rare race, but it's a missed guard given the PR's stated goal of "hardening."

upsertNodeSqlite — nodes.ts:1729-1744

The SQLite path is clean and consistent. setIfNonBlank is a named helper (mirrors setIfProvided) and the hwModel !== 0 guard is properly commented.

---

Test Coverage

The tests in nodes.upsertGuard.test.ts cover the core cases:

• ✅ Blank re-upsert preserves name/macaddr/hwModel
• ✅ snr: 0 / batteryLevel: 0 still write 0

Gaps to consider:

1. No test for the async upsertNode path. The tests cover upsertNodeSqlite only. A corresponding test for the async (PG/MySQL-compatible) upsertNode would give confidence the nameOrExisting helper works as intended.
2. No test for the concurrent-insert conflict path (upsertSet). This is harder to test but the gap noted above (blank longName surviving the conflict update) isn't covered.
3. shortName whitespace string — ' ' (spaces only) isn't covered. The current guard treats it as "provided" since ' ' !== ''. Whether that's the right behavior is debatable, but worth a comment or a deliberate test.

---

Potential Bug

upsertSet (lines 466–509, used in the conflict branch of the INSERT path) does not apply the blank-string guard:

const upsertSet = {
  longName: nodeData.longName ?? null,  // '' would persist here
  shortName: nodeData.shortName ?? null, // same
  macaddr: nodeData.macaddr ?? null,
  hwModel: nodeData.hwModel ?? null,    // 0 would persist here
  ...
};

This is the same pattern that caused the original bug. Since this is the race-condition fallback (INSERT ... ON CONFLICT DO UPDATE SET), it's unlikely to trigger in practice, but it's inconsistent with the stated fix policy. Consider applying nameOrExisting to longName/shortName/macaddr and a similar hwModel guard in upsertSet.

---

Minor Observations

• The PR correctly leaves role: 0 (CLIENT) unguarded — good, that's intentional.
• The latitude ?? existingNode.latitude pattern for numerics is correct — null/undefined means "not reported"; 0 is a valid coordinate (equator/prime meridian).
• The comment on the nameOrExisting helper (// String identity fields: '' means "blank / not reported", not "clear") is useful and should stay.

---

Summary

The fix is correct for the primary update path (existingNode branch). The one concrete gap is the upsertSet (conflict update on INSERT) block, which is a latent re-introduction of the same bug under a race condition. The test file covers upsertNodeSqlite well but leaves the async upsertNode path and the insert-conflict path untested.

[github-actions]

Claude encountered an error —— View job

---

I'll analyze this and get back to you.

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review

• Read changed files
• Review code quality and best practices
• Review test coverage
• Check for potential bugs or issues
• Post final review

---

Overall Assessment

This is a solid iteration over the previous review. The two main gaps called out before — the upsertSet race-conflict path and missing async-path tests — have both been addressed. The fix is now complete and self-consistent across all three code paths.

---

What was fixed since the last review

upsertSet (race-conflict INSERT path) — nodes.ts:466–514

The previous review flagged that upsertSet wrote longName: nodeData.longName ?? null which would persist '' as a conflicting update. This has been fixed with a blankToNull helper:

const blankToNull = (v: any) => (v === '' || v === null || v === undefined) ? null : v;
const upsertSet = {
  longName: blankToNull(nodeData.longName),   // '' → null ✓
  shortName: blankToNull(nodeData.shortName),  // '' → null ✓
  hwModel: (nodeData.hwModel === 0 || ...) ? null : nodeData.hwModel,  // 0 → null ✓
  ...
};

The comment on this block (lines 464–469) correctly explains the intent: this is the "first-seen" insert path where there's no prior value to preserve, so '' is just normalized to null rather than trying to fall back to an existing value.

Async upsertNode now covered in nodes.test.ts

Tests at lines 284–323 verify the nameOrExisting guard on the async path, mirroring the SQLite guard tests in nodes.upsertGuard.test.ts.

---

Remaining minor observations

macaddr in upsertSet still uses ?? instead of blankToNull — nodes.ts:480

macaddr: nodeData.macaddr ?? null,  // '' would persist here

longName and shortName are guarded with blankToNull in upsertSet, but macaddr is not. This is technically inconsistent with the stated goal. On the insert path a blank macaddr being stored as '' instead of null is probably harmless in practice (the row is brand-new so there's no prior value to clobber), but it's a cosmetic gap worth noting.

nameOrExisting typing — nodes.ts:356

const nameOrExisting = (incoming: any, existing: any) =>

Both parameters are any. They're exclusively used for string | null | undefined identity fields — typing them as string | null | undefined would catch accidental numeric usage at the type level. Minor but worth a future cleanup.

shortName whitespace strings — nodes.upsertGuard.test.ts

' ' (spaces-only) is still treated as "provided" by both guards since ' ' !== ''. No test documents this boundary. The current behavior (spaces persist) is probably fine, but a comment acknowledging the decision would help future readers.

---

No blocking issues

The original latent bug is closed from all three angles:

• Update path (primary): nameOrExisting + hwModel guard ✓
• Insert/conflict path: blankToNull + hwModel null-guard ✓
• SQLite sync path: setIfNonBlank + hwModel guard ✓

Tests cover the async and SQLite paths. The macaddr/blankToNull gap in upsertSet is cosmetic given this is a first-insert path with no existing value to clobber.

---

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review

• Read changed files
• Review code quality and best practices
• Review test coverage
• Check for potential bugs or issues
• Post final review

---

Overall Assessment

This PR is in excellent shape. All gaps from the two previous automated review rounds have been addressed. The fix is now consistent across all three code paths, and the test suite covers the key scenarios well.

---

What's covered

Three-path consistency:

Path	Guard applied
Async upsertNode — update branch (nodes.ts:365–374)	nameOrExisting() for longName/shortName/macaddr; hwModel === 0 guard
Async upsertNode — insert/conflict branch upsertSet (nodes.ts:470–480)	blankToNull() for longName/shortName/macaddr; hwModel === 0 → null
SQLite upsertNodeSqlite — update branch (nodes.ts:1734–1749)	setIfNonBlank() for longName/shortName/macaddr; explicit hwModel !== 0 gate

All three paths are consistent. The previous gap (macaddr using ?? instead of blankToNull in upsertSet) is fixed in commit 977eca5.

Test coverage:

• nodes.upsertGuard.test.ts — SQLite sync path (preserve learned values; genuine snr: 0 / batteryLevel: 0 still write)
• nodes.test.ts:284–323 — async path (same two cases for the nameOrExisting guard)

---

Minor observations (none blocking)

1. nameOrExisting and blankToNull are typed any (nodes.ts:356, nodes.ts:470). Both helpers deal exclusively with string | null | undefined fields. Tightening to string | null | undefined would make accidental numeric usage a type error rather than a silent misuse. Low priority, but worth a follow-up.

2. upsertSet hwModel guard is more verbose than needed (nodes.ts:475):

   hwModel: (nodeData.hwModel === 0 || nodeData.hwModel === null || nodeData.hwModel === undefined) ? null : nodeData.hwModel,

   This is equivalent to nodeData.hwModel || null only for non-negative integers, but the explicit form is more readable and documents intent clearly. No change needed — just noting it reads fine as-is.

3. shortName whitespace strings (' ') are treated as "provided" by both guards since ' ' !== ''. This is probably the right behavior (a node that deliberately sets its short name to spaces should not be silently ignored), but it is an undocumented boundary. A one-line comment noting this edge case would help future readers.

4. upsertSet in the insert path has no role: 0 test. The role: 0 = CLIENT case is intentionally unguarded (correct), but it's not explicitly tested. Given the explicit comment in the PR description about this decision, a test would permanently document the intent. Not blocking.

---

No blocking issues

The latent clobber bug is fully closed across all three paths. The blankToNull + nameOrExisting + setIfNonBlank triad is consistent, well-commented, and covered by tests. This is ready to merge.