Skip to content

feat(node): label SNR quality on the Node Details signal row - #6703

Open
beecho01 wants to merge 2 commits into
meshtastic:mainfrom
beecho01:enhancement/node-details-snr-quality-label
Open

feat(node): label SNR quality on the Node Details signal row#6703
beecho01 wants to merge 2 commits into
meshtastic:mainfrom
beecho01:enhancement/node-details-snr-quality-label

Conversation

@beecho01

@beecho01 beecho01 commented Aug 14, 2026

Copy link
Copy Markdown

Why

The node list already labels signal quality (e.g. "-17.0 dB · Good") via SignalInfo.kt's pill, using determineSignalQuality/Quality from LoraSignalIndicator.kt (#5903). The Node Details screen's SignalRow was missed - it only showed the raw SNR/RSSI numbers, with no interpretation.

What changed

  • SignalRow (feature/node/.../NodeDetailsSection.kt) now labels SNR with its quality word, reusing the existing, already-tested determineSignalQuality - no new thresholds introduced.
  • Format is "<raw value> · <quality>" (e.g. "-17.0 dB · Good"), matching the value-then-quality order already used by the node-list pill in SignalInfo.kt, rather than inventing a new convention.
  • InfoItem (NodeDetailComponents.kt) gained one optional valueColor parameter (default unchanged) so the SNR value can be tinted by quality, same as elsewhere in the app.
  • RSSI is deliberately left unlabeled. The app's existing signal-quality logic is SNR-only (LoraSignalIndicatorTest's RSSI does not influence the rating), because RSSI alone can't indicate quality without the noise floor - adding a quality word to RSSI here would contradict that existing, tested design decision.
  • The raw value is never removed, only the quality word is added alongside it.

Test plan

  • New test: feature/node/src/jvmTest/.../SignalRowQualityLabelTest.kt - 6 desktop Compose UI tests (see the comment in feature/node/build.gradle.kts for why Compose UI tests live in jvmTest) covering all 4 quality bands (Good/Fair/Bad/None) at the LONG_FAST preset boundaries, confirms RSSI shows only the raw value, and confirms the SNR row is absent (not mislabeled) when a node has no SNR reading.
  • LoraSignalIndicatorTest (the underlying quality logic) is unchanged and still passing - this PR only adds a new call site.
  • Screenshot goldens regenerated and validated for the 4 affected previews (light/dark): ScreenshotNodeDetailsSection, ScreenshotNodeDetailsSectionSigned, ScreenshotNodeDetailContentLocal, ScreenshotNodeDetailContentRemote.
  • Local baseline green: spotlessApply spotlessCheck detekt assembleDebug test allTests and kmpSmokeCompile.
  • Hardware smoke test: installed the fdroid debug build on a physical device (Samsung Galaxy S24+) connected to a real Meshtastic radio, with a directly-heard node in range. Confirmed the Node Details SNR label matches the existing node-list pill for the same node, same quality word and color.

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

SignalRow only showed the raw SNR/RSSI values, unlike the node-list
signal pill which already labels quality via determineSignalQuality.
Reuses that existing, tested logic to show e.g. "-17.0 dB - Good" for
SNR, matching the value-then-quality format already used by
SignalInfo.kt. RSSI is left unlabeled: the app's existing signal-
quality logic is SNR-only, since RSSI alone can't indicate quality
without the noise floor.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@CLAassistant

CLAassistant commented Aug 14, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@coderabbitai

coderabbitai Bot commented Aug 14, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: b9a8d234-d02e-4a28-bfbb-f5bc97bbf142

📥 Commits

Reviewing files that changed from the base of the PR and between 6cb1bf6 and 49adc3e.

⛔ Files ignored due to path filters (8)
  • screenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailContentLocal_Dark_d19fbf1f_0.png is excluded by !**/*.png, !**/*.png
  • screenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailContentLocal_Light_b29dc7a7_0.png is excluded by !**/*.png, !**/*.png
  • screenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailContentRemote_Dark_d19fbf1f_0.png is excluded by !**/*.png, !**/*.png
  • screenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailContentRemote_Light_b29dc7a7_0.png is excluded by !**/*.png, !**/*.png
  • screenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailsSectionSigned_Dark_d19fbf1f_0.png is excluded by !**/*.png, !**/*.png
  • screenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailsSectionSigned_Light_b29dc7a7_0.png is excluded by !**/*.png, !**/*.png
  • screenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailsSection_Dark_d19fbf1f_0.png is excluded by !**/*.png, !**/*.png
  • screenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailsSection_Light_b29dc7a7_0.png is excluded by !**/*.png, !**/*.png
📒 Files selected for processing (3)
  • feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/component/NodeDetailComponents.kt
  • feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/component/NodeDetailsSection.kt
  • feature/node/src/jvmTest/kotlin/org/meshtastic/feature/node/component/SignalRowQualityLabelTest.kt

📝 Walkthrough

Walkthrough

Node details now display modem-aware SNR quality labels with matching colors. RSSI remains numeric-only. Compose UI tests cover SNR thresholds, unset readings, and RSSI rendering.

Changes

Node signal quality

Layer / File(s) Summary
Configurable value color
feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/component/NodeDetailComponents.kt
InfoItem accepts an optional valueColor and applies it to the value text.
SNR quality display and validation
feature/node/src/commonMain/kotlin/org/meshtastic/feature/node/component/NodeDetailsSection.kt, feature/node/src/jvmTest/kotlin/org/meshtastic/feature/node/component/SignalRowQualityLabelTest.kt
SignalRow displays modem-aware SNR quality labels with matching colors. RSSI remains numeric-only. Tests cover quality thresholds and unset SNR readings.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: 🟡 Moderate · up to 49adc

The Node Details SNR label currently uses a middle-dot separator instead of the required value-then-quality format with " - ", so the UI does not match the stated product convention and its test expectation. This is localized and straightforward to fix before merge.

Suggested reviewers: jamesarich

🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Sibling Call Sites And Presence Semantics ✅ Passed The diff only adds SNR quality rendering and an optional color; it does not change field nullability, guards, sentinels, or introduce a zero-default field.
Tests Prove The Path, Not The End State ✅ Passed The four new SNR-label tests assert exact composed text absent from parent SignalRow; tests use real composition and no fake store, size-only, or Unconfined-order assertions.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: adding an SNR quality label to the Node Details signal row.

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@github-actions github-actions Bot added the enhancement New feature or request label Aug 14, 2026
@beecho01
beecho01 marked this pull request as ready for review August 14, 2026 19:44
@jamesarich

Copy link
Copy Markdown
Collaborator

Thanks for this, it's a really nice first PR. Reusing determineSignalQuality instead of introducing new thresholds is exactly the right call, and I appreciate that you left RSSI unlabeled and wrote down why. The hardware smoke test is above and beyond.

Three small things and then I'm happy to get this in:

  1. VerificationRow's KDoc picked up a "tappable" to "tap-able" change that looks incidental to the SNR work. Mind reverting it? "tappable" is what we use elsewhere (AutoLinkText.kt, NodeSignedStatusIcon.kt, FdroidMapOverlayRenderer.kt), so it keeps us consistent.

  2. Description vs. code on the format string: the PR body says "-17.0 dB - Good" but the code emits " · ". The middot is fine, could you just update the description to match? It ends up in the commit history, so I like keeping those honest.

  3. Minor labelling one: the test plan calls these "Robolectric Compose tests", but they're actually desktop Compose UI tests in jvmTest. There's a comment in feature/node/build.gradle.kts explaining that Compose UI tests live in jvmTest specifically because the Robolectric idling strategy NPEs on the null Build.FINGERPRINT the androidHostTest stubs leave behind. The tests themselves are good, just the label.

No rush on any of it. Thanks again for the test coverage, especially the "SNR row is absent rather than mislabeled" case.

VerificationRow's KDoc picked up "tappable" -> "tap-able" unrelated to
the SNR quality label work; revert to keep it consistent with
AutoLinkText.kt, NodeSignedStatusIcon.kt, and FdroidMapOverlayRenderer.kt.

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@beecho01

Copy link
Copy Markdown
Author

Hi @jamesarich,

These should all be complete now:

  1. This was an error on my part. VSCode was complaining about the spelling, and I didn't check the correct terminology before making the change.
  2. Updated the description to use · so it matches what the code actually uses.
  3. Updated the test plan to just describe them as desktop Compose UI tests living in jvmTest, rather than calling them Robolectric.

Thanks for the thorough review, appreciate you catching all three. Let me know if you need or recommend anything more.

@jamesarich jamesarich left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice add, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants