Skip to content

fix(meshcore): preserve node name on empty advert (#3756)#3785

Merged
Yeraze merged 1 commit into
mainfrom
fix/3756-preserve-node-name
Jun 26, 2026
Merged

fix(meshcore): preserve node name on empty advert (#3756)#3785
Yeraze merged 1 commit into
mainfrom
fix/3756-preserve-node-name

Conversation

@Yeraze

@Yeraze Yeraze commented Jun 26, 2026

Copy link
Copy Markdown
Owner

Summary

Fixes #3756 — MeshCore nodes showing as unnamed in the node list after a re-advertise.

Root cause: Zero-hop repeaters (and some MeshCore firmware builds) send contact_advertised events with adv_name: "". The ?? null-coalescing operator passes an empty string through unchanged (since "" is not null/undefined), so the stored node name was silently overwritten with an empty value on every re-advert.

Fix: Switch ??|| in two places so an empty name falls back to the previously known value, the same way null/undefined already did:

  • handleBridgeEvent (in-memory contact merge) — meshcoreManager.ts:1112
  • persistContact (DB write into meshcore_nodes) — meshcoreManager.ts:1473

Test: New regression case in meshcoreManager.contactPersistence.test.ts fires a named advert followed by an empty-name re-advert and asserts both the in-memory contact (getContact) and the upsertNode DB call retain the original name.

Provenance

This is the minimal, still-relevant half of the now-closed PRs #3771 / #3772 (which were duplicates of each other). Their other half — the #3769 message-metadata contrast fix — already shipped in #3778, so those PRs were closed and this carries forward only the node-name fix, rebased clean on current main with no CSS changes.

Test plan

  • vitest run src/server/meshcoreManager.contactPersistence.test.ts — 10/10 pass (incl. new regression)
  • tsc -p tsconfig.server.json --noEmit — clean
  • Manual: zero-hop repeater (MeshCore firmware) keeps its name after a re-advert cycle

🤖 Generated with Claude Code

Zero-hop repeaters (and some firmware builds) emit `contact_advertised`
events with `adv_name: ""`. The `??` null-coalescing operator passes an
empty string through unchanged, so the stored node name was silently
overwritten with "" on every re-advert, leaving the node unnamed in the
list.

Switch `??` to `||` in both the in-memory contact merge (handleBridgeEvent)
and the DB write (persistContact) so an empty name falls back to the
previously known value, the same way null/undefined already did.

Adds a regression test that fires a named advert followed by an empty-name
re-advert and asserts both the in-memory contact and the upsertNode DB call
retain the original name.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VBLhGGNh35oMwTL53va1Y5
@github-actions

github-actions Bot commented Jun 26, 2026

Copy link
Copy Markdown

Claude finished @Yeraze's task —— View job


PR Review: fix(meshcore): preserve node name on empty advert (#3756)

Tasks:

  • Gather context and read changed files
  • Review code changes
  • Assess test coverage
  • Provide final review

Summary

This is a clean, minimal, well-targeted fix. The root cause analysis is correct, the fix is idiomatic, and the regression test is solid. Ready to merge with one minor observation noted below.


Code Review

meshcoreManager.ts — Core Fix ✅

The ??|| change at lines 1115 and 1478 is correct and precisely targeted:

// Line 1115 — in-memory merge
advName: data.adv_name || existing.advName,

// Line 1478 — DB persistence
name: contact.advName || contact.name || null,
  • In-memory merge (handleBridgeEvent, line 1115): Falls back to existing.advName when data.adv_name is "". This correctly preserves the known name on re-advertise.
  • DB persistence (persistContact, line 1478): The chain contact.advName || contact.name || null is slightly more complex. Because advName is already preserved by the in-memory merge at the time persistContact is called, contact.advName will always hold the retained name by this point. The fallback to contact.name is a belt-and-suspenders guard for contacts populated via a path other than handleBridgeEvent. This is defensively sound.

Inline comments explain the ?? vs || reasoning clearly — good documentation for future reviewers.

lxc/sparse-cone.txt ⚠️ Minor Observation

The diff shows only whitespace/line-ending normalization (CRLF/LF or trailing newline) plus the addition of lxc to the directory list. The functional change (adding lxc) is correct per CLAUDE.md rules, but the bulk whitespace churn is noise in this PR. It doesn't affect functionality and the content is correct, but it makes the diff harder to read and could mask future changes. Consider normalizing line endings in a dedicated commit, or just keeping the existing whitespace if the file isn't being moved.

This does not block the PR — the end state of the file is correct.


Test Coverage ✅

The new regression test (lines 288–330) is well-constructed:

  1. Dispatches a named contact_advertised event → establishes the known name
  2. Dispatches a second contact_advertised with adv_name: "" → the empty-name re-advert
  3. Asserts getContact().advName === 'MyRepeater' — in-memory retention confirmed
  4. Asserts upsertNode was called with name: 'MyRepeater' — DB write retention confirmed

Both sides of the fix (in-memory + DB) are covered. The existing 9 tests are unaffected.

One optional enhancement: a test that sends an initial advert with no adv_name key at all (vs adv_name: ""), then a re-advert with a real name, to verify name assignment works forward as well as backward. Not required for this fix, but would round out the coverage.


Verdict

Approve. The fix is correct, minimal, and well-tested. The lxc/sparse-cone.txt whitespace noise is cosmetic and doesn't affect correctness.


@Yeraze Yeraze merged commit 35bf160 into main Jun 26, 2026
19 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant