Repository navigation
feat(node): compact layout improvements - #7802
vpolevikoff wants to merge 4 commits into
Conversation
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Battery composables core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/MaterialBatteryInfo.kt |
MaterialBatteryInfo now delegates percentage and voltage rendering to shared helpers. Its level parameter precedes modifier. MaterialBatteryCompactInfo displays percentage and voltage in separate rows. The previews use the same parameter provider. |
Compact node layout core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItemCompact.kt |
NodeItemCompact now uses MaterialBatteryCompactInfo. Its chip-and-battery column is centered with updated padding and spacing, and CompactNameRow has less spacing between items. |
Priority:
Merge Risk: 
f34d0
-
No actionable merge-blocking issue is established for the compact battery layout.
Pre-merge checks: 
-
Check name Status Explanation Resolution Regression Coverage For Changed Behavior Coverage gaps remain for all three changed surfaces. 1. Impacted code path: MaterialBatteryCompactInfo,Percents, andVoltsin `core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/Mat…Add the compact battery Compose state and layout matrix for unknown, negative, external-power, zero/absent-voltage, custom-label, and separate-row behavior. Add a constrained NodeItemCompactgeometry and accessibility test covering all-fi…-
Check name Status Explanation Description Check Check skipped - CodeRabbit’s high-level summary is enabled. Title check The title clearly describes the main changes to the compact node layout, including spacing, battery display, and element arrangement. Linked Issues check Check skipped because no linked issues were found for this pull request. Out of Scope Changes check Check skipped because no linked issues were found for this pull request. Sibling Call Sites And Presence Semantics PASS. The pull request changes battery rendering and layout, but it does not change absence semantics for telemetry fields. All MaterialBatteryInfocallers use named arguments, and the new compact h…Tests Prove The Path, Not The End State No test code was added or changed. The PR changes only production Kotlin files and 24 screenshot reference PNGs; the existing @PreviewTestdeclarations remain unchanged. The changed references do no…Moved Code Diffed Against Its Original The extracted percentage and voltage rendering preserve the old branches, defaults, nullability, validation behavior, annotations, and voltage condition. MaterialBatteryInforemains public, and all …Full details: Sibling Call Sites And Presence Semantics
-
Explanation
PASS. The pull request changes battery rendering and layout, but it does not change absence semantics for telemetry fields. All
MaterialBatteryInfocallers use named arguments, and the new compact helper receives the same existingthatNode.voltage ?: 0fexpression; no new zero-default field was added. Both ambient-temperature UI siblings use nullable presence checks:NodeItem.kt:366andNodeItemCompact.kt:472. Regression tests cover zero and absent temperature for both siblings.
Full details: Tests Prove The Path, Not The End State
-
Explanation
No test code was added or changed. The PR changes only production Kotlin files and 24 screenshot reference PNGs; the existing
@PreviewTestdeclarations remain unchanged. The changed references do not seed fake storage, assert only collection size, or testDispatchers.Unconfinedemission order.
Full details: Moved Code Diffed Against Its Original
-
Explanation
The extracted percentage and voltage rendering preserve the old branches, defaults, nullability, validation behavior, annotations, and voltage condition.
MaterialBatteryInforemains public, and all repository call sites use named arguments with the new parameter order. No caller retains an old behavioral assumption. The new compact composable adds the intended two-row layout; no moved-code contract change matches the failure conditions.
-
Full details: Regression Coverage For Changed Behavior
-
Explanation
Coverage gaps remain for all three changed surfaces. 1. Impacted code path:
MaterialBatteryCompactInfo,Percents, andVoltsincore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/MaterialBatteryInfo.kt:58-190. Regression risk: compact rendering can regress for null or negative levels, external-power levels, zero or absent voltage, a customunknownLabel, or loss of the separate vertical rows. The only executable battery test checks the oldMaterialBatteryInfounknown-label case (LoraSignalIndicatorUiTest.kt:60-65), and the PR adds no Kotlin test. Missing test shape: a Compose state matrix for those inputs that asserts text and content descriptions, omission of non-positive voltage, and separate row bounds. 2. Impacted code path: the leading chip/battery column andCompactNameRowinNodeItemCompact.kt:180-220, 259-290. Regression risk: constrained widths can reintroduce node-name truncation or empty space, and minimal or power-disabled nodes can receive incorrect spacing. Existing compact tests cover unrelated state behavior, while the PR changes only screenshot reference PNGs. Missing test shape: a constrained-width Compose test for all-fields, power-disabled, and minimal nodes that checks name, chip, and battery bounds, row separation, and the merged card accessibility touch-target bounds. Updated screenshot goldens do not prove these properties. 3. Impacted code path: publicMaterialBatteryInfochanged from(modifier, level, ...)to(level, modifier, ...)in the same file. Regression risk: downstream positional consumers of the old signature can fail to compile, and no repository API compatibility fixture or deprecated forwarding overload exists. Current repository call sites use named arguments, so they do not cover this consumer case. Missing test shape: an API compatibility or consumer compile fixture for the old positional invocation, or an explicit breaking-API decision with a compatibility-preserving overload if positional compatibility is required.Resolution
Add the compact battery Compose state and layout matrix for unknown, negative, external-power, zero/absent-voltage, custom-label, and separate-row behavior. Add a constrained
NodeItemCompactgeometry and accessibility test covering all-fields, power-disabled, and minimal nodes. Add an API compatibility or consumer compile check for the oldMaterialBatteryInfoinvocation; if positional compatibility is required, restore it with a deprecated forwarding overload.
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
-
-
- Source mode: Auto
- Repository: meshtastic/Meshtastic-Android
- Branch: main
- File: .coderabbit.yaml
- Commit: aa6625e
- Layers: Repository YAML
- Review profile: CHILL
- Plan: Advanced
- Run ID:
b433ad9d-faee-45ae-b1cb-e719f4be581b
-
screenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/core/AlertScreenshotTestsKt/ScreenshotShareDialogWithNfc_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/core/AlertScreenshotTestsKt/ScreenshotShareDialogWithNfc_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/core/AlertScreenshotTestsKt/ScreenshotShareDialog_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/core/AlertScreenshotTestsKt/ScreenshotShareDialog_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/core/ComponentScreenshotTestsKt/ScreenshotListItem_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/core/ComponentScreenshotTestsKt/ScreenshotSwitchListItem_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/core/RecoveryCardScreenshotTestsKt/ScreenshotRecoveryCard_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/MapScreenshotTestsKt/ScreenshotWaypointInfoDialogOptedIn_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/MapScreenshotTestsKt/ScreenshotWaypointInfoDialogOptedIn_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/MapScreenshotTestsKt/ScreenshotWaypointInfoDialogReadOnly_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/MapScreenshotTestsKt/ScreenshotWaypointInfoDialogReadOnly_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailContentLocal_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailContentLocal_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailContentMinimal_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailContentMinimal_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailContentRemote_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailContentRemote_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailsSectionMakerHero_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailsSectionMakerHero_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailsSectionSigned_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailsSectionSigned_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailsSection_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeDetailsSection_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemCompactActive_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemCompactActive_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemCompactAllFields_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemCompactAllFields_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemCompactMinimal_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemCompactMinimal_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemCompactOnlineRemote_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemCompactOnlineRemote_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemCompactUnheard_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemCompactUnheard_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemCompactUnknown_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemCompactUnknown_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemCompleteActive_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemCompleteActive_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemComplete_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemComplete_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemSigned_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemSigned_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemUnknown_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/NodeScreenshotTestsKt/ScreenshotNodeItemUnknown_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotNodeLayoutSettingsCompactMinimal_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotNodeLayoutSettingsCompactMinimal_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotNodeLayoutSettingsCompact_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotNodeLayoutSettingsCompact_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotSampleNodeCompactAllFields_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotSampleNodeCompactAllFields_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotSampleNodeCompactNameOnly_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotSampleNodeCompactNameOnly_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotSampleNodeCompactSignalOnly_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotSampleNodeCompactSignalOnly_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotSampleNodeCompactToggleMatrix_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/SettingsScreenshotTestsKt/ScreenshotSampleNodeCompactToggleMatrix_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/WifiProvisionScreenshotTestsKt/ScreenshotMpwrdDisclaimerBanner_Dark_d19fbf1f_0.pngis excluded by!**/*.png,!**/*.pngscreenshot-tests/src/screenshotTestDebug/reference/org/meshtastic/screenshots/feature/WifiProvisionScreenshotTestsKt/ScreenshotMpwrdDisclaimerBanner_Light_b29dc7a7_0.pngis excluded by!**/*.png,!**/*.png
-
core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/MaterialBatteryInfo.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/NodeItemCompact.kt
Included review availability: This review used your included allowance. Your plan provides up to 4 included reviews per hour; 3 remain after this review.
-
jamesarich
left a comment
jamesarich
left a comment
There was a problem hiding this comment.
screenshot-check is red: the sample node's lastHeard is 2023-11-14 22:13 UTC and the committed goldens read November 15, so they were recorded east of UTC. CI renders the 14th and that one glyph is over the threshold. Re-record with TZ=UTC ./gradlew :screenshot-tests:updateDebugScreenshotTest and drop the 13 goldens outside the node items (ShareDialog, WaypointInfoDialog, MpwrdDisclaimerBanner, ListItem, SwitchListItem, RecoveryCard) - those are sub-threshold host drift.
| ) | ||
| .fillMaxWidth() | ||
| .padding(horizontal = 12.dp, vertical = 8.dp), | ||
| .padding(horizontal = 12.dp, vertical = 0.dp), |
There was a problem hiding this comment.
The content column has no vertical inset now - only the chip column got the 10dp, so once the right side is taller than the chip the footer sits on the card's bottom edge (All fields and No metrics in your after shot). Put vertical = 10.dp back on the Row and drop the Column padding; the chip-only row still lands on 48dp.
There was a problem hiding this comment.
Yep, I see.
The padding was reduced because the IconButton inside CompactNameRow enforces a 48dp height, so if I restore 10dp padding - the top text-to-card gap increases again, so does the overall card height:
But now the footer really sits on the edge, this needs to be fixed
I would suggest some options:
1 - restore this padding, but not too much, 4dp for example. this still keeps the card more compact
2 - asymmetric padding, something like .padding(start = 12.dp, top = 2.dp, end = 12.dp, bottom = 8.dp), which gives this and probably looks better:
Regarding the padding on the chip column — I added it purely to align the chip to the node name. Since the IconButton forces the row height to be 48dp, without that padding the chip sticks to the top edge while the text sits centered. Just some cosmetics padding and it looks better (imho). First screenshot is with that padding dropped, two others - with padding present.
There was a problem hiding this comment.
Makes sense, the IconButton's 48dp is doing the top inset already. Go with option 2, top 2 bottom 8, and keep the chip padding.
| horizontalAlignment = Alignment.CenterHorizontally, | ||
| verticalArrangement = Arrangement.spacedBy(1.dp), | ||
| ) { | ||
| Row { |
There was a problem hiding this comment.
These wrapper Rows lose the CenterVertically and 1dp spacing the full variant has - fine at font scale 1, the icon pins to the top of the label once text scales. Give Percents its own Row and drop the MultipleEmitters suppress on both; Volts emits one Text.
|
@vpolevikoff thanks for giving it another shot - i'm having better feelings about this one after the few tweaks mentioned above. |
|
Code looks right, just spotless left - |
After the recent UI updates (likely related to the new security icons), the compact node list got visually broken. The node names were pushed to the right, creating huge empty gaps. It made the "compact" mode look messy and actually less compact.
🐛 Fixes:
🧪 Testing Performed:
Checked the layout using the existing SampleNodeCompactToggleMatrixPreview (Before/After screenshots attached below).
Checked the layout on device, looks nice and fixed as well
Ran the standard project checks (./gradlew spotlessApply and ./gradlew detekt) to make sure the code is clean.
Summary by CodeRabbit