Fix background network calls on Android 15 and higher#3525
Fix background network calls on Android 15 and higher#3525
Conversation
📝 WalkthroughWalkthroughAdds a new foreground service, Sequence DiagramsequenceDiagram
participant RT as ResourceEngineTask
participant CCT as CommCareTask
participant App as CommCareApplication
participant NNS as NetworkNotificationService
participant NM as NotificationManager
RT->>CCT: execute() (runNotificationService=true)
CCT->>CCT: onPreExecute()\nshouldRunNetworkNotificationService()
CCT->>App: startForegroundService(start Intent)
App->>NNS: deliver Intent -> onCreate/onStartCommand(START)
NNS->>NM: build & notify(initial notification)
CCT->>CCT: doInBackground() / progress updates
CCT->>App: startService(update Intent with progress)
App->>NNS: onStartCommand(UPDATE)
NNS->>NM: notify(updated progress)
CCT->>App: startService(stop Intent) / onPostExecute()
App->>NNS: onStartCommand(STOP) / onDestroy()
NNS->>NM: cancel notification
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested labels
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. 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
🤖 Fix all issues with AI agents
In `@app/src/org/commcare/services/NetworkNotificationService.kt`:
- Around line 84-88: The onDestroy method calls the API-24 overload
stopForeground(int) which will crash on devices < API 24; replace the direct
call with a runtime SDK check in onDestroy (method name onDestroy) that calls
stopForeground(STOP_FOREGROUND_REMOVE) only when Build.VERSION.SDK_INT >=
Build.VERSION_CODES.N and otherwise calls the legacy stopForeground(boolean)
overload (e.g. stopForeground(true) to match removal), then set isServiceRunning
= false and call super.onDestroy(); keep or remove the `@RequiresApi` annotation
as desired since the runtime check provides the actual protection.
🧹 Nitpick comments (1)
app/src/org/commcare/tasks/templates/CommCareTask.java (1)
153-168: Use the PROGRESS_INTENT_EXTRA constant to avoid drift.♻️ Suggested change
- return getNotificationIntent().setAction(UPDATE_PROGRESS_NOTIFICATION_ACTION) - .putExtra("progress", (int[])value); + return getNotificationIntent().setAction(UPDATE_PROGRESS_NOTIFICATION_ACTION) + .putExtra(NetworkNotificationService.PROGRESS_INTENT_EXTRA, (int[])value);
| @RequiresApi(Build.VERSION_CODES.N) | ||
| override fun onDestroy() { | ||
| stopForeground(STOP_FOREGROUND_REMOVE) | ||
| isServiceRunning = false | ||
| super.onDestroy() |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "build.gradle*" -o -name "*.kts" | head -20Repository: dimagi/commcare-android
Length of output: 140
🏁 Script executed:
rg "minSdk" -g "build.gradle*" -g "*.kts" --max-count=20Repository: dimagi/commcare-android
Length of output: 171
🏁 Script executed:
rg "stopForeground" app/src/org/commcare/services/NetworkNotificationService.kt -B 2 -A 2Repository: dimagi/commcare-android
Length of output: 243
🌐 Web query:
Android Service stopForeground API documentation int parameter
💡 Result:
Service.stopForeground(int notificationBehavior) takes an int “notification behavior” flag that controls what happens to the foreground-service notification when the service is removed from the foreground state:
Service.STOP_FOREGROUND_REMOVE: cancel/remove the notification immediately. [1]Service.STOP_FOREGROUND_DETACH: detach the notification from the service; the notification remains shown (and if it was deferred, it is posted immediately). [1]Service.STOP_FOREGROUND_LEGACY(value 0): legacy behavior—notification stays until the service is fully stopped/destroyed, then is cancelled. [1]
Also note: stopForeground(boolean removeNotification) is deprecated in API 33; Android recommends calling stopForeground(int) with either STOP_FOREGROUND_REMOVE or STOP_FOREGROUND_DETACH explicitly. [1]
If you’re using AndroidX for compatibility helpers, the same flags exist in androidx.core.app.ServiceCompat (STOP_FOREGROUND_REMOVE = 1, STOP_FOREGROUND_DETACH = 2). [2]
Sources:
[1] Android Developers — android.app.Service API reference (stopForeground docs) (developer.android.com)
[2] Android Developers — androidx.core.app.ServiceCompat API reference (developer.android.com)
Citations:
- 1: https://developer.android.com/reference/android/app/Service?utm_source=openai
- 2: https://developer.android.com/reference/androidx/core/app/ServiceCompat?utm_source=openai
🌐 Web query:
stopForeground int overload API level added Android 24
💡 Result:
Service.stopForeground(int flags) (the int overload) was added in API level 24 (Android 7.0). It was introduced along with flags like STOP_FOREGROUND_REMOVE and STOP_FOREGROUND_DETACH. [1], [2]
🌐 Web query:
Service.stopForeground boolean overload API level Android compatibility
💡 Result:
Service.stopForeground(boolean removeNotification)was added in API level 5 (Android 2.0) and is deprecated in API level 33 (Android 13). The boolean maps to legacy behaviors (true≈ remove notification;false≈ legacy/keep behavior). [1][2]Service.stopForeground(int notificationBehavior)was added in API level 24 (Android 7.0). UseSTOP_FOREGROUND_REMOVEto remove the notification, orSTOP_FOREGROUND_DETACHto keep it shown but detach it from the service lifecycle. [1][3]
Practical compatibility pattern:
if (Build.VERSION.SDK_INT >= 24) {
stopForeground(Service.STOP_FOREGROUND_REMOVE); // or STOP_FOREGROUND_DETACH
} else {
stopForeground(true); // legacy
}This avoids calling the int overload on API < 24, and avoids the deprecated boolean API on newer devices (and when targeting/compiling newer SDKs). [1][2][3]
Sources:
[1] Android Service API reference (stopForeground overloads, API levels, deprecation) https://developer.android.google.cn/reference/android/app/Service
[2] Android API diff (API 33: stopForeground(boolean) now deprecated) https://developer.android.com/sdk/api_diff/33/changes/android.app.Service
[3] Android Service API reference (STOP_FOREGROUND_* constants, API levels) https://developer.android.com/reference/android/app/Service
Guard stopForeground(int) for API < 24 with runtime check.
The stopForeground(int) overload was added in API 24, but your minSdk is 23. The @RequiresApi annotation provides no runtime protection—it only affects lint checks. On API 23 devices, this will crash with NoSuchMethodError. Add a runtime SDK check and fall back to the legacy stopForeground(boolean) overload for older APIs.
🛠️ Proposed fix
- `@RequiresApi`(Build.VERSION_CODES.N)
- override fun onDestroy() {
- stopForeground(STOP_FOREGROUND_REMOVE)
+ override fun onDestroy() {
+ if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
+ stopForeground(STOP_FOREGROUND_REMOVE)
+ } else {
+ stopForeground(true)
+ }
isServiceRunning = false
super.onDestroy()
}🤖 Prompt for AI Agents
In `@app/src/org/commcare/services/NetworkNotificationService.kt` around lines 84
- 88, The onDestroy method calls the API-24 overload stopForeground(int) which
will crash on devices < API 24; replace the direct call with a runtime SDK check
in onDestroy (method name onDestroy) that calls
stopForeground(STOP_FOREGROUND_REMOVE) only when Build.VERSION.SDK_INT >=
Build.VERSION_CODES.N and otherwise calls the legacy stopForeground(boolean)
overload (e.g. stopForeground(true) to match removal), then set isServiceRunning
= false and call super.onDestroy(); keep or remove the `@RequiresApi` annotation
as desired since the runtime check provides the actual protection.
avazirna
force-pushed
the
vaz/fix-background-network-calls-on-Android15-and-higher
branch
from
96a93cb to
1e9e829
Compare
avazirna
force-pushed
the
vaz/fix-background-network-calls-on-Android15-and-higher
branch
from
1e9e829 to
9e6c39d
Compare
avazirna
requested review from
OrangeAndGreen,
conroy-ricketts and
shubham1g5
|
What's the LOE on the better long-term solution? Trying to get a sense of why we wouldn't do that instead. And what's the rationale for doing this knowing that it may be rejected by the Play Store anyways? |
|
Also, is there a ticket for this? |
| network.requests.starting=Starting network requests | ||
| network.requests.running=Running network requests | ||
| network.requests.progress=Completed: ${0}/${1} |
There was a problem hiding this comment.
This is not super user-friendly and ideally we should show contextual messages here depending on the task in progress, for eg. "Update in progress" and less ideally change it to a generic but more user friendly messaging like "Downloading data.."
| val activityToLaunch = Intent(this, DispatchActivity::class.java) | ||
| activityToLaunch.setAction("android.intent.action.MAIN") | ||
| activityToLaunch.addCategory("android.intent.category.LAUNCHER") |
There was a problem hiding this comment.
- is not this already a default behaviour for notifications click ?
- Is it possible to not do anything on notification click instead ? I think it's just a contextual message and it might not be good UX for users if they move out of "Form Entry" screen on clicking these notifications.
There was a problem hiding this comment.
- is not this already a default behaviour for notifications click ?
This is for to set the content intent, without it, there won't be any action if the user taps the notification. Considering that this notification is mainly for when CommCare goes to the background, it makes sense to me to offer the user a way to go go back.
There was a problem hiding this comment.
But the notification will appear even if the app is not backgrounded right ?
There was a problem hiding this comment.
- I think it's just a contextual message and it might not be good UX for users if they move out of "Form Entry" screen on clicking these notifications.
Tapping the notification shouldn't make the users exit "Form entry". This is the same behavior that occurs when users tap the session notification.
There was a problem hiding this comment.
Tapping the notification shouldn't make the users exit "Form entry".
Interesting, curious how is that controlled in the code ?
There was a problem hiding this comment.
Not directly.DispatchActivity.class is a root activity and already present in the task’s stack, so a new instance is not created. But the intended behavior here is that when the app is in the background, the system brings it to the foreground to deliver the pending intent.
| val pendingIntentFlags = PendingIntent.FLAG_UPDATE_CURRENT or PendingIntent.FLAG_IMMUTABLE | ||
| val pendingIntent = PendingIntent.getActivity(this, 0, activityToLaunch, pendingIntentFlags) | ||
|
|
||
| return NotificationCompat |
There was a problem hiding this comment.
Is it possible for these notifications to be sticky (non-dismissable) ?
There was a problem hiding this comment.
It should be already, the ongoing flag is set to true. Unless the comment is about the service itself.
There was a problem hiding this comment.
Sounds good, didn't realise this flag is used to communicate UX on a notification being "user-dismissable". But do want to do double confirm that user is not able to dismiss this notification ?
| ) | ||
|
|
||
| @RequiresApi(Build.VERSION_CODES.N) | ||
| override fun onDestroy() { |
There was a problem hiding this comment.
we should also auto-dismiss the notifications started by this service.
There was a problem hiding this comment.
I believe we want to make them sticky, right? Are you referring to the FLAG_AUTO_CANCEL?
There was a problem hiding this comment.
We want to make them sticky but only for the duration of the service but once service completes, the notification should auto disappear.
Are you referring to the FLAG_AUTO_CANCEL?
No, we don't really want user to be able to dismiss the notification.
| if (shouldUpdateNetworkNotificationProgress(values)) { | ||
| CommCareApplication.instance().startService(getNotificationIntentProgress(values[0])); | ||
| } |
There was a problem hiding this comment.
Think we handle progress differently in different tasks and this will need to be cross-checked for every task , I think it's better to not show any progress in the notification instead by default and have individual tasks implement progress updates if required. Just saying "Download in Progress" should be sufficient here for most cases.
There was a problem hiding this comment.
Yeah, I battled with this myself and may have overcomplicated it. The intention was to address immediate needs and refactor as we expand this service to other tasks. But I'm onboard with the idea of starting small.
| if (NetworkNotificationService.Companion.isServiceRunning()) { | ||
| CommCareApplication.instance().stopService(getNotificationIntent()); | ||
| } |
There was a problem hiding this comment.
This would not work anytime we have more than one Async tasks in parallel as any task terminating will also stop the service for the in progress task.
There was a problem hiding this comment.
@shubham1g5 I have opted to store the running tasks IDs in a List and call stopSelf() once the list is empty - 508fa39
Yes, there is, let me link it to the PR. |
@conroy-ricketts With the |
I don't expect to be rejected now, but it's a risk that we need to monitor. |
avazirna
force-pushed
the
vaz/fix-background-network-calls-on-Android15-and-higher
branch
from
66eec0d to
bdb2f0f
Compare
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
app/src/org/commcare/tasks/templates/CommCareTask.java (1)
73-88:⚠️ Potential issue | 🟠 MajorService is not stopped when a task is cancelled — potential service leak.
onCancelled()does not send aSTOP_NOTIFICATION_ACTIONto theNetworkNotificationService. If a task that opted into the notification service is cancelled, its task ID remains registered in the service, preventing the auto-shutdown (stopSelf()triggered when the task list becomes empty). The service will keep running indefinitely.Suggested fix
`@Override` protected void onCancelled() { super.onCancelled(); synchronized (connectorLock) { CommCareTaskConnector<Receiver> connector = getConnector(); if (connector != null) { connector.startTaskTransition(); connector.stopBlockingForTask(getTaskId()); connector.taskCancelled(); handleCancellation(connector.getReceiver()); connector.stopTaskTransition(taskId); } } + + if (NetworkNotificationService.Companion.isServiceRunning()) { + CommCareApplication.instance().startForegroundService(getNotificationStopIntent()); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/tasks/templates/CommCareTask.java` around lines 73 - 88, onCancelled() must notify NetworkNotificationService that this task is no longer active to avoid a service leak; modify CommCareTask.onCancelled to send the STOP_NOTIFICATION_ACTION intent to NetworkNotificationService (including the task id from getTaskId()) when a connector is non-null and the task opted into notifications—do this alongside the existing connector.stopBlockingForTask(getTaskId())/connector.taskCancelled() calls (before or after them) so the service can remove the task and possibly call stopSelf(); reference the onCancelled method, getConnector(), getTaskId(), connector.stopBlockingForTask(...) and NetworkNotificationService.STOP_NOTIFICATION_ACTION when locating where to add the stop-intent dispatch.
🧹 Nitpick comments (3)
app/AndroidManifest.xml (1)
343-347: Consider addingandroid:exported="false"explicitly.The
NetworkNotificationServicedeclaration omits theandroid:exportedattribute. While it defaults tofalsewhen no intent-filter is present, other services in this manifest (e.g.,AudioRecordingServiceat line 350) explicitly set it. Being explicit is preferable for consistency and to prevent accidental exposure if an intent-filter is added later.Suggested change
<service android:enabled="true" + android:exported="false" android:foregroundServiceType="dataSync" android:name="org.commcare.services.NetworkNotificationService"> </service>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/AndroidManifest.xml` around lines 343 - 347, The NetworkNotificationService declaration (android:name="org.commcare.services.NetworkNotificationService") lacks an explicit android:exported attribute; update the <service> element for NetworkNotificationService to include android:exported="false" to match other services (e.g., AudioRecordingService) and prevent accidental external exposure if an intent-filter is added later.app/src/org/commcare/services/NetworkNotificationService.kt (1)
93-93:super.onStartCommand()returnsSTART_STICKY, which will restart the service without the original intent.If the system kills and restarts this service,
onStartCommandwill be called with anullintent, resulting in no action taken and an empty task list — the service will then stop itself immediately. While not a crash, returningSTART_NOT_STICKYwould be more appropriate for a task-driven service that should not be restarted without an explicit command.Suggested fix
- return super.onStartCommand(intent, flags, startId) + return START_NOT_STICKY🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/services/NetworkNotificationService.kt` at line 93, The service currently returns super.onStartCommand(...), which yields START_STICKY; change NetworkNotificationService.onStartCommand to return START_NOT_STICKY instead so the system won't restart the service without the original intent. Locate the onStartCommand method in class NetworkNotificationService and replace the final return expression (currently calling super.onStartCommand(intent, flags, startId)) with the Service constant START_NOT_STICKY; keep existing intent-null checks/early exits as-is so the method still stops itself when no work is provided.app/src/org/commcare/tasks/templates/CommCareTask.java (1)
139-142: API level gate usesUPSIDE_DOWN_CAKE(API 34), butonCreatein the service handles API < 34 too.
shouldRunNetworkNotificationService()gates onBuild.VERSION_CODES.UPSIDE_DOWN_CAKE(API 34), meaning the service is never started on API 33 or below. Meanwhile,NetworkNotificationService.onCreate()has a branch for< UPSIDE_DOWN_CAKE(lines 52-53 of the service). That branch is dead code since the service is never started on those API levels. This is fine functionally (the fix targets Android 15+ restrictions), but the dead code in the service is misleading — consider either documenting the intent or removing theelsebranch from the service'sonCreate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@app/src/org/commcare/tasks/templates/CommCareTask.java` around lines 139 - 142, shouldRunNetworkNotificationService() gates starting the service to API 34+, but NetworkNotificationService.onCreate() still contains an else branch for < UPSIDE_DOWN_CAKE which is dead code; either remove that else branch and simplify onCreate() to only handle the API >= UPSIDE_DOWN_CAKE path, or if you intend to support older APIs, update shouldRunNetworkNotificationService() to allow starting the service on lower API levels. Locate shouldRunNetworkNotificationService() and NetworkNotificationService.onCreate() and then either (A) delete the < UPSIDE_DOWN_CAKE branch and add a brief comment in onCreate() clarifying the service is only started on API 34+, or (B) change shouldRunNetworkNotificationService() to not require UPSIDE_DOWN_CAKE so the onCreate() else branch is reachable — pick the option matching intended behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@app/assets/locales/android_translatable_strings.txt`:
- Around line 1021-1023: The localization keys used by
NetworkNotificationService.kt ("network.requests.starting" and
"network.requests.running") do not match the keys in the translations file
(network.notification.service.starting / .running); fix this by making them
identical: either rename the keys in the translation file to
"network.requests.starting" and "network.requests.running" or change the string
literals in NetworkNotificationService.kt to
"network.notification.service.starting" and
"network.notification.service.running" so that Localization.get() can resolve
the texts (update the three symbols: the two keys in the translations and the
occurrences in NetworkNotificationService.kt at the places using those
literals).
In `@app/src/org/commcare/services/NetworkNotificationService.kt`:
- Around line 80-85: The fallback localization key used when building the
notification is incorrect: in NetworkNotificationService (inside the
notificationManager.notify call that calls buildNotification), replace the
hard-coded fallback string "network.requests.running" with the correct key
"network.notification.service.running" where the intent extra
PROGRESS_TEXT_KEY_INTENT_EXTRA is read, so buildNotification(...) receives the
proper default localization key (keep the existing null-coalescing logic).
- Around line 101-108: registerTaskId can add duplicates when update is false
because the presence check is skipped; change registerTaskId (and its use of
_taskIds.update) to always avoid adding an existing taskId. Specifically, in
registerTaskId replace the current early-return logic with an atomic check
inside the _taskIds.update lambda: if current.contains(taskId) return current
else return current + taskId (so use current.contains/taskId and _taskIds.update
to decide), ensuring duplicates are never appended and removeTaskId can fully
clear entries.
- Around line 49-53: The localization keys used in buildNotification calls are
incorrect; update the string keys passed to buildNotification (and any direct
Localization.get usages) from "network.requests.starting" /
"network.requests.running" to the correct keys
"network.notification.service.starting" and
"network.notification.service.running" so they match
android_translatable_strings.txt—search for buildNotification(...) and any
Localization.get(...) in NetworkNotificationService (and usages around
startForeground/NON-foreground branches) and replace the keys accordingly.
---
Outside diff comments:
In `@app/src/org/commcare/tasks/templates/CommCareTask.java`:
- Around line 73-88: onCancelled() must notify NetworkNotificationService that
this task is no longer active to avoid a service leak; modify
CommCareTask.onCancelled to send the STOP_NOTIFICATION_ACTION intent to
NetworkNotificationService (including the task id from getTaskId()) when a
connector is non-null and the task opted into notifications—do this alongside
the existing
connector.stopBlockingForTask(getTaskId())/connector.taskCancelled() calls
(before or after them) so the service can remove the task and possibly call
stopSelf(); reference the onCancelled method, getConnector(), getTaskId(),
connector.stopBlockingForTask(...) and
NetworkNotificationService.STOP_NOTIFICATION_ACTION when locating where to add
the stop-intent dispatch.
---
Duplicate comments:
In `@app/src/org/commcare/services/NetworkNotificationService.kt`:
- Around line 130-136: The call to stopForeground(STOP_FOREGROUND_REMOVE) in
onDestroy requires API 24 but is currently annotated with `@RequiresApi`; add a
brief explanatory comment inside onDestroy (near stopForeground) stating that
runtime startup is already gated by
CommCareTask.shouldRunNetworkNotificationService() (which prevents this service
from running on API < 34), so this code path is unreachable on older SDKs and a
runtime check is intentionally omitted; reference stopForeground, onDestroy,
`@RequiresApi`, and CommCareTask.shouldRunNetworkNotificationService in the
comment for future maintainers.
---
Nitpick comments:
In `@app/AndroidManifest.xml`:
- Around line 343-347: The NetworkNotificationService declaration
(android:name="org.commcare.services.NetworkNotificationService") lacks an
explicit android:exported attribute; update the <service> element for
NetworkNotificationService to include android:exported="false" to match other
services (e.g., AudioRecordingService) and prevent accidental external exposure
if an intent-filter is added later.
In `@app/src/org/commcare/services/NetworkNotificationService.kt`:
- Line 93: The service currently returns super.onStartCommand(...), which yields
START_STICKY; change NetworkNotificationService.onStartCommand to return
START_NOT_STICKY instead so the system won't restart the service without the
original intent. Locate the onStartCommand method in class
NetworkNotificationService and replace the final return expression (currently
calling super.onStartCommand(intent, flags, startId)) with the Service constant
START_NOT_STICKY; keep existing intent-null checks/early exits as-is so the
method still stops itself when no work is provided.
In `@app/src/org/commcare/tasks/templates/CommCareTask.java`:
- Around line 139-142: shouldRunNetworkNotificationService() gates starting the
service to API 34+, but NetworkNotificationService.onCreate() still contains an
else branch for < UPSIDE_DOWN_CAKE which is dead code; either remove that else
branch and simplify onCreate() to only handle the API >= UPSIDE_DOWN_CAKE path,
or if you intend to support older APIs, update
shouldRunNetworkNotificationService() to allow starting the service on lower API
levels. Locate shouldRunNetworkNotificationService() and
NetworkNotificationService.onCreate() and then either (A) delete the <
UPSIDE_DOWN_CAKE branch and add a brief comment in onCreate() clarifying the
service is only started on API 34+, or (B) change
shouldRunNetworkNotificationService() to not require UPSIDE_DOWN_CAKE so the
onCreate() else branch is reachable — pick the option matching intended
behavior.
| network.notification.service.starting=Starting network service | ||
| network.notification.service.running=Running network requests... | ||
| network.notification.service.downloading.files=Downloading application files... |
There was a problem hiding this comment.
Localization key mismatch — service uses different keys than those defined here.
The translation file defines:
network.notification.service.startingnetwork.notification.service.running
But NetworkNotificationService.kt uses:
"network.requests.starting"(lines 49, 53)"network.requests.running"(line 83)
These keys don't match, so Localization.get() will fail to find the correct text at runtime. Either the keys here or the string literals in the service need to be aligned.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/assets/locales/android_translatable_strings.txt` around lines 1021 -
1023, The localization keys used by NetworkNotificationService.kt
("network.requests.starting" and "network.requests.running") do not match the
keys in the translations file (network.notification.service.starting /
.running); fix this by making them identical: either rename the keys in the
translation file to "network.requests.starting" and "network.requests.running"
or change the string literals in NetworkNotificationService.kt to
"network.notification.service.starting" and
"network.notification.service.running" so that Localization.get() can resolve
the texts (update the three symbols: the two keys in the translations and the
occurrences in NetworkNotificationService.kt at the places using those
literals).
| buildNotification("network.requests.starting"), | ||
| ServiceInfo.FOREGROUND_SERVICE_TYPE_DATA_SYNC, | ||
| ) | ||
| } else { | ||
| startForeground(NETWORK_NOTIFICATION_ID, buildNotification("network.requests.starting")) |
There was a problem hiding this comment.
Localization keys in this service don't match the keys defined in android_translatable_strings.txt.
The code uses "network.requests.starting" (line 49/53) and "network.requests.running" (line 83), but the translation file defines "network.notification.service.starting" and "network.notification.service.running". This will cause Localization.get() to fail or return the raw key string at runtime.
Suggested fix
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.UPSIDE_DOWN_CAKE) {
startForeground(
NETWORK_NOTIFICATION_ID,
- buildNotification("network.requests.starting"),
+ buildNotification("network.notification.service.starting"),
ServiceInfo.FOREGROUND_SERVICE_TYPE_DATA_SYNC,
)
} else {
- startForeground(NETWORK_NOTIFICATION_ID, buildNotification("network.requests.starting"))
+ startForeground(NETWORK_NOTIFICATION_ID, buildNotification("network.notification.service.starting"))
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/services/NetworkNotificationService.kt` around lines 49
- 53, The localization keys used in buildNotification calls are incorrect;
update the string keys passed to buildNotification (and any direct
Localization.get usages) from "network.requests.starting" /
"network.requests.running" to the correct keys
"network.notification.service.starting" and
"network.notification.service.running" so they match
android_translatable_strings.txt—search for buildNotification(...) and any
Localization.get(...) in NetworkNotificationService (and usages around
startForeground/NON-foreground branches) and replace the keys accordingly.
| notificationManager.notify( | ||
| NETWORK_NOTIFICATION_ID, | ||
| buildNotification( | ||
| intent.getStringExtra(PROGRESS_TEXT_KEY_INTENT_EXTRA) ?: "network.requests.running", | ||
| ), | ||
| ) |
There was a problem hiding this comment.
Same localization key mismatch for the fallback key.
"network.requests.running" should be "network.notification.service.running" to match the translation file.
Suggested fix
notificationManager.notify(
NETWORK_NOTIFICATION_ID,
buildNotification(
- intent.getStringExtra(PROGRESS_TEXT_KEY_INTENT_EXTRA) ?: "network.requests.running",
+ intent.getStringExtra(PROGRESS_TEXT_KEY_INTENT_EXTRA) ?: "network.notification.service.running",
),
)📝 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.
| notificationManager.notify( | |
| NETWORK_NOTIFICATION_ID, | |
| buildNotification( | |
| intent.getStringExtra(PROGRESS_TEXT_KEY_INTENT_EXTRA) ?: "network.requests.running", | |
| ), | |
| ) | |
| notificationManager.notify( | |
| NETWORK_NOTIFICATION_ID, | |
| buildNotification( | |
| intent.getStringExtra(PROGRESS_TEXT_KEY_INTENT_EXTRA) ?: "network.notification.service.running", | |
| ), | |
| ) |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/services/NetworkNotificationService.kt` around lines 80
- 85, The fallback localization key used when building the notification is
incorrect: in NetworkNotificationService (inside the notificationManager.notify
call that calls buildNotification), replace the hard-coded fallback string
"network.requests.running" with the correct key
"network.notification.service.running" where the intent extra
PROGRESS_TEXT_KEY_INTENT_EXTRA is read, so buildNotification(...) receives the
proper default localization key (keep the existing null-coalescing logic).
| private fun registerTaskId( | ||
| taskId: Int, | ||
| update: Boolean, | ||
| ) { | ||
| if (taskId == -1 || (update && taskId in _taskIds.value)) return | ||
|
|
||
| _taskIds.update { current -> current + taskId } | ||
| } |
There was a problem hiding this comment.
registerTaskId allows duplicate entries when update is false.
When called from START_NOTIFICATION_ACTION (update = false), the duplicate check is skipped, so the same taskId can be appended multiple times to the list. Each duplicate requires a corresponding removeTaskId call to fully clear it (since List.minus only removes the first occurrence). This could prevent the service from stopping.
Suggested fix — always prevent duplicates
private fun registerTaskId(
taskId: Int,
update: Boolean,
) {
- if (taskId == -1 || (update && taskId in _taskIds.value)) return
+ if (taskId == -1 || taskId in _taskIds.value) return
_taskIds.update { current -> current + taskId }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@app/src/org/commcare/services/NetworkNotificationService.kt` around lines 101
- 108, registerTaskId can add duplicates when update is false because the
presence check is skipped; change registerTaskId (and its use of
_taskIds.update) to always avoid adding an existing taskId. Specifically, in
registerTaskId replace the current early-return logic with an atomic check
inside the _taskIds.update lambda: if current.contains(taskId) return current
else return current + taskId (so use current.contains/taskId and _taskIds.update
to decide), ensuring duplicates are never appended and removeTaskId can fully
clear entries.
There was a problem hiding this comment.
This is expected, as we need to cater for legitimate duplicates, considering scenarios where multiple instances of the same AsyncTask may run at the same time.
There was a problem hiding this comment.
@avazirna, thank you for the clarification! That makes sense—tracking each running instance separately (even when they share the same task ID) ensures that the service only stops once all concurrent instances have completed. The List.minus operation removing one occurrence at a time aligns perfectly with this design.
🧠 Learnings used
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.
3 participants
Product Description
This PR addresses an issue where network requests triggered by
AsyncTasksare interrupted when CommCare goes to the background on devices running Android 15 and higher. This behavior is due the background network access restrictions introduced in that version. As a result, the app installation feature was impacted as demonstrated in the video below:app-installation-issue-on-android15.mp4
Technical Summary
After some tests, it was noticed that the restrictions are not applied when
CommCareSessionServiceis running, and that the affected features are those accessible when the service is not running. Based on this, the chosen approach was to implement a service,NetworkNotificationService, that could run in parallel to those features.It's important to note that this is a temporary solution. The scope of these restrictions seem to be expanding, and as a result this approach mayb be rejected by the Google Play Store. Additionally,
AsyncTaskswere deprecated in Android 11, so the long term goal is to replace them with components align with modern Android development best practices.Feature Flag
Safety Assurance
Safety story
This was successfully tested locally.
QA Plan
I plan to send this to QA for regression testing.
Labels and Review