fix(ui): stop discarding measured-zero sensor and RSSI readings - #6507
Conversation
The protobuf models are Wire-generated, so presence IS nullability. A
`(x ?: 0f) != 0f` guard collapses "absent" and "measured zero" into one
state and throws away a real reading. Ten live sites did this.
Replace the zero-guards with `?.let { }` presence checks, and keep RSSI
nullable end-to-end so an unknown signal no longer renders as 0 dBm --
the strongest value on that scale.
Humidity keeps its zero-guard: 0 %RH is not physically reachable.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
📝 WalkthroughWalkthroughThe PR changes BLE RSSI and environment metric handling to preserve nullable absence separately from valid zero values. It updates APIs, UI state, telemetry and power rendering, tests, Compose test wiring, and adds ast-grep rules for zero-sentinel patterns. ChangesNullable RSSI propagation
Presence-based metric rendering
Zero-sentinel guardrails
Estimated code review effort: 3 (Moderate) | ~30 minutes Sequence Diagram(s)sequenceDiagram
participant BLEDevice
participant CurrentlyConnectedInfo
participant DeviceListItem
BLEDevice->>CurrentlyConnectedInfo: readRssi()
CurrentlyConnectedInfo->>CurrentlyConnectedInfo: store nullable RSSI
BLEDevice->>DeviceListItem: provide nullable RSSI
DeviceListItem->>DeviceListItem: refresh displayed RSSI by device address
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.coderabbit/ast-grep-rules/no-float-metric-zero-sentinel.yml:
- Around line 14-20: Extend the rule patterns under rule.any in
no-float-metric-zero-sentinel.yml to recognize equivalent zero-float literal
spellings, including 0F and 0.0f, for both equality and inequality checks;
alternatively, add invalid fixtures covering each spelling so formatting changes
cannot bypass the rule.
In @.coderabbit/ast-grep-rules/no-rssi-zero-default.yml:
- Around line 9-15: Update the rule anchored by the `X` RSSI constraint to cover
nullable zero checks such as `reaction.rssi == null || reaction.rssi == 0`, or
explicitly narrow and document the legacy exception if it must remain allowed.
Ensure the guard’s contract consistently detects the intended RSSI-zero
anti-pattern beyond the existing `?: 0` patterns.
In
`@core/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.kt`:
- Line 509: Update the connection-confirmation log in BleRadioTransport to
replace the raw address interpolation with address.anonymize(), matching the
surrounding transport logs while preserving the existing RSSI message.
In `@core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItem.kt`:
- Around line 397-399: Update the soilMoisture handling in NodeItem to discard
values outside 0..100 using takeIf, then format valid readings with the shared
MetricFormatter.percent instead of interpolating a percent sign. Add a UI
regression test covering an out-of-range soil-moisture value and confirming it
is not displayed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ecb9af64-6463-4abc-a192-2855d15b4a2e
📒 Files selected for processing (20)
.coderabbit/ast-grep-rules/no-float-metric-zero-sentinel.yml.coderabbit/ast-grep-rules/no-rssi-zero-default.ymlcore/ble/src/commonMain/kotlin/org/meshtastic/core/ble/BleDevice.ktcore/ble/src/commonMain/kotlin/org/meshtastic/core/ble/MeshtasticBleDevice.ktcore/ble/src/commonTest/kotlin/org/meshtastic/core/ble/MeshtasticBleDeviceRssiTest.ktcore/model/src/commonMain/kotlin/org/meshtastic/core/model/Node.ktcore/model/src/commonTest/kotlin/org/meshtastic/core/model/NodeTelemetryStringsTest.ktcore/network/src/commonMain/kotlin/org/meshtastic/core/network/radio/BleRadioTransport.ktcore/testing/src/commonMain/kotlin/org/meshtastic/core/testing/FakeBle.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItem.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItemCompact.ktcore/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/NodeItemZeroMetricsTest.ktfeature/connections/build.gradle.ktsfeature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/component/ConnectionsPreviews.ktfeature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/components/CurrentlyConnectedInfo.ktfeature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/components/DeviceListItem.ktfeature/connections/src/jvmTest/kotlin/org/meshtastic/feature/connections/ui/components/DeviceListItemRssiTest.ktfeature/node/build.gradle.ktsfeature/node/src/commonMain/kotlin/org/meshtastic/feature/node/component/PowerMetrics.ktfeature/node/src/jvmTest/kotlin/org/meshtastic/feature/node/component/PowerMetricsZeroVoltageTest.kt
…dress - NodeItem soil moisture now range-checks 0..100 and formats via MetricFormatter, matching Node.getTelemetryStrings. Previously a sensor fault reporting 101% rendered in the UI but not in the telemetry strings. - Anonymize the address in the BleRadioTransport connection-confirmed log. - ast-grep float rule now covers the 0F / 0.0f / 0.0F literal spellings so a reformat cannot bypass it. - Document why the RSSI rule stops at `?: 0` and deliberately does not match `== 0` against stored rows (Reaction.kt reads pre-schema-51 data where 0 really is indistinguishable from "no reading"). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
Batched all four review findings into one push (c6d20c1) rather than fix-at-a-time. Baseline re-run green: @coderabbitai review |
|
✅ Action performedReview finished.
|
A node reporting exactly 0 °C shows no temperature at all. A power channel measuring 0.00 V vanishes from the metrics card. A BLE device we have never heard from renders as having a perfect signal.
All three are the same defect:
0treated as "not reported" on a scale where0is a real value. The enabling fact is that the protobuf models are Wire-generated, so presence IS nullability —EnvironmentMetrics.temperatureand friends are declaredFloat? = nulland there is nohasX()accessor. A(x ?: 0f) != 0fguard collapses "absent" and "measured zero" into one state and silently throws away a real reading, even though the data layer already round-trips it correctly.NodeItem.ktwas fixed for ambient temperature previously. This applies the same treatment to the ten sites that were missed.🐛 Fixes
Environment metrics —
?.let { }instead of a zero-guardNodeItemCompacttemperature. This is the user-visible bug: the fix landed inNodeItemand its compact sibling was missed, so a node at 0 °C showed nothing in the compact row.NodeItemsoil temperature, soil moisture, voltage, current.Node.getDisplayStringstemperature, soil temperature, soil moisture, voltage, current.PowerMetricschannel voltage — a 10th site the audit list did not name, and the same "sibling was missed" shape: thecurrenthandling two lines below had already been given the presence treatment, but thevoltagefilter above it still dropped any channel reading exactly 0.00 V.Soil moisture also no longer requires a soil-temperature reading to render. It now stands on its own presence plus its
0..100range check, and is formatted viaMetricFormatter.percentso the UI andNode.getTelemetryStringsagree — previously a sensor fault reporting101%rendered in one path but not the other.RSSI — nullable end-to-end
0 dBm is the strongest value on a signal-strength scale, so defaulting a missing reading to 0 renders an unknown signal as an excellent one.
BleDevice.readRssi()now returnsInt?, which propagates throughMeshtasticBleDevice,DeviceListItem, andCurrentlyConnectedInfo.MetricFormatter.rssi(null)already renders—.CurrentlyConnectedInfoseeded its own state to0, so it displayed "0 dBm" until the first read landed.🛠️ Also
DeviceListItemandCurrentlyConnectedInfonow key their remembered RSSI state by device address. A recycled list slot previously kept the previous device's reading for up to the 2 s poll interval, andLaunchedEffect(Unit)never restarted on rebind.FakeBleDevice.readRssi()returns its nullablerssiinstead of manufacturing-60fromnull. No test depended on the fallback.BleRadioTransportconnection-confirmed log is now anonymized, matching the other logs in that file.🧹 Regression guard
Adds
.coderabbit/ast-grep-rules/with the two rules that found these. Both fire on the pre-fix code and report zero matches acrosscore/ feature/ androidApp/ desktopApp/now.Deliberately unchanged
relative_humidity/co2_humidityzero-guards are intentional and tested —EnvironmentMetricsForGraphingTest.humidity_zeroFilteredOutasserts it. Same reasoning forbarometric_pressure(0 hPa is a vacuum).rx_snr. StillFloat = 0fupstream with no proto presence, so its guards genuinely cannot be fixed app-side.packet.rx_rssi. App-level nullability only here; the proto side landed separately in fix(model): rssi explicit presence for protobufs 2.7.26.138 #6498.Reaction.kt:281'srssi == 0. Documented pre-schema-51 migration handling — those rows stored0where the column is now nullable, so there a0genuinely is indistinguishable from "no reading". The RSSI ast-grep rule deliberately stops at the?: 0defaulting form so it does not flag this; the boundary is spelled out in the rule header.Testing Performed
./gradlew spotlessApply spotlessCheck detekt assembleDebug test allTests— green.22 new tests across four files. Every changed metric is pinned twice, absent and measured-zero, because either assertion alone lets the two states collapse back into one.
NodeTelemetryStringsTest(7)Node.getTelemetryStrings— zero temp / voltage / current / soil, out-of-range moisture still rejected, humidity zero still filteredNodeItemZeroMetricsTest(10)NodeItem+NodeItemCompactrender zero readings, hide absent ones, reject out-of-range moisturePowerMetricsZeroVoltageTest(3)MeshtasticBleDeviceRssiTest(2)nullDeviceListItemRssiTest(3)All confirmed executed via the test-results XML rather than assumed from a green tick.
One structural note: the two Compose UI tests live in
jvmTest, notcommonTest. Both modules enablewithHostTest, and underandroidHostTestthe Compose Robolectric idling strategy NPEs on a nullBuild.FINGERPRINT— acommonTestclass cannot carry@RunWith(RobolectricTestRunner::class). This matchescore/ui, which has host tests disabled and runs its UI tests JVM-only. The reason is commented in both build files.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
0 dBmsignal and an unavailable reading.Bug Fixes
Tests