fix: add recording foreground service and fix call interruption, stop record when kill app or crash app#758
Conversation
📝 WalkthroughWalkthroughAdds platform-wide crash-resilient WAV recording with Android foreground-service recording, a WavRecorder, WAV→M4A conversion utilities, recovery APIs (restorePendingRecordings, restoreRecording), audio-focus and lifecycle handling, and supporting documentation. Changes
Sequence Diagram(s)sequenceDiagram
participant App as App Client
participant FG as RecordingForegroundService
participant Wav as WavRecorder
participant Timer as Progress Timer
App->>FG: startRecording(params)
activate FG
FG->>Wav: startRecording(path, config)
activate Wav
Wav-->>FG: started
FG->>Timer: start periodic updates
activate Timer
loop every subscriptionDuration
Timer->>Wav: getMaxAmplitude(), getCurrentDuration()
Wav-->>Timer: amplitude, duration
Timer->>App: onRecordingUpdate(amplitude,duration)
end
App->>FG: pauseRecording()
FG->>Wav: pauseRecording()
App->>FG: resumeRecording()
FG->>Wav: resumeRecording()
App->>FG: stopRecording()
FG->>Wav: stopRecording()
Wav-->>FG: wavFilePath
FG->>App: wavFilePath / converted M4A
deactivate Timer
deactivate Wav
deactivate FG
sequenceDiagram
participant Hybrid as HybridSound
participant FS as File System
participant Wav as WavRecorder (repair)
participant Conv as WavToM4aConverter
participant Codec as MediaCodec/AVAssetWriter
Hybrid->>FS: locate .wav files (restorePendingRecordings)
loop per wav file
Hybrid->>Wav: repairWavFile(wavPath)
Wav-->>Hybrid: repaired
Hybrid->>Conv: convert(wavPath)
activate Conv
Conv->>Codec: encode PCM→AAC + mux
Codec-->>Conv: finished
Conv->>FS: validate output & (optionally) delete wav
Conv-->>Hybrid: Success(path,duration) / Error
end
Hybrid-->>Caller: Array<RestoredRecording>
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello @hongdtt-ominext, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly upgrades the audio recording functionality, making it far more resilient to interruptions and unexpected app terminations across both Android and iOS. By introducing a crash-resilient WAV recording mode, robust audio focus management, and a comprehensive recovery mechanism, the changes ensure that users' recordings are preserved and can be restored even in challenging scenarios. The update also includes detailed documentation to guide developers in leveraging these new capabilities for a more reliable recording experience. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 14
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
🤖 Fix all issues with AI agents
In
`@android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt`:
- Around line 388-400: The acquireWakeLock() method creates a PARTIAL_WAKE_LOCK
with a 10-hour timeout (wakeLock, WAKE_LOCK_TAG) which is too long; change to a
shorter timeout (e.g., 30–60 minutes) and implement renewal while recording is
active and prompt release when recording stops. Update acquireWakeLock() to use
a reduced timeout constant, add logic (e.g., a renewWakeLock() helper or a
scheduled Runnable) to re-acquire or extend the lock periodically during active
recording, and ensure the lock is released immediately in the recording stop
path and in onDestroy() if still held; reference the wakeLock variable and
WAKE_LOCK_TAG to locate and modify the code.
- Around line 230-244: resumeRecording() currently calls startRecordTimer(60L)
which leaves the timer wrong; replace the hardcoded 60L with the stored
subscriptionDurationMs field value (convert to expected units for
startRecordTimer if necessary—for example subscriptionDurationMs / 1000 if
startRecordTimer expects seconds). Update resumeRecording() to read
subscriptionDurationMs and pass that value to startRecordTimer so the resumed
session uses the correct duration; reference wavRecorder.resumeRecording(),
startRecordTimer(...) and subscriptionDurationMs when locating the change.
- Around line 246-250: In stopRecording(), remove the redundant call to
wavRecorder?.stopRecording() and instead rely on stopRecordingInternal() to stop
and return the path (adjust stopRecordingInternal() to return the stop path if
needed) so stopRecording() simply delegates; in resumeRecording(), replace the
hardcoded startRecordTimer(60L) with the subscriptionDuration value used by
startRecording() (use the same parameter or field) so both use the same timer;
in createNotification(), null-check the result of
packageManager.getLaunchIntentForPackage(...) and provide a safe fallback
PendingIntent (e.g., create an intent to launch the current activity or use a
no-op intent) before calling PendingIntent.getActivity(); finally, reduce the
WakeLock acquire timeout from 10 * 60 * 60 * 1000L to a more reasonable duration
(e.g., 10 * 60 * 1000L) and ensure WakeLock release paths remain correct
(references: stopRecording(), stopRecordingInternal(), resumeRecording(),
startRecordTimer(), startRecording(), createNotification(),
packageManager.getLaunchIntentForPackage(), PendingIntent.getActivity(),
WakeLock.acquire()).
In `@android/src/main/java/com/margelo/nitro/audiorecorderplayer/Sound.kt`:
- Around line 53-71: The onRecordingUpdate callback is being assigned twice
(once in the ServiceConnection object’s onServiceConnected and again inside
startRecorder), which can race and overwrite handlers; remove the duplicate
assignment from startRecorder (the block that sets
recordingService?.onRecordingUpdate around lines ~219-230) and rely on the
ServiceConnection’s setup (serviceConnection / onServiceConnected and
RecordingForegroundService.RecordingBinder) to set onRecordingUpdate once,
ensuring any calls in startRecorder instead check isServiceBound or use the
existing recordingService without reassigning the callback.
- Around line 700-730: The duration estimation currently hardcodes 44100 Hz mono
math in the WAV fallback (around the WavRecorder.repairWavFile /
WavToM4aConverter.convert error path), which is wrong for other presets; instead
parse the WAV header to get the actual byteRate (or sampleRate, channels,
bitsPerSample) and data chunk offset and compute estimatedDurationMs =
(file.length() - dataChunkOffset) / byteRate * 1000. Update the error branch
that builds RestoredRecording to call a small helper (e.g., parseWavHeader or
getWavByteRateAndDataOffset) to extract byteRate/data offset from the WAV file
and use those values for the duration calculation so stereo/48k WAVs are
estimated correctly.
- Around line 200-276: The Thread.sleep(300) race should be removed and
recording start moved into the ServiceConnection.onServiceConnected flow:
instead of calling RecordingForegroundService.getInstance() after sleep, capture
the recording parameters and the Promise into a pending holder (e.g.,
pendingRecordingParams and pendingRecordingPromise) before binding, then in
serviceConnection.onServiceConnected (where you set recordingService and
isServiceBound) call a helper like startRecordingOnService to perform the same
start logic (setup onRecordingUpdate, call recordingService.startRecording,
resolve/reject the pending promise and handle unbind/stop on failure). Keep the
existing cleanup (context.unbindService, isServiceBound flag,
RecordingForegroundService.stop) but invoke it from the
helper/onServiceConnected paths rather than relying on getInstance() +
Thread.sleep.
In `@android/src/main/java/com/margelo/nitro/audiorecorderplayer/WavRecorder.kt`:
- Around line 247-294: stopRecording() currently joins recordingThread with a 1s
timeout and then proceeds to close outputStream and release audioRecord while
the thread may still be running; change the logic so that after
recordingThread?.join(1000) you check if recordingThread?.isAlive == true and if
so call recordingThread?.interrupt() and then join again (e.g., join another
short timeout or until termination) before closing/releasing resources; ensure
the thread implementation honors interruption and checks
isRecording/Thread.currentThread().isInterrupted so it can finish any pending
writes safely; keep updateWavHeader(path, totalBytesWritten) and the final
logging only after the thread has fully terminated.
- Around line 29-53: The fields isRecording, isPaused, totalBytesWritten, and
lastMaxAmplitude are accessed from both the recording thread (recordingLoop) and
caller threads (stopRecording, pauseRecording, getMaxAmplitude) and need
cross-thread visibility; mark these fields as `@Volatile` (or otherwise
synchronize reads/writes) so updates in stopRecording/pauseRecording are seen by
the recordingLoop and getMaxAmplitude returns consistent values, ensuring
thread-safety for those variables.
In
`@android/src/main/java/com/margelo/nitro/audiorecorderplayer/WavToM4aConverter.kt`:
- Around line 60-146: The convert and convertSync methods duplicate WAV
validation, conversion, size-checking and deletion logic; extract that shared
flow into a single private helper (e.g., private suspend fun
performConversion(wavFilePath: String, outputPath: String, bitRate: Int,
deleteWavAfterConversion: Boolean): ConversionResult) that calls parseWavHeader,
encodeToM4a, performs the "output too small" check (normalize the threshold so
both callers use the same condition), computes durationMs, handles output/input
file verification, deletes the WAV only after validation and logs/errors
consistently, and have convert and convertSync both delegate to this helper
while preserving their signatures and return types (ConversionResult) and
exception logging semantics.
In `@ios/Sound.swift`:
- Around line 1336-1360: In the .began interruption handling block where
audioRecorder is stopped, include the recorder file URL in the RecordBackType
callback so JS can access the partial recording (refer to the .began case,
audioRecorder, recordBackListener and RecordBackType); specifically, capture
recorder.url before nulling audioRecorder and add a filePath/fileUri field to
the RecordBackType payload (or populate an existing field) so the interruption
callback returns the file path; alternatively, if you prefer not to change
RecordBackType, call restorePendingRecordings() (reference
restorePendingRecordings) or otherwise surface the pending file URI to JS
immediately before setting audioRecorder = nil.
- Around line 819-823: The current duration estimate uses a hardcoded constant
(Double(fileSize - 44) / 88.2) which assumes 44.1kHz mono 16-bit and is wrong
for other recording presets; update the logic around wavPath and
estimatedDuration to read the WAV header's byteRate (4 bytes at offset 28,
little-endian) and compute duration as (fileSize - headerSize) / byteRate *
1000, falling back to a safe alternative (e.g., AVAudioFile.duration or
documenting the limitation) if the header cannot be read; ensure the change
replaces both occurrences of the hardcoded formula (the estimatedDuration
computation and the duplicate at the other occurrence) and handle file handle
errors and byteRate==0 gracefully.
- Around line 1086-1090: The current cleanup scheduled with
DispatchQueue.global(...).asyncAfter that removes tempFile after 1 second can
corrupt playback because AVAudioPlayer streams from disk; instead make the
tempFile URL an instance property (e.g., store the tempFile in the Sound class
or the player wrapper), remove the DispatchQueue.asyncAfter block, and perform
deletion in stopPlayer() (and also when the player is deinitialized or errors)
so that the file is removed only when AVAudioPlayer is stopped/released; update
stopPlayer() to check the stored tempFile URL, delete it with
FileManager.default.removeItem(at:), and nil out the stored tempFile reference.
In `@ios/WavToM4aConverter.swift`:
- Around line 85-101: The code uses deprecated synchronous AVAsset APIs
(asset.tracks(withMediaType:), audioTrack.formatDescriptions and asset.duration)
and also silently falls back to default sampleRate/channels if
CMAudioFormatDescription fails; update to use AVAsset.load(.tracks) or audio
asset.load(.tracks) and asynchronous CMAudioFormatDescription loading via
audioTrack.load(.formatDescriptions) or AVAsset.load(.duration) with await/Task
(iOS 16+), guard the loaded values and return a clear .error when
formatDescriptions or stream basic description is missing instead of defaulting
to 44100/1, and update callers of duration to await asset.load(.duration) as
well; locate these changes around AVAsset initialization, the audioTrack usage
and the extraction of sourceFormat/sampleRate/channels in
WavToM4aConverter.convert (the variables asset, audioTrack, formatDescription,
sourceFormat, sampleRate, channels).
In `@package.json`:
- Line 4: The package.json currently sets "private": true which prevents npm
publish and breaks the release-it/npm.publish workflow; remove the "private" key
(or set it to false) so the repository can be published, and verify that
publishConfig and release-it's npm.publish remain as intended; ensure no other
CI or branch-protection step relies on "private": true before merging.
🟡 Minor comments (7)
src/specs/Sound.nitro.ts-115-116 (1)
115-116:⚠️ Potential issue | 🟡 MinorLint/Prettier formatting violations will fail CI.
The lint check flags formatting issues on Lines 116 and 149–152. These need to be reformatted to pass the pipeline.
Suggested fix (Line 116)
-export interface AudioSet - extends IOSAudioSet, AndroidAudioSet, CommonAudioSet {} +export interface AudioSet + extends IOSAudioSet, + AndroidAudioSet, + CommonAudioSet {}Suggested fix (Lines 149-152)
-export interface Sound extends HybridObject<{ - ios: 'swift'; - android: 'kotlin'; -}> { +export interface Sound + extends HybridObject<{ + ios: 'swift'; + android: 'kotlin'; + }> {As per coding guidelines, "Use 2-space indentation, single quotes, and es5 trailing commas in TypeScript code (enforced by Prettier/ESLint)".
Also applies to: 149-152
android/src/main/java/com/margelo/nitro/audiorecorderplayer/WavRecorder.kt-69-95 (1)
69-95:⚠️ Potential issue | 🟡 MinorInteger truncation in
repairWavFilefor large recordings.Lines 82 and 86 call
.toInt()onLongvalues. For recordings exceeding ~24 minutes of stereo 44.1kHz/16-bit audio (~254 MB data), the data size exceedsInt.MAX_VALUE(2,147,483,647 bytes), producing a corrupt header. This is an inherent WAV format limitation (the RIFF size fields are 32-bit), but the truncation here is silent — the method returnstrue(success) even though the header is wrong.Consider logging a warning when
dataSize > Int.MAX_VALUEso callers are aware:Suggested improvement
val dataSize = file.length() - WAV_HEADER_SIZE val fileSize = dataSize + WAV_HEADER_SIZE - 8 + + if (dataSize > Int.MAX_VALUE) { + Logger.w("[WavRecorder] WAV file exceeds 2GB data limit, header may be inaccurate") + }package.json-88-88 (1)
88-88:⚠️ Potential issue | 🟡 MinorRemove unused
baseline-browser-mappingdependency.This package is not referenced anywhere in the codebase and appears unrelated to the PR's objectives (foreground service, WAV recording, crash recovery). It should be removed from
package.json.docs/ANDROID_RECORDING_STATE_FIX.md-16-161 (1)
16-161:⚠️ Potential issue | 🟡 MinorDocumentation code snippets reference
MediaRecorderbut the implementation usesWavRecorder.The code examples throughout this document show
mediaRecorder?.pause(),mediaRecorder?.resume(),mediaRecorder?.stop(), etc., but the actualRecordingForegroundService.ktdelegates all recording toWavRecorder(AudioRecord-based). This mismatch will mislead contributors who read the doc and then look at the code.Consider updating the snippets to reflect the real implementation, or add a note that these are simplified/conceptual illustrations.
android/src/main/java/com/margelo/nitro/audiorecorderplayer/WavToM4aConverter.kt-117-117 (1)
117-117:⚠️ Potential issue | 🟡 MinorDivision by zero if
wavHeader.byteRateis 0 (corrupt or repaired WAV).Lines 117 and 199 calculate
(wavHeader.dataSize * 1000L) / wavHeader.byteRate. If the WAV file is corrupt or has a damaged header,byteRatecould be 0, causing anArithmeticException.Proposed fix
-val durationMs = (wavHeader.dataSize * 1000L) / wavHeader.byteRate +val durationMs = if (wavHeader.byteRate > 0) { + (wavHeader.dataSize * 1000L) / wavHeader.byteRate +} else { + 0L +}Also applies to: 199-199
android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt-354-378 (1)
354-378:⚠️ Potential issue | 🟡 Minor
getLaunchIntentForPackagecan returnnull, which would passnulltoPendingIntent.getActivity.On some devices or configurations,
packageManager.getLaunchIntentForPackage(packageName)may returnnull. Passing it toPendingIntent.getActivitycould cause unexpected behavior.Proposed fix
private fun createNotification(contentText: String = "Tap to return to app"): Notification { val packageName = packageName val launchIntent = packageManager.getLaunchIntentForPackage(packageName) + ?: Intent() // Fallback to empty intent val pendingIntentFlags = if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.M) {ios/Sound.swift-1246-1263 (1)
1246-1263:⚠️ Potential issue | 🟡 MinorAdd missing
import UIKitto supportUIApplication.willTerminateNotification.
UIApplicationandUIApplication.willTerminateNotificationare part of the UIKit framework. The file currently only importsFoundation,AVFoundation, andNitroModulesbut is missing the explicit UIKit import required for this code to compile. Addimport UIKitat the top of the file.
🧹 Nitpick comments (13)
ios/WavToM4aConverter.swift (2)
170-196: Semaphore blocks calling thread — potential deadlock risk if called from the main queue.
convertSyncuses twoDispatchSemaphore.wait()calls. If this is ever called from the main thread, and the writer's callback dispatches to main, it will deadlock. Theconvert()wrapper at Line 39 dispatches to.userInitiated, so the typical path is safe, butconvertSyncisstaticand public, making it callable from anywhere.Consider adding a precondition or documenting that
convertSyncmust not be called on the main thread.
238-249: Duration validation only runs whendeleteWavAfterConversionis true.The duration integrity check (comparing output M4A duration to source WAV duration) is gated by
deleteWavAfterConversion. If the caller passesfalse, a silently truncated M4A could be returned assuccesswithout any warning. Consider always performing the validation and logging a warning on mismatch, regardless of the deletion flag.docs/CALL_INTERRUPTION_ANALYSIS.md (2)
146-162: Add language specifiers to fenced code blocks for diagrams.Several diagram/ASCII-art code blocks lack a language specifier (Lines 146, 186, 213, 259), which triggers markdownlint MD040. Use
```textfor these blocks to satisfy the linter and improve rendering in some Markdown processors.Also applies to: 186-203, 213-229, 259-286
547-551: Blank line inside blockquote creates two separate blockquotes.The blank line between the two quoted references (Line 549) splits them into separate blockquotes per Markdown spec, triggering MD028.
Suggested fix
> "When an app is force-stopped, the entire process is killed instantly. Standard lifecycle methods such as onStop() and onDestroy() are not guaranteed to run." > — [Android Developer Documentation](https://developer.android.com/guide/components/activities/process-lifecycle) - +> > "The system provides no notification when an app is terminated while in a suspended state." > — [Apple Developer Documentation](https://developer.apple.com/documentation/uikit/uiapplicationdelegate/applicationwillterminate(_:))android/src/main/java/com/margelo/nitro/audiorecorderplayer/WavRecorder.kt (1)
505-525: Cleanup swallows exceptions without any logging.Per detekt hints, Lines 512 and 519 catch and discard exceptions. While cleanup code is inherently best-effort, a debug-level log helps diagnose issues during development.
Suggested fix
try { audioRecord?.stop() audioRecord?.release() } catch (e: Exception) { - // Ignore + Logger.d("[WavRecorder] Cleanup: ${e.message}") } audioRecord = null try { outputStream?.close() } catch (e: Exception) { - // Ignore + Logger.d("[WavRecorder] Cleanup: ${e.message}") }docs/M4A_DATA_LOSS_EXPLAINED.md (1)
20-29: Missing blank lines around headings (MD022) and unlabeled code blocks (MD040).Lines 20 and 26 need a blank line before the heading. Additionally, diagram code blocks (Lines 37, 90, 106, 142, 162, 196) should use
```textto satisfy markdownlint MD040.docs/ANDROID_RECORDING_STATE_FIX.md (1)
174-193: Markdown formatting: missing blank lines around headings and fenced code blocks, and missing language specifiers.Static analysis flagged multiple markdownlint issues in the testing scenarios section (and earlier headings). Headings need surrounding blank lines (MD022), fenced code blocks need surrounding blank lines (MD031), and code blocks should specify a language (MD040).
Example fix for one scenario block
### ✅ Scenario 1: Phone Call → Resume + -``` +```text Start recording → Call arrives → Pause → Call ends → Resume → OK
✅ Scenario 2: Phone Call → Stop
Apply the same pattern to all scenario blocks and headings under "Important Notes". </details> </blockquote></details> <details> <summary>android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt (2)</summary><blockquote> `61-78`: **Static singleton `instance` pattern is fragile for an Android `Service`.** Storing the service instance in a `companion object` and accessing it via `getInstance()` is a common but fragile pattern. If the system recreates the service (e.g., after a low-memory kill with `START_STICKY`), there's a window where `getInstance()` returns `null` while the service is being recreated. The `Sound.kt` code relies on `Thread.sleep(300)` + `getInstance()` to work around this, which is inherently racy. Prefer relying solely on the `ServiceConnection` binding for communication. --- `85-95`: **WakeLock is acquired in `onCreate` regardless of whether recording actually starts.** The service acquires a wake lock immediately in `onCreate`, but recording may not start successfully. Consider acquiring the wake lock only when recording begins and releasing it when recording stops, rather than tying it to the service lifecycle. </blockquote></details> <details> <summary>ios/Sound.swift (1)</summary><blockquote> `429-463`: **`stopRecorder` calls `WavToM4aConverter.convertSync` with `deleteWavAfterConversion: true` — verify the WAV-to-M4A conversion is tested for edge cases.** If the WAV file is very short (e.g., interrupted immediately), the conversion might produce a corrupt M4A. The fallback (returning WAV path) handles this, but the user-facing contract changes from M4A to WAV silently, which could break downstream consumers expecting a specific format. </blockquote></details> <details> <summary>android/src/main/java/com/margelo/nitro/audiorecorderplayer/WavToM4aConverter.kt (1)</summary><blockquote> `437-453`: **Empty catch blocks in `finally` swallow exceptions silently — add minimal logging.** While it's reasonable to catch and ignore cleanup exceptions in a `finally` block, completely swallowing them makes debugging harder. At minimum, use named exceptions per detekt conventions or add a brief log. <details> <summary>Proposed fix</summary> ```diff } finally { try { inputStream?.close() - } catch (e: Exception) { } + } catch (e: Exception) { + Logger.w("[WavToM4a] Error closing input stream: ${e.message}") + } try { encoder?.stop() encoder?.release() - } catch (e: Exception) { } + } catch (e: Exception) { + Logger.w("[WavToM4a] Error releasing encoder: ${e.message}") + } try { if (muxerStarted) { muxer?.stop() } muxer?.release() - } catch (e: Exception) { } + } catch (e: Exception) { + Logger.w("[WavToM4a] Error releasing muxer: ${e.message}") + } }android/src/main/java/com/margelo/nitro/audiorecorderplayer/Sound.kt (2)
869-926: RequestingAUDIOFOCUS_GAINfor a recorder is unconventional and may disrupt other apps' audio.Audio focus is a playback concept; recording apps typically don't request it. By requesting
AUDIOFOCUS_GAIN, you may cause music players and other audio apps to pause unnecessarily when the user starts recording. If the goal is to detect phone call interruptions, consider usingTelephonyManager/PhoneStateListenerinstead, or at minimum useAUDIOFOCUS_GAIN_TRANSIENTto signal temporary usage.
102-102: UnscopedCoroutineScope(Dispatchers.IO).launch— coroutines outlive the object.Multiple methods (e.g.,
startRecorder,stopRecorder,restorePendingRecordings) create ad-hocCoroutineScopeinstances. These coroutines are not cancelled when theHybridSoundobject is destroyed, which can cause callbacks on garbage-collected objects. Consider using a class-levelCoroutineScopewithSupervisorJob()that can be cancelled in a cleanup/dispose method.
android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt
Show resolved
Hide resolved
android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt
Show resolved
Hide resolved
android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt
Show resolved
Hide resolved
android/src/main/java/com/margelo/nitro/audiorecorderplayer/Sound.kt
Outdated
Show resolved
Hide resolved
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces significant enhancements to audio recording functionality on both Android and iOS, primarily focusing on crash resilience and interruption handling. On Android, a new RecordingForegroundService and WavRecorder are implemented to manage background audio recording in WAV format, utilizing a WakeLock and foreground notifications to ensure continuous operation and prevent system termination. A WavToM4aConverter is added to convert these crash-resilient WAV files to smaller M4A format upon successful completion or recovery. The main HybridSound module is refactored to delegate recording tasks to this new service, incorporating audio focus management to handle interruptions like phone calls by pausing the recording. Similarly, on iOS, recording now defaults to WAV format, and interruption observers are set up to stop and finalize recordings gracefully during audio session interruptions or app termination. Both platforms gain new restorePendingRecordings and restoreRecording methods, allowing users to recover incomplete WAV recordings by repairing their headers and converting them to M4A. Documentation has been updated to explain these new features, including the rationale behind WAV for crash resilience and detailed usage guides. Review comments highlight concerns about using Thread.sleep() for service binding on Android, recommending an asynchronous, event-driven approach; potential path traversal vulnerabilities in restorePendingRecordings and restoreRecording due to direct path usage; incorrect WakeLock management leading to battery drain; synchronous network calls in setupEnginePlayer on iOS that could cause performance issues or SSRF vulnerabilities; a hardcoded timer duration in resumeRecording on Android; and redundant code and inconsistent logging practices in the WavRecorder class.
android/src/main/java/com/margelo/nitro/audiorecorderplayer/Sound.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt
Outdated
Show resolved
Hide resolved
android/src/main/java/com/margelo/nitro/audiorecorderplayer/WavRecorder.kt
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Actionable comments posted: 7
🤖 Fix all issues with AI agents
In
`@android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt`:
- Around line 94-97: onStartCommand currently returns START_STICKY which can
restart the service after process death leaving a foreground notification with
no active recorder (wavRecorder = null); change the return value to
START_NOT_STICKY in RecordingForegroundService.onStartCommand and ensure
createNotification or startForeground is only called when wavRecorder is
non-null (or guard startForeground with a check) so the service is not restarted
as a zombie and the notification accurately reflects active recording state.
In `@android/src/main/java/com/margelo/nitro/audiorecorderplayer/Sound.kt`:
- Around line 246-264: If startRecorder is called while the service is still
connecting, the code currently overwrites
pendingRecordingParams/pendingRecordingPromise which can orphan the first
promise; before assigning pendingRecordingParams and pendingRecordingPromise in
the else branch, check if pendingRecordingPromise is non-null and reject it (or
resolve with a clear error) so the first caller is not left hanging, then assign
the new params and promise; keep using the existing
serviceConnection/handler.post flow (RecordingForegroundService.start,
context.bindService, setupAudioFocus) and ensure you clear or replace
pendingRecordingPromise when startRecordingOnService(recordingService, params,
promise) is called in onServiceConnected.
In `@android/src/main/java/com/margelo/nitro/audiorecorderplayer/WavRecorder.kt`:
- Around line 69-95: The code currently casts fileSize/dataSize to Int
(intToByteArray(fileSize.toInt()) and intToByteArray(dataSize.toInt())), which
silently overflows for WAVs >2GB; update repairWavFile (and updateWavHeader) to
write the lower 32 bits of the Long values as an unsigned little-endian 4-byte
sequence instead of casting to Int (create a helper like
longToLittleEndian4Bytes(value: Long) that produces 4 bytes from value &
0xFFFFFFFFL), use that helper when writing the RIFF/data chunk sizes, and add a
guard/log if dataSize > 0xFFFFFFFFL to surface potential truncation (or
explicitly clamp to 0xFFFFFFFFL) rather than relying on toInt().
- Around line 351-386: finalizeOnKill currently flips isRecording and
immediately closes audio resources while the recording thread may still be
writing; mark the isRecording flag as `@Volatile` to ensure visibility to the
recorder thread, then after setting isRecording = false and isPaused = false,
wait for the recording thread to finish by calling join with a short timeout
(e.g., recordingThread?.join(200) or similar) before calling
audioRecord?.stop(), audioRecord?.release(), or closing outputStream; handle
InterruptedException/InterruptedThrowable and proceed to close resources if the
join times out, preserving the existing updateWavHeader(filePath,
totalBytesWritten) call.
In `@ios/Sound.swift`:
- Around line 768-846: The iOS recovery methods accept arbitrary file paths —
add a path traversal/sandbox guard similar to Android by implementing a
validatePathSecurity(_:) helper that resolves symlinks and ensures the canonical
path starts with one of the allowed roots (documentDirectory, cachesDirectory,
NSTemporaryDirectory), then call this helper at the start of
restorePendingRecordings(directory:) and restoreRecording(wavFilePath:) to
reject any path outside those roots (reject the Promise with a RuntimeError and
return) before performing file operations; ensure you check both provided
directory strings and individual wavFilePath inputs and reuse the same
validatePathSecurity symbol for consistency.
In `@ios/WavToM4aConverter.swift`:
- Around line 146-152: The current code silently falls back to defaults when
CMAudioFormatDescriptionGetStreamBasicDescription(formatDescription) yields nil
(sourceFormat), which can corrupt output; instead, when sourceFormat is nil, log
an error with context and immediately abort the conversion by returning .error
(do not continue using sampleRate/channels defaults). Update the block around
CMAudioFormatDescriptionGetStreamBasicDescription/ sourceFormat (and any
downstream use of sampleRate/channels) to early-return .error and avoid
producing an M4A when the source format cannot be determined.
- Around line 70-74: The code sets outputPath by calling replacingOccurrences on
wavFilePath which replaces all ".wav" substrings (including directory names);
change this to replace only the file extension: if m4aFilePath is nil, build the
output path by using wavFilePath's file-extension-safe API (e.g.,
URL(fileURLWithPath:).deletingPathExtension().appendingPathExtension("m4a") or
NSString(string: wavFilePath).deletingPathExtension + ".m4a") so the directory
components are preserved; update the assignment that defines outputPath in
WavToM4aConverter.swift (references: outputPath, wavFilePath, m4aFilePath)
accordingly.
🧹 Nitpick comments (5)
android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt (2)
280-288: Swallowed exception incleanupRecorderloses diagnostic context.The
catchblock at line 283 silently discards the exception. At minimum, log it so crash-path debugging is not hampered.Proposed fix
private fun cleanupRecorder() { try { wavRecorder?.stopRecording() } catch (e: Exception) { - // Ignore + Logger.w("[ForegroundService] Error during recorder cleanup: ${e.message}") } wavRecorder = null currentRecordingPath = null }
406-455: WakeLock renewal creates a new instance each cycle — consider re-acquiring on the same lock.Each renewal releases the old lock, creates a brand-new
PowerManager.WakeLock, and acquires it. You could simplify by callingacquire(timeout)on the existing (released) lock, but the current approach is functionally correct and the overhead is negligible. No action required.android/src/main/java/com/margelo/nitro/audiorecorderplayer/WavRecorder.kt (1)
498-518: Swallowed exceptions incleanup()lose diagnostic info.Per detekt hints, both catch blocks discard the exception. Since
cleanup()is called on error paths, logging helps diagnose initialization failures.Proposed fix
try { audioRecord?.stop() audioRecord?.release() } catch (e: Exception) { - // Ignore + Logger.w("[WavRecorder] Error during cleanup: ${e.message}") } audioRecord = null try { outputStream?.close() } catch (e: Exception) { - // Ignore + Logger.w("[WavRecorder] Error closing stream during cleanup: ${e.message}") }ios/WavToM4aConverter.swift (1)
93-116: Semaphore + Task bridge for iOS 16+ async APIs is functional but carries a subtle risk.Blocking a GCD thread with
semaphore.wait()while theTaskruns on the cooperative thread pool works in practice, but Apple discourages mixing GCD semaphores with Swift concurrency because of potential thread starvation under heavy load. Since this runs on.userInitiatedand is short-lived, the risk is low — just flagging it for awareness.ios/Sound.swift (1)
99-120: Forced.wavextension on user-provided URI may surprise callers expecting their original extension.The user's
.m4a(or other) extension is silently replaced with.wav. While this is necessary for the crash-resilient WAV recording strategy, thestartRecorderpromise resolves with the.wavpath (line 252). Callers relying on the returned path should be fine, but callers comparing the passed-in URI with the resolved path may be confused. This is documented implicitly by the conversion instopRecorder, so just noting it.
android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt
Show resolved
Hide resolved
android/src/main/java/com/margelo/nitro/audiorecorderplayer/WavRecorder.kt
Show resolved
Hide resolved
android/src/main/java/com/margelo/nitro/audiorecorderplayer/WavRecorder.kt
Show resolved
Hide resolved
- Replace Thread.sleep(300) race condition with event-driven service binding via onServiceConnected callback and pending params pattern - Add path traversal validation for restorePendingRecordings and restoreRecording to prevent unauthorized filesystem access - Move WakeLock acquire/release to recording lifecycle (start/pause/ resume/stop) instead of entire service lifetime to reduce battery drain - Reduce WakeLock timeout from 10h to 30min with automatic renewal - Use subscriptionDurationMs instead of hardcoded 60L in resumeRecording - Replace e.printStackTrace() with Logger calls for consistent logging - Remove duplicate intToByteArrayLE in WavRecorder, reuse companion object's intToByteArray - Fix redundant double stopRecording() call in ForegroundService - Null-check launchIntent in createNotification to prevent NPE - Fix temp audio file deleted after 1s corrupting remote playback on iOS; defer cleanup to stopPlayer/deinit - Read WAV header byte rate for accurate duration estimation instead of hardcoded 44100Hz mono 16-bit assumption (iOS + Android) - Replace deprecated AVAsset sync APIs with async load() on iOS 16+ - Document interruption callback behavior for iOS recording
hongdtt-ominext
force-pushed
the
conver-m4a-into-wav
branch
from
2b6b3e5 to
6db4b22
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
ios/Sound.swift (2)
507-511:⚠️ Potential issue | 🔴 CriticalForce unwrap on user-provided URI will crash on malformed input.
URL(string: uri)returnsnilfor URIs with unescaped spaces or certain special characters. Afile://prefixed path with spaces (e.g.,file:///path/to my file.m4a) will crash here.Proposed fix
if uri.hasPrefix("file://") { print("🎵 URI has file:// prefix") // Handle file:// URLs - url = URL(string: uri)! + guard let parsedURL = URL(string: uri) else { + promise.reject(withError: RuntimeError.error(withMessage: "Invalid file URL: \(uri)")) + return + } + url = parsedURL print("🎵 Created URL from string: \(url)")
247-252:⚠️ Potential issue | 🟡 Minor
startRecorderresolves with WAV path, butstopRecorderreturns M4A path after conversion — document this contract.
startRecorderresolves with the WAV file URL (line 252), butstopRecorderconverts to M4A and deletes the WAV (lines 448-451), resolving with the M4A path. If the caller persists the path fromstartRecorder, it will become a dangling reference afterstopRecordercompletes. This asymmetry should be documented in the API or the JS layer.
🤖 Fix all issues with AI agents
In `@android/src/main/java/com/margelo/nitro/audiorecorderplayer/Sound.kt`:
- Around line 246-267: The pendingRecordingPromise can hang if
context.bindService(...) fails; after calling context.bindService(intent,
serviceConnection, Context.BIND_AUTO_CREATE) check its boolean return and if it
is false immediately reject pendingRecordingPromise with a clear error, clear
pendingRecordingParams and pendingRecordingPromise, and undo any
started/registered work (e.g., stop or not-start
RecordingForegroundService.start and teardown audio focus via setupAudioFocus
cleanup) so there are no orphaned resources; if bindService returns true keep
the existing flow so onServiceConnected will use pendingRecordingParams and call
startRecordingOnService.
- Around line 676-684: The restorePendingRecordings logic currently scans all
.wav files (wavFiles) and can accidentally pick up the WAV for an in-progress
recording; update the filter to exclude the active recording file by checking
the recorder's current recording path or recording state. Specifically, in
restorePendingRecordings (Sound.kt) when creating wavFiles, add an exclusion:
skip any file whose path equals the recorder/recording service's
currentRecordingPath (or when recorder.isRecording() && file.path ==
recorder.getCurrentRecordingPath()), or if no path accessor exists, skip files
modified within the last few seconds to avoid the active file; ensure you
reference the recorder instance (e.g., recorder.isRecording(),
recorder.getCurrentRecordingPath()) and apply that predicate to the listFiles
filter so active recordings are never processed.
In `@android/src/main/java/com/margelo/nitro/audiorecorderplayer/WavRecorder.kt`:
- Around line 517-537: The cleanup() method currently wraps audioRecord?.stop()
and audioRecord?.release() in the same try block so if stop() throws release()
never runs; change cleanup() to use separate try/catch blocks for
audioRecord?.stop() and audioRecord?.release() (mirroring stopRecording()),
ensuring release() is always attempted even if stop() fails, then set
audioRecord = null and proceed to close outputStream in its own try/catch as
already done.
In `@ios/Sound.swift`:
- Around line 794-805: The restorePendingRecordings function currently converts
and deletes every .wav in the scanDirectory, risking data loss; update its
filtering logic to only target recordings created by this library (e.g., match
the naming convention used in startRecorder such as "sound_" prefix or a
specific UUID pattern) or restrict scanning to a dedicated subdirectory for
crash-resilient recordings; ensure the wavFiles collection is built using a
filter that checks URL.lastPathComponent.hasPrefix("sound_") (or the exact
prefix/pattern you use) and leave non-matching .wav files untouched, and
document/ensure startRecorder writes to that same prefix or subdirectory and
honors deleteWavAfterConversion only for matched files.
- Around line 924-935: The validatePathSecurity function currently uses
standardizingPath which doesn't resolve symlinks and allows path traversal via
symlink; update canonicalization to use resolvingSymlinksInPath for both the
input path (canonicalPath) and when normalizing allowedDirs entries (the map
over FileManager paths and NSTemporaryDirectory()) so you compare fully resolved
paths; ensure you still handle nils with compactMap and call standardizingPath
if needed after resolvingSymlinksInPath to normalize trailing components.
In `@package.json`:
- Line 87: Remove the unused devDependency "baseline-browser-mapping" from
package.json's devDependencies section by deleting the
"baseline-browser-mapping": "^2.9.11" entry, then update the lockfile (run npm
install or yarn install) to reflect the change and ensure no other references to
baseline-browser-mapping remain in the repo.
🧹 Nitpick comments (2)
ios/Sound.swift (2)
1464-1467: Timer invalidation indeinitmay execute off the main thread.
Timer.invalidate()must be called from the thread on which the timer was installed (main thread).deinitcan run on any thread. The existingstopTimer(for:)helper already handles this correctly with main-thread dispatch, butdeinitbypasses it.In practice, the
[weak self]captures in timer closures provide a safety net (timers self-stop whenselfis nil), so this is unlikely to cause issues, but it's worth aligning with the rest of the code.Proposed fix: reuse existing helpers
deinit { `#if` DEBUG print("🎙️ HybridSound deinit called - cleaning up resources") `#endif` // Remove notification observer removeInterruptionObserver() - // Stop and invalidate timers - recordTimer?.invalidate() - recordTimer = nil - playTimer?.invalidate() - playTimer = nil + // Stop and invalidate timers on the correct thread + stopRecordTimer() + stopPlayTimer()
1148-1150: Misleading comment: this is not streaming — it loads the entire resource into memory first.
Data(contentsOf: audioURL)is a synchronous, full-content download. The comment says "streaming download" but it's a bulk load followed by a write to disk. Consider updating the comment to reflect reality, and note that this will block the background thread and consume peak memory equal to the file size.
android/src/main/java/com/margelo/nitro/audiorecorderplayer/WavRecorder.kt
Show resolved
Hide resolved
- Guard against division by zero when byteRate is 0 in WavToM4aConverter - Fix Prettier formatting violations in Sound.nitro.ts - Remove unused baseline-browser-mapping devDependency - Add missing import UIKit for UIApplication.willTerminateNotification - Add debug/warning logging to empty catch blocks in WavRecorder and WavToM4aConverter cleanup/finally - Rewrite ANDROID_RECORDING_STATE_FIX.md to reference WavRecorder instead of MediaRecorder - Add language specifiers to diagram code blocks in all docs (MD040) - Fix blockquote spacing in CALL_INTERRUPTION_ANALYSIS.md (MD028) Co-authored-by: Cursor <cursoragent@cursor.com>
1 participant
Features & Fixes
iOS (Sound.swift): Added AVAudioSession.interruptionNotification observer
Android (Sound.kt): Added AudioFocusRequest handling
Android (WavRecorder.kt - NEW): Full AudioRecord-based WAV recorder
iOS (Sound.swift): WAV recording via AVAudioEngine / AVAudioRecorder with LPCM format
Summary by CodeRabbit
New Features
Improvements
Documentation