Enhance install error message on app backgrounding #3432
Conversation
📝 WalkthroughWalkthroughThis PR introduces network error detection during app installation. A new foreground state tracking mechanism is added to CommCareApplication, setting isForeground to true on START/RESUME events and false on STOP events, with a public accessor isAppInForeground(). During file system installation, when an UnknownHostException occurs, the system checks if the app is in the background with no network connectivity; if so, it throws an UnresolvedResourceException indicating connection interruption. A new ConnectionInterrupted status is added to AppInstallStatus and handled in ResourceInstallUtils to map and report this specific failure. Supporting notification locale strings for the connection interruption are also added. Sequence DiagramsequenceDiagram
participant App as App Lifecycle
participant Installer as FileSystemInstaller
participant ConnStatus as AppInstallStatus
participant Utils as ResourceInstallUtils
App->>App: Activity.onStart()
App->>App: isForeground = true
Note over App: App moves to background
App->>App: Activity.onStop()
App->>App: isForeground = false
Note over Installer: Installation attempt with<br/>no network connectivity
Installer->>Installer: Catch UnknownHostException
Installer->>Installer: isAppInBackgroundAndConnected()
alt App in background AND no network
Installer->>ConnStatus: UnresolvedResourceException<br/>(connection interrupted)
else Otherwise
Installer->>ConnStatus: UnreliableSourceException
end
ConnStatus->>Utils: handleAppInstallResult()
alt Status is ConnectionInterrupted
Utils->>Utils: Return AppInstallStatus.ConnectionInterrupted
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/assets/locales/android_translatable_strings.txt(1 hunks)app/src/org/commcare/CommCareApplication.java(2 hunks)app/src/org/commcare/android/resource/installers/FileSystemInstaller.java(3 hunks)app/src/org/commcare/engine/resource/AppInstallStatus.java(1 hunks)app/src/org/commcare/engine/resource/ResourceInstallUtils.java(3 hunks)
🧰 Additional context used
🧠 Learnings (6)
📓 Common learnings
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/src/org/commcare/activities/CommCareSetupActivity.java:360-364
Timestamp: 2025-05-22T14:28:35.959Z
Learning: In CommCareSetupActivity.java, the call to installFragment.showConnectErrorMessage() after fragment transactions is intentionally unguarded with null checks. This follows the app's design pattern where critical error paths prefer immediate crashes over silent failures, making potential issues immediately visible during development rather than hiding them with defensive programming.
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/src/org/commcare/fragments/SelectInstallModeFragment.java:201-205
Timestamp: 2025-05-22T14:26:41.341Z
Learning: In SelectInstallModeFragment.java, the showConnectErrorMessage method intentionally omits null checks because it's called at a specific point in the startup flow where UI is guaranteed to be loaded. It's designed to crash if activity or view is null to make potential issues immediately visible rather than hiding them with defensive programming.
📚 Learning: 2025-05-08T11:08:18.530Z
Learnt from: shubham1g5
Repo: dimagi/commcare-android PR: 0
File: :0-0
Timestamp: 2025-05-08T11:08:18.530Z
Learning: PR #3048 "Phase 4 Connect PR" introduces a substantial feature called "Connect" to the CommCare Android app, which includes messaging, job management, delivery tracking, payment processing, authentication flows, and learning modules. It follows a modern architecture using Navigation Components with three navigation graphs, segregated business logic in Manager classes, and proper database persistence.
Applied to files:
app/src/org/commcare/CommCareApplication.javaapp/src/org/commcare/android/resource/installers/FileSystemInstaller.java
📚 Learning: 2025-05-22T14:28:35.959Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3121
File: app/src/org/commcare/activities/CommCareSetupActivity.java:360-364
Timestamp: 2025-05-22T14:28:35.959Z
Learning: In CommCareSetupActivity.java, the call to installFragment.showConnectErrorMessage() after fragment transactions is intentionally unguarded with null checks. This follows the app's design pattern where critical error paths prefer immediate crashes over silent failures, making potential issues immediately visible during development rather than hiding them with defensive programming.
Applied to files:
app/src/org/commcare/android/resource/installers/FileSystemInstaller.java
📚 Learning: 2025-06-06T20:15:21.134Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 3108
File: app/src/org/commcare/fragments/connect/ConnectJobIntroFragment.java:65-71
Timestamp: 2025-06-06T20:15:21.134Z
Learning: In the CommCare Android Connect module, job.getLearnAppInfo() and getLearnModules() should never return null according to the system design. The team prefers to let the code crash if these values are unexpectedly null rather than adding defensive null checks, following a fail-fast philosophy to catch bugs early rather than masking them.
Applied to files:
app/src/org/commcare/android/resource/installers/FileSystemInstaller.java
📚 Learning: 2025-01-21T17:29:58.014Z
Learnt from: OrangeAndGreen
Repo: dimagi/commcare-android PR: 2912
File: app/src/org/commcare/fragments/connect/ConnectPaymentSetupFragment.java:61-66
Timestamp: 2025-01-21T17:29:58.014Z
Learning: In the CommCare Android app, for non-critical convenience features like phone number auto-population, exceptions should be logged but fail silently when there's a manual fallback available. This approach prevents app crashes while maintaining the ability to debug issues through logs.
Applied to files:
app/src/org/commcare/android/resource/installers/FileSystemInstaller.java
📚 Learning: 2025-05-16T15:00:47.041Z
Learnt from: pm-dimagi
Repo: dimagi/commcare-android PR: 3113
File: app/src/org/commcare/utils/OTPVerificationCallback.java:1-9
Timestamp: 2025-05-16T15:00:47.041Z
Learning: Public interfaces, classes, and methods in the CommCare Android codebase should include proper JavaDoc comments that describe their purpose, parameters, return values, and exceptions where applicable.
Applied to files:
app/src/org/commcare/android/resource/installers/FileSystemInstaller.java
🔇 Additional comments (1)
app/src/org/commcare/engine/resource/ResourceInstallUtils.java (1)
274-276: Inconsistency confirmed: OfflineUserRestoreAndroidInstaller bypasses background-state checks in ResourceInstallUtils mapping.The issue is real. While
FileSystemInstaller.java(lines 131-138) conditionally wrapsUnknownHostExceptioninUnresolvedResourceExceptiononly when the app is in the background AND connected,OfflineUserRestoreAndroidInstaller.java(lines 56-57) catchesIOExceptionbroadly and wraps ALL cases inUnresolvedResourceExceptionwithout any background-state check. This meansUnknownHostExceptionfromOfflineUserRestoreAndroidInstallergets passed toResourceInstallUtils.processUnresolvedResource()and unconditionally mapped toConnectionInterrupted, bypassing the background-state validation thatFileSystemInstallerenforces.Questions for resolution:
- Should
OfflineUserRestoreAndroidInstalleralso apply theisAppInBackgroundAndConnected()check before wrapping network-relatedIOExceptionasUnresolvedResourceException?- Or should
processUnresolvedResource()apply the background check universally before returningConnectionInterrupted?- Consider centralizing background-state validation in
processUnresolvedResource()to enforce consistency across all installers.
| notification.install.connection.interrupted.title=Internet connection was interrupted | ||
| notification.install.connection.interrupted.detail=Installation failed because the internet connection was interrupted, possibly due to moving away from CommCare | ||
| notification.install.connection.interrupted.action=Please try again and keep CommCare open untill the installation is complete. If the issue persists, contact your supervisor. |
There was a problem hiding this comment.
Fix typo and improve phrasing in locale strings.
Two issues in the new locale strings:
- Line 683: "untill" is misspelled - should be "until"
- Line 682: "possibly due to moving away from CommCare" is awkward phrasing - consider "possibly because the app was moved to the background"
Apply this diff to fix:
-notification.install.connection.interrupted.detail=Installation failed because the internet connection was interrupted, possibly due to moving away from CommCare
-notification.install.connection.interrupted.action=Please try again and keep CommCare open untill the installation is complete. If the issue persists, contact your supervisor.
+notification.install.connection.interrupted.detail=Installation failed because the internet connection was interrupted, possibly because the app was moved to the background
+notification.install.connection.interrupted.action=Please try again and keep CommCare open until the installation is complete. If the issue persists, contact your supervisor.🤖 Prompt for AI Agents
In app/assets/locales/android_translatable_strings.txt around lines 681 to 683,
fix two issues in the notification.install.connection.interrupted strings:
replace "possibly due to moving away from CommCare" with "possibly because the
app was moved to the background" on the detail line, and correct the typo
"untill" to "until" on the action line; update the two offending lines
accordingly so the phrasing reads smoothly and the spelling is correct.
| private CommCareNoficationManager noficationManager; | ||
|
|
||
| private boolean backgroundSyncSafe; | ||
| private boolean isForeground = true; |
There was a problem hiding this comment.
Consider thread safety for the foreground state flag.
The isForeground boolean is accessed from multiple threads (lifecycle callbacks and the install process) without synchronization. While boolean reads/writes are atomic in Java, there's no guarantee of visibility across threads without proper synchronization.
Consider one of these approaches:
- Make the field
volatileto ensure visibility across threads - Use
AtomicBooleanfor thread-safe operations - Synchronize access to the field
Apply this diff to make the field volatile:
- private boolean isForeground = true;
+ private volatile boolean isForeground = true;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private boolean isForeground = true; | |
| private volatile boolean isForeground = true; |
🤖 Prompt for AI Agents
In app/src/org/commcare/CommCareApplication.java around line 215, the
isForeground boolean is accessed from multiple threads without synchronization
which can cause visibility issues; make the field volatile (or alternatively
replace with AtomicBoolean or synchronize access) so updates in one thread are
visible to others—update the field declaration to use the volatile modifier and
adjust any direct reads/writes if you choose AtomicBoolean or synchronized
access instead.
avazirna
force-pushed
the
improve-install-error-message-on-app-backgrounding
branch
2 times, most recently
from
abeeca1 to
e4c4f6e
Compare
avazirna
force-pushed
the
improve-install-error-message-on-app-backgrounding
branch
from
e4c4f6e to
cdb47f8
Compare
|
|
||
| throw mURE; | ||
| } catch (IOException e) { | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
Please remove printStackTrace to protect production app
| UnreliableSourceException exception = new UnreliableSourceException(r, e.getMessage()); | ||
| exception.initCause(e); | ||
| throw exception; | ||
| if (e instanceof UnknownHostException && !CommCareApplication.instance().isAppInForeground()) { |
There was a problem hiding this comment.
@avazirna Can it also throw IOException in some cases?
There was a problem hiding this comment.
Yes, UnknownHostException is a subclass of IOException as well as SocketException, SocketTimeoutException and a few others
| case ON_START, ON_RESUME: | ||
| isForeground = true; | ||
| break; | ||
| case ON_STOP: |
| Logger.exception("Encountered SSLException while trying to install a resource", exception); | ||
| return AppInstallStatus.BadSslCertificate; | ||
| } | ||
| if (exception.getCause() instanceof UnknownHostException) { |
There was a problem hiding this comment.
@avazirna Should it check for UnresolvedResourceException ? UnknownHostException might also come whenever there is no internet in app
There was a problem hiding this comment.
The determining factor is when CommCare is in the background, and in that case we wrap the UnknownHostException inside a UnresolvedResourceException. From Android 15+, checking if the device is connected when in the background seems to always return false.
There was a problem hiding this comment.
So just trying to understand here. UnresolvedResourceException is raised whenever there is UnknownHostException and app is in background so shouldn't we check for UnresolvedResourceException instead of UnknownHostException?
There was a problem hiding this comment.
I see, so this check is inside a method that only handles UnresolvedResourceException wrapped exceptions, processUnresolvedResource() and this is the catch block where that method is called.
| exception.initCause(e); | ||
| throw exception; | ||
| if (e instanceof UnknownHostException && !CommCareApplication.instance().isAppInForeground()) { | ||
| UnresolvedResourceException mURE = |
There was a problem hiding this comment.
Should also add initCause method?
There was a problem hiding this comment.
This constructor I'm using already accepts a cause object.
|
@avazirna can you explain the rationale on us trying to fix the error state here instead of stopping the error from happening all-together ? |
@shubham1g5 the initial fix involve wrapping the sync network call inside a WorkManager worker and blocking the thread with a |
|
Got it, my strong sense is that fix here would involve a foreground service to be attached to all background processes (it's a common practice in Android to use that for long networking tasks). I do agree that it's a better behaviour for short term and no objections on the change itself. |
| notification.install.connection.interrupted.title=Internet connection was interrupted | ||
| notification.install.connection.interrupted.detail=Installation failed because the internet connection was interrupted, possibly due to moving away from CommCare | ||
| notification.install.connection.interrupted.action=Please try again and keep CommCare open untill the installation is complete. If the issue persists, contact your supervisor. | ||
|
|
There was a problem hiding this comment.
Has anyone from the Product team had input on this messaging?
There was a problem hiding this comment.
That's a good point @conroy-ricketts, we typically don't request sign-off at this level unless the change is more substantial, but maybe we should start doing so. How is this handled on the Connect side? Does the Product team reviewing every single change related to client-facing messaging?
There was a problem hiding this comment.
I'm not too familiar with the process on the Connect side, but we did recently have a push to review all the error messages in Personal ID. If not already, I think it would be a great idea for the team to start getting input from someone very familiar with UI/UX design on user-facing messages! This is something I've done working with other engineering teams in the past and it really does help ensure that we are providing the best experience for users (e.g. clarity of messages, accessibility improvements, etc.)
Product Description
This PR improves messaging when an app installation fails because CommCare was put in the background on Android 15+. When this happens, the system interrupts the connection and an
UnknownHostExceptionis thrown, which currently leads to CommCare showing aUnknown error during app installmessage. With this change the users will instead see:Technical summary
ConnectionInterruptedtoAppInstallStatus;IOException, if the cause isUnknownHostExceptionand CommCare is in the background, this exception is then wrapped in aUnresolvedResourceException.Safety Assurance
Safety story
Successfully tested locally.
Labels and Review
Release Note: Better error messaging for failed App installations