HotFix: improvements in audio recording using service - #3
Conversation
|
@codex review this PR |
|
To use Codex here, create a Codex account and connect to github. |
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the voice recorder service initialization and state management by simplifying the service connection lifecycle and moving recording options from service startup to recording start.
Key changes:
- Replaced
ServiceStateenum with a simpleisConnectedboolean flag - Moved
RecordOptionsinitialization from service start to recording start - Refactored the service start and recording start logic into separate, clearer methods
Reviewed Changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| VoiceRecorderService.java | Removed RecordOptions field and moved options initialization to startRecording() method with added error handling |
| VoiceRecorder.java | Replaced ServiceState enum with isConnected boolean, refactored recording logic into helper methods, and improved error handling during recording start |
| ServiceState.java | Deleted unused ServiceState enum class |
| .prettierignore | Added build/ directory to prettier ignore list |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (isConnected) { | ||
| try { | ||
| recorderService.stopRecording(); | ||
| } catch (Exception e) { | ||
| Log.d("VoiceRecorder", "Forcing stop recording.", e); | ||
| } | ||
| startRecordingByCall(call); | ||
| return; | ||
| } |
There was a problem hiding this comment.
The logic silently stops an existing recording and starts a new one when the service is already connected. This differs from the previous behavior where ALREADY_RECORDING was rejected. This is a significant behavioral change that removes the ability for clients to detect duplicate recording attempts. Consider either maintaining the previous behavior by checking if mediaRecorder is not null and rejecting with ALREADY_RECORDING, or document this new auto-restart behavior clearly.
| } | ||
| try { | ||
| context.unbindService(connection); | ||
| isConnected = false; |
There was a problem hiding this comment.
Setting isConnected = false manually after unbinding is redundant and could cause race conditions. The onServiceDisconnected() callback at line 54 already sets isConnected = false. If the service disconnects asynchronously while this code runs, there could be conflicting state updates. Remove this manual flag update and rely solely on the callback.
| isConnected = false; |
|
@codex review this PR |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (isConnected) { | ||
| try { | ||
| recorderService.stopRecording(); | ||
| } catch (Exception e) { | ||
| Log.d("VoiceRecorder", "Forcing stop recording.", e); | ||
| } | ||
| startRecordingByCall(call); | ||
| return; |
There was a problem hiding this comment.
When startRecording now sees an existing service connection it immediately calls recorderService.startRecording without starting the service via startForegroundService. Because load() binds the service at startup, the first recording request runs while the service has never entered foreground mode and VoiceRecorderService.onStartCommand is never invoked to display the notification. On Android 8+ this violates the requirement that long‑running audio recording services run in the foreground and risks the OS killing the recorder as soon as the app is backgrounded. The previous implementation always called startService(...) before starting to record, so this change regresses the initial recording session.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
No método load somente é feito o bind se o serviço existir, ele não cria o serviço se ele não existir.
|
@coderabbitai review this pr |
|
@ModestinoAndre: I'll review this PR for you right away. ✅ Actions performedReview triggered.
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughRemoves the Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant VoiceRecorder
participant VoiceRecorderService
participant Binder
participant CustomMediaRecorder
User->>VoiceRecorder: startRecording(call)
alt isConnected (fast path)
VoiceRecorder->>VoiceRecorder: best-effort stop current recording
VoiceRecorder->>VoiceRecorder: startRecordingByCall(call)
VoiceRecorder->>VoiceRecorderService: startRecording(dir, subdir)
VoiceRecorderService->>CustomMediaRecorder: prepare/start
else not connected (init path)
VoiceRecorder->>VoiceRecorder: startServiceByCall(call) (stores startCall)
VoiceRecorder->>VoiceRecorderService: startService / startForegroundService
VoiceRecorder->>Binder: bindService (async)
VoiceRecorderService-->>Binder: onBind()
Binder-->>VoiceRecorder: onServiceConnected
VoiceRecorder->>VoiceRecorder: isConnected = true
alt startCall present
VoiceRecorder->>VoiceRecorder: startRecordingByCall(startCall)
VoiceRecorder->>VoiceRecorderService: startRecording(dir, subdir)
VoiceRecorderService->>CustomMediaRecorder: prepare/start
end
end
User->>VoiceRecorder: stopRecording(call)
alt isConnected
VoiceRecorder->>VoiceRecorderService: stopRecording()
VoiceRecorderService->>CustomMediaRecorder: getErrorInfo()
alt error detected
VoiceRecorderService-->>VoiceRecorder: throw MessagesException (RUNTIME_FAILED)
else no error
VoiceRecorderService->>CustomMediaRecorder: stop()
VoiceRecorder->>Binder: unbindService
VoiceRecorder->>VoiceRecorder: isConnected = false
end
else not connected
VoiceRecorder-->>User: reject
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45–75 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
@ModestinoAndre: I'll review this PR for you right away. ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (1)
118-126: Document or reconsider the silent restart behavior.This code silently stops any existing recording and starts a new one when
startRecordingis called while the service is already connected. This differs from previous behavior where duplicate recording attempts were rejected withALREADY_RECORDING. This behavioral change could surprise callers and prevent them from detecting duplicate attempts.Consider either:
- Restoring the previous behavior by checking if
mediaRecorderis not null and rejecting withALREADY_RECORDING, or- Clearly documenting this auto-restart behavior in the API documentation.
🧹 Nitpick comments (3)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorderService.java (1)
56-65: Consider more specific exception handling.The catch-all
Exceptionblock might hide specific errors that could be handled differently. Consider catchingIOExceptionseparately from other exceptions to provide more precise error messages to callers.Apply this diff for more granular error handling:
public void startRecording(String directory, String subDirectory) throws MessagesException { try { RecordOptions options = new RecordOptions(directory, subDirectory); mediaRecorder = new CustomMediaRecorder(getApplicationContext(), options); mediaRecorder.startRecording(); - } catch (Exception exp) { + } catch (IOException exp) { mediaRecorder = null; throw new MessagesException(Messages.FAILED_TO_RECORD, exp); + } catch (Exception exp) { + mediaRecorder = null; + throw new MessagesException(Messages.FAILED_TO_RECORD, exp); } }android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (2)
119-123: Verify stop success before restarting recording.When force-stopping an existing recording, exceptions are caught and logged but ignored. If the stop fails and the service is in a bad state, proceeding with
startRecordingByCallmight encounter issues. Consider checking the service state or at minimum ensuringmediaRecorderis null before starting a new recording.Apply this diff to verify the stop was successful:
if (isConnected) { try { recorderService.stopRecording(); } catch (Exception e) { Log.d("VoiceRecorder", "Forcing stop recording.", e); } + // Verify mediaRecorder was cleaned up before starting new recording + if (recorderService.getMediaRecorder() != null) { + call.reject(Messages.FAILED_TO_RECORD, "Failed to stop existing recording"); + return; + } startRecordingByCall(call); return; }
150-167: Consider removing redundant service binding.Line 166 posts a
bindServicecall to the main looper, butload()(line 70) already binds to the service with the same connection. MultiplebindServicecalls with the sameServiceConnectionare safe but redundant. The binding fromload()should be sufficient once the service is started.If the posted binding is intentional (perhaps to ensure it occurs after the service fully starts), please add a comment explaining why. Otherwise, consider removing line 166.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
.prettierignore(1 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/ServiceState.java(0 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java(2 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorderService.java(1 hunks)
💤 Files with no reviewable changes (1)
- android/src/main/java/com/tchvu3/capacitorvoicerecorder/ServiceState.java
🔇 Additional comments (7)
.prettierignore (1)
1-2: LGTM!Standard practice to exclude build artifacts from Prettier formatting.
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (6)
36-36: LGTM!Simplifying from enum-based state to a boolean flag reduces complexity while maintaining clear service connection status.
41-50: LGTM!The extraction of recording logic into
startRecordingByCallimproves code organization and readability.
52-57: LGTM!Proper state cleanup when service connection is lost.
59-64: LGTM!Correctly checks connection status using the new
isConnectedflag.
139-148: LGTM!The helper method cleanly separates parameter extraction and service call, improving code organization.
170-193: LGTM!The stopRecording flow correctly:
- Checks connection status
- Delegates to service for recording stop
- Cleans up service and connection state
Note: The manual setting of
isConnected = falseat line 189 is necessary and correct. TheonServiceDisconnectedcallback is only invoked when the service crashes or is killed, NOT when explicitly unbound. The previous review suggesting this was redundant was incorrect.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorderService.java (1)
72-103: Ensure stopRecording still releases resources on runtime errors
mediaRecorder.getErrorInfo()is inspected before thetry/finally, so as soon as it returns a value (which the new listener does) the method throws without ever callingstopRecording(), releasing the recorder, or deleting the temp file. The service is left holding on to a liveMediaRecorder, and every subsequent call fails. Please capture the error first, run the normal stop/cleanup, and only then surface the runtime failure to the caller.- if (mediaRecorder.getErrorInfo() != null) { - throw new MessagesException(Messages.RUNTIME_FAILED + " error info: " + mediaRecorder.getErrorInfo()); - } + ErrorInfo errorInfo = mediaRecorder.getErrorInfo(); @@ - if ((recordDataBase64 == null && path == null) || recordData.getMsDuration() < 0) { - throw new MessagesException(Messages.EMPTY_RECORDING); + if (errorInfo != null) { + throw new MessagesException(Messages.RUNTIME_FAILED + " error info: " + errorInfo); + } else if ((recordDataBase64 == null && path == null) || recordData.getMsDuration() < 0) { + throw new MessagesException(Messages.EMPTY_RECORDING);
♻️ Duplicate comments (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (1)
118-126: Do not silently discard an in-flight recordingCalling
recorderService.stopRecording()here drops the active session on the floor: the service deletes/returns the recorded data, but the plugin neither surfaces it nor lets the caller know anything happened. Previously this path rejected withALREADY_RECORDING; now it causes data loss. Please restore the guard instead of force-stopping.- if (isConnected) { - try { - //If the user is initiating a new recording, it's because the previous recording has already been closed. - recorderService.stopRecording(); - } catch (Exception e) { - Log.d("VoiceRecorder", "Forcing stop recording.", e); - } - startRecordingByCall(call); - return; - } + if (isConnected && recorderService.getMediaRecorder() != null) { + call.reject(Messages.ALREADY_RECORDING); + return; + } + + if (isConnected) { + startRecordingByCall(call); + return; + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/CustomMediaRecorder.java(3 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/ErrorInfo.java(1 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/Messages.java(1 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java(2 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorderService.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorderService.java (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/Messages.java (1)
Messages(3-16)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (2)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/Messages.java (1)
Messages(3-16)android/src/main/java/com/tchvu3/capacitorvoicerecorder/ResponseGenerator.java (1)
ResponseGenerator(5-37)
🪛 GitHub Actions: CI
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorderService.java
[error] Prettier formatting issues detected in this file. Run 'prettier --write' to fix code style issues.
android/src/main/java/com/tchvu3/capacitorvoicerecorder/ErrorInfo.java
[error] Prettier formatting issues detected in this file. Run 'prettier --write' to fix code style issues.
| startCall = call; | ||
| new Handler(Looper.getMainLooper()).post(() -> context.bindService(intent, connection, 0)); | ||
| } |
There was a problem hiding this comment.
Protect startCall from concurrent startRecording invocations
If startRecording is called again while the initial bind is still pending, this assignment overwrites the original PluginCall. When the service finally connects, only the last call is resolved; the earlier promise hangs forever. Guard the assignment so a second caller is rejected (or queued explicitly) while the first start is still in flight.
- startCall = call;
+ if (startCall != null) {
+ call.reject(Messages.ALREADY_RECORDING);
+ return;
+ }
+ startCall = call;📝 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.
| startCall = call; | |
| new Handler(Looper.getMainLooper()).post(() -> context.bindService(intent, connection, 0)); | |
| } | |
| if (startCall != null) { | |
| call.reject(Messages.ALREADY_RECORDING); | |
| return; | |
| } | |
| startCall = call; | |
| new Handler(Looper.getMainLooper()).post(() -> context.bindService(intent, connection, 0)); | |
| } |
🤖 Prompt for AI Agents
In android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java
around lines 166-168, protect the startCall assignment so concurrent
startRecording invocations don’t overwrite the in-flight PluginCall: add a guard
(e.g., check if startCall != null or use an AtomicBoolean isBinding) and if a
bind is already in progress reject the new call immediately with a clear error
(or queue explicitly), otherwise set the flag and assign startCall; ensure the
flag/startCall is cleared when the service connects or on error, and perform the
checks/assignments on the main thread (or synchronize) to avoid race conditions.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/ErrorInfo.java(1 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorderService.java(2 hunks)package.json(1 hunks)
✅ Files skipped from review due to trivial changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (1)
- android/src/main/java/com/tchvu3/capacitorvoicerecorder/ErrorInfo.java
🧰 Additional context used
🧬 Code graph analysis (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorderService.java (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/Messages.java (1)
Messages(3-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Android Verification
- GitHub Check: iOS Verification
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorderService.java (1)
72-74: Resource leak when runtime error check throws early.This issue has already been flagged in a previous review. The error check throws before entering the
tryblock, preventing thefinallycleanup from running. This leaves the recorder instance and temp files unreleased, blocking subsequent recordings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/CustomMediaRecorder.java(3 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorderService.java(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- android/src/main/java/com/tchvu3/capacitorvoicerecorder/CustomMediaRecorder.java
🧰 Additional context used
🧬 Code graph analysis (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorderService.java (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/Messages.java (1)
Messages(3-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Android Verification
🔇 Additional comments (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorderService.java (1)
137-139: LGTM!Capturing the duration in a local variable before releasing the
MediaPlayeris a defensive improvement that ensures the resource is properly released regardless of the return path.
| public void startRecording(String directory, String subDirectory) throws MessagesException { | ||
| try { | ||
| RecordOptions options = new RecordOptions(directory, subDirectory); | ||
| mediaRecorder = new CustomMediaRecorder(getApplicationContext(), options); | ||
| mediaRecorder.startRecording(); | ||
| } catch (Exception exp) { | ||
| mediaRecorder = null; | ||
| throw new MessagesException(Messages.FAILED_TO_RECORD, exp); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add guard against concurrent recordings.
The method doesn't check whether mediaRecorder is already non-null before initializing a new instance. If startRecording is called while a recording is in progress, the previous CustomMediaRecorder instance will be orphaned, leaking resources and potentially leaving temp files on disk.
Apply this diff to add a guard:
public void startRecording(String directory, String subDirectory) throws MessagesException {
+ if (mediaRecorder != null) {
+ throw new MessagesException(Messages.ALREADY_RECORDING);
+ }
try {
RecordOptions options = new RecordOptions(directory, subDirectory);
mediaRecorder = new CustomMediaRecorder(getApplicationContext(), options);📝 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.
| public void startRecording(String directory, String subDirectory) throws MessagesException { | |
| try { | |
| RecordOptions options = new RecordOptions(directory, subDirectory); | |
| mediaRecorder = new CustomMediaRecorder(getApplicationContext(), options); | |
| mediaRecorder.startRecording(); | |
| } catch (Exception exp) { | |
| mediaRecorder = null; | |
| throw new MessagesException(Messages.FAILED_TO_RECORD, exp); | |
| } | |
| } | |
| public void startRecording(String directory, String subDirectory) throws MessagesException { | |
| if (mediaRecorder != null) { | |
| throw new MessagesException(Messages.ALREADY_RECORDING); | |
| } | |
| try { | |
| RecordOptions options = new RecordOptions(directory, subDirectory); | |
| mediaRecorder = new CustomMediaRecorder(getApplicationContext(), options); | |
| mediaRecorder.startRecording(); | |
| } catch (Exception exp) { | |
| mediaRecorder = null; | |
| throw new MessagesException(Messages.FAILED_TO_RECORD, exp); | |
| } | |
| } |
🤖 Prompt for AI Agents
In
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorderService.java
around lines 56 to 65, add a guard to prevent starting a new recording when one
is already active: check if mediaRecorder != null at the start of startRecording
and immediately throw a MessagesException (e.g., Messages.ALREADY_RECORDING)
instead of creating a new CustomMediaRecorder; also make the method synchronized
(or wrap the guard + initialization in a synchronized block) to prevent race
conditions so the previous recorder is never orphaned.
| int a = 2; | ||
| if (1 + 1 == a) { | ||
| throw new MessagesException("Testando pra ver se esta mensagem aparece"); | ||
| } |
There was a problem hiding this comment.
Remove debug code that breaks all recordings.
This test block unconditionally throws an exception on every call to stopRecording, making the entire recording feature non-functional. The condition 1 + 1 == a always evaluates to true.
Apply this diff to remove the test code:
RecordData recordData = new RecordData(
recordDataBase64,
getMsDurationOfAudioFile(recordedFile.getAbsolutePath()),
"audio/aac",
path
);
- int a = 2;
- if (1 + 1 == a) {
- throw new MessagesException("Testando pra ver se esta mensagem aparece");
- }
if ((recordDataBase64 == null && path == null) || recordData.getMsDuration() < 0) {📝 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.
| int a = 2; | |
| if (1 + 1 == a) { | |
| throw new MessagesException("Testando pra ver se esta mensagem aparece"); | |
| } | |
| RecordData recordData = new RecordData( | |
| recordDataBase64, | |
| getMsDurationOfAudioFile(recordedFile.getAbsolutePath()), | |
| "audio/aac", | |
| path | |
| ); | |
| if ((recordDataBase64 == null && path == null) || recordData.getMsDuration() < 0) { |
🤖 Prompt for AI Agents
In
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorderService.java
around lines 98 to 101, remove the debug block that declares "int a = 2;" and
the conditional that throws a MessagesException (the always-true check `if (1 +
1 == a) { throw ... }`) so stopRecording no longer throws unconditionally;
simply delete that test code (or guard it behind a debug flag) and ensure the
method proceeds with the normal stop/cleanup logic and compiles successfully.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (1)
166-168: Guard startCall assignment against concurrent invocations.If
startRecording(which calls this method) is invoked again before the service binding completes, line 166 overwritesstartCall, causing the first caller's promise to hang indefinitely. Check ifstartCallis already non-null and reject the new call immediately.Apply this diff to guard the assignment:
+ if (startCall != null) { + call.reject(Messages.ALREADY_RECORDING); + return; + } startCall = call; new Handler(Looper.getMainLooper()).post(() -> context.bindService(intent, connection, 0));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (2)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/Messages.java (1)
Messages(3-16)android/src/main/java/com/tchvu3/capacitorvoicerecorder/ResponseGenerator.java (1)
ResponseGenerator(5-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Publish Plugin
| if (startCall != null) { | ||
| try { | ||
| recorderService.startRecording(); | ||
| startCall.resolve(ResponseGenerator.successResponse()); | ||
| } catch (Exception e) { | ||
| startCall.reject(Messages.FAILED_TO_RECORD, e); | ||
| } | ||
| startRecordingByCall(startCall); | ||
| startCall = null; | ||
| } |
There was a problem hiding this comment.
Protect startCall from uncaught exceptions in startRecordingByCall.
If startRecordingByCall throws an unexpected RuntimeException (not MessagesException), startCall is cleared on line 48 without rejecting the call, leaving the JavaScript promise unresolved. Wrap the invocation in a try-catch block and reject startCall on unexpected exceptions before clearing it.
Apply this diff to protect the call:
if (startCall != null) {
- startRecordingByCall(startCall);
- startCall = null;
+ try {
+ startRecordingByCall(startCall);
+ } catch (Exception e) {
+ startCall.reject(Messages.FAILED_TO_RECORD, e);
+ } finally {
+ startCall = null;
+ }
}📝 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.
| if (startCall != null) { | |
| try { | |
| recorderService.startRecording(); | |
| startCall.resolve(ResponseGenerator.successResponse()); | |
| } catch (Exception e) { | |
| startCall.reject(Messages.FAILED_TO_RECORD, e); | |
| } | |
| startRecordingByCall(startCall); | |
| startCall = null; | |
| } | |
| if (startCall != null) { | |
| try { | |
| startRecordingByCall(startCall); | |
| } catch (Exception e) { | |
| startCall.reject(Messages.FAILED_TO_RECORD, e); | |
| } finally { | |
| startCall = null; | |
| } | |
| } |
🤖 Prompt for AI Agents
In android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java
around lines 46–49, wrap the startRecordingByCall(startCall) invocation in a
try-catch-finally: call startRecordingByCall inside try, catch unexpected
exceptions (e.g., RuntimeException or Exception) and call startCall.reject(...)
with a clear error message (include e.getMessage() or stack info), and ensure
startCall is nulled in finally so the JS promise is rejected on error and
startCall is always cleared.
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (4)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (4)
46-49: Protect startCall from unexpected exceptions.Past review identified this gap: if
startRecordingByCallthrows an unexpected exception that isn'tMessagesException(e.g.,NullPointerException),startCallis cleared on line 48 without rejecting the call, leaving the JavaScript promise unresolved.Apply this diff to protect the call:
if (startCall != null) { - startRecordingByCall(startCall); - startCall = null; + try { + startRecordingByCall(startCall); + } catch (Exception e) { + startCall.reject(Messages.FAILED_TO_RECORD, e); + } finally { + startCall = null; + } }
118-127: Document the auto-restart behavior change.This code silently stops an existing recording and starts a new one when the service is already connected. This differs from the previous behavior where duplicate recording attempts were rejected with
ALREADY_RECORDING. The comment on line 120 is misleading—it states "the previous recording has already been closed," but the code is actively stopping a potentially active recording.Consider either:
- Maintain previous behavior by checking if recording is active and rejecting with
ALREADY_RECORDING, or- Document this new auto-restart behavior in the method's documentation and update the comment to accurately reflect that the code forcibly stops an active recording.
166-166: Protect against concurrent startRecording overwrites.If
startRecordingis called again while a previous bind is still pending, this assignment unconditionally overwritesstartCall. The first caller's promise will hang forever becauseonServiceConnected(line 46) will only resolve the most recent call.Guard the assignment:
+ if (startCall != null) { + call.reject(Messages.ALREADY_RECORDING); + return; + } startCall = call;
192-197: Ensure isConnected flag is always updated.If
unbindServicethrowsIllegalArgumentException(service was already unbound or never bound), line 194 is skipped, leavingisConnected = true. SubsequentstartRecordingcalls will incorrectly assume the service is connected and skip service initialization.Move the flag update to a finally block:
try { context.unbindService(connection); - isConnected = false; } catch (IllegalArgumentException e) { Log.d("VoiceRecorder", "Attempted to unbind service, but it was already unbound.", e); + } finally { + isConnected = false; }
🧹 Nitpick comments (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (1)
140-149: Consider catching broader exception types.
startRecordingByCallonly catchesMessagesException. IfrecorderService.startRecordingthrows an unexpectedRuntimeException(e.g.,IllegalStateExceptionfrom MediaRecorder), it will bubble up and may leave the caller in an inconsistent state.Consider catching and wrapping unexpected exceptions:
private void startRecordingByCall(PluginCall call) { try { String directory = call.getString("directory"); String subDirectory = call.getString("subDirectory"); recorderService.startRecording(directory, subDirectory); call.resolve(ResponseGenerator.successResponse()); } catch (MessagesException e) { call.reject(e.getMessage(), e); + } catch (Exception e) { + call.reject(Messages.RUNTIME_FAILED, e); } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (2)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/Messages.java (1)
Messages(3-16)android/src/main/java/com/tchvu3/capacitorvoicerecorder/ResponseGenerator.java (1)
ResponseGenerator(5-37)
| } | ||
|
|
||
| startCall = call; | ||
| new Handler(Looper.getMainLooper()).post(() -> context.bindService(intent, connection, 0)); |
There was a problem hiding this comment.
Handle bindService failure.
bindService can return false if binding fails, but the return value is ignored. If binding fails, onServiceConnected is never called, and startCall hangs unresolved forever.
Capture the result and reject the call on failure:
- startCall = call;
- new Handler(Looper.getMainLooper()).post(() -> context.bindService(intent, connection, 0));
+ startCall = call;
+ new Handler(Looper.getMainLooper()).post(() -> {
+ boolean bound = context.bindService(intent, connection, 0);
+ if (!bound) {
+ call.reject(Messages.FAILED_TO_RECORD, "Failed to bind to recording service");
+ startCall = null;
+ context.stopService(intent);
+ }
+ });📝 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.
| new Handler(Looper.getMainLooper()).post(() -> context.bindService(intent, connection, 0)); | |
| startCall = call; | |
| new Handler(Looper.getMainLooper()).post(() -> { | |
| boolean bound = context.bindService(intent, connection, 0); | |
| if (!bound) { | |
| call.reject(Messages.FAILED_TO_RECORD, "Failed to bind to recording service"); | |
| startCall = null; | |
| context.stopService(intent); | |
| } | |
| }); |
🤖 Prompt for AI Agents
In android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java
around line 167, the call to bindService is invoked on the main thread but its
boolean return value is ignored; if binding fails on false, onServiceConnected
will never be called and startCall will hang. Capture the result of
context.bindService(intent, connection, 0) into a boolean inside the posted
Runnable, and if it returns false immediately reject the pending PluginCall (or
invoke the startCall rejection path) with an appropriate error message and
perform any needed cleanup/state reset so the call does not hang.
| try { | ||
| JSObject obj = recorderService.stopRecording(); | ||
| call.resolve(obj); | ||
| } catch (MessagesException e) { | ||
| if (e.getCause() != null && e.getCause() instanceof MessagesException) { | ||
| call.reject(e.getMessage(), e.getCause().getMessage()); | ||
| } else { | ||
| call.reject(e.getMessage()); | ||
| } | ||
| } else { | ||
| call.reject(Messages.PLUGIN_UNBOUND + ": " + serviceState); | ||
| } | ||
|
|
||
| Context context = getContext(); | ||
| Intent intent = new Intent(context, VoiceRecorderService.class); | ||
| context.stopService(intent); |
There was a problem hiding this comment.
Ensure service cleanup in finally block.
If recorderService.stopRecording() throws (lines 177-186), execution jumps to the catch block and completes without reaching line 190, leaving the service running. Service cleanup should execute regardless of whether stopping the recorder succeeds.
Move service cleanup to a finally block:
try {
JSObject obj = recorderService.stopRecording();
call.resolve(obj);
} catch (MessagesException e) {
if (e.getCause() != null && e.getCause() instanceof MessagesException) {
call.reject(e.getMessage(), e.getCause().getMessage());
} else {
call.reject(e.getMessage());
}
+ } finally {
+ Context context = getContext();
+ Intent intent = new Intent(context, VoiceRecorderService.class);
+ context.stopService(intent);
}
-
- Context context = getContext();
- Intent intent = new Intent(context, VoiceRecorderService.class);
- context.stopService(intent);🤖 Prompt for AI Agents
In android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java
around lines 177 to 190, move the service cleanup (creating the Intent and
calling context.stopService) into a finally block so it always runs even if
recorderService.stopRecording() throws; keep the try/catch for
resolving/rejecting the JS call as-is, but obtain the Context (or cache it
before the try) so the finally block can create the Intent and call
context.stopService(intent) reliably, ensuring no code path leaves the service
running.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (6)
46-49: Wrap startRecordingByCall in try-catch to prevent unresolved promises.If
startRecordingByCallthrows an unexpected exception (notMessagesException),startCallis cleared on line 48 without rejecting the call, leaving the JavaScript promise unresolved indefinitely.Apply this diff to ensure the call is always rejected on error:
if (startCall != null) { - startRecordingByCall(startCall); - startCall = null; + try { + startRecordingByCall(startCall); + } catch (Exception e) { + startCall.reject(Messages.FAILED_TO_RECORD, e); + } finally { + startCall = null; + } }
118-127: Auto-restart behavior removes duplicate recording detection.The logic now silently force-stops an existing recording and starts a new one when already connected. This differs from the previous behavior where
ALREADY_RECORDINGwas rejected, removing the ability for clients to detect duplicate recording attempts. Additionally, the comment on line 120 is misleading—this actually enables concurrent calls to force-stop and restart.Consider either:
- Maintaining the previous behavior by checking if the recorder is active and rejecting with
ALREADY_RECORDING, or- Documenting this auto-restart behavior clearly in the public API.
if (isConnected) { + CustomMediaRecorder recorder = getMediaRecorder(); + if (recorder != null && recorder.getCurrentStatus() != CurrentRecordingStatus.NONE) { + call.reject(Messages.ALREADY_RECORDING); + return; + } try { - //If the user is initiating a new recording, it's because the previous recording has already been closed. recorderService.stopRecording(); } catch (Exception e) { Log.d("VoiceRecorder", "Forcing stop recording.", e); }
129-137: Clean up binding state on service start failure.If
startServiceByCallsuccessfully initiates binding but then throws an exception, the catch block stops the service but doesn't account for the asynchronous binding that may still complete later. This can leaveisConnected = trueandrecorderServiceset even though the service was stopped.Apply this diff to ensure proper cleanup:
try { startServiceByCall(call); } catch (Exception exp) { call.reject(Messages.FAILED_TO_RECORD, exp); Context context = getContext(); Intent intent = new Intent(context, VoiceRecorderService.class); + try { + context.unbindService(connection); + } catch (IllegalArgumentException e) { + // Service was never bound + } context.stopService(intent); + startCall = null; + isConnected = false; }
166-168: Protect startCall from concurrent invocations.If
startRecordingis called again while the initial bind is pending, line 166 overwrites the originalPluginCall. When the service finally connects, only the last call is resolved; earlier promises hang forever.Apply this diff to reject concurrent calls:
+ if (startCall != null) { + call.reject(Messages.ALREADY_RECORDING); + return; + } startCall = call; new Handler(Looper.getMainLooper()).post(() -> context.bindService(intent, connection, 0));
167-167: Handle bindService failure to prevent hanging promises.
bindServicecan returnfalseif binding fails, but the return value is ignored. If binding fails,onServiceConnectedis never called andstartCallhangs unresolved forever.Apply this diff to capture and handle the result:
startCall = call; - new Handler(Looper.getMainLooper()).post(() -> context.bindService(intent, connection, 0)); + new Handler(Looper.getMainLooper()).post(() -> { + boolean bound = context.bindService(intent, connection, 0); + if (!bound) { + call.reject(Messages.FAILED_TO_RECORD, "Failed to bind to recording service"); + startCall = null; + context.stopService(intent); + } + });
177-193: Move service cleanup to finally blocks to prevent resource leaks.Two critical issues:
- If
recorderService.stopRecording()throws (lines 177-182), execution skips the service cleanup at lines 184-186, leaving the service running.- If
unbindServicethrowsIllegalArgumentException(line 191), line 190 is skipped, leavingisConnected = true.Apply this diff to ensure cleanup always executes:
try { JSObject obj = recorderService.stopRecording(); call.resolve(obj); } catch (MessagesException e) { call.reject(e.getMessage(), e.toJSObject()); + } finally { + Context context = getContext(); + Intent intent = new Intent(context, VoiceRecorderService.class); + context.stopService(intent); + + try { + context.unbindService(connection); + } catch (IllegalArgumentException e) { + Log.d("VoiceRecorder", "Attempted to unbind service, but it was already unbound.", e); + } finally { + isConnected = false; + } } - - Context context = getContext(); - Intent intent = new Intent(context, VoiceRecorderService.class); - context.stopService(intent); - - try { - context.unbindService(connection); - isConnected = false; - } catch (IllegalArgumentException e) { - Log.d("VoiceRecorder", "Attempted to unbind service, but it was already unbound.", e); - }
🧹 Nitpick comments (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/MessagesException.java (1)
28-33: Consider using try-with-resources for cleaner resource management.While
StringWriterandPrintWriterdon't hold system resources that require explicit cleanup, using try-with-resources improves code consistency and clarity.Apply this diff to use try-with-resources:
private String getStackTraceAsString() { - StringWriter sw = new StringWriter(); - PrintWriter pw = new PrintWriter(sw); - this.printStackTrace(pw); - return sw.toString(); + try (StringWriter sw = new StringWriter(); + PrintWriter pw = new PrintWriter(sw)) { + this.printStackTrace(pw); + return sw.toString(); + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/MessagesException.java(2 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (2)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/Messages.java (1)
Messages(3-16)android/src/main/java/com/tchvu3/capacitorvoicerecorder/ResponseGenerator.java (1)
ResponseGenerator(5-37)
🔇 Additional comments (3)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/MessagesException.java (1)
3-6: LGTM!The imports are appropriate for the new JSObject serialization functionality.
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (2)
59-64: LGTM!The refactor from
serviceStatetoisConnectedis correct and maintains the same behavior.
140-149: LGTM!The helper method cleanly encapsulates the recording start logic with appropriate error handling.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (5)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (5)
46-49: Protect startCall from uncaught exceptions in startRecordingByCall.If
startRecordingByCallthrows an unexpected exception (not caught internally),startCallis cleared on line 48 without rejecting the call, leaving the JavaScript promise unresolved.Apply this diff to protect the call:
if (startCall != null) { - startRecordingByCall(startCall); - startCall = null; + try { + startRecordingByCall(startCall); + } catch (Exception e) { + startCall.reject(Messages.FAILED_TO_RECORD, e); + } finally { + startCall = null; + } }
129-137: Guard against service/binding state inconsistency on error.If
startServiceByCallsuccessfully initiates binding but then throws an exception, the asynchronous binding may still complete later, settingisConnected = trueand calling the already-rejectedstartCall. Additionally,startCallis not cleared in this catch block.Apply this diff to ensure proper cleanup:
try { startServiceByCall(call); } catch (Exception exp) { call.reject(Messages.FAILED_TO_RECORD, exp); Context context = getContext(); Intent intent = new Intent(context, VoiceRecorderService.class); + try { + context.unbindService(connection); + } catch (IllegalArgumentException e) { + // Service was never bound or already unbound + } context.stopService(intent); + startCall = null; + isConnected = false; }
166-168: Protect startCall from concurrent startRecording invocations.If
startRecordingis called again while the initial bind is still pending, line 166 overwrites the originalPluginCall. When the service finally connects, only the last call is resolved; earlier promises hang forever.Guard the assignment so a second caller is rejected while the first start is in flight:
- startCall = call; + if (startCall != null) { + call.reject(Messages.ALREADY_RECORDING); + return; + } + startCall = call;
167-167: Handle bindService failure.
bindServicecan returnfalseif binding fails, but the return value is ignored. If binding fails,onServiceConnectedis never called, andstartCallhangs unresolved forever.Capture the result and reject the call on failure:
startCall = call; new Handler(Looper.getMainLooper()).post(() -> { - context.bindService(intent, connection, 0)); + boolean bound = context.bindService(intent, connection, 0); + if (!bound) { + if (startCall != null) { + startCall.reject(Messages.FAILED_TO_RECORD, "Failed to bind to recording service"); + startCall = null; + } + context.stopService(intent); + isConnected = false; + } + });
188-193: Ensure isConnected is updated even when unbindService throws.If
unbindServicethrowsIllegalArgumentException(service already unbound), line 190 is skipped, leavingisConnectedset totrue. SubsequentstartRecordingcalls will incorrectly assume the service is connected.Move the flag update to a finally block:
try { context.unbindService(connection); - isConnected = false; } catch (IllegalArgumentException e) { Log.d("VoiceRecorder", "Attempted to unbind service, but it was already unbound.", e); + } finally { + isConnected = false; }
🧹 Nitpick comments (3)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (3)
36-36: Consider tracking binding state to prevent race conditions.The boolean
isConnectedflag only reflects whether the service is connected or not, but doesn't distinguish intermediate states like "binding in progress." This can allow concurrentstartRecordingcalls to proceed simultaneously during the binding phase, potentially overwritingstartCallor starting multiple binding operations.Consider adding an additional flag or using an enum to track intermediate states:
- private boolean isConnected = false; + private boolean isConnected = false; + private boolean isBinding = false;Then guard against concurrent operations by checking
isBindingbefore initiating a new bind.
118-127: Document the auto-restart behavior change.The code now silently stops any existing recording and starts a new one when
isConnectedis true. This differs from the previous behavior where concurrent recording attempts were rejected withALREADY_RECORDING. This behavioral change removes the client's ability to detect duplicate recording attempts and could lead to unexpected data loss if a recording is inadvertently stopped.Consider either:
- Restoring the previous behavior by checking if recording is active and rejecting with
ALREADY_RECORDING, or- Documenting this new auto-restart behavior clearly in API documentation and release notes
If auto-restart is intentional:
if (isConnected) { try { //If the user is initiating a new recording, it's because the previous recording has already been closed. + // Note: This will automatically stop any in-progress recording recorderService.stopRecording(); } catch (Exception e) { Log.d("VoiceRecorder", "Forcing stop recording.", e); }
140-149: Consider consistent error reporting with stopRecording.The error handling on line 147 passes the exception object directly to
reject(), whereasstopRecordingat line 181 usese.toJSObject().toString()for the detail message. This inconsistency may result in different error formats being returned to JavaScript for similar exceptions.For consistency, consider using the same error format:
} catch (MessagesException e) { - call.reject(e.getMessage(), e); + call.reject(e.getMessage(), e.toJSObject().toString()); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (2)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/Messages.java (1)
Messages(3-16)android/src/main/java/com/tchvu3/capacitorvoicerecorder/ResponseGenerator.java (1)
ResponseGenerator(5-37)
🔇 Additional comments (2)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (2)
59-64: LGTM!The simplified boolean check is clearer and functionally equivalent to the previous enum-based check.
171-194: LGTM with one caveat - see separate comment about isConnected update.The overall flow correctly stops the recording, resolves/rejects the call, stops the service, and unbinds. The service cleanup at lines 184-186 executes regardless of whether
stopRecording()succeeds, which is correct.However, there's still an issue with the
isConnectedflag update (see separate comment below).
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (5)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (5)
46-49: Protect startCall from uncaught exceptions.If
startRecordingByCallthrows an unexpected exception (not caught internally),startCallis cleared on line 48 without rejecting the call, leaving the JavaScript promise unresolved forever.Apply this diff to ensure the promise is always resolved or rejected:
if (startCall != null) { + try { startRecordingByCall(startCall); + } catch (Exception e) { + startCall.reject(Messages.FAILED_TO_RECORD, e); + } finally { startCall = null; + } - startCall = null; }
151-168: Protect against concurrent startRecording calls and handle bindService failure.Two critical issues exist in this flow:
Concurrent call protection (lines 166-167): If
startRecordingis called again while the service is binding, the second call overwritesstartCall, causing the first caller's promise to hang forever.bindService failure handling (line 167):
bindServicecan returnfalseif binding fails. The return value is ignored, soonServiceConnectedwill never fire andstartCallwill hang forever.Apply this diff to address both issues:
- startCall = call; - new Handler(Looper.getMainLooper()).post(() -> context.bindService(intent, connection, 0)); + if (startCall != null) { + call.reject(Messages.ALREADY_RECORDING); + return; + } + startCall = call; + new Handler(Looper.getMainLooper()).post(() -> { + boolean bound = context.bindService(intent, connection, 0); + if (!bound) { + call.reject(Messages.FAILED_TO_RECORD, "Failed to bind to recording service"); + startCall = null; + context.stopService(intent); + } + });
129-137: Guard against service/binding state inconsistency on error.If
startServiceByCallstarts the service and initiates binding (line 167) but then throws an exception, the catch block stops the service but does not unbind. The asynchronous binding may still complete later, leavingisConnected = trueandrecorderServiceset even though the service was stopped.Apply this diff to clean up binding state on error:
try { startServiceByCall(call); } catch (Exception exp) { call.reject(Messages.FAILED_TO_RECORD, exp); Context context = getContext(); Intent intent = new Intent(context, VoiceRecorderService.class); + try { + context.unbindService(connection); + } catch (IllegalArgumentException e) { + // Service was never bound + } context.stopService(intent); + startCall = null; + isConnected = false; }
177-186: Move service cleanup to finally block.If
recorderService.stopRecording()throws (line 178), execution jumps to the catch block and completes without reaching lines 184-186, leaving the service running. Service cleanup must execute regardless of whether stopping the recorder succeeds.Apply this diff to ensure cleanup always runs:
+ Context context = getContext(); + Intent intent = new Intent(context, VoiceRecorderService.class); + try { JSObject obj = recorderService.stopRecording(); call.resolve(obj); } catch (MessagesException e) { call.reject(e.getMessage(), e.toJSObject()); + } finally { + context.stopService(intent); } - - Context context = getContext(); - Intent intent = new Intent(context, VoiceRecorderService.class); - context.stopService(intent);
188-193: Move isConnected flag update to finally block.If
unbindServicethrowsIllegalArgumentException(service already unbound), line 190 is skipped, leavingisConnected = true. SubsequentstartRecordingcalls will incorrectly assume the service is connected and attempt to use a nullrecorderService.Apply this diff to ensure the flag is always updated:
try { context.unbindService(connection); - isConnected = false; } catch (IllegalArgumentException e) { Log.d("VoiceRecorder", "Attempted to unbind service, but it was already unbound.", e); + } finally { + isConnected = false; }
🧹 Nitpick comments (2)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (2)
140-149: Consider validating directory parameters.The
directoryandsubDirectoryparameters are extracted but not validated before passing torecorderService.startRecording(). If the service expects non-null or non-empty values, this could throw confusing errors.
188-193: Consider removing manual isConnected flag update.Setting
isConnected = falsemanually after unbinding could create race conditions. TheonServiceDisconnected()callback at line 54 already setsisConnected = false. If the service disconnects asynchronously while this code runs, there could be conflicting state updates.However, since
unbindServiceis synchronous and should trigger immediate disconnection, the manual flag update may be intentionally defensive. If so, consider adding a comment explaining why both updates are needed.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/MessagesException.java(2 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (2)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/Messages.java (1)
Messages(3-16)android/src/main/java/com/tchvu3/capacitorvoicerecorder/ResponseGenerator.java (1)
ResponseGenerator(5-37)
🪛 GitHub Actions: CI
android/src/main/java/com/tchvu3/capacitorvoicerecorder/MessagesException.java
[warning] 1-1: Code style issues found in the above file. Forgot to run Prettier?
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues in this file.
🔇 Additional comments (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (1)
118-127: Document the behavioral change: auto-restart instead of rejecting duplicate recordings.The code now silently stops any existing recording and starts a new one when already connected. This differs from the previous behavior where
ALREADY_RECORDINGwould be rejected. While the inline comment explains the intent, this is a breaking change that removes the client's ability to detect duplicate recording attempts.Consider whether this auto-restart behavior should be:
- Documented in the public API
- Made opt-in via a parameter
- Preserved as-is if this is the desired UX
| @@ -1,5 +1,9 @@ | |||
| package com.tchvu3.capacitorvoicerecorder; | |||
There was a problem hiding this comment.
Fix Prettier formatting to resolve pipeline failure.
The CI pipeline reports code style issues. Run prettier --write on this file to fix formatting.
🧰 Tools
🪛 GitHub Actions: CI
[warning] 1-1: Code style issues found in the above file. Forgot to run Prettier?
[error] 1-1: Prettier formatting check failed. Run 'prettier --write' to fix code style issues in this file.
🤖 Prompt for AI Agents
In
android/src/main/java/com/tchvu3/capacitorvoicerecorder/MessagesException.java
around line 1, the file fails CI due to Prettier formatting; run the code
formatter (e.g., run `prettier --write
android/src/main/java/com/tchvu3/capacitorvoicerecorder/MessagesException.java`)
or apply the project's Prettier rules to reformat the file so it matches
repository style and then re-commit the formatted file.
| public JSObject toJSObject() { | ||
| JSObject toReturn = new JSObject(); | ||
| toReturn.put("message", this.getMessage()); | ||
| JSONArray messagesArray = getAllMessages(); | ||
| toReturn.put("causes", messagesArray); | ||
| return toReturn; | ||
| } |
There was a problem hiding this comment.
Guard against null messages in toJSObject().
If this.getMessage() returns null, the JSObject will contain a null "message" field, which may break JavaScript consumers expecting a string.
Apply this diff to provide a safe fallback:
public JSObject toJSObject() {
JSObject toReturn = new JSObject();
- toReturn.put("message", this.getMessage());
+ String message = this.getMessage();
+ toReturn.put("message", message != null ? message : "Unknown error");
JSONArray messagesArray = getAllMessages();
toReturn.put("causes", messagesArray);
return toReturn;
}🤖 Prompt for AI Agents
In
android/src/main/java/com/tchvu3/capacitorvoicerecorder/MessagesException.java
around lines 17 to 23, toJSObject() places this.getMessage() directly into the
JSObject which may be null; change it to guard against null by computing a
safeMessage = (this.getMessage() != null ? this.getMessage() : "") and put that
safeMessage into the "message" field so JavaScript consumers always receive a
string.
| private JSONArray getAllMessages() { | ||
| JSONArray messages = new JSONArray(); | ||
| Throwable current = getCause(); | ||
| while (current != null) { | ||
| messages.put(current.getMessage()); | ||
| current = current.getCause(); | ||
| } | ||
| return messages; | ||
| } |
There was a problem hiding this comment.
Guard against null messages in cause chain.
If any cause in the exception chain has a null message, JSONArray.put(null) will insert a null entry. JavaScript consumers may not expect null values in the causes array.
Apply this diff to filter out null messages:
private JSONArray getAllMessages() {
JSONArray messages = new JSONArray();
Throwable current = getCause();
while (current != null) {
- messages.put(current.getMessage());
+ String message = current.getMessage();
+ if (message != null) {
+ messages.put(message);
+ }
current = current.getCause();
}
return messages;
}🤖 Prompt for AI Agents
In
android/src/main/java/com/tchvu3/capacitorvoicerecorder/MessagesException.java
around lines 25 to 33, the getAllMessages method currently puts every cause
message into the JSONArray even when a cause's getMessage() returns null; update
the loop to check each cause message and only put non-null messages into the
JSONArray (i.e., retrieve current.getMessage(), if it is not null then
messages.put(message)), ensuring nulls are skipped so the returned JSON array
contains only actual strings.
2812a4e to
c95a2af
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (7)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/MessagesException.java (2)
16-22: Guard against null messages in toJSObject().If
this.getMessage()returnsnull, the JSObject will contain a null "message" field, which may break JavaScript consumers expecting a string. Provide a safe fallback:public JSObject toJSObject() { JSObject toReturn = new JSObject(); - toReturn.put("message", this.getMessage()); + String message = this.getMessage(); + toReturn.put("message", message != null ? message : "Unknown error"); JSONArray messagesArray = getAllMessages(); toReturn.put("causes", messagesArray); return toReturn; }
24-32: Guard against null messages in cause chain.If any cause in the exception chain has a null message,
JSONArray.put(null)will insert a null entry. JavaScript consumers may not expect null values in the causes array. Filter out null messages:private JSONArray getAllMessages() { JSONArray messages = new JSONArray(); Throwable current = getCause(); while (current != null) { - messages.put(current.getMessage()); + String message = current.getMessage(); + if (message != null) { + messages.put(message); + } current = current.getCause(); } return messages; }android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorderService.java (1)
56-65: Add guard against concurrent recordings.The method doesn't check whether
mediaRecorderis already non-null before initializing a new instance. IfstartRecordingis called while a recording is in progress, the previousCustomMediaRecorderinstance will be orphaned, leaking resources and potentially leaving temp files on disk.public void startRecording(String directory, String subDirectory) throws MessagesException { + if (mediaRecorder != null) { + throw new MessagesException(Messages.ALREADY_RECORDING); + } try { RecordOptions options = new RecordOptions(directory, subDirectory); mediaRecorder = new CustomMediaRecorder(getApplicationContext(), options);android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (4)
44-49: Protect startCall from uncaught exceptions in startRecordingByCall.If
startRecordingByCallthrows an unexpectedRuntimeException(notMessagesException),startCallis cleared on Line 48 without rejecting the call, leaving the JavaScript promise unresolved. Wrap the invocation in try-catch-finally:if (startCall != null) { - startRecordingByCall(startCall); - startCall = null; + try { + startRecordingByCall(startCall); + } catch (Exception e) { + startCall.reject(Messages.FAILED_TO_RECORD, e); + } finally { + startCall = null; + } }
129-137: Guard against service/binding state inconsistency on error.If
startServiceByCallsuccessfully starts the service and begins binding but then throws an exception, Line 136 callsstopService, but the asynchronous binding initiated on Line 167 may still complete later, leavingisConnected = trueandrecorderServiceset even though the service was stopped. Track binding state and unbind on error:try { startServiceByCall(call); } catch (Exception exp) { call.reject(Messages.FAILED_TO_RECORD, exp); Context context = getContext(); Intent intent = new Intent(context, VoiceRecorderService.class); + try { + context.unbindService(connection); + } catch (IllegalArgumentException e) { + // Service was never bound + } context.stopService(intent); + startCall = null; + isConnected = false; }
151-168: Handle bindService failure and protect startCall from concurrent access.Two issues:
Line 167:
bindServicecan returnfalseif binding fails, but the return value is ignored. If binding fails,onServiceConnectedis never called, andstartCallhangs unresolved forever.Line 166: If
startRecordingis called again while the initial bind is pending, this assignment overwrites the originalPluginCall. When the service connects, only the last call is resolved; earlier promises hang.Apply this diff:
- startCall = call; - new Handler(Looper.getMainLooper()).post(() -> context.bindService(intent, connection, 0)); + if (startCall != null) { + call.reject(Messages.ALREADY_RECORDING); + return; + } + startCall = call; + new Handler(Looper.getMainLooper()).post(() -> { + boolean bound = context.bindService(intent, connection, 0); + if (!bound) { + call.reject(Messages.FAILED_TO_RECORD, "Failed to bind to recording service"); + startCall = null; + context.stopService(intent); + } + });
177-193: Move service cleanup to finally block and fix unbind flag update.Two issues:
Lines 177-186: If
recorderService.stopRecording()throws, execution jumps to the catch block without reaching Line 186, leaving the service running. Service cleanup should execute regardless of whether stopping succeeds.Lines 188-190: If
unbindServicethrowsIllegalArgumentException(service already unbound), Line 190 is skipped, leavingisConnected = true. SubsequentstartRecordingcalls will incorrectly assume the service is connected.Apply this diff:
try { JSObject obj = recorderService.stopRecording(); call.resolve(obj); } catch (MessagesException e) { call.reject(e.getMessage(), e.toJSObject()); + } finally { + Context context = getContext(); + Intent intent = new Intent(context, VoiceRecorderService.class); + context.stopService(intent); + + try { + context.unbindService(connection); + } catch (IllegalArgumentException e) { + Log.d("VoiceRecorder", "Attempted to unbind service, but it was already unbound.", e); + } finally { + isConnected = false; + } } - - Context context = getContext(); - Intent intent = new Intent(context, VoiceRecorderService.class); - context.stopService(intent); - - try { - context.unbindService(connection); - isConnected = false; - } catch (IllegalArgumentException e) { - Log.d("VoiceRecorder", "Attempted to unbind service, but it was already unbound.", e); - }
🧹 Nitpick comments (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (1)
118-127: Document the auto-restart behavior.When the service is already connected, the code now silently stops any existing recording and starts a new one. This differs from previous behavior that rejected duplicate recording attempts with
ALREADY_RECORDING. Consider documenting this behavioral change or maintaining the guard to prevent accidental recording overwrites.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.prettierignore(1 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/CustomMediaRecorder.java(3 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/ErrorInfo.java(1 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/Messages.java(1 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/MessagesException.java(2 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/ServiceState.java(0 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java(2 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorderService.java(3 hunks)
💤 Files with no reviewable changes (1)
- android/src/main/java/com/tchvu3/capacitorvoicerecorder/ServiceState.java
🚧 Files skipped from review as they are similar to previous changes (3)
- android/src/main/java/com/tchvu3/capacitorvoicerecorder/ErrorInfo.java
- .prettierignore
- android/src/main/java/com/tchvu3/capacitorvoicerecorder/CustomMediaRecorder.java
🧰 Additional context used
🧬 Code graph analysis (2)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (2)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/Messages.java (1)
Messages(3-16)android/src/main/java/com/tchvu3/capacitorvoicerecorder/ResponseGenerator.java (1)
ResponseGenerator(5-37)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorderService.java (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/Messages.java (1)
Messages(3-16)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Android Verification
🔇 Additional comments (5)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/Messages.java (1)
15-15: LGTM!The new
RUNTIME_FAILEDconstant follows the existing pattern and aligns with the enhanced error reporting introduced elsewhere in the PR.android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorderService.java (2)
72-77: LGTM! Error check now inside try/finally block.The error info check is now properly placed inside the try block, ensuring the finally clause executes for cleanup even when runtime errors are detected. This addresses the resource leak concern from previous reviews.
133-135: LGTM! Proper MediaPlayer cleanup.Capturing the duration before releasing the MediaPlayer ensures the resource is properly freed while preserving the return value. Good defensive programming.
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (2)
36-36: LGTM! Simplified state tracking.Replacing the
ServiceStateenum with a booleanisConnectedflag simplifies the connection state management and aligns well with the service binding lifecycle.
140-149: LGTM! Clean separation of recording logic.The new
startRecordingByCallhelper properly encapsulates the recording initiation logic and handlesMessagesExceptionappropriately.
c95a2af to
3b60408
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (8)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/MessagesException.java (2)
16-22: Guard against null message in toJSObject().If
this.getMessage()returnsnull, the JSObject will contain a null "message" field, which may break JavaScript consumers expecting a string. Consider providing a safe fallback such as an empty string or "Unknown error".
24-32: Guard against null messages in cause chain.If any cause in the exception chain has a null message,
JSONArray.put(null)will insert a null entry. JavaScript consumers may not expect null values in the causes array. Consider filtering out null messages before adding them to the array.android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (6)
46-49: Protect startCall from uncaught exceptions in startRecordingByCall.If
startRecordingByCallthrows an unexpectedRuntimeException(notMessagesException),startCallis cleared on line 48 without rejecting the call, leaving the JavaScript promise unresolved. Wrap the invocation in try-catch-finally to rejectstartCallon unexpected exceptions before clearing it.
118-126: Behavioral change: Silent restart instead of ALREADY_RECORDING error.The logic now silently stops an existing recording and starts a new one when the service is already connected. This differs from the previous behavior where
ALREADY_RECORDINGwas rejected. This removes the ability for clients to detect duplicate recording attempts. Consider either maintaining the previous behavior by checking if recording is active and rejecting withALREADY_RECORDING, or document this new auto-restart behavior clearly.
129-137: Guard against service/binding state inconsistency on error.If
startServiceByCallsuccessfully starts the service and begins binding but then throws an exception, line 136 callsstopService, but the asynchronous binding initiated on line 167 may still complete later. This can leaveisConnected = trueandrecorderServiceset even though the service was stopped. Consider tracking binding state explicitly and unbinding on error, or clearingstartCalland ensuring theonServiceConnectedcallback no-ops if the component is in an error state.
166-166: Protect startCall from concurrent startRecording invocations.If
startRecordingis called again while the initial bind is still pending, this assignment overwrites the originalPluginCall. When the service finally connects, only the last call is resolved; earlier promises hang forever. Guard the assignment so a second caller is rejected (or queued explicitly) while the first start is still in flight.
167-167: Handle bindService failure.
bindServicecan returnfalseif binding fails, but the return value is ignored. If binding fails,onServiceConnectedis never called, andstartCallhangs unresolved forever. Capture the result and reject the call on failure, cleaning up the service and state.
177-186: Ensure service cleanup in finally block.If
recorderService.stopRecording()throws (line 178), execution jumps to the catch block (lines 180-182) and completes without reaching lines 184-186, leaving the service running. Service cleanup (lines 184-186) should execute regardless of whether stopping the recorder succeeds. Move the service stop logic to a finally block.
🧹 Nitpick comments (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (1)
140-149: Consider broader exception handling in startRecordingByCall.The method only catches
MessagesException. If an unexpectedRuntimeExceptionoccurs during recording start, it will propagate uncaught. While this is already flagged in theonServiceConnectedcontext, consider wrapping the entire body in try-catch to ensure all exceptions are handled consistently.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/MessagesException.java(2 hunks)android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (2)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/Messages.java (1)
Messages(3-16)android/src/main/java/com/tchvu3/capacitorvoicerecorder/ResponseGenerator.java (1)
ResponseGenerator(5-37)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Android Verification
🔇 Additional comments (8)
android/src/main/java/com/tchvu3/capacitorvoicerecorder/MessagesException.java (1)
3-4: LGTM: Imports support new JSON serialization.The added imports are necessary for the new
toJSObject()method and its helper.android/src/main/java/com/tchvu3/capacitorvoicerecorder/VoiceRecorder.java (7)
36-36: LGTM: Simplified state tracking with isConnected flag.The boolean flag simplifies the previous
serviceStatetracking. Note that this field is not markedvolatileor synchronized, so ensure all reads and writes occur on the same thread (main thread) to avoid visibility issues.
53-57: LGTM: Proper cleanup on service disconnection.The callback correctly resets the connection state and clears the service reference when the service disconnects unexpectedly.
59-64: LGTM: Updated to use isConnected flag.The method correctly uses the new
isConnectedflag instead of the previousserviceStateenum.
66-71: LGTM: Simplified load() binds to existing service.The method now only binds to the service without state management, which is appropriate for the plugin load lifecycle.
188-194: LGTM: Proper unbind with finally block.The try-catch-finally structure ensures
isConnectedis always set tofalse, even ifunbindServicethrowsIllegalArgumentException. The error logging is helpful for debugging.
181-181: LGTM: Structured error reporting with toJSObject().Using
e.toJSObject()provides richer error information to JavaScript consumers, including the error message and cause chain.
197-233: LGTM: Pause, resume, and status methods use updated helper.These methods correctly use the updated
getMediaRecorder()helper, which now checks theisConnectedflag. The logic remains sound.
Summary by CodeRabbit
Bug Fixes
New Features
Chores