fix(android): avoid blocking resource loads in connection status#6398
fix(android): avoid blocking resource loads in connection status#6398jeremiah-k wants to merge 6 commits into
Conversation
📝 WalkthroughWalkthroughThe change introduces asynchronous, generation-safe service notification rendering with coroutine-scoped tests. It also allows connected-device UI labels to be resolved by the parent screen and passed into reusable RSSI, battery, and connection components. ChangesService notification rendering
Connected-device UI labels
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ServiceState
participant MeshNotificationManagerImpl
participant ServiceScope
participant AndroidNotificationManager
ServiceState->>MeshNotificationManagerImpl: updateServiceStateNotification()
MeshNotificationManagerImpl->>ServiceScope: launch lazy render job
ServiceScope->>MeshNotificationManagerImpl: render captured snapshot
MeshNotificationManagerImpl->>AndroidNotificationManager: commit latest notification
Possibly related PRs
🚥 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 |
![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
🧹 Nitpick comments (1)
core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplConversationTest.kt (1)
75-75: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a deterministic test dispatcher here.
updateServiceStateNotification()runs async render work throughgetStringSuspend(...), but these tests assert immediately after calling it underDispatchers.Unconfined; if resource resolution resumes off the current thread, the assertions become order-dependent. Move this test toStandardTestDispatcher + advanceUntilIdle()or otherwise advance the renderer before asserting.🤖 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/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplConversationTest.kt` at line 75, Update the notificationScope in MeshNotificationManagerImplConversationTest to use a deterministic StandardTestDispatcher-backed test scope instead of Dispatchers.Unconfined, and advance the dispatcher with advanceUntilIdle() after updateServiceStateNotification() before assertions so asynchronous getStringSuspend rendering completes deterministically.
🤖 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
`@feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/components/CurrentlyConnectedInfo.kt`:
- Around line 139-145: Update the CurrentlyConnectedText preview configuration
to replace the hardcoded “Unknown”, “RSSI”, and “Disconnect” values with
localized core:resources entries resolved through stringResource(Res.string.*),
while leaving the remaining preview setup unchanged.
---
Nitpick comments:
In
`@core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplConversationTest.kt`:
- Line 75: Update the notificationScope in
MeshNotificationManagerImplConversationTest to use a deterministic
StandardTestDispatcher-backed test scope instead of Dispatchers.Unconfined, and
advance the dispatcher with advanceUntilIdle() after
updateServiceStateNotification() before assertions so asynchronous
getStringSuspend rendering completes deterministically.
🪄 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: 8a2cf68b-ca27-4f0a-94c3-84016e17945a
📒 Files selected for processing (8)
core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplConversationTest.ktcore/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplTest.ktcore/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/LoraSignalIndicator.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/MaterialBatteryInfo.ktcore/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/LoraSignalIndicatorUiTest.ktfeature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/ConnectionsScreen.ktfeature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/components/CurrentlyConnectedInfo.kt
…ryInfo Add optional label parameters to Rssi and MaterialBatteryInfo composables, enabling callers to supply pre-resolved display text without changing existing invocations. This decouples animated composition slots from the Compose Multiplatform resource lookup that can block the main thread.
Move Rssi, battery, firmware-version, and disconnect-text resolution out of the animated connected-device subtree into stable ConnectionsScreen composition slots. Introduces CurrentlyConnectedText as an immutable value object to pass pre-computed display strings through the animation scope.
Move service-state title and telemetry formatting onto ServiceScope with getStringSuspend so notification rendering never blocks callers on Compose resource loading. Capture immutable render snapshots, install lazy jobs before starting them, cancel superseded work, and guard cache publication by generation. Preserve the latest message when telemetry is absent and fall back to the Android application label for immediate foreground startup. Keep render state and job bookkeeping under the service notification lock, but perform NotificationManager Binder calls only after releasing it so foreground notification reads cannot be delayed by IPC. Use valid Wire oneof telemetry fixtures and a deterministic node-repository fallback, and centralize render-scope cleanup across the Android host regressions.
Resolve "Unknown", "RSSI", and "Disconnect" through stringResource() in the CurrentlyConnectedInfo preview so it reflects the production UI and follows the user-facing-strings convention.
Replace the class-level Dispatchers.Unconfined notificationScope with a per-test StandardTestDispatcher-backed scope managed by runWithRenderScope, matching the sibling MeshNotificationManagerImplTest pattern. advanceUntilIdle() after updateMessageNotification / updateServiceStateNotification ensures getStringSuspend render jobs complete before the assertions inspect active notifications, removing the order-dependence that let the test pass under Unconfined but fail when resource resolution resumed off the current thread.
a3544a4 to
ae22f8b
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplConversationTest.kt`:
- Around line 100-107: Move the duplicated runWithRenderScope helper from
MeshNotificationManagerImplConversationTest and MeshNotificationManagerImplTest
into the core:testing module, preserving its TestScope, CoroutineScope,
dispatcher, SupervisorJob, and cancellation behavior. Expose the shared utility
and import it from both tests, removing their local helper definitions.
🪄 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: 85355b3f-69d7-4e4e-9997-405967f2a33b
📒 Files selected for processing (8)
core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplConversationTest.ktcore/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplTest.ktcore/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/LoraSignalIndicator.ktcore/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/MaterialBatteryInfo.ktcore/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/LoraSignalIndicatorUiTest.ktfeature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/ConnectionsScreen.ktfeature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/components/CurrentlyConnectedInfo.kt
🚧 Files skipped from review as they are similar to previous changes (7)
- core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/LoraSignalIndicatorUiTest.kt
- core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/MaterialBatteryInfo.kt
- core/ui/src/commonMain/kotlin/org/meshtastic/core/ui/component/LoraSignalIndicator.kt
- feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/ConnectionsScreen.kt
- feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/components/CurrentlyConnectedInfo.kt
- core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplTest.kt
- core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt
| private fun runWithRenderScope(block: suspend TestScope.(CoroutineScope) -> Unit) = runTest { | ||
| val scope = CoroutineScope(StandardTestDispatcher(testScheduler) + SupervisorJob()) | ||
| try { | ||
| block(scope) | ||
| } finally { | ||
| scope.cancel() | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Centralize the duplicated render-scope helper.
runWithRenderScope is duplicated in MeshNotificationManagerImplTest.kt; move it into core:testing and import it from both tests. As per coding guidelines, “Place reusable test fakes, doubles, and utilities in core:testing.”
🤖 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/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplConversationTest.kt`
around lines 100 - 107, Move the duplicated runWithRenderScope helper from
MeshNotificationManagerImplConversationTest and MeshNotificationManagerImplTest
into the core:testing module, preserving its TestScope, CoroutineScope,
dispatcher, SupervisorJob, and cancellation behavior. Expose the shared utility
and import it from both tests, removing their local helper definitions.
Source: Coding guidelines
Guard notificationManager.notify() in the render-catch path with safeCatching so a failure while posting the degraded fallback cannot escape the render handler and cancel sibling render jobs running under the SupervisorJob. Preserve the primary rendered-notify path unchanged. Centralize the deterministic supervised render-scope fixture in core:testing so both notification suites share the same test scheduler, lifecycle, and guaranteed cancellation behavior.
ae22f8b to
3e939c2
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/testing/src/commonMain/kotlin/org/meshtastic/core/testing/TestScopes.kt`:
- Around line 27-32: Update runWithRenderScope so the renderScope job is
parented to the runTest coroutine context instead of creating an independent
SupervisorJob, or otherwise explicitly await its rendered work before the
finally block cancels it. Ensure render-scope coroutine failures remain
observable and runTest waits for all launched work.
🪄 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: 85be81a0-6d01-4ac4-bcc9-90954d664bbb
📒 Files selected for processing (4)
core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplConversationTest.ktcore/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplTest.ktcore/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.ktcore/testing/src/commonMain/kotlin/org/meshtastic/core/testing/TestScopes.kt
🚧 Files skipped from review as they are similar to previous changes (3)
- core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplTest.kt
- core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplConversationTest.kt
- core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt
| fun runWithRenderScope(block: suspend TestScope.(CoroutineScope) -> Unit) = runTest { | ||
| val renderScope = CoroutineScope(StandardTestDispatcher(testScheduler) + SupervisorJob()) | ||
| try { | ||
| block(renderScope) | ||
| } finally { | ||
| renderScope.cancel() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
rg -n -C3 'runWithRenderScope|SupervisorJob\(' .
rg -n -C2 'kotlinx-coroutines-test|coroutines' gradle/libs.versions.tomlRepository: meshtastic/Meshtastic-Android
Length of output: 50385
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== TestScopes.kt =="
sed -n '1,80p' core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/TestScopes.kt
echo
echo "== runWithRenderScope calls (first 80 matches) =="
rg -n 'runWithRenderScope|renderScope|launch|asStateFlow|StateFlow' core/service/src/androidHostTest/core/service/src/commonTest 2>/dev/null | head -n 120 || true
echo
echo "== core/testing build.gradle variants =="
for f in core/testing/build.gradle* core/testing/build.gradle.kts; do
[ -f "$f" ] && { echo "--- $f"; sed -n '1,160p' "$f"; }
done
echo
echo "== coroutines version aliases =="
sed -n '1,220p' gradle/libs.versions.toml | rg -n -C2 'coroutines|kotlinx|compose-multiplatform' || trueRepository: meshtastic/Meshtastic-Android
Length of output: 8169
🌐 Web query:
kotlinx.coroutines.test runTest cancels any coroutines launched in the scope Job parented by parent job
💡 Result:
In kotlinx.coroutines.test, the runTest function does not automatically cancel all coroutines simply because they are parented by a job passed to it. Understanding the behavior: 1. Test Completion Requirement: By design, runTest requires all coroutines launched within the test body to complete before it returns [1][2]. If active, uncompleted coroutines remain after the test body finishes, runTest will fail the test (typically by throwing an AssertionError) [1][3]. This behavior is intended to prevent resource leaks and ensure predictable test outcomes [4]. 2. Job Parentage: When you provide a parent Job to runTest, the test scope becomes a child of that job [5][6]. However, runTest does not implicitly cancel that parent job or its other children upon test completion [5]. It only tracks and waits for the completion of the coroutines launched within its own test scope [1][7]. 3. Handling Never-Ending Coroutines: If your test involves coroutines that are intentionally long-running or never-ending (e.g., background observers), you should use TestScope.backgroundScope [1][2]. Coroutines launched in backgroundScope are automatically cancelled when the test finishes [3][8]. This is the recommended approach for tasks that outlive the tested code, rather than relying on the parent job's lifecycle [7][4]. 4. Timeout and Failure: runTest includes a default timeout (60 seconds) [2][8]. If the test takes longer, it will cancel the test coroutine to prevent hanging [1][2]. If the test body itself fails, child coroutines are cancelled as part of standard structured concurrency behavior [5]. In summary, runTest enforces completion of launched coroutines but does not perform cleanup of external or parent-provided jobs [1][5]. You must manage the lifecycle of such jobs explicitly or utilize backgroundScope for background tasks [7][3].
Citations:
- 1: https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-test/kotlinx.coroutines.test/run-test.html
- 2: https://kotlinlang.org/api/kotlinx.coroutines/kotlinx-coroutines-test/
- 3: https://github.com/Kotlin/kotlinx.coroutines/blob/master/kotlinx-coroutines-test/common/src/TestScope.kt
- 4: What is the preferred way of testing when the code being tested launches a never-ending coroutine? Kotlin/kotlinx.coroutines#3283
- 5: https://android.googlesource.com/platform/external/kotlinx.coroutines/+/e20e479cc32b15993171d05a2f3f500cd8c04991/kotlinx-coroutines-test/common/test/RunTestTest.kt
- 6: https://android.googlesource.com/platform/external/kotlinx.coroutines/+/e20e479cc32b15993171d05a2f3f500cd8c04991/kotlinx-coroutines-test/common/src/TestScope.kt
- 7: https://developer.android.com/kotlin/coroutines/test
- 8: https://github.com/Kotlin/kotlinx.coroutines/blob/master/kotlinx-coroutines-test/README.md
Parent the render scope to runTest or make failures observable.
SupervisorJob() gives renderScope its own root job, so coroutines launched in it are not part of runTest’s wait-for-completion path, and render exceptions can be swallowed if callers don’t handle them. Parent the job to the test scope, or wait for/rendered work to complete before the finally { renderScope.cancel() }.
🤖 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/testing/src/commonMain/kotlin/org/meshtastic/core/testing/TestScopes.kt`
around lines 27 - 32, Update runWithRenderScope so the renderScope job is
parented to the runTest coroutine context instead of creating an independent
SupervisorJob, or otherwise explicitly await its rendered work before the
finally block cancels it. Ensure render-scope coroutine failures remain
observable and runTest waits for all launched work.
1 participant
Overview
This removes blocking Compose resource work from two connection-status paths that can overlap during normal Android lifecycle transitions.
The observed ANR had the Connections screen waiting in Compose resource loading while foreground-service telemetry formatting was resolving Compose resources at the same time. The UI lookup lived inside frequently re-entered animated content, while the service notification path used blocking non-composable resource helpers.
This change resolves connected-device display text outside the animated subtree and moves service-notification formatting onto the existing service coroutine scope.
Review Guide
The commits are intended to be reviewed in order:
Key Changes
Connection UI
RssiandMaterialBatteryInfoto accept caller-provided labels.CurrentlyConnectedTextvalue for localized connected-device text.AnimatedContent.Service Notifications
startForegroundfallback.serviceNotificationLock, then release the lock before posting throughNotificationManager.Testing
Added or updated coverage for:
Runtime validation repeatedly backgrounded, foregrounded, stopped, and restarted
MainActivitywhile service telemetry continued to update. The original input-timeout and Compose resource-loading stall did not recur, and the capture contains no notification-render exception, ANR, or process restart.Migration Notes
Summary by CodeRabbit
New Features
Bug Fixes
Tests