Add Audio recording foreground service #3057
Conversation
📝 WalkthroughWalkthroughThe changes introduce a dedicated Android foreground service, Sequence Diagram(s)sequenceDiagram
participant User
participant RecordingFragment
participant AudioRecordingService
participant NotificationManager
User->>RecordingFragment: Initiates recording
RecordingFragment->>AudioRecordingService: Start and bind service (with filename)
AudioRecordingService->>NotificationManager: Show "Recording in progress..." notification
AudioRecordingService->>AudioRecordingService: Start MediaRecorder
User->>RecordingFragment: Pause recording
RecordingFragment->>AudioRecordingService: pauseRecording()
AudioRecordingService->>NotificationManager: Update notification to "Recording paused"
AudioRecordingService->>AudioRecordingService: Pause MediaRecorder
User->>RecordingFragment: Resume recording
RecordingFragment->>AudioRecordingService: resumeRecording()
AudioRecordingService->>NotificationManager: Update notification to "Recording in progress..."
AudioRecordingService->>AudioRecordingService: Resume MediaRecorder
User->>RecordingFragment: Stop recording
RecordingFragment->>AudioRecordingService: stopRecording()
AudioRecordingService->>NotificationManager: Remove notification
AudioRecordingService->>AudioRecordingService: Stop MediaRecorder
RecordingFragment->>AudioRecordingService: Unbind and stop service (on view destroy)
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
app/src/org/commcare/views/widgets/AudioRecordingService.java (1)
188-193: Mirror the runtime guard inresumeRecording().
Consistency with the previous comment.
🧹 Nitpick comments (4)
app/AndroidManifest.xml (1)
39-40: Addtools:targetApi="34"to future-API permission to silence lint on pre-34 builds.
android.permission.FOREGROUND_SERVICE_MICROPHONEis introduced in API 34. When thetargetSdkVersionis < 34, some build systems/lint rules flag “unknown permission” warnings. A small annotation keeps CI quiet without affecting runtime behaviour on older devices.- <uses-permission android:name="android.permission.FOREGROUND_SERVICE_MICROPHONE" /> + <uses-permission + android:name="android.permission.FOREGROUND_SERVICE_MICROPHONE" + tools:targetApi="34" />app/src/org/commcare/views/widgets/AudioRecordingService.java (2)
40-50: Service enters foreground before recorder setup – harmless but unnecessary.
startForeground()is invoked inonCreate()even when the caller only meant to bind to the service. A foreground notification the user can’t dismiss appears even if nothing is recording yet.
MovestartForeground()next torecorder.start()insideonStartCommand()after successfulprepare()/start().
181-186:pauseRecording()should no-op on pre-Nougat devices.
Although the method is annotated, a caller might still invoke it conditionally. Add a runtime guard to be safe:if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { recorder.pause(); notificationManager.notify(...); }app/src/org/commcare/views/widgets/RecordingFragment.java (1)
188-196: UseContextCompat.startForegroundService()for API-safe startup
startForegroundService()is only available on API 26+. Using it directly is fine because you guard with>= O, but for consistency and testabilityContextCompat.startForegroundService()works on all API levels and removes the need for the dual branch.Not blocking, just a minor improvement.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
app/AndroidManifest.xml(2 hunks)app/assets/locales/android_translatable_strings.txt(1 hunks)app/src/org/commcare/views/widgets/AudioRecordingService.java(1 hunks)app/src/org/commcare/views/widgets/RecordingFragment.java(11 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Lint Code Base
🔇 Additional comments (1)
app/assets/locales/android_translatable_strings.txt (1)
467-469: ✅ New notification strings look good, but remember they still need translation files.
Keys follow the existing naming scheme and will be picked up automatically. Please add them to the other locale folders (e.g.values-es/…,values-fr/…) before release so users don’t see fallback English text.
| <service | ||
| android:enabled="true" | ||
| android:foregroundServiceType="microphone" | ||
| android:name="org.commcare.views.widgets.AudioRecordingService"> | ||
| </service> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Limit service surface-area & clarify shutdown semantics.
- The service doesn’t need to be discoverable by other apps. Set
android:exported="false"to harden it. - Consider adding
android:stopWithTask="true"so the service doesn’t survive process-death when the task is swiped away (this matches current recording UX).
- <service
- android:enabled="true"
- android:foregroundServiceType="microphone"
- android:name="org.commcare.views.widgets.AudioRecordingService">
- </service>
+ <service
+ android:enabled="true"
+ android:exported="false"
+ android:stopWithTask="true"
+ android:foregroundServiceType="microphone"
+ android:name="org.commcare.views.widgets.AudioRecordingService"/>🤖 Prompt for AI Agents (early access)
In app/AndroidManifest.xml around lines 335 to 339, the AudioRecordingService is missing security and lifecycle attributes. Add android:exported="false" to prevent other apps from accessing this service, and add android:stopWithTask="true" to ensure the service stops when the user swipes away the app task, matching the expected recording behavior.
| @Override | ||
| public int onStartCommand(Intent intent, int flags, int startId) { | ||
| String fileName = intent.getExtras().getString(RECORDING_FILENAME_EXTRA_KEY); | ||
| setupRecorder(fileName); | ||
| recorder.start(); | ||
| return START_NOT_STICKY; | ||
| } |
There was a problem hiding this comment.
NullPointerException risk when intent has no extras.
String fileName = intent.getExtras().getString(RECORDING_FILENAME_EXTRA_KEY);If the bundle or key is missing, the service crashes. Guard this and fail fast:
+ if (intent == null || intent.getExtras() == null
+ || !intent.getExtras().containsKey(RECORDING_FILENAME_EXTRA_KEY)) {
+ stopSelf();
+ return START_NOT_STICKY;
+ }🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/AudioRecordingService.java around lines 52 to 58, the code assumes intent.getExtras() and the key RECORDING_FILENAME_EXTRA_KEY are always present, risking a NullPointerException. Fix this by checking if intent and intent.getExtras() are non-null and if the extras contain the key before accessing it. If any check fails, log an error and stop the service immediately to fail fast and avoid crashes.
| private void setupRecorder(String fileName) { | ||
| if (recorder == null) { | ||
| recorder = new MediaRecorder(); | ||
| } | ||
|
|
||
| boolean isHeAacSupported = isHeAacEncoderSupported(); | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.Q) { | ||
| recorder.setAudioSource(MediaRecorder.AudioSource.VOICE_COMMUNICATION); | ||
| } else { | ||
| recorder.setAudioSource(MediaRecorder.AudioSource.MIC); | ||
| } | ||
|
|
||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.R) { | ||
| recorder.setPrivacySensitive(true); | ||
| } | ||
| recorder.setAudioSamplingRate(isHeAacSupported ? HEAAC_SAMPLE_RATE : AMRNB_SAMPLE_RATE); | ||
| recorder.setOutputFormat(MediaRecorder.OutputFormat.MPEG_4); | ||
| if (isHeAacSupported) { | ||
| recorder.setAudioEncoder(MediaRecorder.AudioEncoder.HE_AAC); | ||
| } else { | ||
| recorder.setAudioEncoder(MediaRecorder.AudioEncoder.AMR_NB); | ||
| } | ||
| recorder.setOutputFile(fileName); | ||
|
|
||
| try { | ||
| recorder.prepare(); | ||
| Logger.log(LogTypes.TYPE_MEDIA_EVENT, "Preparing recording: " + fileName | ||
| + " | " + (isHeAacSupported ? HEAAC_SAMPLE_RATE : AMRNB_SAMPLE_RATE) | ||
| + " | " + (isHeAacSupported ? MediaRecorder.AudioEncoder.HE_AAC : MediaRecorder.AudioEncoder.AMR_NB)); | ||
|
|
||
| } catch (IOException e) { | ||
| e.printStackTrace(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle IllegalStateException from prepare() / start() to avoid crashes.
Both methods can throw when invoked in the wrong state or on unsupported hardware. Wrap them and bubble an error back to the client so the UI can show feedback instead of killing the app.
try {
recorder.prepare();
recorder.start();
} catch (IOException | IllegalStateException e) {
Logger.exception(LogTypes.TYPE_ERROR_WORKFLOW, e);
stopSelf();
}🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/AudioRecordingService.java between lines 105 and 138, the prepare() and start() calls on the MediaRecorder can throw IllegalStateException in addition to IOException, which is currently unhandled and may crash the app. Update the try-catch block to catch both IOException and IllegalStateException, log the exception using Logger.exception with an appropriate error type, and call stopSelf() to safely stop the service and allow the UI to handle the error gracefully.
There was a problem hiding this comment.
The goal here is to let the app crash in case an IllegalStateException is thrown during prepare() or start(), for visibility
There was a problem hiding this comment.
@avazirna I understand your approach. Allowing the app to crash when an IllegalStateException occurs provides high visibility for these issues through crash reports, making them immediately apparent to developers rather than being silently handled. This is a valid debugging strategy that forces addressing the root cause rather than masking it with error handling.
✏️ Learnings added
Learnt from: avazirna
PR: dimagi/commcare-android#3057
File: app/src/org/commcare/views/widgets/AudioRecordingService.java:105-138
Timestamp: 2025-05-01T10:50:29.126Z
Learning: In the CommCare Android app, IllegalStateException from MediaRecorder operations should not be caught to ensure visibility of recording setup issues through app crashes.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| private void resetRecording(){ | ||
| if (recorder != null) { | ||
| recorder.release(); | ||
| recorder = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Recorder never released after stopRecording(), leaks mic & file handle.
resetRecording() releases resources, but it’s only called in onDestroy(). If the UI calls stopRecording() and keeps the service alive (e.g. to allow a re-record), the mic remains locked.
public void stopRecording() {
recorder.stop();
+ resetRecording();
}🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/AudioRecordingService.java around lines 66 to 71, the recorder is only released in resetRecording(), which is called in onDestroy(), causing the microphone and file handle to remain locked after stopRecording() if the service stays alive. To fix this, modify stopRecording() to call resetRecording() after stopping the recorder, ensuring resources are released promptly and the mic is freed for reuse.
| import org.commcare.CommCareNoficationManager; | ||
| import org.commcare.activities.DispatchActivity; | ||
| import org.commcare.dalvik.R; | ||
| import org.commcare.util.LogTypes; | ||
| import org.javarosa.core.services.Logger; | ||
| import org.javarosa.core.services.locale.Localization; | ||
|
|
There was a problem hiding this comment.
Typo in class name breaks the import – build will fail.
CommCareNoficationManager is miss-spelled; the project uses CommCareNotificationManager.
-import org.commcare.CommCareNoficationManager;
+import org.commcare.CommCareNotificationManager;📝 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.
| import org.commcare.CommCareNoficationManager; | |
| import org.commcare.activities.DispatchActivity; | |
| import org.commcare.dalvik.R; | |
| import org.commcare.util.LogTypes; | |
| import org.javarosa.core.services.Logger; | |
| import org.javarosa.core.services.locale.Localization; | |
| import org.commcare.CommCareNotificationManager; | |
| import org.commcare.activities.DispatchActivity; | |
| import org.commcare.dalvik.R; | |
| import org.commcare.util.LogTypes; | |
| import org.javarosa.core.services.Logger; | |
| import org.javarosa.core.services.locale.Localization; |
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/AudioRecordingService.java between lines 17 and 23, the import statement for CommCareNotificationManager is misspelled as CommCareNoficationManager. Correct the import by changing CommCareNoficationManager to CommCareNotificationManager to fix the build error.
| // resume first just in case we were paused | ||
| if (inPausedState) { | ||
| recorder.resume(); | ||
| audioRecordingService.resumeRecording(); | ||
| } | ||
|
|
||
| recorder.stop(); | ||
| audioRecordingService.stopRecording(); | ||
| toggleRecording.setBackgroundResource(R.drawable.play); | ||
| toggleRecording.setOnClickListener(v -> playAudio()); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
All recorder calls assume a non-null service – add null-safety guards
audioRecordingService can be null if the user dismisses the dialog before the ServiceConnection is established, or if the service crashes.
Direct calls (resumeRecording, stopRecording, pauseRecording) without a null check will crash the app with NullPointerException.
- audioRecordingService.resumeRecording();
+ if (audioRecordingService != null) {
+ audioRecordingService.resumeRecording();
+ }
...
- audioRecordingService.stopRecording();
+ if (audioRecordingService != null) {
+ audioRecordingService.stopRecording();
+ }
...
- audioRecordingService.pauseRecording();
+ if (audioRecordingService != null) {
+ audioRecordingService.pauseRecording();
+ }A helper like safeCallService(Consumer<AudioRecordingService>) can reduce repetition.
Also applies to: 268-269, 291-292
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/RecordingFragment.java around lines 248 to 255, the code calls methods on audioRecordingService without checking if it is null, risking NullPointerException if the service is not connected or crashes. Add null-safety guards before calling resumeRecording, stopRecording, and other service methods by verifying audioRecordingService is not null. To reduce repetition, create a helper method like safeCallService that accepts a Consumer<AudioRecordingService> and invokes it only if the service is non-null. Apply this pattern also to lines 268-269 and 291-292 as indicated.
| @Override | ||
| public void onServiceDisconnected(ComponentName name) { | ||
| audioRecordingService = null; | ||
| } | ||
| }; |
There was a problem hiding this comment.
audioRecordingServiceBounded never reset on disconnect – can trigger IllegalArgumentException when un-binding
onServiceDisconnected() nulls the service but leaves audioRecordingServiceBounded true.
If the process later calls unbindService() (e.g. in onDestroyView()), Android will throw IllegalArgumentException: Service not registered because the connection was already severed.
@Override
public void onServiceDisconnected(ComponentName name) {
audioRecordingService = null;
+ audioRecordingServiceBounded = false; // keep the flag in sync with reality
}📝 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.
| @Override | |
| public void onServiceDisconnected(ComponentName name) { | |
| audioRecordingService = null; | |
| } | |
| }; | |
| @Override | |
| public void onServiceDisconnected(ComponentName name) { | |
| audioRecordingService = null; | |
| audioRecordingServiceBounded = false; // keep the flag in sync with reality | |
| } | |
| }; |
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/RecordingFragment.java around lines 219 to 223, the onServiceDisconnected() method sets audioRecordingService to null but does not reset audioRecordingServiceBounded to false. This causes an IllegalArgumentException when unbinding the service later because the system thinks the service is still bound. To fix this, update onServiceDisconnected() to also set audioRecordingServiceBounded to false, ensuring the bound state accurately reflects the service connection status.
| @Override | ||
| public void onDestroyView() { | ||
| super.onDestroyView(); | ||
| unbindAudioRecordingService(); | ||
| } | ||
|
|
||
| private void unbindAudioRecordingService() { | ||
| if (audioRecordingServiceBounded) { | ||
| audioRecordingServiceBounded = false; | ||
|
|
||
| requireActivity().unbindService(audioRecordingServiceConnection); | ||
| requireActivity() | ||
| .stopService(new Intent(requireActivity(), AudioRecordingService.class)); | ||
| } | ||
| } |
There was a problem hiding this comment.
Callback never unregistered → memory-leak & stray notifications
registerAudioRecordingConfigurationChangeCallback() adds a process-wide listener, but unregisterAudioRecordingConfigurationChangeCallback() is not invoked in
onDestroyView().
If the user simply closes the dialog (without saving) the callback—and hence the fragment instance—remains referenced inside AudioManager, leaking the activity and producing unexpected notifications.
@Override
public void onDestroyView() {
super.onDestroyView();
+ if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
+ unregisterAudioRecordingConfigurationChangeCallback();
+ }
unbindAudioRecordingService();
}Also consider calling unregister…() inside unbindAudioRecordingService() as a safety net.
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/RecordingFragment.java around lines 466 to 480, the callback registered via registerAudioRecordingConfigurationChangeCallback() is not unregistered in onDestroyView(), causing a memory leak and stray notifications. To fix this, add a call to unregisterAudioRecordingConfigurationChangeCallback() inside onDestroyView() to properly remove the listener. Additionally, include the unregisterAudioRecordingConfigurationChangeCallback() call within unbindAudioRecordingService() as a safety measure to ensure the callback is always unregistered when the service is unbound.
|
@avazirna this needs to have videos/screenshots around the UX around foreground service before we can begin review. |
On it, but overall the UX is the same as before. |
|
@shubham1g5 I've added a video of the audio recording with the foreground notification. |
|
@avazirna Thanks. One other question, What happens when you dismiss the notification ? Does the service gets killed ? If so, what's the behavior on resuming the recording screen ? |
|
|
||
| @Override | ||
| public void onDestroy() { | ||
| resetRecording(); |
There was a problem hiding this comment.
nit: rename to resetRecorder as resetRecording seems like we are resetting the recorded file itself
| } | ||
| } | ||
|
|
||
| private void setupRecorder(String fileName) { |
There was a problem hiding this comment.
Can we abstract this into another RecordingHelper class so that we don't have to move this around next time there is a need to access this code from another Android component.
The service continues running but not treated with the same priority by the system, so more susceptible to being killed if the device starts experiencing resource contraints. |
that seems a bit weird to me that the service doesn't get kiiled on dismissing notification. Does it matter if the app is in background or foreground ? Also what is the behaviour if you dismiss the notification and resume the app from background to foreground to the audio question ? Does the notification appear again for user ? |
Same with |
This is not valid for Android 14+, where the concept of dismissing notification is not supported except for a few use cases: https://developer.android.com/about/versions/14/behavior-changes-all#non-dismissable-notifications
No, it doesn't but the service continues running. |
|
@damagatchi retest this please? |
|
Moved to #3100 |
Product Description
https://dimagi.atlassian.net/browse/SAAS-16641
This PR adds a bounded foreground service to the Audio recording feature. This service will ensure that the recording will continue when the device enters sleep or doze mode.
Users will now see a notification when the recording is in progress and another when it gets paused:
audio_recording_service.webm
Safety Assurance
Safety story
Successfully tested locally.
QA Plan
I've requested QA to run regression tests around this feature.
QA ticket: https://dimagi.atlassian.net/browse/QA-7692
Labels and Review