fix(dictation): stop calling into the recorder after expo-audio released it - #3467
Conversation
…sed it Tapping Done closed the dictation sheet and killed the app. Registered sheets unmount on hide, so the hook's unmount cleanup runs on every close. expo-audio's useAudioRecorder is the first hook in useDictationRecorder, so useReleasingSharedObject's release() cleanup runs first and detaches the JS recorder from its native counterpart. The cleanup then read recorder.isRecording, which throws NativeSharedObjectNotFoundException on iOS and InvalidSharedObjectIdException on Android. Sheets render as siblings of <Application/>, outside the app's ErrorBoundary, so the throw reached React Native's global handler as a fatal. Dropping the recorder access leaks nothing: release() already stops a running recorder on both platforms. It also lets the setAudioModeAsync call that the throw used to skip actually run, so the iOS session is no longer left active in playAndRecord after dictation. Also corrects the 'sheets stay mounted' claim in CLAUDE.md and the comments written against it.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
📝 WalkthroughWalkthroughThe dictation sheet lifecycle documentation now reflects unmounting on hide. Recorder cleanup no longer accesses released native objects, and tests verify safe unmount behavior and recording-session deactivation. ChangesDictation lifecycle
Estimated code review effort: 2 (Simple) | ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/hooks/useDictationRecorder.test.tsx (1)
2-2: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid adding new
react-test-rendererusage.The repository does not provide a Testing Library renderer to migrate this test to, so keep only the existing usage in
src/hooks/useDictationRecorder.test.tsx; adding a newreact-test-rendererimport contributes to deprecated React testing API usage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/hooks/useDictationRecorder.test.tsx` at line 2, Remove the newly added TestRenderer import and avoid introducing any additional react-test-renderer usage in the useDictationRecorder tests; retain only the repository’s existing usage in useDictationRecorder.test.tsx.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/hooks/useDictationRecorder.test.tsx`:
- Line 2: Remove the newly added TestRenderer import and avoid introducing any
additional react-test-renderer usage in the useDictationRecorder tests; retain
only the repository’s existing usage in useDictationRecorder.test.tsx.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 260b9dce-ae58-45b0-a1c1-d0d1c6a85454
📒 Files selected for processing (4)
CLAUDE.mdsrc/components/dictationModal/dictationModal.tsxsrc/hooks/useDictationRecorder.test.tsxsrc/hooks/useDictationRecorder.ts
|
Re the |
1 participant
Closes #3466
Tapping Done in the dictation sheet closed the sheet and killed the app.
What was happening
Registered sheets unmount on hide (
SheetProviderrenders!visible ? null : <Sheet/>), souseDictationRecorder's unmount cleanup runs on every close, not "rarely" as its comment claimed.useAudioRecorderis the first hook inuseDictationRecorder, so its internaluseReleasingSharedObjectcleanup runs first and callsrecorder.release(), detaching the JS object from its native counterpart. The cleanup declared after it then readrecorder.isRecording, which throwsNativeSharedObjectNotFoundExceptionon iOS andInvalidSharedObjectIdExceptionon Android.Sheets render as siblings of
<Application/>(src/index.tsx), so they sit outside the app'sErrorBoundary. The throw reached React Native's global handler as a fatal.Change
recorder. It clears the ticker and deactivates the recording session, nothing else.recorder.stop()leaks nothing:release()already stops a running recorder on both platforms (iOSsharedObjectWillRelease, AndroidsharedObjectDidRelease->reset).setAudioModeAsync({ allowsRecording: false })used to be skipped because the throw happened on the line above it, leaving the iOS session active in.playAndRecordafter dictation. It now runs.CLAUDE.mdand the comments written against it, and documents that sheets sit outside theErrorBoundary.Test plan
src/hooks/useDictationRecorder.test.tsxmounts the hook against a mock mirroring expo-audio's real hook ordering, where the recorder throws on any member access after release. Verified it fails on the pre-fix code withUnable to find the native shared object associated with given JavaScript object (isRecording)and passes after.yarn test:cigreen: 738 passed, 1 skipped.tsc --noEmitclean, eslint clean on changed files.Summary by CodeRabbit
Bug Fixes
Documentation