fix(neighbor-info): deduplicate bidirectional links and fix MQTT transport class (#3777, #3780)

[Yeraze]

Closes #3777
Closes #3780

Summary

Fixes two related bugs in how neighbor links are built and displayed on the dashboard map.

Bug #3780 — MQTT neighbor lines not affected by "Show RF" toggle

Root cause (two-part):

1. upsertNode in nodes.ts included transportMechanism in neither its UPDATE nor INSERT paths (including the legacy upsertNodeSqlite sync path), so the field added by migration 066 was never actually written to the DB even though meshtasticManager.ts correctly passed it in.

2. buildSourceNeighborInfo in sourceDashboardData.ts called getNodesByNums(allNodeNums) without a sourceId, so in multi-source deployments it could pick up a row from a different source — giving an MQTT node the RF transport class of a same-nodeNum row from another source.

Fix:

• Add transportMechanism to upsertNode and upsertNodeSqlite's INSERT and UPDATE sets.
• Add an optional sourceId parameter to getNodesByNums and use it in buildSourceNeighborInfo so node transport classification is always scoped to the source that heard the packet.

Bug #3777 — Neighbor lines drawn twice + missing signal info in both directions

When both A→B and B→A records exist in the DB, both were being sent to the map (two overlapping polylines). Additionally, the reverse direction's SNR/timestamp was being discarded by the dedup.

Fix:

• In buildSourceNeighborInfo: deduplicate using a canonical pair key (min(A,B)-max(A,B)). Only the smaller-nodeNum-first direction is kept; bidirectional is set to true and the dropped reverse link's SNR/timestamp is backfilled as reverseSnr/reverseTimestamp on the kept record.
• In mergeUnifiedSourceData (useDashboardData.ts): same dedup across sources.
• In DashboardMap.tsx: neighbor polylines now have a DashboardNeighborPopup showing transport class, bidirectional state, and both directions' SNR + report time.

Changes

File	Change
src/db/repositories/nodes.ts	Add transportMechanism to upsertNode and upsertNodeSqlite INSERT/UPDATE; add optional sourceId to getNodesByNums
src/server/services/sourceDashboardData.ts	Source-scoped node lookup + within-source bidirectional dedup + reverseSnr/reverseTimestamp backfill
src/hooks/useDashboardData.ts	Cross-source neighbor dedup in mergeUnifiedSourceData
src/components/Dashboard/DashboardMap.tsx	Stable polyline keys + DashboardNeighborPopup on each line
src/components/Dashboard/DashboardNeighborPopup.tsx	New popup showing transport, bidirectionality, and per-direction SNR/time
src/db/repositories/nodes.test.ts	Test for getNodesByNums sourceId scoping
src/hooks/useDashboardData.test.ts	3 tests for cross-source neighbor dedup
src/server/routes/sourceRoutes.neighbor-info.test.ts	Updated expectations + reverseSnr assertion
src/components/Dashboard/DashboardNeighborPopup.test.tsx	Tests for popup rendering (both-directions / one-way / missing-SNR)

Test plan

• npx vitest run src/db/repositories/nodes.test.ts — sourceId-scoping test passes
• npx vitest run src/hooks/useDashboardData.test.ts — 3 new dedup tests pass
• npx vitest run src/server/routes/sourceRoutes.neighbor-info.test.ts — length 1, bidirectional=true, reverseSnr correct
• npx vitest run src/components/Dashboard/DashboardNeighborPopup.test.tsx — all popup tests pass
• On a multi-source deployment with MQTT nodes: MQTT neighbor lines respond to Show MQTT toggle, RF lines to Show RF
• Verify single polyline per link; clicking shows popup with both directions' SNR

🤖 Generated with Claude Code

https://claude.ai/code/session_01Jf83jmbVNEgu8Fh4Dvuqea

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review

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

---

Overall Assessment

This is a well-targeted, clean fix for two genuine bugs. The root-cause analysis in the PR description is accurate, and the implementation is consistent with the codebase's existing patterns.

---

Bug Fix Analysis

Bug #3780 (MQTT transport class): The fix is correct. transportMechanism was previously omitted from upsertNode's UPDATE and INSERT sets despite migration 066 adding the column. It's now added in both paths (lines 376 and 432 in nodes.ts).

Bug #3777 (duplicate polylines): The dedup logic is correct. Using min(A,B)-max(A,B) as the canonical pair key is the right approach for unordered pairs.

---

Issues Found

1. Missing upsertNodeSqlite fix (nodes.ts:1765–1766)

The SQLite sync path (upsertNodeSqlite) still doesn't write transportMechanism in either the update or insert branch. The async upsertNode was fixed, but this sync variant — used by the legacy SQLite facade — was skipped. Any SQLite deploy calling the sync path will still silently drop transportMechanism.

Specifically:

• Update path (~line 1765): if (nodeData.viaMqtt !== undefined) updateSet.viaMqtt = nodeData.viaMqtt; is present, but no equivalent for transportMechanism.
• Insert path (~line 1808): viaMqtt is included in the db.insert(nodes).values({...}) call but transportMechanism is not.

2. getNodesByNums can still return wrong row in multi-source (nodes.ts:175–180)

When sourceId is provided, the WHERE clause correctly scopes the query. However, when multiple rows for the same nodeNum exist across sources (and no sourceId is given), the Map only retains one entry per nodeNum (last write wins). This was the existing behavior before this PR and isn't a regression — but worth noting since the test at line 433 only asserts unscoped.size === 1, not which row is kept.

3. Cross-source neighbor dedup discards transport metadata (useDashboardData.ts:382–389)

In mergeUnifiedSourceData, when a duplicate pair is detected across sources, the first-seen link is kept and the later one is silently dropped. If source 1 reports A→B as rf and source 2 reports A→B as mqtt, the result keeps the RF classification even though MQTT is also a valid transport for that link. This is a minor limitation — the fix handles the primary dedup goal, but the transport class on the surviving record may not reflect all transport mechanisms active on that link. The comment acknowledges only the single-source bidirectional case was server-side fixed; this cross-source transport-class union isn't addressed.

4. bidirectional flag only set within a source (sourceDashboardData.ts:289)

bidirectional: linkKeys.has(`${ni.neighborNodeNum}-${ni.nodeNum}`),

linkKeys is built from neighborInfo which is scoped to a single source. So if source 1 reports A→B and source 2 reports B→A, neither source will set bidirectional=true — it's only set when both directions exist within the same source's neighbor records. This is a pre-existing limitation not introduced by this PR, but it's worth being aware of.

---

Code Quality

• The seenPairs Set approach is clean and O(n) — no concerns.
• The sourceId parameter on getNodesByNums follows existing patterns from getNode, getAllNodes, etc. correctly.
• The withSourceScope helper already existed for this pattern; the PR opts for the manual and(inArray(...), eq(...)) approach instead for the new getNodesByNums sourceId path. Minor style inconsistency but functionally equivalent.

---

Test Coverage

Tests are well-written:

• sourceId-scoping test in nodes.test.ts verifies isolation between rf-source and mqtt-source with different transportMechanism values — good.
• The three new mergeUnifiedSourceData tests cover same-pair dedup, reversed dedup, and distinct pairs — complete coverage for the cross-source case.
• The existing sourceRoutes.neighbor-info.test.ts was correctly updated to expect length 1 with bidirectional=true.

Missing test: No test verifies that upsertNodeSqlite correctly writes transportMechanism. Since SQLite is the default and most common deployment, this should be tested.

---

Minor Suggestions

The existing concatenates traceroutes and neighborInfo across sources test at line 321 still expects neighborInfo: [{ id: 'n1' }, { id: 'n2' }] — these records have no nodeNum/neighborNodeNum, so the dedup guard's numA !== 0 || numB !== 0 condition (line 384) lets them pass through. This works correctly, but the test predates the dedup change and is a bit accidental. Not a bug, just something to be aware of.

---

Summary

The core fixes are correct and the logic is sound. The main actionable gap is the missing transportMechanism in upsertNodeSqlite (issue 1 above) — this will cause the bug to persist on SQLite deployments if the sync path is exercised. Issues 3 and 4 are pre-existing limitations that are out of scope for this PR.

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review

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

---

Overall Assessment

This is a clean, well-scoped fix for two genuine bugs. The previous review cycle identified a gap in upsertNodeSqlite — that has now been addressed in the follow-up commit (d2aad5f). The code is in good shape.

---

What Changed Since Last Review

upsertNodeSqlite is now fixed — both the update path (line 1766) and the insert path (line 1818) correctly include transportMechanism. The previous review's primary actionable concern is resolved.

---

Bug Fix Verification

Bug #3780 (MQTT transport class):

• upsertNode async path: ✅ transportMechanism at lines 376 (UPDATE) and 432 (INSERT)
• upsertNodeSqlite sync path: ✅ transportMechanism at lines 1766 (UPDATE) and 1818 (INSERT)
• getNodesByNums source scoping: ✅ and(inArray(nodes.nodeNum, validNums), eq(nodes.sourceId, sourceId)) at line 168
• buildSourceNeighborInfo uses source.id when calling getNodesByNums: ✅ line 239

Bug #3777 (duplicate polylines):

• Within-source dedup via seenPairs: ✅ lines 254–268 in sourceDashboardData.ts
• Cross-source dedup via seenNeighborPairs: ✅ lines 382–388 in useDashboardData.ts
• Canonical key (min(A,B)-max(A,B)) is consistent between both dedup locations: ✅

---

Known Limitations (Pre-existing, Not Introduced Here)

1. Cross-source transport class on deduped links — When source 1 reports A→B as rf and source 2 also reports A→B as mqtt, the first-seen link's transportClass is kept and the second is silently dropped. The transportClass on the surviving record may not reflect all active transports on that link. This is a pre-existing limitation acknowledged in the PR.

2. bidirectional flag is per-source only — If source 1 reports A→B and source 2 reports B→A (two different sources), the cross-source dedup will retain one record but neither will have bidirectional=true. The bidirectional flag is set in buildSourceNeighborInfo using linkKeys scoped to a single source's neighbor records.

Both limitations are out of scope for this PR and are noted only for awareness.

---

Test Coverage

Tests are thorough and well-structured:

• nodes.test.ts — New test at line 420 correctly verifies source isolation for getNodesByNums, asserting different transportMechanism values are kept separate per source.
• useDashboardData.test.ts — Three new tests covering same-pair dedup, reversed-pair dedup, and distinct-pair passthrough. The reversed-pair test (line 463) is particularly important as it validates the canonical key direction.
• sourceRoutes.neighbor-info.test.ts — Updated expectation (line 155) correctly reflects that [ni1, ni2] now yields a single canonical record with bidirectional=true.

One minor gap: The upsertNodeSqlite sync path fix (d2aad5f) doesn't have a dedicated test verifying that transportMechanism persists correctly through the sync path. The async path is covered by the nodes.test.ts test (which uses the async upsertNode), but since the SQLite sync path is the default deployment path, a test would be valuable. Not blocking, but worth adding.

---

Code Quality

• The seenPairs Set approach is O(n) — appropriate for the expected data volumes.
• The optional sourceId parameter on getNodesByNums follows the same pattern as getNodeByNodeId, getNode, and getAllNodes — consistent with established conventions.
• The (node as any)?.transportMechanism cast in sourceDashboardData.ts lines 276–277 reflects a type gap in DbNode — not introduced by this PR, but if transportMechanism is being used more broadly now that it's correctly persisted, updating the DbNode type to include it explicitly would be a nice follow-up.

---

Summary

The fixes are correct, the primary concern from the previous review is resolved, and the test coverage is solid. The two known limitations are pre-existing and explicitly scoped out. This PR is ready to merge.

[github-actions]

Claude finished @Yeraze's task —— View job

---

PR Review

• Read changed files
• Analyze new components (DashboardNeighborPopup, DashboardMap changes)
• Review code quality and best practices
• Check for potential bugs
• Review test coverage
• Post final review

---

Overall Assessment

This is a solid, well-scoped PR that fixes two genuine bugs. The previous two review cycles caught and resolved the main issues (missing transportMechanism in upsertNodeSqlite, source-scoped node lookup). The newest commit adds the DashboardNeighborPopup component and wires it into DashboardMap, completing the UX side of the dedup fix. The code is in good shape overall.

---

New in This Commit: DashboardNeighborPopup

Design is correct. The popup correctly exposes both directions of a deduplicated bidirectional link. The reverseSnr/reverseTimestamp fields attached by buildSourceNeighborInfo (lines 304–305 in sourceDashboardData.ts) are used here, so the signal data from the dropped reverse row is not lost to the user.

Minor issues:

1. link: any prop type (DashboardNeighborPopup.tsx:19)

The prop is typed as any. Given that sourceDashboardData.ts already builds a well-shaped object with known fields (snr, reverseSnr, timestamp, reverseTimestamp, bidirectional, transportClass, etc.), a local interface would be more robust and catch future breakage:

interface NeighborLink {
  nodeNum?: number;
  neighborNodeNum?: number;
  nodeName?: string;
  neighborName?: string;
  bidirectional?: boolean;
  transportClass?: string;
  snr?: number | null;
  timestamp?: number | null;
  reverseSnr?: number | null;
  reverseTimestamp?: number | null;
}

Not blocking, but consistent with how DashboardNodePopup handles its props.

2. hasReverse condition is redundant (DashboardNeighborPopup.tsx:53)

const hasReverse = bidirectional || link?.reverseSnr != null;

In practice, bidirectional=true is always set when reverseSnr is populated (see sourceDashboardData.ts:303–304), so the || link?.reverseSnr != null branch is defensive code that can't be triggered through the normal data path. Not a bug, just noise. The tests don't exercise the !bidirectional && reverseSnr != null case either.

3. Transport label fallback in subtitle (DashboardNeighborPopup.tsx:62)

{TRANSPORT_LABEL[transportClass] ?? transportClass}

This is good defensive fallback — if a new transport class is added to the backend ('serial', etc.) it renders the raw string rather than undefined. Clean.

---

DashboardMap.tsx Changes

Neighbor link rendering (lines 569–616): The transport-based color and style logic is correct. RF links render as solid blue, MQTT as solid green, UDP as solid orange. Bidirectional links get weight: 2, one-way links get weight: 1 with a dashArray. The canonical pair key (min-max) matches the server-side dedup key — good.

One potential visual issue: The color for RF links is the literal string 'blue' (line 594), while MQTT and UDP use hex codes ('#22c55e', '#f97316'). This is pre-existing behavior, not introduced here, but it means RF links use the browser's named blue (#0000ff) which can be harder to see on some basemaps. Not blocking.

Key stability (lines 601–603): The pairKey fallback to idx-${idx} when nodeNum/neighborNodeNum are missing is correct — it avoids React key collisions while ensuring that enriched records (which always have those fields) use stable canonical keys across polls.

---

Previously Flagged Issues — Status

Issue	Status
upsertNodeSqlite missing transportMechanism	✅ Fixed in d2aad5f
getNodesByNums source scoping	✅ Fixed
Within-source bidirectional dedup	✅ Fixed
Cross-source dedup in mergeUnifiedSourceData	✅ Fixed
Type gap: (node as any)?.transportMechanism in sourceDashboardData.ts	Pre-existing, still present — follow-up candidate
Missing test for upsertNodeSqlite sync path	Still missing — noted but non-blocking

---

Test Coverage

The three new DashboardNeighborPopup tests are well-chosen:

• Bidirectional link: verifies both direction rows render with correct SNR values ✅
• One-way link: verifies no reverse row, one-way indicator ✅
• Missing SNR (null): verifies em-dash fallback ✅

One gap: the hasReverse guard covers bidirectional=false but reverseSnr != null — this state isn't reachable in production but could be worth a test if the condition is intentional.

---

Summary

The core fixes are correct, the new popup component is clean, and the UX change (clickable polylines showing both directions) directly addresses the dedup's information loss. The link: any type and the 'blue' color string are minor pre-existing style issues not introduced by this PR. Ready to merge.

[Yeraze]

Pushed 98ffc4b0 to complete the #3777 "expose both directions on click" requirement, which the dedup alone didn't cover (the dedup drops the reverse row, so its SNR was being lost).

Backend (sourceDashboardData.ts): a directed-key lookup now backfills the dropped reverse direction's signal onto the kept canonical record as reverseSnr / reverseTimestamp.

Frontend: the neighbor <Polyline> now has a <Popup> (DashboardNeighborPopup) showing the transport, bidirectional/one-way state, and both directions' SNR + report time. Also keyed the line by canonical node-pair instead of array index for stable identity across polls.

Tests: new DashboardNeighborPopup.test.tsx (both-directions / one-way / missing-SNR) and a reverse-SNR assertion added to sourceRoutes.neighbor-info.test.ts.

Full suite: 7603 passed / 0 failed (the 4 failing suites are the known worktree-only ts-overlapping-marker-spiderfier-leaflet resolution issue, unrelated). Server tsc clean.

With this, #3784 now resolves all of #3777 (items 1–3) and #3780 — the body can take Closes #3777 / Closes #3780.