Conversation
WPT non-regression comparisonFAIL — 1 regressed section(s) · 0 improved section(s) · overall 2717 → 2716 (-1)
Unchanged sections (27)
Baseline: Workflow run · this comment is updated on every push. |
a16212c to
6dd83cf
Compare
michalsek
left a comment
There was a problem hiding this comment.
a bit of comments for later 👉 👈
maciejmakowski2003
left a comment
There was a problem hiding this comment.
a few issues identified by claude. worth to check.
-
RecordingNotificationReceiver.kt:90 — stopRecordingNatively ignores the stopActiveRecording() return value, so it hides the notification and dispatches recordingNotificationStop even when nothing was stopped. When the handle's slot is empty but a recording is live, this cancels the notification and unsubscribes the mic-typed foreground service while the Oboe stream keeps capturing — no notification, no way to stop. When the stop failed at closeFile(), JS gets a stop event and a null result, silently losing the recording.
-
AudioRecorderHostObject.cpp:35 — the constructor unconditionally overwrites ActiveRecorderHandle's single slot. A second, never-started AudioRecorder displaces a recording one, and when that second one is GC'd, clearRecorder sees an identity match and empties the slot outright — the live recording becomes permanently unreachable to every notification action, and isRecordingOngoing() reports false. The one-recorder assumption is documented, but the failure mode is worse than the doc implies.
-
AndroidAudioRecorder.cpp:253 — stop() is documented "JS thread only" and mutates recordingSegmentPaths_/filePath_ after releasing stopLock; the notification path now calls it from the receiver's executor. If the user starts a new recording while the old stop is blocked in closeFile(), the executor's trailing clear() / filePath_ = "" wipes the new session's path and its later stop() returns no files.
-
ForegroundServiceManager.kt:65 — onServiceDestroyed() clears isServiceRunning unconditionally. A hide-then-show in one tick (recording → playback notification) lets the old instance's onDestroy clear the flag after the restart, and the next unsubscribe then never stops the service — it leaks with no notification behind it.
-
RecordingNotification.kt:235 — paused resets to false when absent while every other option is sticky. A partial show({ contentText: … }) during a natively-paused recording flips the button back to Pause; tapping it no-ops (pauseActiveRecording() returns false), so the notification is stuck and the user can't resume from it.
-
Record.tsx:136 — demo decodes paths[0] only after concatAudioFiles was dropped; with rotateIntervalBytes (which the new docs recommend for exactly this scenario) all segments but the first are silently discarded.
could you please clean-up comments as well.
The recording notification's pause, resume and stop actions now act on the recorder natively, so they keep working after the app task is removed while the foreground service (stopWithTask=false) keeps the recording alive: - ActiveRecorderHandle: process-global one-slot handle to the live recorder (registered by AudioRecorderHostObject), with a consume-once stash of the file info produced by a native stop - NativeRecorderControl: static-JNI entry points callable from Kotlin without a React context; the notification receiver stops/pauses/resumes through it on an executor and still emits the matching AudioEvent so a live app can sync its UI (new event: RECORDING_NOTIFICATION_STOP) - RecordingNotification rewritten to standard NotificationCompat actions (RemoteViews layouts removed), rebuilt on every show(); adds stop action, action titles, deepLinkUri tap routing (ACTION_VIEW) and a chronometer that excludes paused spans; native pause/resume re-post the notification so the action button flips without JS - onErrorAfterClose now restores the pre-teardown state after a stream reclaim instead of force-resuming a paused recording Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
With stopWithTask=false a recording outlives the app UI, but a remounted screen (or relaunched app) had no way to learn about it — recorder state was only reachable through the instance that started it: - new JSI globals backed by ActiveRecorderHandle, surfaced as statics: AudioRecorder.isRecordingOngoing() and the consume-once AudioRecorder.takeLastRecordingResult() for files finalized by the notification stop action (mock parity + jest coverage included) - Record demo mounts directly in the live recorder's state, picks up natively stopped files, keeps the recording alive across screen exits and only enables file output when no session is ongoing (re-enabling mid-recording replaces the writer and resets the duration) - deep-link routing for the notification tap (react-navigation linking), duration displays seeded from the recorder instead of assuming a fresh session, and RecordingTime rewritten to plain state — the animated-prop binding went stale on the frozen value while paused and showed zeros Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
da2b737 to
4b82eda
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Recorder-handle races, active-recorder displacement, stale service types, and misleading file-output success results can break the core recording-survival flow.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (3)
packages/react-native-audio-api/common/cpp/audioapi/core/inputs/ActiveRecorderHandle.cpp:45
- This reads the shared
weak_ptrbefore taking the mutex, racing with recorder registration/destruction. Lock first so theweak_ptr::lock()operation is synchronized with assignments and resets.
packages/react-native-audio-api/common/cpp/audioapi/core/inputs/ActiveRecorderHandle.cpp:54 - As in the other handle methods, accessing
recorder_before acquiringdestructorMutex_races withsetRecorder()/clearRecorder(). Reverse these statements.
packages/react-native-audio-api/common/cpp/audioapi/core/inputs/ActiveRecorderHandle.cpp:63 - The notification-stop path also locks the shared
weak_ptrbefore taking its mutex. Concurrent HostObject creation/destruction can therefore race this read; acquiredestructorMutex_first.
- Files reviewed: 57/58 changed files
- Comments generated: 10
- Review effort level: Balanced
| if (!isIdle()) { | ||
| return Result<NoneType, std::string>::Ok(None); | ||
| } |
| if (!isIdle()) { | ||
| return Result<NoneType, std::string>::Ok(None); | ||
| } |
| sed 's/=.*//; s/[[:space:]]//g' | | ||
| grep -E '^[A-Za-z_][A-Za-z0-9_]*$' || true |
| static isRecordingOngoing(): boolean { | ||
| return globalThis.isRecordingOngoing?.() ?? false; | ||
| } | ||
|
|
||
| static takeLastRecordingResult(): FileInfo | null { |
| | Non-primitive, can be written by audio thread | Triple buffer (see `AnalyserNode` for reference) | | ||
| | CPU-heavy work, must not block JS or audio | `TaskOffloader` on a dedicated worker thread | | ||
| | Context lifecycle (`resume`/`suspend`/`close`) | `scheduleContextPromise` → `pendingPromisesOffloader_` | | ||
| | Platform code (Kotlin) must reach a C++ object with no JS runtime alive | Process-global handle (`ActiveRecorderHandle` — mutex + `weak_ptr`, registered by the HostObject ctor/dtor) + static-JNI `JavaClass` (`NativeRecorderControl`, no HybridData needed). Blocking calls run on a Kotlin executor (`goAsync()` in receivers), never a detached `std::thread` — Kotlin threads are already JNI-attached | |
| title?: string; | ||
| contentText?: string; | ||
| paused?: boolean; // flag indicating whether to display pauseIcon or resumeIcon | ||
| paused?: boolean; |
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
maciejmakowski2003
left a comment
There was a problem hiding this comment.
could you test what happens if user dismisses notification. strip off or edit comments added by ai. verify logs usefulness.
| if (!isIdle()) { | ||
| return Result<NoneType, std::string>::Ok(None); | ||
| } | ||
|
|
||
| std::scoped_lock lock(fileWriterMutex_, errorCallbackMutex_); | ||
| fileProperties_ = properties; | ||
| fileOutputEnabled_.store(true, std::memory_order_release); | ||
| fileOutputConfigured_.store(false, std::memory_order_release); | ||
|
|
||
| if (!isIdle()) { | ||
| AVAudioFormat *resolvedInputFormat = [nativeRecorder_ getResolvedInputFormat]; | ||
| int resolvedBufferSize = [nativeRecorder_ getResolvedBufferSize]; | ||
|
|
||
| if (!hasUsableRecorderFormat(resolvedInputFormat) || resolvedBufferSize <= 0) { | ||
| return Result<NoneType, std::string>::Err( | ||
| "Failed to open file for writing: recorder input format is unavailable"); | ||
| } | ||
|
|
||
| auto writerResult = setupFileWriter(properties); | ||
| if (writerResult.is_err()) { | ||
| fileOutputEnabled_.store(false, std::memory_order_release); | ||
| return Result<NoneType, std::string>::Err(writerResult.unwrap_err()); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
this changed the behavior that we cannot suddenly enable file output in ongoing recorder, thus we don't need that code, we can discuss it internally if we would like to support this, if yes we can revert that
There was a problem hiding this comment.
recheck comments in both .h and .cpp file and use singleton pattern instead of global method
There was a problem hiding this comment.
I do not agree with you, because singleton means that it has to be created somewhere and each consecutive call to constructor would return it, it serves slight different purpose
| // service unwind — otherwise it runs forever. | ||
| activeNotifications[key] = false | ||
| ForegroundServiceManager.unsubscribe(notification) | ||
| Log.d(TAG, "Hiding notification: $key (unsubscribed from foreground service)") |
There was a problem hiding this comment.
it could leave the notification hanging otherwise
|
claude code review findings - could Bugs
JS calls start(), then enableFileOutput(). After that, usesFileOutput() is false: onAudioReady stops writing, and stop() never moves or closes fileWriter_, so the file is truncated and never finalized. On Android, onErrorAfterClose briefly sets the state to Idle between cleanup() and reopening the stream, which opens the same window. Fix: do the isIdle() check after taking the lock.
the isRecordingOngoing() JSI global Also, recorder is declared after lock, so it's destroyed while the lock is still held. If the handle's copy is the last owner, the recorder's destructor (stream close, file close) runs under the mutex. clearRecorder avoids this on purpose, but the other methods don't. Fix: copy the shared_ptr under the lock, unlock, do the work, then relock only to write lastResult_.
Before start() resolves: setRecorder only runs at the end of the async lambda. If the notification is shown first and the user taps Pause in that window, the notification and the microphone foreground service go away while recording is starting. Stop on an empty slot also sends RECORDING_NOTIFICATION_STOP to JS even though nothing was stopped. Consider a separate "no recorder" result, and only hide on the stop action.
Nits |
2484feb to
6d75b49
Compare
Closes #
enableFileOutputcannot be set once recorder is startedIntroduced changes
With stopWithTask=false a recording outlives the app UI, but a remounted screen (or relaunched app) had no way to learn about it — recorder state was only reachable through the instance that started it:
new JSI globals backed by ActiveRecorderHandle, surfaced as statics: AudioRecorder.isRecordingOngoing() and the consume-once AudioRecorder.takeLastRecordingResult() for files finalized by the notification stop action (mock parity + jest coverage included)
Record demo mounts directly in the live recorder's state, picks up natively stopped files, keeps the recording alive across screen exits and only enables file output when no session is ongoing (re-enabling mid-recording replaces the writer and resets the duration)
deep-link routing for the notification tap (react-navigation linking), duration displays seeded from the recorder instead of assuming a fresh session, and RecordingTime rewritten to plain state — the animated-prop binding went stale on the frozen value while paused and showed zeros
possibility of swiping away notification is configurable, it can be easily swiped, thus ending recording or unswipable, making it stick, but in android 14+ it cannot be made for certain notifications, so it is being reattached in this scenario
Checklist