fix(android): avoid resource stalls in connection status#6398
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe change adds asynchronous service-notification rendering with coroutine-scoped tests and introduces caller-provided labels for connected-device UI 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: emit service snapshot
ServiceScope->>MeshNotificationManagerImpl: render latest snapshot
MeshNotificationManagerImpl->>AndroidNotificationManager: post rendered notification
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 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
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
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
3e939c2 to
a4eee02
Compare
|
The notification half of this is a genuine fix, and I want to be clear about that up front: Three things, none of which I consider merge blockers. 1. The concurrency machinery is heavier than the problem, and has a narrow race
The generation counter + private val serviceState = MutableSharedFlow<ServiceNotificationSnapshot>(
extraBufferCapacity = 1,
onBufferOverflow = BufferOverflow.DROP_OLDEST,
)
init {
scope.launch { serviceState.collectLatest { snapshot -> renderAndNotify(snapshot) } }
}
2. The Compose half may not do what the title saysHoisting That may well be the outcome you want, since blocking during an animation frame is worse than blocking at screen entry. Was there a measured jank or ANR behind it? The cost is real API friction — 3. Minor: misleading sequencing left behind
Checked and fine
|
|
@jamesarich Thanks for the detailed review. This work was driven by a captured ANR rather than a theoretical cleanup: the main thread was blocked in CMP resource loading from I agree the current generation/job machinery is heavier than necessary, and the notify-after-generation-check window means it is not completely generation-safe. I’m going to move the PR back to draft and rework that path around a single ordered I’ll retain the Connections hoisting as mitigation for the measured animation-path ANR, add a TODO identifying the temporary CMP workaround, and clarify that the initial foreground notification is only the immediate fallback. |
a4eee02 to
51b102f
Compare
jeremiah-k
changed the title
|
I replaced the generation/job machinery with one replaying, drop-oldest |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt (1)
380-398: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer
safeCatching {}for the render path too.The fallback post already uses
safeCatching, while the primary render uses a hand-rolledtry/catch(Exception)plus@Suppress("TooGenericExceptionCaught")and manualCancellationExceptionrethrow. UsingsafeCatchinghere keeps cancellation semantics centralized and drops the suppression.As per coding guidelines, "Use
safeCatching {}fromcore:commonin coroutine or suspend contexts; reserverunCatchingfor cleanup and teardown code."♻️ Sketch
- `@Suppress`("TooGenericExceptionCaught") private suspend fun renderAndPostServiceNotification(snapshot: ServiceNotificationSnapshot) { - try { - val rendered = renderServiceNotification(snapshot) - postServiceNotification(rendered.notification, rendered.message) - } catch (ex: CancellationException) { - throw ex - } catch (ex: Exception) { - Logger.e(ex) { "Failed to render service notification resources" } - val fallback = - createServiceStateNotification( - name = applicationLabel, - message = snapshot.previousMessage, - nextUpdateAt = snapshot.nextUpdateAt, - ) - safeCatching { postServiceNotification(fallback, snapshot.previousMessage) } - .onFailure { Logger.e(it) { "Failed to post fallback service notification" } } - } + safeCatching { + val rendered = renderServiceNotification(snapshot) + postServiceNotification(rendered.notification, rendered.message) + } + .onFailure { ex -> + Logger.e(ex) { "Failed to render service notification resources" } + val fallback = + createServiceStateNotification( + name = applicationLabel, + message = snapshot.previousMessage, + nextUpdateAt = snapshot.nextUpdateAt, + ) + safeCatching { postServiceNotification(fallback, snapshot.previousMessage) } + .onFailure { Logger.e(it) { "Failed to post fallback service notification" } } + } }Verify
safeCatchingrethrowsCancellationExceptionbefore dropping the explicit rethrow.🤖 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/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt` around lines 380 - 398, Update renderAndPostServiceNotification to wrap the primary renderServiceNotification and postServiceNotification flow in safeCatching, removing the `@Suppress` annotation and manual try/catch with CancellationException handling. Preserve the existing error log, fallback notification creation, and fallback posting behavior through the safeCatching failure path, and verify safeCatching preserves coroutine cancellation by rethrowing CancellationException.Source: Coding guidelines
🤖 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/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt`:
- Around line 421-433: Decouple local-stats seeding from the node lookup in the
telemetry cache initialization block. Keep cachedDeviceMetrics assignment inside
the myNodeNum/nodeDBbyNum lookup, but assign cachedLocalStats directly from
repo.localStats when it is null, preserving the nonzero-uptime filter.
---
Nitpick comments:
In
`@core/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshNotificationManagerImpl.kt`:
- Around line 380-398: Update renderAndPostServiceNotification to wrap the
primary renderServiceNotification and postServiceNotification flow in
safeCatching, removing the `@Suppress` annotation and manual try/catch with
CancellationException handling. Preserve the existing error log, fallback
notification creation, and fallback posting behavior through the safeCatching
failure path, and verify safeCatching preserves coroutine cancellation by
rethrowing CancellationException.
🪄 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: 8800bf02-ec62-4d8b-8fb5-64f5ac331c65
📒 Files selected for processing (10)
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/service/src/androidMain/kotlin/org/meshtastic/core/service/MeshService.ktcore/testing/src/commonMain/kotlin/org/meshtastic/core/testing/TestScopes.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 (8)
- core/ui/src/commonTest/kotlin/org/meshtastic/core/ui/component/LoraSignalIndicatorUiTest.kt
- core/testing/src/commonMain/kotlin/org/meshtastic/core/testing/TestScopes.kt
- core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplTest.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
- core/service/src/androidHostTest/kotlin/org/meshtastic/core/service/MeshNotificationManagerImplConversationTest.kt
- feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/components/CurrentlyConnectedInfo.kt
- feature/connections/src/commonMain/kotlin/org/meshtastic/feature/connections/ui/ConnectionsScreen.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.
…d exception containment Replace blocking resource loads with suspend-aware formatting in notification paths. Use deterministic test dispatchers for render assertions. Contain exceptions from posting fallback notifications. Parent the render scope Job to runTest so render-coroutine failures remain observable through test completion.
… and localize preview labels Resolve Rssi and battery display values outside animated composition to prevent recomposition jank. Localize connection preview labels through core:resources for consistent string handling.
Replace the generation counter and manually cancelled lazy jobs with one replaying, drop-oldest SharedFlow collector. collectLatest cancels obsolete resource work and serializes the complete render-and-notify action, closing the unlocked Binder last-writer window without holding the snapshot lock across IPC. Preserve the immediate foreground fallback, cached telemetry and message state, and fallback notification behavior. Use cancellation-aware error containment for asynchronous rendering, and seed persisted local stats independently of local-node row readiness after process or transport restart. Cover latest-state selection, identical-state reposting after notifications are cleared, and local-stat recovery before the node cache is warm. Clarify the asynchronous startForeground sequence and document the caller-provided Connections labels as a temporary CMP-6615 workaround for Compose Multiplatform 1.11.1.
51b102f to
1f8df84
Compare
2 participants
Overview
This avoids Compose resource-loading stalls in 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 during foreground-service startup and telemetry updates.
The Connections UI now resolves stable connected-device display text outside the animated subtree, avoiding repeated entry into the blocking CMP resource state during animation frames. Service notifications return a cached or immediate application-label fallback for
startForeground, then render localized status text asynchronously on the existingServiceScope.Review Guide
The commits are intended to be reviewed in order:
SharedFlowcollector, closes the last-writer window, and documents the temporary CMP workaround.Key Changes
Connection UI
RssiandMaterialBatteryInfoto accept caller-provided labels while retaining localized defaults for other callers.CurrentlyConnectedTextvalue for localized connected-device text.AnimatedContent.Service Notifications
startForegroundfallback.ServiceScope.SharedFlowcollector.collectLatestto cancel obsolete resource work and serialize the complete render-and-notify action, preventing an older Binder post from overwriting newer notification text.serviceNotificationLock, then release the lock before posting throughNotificationManager.MeshService.startForegrounduses the cached or immediate fallback while localized rendering completes asynchronously.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
Improvements
Tests