Add support for audio recording interruptions with segmentation #648
Conversation
|
@hyochan thoughts? |
|
I don't think all this is needed, simply removing |
|
I've tested this fix and observed that the stopRecorder function's promise takes around 20 seconds to resolve when there are 3–4 call interruptions during a 30-minute recording. This delay is quite significant. It would be great if the performance of this function could be optimized, especially for scenarios involving long recordings with interruptions. |
There was a problem hiding this comment.
Pull Request Overview
Enhance recording resilience by segmenting audio files around interruptions and merging them on stop.
- Introduce two native methods: one with an explicit
returnSegmentsflag and one without options to preserve backwards compatibility in JS bridging. - Initialize TS-side recorder/player state defaults and nullable subscriptions/callbacks.
- Update
stopRecorderin TS to route calls to the correct native method based on platform and optional flag.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ios/RNAudioRecorderPlayer.m | Add stopRecorderWithNoOptions and overload stopRecorder |
| index.ts | Initialize fields, make subscriptions nullable, refine stopRecorder, update play/pause returns |
Comments suppressed due to low confidence (4)
ios/RNAudioRecorderPlayer.m:23
- This selector exposes a
BOOLflag to JS; ensure your TypeScript definitions forRNAudioRecorderPlayerare updated to include this overload, otherwise calls from JS/TS may fail to resolve.
RCT_EXTERN_METHOD(stopRecorder:(BOOL)returnSegments
index.ts:296
- The JSDoc notes iOS-only behavior but doesn’t clarify Android’s default behavior. Add a remark on Android returning the merged file path or adjust the docs to reflect cross-platform behavior.
* @param {boolean} returnSegments - If true, return comma-separated list of segment file paths (iOS only)
index.ts:299
- New behavior around segmented recordings and branching by platform isn’t covered by existing tests. Consider adding unit or integration tests that simulate interruptions to verify both merge and segment-return flows.
stopRecorder = async (returnSegments?: boolean): Promise<string> => {
index.ts:299
- [nitpick] The optional
returnSegmentsflag name could be misinterpreted—perhaps rename it to something more descriptive likeshouldReturnSegmentPathsfor clarity.
stopRecorder = async (returnSegments?: boolean): Promise<string> => {
| reject:(RCTPromiseRejectBlock)reject); | ||
|
|
||
| RCT_EXTERN_METHOD(stopRecorder:(RCTPromiseResolveBlock)resolve | ||
| RCT_EXTERN_METHOD(stopRecorderWithNoOptions:(RCTPromiseResolveBlock)resolve |
There was a problem hiding this comment.
The rejecter: label deviates from the standard reject: used elsewhere in your extern declarations. Consider renaming it to reject: to maintain consistency and avoid bridging issues.
I would love to see a proper fix for this at this layer; we are using a pretty jank approach though with server side stitching so somebody else would probably have to take the lead on a real client fix |
@lokeshwebdev007 for short recordings it stitches on the client very fast. For longer recordings you will need to accept the delay or stitch server side. |
|
Hi @awallish, thanks again for this excellent PR — it's a very thoughtful solution to a real-world problem. We've recently been migrating the library to support TurboModules via NitroModules, and unfortunately, this PR was overlooked during that transition 😢 I'll definitely take a closer look and consider reimplementing this functionality based on your approach — though I have to admit, things are quite hectic on my end lately, so I can’t promise exactly when that'll happen 😅 Really appreciate your work and patience — it's on my radar! |
awallish
force-pushed
the
feature/audio-interruption-handling
branch
2 times, most recently
from
c1ef063 to
c5d560d
Compare
hyochan
force-pushed
the
main
branch
3 times, most recently
from
11feb3a to
14b6492
Compare
|
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. WalkthroughAdds optional returnSegments parameter to stopRecorder across platforms, introduces new iOS exports (with and without options), adds Android overload with centralized stop logic, improves Android recording file path generation and stop cleanup/validation, and updates JS class fields’ defaults/nullability and control-flow return strings. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant JS as AudioRecorderPlayer (JS)
participant Native as Native Module (iOS/Android)
participant Rec as Recorder Engine
App->>JS: stopRecorder(returnSegments?)
alt returnSegments provided
JS->>Native: stopRecorder(returnSegments)
else no option provided
JS->>Native: iOS: stopRecorderWithNoOptions()<br/>Android: stopRecorder(false)
end
Native->>Rec: stop / release
Rec-->>Native: stop result
alt Success
Native-->>JS: resolve(path or segments)
else Failure
Native-->>JS: reject(error)
end
JS-->>App: Promise settled
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
index.ts (1)
390-399: Detach playback listener on stop to prevent leaks/duplicates.
_playerSubscriptionis never cleared on normal stop; subsequent starts reuse the same subscription.stopPlayer = async (): Promise<string> => { if (this._isPlaying) { this._isPlaying = false; this._hasPaused = false; - - return RNAudioRecorderPlayer.stopPlayer(); + const res = await RNAudioRecorderPlayer.stopPlayer(); + if (this._playerSubscription) { + this._playerSubscription.remove(); + this._playerSubscription = null; + } + return res; }
🧹 Nitpick comments (12)
ios/RNAudioRecorderPlayer.swift (8)
47-52: Remove redundant optional initializations (SwiftLint).
= nilon optionals is unnecessary; drop it.- var interruptionResumeTimer: Timer? = nil - var lastInterruptionTime: Date? = nil + var interruptionResumeTimer: Timer? + var lastInterruptionTime: Date?
185-193: Fix AVAudioSession activation options.Use
.setActive(true)without.notifyOthersOnDeactivation(which is for deactivation).- try self.audioSession.setActive(true, options: .notifyOthersOnDeactivation) + try self.audioSession.setActive(true)
258-307: Avoid calling pause after stop during interruption.You already stop the recorder in the interruption-began path; the subsequent
pauseRecorderrejects due toisHandlingInterruptionand is noisy. Remove it.- } - - pauseRecorder { _ in } rejecter: { _, _, _ in } - break + }
332-334: Comment/time mismatch: use a sane resume delay.Comment says 1.0s but code uses 0.01s. Use 1.0s (or update the comment). The longer delay tends to be more reliable post-interruption.
- interruptionResumeTimer = Timer.scheduledTimer(withTimeInterval: 0.01, repeats: false) { [weak self] _ in + interruptionResumeTimer = Timer.scheduledTimer(withTimeInterval: 1.0, repeats: false) { [weak self] _ in
353-357: Silence unused closure parameters (SwiftLint).Use
_for unusedrejecterparams.- } rejecter: { code, message, error in + } rejecter: { _, _, _ in
307-307: Drop unneededbreakstatements in switch (SwiftLint).- breakAlso applies to: 364-364
736-751: Move heavy merging off the main thread to fix long stop delays.
mergeAudioSegmentsruns synchronous waits and export; invoking it on the main queue can freeze for tens of seconds. Dispatch to a background queue and resolve/reject on main.- self.mergeAudioSegments(completion: { success, error in + DispatchQueue.global(qos: .userInitiated).async { + self.mergeAudioSegments(completion: { success, error in + DispatchQueue.main.async { // Clean up temp files regardless of success or failure for url in self.audioSegmentURLs { try? FileManager.default.removeItem(at: url) } // Reset segment tracking self.audioSegmentURLs = [] self.currentSegmentURL = nil if success { resolve(self.audioFileURL?.absoluteString) } else { reject("RNAudioRecorderPlayer", "Failed to merge audio segments: \(error?.localizedDescription ?? "Unknown error")", nil) } - }) + } + }) + }
554-779: Stop flow robustness: good, but consider deactivating the session.After completing stop/merge, call
audioSession.setActive(false, options: .notifyOthersOnDeactivation)to relinquish the session. This avoids interfering with other audio post-stop.Would you like me to add the deactivation at the end of each stop path?
android/src/main/java/com/dooboolab.audiorecorderplayer/RNAudioRecorderPlayerModule.kt (2)
148-149: Return a correct file URI.
"file:///$audioFileURL"yields four slashes for absolute paths. UseUri.fromFile(...).toString()to consistently returnfile:///....- promise.resolve("file:///$audioFileURL") + promise.resolve(Uri.fromFile(java.io.File(audioFileURL)).toString()) @@ - promise.resolve("file:///$audioFileURL") + promise.resolve(Uri.fromFile(java.io.File(audioFileURL)).toString())Also applies to: 236-236
228-236: Over‑strict small‑file rejection.Rejecting files < 1 KB can flag valid short recordings. Prefer zero‑length check or make the threshold configurable.
- if (audioFile.length() < 1000) { // Files smaller than 1KB are likely corrupted + if (audioFile.length() == 0L) { // Zero-length indicates failure Log.w(tag, "Recording file is unusually small (${audioFile.length()} bytes), may be corrupted") audioFile.delete() // Clean up potentially corrupted file - promise.reject("stopRecord", "Recording file appears to be corrupted (too small)") + promise.reject("stopRecord", "Recording file is empty") return }index.ts (2)
294-313: Clarify cross‑platform semantics ofreturnSegments.Docs say “iOS only.” Consider returning a consistent shape across platforms (e.g., Android always returns a single path; iOS returns merged path unless
returnSegmentsis true). At minimum, add a JSDoc note that Android ignores the flag.
376-384: User‑facing strings: keep tone consistent.You mix terminal periods across messages (“Already paused recording.” vs “Already playing”). Make consistent per repo guidelines.
Also applies to: 413-417
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
android/src/main/java/com/dooboolab.audiorecorderplayer/RNAudioRecorderPlayerModule.kt(3 hunks)index.ts(4 hunks)ios/RNAudioRecorderPlayer.m(1 hunks)ios/RNAudioRecorderPlayer.swift(13 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use 2-space indentation, single quotes, and es5 trailing commas in TypeScript code (enforced by Prettier/ESLint)
Use camelCase for variables and functions in TypeScript
Use PascalCase for classes and type names in TypeScript
Files:
index.ts
🪛 SwiftLint (0.57.0)
ios/RNAudioRecorderPlayer.swift
[Warning] 48-48: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Warning] 49-49: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Error] 285-285: Force casts should be avoided
(force_cast)
[Warning] 307-307: Avoid using unneeded break statements
(unneeded_break_in_switch)
[Warning] 364-364: Avoid using unneeded break statements
(unneeded_break_in_switch)
[Warning] 355-355: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 355-355: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 355-355: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
[Warning] 808-808: Initializing an optional variable with nil is redundant
(redundant_optional_initialization)
[Warning] 559-559: Returning Void in a function declaration is redundant
(redundant_void_return)
[Warning] 1152-1152: Returning Void in a function declaration is redundant
(redundant_void_return)
🔇 Additional comments (4)
ios/RNAudioRecorderPlayer.m (1)
20-25: Extern labels: keep consistent with Swift @objc names; consider unifying later.
rejecter:here is fine because your Swift methods are annotated with@objc(..., rejecter:), whilestartRecorderusesreject:per its annotation. If you do a later cleanup to unify labels across APIs, update both the@objcannotations and these externs together to avoid bridging breaks.Run to verify label parity between Objective‑C externs and Swift annotations:
#!/bin/bash rg -n 'RCT_EXTERN_METHOD\(.+rejecter:' ios | sed -n '1,120p' rg -n '@objc\(.+rejecter:\)' ios | sed -n '1,120p' rg -n 'RCT_EXTERN_METHOD\(.+reject\)' ios/RNAudioRecorderPlayer.m | sed -n '1,120p' rg -n '@objc\(.+reject:\)' ios/RNAudioRecorderPlayer.swift | sed -n '1,200p'android/src/main/java/com/dooboolab.audiorecorderplayer/RNAudioRecorderPlayerModule.kt (2)
69-76: Good: unique file names for DEFAULT path reduce corruption on restarts.
194-204: API parity note:returnSegmentsis ignored on Android.Document this in JS types and consider resolving with a structured return (e.g., always a single path) to avoid platform surprises.
index.ts (1)
159-167: LGTM: explicit field defaults and nullability.Clear initialization removes undefined states.
| // Send local notification to inform user | ||
| showInterruptionNotification() | ||
|
|
There was a problem hiding this comment.
Remove or gate local notifications.
Library code shouldn’t show end-user notifications by default (hard‑coded “Craft Paused” text is app-specific). Remove the call or gate it behind an opt‑in flag.
- // Send local notification to inform user
- showInterruptionNotification()
+ // Consider exposing an opt-in flag from JS before surfacing any notifications to end users.Also applies to: 1158-1178
🤖 Prompt for AI Agents
In ios/RNAudioRecorderPlayer.swift around lines 260-262 (and similarly at
1158-1178), the library is unconditionally sending a local notification ("Craft
Paused") which is app-specific; replace this by gating the notification behind a
configurable, opt-in boolean (e.g., notifyOnInterruption defaulting to false) or
remove the call entirely. Add a public property/option to the recorder/player
interface that users can set to enable notifications, and wrap the
showInterruptionNotification() call in an if check that only executes when that
flag is true; ensure any other hard-coded notification text is removed or made
configurable and document the new opt-in setting.
| // Check if the file has valid content before adding it | ||
| let attr = try FileManager.default.attributesOfItem(atPath: currentURL.path) | ||
| let fileSize = attr[FileAttributeKey.size] as! UInt64 | ||
|
|
||
| if fileSize > 0 && !self.isSegmentAlreadyAdded(currentURL) { | ||
| self.audioSegmentURLs.append(currentURL) | ||
| } else if fileSize == 0 { | ||
| try FileManager.default.removeItem(at: currentURL) | ||
| } |
There was a problem hiding this comment.
Avoid force-cast when checking file size.
Replace as! with safe casting and fold the branches.
- let attr = try FileManager.default.attributesOfItem(atPath: currentURL.path)
- let fileSize = attr[FileAttributeKey.size] as! UInt64
-
- if fileSize > 0 && !self.isSegmentAlreadyAdded(currentURL) {
- self.audioSegmentURLs.append(currentURL)
- } else if fileSize == 0 {
- try FileManager.default.removeItem(at: currentURL)
- }
+ let attr = try FileManager.default.attributesOfItem(atPath: currentURL.path)
+ if let fileSize = attr[.size] as? UInt64, fileSize > 0 {
+ if !self.isSegmentAlreadyAdded(currentURL) {
+ self.audioSegmentURLs.append(currentURL)
+ }
+ } else {
+ try FileManager.default.removeItem(at: currentURL)
+ }📝 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.
| // Check if the file has valid content before adding it | |
| let attr = try FileManager.default.attributesOfItem(atPath: currentURL.path) | |
| let fileSize = attr[FileAttributeKey.size] as! UInt64 | |
| if fileSize > 0 && !self.isSegmentAlreadyAdded(currentURL) { | |
| self.audioSegmentURLs.append(currentURL) | |
| } else if fileSize == 0 { | |
| try FileManager.default.removeItem(at: currentURL) | |
| } | |
| // Check if the file has valid content before adding it | |
| let attr = try FileManager.default.attributesOfItem(atPath: currentURL.path) | |
| if let fileSize = attr[.size] as? UInt64, fileSize > 0 { | |
| if !self.isSegmentAlreadyAdded(currentURL) { | |
| self.audioSegmentURLs.append(currentURL) | |
| } | |
| } else { | |
| try FileManager.default.removeItem(at: currentURL) | |
| } |
🧰 Tools
🪛 SwiftLint (0.57.0)
[Error] 285-285: Force casts should be avoided
(force_cast)
🤖 Prompt for AI Agents
In ios/RNAudioRecorderPlayer.swift around lines 283 to 291, the code force-casts
the file size with `as!` and splits logic into multiple branches; change this to
safely unwrap the attribute (e.g., cast to NSNumber or use `as? UInt64` with `if
let`) and then handle both cases in a single conditional: if the size is > 0 and
segment not already added, append URL; else (size == 0) remove the file. Ensure
you avoid force-casts and handle the optional cast failing by treating it as a
zero-size/remove case or by propagating the error as appropriate.
5 participants
This pull request enhances the
react-native-audio-recorder-playerlibrary by adding support for handling audio recording interruptions, such as incoming calls, in a seamless way. Previously, when an interruption occurred, resuming the recording would overwrite the existing audio file becauseAVAudioRecorder’srecord()method implicitly callsprepareToRecord(), creating a new file at the specified URL. This made it impossible to truly "resume" a recording with the existing API.To address this issue, we’ve introduced a new approach:
AVMutableComposition, ensuring no audio is lost.This solution ensures that recordings remain intact across interruptions, providing a smoother and more reliable experience for users. The changes are implemented in
RNAudioRecorderPlayer.swift, with updates to key methods likestartRecorder,resumeRecorder, andstopRecorder, along with new functions for segment management and merging.Summary by CodeRabbit
New Features
Bug Fixes