Add audio recording foreground service #3100
Conversation
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
📝 WalkthroughWalkthroughThe changes introduce a new audio recording feature using an Android foreground service. A new Sequence Diagram(s)sequenceDiagram
participant User
participant RecordingFragment
participant AudioRecordingService
participant AudioRecordingHelper
participant MediaRecorder
User->>RecordingFragment: Initiate audio recording
RecordingFragment->>AudioRecordingService: Start and bind service (with file name)
AudioRecordingService->>AudioRecordingHelper: setupRecorder(fileName)
AudioRecordingHelper->>MediaRecorder: Configure and prepare
AudioRecordingService->>MediaRecorder: Start recording
AudioRecordingService->>RecordingFragment: Expose control methods (pause, resume, stop)
RecordingFragment->>AudioRecordingService: Pause/Resume/Stop via binder
AudioRecordingService->>MediaRecorder: Pause/Resume/Stop
AudioRecordingService->>User: Show notification (recording status)
RecordingFragment->>AudioRecordingService: Unbind/stop service on destroy
AudioRecordingService->>MediaRecorder: Release resources
Suggested labels
Suggested reviewers
Tip ⚡️ Faster reviews with caching
Enjoy the performance boost—your workflow just got faster. ✨ 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:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. 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: 7
🧹 Nitpick comments (4)
app/assets/locales/android_translatable_strings.txt (1)
45-46: Consider updating obsolete marketplace reference
"Android market"has been deprecated for many years and may confuse translators and users. Replacing it with “Google Play Store” (or a more generic “app store”) would keep the copy current and avoid localisation questions.app/src/org/commcare/views/widgets/AudioRecordingHelper.java (1)
54-81: Minor performance nit – cache codec scan
isHeAacEncoderSupported()scans every codec each time a recording starts. Caching the result in a static field avoids repeated reflection-heavy work:-private boolean isHeAacEncoderSupported() { +private static Boolean heAacSupported = null; +private boolean isHeAacEncoderSupported() { + if (heAacSupported != null) { + return heAacSupported; + } int numCodecs = MediaCodecList.getCodecCount(); @@ - return false; + heAacSupported = false; + return false; }app/src/org/commcare/views/widgets/AudioRecordingService.java (1)
122-134: Guard pause/resume with an API check inside the methodEven if callers respect the
Build.VERSION.SDK_INTguard, defensive coding prevents accidental crashes:if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N && isRecorderActive()) { recorder.pause(); ... }app/src/org/commcare/views/widgets/RecordingFragment.java (1)
202-227:ServiceConnectionretains the Fragment – consider a static holder +WeakReference
ServiceConnectionis an anonymous inner class; it holds an implicit reference to the outerRecordingFragment.
If, for any reason, the service lives longer than the fragment’s view ( e.g. configuration change ), the fragment instance can’t be garbage-collected, leaking the entire view hierarchy.Creating a small static inner class avoids that:
-private ServiceConnection getAudioRecordingServiceConnection() { - return audioRecordingServiceConnection = new ServiceConnection() { +private ServiceConnection getAudioRecordingServiceConnection() { + class RecorderConn implements ServiceConnection { + private final WeakReference<RecordingFragment> fragRef; + RecorderConn(RecordingFragment f) { this.fragRef = new WeakReference<>(f); } + + @Override + public void onServiceConnected(ComponentName name, IBinder binder) { + RecordingFragment frag = fragRef.get(); + if (frag == null) { return; } + ... + } + + @Override + public void onServiceDisconnected(ComponentName name) { + RecordingFragment frag = fragRef.get(); + if (frag == null) { return; } + ... + } + } + return audioRecordingServiceConnection = new RecorderConn(this); }This eliminates the strong reference chain and keeps the fragment eligible for GC once it’s detached.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
app/AndroidManifest.xml(2 hunks)app/assets/locales/android_translatable_strings.txt(7 hunks)app/src/org/commcare/views/widgets/AudioRecordingHelper.java(1 hunks)app/src/org/commcare/views/widgets/AudioRecordingService.java(1 hunks)app/src/org/commcare/views/widgets/RecordingFragment.java(11 hunks)
🔇 Additional comments (4)
app/assets/locales/android_translatable_strings.txt (1)
467-469: Good: notification strings addedThe three new strings follow the existing naming pattern and will be picked up automatically by
Localization.get(...). 👍app/AndroidManifest.xml (2)
39-40: Permission may be redundant on pre-34 devices
android.permission.FOREGROUND_SERVICE_MICROPHONEis only recognised from API 34 (Android 14). On older versions it’s silently ignored, so shipping it is fine, but you might want to wrap it with atools:targetApi="34"attribute to silence aapt / Lint warnings when building with a lowertargetSdk.
335-341: Lifecycle & visibility flags look good, but you’re missing a required permissionTo run a foreground‐microphone service on Android 10+ you must request
FOREGROUND_SERVICE(already present) andRECORD_AUDIO(also present). ✅
No issues detected with the manifest entry itself.app/src/org/commcare/views/widgets/AudioRecordingService.java (1)
84-90: Typo in channel helper class
CommCareNoficationManagerlooks misspelled (missing “ti”). If the class really isCommCareNotificationManager, this will be a compile-time error.
| 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.
Surface prepare() failures to the caller
If recorder.prepare() throws, the exception is swallowed and the (un-prepared) recorder is returned. A later start() call will crash with IllegalStateException.
try {
recorder.prepare();
@@
} catch (IOException e) {
- e.printStackTrace();
-}
-return recorder;
+ recorder.release();
+ throw new IllegalStateException("Failed to prepare MediaRecorder", e);
+}
+return recorder;Escalating the error early prevents a half-initialised recorder from propagating.
📝 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.
| 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(); | |
| } | |
| 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) { | |
| recorder.release(); | |
| throw new IllegalStateException("Failed to prepare MediaRecorder", e); | |
| } | |
| return recorder; |
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/AudioRecordingHelper.java around lines 41
to 50, the IOException thrown by recorder.prepare() is caught and only logged,
allowing the method to return an unprepared recorder which causes a crash later.
Modify the code to rethrow the IOException after logging it, so the caller is
made aware of the failure and can handle it appropriately, preventing
propagation of a half-initialized recorder.
| recorder.start(); | ||
| return START_NOT_STICKY; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Handle start() failure
recorder.start() throws RuntimeException if the mic is busy or preparation failed. Wrap it to avoid process crashes and inform the UI.
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/AudioRecordingService.java around lines 56
to 58, the call to recorder.start() can throw a RuntimeException if the
microphone is busy or preparation failed. To fix this, wrap the recorder.start()
call in a try-catch block that catches RuntimeException, handle the exception
gracefully by logging the error and notifying the UI about the failure, and
ensure the service does not crash by returning an appropriate status.
| String fileName = intent.getExtras().getString(RECORDING_FILENAME_EXTRA_KEY); | ||
| if (recorder == null) { | ||
| recorder = audioRecordingHelper.setupRecorder(fileName); | ||
| } | ||
| recorder.start(); |
There was a problem hiding this comment.
Null-safety for intent extras
intent.getExtras() can be null, leading to an NPE on devices that restart the service after being killed. Guard against this:
-String fileName = intent.getExtras().getString(RECORDING_FILENAME_EXTRA_KEY);
+Bundle extras = intent.getExtras();
+if (extras == null || !extras.containsKey(RECORDING_FILENAME_EXTRA_KEY)) {
+ stopSelf();
+ return START_NOT_STICKY;
+}
+String fileName = extras.getString(RECORDING_FILENAME_EXTRA_KEY);📝 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.
| String fileName = intent.getExtras().getString(RECORDING_FILENAME_EXTRA_KEY); | |
| if (recorder == null) { | |
| recorder = audioRecordingHelper.setupRecorder(fileName); | |
| } | |
| recorder.start(); | |
| Bundle extras = intent.getExtras(); | |
| if (extras == null || !extras.containsKey(RECORDING_FILENAME_EXTRA_KEY)) { | |
| stopSelf(); | |
| return START_NOT_STICKY; | |
| } | |
| String fileName = extras.getString(RECORDING_FILENAME_EXTRA_KEY); | |
| if (recorder == null) { | |
| recorder = audioRecordingHelper.setupRecorder(fileName); | |
| } | |
| recorder.start(); |
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/AudioRecordingService.java around lines 52
to 56, the code calls intent.getExtras() without checking for null, which can
cause a NullPointerException if the service is restarted and the intent extras
are missing. To fix this, add a null check for intent.getExtras() before
accessing getString(RECORDING_FILENAME_EXTRA_KEY). If extras are null, handle
this case gracefully, for example by not proceeding with recorder setup or by
providing a default filename.
| this.stopForeground(true); | ||
| } | ||
|
|
||
| private void resetRecorder() { | ||
| if (recorder != null) { | ||
| recorder.release(); | ||
| recorder = null; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Release recorder after stopRecording()
stopRecording() stops but never releases resources; only onDestroy() frees them. If the fragment stops recording but keeps the service alive (e.g., during rotation) the mic remains locked.
public void stopRecording() {
recorder.stop();
+ resetRecorder();
}📝 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.
| this.stopForeground(true); | |
| } | |
| private void resetRecorder() { | |
| if (recorder != null) { | |
| recorder.release(); | |
| recorder = null; | |
| } | |
| } | |
| public void stopRecording() { | |
| recorder.stop(); | |
| resetRecorder(); | |
| } |
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/AudioRecordingService.java around lines 63
to 71, the stopRecording() method stops the recorder but does not release its
resources, causing the microphone to remain locked if the service stays alive.
Modify stopRecording() to call resetRecorder() after stopping the recorder to
release resources immediately and prevent the mic from being locked
unnecessarily.
| // resume first just in case we were paused | ||
| if (inPausedState) { | ||
| recorder.resume(); | ||
| audioRecordingService.resumeRecording(); | ||
| } | ||
|
|
||
| recorder.stop(); | ||
| audioRecordingService.stopRecording(); | ||
| toggleRecording.setBackgroundResource(R.drawable.play); |
There was a problem hiding this comment.
Possible NullPointerException when the service dies or is not yet bound
audioRecordingService is dereferenced in several places without a null check.
If the service crashes, is killed for resources, or the bind fails, these calls will crash the UI thread.
-if (inPausedState) {
- audioRecordingService.resumeRecording();
-}
-...
-audioRecordingService.stopRecording();
+if (inPausedState && audioRecordingService != null) {
+ audioRecordingService.resumeRecording();
+}
+...
+if (audioRecordingService != null) {
+ audioRecordingService.stopRecording();
+}Apply the same guard in pauseRecording() and resumeRecording():
-audioRecordingService.pauseRecording();
+if (audioRecordingService != null) {
+ audioRecordingService.pauseRecording();
+}
...
-audioRecordingService.resumeRecording();
+if (audioRecordingService != null) {
+ audioRecordingService.resumeRecording();
+}Fail-safe checks prevent user-visible crashes and make the UI resilient to service lifecycle surprises.
Also applies to: 271-272, 294-295
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/RecordingFragment.java around lines 251 to
257, 271 to 272, and 294 to 295, the audioRecordingService is accessed without
null checks, risking NullPointerExceptions if the service is not bound or has
died. Add null checks before calling any methods on audioRecordingService in
these sections, including in pauseRecording() and resumeRecording(), to ensure
the UI thread does not crash and handles service lifecycle issues gracefully.
| Intent serviceIntent = new Intent(requireActivity(), AudioRecordingService.class); | ||
| serviceIntent.putExtra(RECORDING_FILENAME_EXTRA_KEY, fileName); | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | ||
| requireActivity().startForegroundService(serviceIntent); | ||
| } else { | ||
| requireActivity().startService(serviceIntent); | ||
| } | ||
| requireActivity().bindService(serviceIntent, getAudioRecordingServiceConnection(), | ||
| Context.BIND_AUTO_CREATE); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use application context when starting / binding the service to avoid activity-lifecycle leaks
requireActivity() ties the service start & bind to the current activity instance.
If that activity is destroyed (e.g., by the system, or if another task brings a new activity to the foreground) while recording is still running, the reference becomes invalid and may crash or leak.
-Intent serviceIntent = new Intent(requireActivity(), AudioRecordingService.class);
+Context appCtx = requireContext().getApplicationContext();
+Intent serviceIntent = new Intent(appCtx, AudioRecordingService.class);
[...]
-if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
- requireActivity().startForegroundService(serviceIntent);
-} else {
- requireActivity().startService(serviceIntent);
-}
-requireActivity().bindService(serviceIntent, getAudioRecordingServiceConnection(),
- Context.BIND_AUTO_CREATE);
+if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) {
+ ContextCompat.startForegroundService(appCtx, serviceIntent);
+} else {
+ appCtx.startService(serviceIntent);
+}
+appCtx.bindService(serviceIntent, getAudioRecordingServiceConnection(),
+ Context.BIND_AUTO_CREATE);This keeps the service alive independently of the activity and prevents accidental IllegalStateException when the fragment lives longer than its host activity.
📝 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.
| Intent serviceIntent = new Intent(requireActivity(), AudioRecordingService.class); | |
| serviceIntent.putExtra(RECORDING_FILENAME_EXTRA_KEY, fileName); | |
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | |
| requireActivity().startForegroundService(serviceIntent); | |
| } else { | |
| requireActivity().startService(serviceIntent); | |
| } | |
| requireActivity().bindService(serviceIntent, getAudioRecordingServiceConnection(), | |
| Context.BIND_AUTO_CREATE); | |
| Context appCtx = requireContext().getApplicationContext(); | |
| Intent serviceIntent = new Intent(appCtx, AudioRecordingService.class); | |
| serviceIntent.putExtra(RECORDING_FILENAME_EXTRA_KEY, fileName); | |
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { | |
| ContextCompat.startForegroundService(appCtx, serviceIntent); | |
| } else { | |
| appCtx.startService(serviceIntent); | |
| } | |
| appCtx.bindService( | |
| serviceIntent, | |
| getAudioRecordingServiceConnection(), | |
| Context.BIND_AUTO_CREATE | |
| ); |
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/RecordingFragment.java around lines 189 to
197, replace all uses of requireActivity() when starting and binding the
AudioRecordingService with requireActivity().getApplicationContext() to use the
application context instead of the activity context. This change prevents
lifecycle-related leaks and crashes by decoupling the service lifecycle from the
current activity instance.
| private void unbindAudioRecordingService() { | ||
| if (audioRecordingServiceBounded) { | ||
| requireActivity().unbindService(audioRecordingServiceConnection); | ||
| requireActivity() | ||
| .stopService(new Intent(requireActivity(), AudioRecordingService.class)); | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
audioRecordingServiceBounded never reset on manual unbind – can lead to repeated-unbind crash
onServiceDisconnected() is not invoked when you unbind the service; therefore the flag remains true after unbindService, causing a second call (e.g. on another lifecycle event) to throw IllegalArgumentException.
if (audioRecordingServiceBounded) {
requireActivity().unbindService(audioRecordingServiceConnection);
requireActivity()
.stopService(new Intent(requireActivity(), AudioRecordingService.class));
+ audioRecordingServiceBounded = false;
+ audioRecordingService = null;
}Explicitly resetting the flag keeps the state consistent 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.
| private void unbindAudioRecordingService() { | |
| if (audioRecordingServiceBounded) { | |
| requireActivity().unbindService(audioRecordingServiceConnection); | |
| requireActivity() | |
| .stopService(new Intent(requireActivity(), AudioRecordingService.class)); | |
| } | |
| } | |
| private void unbindAudioRecordingService() { | |
| if (audioRecordingServiceBounded) { | |
| requireActivity().unbindService(audioRecordingServiceConnection); | |
| requireActivity() | |
| .stopService(new Intent(requireActivity(), AudioRecordingService.class)); | |
| audioRecordingServiceBounded = false; | |
| audioRecordingService = null; | |
| } | |
| } |
🤖 Prompt for AI Agents (early access)
In app/src/org/commcare/views/widgets/RecordingFragment.java around lines 478 to
484, the boolean flag audioRecordingServiceBounded is not reset to false after
unbinding the service manually, which can cause repeated unbind calls and lead
to IllegalArgumentException. To fix this, explicitly set
audioRecordingServiceBounded to false immediately after calling unbindService to
keep the flag consistent with the actual service binding state.
|
@damagatchi retest this please |
4 similar comments
|
@damagatchi retest this please |
|
@damagatchi retest this please |
|
@damagatchi retest this please |
|
@damagatchi retest this please |
avazirna
force-pushed
the
add-audio-recording-service-hotfix
branch
from
391d0dc to
0e9b2f3
Compare
|
@damagatchi retest this please |
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
cross-request: dimagi/commcare-core#1477