Fix record audio in background #753
Conversation
📝 WalkthroughWalkthroughThis PR introduces a new Android foreground service for background audio recording, refactors Sound.kt to delegate recording lifecycle to the service with audio focus management, and enhances iOS lifecycle handling with interruption observers, memory optimization, and autoreleasepool usage. Minor TypeScript interface formatting adjustments and package metadata update included. Changes
Sequence DiagramsequenceDiagram
participant SoundActivity as Sound (Activity)
participant AudioMgr as AudioManager
participant RecordingFG as RecordingForegroundService
participant MediaRec as MediaRecorder
participant NotifMgr as Notification System
SoundActivity->>AudioMgr: requestAudioFocus()
AudioMgr-->>SoundActivity: focus granted
SoundActivity->>SoundActivity: bindService()
SoundActivity->>RecordingFG: onServiceConnected()
RecordingFG->>RecordingFG: onCreate()
RecordingFG->>NotifMgr: createNotificationChannel()
SoundActivity->>RecordingFG: startRecording(filePath, config, enableMetering)
RecordingFG->>MediaRec: setAudioSource(), setOutputFormat(), setAudioEncoder()
RecordingFG->>MediaRec: prepare() + start()
RecordingFG->>RecordingFG: startTimer() for metering updates
RecordingFG->>NotifMgr: startForeground(notification)
RecordingFG-->>SoundActivity: recording started
RecordingFG->>RecordingFG: Timer: query maxAmplitude
RecordingFG-->>SoundActivity: recordBackListener(metering callback)
alt Pause Recording
SoundActivity->>RecordingFG: pauseRecording()
RecordingFG->>MediaRec: pause()
else Resume Recording
SoundActivity->>RecordingFG: resumeRecording()
RecordingFG->>MediaRec: resume()
else Stop Recording
SoundActivity->>RecordingFG: stopRecording()
RecordingFG->>MediaRec: stop() + release()
RecordingFG->>RecordingFG: stopTimer()
RecordingFG->>NotifMgr: stopForeground()
RecordingFG-->>SoundActivity: return filePath
SoundActivity->>AudioMgr: abandonAudioFocus()
SoundActivity->>SoundActivity: unbindService()
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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 enhances the audio recording and playback capabilities across both Android and iOS platforms. The primary focus is on improving the reliability of background recording, handling audio interruptions gracefully, and optimizing memory usage during playback, leading to a more stable and efficient audio 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. 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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ios/Sound.swift (1)
848-880: Temp file deletion timing is risky; player may still need the file.The temp file is deleted after only 1 second (line 873-875), but
AVAudioPlayer(contentsOf:)may still be buffering or reading from the file during playback. This could cause playback failures for longer audio files or on slower devices.🐛 Suggested fix: delete file after playback completes
Store the temp file URL and delete it in the player delegate's
audioPlayerDidFinishPlayingor whenstopPlayeris called:+ private var tempAudioFileURL: URL? + private func setupEnginePlayer(url: String, httpHeaders: Dictionary<String, String>?, promise: Promise<String>) { // ... do { // Use streaming download instead of loading into memory let data = try Data(contentsOf: audioURL) try data.write(to: tempFile) + self.tempAudioFileURL = tempFile // Create player from file instead of data (more memory efficient) self.audioPlayer = try AVAudioPlayer(contentsOf: tempFile) // ... promise.resolve(withResult: url) - - // Clean up temp file after a delay (player keeps reference) - DispatchQueue.global(qos: .utility).asyncAfter(deadline: .now() + 1.0) { - try? FileManager.default.removeItem(at: tempFile) - } } catch {Then in
stopPlayer()andaudioPlayerDidFinishPlaying:if let tempURL = self.tempAudioFileURL { try? FileManager.default.removeItem(at: tempURL) self.tempAudioFileURL = nil }
🤖 Fix all issues with AI agents
In
@android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt:
- Around line 66-68: The singleton backing field instance in
RecordingForegroundService is not volatile, so updates from onCreate/onDestroy
may not be visible to other threads; annotate the private var instance:
RecordingForegroundService? with @Volatile to ensure proper visibility across
threads and keep the existing getInstance() accessor unchanged (refer to the
RecordingForegroundService class, the instance property and getInstance()
method).
- Around line 252-271: resumeRecording currently calls startRecordTimer(60L)
which hardcodes the update interval; replace the literal with the stored
subscriptionDurationMs used in startRecording so the resumed timer respects the
caller's requested interval. In the resumeRecording method (and where
startRecordTimer is invoked), use subscriptionDurationMs instead of 60L,
ensuring subscriptionDurationMs is non-null/initialized (fall back to a sensible
default only if necessary) and leave recordStartTime, pausedRecordTime, isPaused
and isRecording updates intact.
In @android/src/main/java/com/margelo/nitro/audiorecorderplayer/Sound.kt:
- Around line 209-213: The Thread.sleep(300) call in Sound.kt is fragile;
replace the sleep-based wait with a ServiceConnection-based flow: remove the
Thread.sleep(300) and stop relying on RecordingForegroundService.getInstance()
immediately; instead, store the recording inputs in a pendingRecordingParams
data holder and the promise in pendingRecordPromise, bind to the service and
start the actual recording from onServiceConnected by calling a helper like
startRecordingWithService(pendingRecordingParams, pendingRecordPromise), then
clear the pending values and set isServiceBound/recordingService from the binder
(RecordingForegroundService.RecordingBinder) so recording only begins when the
service is actually connected.
In @ios/Sound.swift:
- Line 942: The closure passed to Timer.scheduledTimer is capturing an unused
parameter named `timer` (in the `playTimer =
Timer.scheduledTimer(withTimeInterval:subscriptionDuration,repeats:true)` call);
change the closure signature to use an unused wildcard parameter (i.e. replace
`timer in` with `_ in`) so the unused parameter warning is resolved while
preserving the existing `[weak self]` capture and body.
In @package.json:
- Line 4: The package.json now has "private": true which prevents npm publishing
but conflicts with the existing release-it config that sets "publish": true;
decide the intended behavior and make them consistent: if the package should be
published, remove or set "private" to false so release-it can publish; if the
package is intentionally private, update the release-it configuration (the
"publish": true setting) to false to prevent attempts to publish. Ensure the
change touches the "private" field and the release-it "publish" option to keep
both aligned.
In @src/specs/Sound.nitro.ts:
- Around line 115-116: The export interface block for AudioSet (and the similar
block at lines 140-143) is misformatted and violates the project's
Prettier/ESLint 2-space indentation rule; fix by reformatting the declaration so
it uses 2-space indentation and standard line breaks (e.g., place the extends
clause on the next line indented with two spaces and ensure the closing brace
aligns correctly) for the symbols AudioSet, IOSAudioSet, AndroidAudioSet, and
CommonAudioSet so the file matches the project's lint/Prettier configuration.
🧹 Nitpick comments (3)
android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt (1)
279-308: Consider logging swallowed exceptions for debugging.The try-catch blocks intentionally swallow exceptions during cleanup, which is reasonable. However, adding debug logging would help diagnose issues in production.
♻️ Optional: Add logging for swallowed exceptions
mediaRecorder?.apply { try { stop() } catch (e: Exception) { - // Ignore stop errors + Logger.d("[ForegroundService] Ignoring stop error: ${e.message}") } try { release() } catch (e: Exception) { - // Ignore release errors + Logger.d("[ForegroundService] Ignoring release error: ${e.message}") } }android/src/main/java/com/margelo/nitro/audiorecorderplayer/Sound.kt (2)
50-68: DuplicateonRecordingUpdatecallback setup.The callback is set in both
onServiceConnected(line 57) and after theThread.sleepinstartRecorder(line 216). This redundancy is likely a workaround for the fragile service startup approach. Fixing theThread.sleepissue would also eliminate this duplication.Also applies to: 214-227
717-721: Consider usingAudioFocusRequestfor API 26+.The
requestAudioFocus(listener, streamType, durationHint)method is deprecated since API 26. For better compatibility and to silence deprecation warnings, use theAudioFocusRequest.Builderpattern.♻️ Suggested implementation
private var audioFocusRequest: Any? = null // AudioFocusRequest for API 26+ private fun setupAudioFocus() { audioManager = context.getSystemService(Context.AUDIO_SERVICE) as? AudioManager audioFocusChangeListener = AudioManager.OnAudioFocusChangeListener { focusChange -> // ... existing listener code } if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { val request = android.media.AudioFocusRequest.Builder(AudioManager.AUDIOFOCUS_GAIN_TRANSIENT) .setOnAudioFocusChangeListener(audioFocusChangeListener!!) .build() audioFocusRequest = request audioManager?.requestAudioFocus(request) } else { @Suppress("DEPRECATION") audioManager?.requestAudioFocus( audioFocusChangeListener, AudioManager.STREAM_MUSIC, AudioManager.AUDIOFOCUS_GAIN_TRANSIENT ) } } private fun releaseAudioFocus() { if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.O) { (audioFocusRequest as? android.media.AudioFocusRequest)?.let { audioManager?.abandonAudioFocusRequest(it) } } else { audioFocusChangeListener?.let { listener -> @Suppress("DEPRECATION") audioManager?.abandonAudioFocus(listener) } } audioFocusRequest = null audioFocusChangeListener = null audioManager = null }
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
nitrogen/generated/ios/NitroSound-Swift-Cxx-Bridge.cppis excluded by!**/generated/**nitrogen/generated/ios/NitroSound-Swift-Cxx-Bridge.hppis excluded by!**/generated/**
📒 Files selected for processing (6)
android/src/main/AndroidManifest.xmlandroid/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.ktandroid/src/main/java/com/margelo/nitro/audiorecorderplayer/Sound.ktios/Sound.swiftpackage.jsonsrc/specs/Sound.nitro.ts
🧰 Additional context used
📓 Path-based instructions (2)
src/specs/*.nitro.ts
📄 CodeRabbit inference engine (AGENTS.md)
Define Nitro module surfaces in TypeScript spec files located at src/specs/*.nitro.ts
Files:
src/specs/Sound.nitro.ts
**/*.{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:
src/specs/Sound.nitro.ts
🧠 Learnings (2)
📚 Learning: 2025-09-07T05:13:03.063Z
Learnt from: CR
Repo: hyochan/react-native-nitro-sound PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T05:13:03.063Z
Learning: Modify behavior via Nitro specs (src/specs/*.nitro.ts) and native implementations, not by editing generated bindings
Applied to files:
src/specs/Sound.nitro.ts
📚 Learning: 2025-09-07T05:13:03.063Z
Learnt from: CR
Repo: hyochan/react-native-nitro-sound PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-07T05:13:03.063Z
Learning: Applies to src/specs/*.nitro.ts : Define Nitro module surfaces in TypeScript spec files located at src/specs/*.nitro.ts
Applied to files:
src/specs/Sound.nitro.ts
🧬 Code graph analysis (1)
ios/Sound.swift (7)
android/src/main/java/com/margelo/nitro/audiorecorderplayer/Sound.kt (4)
startRecorder(78-292)addPlayBackListener(605-607)removePlayBackListener(609-611)stopPlayTimer(682-685)nitrogen/generated/ios/c++/HybridSoundSpecSwift.hpp (10)
uri(99-106)uri(99-99)uri(131-138)uri(131-131)callback(193-198)callback(193-193)callback(205-210)callback(205-205)callback(217-222)callback(217-217)android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt (2)
stopRecordTimer(361-364)stop(79-83)nitrogen/generated/android/kotlin/com/margelo/nitro/sound/RecordBackType.kt (1)
isRecording(17-45)src/specs/Sound.nitro.ts (2)
RecordBackType(118-123)PlayBackType(125-129)nitrogen/generated/android/kotlin/com/margelo/nitro/sound/PlaybackEndType.kt (1)
duration(17-39)nitrogen/generated/android/kotlin/com/margelo/nitro/sound/PlayBackType.kt (1)
isMuted(17-42)
🪛 detekt (1.23.8)
android/src/main/java/com/margelo/nitro/audiorecorderplayer/Sound.kt
[warning] 250-250: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 265-265: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 280-280: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 337-337: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 361-361: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 438-438: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 657-657: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt
[warning] 286-286: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 291-291: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 313-313: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 377-377: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
[warning] 456-456: The caught exception is swallowed. The original exception could be lost.
(detekt.exceptions.SwallowedException)
🪛 GitHub Check: lint
src/specs/Sound.nitro.ts
[warning] 116-116:
Replace ·AndroidAudioSet, with ⏎····AndroidAudioSet,⏎···
[warning] 143-143:
Insert ··
[warning] 142-142:
Insert ··
[warning] 141-141:
Insert ··
[warning] 140-140:
Insert ⏎·
🪛 SwiftLint (0.57.0)
ios/Sound.swift
[Warning] 942-942: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
⏰ 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). (3)
- GitHub Check: ci-ios / ios-dependencies
- GitHub Check: ci-web / build-web
- GitHub Check: ios-dependencies
🔇 Additional comments (7)
ios/Sound.swift (3)
1029-1054: Good implementation of interruption handling lifecycle.The observer setup/teardown pattern is well-implemented with proper cleanup in
removeInterruptionObserver()anddeinit. The dual observation of bothinterruptionNotificationandwillTerminateNotificationensures recordings are properly finalized.
1154-1195: Thorough resource cleanup indeinit.The cleanup is comprehensive: removing observers, invalidating timers, stopping audio components, clearing listeners (to break retain cycles), and deactivating the session. This is well-implemented.
1138-1144: Swift syntax error: trailing comma before closing parenthesis.Swift does not allow trailing commas in function/initializer argument lists. This will cause a compilation error.
🐛 Fix the trailing comma
listener(RecordBackType( isRecording: false, currentPosition: currentTime, currentMetering: nil, - recordSecs: currentTime, + recordSecs: currentTime ))Likely an incorrect or invalid review comment.
android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt (2)
434-446: Wake lock has a 10-hour timeout - verify this aligns with expected use cases.The wake lock timeout of 10 hours (line 441) is quite long. While this prevents indefinite locks, it could drain battery if the service isn't properly stopped. The
releaseWakeLock()is called inonDestroy(), which should handle normal cleanup.
117-169: Good implementation of recording finalization on app kill.The
onTaskRemoved→finalizeRecordingOnKillflow ensures the audio file is properly finalized when the user swipes away the app. The file header will be written correctly, making the recording playable.android/src/main/AndroidManifest.xml (1)
6-20: Manifest correctly declares foreground service permissions and service.The permissions (
FOREGROUND_SERVICE,FOREGROUND_SERVICE_MICROPHONEfor API 34+,WAKE_LOCK) align with theRecordingForegroundServiceimplementation. The service is correctly declared withforegroundServiceType="microphone"andexported="false".android/src/main/java/com/margelo/nitro/audiorecorderplayer/Sound.kt (1)
324-374: Good cleanup implementation instopRecorder.The method properly unbinds the service, stops the foreground service, and releases audio focus in both success and error paths. This ensures resources are always cleaned up.
| private var instance: RecordingForegroundService? = null | ||
|
|
||
| fun getInstance(): RecordingForegroundService? = instance |
There was a problem hiding this comment.
Add @Volatile annotation for thread-safe singleton access.
The instance variable is accessed from multiple threads (main thread in onCreate/onDestroy, IO thread in Sound.kt). Without @Volatile, changes may not be visible across threads.
🔧 Suggested fix
- private var instance: RecordingForegroundService? = null
+ @Volatile
+ private var instance: RecordingForegroundService? = 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.
| private var instance: RecordingForegroundService? = null | |
| fun getInstance(): RecordingForegroundService? = instance | |
| @Volatile | |
| private var instance: RecordingForegroundService? = null | |
| fun getInstance(): RecordingForegroundService? = instance |
🤖 Prompt for AI Agents
In
@android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt
around lines 66 - 68, The singleton backing field instance in
RecordingForegroundService is not volatile, so updates from onCreate/onDestroy
may not be visible to other threads; annotate the private var instance:
RecordingForegroundService? with @Volatile to ensure proper visibility across
threads and keep the existing getInstance() accessor unchanged (refer to the
RecordingForegroundService class, the instance property and getInstance()
method).
| fun resumeRecording(): Boolean { | ||
| if (!isPaused) return false | ||
|
|
||
| return try { | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { | ||
| mediaRecorder?.resume() | ||
| recordStartTime = System.currentTimeMillis() - pausedRecordTime | ||
| isPaused = false | ||
| isRecording = true | ||
| startRecordTimer(60L) // Default, will be updated | ||
| updateNotification("Recording in progress...") | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| } catch (e: Exception) { | ||
| e.printStackTrace() | ||
| false | ||
| } | ||
| } |
There was a problem hiding this comment.
Hardcoded timer interval in resumeRecording ignores caller's subscription duration.
Line 261 uses a hardcoded 60L instead of the subscriptionDurationMs that was stored during startRecording. This could cause inconsistent update intervals after pause/resume.
🐛 Suggested fix
mediaRecorder?.resume()
recordStartTime = System.currentTimeMillis() - pausedRecordTime
isPaused = false
isRecording = true
- startRecordTimer(60L) // Default, will be updated
+ startRecordTimer(subscriptionDurationMs)
updateNotification("Recording in progress...")
true📝 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.
| fun resumeRecording(): Boolean { | |
| if (!isPaused) return false | |
| return try { | |
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { | |
| mediaRecorder?.resume() | |
| recordStartTime = System.currentTimeMillis() - pausedRecordTime | |
| isPaused = false | |
| isRecording = true | |
| startRecordTimer(60L) // Default, will be updated | |
| updateNotification("Recording in progress...") | |
| true | |
| } else { | |
| false | |
| } | |
| } catch (e: Exception) { | |
| e.printStackTrace() | |
| false | |
| } | |
| } | |
| fun resumeRecording(): Boolean { | |
| if (!isPaused) return false | |
| return try { | |
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { | |
| mediaRecorder?.resume() | |
| recordStartTime = System.currentTimeMillis() - pausedRecordTime | |
| isPaused = false | |
| isRecording = true | |
| startRecordTimer(subscriptionDurationMs) | |
| updateNotification("Recording in progress...") | |
| true | |
| } else { | |
| false | |
| } | |
| } catch (e: Exception) { | |
| e.printStackTrace() | |
| false | |
| } | |
| } |
🤖 Prompt for AI Agents
In
@android/src/main/java/com/margelo/nitro/audiorecorderplayer/RecordingForegroundService.kt
around lines 252 - 271, resumeRecording currently calls startRecordTimer(60L)
which hardcodes the update interval; replace the literal with the stored
subscriptionDurationMs used in startRecording so the resumed timer respects the
caller's requested interval. In the resumeRecording method (and where
startRecordTimer is invoked), use subscriptionDurationMs instead of 60L,
ensuring subscriptionDurationMs is non-null/initialized (fall back to a sensible
default only if necessary) and leave recordStartTime, pausedRecordTime, isPaused
and isRecording updates intact.
| // Wait a bit for service to start, then start recording | ||
| Thread.sleep(300) | ||
|
|
||
| // Get service instance and start recording | ||
| val service = RecordingForegroundService.getInstance() |
There was a problem hiding this comment.
Replace Thread.sleep(300) with proper service binding callback.
Using Thread.sleep to wait for service startup is fragile and can cause race conditions. The service might not be ready after 300ms on slower devices, or the delay is unnecessarily long on fast devices.
🐛 Suggested approach: Use the ServiceConnection callback
Instead of sleeping, start recording from the onServiceConnected callback. Store the recording parameters and use a pending promise pattern:
private var pendingRecordingParams: RecordingParams? = null
private var pendingRecordPromise: Promise<String>? = null
data class RecordingParams(
val filePath: String,
val audioSource: Int,
val outputFormat: Int,
val audioEncoder: Int,
val samplingRate: Int?,
val channels: Int?,
val bitrate: Int?,
val enableMetering: Boolean
)
override fun onServiceConnected(name: ComponentName?, service: IBinder?) {
val binder = service as RecordingForegroundService.RecordingBinder
recordingService = binder.getService()
isServiceBound = true
// Start recording if params are pending
pendingRecordingParams?.let { params ->
startRecordingWithService(params, pendingRecordPromise!!)
pendingRecordingParams = null
pendingRecordPromise = null
}
}🤖 Prompt for AI Agents
In @android/src/main/java/com/margelo/nitro/audiorecorderplayer/Sound.kt around
lines 209 - 213, The Thread.sleep(300) call in Sound.kt is fragile; replace the
sleep-based wait with a ServiceConnection-based flow: remove the
Thread.sleep(300) and stop relying on RecordingForegroundService.getInstance()
immediately; instead, store the recording inputs in a pendingRecordingParams
data holder and the promise in pendingRecordPromise, bind to the service and
start the actual recording from onServiceConnected by calling a helper like
startRecordingWithService(pendingRecordingParams, pendingRecordPromise), then
clear the pending values and set isServiceBound/recordingService from the binder
(RecordingForegroundService.RecordingBinder) so recording only begins when the
service is actually connected.
| val service = RecordingForegroundService.getInstance() | ||
| if (service != null && service.isCurrentlyRecording()) { | ||
| service.pauseRecording() | ||
|
|
||
| recordBackListener?.invoke( | ||
| RecordBackType( | ||
| isRecording = false, | ||
| currentPosition = 0.0, | ||
| currentMetering = null, | ||
| recordSecs = 0.0 | ||
| ) | ||
| ) | ||
| } |
There was a problem hiding this comment.
Incorrect currentPosition reported on audio focus loss.
When pausing due to audio focus loss, the callback reports currentPosition = 0.0 and recordSecs = 0.0. This should reflect the actual recording position at the time of interruption.
🐛 Suggested fix: Get the actual position from the service
handler.post {
val service = RecordingForegroundService.getInstance()
if (service != null && service.isCurrentlyRecording()) {
+ val currentTime = System.currentTimeMillis() - service.getRecordStartTime()
service.pauseRecording()
recordBackListener?.invoke(
RecordBackType(
isRecording = false,
- currentPosition = 0.0,
+ currentPosition = currentTime.toDouble(),
currentMetering = null,
- recordSecs = 0.0
+ recordSecs = currentTime.toDouble()
)
)
}
}Note: This requires exposing getRecordStartTime() from RecordingForegroundService.
Committable suggestion skipped: line range outside the PR's diff.
|
|
||
| self.didEmitPlaybackEnd = false | ||
|
|
||
| self.playTimer = Timer.scheduledTimer(withTimeInterval: self.subscriptionDuration, repeats: true) { [weak self] timer in |
There was a problem hiding this comment.
Replace unused timer parameter with _.
Per SwiftLint warning, the timer parameter is unused.
🔧 Suggested fix
- self.playTimer = Timer.scheduledTimer(withTimeInterval: self.subscriptionDuration, repeats: true) { [weak self] timer in
+ self.playTimer = Timer.scheduledTimer(withTimeInterval: self.subscriptionDuration, repeats: true) { [weak self] _ in📝 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.
| self.playTimer = Timer.scheduledTimer(withTimeInterval: self.subscriptionDuration, repeats: true) { [weak self] timer in | |
| self.playTimer = Timer.scheduledTimer(withTimeInterval: self.subscriptionDuration, repeats: true) { [weak self] _ in |
🧰 Tools
🪛 SwiftLint (0.57.0)
[Warning] 942-942: Unused parameter in a closure should be replaced with _
(unused_closure_parameter)
🤖 Prompt for AI Agents
In @ios/Sound.swift at line 942, The closure passed to Timer.scheduledTimer is
capturing an unused parameter named `timer` (in the `playTimer =
Timer.scheduledTimer(withTimeInterval:subscriptionDuration,repeats:true)` call);
change the closure signature to use an unused wildcard parameter (i.e. replace
`timer in` with `_ in`) so the unused parameter warning is resolved while
preserving the existing `[weak self]` capture and body.
| { | ||
| "name": "react-native-nitro-sound", | ||
| "version": "0.2.10", | ||
| "private": true, |
There was a problem hiding this comment.
Adding "private": true will prevent npm publishing.
This change conflicts with the release-it configuration at line 134 which has "publish": true. If this package is intended to be published to npm (as the existing config suggests), this field will block publishing.
If this was intentional (e.g., converting to a monorepo-only package), please also update the release-it config. Otherwise, this appears to be an accidental addition unrelated to the background recording feature.
🤖 Prompt for AI Agents
In @package.json at line 4, The package.json now has "private": true which
prevents npm publishing but conflicts with the existing release-it config that
sets "publish": true; decide the intended behavior and make them consistent: if
the package should be published, remove or set "private" to false so release-it
can publish; if the package is intentionally private, update the release-it
configuration (the "publish": true setting) to false to prevent attempts to
publish. Ensure the change touches the "private" field and the release-it
"publish" option to keep both aligned.
| export interface AudioSet | ||
| extends IOSAudioSet, | ||
| AndroidAudioSet, | ||
| CommonAudioSet {} | ||
| extends IOSAudioSet, AndroidAudioSet, CommonAudioSet {} |
There was a problem hiding this comment.
Fix formatting to pass lint checks.
The static analysis reports multiple formatting warnings on these lines. The formatting changes introduced here don't match the project's Prettier/ESLint configuration (2-space indentation as per coding guidelines).
🔧 Suggested fix based on lint hints
export interface AudioSet
- extends IOSAudioSet, AndroidAudioSet, CommonAudioSet {}
+ extends IOSAudioSet,
+ AndroidAudioSet,
+ CommonAudioSet {}-export interface Sound extends HybridObject<{
- ios: 'swift';
- android: 'kotlin';
-}> {
+export interface Sound
+ extends HybridObject<{
+ ios: 'swift';
+ android: 'kotlin';
+ }> {Also applies to: 140-143
🧰 Tools
🪛 GitHub Check: lint
[warning] 116-116:
Replace ·AndroidAudioSet, with ⏎····AndroidAudioSet,⏎···
🤖 Prompt for AI Agents
In @src/specs/Sound.nitro.ts around lines 115 - 116, The export interface block
for AudioSet (and the similar block at lines 140-143) is misformatted and
violates the project's Prettier/ESLint 2-space indentation rule; fix by
reformatting the declaration so it uses 2-space indentation and standard line
breaks (e.g., place the extends clause on the next line indented with two spaces
and ensure the closing brace aligns correctly) for the symbols AudioSet,
IOSAudioSet, AndroidAudioSet, and CommonAudioSet so the file matches the
project's lint/Prettier configuration.
![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 significantly improves background audio recording on Android by introducing a Foreground Service, and adds robust interruption handling for both Android and iOS. The iOS implementation also benefits from memory usage optimizations. While these are great additions, I've identified a few critical and high-severity issues. On Android, there's a race condition when starting the recording service that needs to be addressed. On iOS, there's a potential memory issue with remote audio playback. I've provided detailed comments and suggestions for these points.
| val fileUri = Uri.fromFile(File(filePath)).toString() | ||
| promise.resolve(fileUri) | ||
| // Wait a bit for service to start, then start recording | ||
| Thread.sleep(300) |
There was a problem hiding this comment.
Using Thread.sleep() to wait for the service to start is unreliable and can lead to race conditions. The service might take longer than 300ms to initialize, especially on older devices or under heavy load. This could cause RecordingForegroundService.getInstance() to return null, failing the recording.
The recording logic should be triggered from within the onServiceConnected callback to ensure the service is properly bound and available. This would involve storing the Promise and recording parameters as class members and using them inside onServiceConnected.
| fun pauseRecording(): Boolean { | ||
| if (!isRecording || isPaused) return false | ||
|
|
||
| return try { | ||
| if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) { | ||
| mediaRecorder?.pause() | ||
| pausedRecordTime = System.currentTimeMillis() - recordStartTime | ||
| isPaused = true | ||
| isRecording = false | ||
| stopRecordTimer() | ||
| updateNotification("Recording paused") | ||
| true | ||
| } else { | ||
| false | ||
| } | ||
| } catch (e: Exception) { | ||
| e.printStackTrace() | ||
| false | ||
| } | ||
| } |
There was a problem hiding this comment.
To fix an issue with interruption handling, pauseRecording should send one final update to listeners with the correct paused position. This ensures the UI has the correct state when recording is paused due to an interruption (like a phone call).
fun pauseRecording(): Boolean {
if (!isRecording || isPaused) return false
return try {
if (Build.VERSION.SDK_INT >= Build.VERSION_CODES.N) {
mediaRecorder?.pause()
pausedRecordTime = System.currentTimeMillis() - recordStartTime
isPaused = true
isRecording = false
stopRecordTimer()
// Notify listener of paused state with final position
val meteringValue = if (meteringEnabled) lastMeteringValue else METERING_DISABLED_VALUE
handler.post {
onRecordingUpdate?.invoke(isRecording, pausedRecordTime.toDouble(), meteringValue)
}
updateNotification("Recording paused")
true
} else {
false
}
} catch (e: Exception) {
Logger.e("[ForegroundService] Error pausing recording: ${e.message}", e)
false
}
}| recordStartTime = System.currentTimeMillis() - pausedRecordTime | ||
| isPaused = false | ||
| isRecording = true | ||
| startRecordTimer(60L) // Default, will be updated |
There was a problem hiding this comment.
startRecordTimer is called with a hardcoded value of 60L. This overrides the original subscriptionDuration provided when the recording was started. You should use the stored subscriptionDurationMs to maintain the correct update interval.
startRecordTimer(subscriptionDurationMs) // Use original subscription duration|
|
||
| recordBackListener?.invoke( | ||
| RecordBackType( | ||
| isRecording = false, | ||
| currentPosition = 0.0, | ||
| currentMetering = null, | ||
| recordSecs = 0.0 | ||
| ) | ||
| ) |
There was a problem hiding this comment.
When audio focus is lost, you are invoking recordBackListener with currentPosition = 0.0. This will incorrectly reset the recording progress in the UI. The service should be the single source of truth for the recording state. I've added another comment suggesting a change in RecordingForegroundService.kt to have pauseRecording() emit one final update with the correct position. After applying that change, this recordBackListener invocation should be removed.
|
|
||
| do { | ||
| // Use streaming download instead of loading into memory | ||
| let data = try Data(contentsOf: audioURL) |
There was a problem hiding this comment.
The comment mentions using a streaming download, but Data(contentsOf: audioURL) loads the entire file's content into memory at once. For large audio files, this can lead to high memory usage and potential app crashes. To achieve true streaming download and playback, you should use URLSession with a URLSessionDownloadTask to download the file to a temporary location and then play it from the file. Alternatively, for real-time streaming, AVPlayer with an AVURLAsset is a better choice than AVAudioPlayer.
|
|
||
| return true | ||
| } catch (e: Exception) { | ||
| e.printStackTrace() |
There was a problem hiding this comment.
Throughout this file, e.printStackTrace() is used for error handling (e.g., also in pauseRecording, resumeRecording, etc.). It's better to use the provided Logger.e for consistency and to have more control over logging. printStackTrace prints to standard error and bypasses the Android logging system, making logs harder to filter and manage.
| e.printStackTrace() | |
| Logger.e("[ForegroundService] Error starting recording: ${e.message}", e) |
| mediaRecorder?.apply { | ||
| try { | ||
| stop() | ||
| } catch (e: Exception) { | ||
| // Ignore stop errors | ||
| } | ||
| try { | ||
| release() | ||
| } catch (e: Exception) { | ||
| // Ignore release errors | ||
| } | ||
| } |
There was a problem hiding this comment.
Here, exceptions from stop() and release() are silently ignored. In finalizeRecordingOnKill, these exceptions are logged. For consistency and better debugging, consider logging these errors here as well, at least at a warning level.
mediaRecorder?.apply {
try {
stop()
} catch (e: Exception) {
Logger.w("[ForegroundService] Error stopping MediaRecorder: ${e.message}", e)
}
try {
release()
} catch (e: Exception) {
Logger.w("[ForegroundService] Error releasing MediaRecorder: ${e.message}", e)
}
}| CoroutineScope(Dispatchers.IO).launch { | ||
| try { | ||
| mediaRecorder?.apply { | ||
| stop() | ||
| release() | ||
| } | ||
| mediaRecorder = null | ||
|
|
||
| // Reset metering state | ||
| meteringEnabled = false | ||
| lastMeteringUpdateTime = 0L | ||
| lastMeteringValue = SILENCE_THRESHOLD_DB | ||
|
|
||
| val service = RecordingForegroundService.getInstance() | ||
| val path = service?.stopRecording() ?: currentRecordingPath | ||
|
|
||
| handler.post { | ||
| stopRecordTimer() | ||
| // Unbind from service | ||
| if (isServiceBound) { | ||
| try { | ||
| context.unbindService(serviceConnection) | ||
| } catch (e: Exception) { | ||
| // Ignore unbind errors | ||
| } | ||
| isServiceBound = false | ||
| } | ||
|
|
||
| // Stop the service | ||
| RecordingForegroundService.stop(context) | ||
|
|
||
| // Release audio focus | ||
| releaseAudioFocus() | ||
| } | ||
|
|
||
| val path = currentRecordingPath | ||
| currentRecordingPath = null // State is cleared regardless of outcome | ||
| currentRecordingPath = null | ||
|
|
||
| path?.let { | ||
| val fileUri = Uri.fromFile(File(it)).toString() | ||
| promise.resolve(fileUri) | ||
| } ?: promise.reject(Exception("Recorder not started or path is unavailable.")) | ||
| } catch (e: Exception) { | ||
| mediaRecorder?.release() | ||
| mediaRecorder = null | ||
|
|
||
| // Reset metering state even on error | ||
| meteringEnabled = false | ||
| lastMeteringUpdateTime = 0L | ||
| lastMeteringValue = SILENCE_THRESHOLD_DB | ||
|
|
||
| handler.post { | ||
| if (isServiceBound) { | ||
| try { | ||
| context.unbindService(serviceConnection) | ||
| } catch (ex: Exception) { | ||
| // Ignore | ||
| } | ||
| isServiceBound = false | ||
| } | ||
| RecordingForegroundService.stop(context) | ||
| releaseAudioFocus() | ||
| } | ||
| promise.reject(e) | ||
| } | ||
| } |
There was a problem hiding this comment.
The cleanup logic for unbinding the service, stopping it, and releasing audio focus is duplicated in multiple places (here in stopRecorder, and also in startRecorder). This could be extracted into a private helper function to reduce code duplication and improve maintainability.
| { | ||
| "name": "react-native-nitro-sound", | ||
| "version": "0.2.10", | ||
| "private": true, |
There was a problem hiding this comment.
The "private": true flag has been added. This will prevent the package from being published to the npm registry. If this library is intended to be published, this line should be removed.
1 participant
feat: test record when incoming call
fix: add recording foreground service and fix call interruption
fix: stop record when kill app or crash app
fix: record in background for android
Summary by CodeRabbit
New Features
Performance
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.