From 72ca3313d8e42541c08a45a93ccc8bf0248ee85e Mon Sep 17 00:00:00 2001 From: James Rich <2199651+jamesarich@users.noreply.github.com> Date: Tue, 28 Jul 2026 10:11:09 -0500 Subject: [PATCH] fix(node): disambiguate signal-log packet keys (#6492) Co-authored-by: Claude Opus 5 --- .../feature/node/metrics/SignalMetrics.kt | 25 +++--- .../feature/node/metrics/SignalLogKeyTest.kt | 76 +++++++++++++++++++ 2 files changed, 90 insertions(+), 11 deletions(-) create mode 100644 feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/metrics/SignalLogKeyTest.kt diff --git a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/SignalMetrics.kt b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/SignalMetrics.kt index 584e12d87a..16729b6840 100644 --- a/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/SignalMetrics.kt +++ b/feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/metrics/SignalMetrics.kt @@ -111,7 +111,14 @@ private val LEGEND_DATA = LegendData(nameRes = Res.string.snr, color = SignalMetric.SNR.color), ) -private sealed interface SignalLogEntry { +/** Builds the combined signal log, newest first. Kept out of the composable so the LazyColumn keys are testable. */ +internal fun buildSignalLog(signalData: List, localStatsData: List): List { + val localStats = localStatsData.mapIndexed { index, telemetry -> SignalLogEntry.LocalStatsEntry(telemetry, index) } + val packets = signalData.mapIndexed { index, packet -> SignalLogEntry.PacketEntry(packet, index) } + return (localStats + packets).sortedByDescending { it.timeSeconds } +} + +internal sealed interface SignalLogEntry { val timeSeconds: Int /** Stable, collision-free identity for use as a LazyColumn item key across both entry types. */ @@ -128,9 +135,12 @@ private sealed interface SignalLogEntry { override val contentType: Any = "local_stats" } - data class PacketEntry(val meshPacket: MeshPacket) : SignalLogEntry { + data class PacketEntry(val meshPacket: MeshPacket, val index: Int) : SignalLogEntry { override val timeSeconds: Int = meshPacket.rx_time - override val key: Any = "packet_${meshPacket.id}" + + // MeshPacket.id repeats: it is unique only per originating node, and retransmissions are stored per reception. + // The source-list index disambiguates, as it does for local stats. + override val key: Any = "packet_${meshPacket.id}_$index" override val contentType: Any = "signal_packet" } } @@ -144,14 +154,7 @@ fun SignalMetricsScreen(viewModel: MetricsViewModel, onNavigateUp: () -> Unit, m val threshold = timeFrame.timeThreshold() val signalData = state.signalMetrics.filter { it.rx_time.toLong() >= threshold } val localStatsData = state.localStats.filter { it.time.toLong() >= threshold && it.local_stats != null } - val data = - remember(signalData, localStatsData) { - ( - localStatsData.mapIndexed { index, telemetry -> SignalLogEntry.LocalStatsEntry(telemetry, index) } + - signalData.map { SignalLogEntry.PacketEntry(it) } - ) - .sortedByDescending { it.timeSeconds } - } + val data = remember(signalData, localStatsData) { buildSignalLog(signalData, localStatsData) } val hasNoiseFloor = remember(localStatsData) { localStatsData.any { it.local_stats?.noise_floor != 0 } } val hasRssi = remember(signalData) { signalData.any { it.rx_rssi != 0 } } val hasSnr = remember(signalData) { signalData.any { !it.rx_snr.isNaN() } } diff --git a/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/metrics/SignalLogKeyTest.kt b/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/metrics/SignalLogKeyTest.kt new file mode 100644 index 0000000000..e8e13682df --- /dev/null +++ b/feature/node/src/commonTest/kotlin/org/meshtastic/feature/node/metrics/SignalLogKeyTest.kt @@ -0,0 +1,76 @@ +/* + * Copyright (c) 2026 Meshtastic LLC + * + * This program is free software: you can redistribute it and/or modify + * it under the terms of the GNU General Public License as published by + * the Free Software Foundation, either version 3 of the License, or + * (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the + * GNU General Public License for more details. + * + * You should have received a copy of the GNU General Public License + * along with this program. If not, see . + */ +package org.meshtastic.feature.node.metrics + +import org.meshtastic.proto.MeshPacket +import org.meshtastic.proto.Telemetry +import kotlin.test.Test +import kotlin.test.assertEquals + +/** + * Duplicate LazyColumn keys are a fatal `IllegalArgumentException`, and `MeshPacket.id` is unique only per originating + * node, with retransmissions stored per reception. + */ +class SignalLogKeyTest { + + @Test + fun `repeated packet ids still produce distinct keys`() { + val packets = listOf(packet(id = 163275746), packet(id = 163275746), packet(id = 163275746)) + + val keys = buildSignalLog(signalData = packets, localStatsData = emptyList()).map { it.key } + + assertEquals(3, keys.toSet().size, "duplicate LazyColumn keys crash the screen: $keys") + } + + @Test + fun `a negative packet id is not conflated with its positive twin`() { + // Packet ids are signed in the proto, so the sign has to survive into the key. Both packets sit at index 0 so + // the index cannot be what separates them. + val negative = buildSignalLog(signalData = listOf(packet(id = -1344023121)), localStatsData = emptyList()) + val positive = buildSignalLog(signalData = listOf(packet(id = 1344023121)), localStatsData = emptyList()) + + assertEquals("packet_-1344023121_0", negative.single().key) + assertEquals("packet_1344023121_0", positive.single().key) + } + + @Test + fun `local stats and packets sharing a timestamp do not collide`() { + val keys = + buildSignalLog( + signalData = listOf(packet(id = 7, rxTime = 100), packet(id = 7, rxTime = 100)), + localStatsData = listOf(stats(time = 100), stats(time = 100)), + ) + .map { it.key } + + assertEquals(4, keys.toSet().size, "same-second entries across both types must stay distinct: $keys") + } + + @Test + fun `entries are ordered newest first across both types`() { + val log = + buildSignalLog( + signalData = listOf(packet(id = 1, rxTime = 10), packet(id = 2, rxTime = 30)), + localStatsData = listOf(stats(time = 20)), + ) + + assertEquals(listOf(30, 20, 10), log.map { it.timeSeconds }) + } + + private fun packet(id: Int, rxTime: Int = 0) = MeshPacket(id = id, rx_time = rxTime) + + private fun stats(time: Int) = Telemetry(time = time) +}