fix(nodes): refresh distance when display units change#6351
Conversation
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesDistance unit refresh
Estimated code review effort: 2 (Simple) | ~10 minutes 🚥 Pre-merge checks | ✅ 4✅ 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: 1
🧹 Nitpick comments (1)
core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/NodeItemDistanceUnitsTest.kt (1)
71-76: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the updated accessibility description too.
These assertions only cover rendered text; add semantic content-description checks for metric removal and imperial addition so the accessibility fix cannot regress.
🤖 Prompt for 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. In `@core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/NodeItemDistanceUnitsTest.kt` around lines 71 - 76, Add semantic content-description assertions in the NodeItemDistanceUnitsTest flow after switching distanceUnits to imperial: verify the metric content description is absent and the imperial content description is present, alongside the existing rendered-text assertions. Use the existing metricDistance and imperialDistance values or their corresponding semantic expectations.
🤖 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
`@core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/NodeItemDistanceUnitsTest.kt`:
- Line 75: Import androidx.compose.ui.test.assertDoesNotExist in
NodeItemDistanceUnitsTest so the existing
onNodeWithText(metricDistance).assertDoesNotExist() assertion resolves and the
test compiles.
---
Nitpick comments:
In
`@core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/NodeItemDistanceUnitsTest.kt`:
- Around line 71-76: Add semantic content-description assertions in the
NodeItemDistanceUnitsTest flow after switching distanceUnits to imperial: verify
the metric content description is absent and the imperial content description is
present, alongside the existing rendered-text assertions. Use the existing
metricDistance and imperialDistance values or their corresponding semantic
expectations.
🪄 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: 7779fee6-b18b-45f2-9bc9-2de92c6cc3cd
📒 Files selected for processing (3)
core/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/NodeItemDistanceUnitsTest.kt
|
Could a maintainer please add the ai-generated label? |
|
Valid finding fixed and pushed to [PR #6351](#6351). Added assertions confirming:
Validation passed: Spotless, Detekt, compilation, and both focused tests. Commit: |
Summary
Root cause
The formatted distance was remembered using only the two nodes as keys. When the node-list state changed from its initial metric value to the locale-derived imperial value, Compose retained the cached kilometer string until either node refreshed.
User impact
Distances on the Nodes tab now switch to miles immediately when imperial units become active, without requiring a node refresh.
Validation
./gradlew spotlessCheck detekt assembleDebug test --no-configuration-cache./gradlew :core:ui:allTests --no-configuration-cacheRepository-wide
allTestswas also attempted on Windows. The unrelated:core:datastore:allTeststask failed because 18 existingRecentAddressesDataSourceTestcases could not rename DataStore temporary files; the failure reproduces in that module alone. The changed:core:uimodule's completeallTestslifecycle passes.Summary by CodeRabbit