diff --git a/src/components/Dashboard/DashboardMap.tsx b/src/components/Dashboard/DashboardMap.tsx index 1c8c30c91..7883b3a5d 100644 --- a/src/components/Dashboard/DashboardMap.tsx +++ b/src/components/Dashboard/DashboardMap.tsx @@ -21,6 +21,7 @@ import { getTilesetById } from '../../config/tilesets'; import type { CustomTileset, TilesetId } from '../../config/tilesets'; import DashboardWaypoints from './DashboardWaypoints'; import DashboardNodePopup, { type NodeSourceRef } from './DashboardNodePopup'; +import DashboardNeighborPopup from './DashboardNeighborPopup'; import GeoJsonOverlay from '../GeoJsonOverlay'; import { TilesetSelector } from '../TilesetSelector'; import MapLegend from '../MapLegend'; @@ -595,12 +596,22 @@ export default function DashboardMap({ ? { color: colorByTransport, weight: 2, opacity: 0.6 } : { color: colorByTransport, weight: 1, opacity: 0.6, dashArray: '5, 5' }; + // Stable key by canonical node-pair (not array index) so the deduped + // line keeps its identity across polls. + const pairKey = link.nodeNum != null && link.neighborNodeNum != null + ? `${Math.min(Number(link.nodeNum), Number(link.neighborNodeNum))}-${Math.max(Number(link.nodeNum), Number(link.neighborNodeNum))}` + : `idx-${idx}`; + return ( + > + + + + ); })} diff --git a/src/components/Dashboard/DashboardNeighborPopup.test.tsx b/src/components/Dashboard/DashboardNeighborPopup.test.tsx new file mode 100644 index 000000000..bc4345465 --- /dev/null +++ b/src/components/Dashboard/DashboardNeighborPopup.test.tsx @@ -0,0 +1,86 @@ +/** + * @vitest-environment jsdom + */ +import { describe, it, expect } from 'vitest'; +import { render, screen } from '@testing-library/react'; +import DashboardNeighborPopup from './DashboardNeighborPopup'; + +const recent = Date.now() - 2 * 60 * 1000; // 2 minutes ago (ms) + +describe('DashboardNeighborPopup', () => { + it('exposes BOTH directions of a bidirectional link (issue #3777)', () => { + render( + , + ); + + // Header shows both endpoints with a bidirectional indicator. + expect(screen.getByText(/Alpha\s*↔\s*Bravo/)).toBeInTheDocument(); + expect(screen.getByText(/Bidirectional/)).toBeInTheDocument(); + + // Forward direction: Alpha → Bravo with its own SNR. + expect(screen.getByText(/Alpha → Bravo: SNR 5\.25 dB/)).toBeInTheDocument(); + // Reverse direction: Bravo → Alpha with the reverse SNR (would be lost + // without the reverse-data backfill). + expect(screen.getByText(/Bravo → Alpha: SNR -2\.50 dB/)).toBeInTheDocument(); + }); + + it('shows a single direction and a one-way indicator when not bidirectional', () => { + render( + , + ); + + // Exact match targets the header title (the SNR row's text is longer). + expect(screen.getByText('Solo → Mate')).toBeInTheDocument(); + expect(screen.getByText(/One-way · MQTT/)).toBeInTheDocument(); + expect(screen.getByText(/Solo → Mate: SNR 8\.00 dB/)).toBeInTheDocument(); + // No reverse row when there's no reverse data. + expect(screen.queryByText(/Mate → Solo/)).not.toBeInTheDocument(); + }); + + it('renders an em dash when a direction has no SNR', () => { + render( + , + ); + + expect(screen.getByText(/X → Y: SNR —/)).toBeInTheDocument(); + expect(screen.getByText(/Y → X: SNR 1\.50 dB/)).toBeInTheDocument(); + }); +}); diff --git a/src/components/Dashboard/DashboardNeighborPopup.tsx b/src/components/Dashboard/DashboardNeighborPopup.tsx new file mode 100644 index 000000000..3e45ee00e --- /dev/null +++ b/src/components/Dashboard/DashboardNeighborPopup.tsx @@ -0,0 +1,89 @@ +/** + * DashboardNeighborPopup — popup shown when a neighbor-info link on the + * Dashboard map is clicked. + * + * Neighbor links are deduplicated to a single polyline per unordered node-pair + * (issue #3777), so a bidirectional A↔B link is drawn once. This popup exposes + * BOTH directions of the link so the reverse direction's signal data isn't lost + * to the dedup: the kept record's own `snr`/`lastRxTime` describe the forward + * direction (node → neighbor), and `reverseSnr`/`reverseLastRxTime` (attached by + * buildSourceNeighborInfo) describe the reverse direction (neighbor → node). + * + * Reuses the `.node-popup-*` classes (styles/nodes.css) for visual parity with + * the node marker popup. + */ + +import { formatRelativeTime } from '../../utils/datetime'; + +interface DashboardNeighborPopupProps { + link: any; +} + +function formatSnr(snr: unknown): string { + return typeof snr === 'number' && Number.isFinite(snr) ? `${snr.toFixed(2)} dB` : '—'; +} + +/** + * Report time for a direction, formatted relative to now. Uses the NeighborInfo + * report `timestamp` (milliseconds — the same field the server freshness window + * divides by 1000). `lastRxTime` is intentionally not used here: its unit is + * ambiguous across firmware and would risk a wrong "heard" age. + */ +function formatHeard(timestamp: unknown): string | null { + const ms = typeof timestamp === 'number' && timestamp > 0 ? timestamp : null; + return ms == null ? null : formatRelativeTime(ms); +} + +const TRANSPORT_LABEL: Record = { + rf: 'RF (LoRa)', + udp: 'UDP', + mqtt: 'MQTT', +}; + +export default function DashboardNeighborPopup({ link }: DashboardNeighborPopupProps) { + const nodeName: string = link?.nodeName ?? link?.nodeId ?? 'Node'; + const neighborName: string = link?.neighborName ?? link?.neighborNodeId ?? 'Neighbor'; + const bidirectional = !!link?.bidirectional; + const transportClass: string = link?.transportClass ?? 'rf'; + + const forwardHeard = formatHeard(link?.timestamp); + const reverseHeard = formatHeard(link?.reverseTimestamp); + // The reverse direction's data exists when the link is bidirectional (the dedup + // dropped that row but stashed its signal on the kept record). + const hasReverse = bidirectional || link?.reverseSnr != null; + + return ( +
+
+
+ {nodeName} {bidirectional ? '↔' : '→'} {neighborName} +
+ + {bidirectional ? 'Bidirectional' : 'One-way'} · {TRANSPORT_LABEL[transportClass] ?? transportClass} + +
+ +
+
+
+ 📶 + + {nodeName} → {neighborName}: SNR {formatSnr(link?.snr)} + {forwardHeard ? ` · heard ${forwardHeard}` : ''} + +
+ + {hasReverse && ( +
+ 📶 + + {neighborName} → {nodeName}: SNR {formatSnr(link?.reverseSnr)} + {reverseHeard ? ` · heard ${reverseHeard}` : ''} + +
+ )} +
+
+
+ ); +} diff --git a/src/db/repositories/nodes.test.ts b/src/db/repositories/nodes.test.ts index ff9afad45..3dfb56a4e 100644 --- a/src/db/repositories/nodes.test.ts +++ b/src/db/repositories/nodes.test.ts @@ -410,6 +410,30 @@ function runNodesTests(getBackend: () => TestBackend) { expect(map.size).toBe(0); }); + it('getNodesByNums - scopes to sourceId when provided (#3780)', async () => { + const backend = getBackend(); + if (!backend.available) { + console.log(`⚠ Skipped: ${backend.skipReason}`); + return; + } + + // Insert the same nodeNum under two sources with different transportMechanism values. + await repo.upsertNode(makeNode(500, { sourceId: 'rf-source', transportMechanism: 1 })); + await repo.upsertNode(makeNode(500, { sourceId: 'mqtt-source', transportMechanism: 5 })); + + const rfMap = await repo.getNodesByNums([500], 'rf-source'); + expect(rfMap.size).toBe(1); + expect(rfMap.get(500)!.transportMechanism).toBe(1); + + const mqttMap = await repo.getNodesByNums([500], 'mqtt-source'); + expect(mqttMap.size).toBe(1); + expect(mqttMap.get(500)!.transportMechanism).toBe(5); + + // Without sourceId, any matching row is returned (both sources present). + const unscoped = await repo.getNodesByNums([500]); + expect(unscoped.size).toBe(1); // at most one row per nodeNum in the result map + }); + // --- getLowBatteryMonitoredNodes --- it('getLowBatteryMonitoredNodes - returns only monitored nodes below threshold, excluding 101 and nulls', async () => { diff --git a/src/db/repositories/nodes.ts b/src/db/repositories/nodes.ts index ea5f4f17f..9d431913d 100644 --- a/src/db/repositories/nodes.ts +++ b/src/db/repositories/nodes.ts @@ -152,7 +152,7 @@ export class NodesRepository extends BaseRepository { /** * Get multiple nodes by nodeNum in a single query */ - async getNodesByNums(nodeNums: number[]): Promise> { + async getNodesByNums(nodeNums: number[], sourceId?: string): Promise> { if (nodeNums.length === 0) return new Map(); // Filter out-of-range values up front (issue #3186) — one bad entry would // otherwise fail the whole batch query against a PG `bigint` column. @@ -164,10 +164,13 @@ export class NodesRepository extends BaseRepository { if (validNums.length === 0) return new Map(); const { nodes } = this.tables; + const whereClause = sourceId + ? and(inArray(nodes.nodeNum, validNums), eq(nodes.sourceId, sourceId)) + : inArray(nodes.nodeNum, validNums); const result = await this.db .select() .from(nodes) - .where(inArray(nodes.nodeNum, validNums)); + .where(whereClause); const map = new Map(); for (const row of result) { @@ -370,6 +373,7 @@ export class NodesRepository extends BaseRepository { role: nodeData.role ?? existingNode.role, hopsAway: nodeData.hopsAway ?? existingNode.hopsAway, viaMqtt: nodeData.viaMqtt ?? existingNode.viaMqtt, + transportMechanism: nodeData.transportMechanism ?? existingNode.transportMechanism, isStoreForwardServer: nodeData.isStoreForwardServer ?? existingNode.isStoreForwardServer, macaddr: nameOrExisting(nodeData.macaddr, existingNode.macaddr), latitude: nodeData.latitude ?? existingNode.latitude, @@ -425,6 +429,7 @@ export class NodesRepository extends BaseRepository { role: nodeData.role ?? null, hopsAway: nodeData.hopsAway ?? null, viaMqtt: nodeData.viaMqtt ?? null, + transportMechanism: nodeData.transportMechanism ?? null, isStoreForwardServer: nodeData.isStoreForwardServer ?? null, macaddr: nodeData.macaddr ?? null, latitude: nodeData.latitude ?? null, @@ -484,6 +489,7 @@ export class NodesRepository extends BaseRepository { role: nodeData.role ?? null, hopsAway: nodeData.hopsAway ?? null, viaMqtt: nodeData.viaMqtt ?? null, + transportMechanism: nodeData.transportMechanism ?? null, isStoreForwardServer: nodeData.isStoreForwardServer ?? null, macaddr: blankToNull(nodeData.macaddr), latitude: nodeData.latitude ?? null, @@ -1757,6 +1763,7 @@ export class NodesRepository extends BaseRepository { setIfProvided('role', nodeData.role); setIfProvided('hopsAway', nodeData.hopsAway); if (nodeData.viaMqtt !== undefined) updateSet.viaMqtt = nodeData.viaMqtt; + if (nodeData.transportMechanism !== undefined) updateSet.transportMechanism = nodeData.transportMechanism; setIfNonBlank('macaddr', nodeData.macaddr); setIfProvided('latitude', nodeData.latitude); setIfProvided('longitude', nodeData.longitude); @@ -1808,6 +1815,7 @@ export class NodesRepository extends BaseRepository { role: nodeData.role || null, hopsAway: nodeData.hopsAway !== undefined ? nodeData.hopsAway : null, viaMqtt: nodeData.viaMqtt !== undefined ? !!nodeData.viaMqtt : null, + transportMechanism: nodeData.transportMechanism !== undefined ? nodeData.transportMechanism : null, macaddr: nodeData.macaddr || null, latitude: nodeData.latitude || null, longitude: nodeData.longitude || null, diff --git a/src/hooks/useDashboardData.test.ts b/src/hooks/useDashboardData.test.ts index fb34471c1..df5dcb84c 100644 --- a/src/hooks/useDashboardData.test.ts +++ b/src/hooks/useDashboardData.test.ts @@ -439,6 +439,59 @@ describe('mergeUnifiedSourceData', () => { expect([...classes].sort()).toEqual(['mqtt', 'rf']); }); + it('deduplicates neighbor links across sources (#3777)', () => { + // When two sources both report A-B as neighbors, only one line should render. + const linkAB = { nodeNum: 100, neighborNodeNum: 200, transportClass: 'rf', timestamp: 1000 }; + const linkABdupe = { nodeNum: 100, neighborNodeNum: 200, transportClass: 'mqtt', timestamp: 2000 }; + const merged = mergeUnifiedSourceData([ + { + sourceId: 's1', sourceName: 'LoRa', protocol: 'Meshtastic', + nodes: [], traceroutes: [], channels: [], + neighborInfo: [linkAB], + }, + { + sourceId: 's2', sourceName: 'MQTT', protocol: 'Meshtastic', + nodes: [], traceroutes: [], channels: [], + neighborInfo: [linkABdupe], + }, + ]); + // Only the first-seen link per canonical pair survives. + expect(merged.neighborInfo).toHaveLength(1); + expect((merged.neighborInfo[0] as any).nodeNum).toBe(100); + }); + + it('deduplicates reversed neighbor pairs across sources (#3777)', () => { + // Source 1 reports A→B; source 2 reports B→A. Canonical pair is the same. + const linkAB = { nodeNum: 100, neighborNodeNum: 200, transportClass: 'rf', timestamp: 1000 }; + const linkBA = { nodeNum: 200, neighborNodeNum: 100, transportClass: 'rf', timestamp: 1000 }; + const merged = mergeUnifiedSourceData([ + { + sourceId: 's1', sourceName: 'S1', protocol: 'Meshtastic', + nodes: [], traceroutes: [], channels: [], + neighborInfo: [linkAB], + }, + { + sourceId: 's2', sourceName: 'S2', protocol: 'Meshtastic', + nodes: [], traceroutes: [], channels: [], + neighborInfo: [linkBA], + }, + ]); + expect(merged.neighborInfo).toHaveLength(1); + }); + + it('keeps distinct neighbor links with different pairs', () => { + const linkAB = { nodeNum: 100, neighborNodeNum: 200, transportClass: 'rf', timestamp: 1000 }; + const linkAC = { nodeNum: 100, neighborNodeNum: 300, transportClass: 'rf', timestamp: 1000 }; + const merged = mergeUnifiedSourceData([ + { + sourceId: 's1', sourceName: 'S1', protocol: 'Meshtastic', + nodes: [], traceroutes: [], channels: [], + neighborInfo: [linkAB, linkAC], + }, + ]); + expect(merged.neighborInfo).toHaveLength(2); + }); + it('keeps distinct MeshCore nodes (different publicKeys) separate', () => { const merged = mergeUnifiedSourceData([ { diff --git a/src/hooks/useDashboardData.ts b/src/hooks/useDashboardData.ts index 7e00bb7aa..7f544571f 100644 --- a/src/hooks/useDashboardData.ts +++ b/src/hooks/useDashboardData.ts @@ -334,6 +334,9 @@ export function mergeUnifiedSourceData( const recordsByKey = new Map>(); const traceroutes: unknown[] = []; const neighborInfo: unknown[] = []; + // Tracks canonical (min,max) nodeNum pairs already added to neighborInfo so the + // same physical link reported by multiple sources renders as a single line. + const seenNeighborPairs = new Set(); let channels: unknown[] = []; for (const ps of perSource) { @@ -369,7 +372,22 @@ export function mergeUnifiedSourceData( else recordsByKey.set(key, [entry]); } traceroutes.push(...ps.traceroutes); - neighborInfo.push(...ps.neighborInfo); + for (const link of ps.neighborInfo as any[]) { + if (link == null) continue; + // Deduplicate across sources: the same physical pair (A,B) reported by + // multiple sources should render as a single line. Server-side + // buildSourceNeighborInfo already collapses bidirectional A↔B pairs within + // one source; here we also collapse the same pair appearing in a second + // source's neighborInfo bundle. + const numA = Number(link.nodeNum ?? 0); + const numB = Number(link.neighborNodeNum ?? 0); + if (numA !== 0 || numB !== 0) { + const pairKey = `${Math.min(numA, numB)}-${Math.max(numA, numB)}`; + if (seenNeighborPairs.has(pairKey)) continue; + seenNeighborPairs.add(pairKey); + } + neighborInfo.push(link); + } if (channels.length === 0 && ps.channels.length > 0) { channels = ps.channels; } diff --git a/src/server/routes/sourceRoutes.neighbor-info.test.ts b/src/server/routes/sourceRoutes.neighbor-info.test.ts index 1f0e10bca..adccc3e02 100644 --- a/src/server/routes/sourceRoutes.neighbor-info.test.ts +++ b/src/server/routes/sourceRoutes.neighbor-info.test.ts @@ -138,8 +138,8 @@ describe('GET /:id/neighbor-info', () => { }); it('enriches records with node names, positions, and bidirectionality', async () => { - const ni1 = makeNeighborRecord({ nodeNum: 111, neighborNodeNum: 222 }); - const ni2 = makeNeighborRecord({ id: 2, nodeNum: 222, neighborNodeNum: 111 }); // bidirectional + const ni1 = makeNeighborRecord({ nodeNum: 111, neighborNodeNum: 222, snr: -5.5 }); + const ni2 = makeNeighborRecord({ id: 2, nodeNum: 222, neighborNodeNum: 111, snr: 3.25 }); // bidirectional (reverse) mockDb.sources.getSource.mockResolvedValue(MOCK_SOURCE); mockDb.neighbors.getAllNeighborInfo.mockResolvedValue([ni1, ni2]); @@ -150,7 +150,9 @@ describe('GET /:id/neighbor-info', () => { const res = await request(createApp()).get('/src-abc/neighbor-info'); expect(res.status).toBe(200); - expect(res.body).toHaveLength(2); + // Bidirectional pairs are deduplicated: only the canonical direction (smaller + // nodeNum first) is returned, with bidirectional=true on the kept record. + expect(res.body).toHaveLength(1); const first = res.body[0]; expect(first.nodeId).toBe('!0000006f'); @@ -158,6 +160,11 @@ describe('GET /:id/neighbor-info', () => { expect(first.neighborNodeId).toBe('!000000de'); expect(first.neighborName).toBe('Node 222'); expect(first.bidirectional).toBe(true); + // Forward direction's own SNR is the kept record's snr; the reverse + // direction's SNR is backfilled so the map popup can show both (#3777). + expect(first.snr).toBe(-5.5); + expect(first.reverseSnr).toBe(3.25); + expect(typeof first.reverseTimestamp).toBe('number'); expect(typeof first.nodeLatitude).toBe('number'); expect(typeof first.nodeLongitude).toBe('number'); expect(typeof first.neighborLatitude).toBe('number'); diff --git a/src/server/services/sourceDashboardData.ts b/src/server/services/sourceDashboardData.ts index 8a8285bbc..75b4cb073 100644 --- a/src/server/services/sourceDashboardData.ts +++ b/src/server/services/sourceDashboardData.ts @@ -234,7 +234,9 @@ export async function buildSourceNeighborInfo( ...neighborInfo.map(ni => ni.nodeNum), ...neighborInfo.map(ni => ni.neighborNodeNum), ])]; - const nodeMap = await databaseService.nodes.getNodesByNums(allNodeNums); + // Scope node lookup to this source so transportMechanism reflects how THIS + // source hears each node (not a different source's RF/MQTT classification). + const nodeMap = await databaseService.nodes.getNodesByNums(allNodeNums, source.id); // Same channel gate the nodes endpoint uses, so a neighbor-info link whose // endpoint nodes the user can't see doesn't leak their positions (#3092). @@ -245,11 +247,35 @@ export async function buildSourceNeighborInfo( ); const visibleNodeNums = new Set(visibleNodes.map(n => Number((n as any).nodeNum))); + // Deduplicate bidirectional links: if both A→B and B→A are present, keep only + // the canonical direction (smaller nodeNum first) to avoid two overlapping + // polylines on the map. The bidirectional flag on the kept record signals that + // both directions exist. + const seenPairs = new Set(); + + // Directed-key lookup so the kept canonical record can surface the OTHER + // direction's signal data in the map popup (issue #3777: clicking a link must + // expose BOTH directions). The dedup above drops the reverse row, so capture + // each directed observation here first. Keyed `${from}-${to}`. + const byDirected = new Map(); + for (const ni of neighborInfo) { + byDirected.set(`${ni.nodeNum}-${ni.neighborNodeNum}`, ni); + } + const enrichedNeighborInfo = neighborInfo .filter(ni => visibleNodeNums.has(Number(ni.nodeNum)) && visibleNodeNums.has(Number(ni.neighborNodeNum)), ) + .filter(ni => { + // Canonical pair key: always smaller nodeNum first. + const a = Math.min(ni.nodeNum, ni.neighborNodeNum); + const b = Math.max(ni.nodeNum, ni.neighborNodeNum); + const pairKey = `${a}-${b}`; + if (seenPairs.has(pairKey)) return false; + seenPairs.add(pairKey); + return true; + }) .map(ni => { const node = nodeMap.get(ni.nodeNum) ?? null; const neighbor = nodeMap.get(ni.neighborNodeNum) ?? null; @@ -263,6 +289,11 @@ export async function buildSourceNeighborInfo( const isUdp = nTx === TransportMechanism.MULTICAST_UDP || nbTx === TransportMechanism.MULTICAST_UDP; const transportClass: 'rf' | 'udp' | 'mqtt' = isMqtt ? 'mqtt' : isUdp ? 'udp' : 'rf'; + // The kept record's own snr/lastRxTime (spread from `...ni`) describe the + // forward direction (nodeNum → neighborNodeNum). The reverse direction was + // dropped by the dedup filter, so attach its signal data for the popup. + const reverse = byDirected.get(`${ni.neighborNodeNum}-${ni.nodeNum}`); + return { ...ni, nodeId: node?.nodeId || `!${ni.nodeNum.toString(16).padStart(8, '0')}`, @@ -270,6 +301,8 @@ export async function buildSourceNeighborInfo( neighborNodeId: neighbor?.nodeId || `!${ni.neighborNodeNum.toString(16).padStart(8, '0')}`, neighborName: neighbor?.longName || `Node !${ni.neighborNodeNum.toString(16).padStart(8, '0')}`, bidirectional: linkKeys.has(`${ni.neighborNodeNum}-${ni.nodeNum}`), + reverseSnr: reverse?.snr ?? null, + reverseTimestamp: reverse?.timestamp ?? null, transportClass, nodeLatitude: nodePos.latitude, nodeLongitude: nodePos.longitude,