[#49] build record tab with draft recording and playback#158
Open
fel-cesar wants to merge 57 commits into
Open
[#49] build record tab with draft recording and playback#158fel-cesar wants to merge 57 commits into
fel-cesar wants to merge 57 commits into
Conversation
Install expo-audio (~56.0.12) for microphone capture and playback, and expo-crypto (~56.0.4) for RFC 4122 v4 IDs used as recordings.id. Register expo-audio as a config plugin with a Fluent-specific microphone permission string so prebuild injects RECORD_AUDIO and MODIFY_AUDIO_SETTINGS into the Android manifest. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Add Recording / RecordingRow types and RecordingSyncStatus union so the existing recordings schema is typed end-to-end. Introduce repository.insertRecording that atomically demotes the prior is_latest row for a bible_text_id, increments take_number, and inserts the new take, plus deleteRecordingById for the delete flow. Add queries.getBibleTextId to resolve verse coordinates to bible_texts.id and queries.getLatestRecordingForVerse for the Record tab review view. Cover both modules with unit tests over a mocked op-sqlite handle. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Add PausedTakeMarker and get/set/clearPausedTake helpers over the op-sqlite KV store so a paused take (verse, temp file URI, elapsed ms, started-at) survives app backgrounding or crash and can be resumed or recovered on next launch. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Replace the ViewChapter stub with a DraftingPage shell that hosts the selected verse, a disabled ChapterAudioPlayerBar placeholder for the upcoming #47 player, and a DraftingTabBar switching between a Bible placeholder and the new RecordTab. Add useRecorder hook implementing the Idle / Recording / Paused / Review state machine on top of expo-audio: request and track microphone permission (unknown | granted | denied | blocked), capture takes into a temp file, insert them via repository.insertRecording, mint recording IDs with expo-crypto randomUUID, and persist a paused- take marker via the storage KV so pauses survive backgrounding. RecordTab renders verse chevrons, the source-text accordion, per-state controls (start, pause, resume, stop, re-record, delete, playback), a duration counter, a delete confirmation, and a navigate-away prompt. When permission is blocked the tab opens system settings via Linking.openSettings, otherwise it re-requests through the OS dialog. Point AppNavigator.VerseDetail at DraftingPage and delete the legacy ViewChapter screen. Cover the hook and screen with Jest tests using the LoginScreen mocking pattern. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Split the pill-shaped record CTA into an 88px red circle carrying just the target icon, a centered verse-reference label below it, and a disabled play-button placeholder that hints at future draft playback. Add a dedicated recordAccent theme token so the red is not overloaded with destructive semantics. Update the RecordTab smoke test for the new record-start-label and record-play-idle-placeholder test IDs. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Swap the recording controls so the muted stop circle sits on the left and the prominent red pause circle on the right, mirror the same layout for paused (stop + resume), and lift the duration counter above the buttons in muted tabular-nums. Retune the live waveform to a narrower, taller red band with 22 rounded bars and split the bar style into live (red) and static (blue) so the review waveform keeps its own accent. Add per-state coaching tips under the buttons — "Tap pause to study the source, stop to finish." while recording, and "Recording paused — review the source below, then resume." while paused. Rework the review state to mirror the idle affordance: a muted record-done placeholder next to a prominent blue play circle, with Re-record and Delete moved into labeled rounded rectangles (light card fill and destructive-outlined) using a shared placeholder-circle style with the idle play hint. Extend RecordTab.test.tsx to cover the recording tip, the paused-tip wording, and the new review-state controls. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Compose prevDisabled / nextDisabled from the existing edge flags plus a recording-active check so the prev/next chevrons cannot navigate away mid-take. Paused, idle, and review keep their existing guards (paused still prompts Resume/Discard via withPausedGuard). The muted foreground color and reduced-opacity icon style follow the disabled state so the lock is visually obvious. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Recorder previously polled recorder.currentTime which keeps advancing on some devices while the native pipeline is paused, so elapsedMs inflated past a Resume by the length of every prior pause. Replace it with wall-clock timing: baseElapsedRef accumulates completed segments, runningSinceRef marks the start of the current active segment, and each tick reports base + (now - runningSince). Pause commits the segment before native pause, resume opens a new segment, and stop derives the final duration from the same refs so persistence matches the display. Drop the tick interval to 50 ms so the readout is smooth at centisecond precision, and format duration as MM:SS:HH with zero-padded minutes so verses over a minute align in the UI. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Switch the verse-nav row to justifyContent: 'space-around' so the chevrons and reference sit with balanced margins instead of hugging the edges. Delete the two commented-out waveform style lines that were left behind during recording-state polish. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Split the player concern out of useRecorder into useDraftPlayback, which takes only the take's file URI and owns the playback-side audio routing and rewind-on-finish. useRecorder composes it with a one-way dependency and stops playback explicitly before re-record/delete, keeping its public API unchanged. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Hook the review play button up to togglePlayback with a play/pause affordance, and stop playback when navigating between verses. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Recordings need to move out of the evictable cache into the document directory and be read back at absolute paths, which requires the expo-file-system File/Directory/Paths APIs. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Introduce recordingStorage with a user/project/book/chapter/verse key layout rooted at the document directory. Persist only relative keys and resolve absolute uris at read time so paths survive reinstalls, plus best-effort move/delete helpers for takes and per-user cleanup. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Add user_id and chapter_assignment_id columns (with migrations for existing DBs), thread them through the Recording types, inserts and verse queries, and resolve a project id for a project unit to build storage keys. deleteRecordingById now also unlinks the durable file. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Accept optional attribution context (user/project/book/chapter/verse) via a ref, move each committed take out of the cache into its durable key before inserting the row so a record never points at an evictable file, and resolve relative keys to absolute uris for playback. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Resolve the active user and owning project id in DraftingPage and pass them, along with the chapter assignment and book code, through RecordTab into useRecorder so committed takes are stored under the correct key. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Configure useAudioRecorder to write into the document directory instead of the evictable cache so paused or backgrounded partial takes survive a process kill until they are moved or deleted. On discardPaused, read the persisted paused-take marker (recorder.uri is unreliable after a kill) and unlink the durable partial file before clearing state. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Split useRecorder into a use-case-agnostic state machine that owns only recording mechanics (transitions, elapsed ticking, permissions, background auto-pause, playback) and injects persistence, storage layout, and domain identity through a RecorderAdapter. Move all verse-specific SQLite, durable-storage, and paused-marker wiring into a new useVerseRecorder adapter and point RecordTab at it, so the recorder can be reused for other capture use cases without dragging DB concerns along. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Read the committed take's playback uri via adapterRef.current instead of the adapter prop, matching every other adapter call in useRecorder. This keeps the ref indirection consistent so the hook never depends on a fresh adapter object reference when handing a uri to useDraftPlayback. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
The recorder loads its state from the local DB asynchronously, so the tab briefly rendered the idle "Record" button before snapping to review when an existing take resolved. Hold the record/review UI back until the recorder is ready (capped at 100ms so a slow load still surfaces), showing a height-reserved placeholder meanwhile so the layout does not shift. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Split the monolithic Record tab into components, hooks, and utils under drafting/record and colocate useVerseRecorder with its screen. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Reject corrupted paused-take markers unless bibleTextId is a finite number matching the KV lookup key. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI (base), Organization UI (inherited) Review profile: CHILL Plan: Pro Plus Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis PR adds a verse-recording feature (Record tab) with a generic recorder state machine, durable AAC audio storage, database schema/queries for recordings, recovery of paused takes after app kill, permission/tab-switch guards, and full UI components; drafting context/screen and expo dependencies are updated accordingly. ChangesDrafting Record Tab Feature
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant RecordTab
participant useVerseRecorder
participant recordingStorage
participant repository
RecordTab->>useVerseRecorder: stop()
useVerseRecorder->>recordingStorage: concatenateAacSegments()
useVerseRecorder->>recordingStorage: moveIntoStore()
useVerseRecorder->>repository: insertRecording(input)
repository-->>useVerseRecorder: Recording
useVerseRecorder-->>RecordTab: status = review
sequenceDiagram
participant HomeScreen
participant useRecordingRecovery
participant storage
participant queries
participant navigation
HomeScreen->>useRecordingRecovery: enabled = true
useRecordingRecovery->>storage: findPausedTake()
useRecordingRecovery->>queries: getVerseDetailNavByChapterAssignment()
useRecordingRecovery->>navigation: Alert prompt
navigation-->>useRecordingRecovery: Continue or Discard
Possibly related issues
Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Nitpick comments (8)
src/app/tabs/drafting/record/hooks/useRecordTabGuards.test.ts (1)
91-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winBeforehand callback behavior isn't tested, only registration.
This test confirms
addListener('beforeRemove', ...)was called but never invokes the captured callback to verifypreventDefault, the discard alert, andnavigation.dispatchon discard — the core data-loss-prevention logic in the effect body (Lines 78-98 ofuseRecordTabGuards.ts) is untested.it('discards and dispatches the pending action on beforeRemove', async () => { const alertSpy = jest.spyOn(Alert, 'alert').mockImplementation(() => {}); const discardPaused = jest.fn().mockResolvedValue(undefined); renderHook(() => useRecordTabGuards({ status: 'paused', permission: 'granted', requestPermission: jest.fn(), discardPaused, navigation: mockNavigation as never, }), ); const callback = mockNavigation.addListener.mock.calls[0][1]; const preventDefault = jest.fn(); const event = { preventDefault, data: { action: { type: 'GO_BACK' } } }; callback(event); expect(preventDefault).toHaveBeenCalled(); const buttons = alertSpy.mock.calls[0][2]; await buttons.find(b => b.text === 'Discard').onPress(); expect(discardPaused).toHaveBeenCalled(); expect(mockNavigation.dispatch).toHaveBeenCalledWith(event.data.action); alertSpy.mockRestore(); });As per path instructions,
**/*.{test,spec}.{ts,tsx}: "Add tests for non-trivial logic".🤖 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/app/tabs/drafting/record/hooks/useRecordTabGuards.test.ts` around lines 91 - 106, The current test for useRecordTabGuards only checks that addListener('beforeRemove', ...) is registered, but it does not verify the actual guard behavior. Update the test to capture the callback passed to mockNavigation.addListener, invoke it with a beforeRemove event, and assert preventDefault is called, Alert.alert is shown, discardPaused runs on the Discard action, and navigation.dispatch receives the pending action. Use the useRecordTabGuards hook and mockNavigation to locate the effect logic being exercised.Source: Path instructions
src/app/tabs/drafting/record/components/SourceTextPanel.tsx (1)
15-17: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
Platform.OScheck given Android-only target.Since this repo is permanently Android-only, this conditional always evaluates to true. It's harmless boilerplate, but could be simplified to a direct call.
Based on learnings, this repository is permanently Android-only with no iOS app, so guarding this call behind a platform check adds no value.
♻️ Optional simplification
-if (Platform.OS === 'android') { - UIManager.setLayoutAnimationEnabledExperimental?.(true); -} +UIManager.setLayoutAnimationEnabledExperimental?.(true);🤖 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/app/tabs/drafting/record/components/SourceTextPanel.tsx` around lines 15 - 17, The Android-only platform guard around the UIManager.setLayoutAnimationEnabledExperimental call in SourceTextPanel is redundant. Remove the Platform.OS check and invoke the experimental layout animation enablement directly so the component stays simpler and still works for the app’s single-platform target.src/app/tabs/drafting/record/components/RecordingControls.tsx (1)
79-155: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate JSX between 'recording' and 'paused' states.
Both blocks share identical structure (duration text, stop button, primary action circle, tip) and differ only in the primary button's icon/label/handler and tip copy. Consider consolidating into one render path parameterized by these differences.
♻️ Proposed consolidation
-{status === 'recording' && ( - <View style={styles.captureGroup}> - ... - <Pause .../> - ... - Tap pause to study the source, stop to finish. - </View> -)} - -{status === 'paused' && ( - <View style={styles.captureGroup}> - ... - <CircleDot .../> - ... - Recording paused — review the source below, then resume. - </View> -)} +{(status === 'recording' || status === 'paused') && ( + <View style={styles.captureGroup}> + <Text style={styles.duration} testID="record-duration"> + {formatDuration(elapsedMs)} + </Text> + <View style={styles.captureButtonsRow}> + <TouchableOpacity + style={styles.stopCircleButton} + onPress={onStop} + accessibilityRole="button" + accessibilityLabel="Stop recording" + testID="record-stop-button" + > + <Square size={26} color={theme.colors.foreground} strokeWidth={listIconStrokeWidth} /> + </TouchableOpacity> + <TouchableOpacity + style={styles.primaryActionCircle} + onPress={status === 'recording' ? onPause : onResume} + accessibilityRole="button" + accessibilityLabel={status === 'recording' ? 'Pause recording' : 'Resume recording'} + testID={status === 'recording' ? 'record-pause-button' : 'record-resume-button'} + > + {status === 'recording' ? ( + <Pause size={30} color={theme.colors.primaryForeground} strokeWidth={listIconStrokeWidth} /> + ) : ( + <CircleDot size={30} color={theme.colors.primaryForeground} strokeWidth={listIconStrokeWidth} /> + )} + </TouchableOpacity> + </View> + <Text style={styles.captureTip} testID="record-tip"> + {status === 'recording' + ? 'Tap pause to study the source, stop to finish.' + : 'Recording paused — review the source below, then resume.'} + </Text> + </View> +)}🤖 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/app/tabs/drafting/record/components/RecordingControls.tsx` around lines 79 - 155, The rendering in RecordingControls is duplicated between the recording and paused states, so consolidate the shared JSX into one path. Extract the common captureGroup layout and parameterize the state-specific pieces in RecordingControls: the primary action button’s icon, accessibilityLabel/testID, onPress handler, and the tip text. Keep the stop button and duration rendering shared, and use a small config/conditional based on status to select the pause vs resume behavior.src/app/tabs/drafting/record/components/RecordingControls.test.tsx (1)
1-184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd interaction coverage for the remaining controls.
Tests only assert
onDeleteandonTogglePlaybackare wired;onStart,onPause,onResume,onStop, andonReRecordpresses are never fired/asserted, despite these buttons being rendered in the idle/recording/paused states.As per path instructions: "Add tests for non-trivial logic" — button-to-callback wiring is exactly this kind of logic.
Example additions
it('shows pause and stop controls with a duration and tip when recording', () => { + const onPause = jest.fn(); + const onStop = jest.fn(); render( <RecordingControls status="recording" ... - onPause={jest.fn()} + onPause={onPause} ... - onStop={jest.fn()} + onStop={onStop} ... />, ); ... + fireEvent.press(screen.getByTestId('record-pause-button')); + expect(onPause).toHaveBeenCalledTimes(1); + fireEvent.press(screen.getByTestId('record-stop-button')); + expect(onStop).toHaveBeenCalledTimes(1); });🤖 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/app/tabs/drafting/record/components/RecordingControls.test.tsx` around lines 1 - 184, The test suite for RecordingControls is missing press coverage for several rendered actions, so add interaction assertions for the remaining callback wiring. Update the existing idle, recording, paused, and review cases in RecordingControls.test.tsx to press the rendered controls and verify onStart, onPause, onResume, onStop, and onReRecord are each called from the appropriate state-specific buttons. Keep the existing onDelete and onTogglePlayback checks, and use the current test IDs from RecordingControls to locate each control.Source: Path instructions
src/app/tabs/drafting/DraftingPage.tsx (1)
43-43: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant
getActiveUserId()call on every render.
userIdis recomputed each render rather than being read once/memoized. Minor, but worth tidying since it doesn't depend on any render-time state.🤖 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/app/tabs/drafting/DraftingPage.tsx` at line 43, The DraftingPage component recomputes userId by calling getActiveUserId() on every render even though it does not depend on render state. Update DraftingPage so the active user id is read once or otherwise memoized outside the render path, and keep using the existing userId variable where needed to avoid repeated lookups.src/db/queries.ts (1)
424-460: 🗄️ Data Integrity & Integration | 🔵 Trivial | 💤 Low valueInconsistent null-vs-undefined convention for
Recordingfields across producers.Here, absent optional fields (
userId,chapterAssignmentId,blobKey,durationMs,fileSizeBytes,uploadError) map toundefinedvia?? undefined, butinsertRecordinginsrc/db/repository.ts(lines 540-553) builds the sameRecordingshape using?? nullfor the equivalent fields. Since theRecordingtype allows both (field?: T | null), this passes type-checking but produces objects with different shapes depending on the source, which can trip up consumers doing strict=== null/=== undefinedchecks.Consider extracting a single
toRecording(row): Recordingnormalizer used by both call sites to guarantee a consistent representation.🤖 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/db/queries.ts` around lines 424 - 460, The `Recording` object is being produced with mixed absent-value conventions (`undefined` here versus `null` in `insertRecording`), which creates inconsistent shapes for consumers. Update `getLatestRecordingForVerse` to use the same normalization as the other producer in `src/db/repository.ts`, ideally by extracting a shared `toRecording` helper used by both `getLatestRecordingForVerse` and `insertRecording`. Make sure the optional fields (`userId`, `chapterAssignmentId`, `blobKey`, `durationMs`, `fileSizeBytes`, `uploadError`) are mapped consistently everywhere.src/db/queries.recordings.test.ts (1)
42-84: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for
user_id/chapter_assignment_idmapping.The mock row omits
user_idandchapter_assignment_id, so the mapping of these newly-added columns (the main schema change in this PR) is never verified with an actual present value — only the "missing key → undefined" path is implicitly exercised.✅ Suggested additional coverage
{ id: 'rec-1', bible_text_id: 42, + user_id: 'user-9', + chapter_assignment_id: 7, local_file_path: '/tmp/rec-1.m4a',and add matching
userId: 'user-9', chapterAssignmentId: 7to the expectedtoEqualobject.🤖 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/db/queries.recordings.test.ts` around lines 42 - 84, The `getLatestRecordingForVerse` test only covers `snake_case` fields when `user_id` and `chapter_assignment_id` are absent, so the new column mapping is not verified for real values. Update the mock row in `src/db/queries.recordings.test.ts` to include present `user_id` and `chapter_assignment_id` values, and extend the expected object in the `maps snake_case rows into the Recording shape` assertion to include the corresponding `userId` and `chapterAssignmentId` fields. This should exercise the row-to-recording mapping path in `getLatestRecordingForVerse` for the newly added schema columns.src/services/recordingStorage.test.ts (1)
1-165: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for
deleteUserTreeand traversal edge case.
deleteUserTreeis exported fromrecordingStorage.tsbut isn't imported or tested anywhere in this suite. Given it performs a real filesystem delete (with existence check and error swallowing), it warrants its own tests analogous todeleteRecordingFile. Also consider adding a case forbookCode/userIdvalues of"."/".."to lock in the sanitize fix suggested inrecordingStorage.ts.🤖 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/services/recordingStorage.test.ts` around lines 1 - 165, The test suite is missing coverage for the exported deleteUserTree helper in recordingStorage.ts, so add tests that import and exercise deleteUserTree with mocked expo-file-system behavior, mirroring the existing deleteRecordingFile cases for existing and missing paths. Also add a buildRecordingKey/recordingKeySegments case that uses unsafe traversal inputs like userId or bookCode set to "." or ".." to verify the sanitize logic prevents path traversal, using the same symbols deleteUserTree, buildRecordingKey, and recordingKeySegments to locate the affected code.
🤖 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.
Inline comments:
In `@src/app/tabs/drafting/DraftingPage.tsx`:
- Around line 88-109: The verse ID state in DraftingPage’s useEffect is not
cleared immediately when selectedVerse changes, so RecordTab can keep using a
stale bibleTextIdForSelectedVerse until getBibleTextId resolves. Update the
effect around resolveId to setBibleTextId(null) as soon as chapterData or
selectedVerse changes before starting the async lookup, then only set the
resolved id if the request is still current and not cancelled.
In `@src/app/tabs/drafting/record/hooks/useRecordTabGuards.ts`:
- Around line 61-64: The discard-then-continue handlers in useRecordTabGuards
should be guarded so a failure in discardPaused() does not cause an unhandled
rejection or block the follow-up action. Update both onPress handlers in the
record tab guard flow to wrap await discardPaused() in try/catch, only call
action() or navigation.dispatch(...) after a successful discard, and handle/log
the error path so the user is not left stuck without feedback.
- Around line 75-102: The leave guard in useRecordTabGuards only handles the
paused state, so active recording can still exit without prompting. Update the
useEffect beforeRemove listener to also cover status === 'recording' and either
block navigation with a confirmation or pause/stop/discard the take before
dispatching event.data.action. Keep the fix localized around beforeRemove,
navigation.addListener, and discardPaused so both paused and recording states
are guarded consistently.
In `@src/app/tabs/drafting/record/hooks/useVerseRecorder.ts`:
- Around line 84-120: The onCommit flow in useVerseRecorder currently cleans up
only moveIntoStore failures, but a later insertRecording rejection can leave a
durable file orphaned with no DB row. Wrap the insertRecording call in error
handling after moveIntoStore succeeds, and if it fails, remove/unlink the moved
file using the returned moved.key before rethrowing or returning null so the
RecorderAdapter can treat it as a recoverable failure.
In `@src/app/tabs/drafting/record/RecordTab.tsx`:
- Around line 162-164: The pause/resume/stop handlers in RecordTab are firing
recorder.pause(), recorder.resume(), and recorder.stop() without awaiting or
handling rejections, so any recorder failure is silently ignored. Update the
onPause, onResume, and onStop callbacks to use the same async/error-handling
pattern as handleStartPress and handleReRecordPress: await the recorder methods,
catch failures, and surface an appropriate user-visible error or fallback state.
In `@src/db/repository.ts`:
- Around line 556-570: `deleteRecordingById` currently deletes the row and file
but leaves no replacement marked as latest, so the previous take is never
surfaced again. Update `deleteRecordingById` to run the delete plus re-promotion
logic in one transaction on the same `db` connection: after removing the target
row, find the most recent remaining recording for the same verse and set its
`is_latest` back to 1 before commit. Use the existing `deleteRecordingById` and
`getLatestRecordingForVerse`/recording schema concepts to locate the right row
and ensure the re-promotion happens atomically.
In `@src/hooks/useRecorder.ts`:
- Around line 421-430: The delete flow in useRecorder’s deleteCurrent callback
does not handle failures from
adapterRef.current.deleteCommitted(currentRecording), so an exception can skip
the later state resets and surface as an unhandled rejection. Wrap the
deleteCommitted call in try/catch inside deleteCurrent, keep the existing
“review”/currentRecording guard, and only run the reset state updates after a
successful delete; on error, leave the current recording intact and surface/log
the failure through the same hook’s error handling path if available.
- Around line 350-399: The commit path in useRecorder’s commitRecording
currently awaits recorder.stop() without any error handling, unlike
discardPaused. Wrap recorder.stop() in the same try/catch pattern used
elsewhere, and make sure clearTick() still runs on failure so the interval
doesn’t leak. If stop throws, log the error, reset the status appropriately, and
return before continuing with fileUri/adapterRef.current.onCommit.
- Around line 342-348: The resume flow in useRecorder’s resume callback should
not allow rehydrated paused takes to continue, because restored paused state
cannot reconnect to the original native AudioRecorder session. Add a persisted
live-session marker or session token to PausedTakeState, have the mount-time
rehydration logic distinguish live versus restored pauses, and route restored
paused takes away from resume and toward Discard while keeping the live
pause/resume path unchanged.
In `@src/services/recordingStorage.ts`:
- Around line 47-50: sanitizeSegment in recordingStorage.ts still allows
reserved path segments like "." and "..", which can cause traversal when
recordingKeySegments builds paths. Update sanitizeSegment and/or
recordingKeySegments to reject or normalize any segment that resolves to "." or
"..", especially the bookCode segment, so moveIntoStore, resolveRecordingUri,
and deleteUserTree never pass traversal-safe but reserved segments into
Directory/File paths.
---
Nitpick comments:
In `@src/app/tabs/drafting/DraftingPage.tsx`:
- Line 43: The DraftingPage component recomputes userId by calling
getActiveUserId() on every render even though it does not depend on render
state. Update DraftingPage so the active user id is read once or otherwise
memoized outside the render path, and keep using the existing userId variable
where needed to avoid repeated lookups.
In `@src/app/tabs/drafting/record/components/RecordingControls.test.tsx`:
- Around line 1-184: The test suite for RecordingControls is missing press
coverage for several rendered actions, so add interaction assertions for the
remaining callback wiring. Update the existing idle, recording, paused, and
review cases in RecordingControls.test.tsx to press the rendered controls and
verify onStart, onPause, onResume, onStop, and onReRecord are each called from
the appropriate state-specific buttons. Keep the existing onDelete and
onTogglePlayback checks, and use the current test IDs from RecordingControls to
locate each control.
In `@src/app/tabs/drafting/record/components/RecordingControls.tsx`:
- Around line 79-155: The rendering in RecordingControls is duplicated between
the recording and paused states, so consolidate the shared JSX into one path.
Extract the common captureGroup layout and parameterize the state-specific
pieces in RecordingControls: the primary action button’s icon,
accessibilityLabel/testID, onPress handler, and the tip text. Keep the stop
button and duration rendering shared, and use a small config/conditional based
on status to select the pause vs resume behavior.
In `@src/app/tabs/drafting/record/components/SourceTextPanel.tsx`:
- Around line 15-17: The Android-only platform guard around the
UIManager.setLayoutAnimationEnabledExperimental call in SourceTextPanel is
redundant. Remove the Platform.OS check and invoke the experimental layout
animation enablement directly so the component stays simpler and still works for
the app’s single-platform target.
In `@src/app/tabs/drafting/record/hooks/useRecordTabGuards.test.ts`:
- Around line 91-106: The current test for useRecordTabGuards only checks that
addListener('beforeRemove', ...) is registered, but it does not verify the
actual guard behavior. Update the test to capture the callback passed to
mockNavigation.addListener, invoke it with a beforeRemove event, and assert
preventDefault is called, Alert.alert is shown, discardPaused runs on the
Discard action, and navigation.dispatch receives the pending action. Use the
useRecordTabGuards hook and mockNavigation to locate the effect logic being
exercised.
In `@src/db/queries.recordings.test.ts`:
- Around line 42-84: The `getLatestRecordingForVerse` test only covers
`snake_case` fields when `user_id` and `chapter_assignment_id` are absent, so
the new column mapping is not verified for real values. Update the mock row in
`src/db/queries.recordings.test.ts` to include present `user_id` and
`chapter_assignment_id` values, and extend the expected object in the `maps
snake_case rows into the Recording shape` assertion to include the corresponding
`userId` and `chapterAssignmentId` fields. This should exercise the
row-to-recording mapping path in `getLatestRecordingForVerse` for the newly
added schema columns.
In `@src/db/queries.ts`:
- Around line 424-460: The `Recording` object is being produced with mixed
absent-value conventions (`undefined` here versus `null` in `insertRecording`),
which creates inconsistent shapes for consumers. Update
`getLatestRecordingForVerse` to use the same normalization as the other producer
in `src/db/repository.ts`, ideally by extracting a shared `toRecording` helper
used by both `getLatestRecordingForVerse` and `insertRecording`. Make sure the
optional fields (`userId`, `chapterAssignmentId`, `blobKey`, `durationMs`,
`fileSizeBytes`, `uploadError`) are mapped consistently everywhere.
In `@src/services/recordingStorage.test.ts`:
- Around line 1-165: The test suite is missing coverage for the exported
deleteUserTree helper in recordingStorage.ts, so add tests that import and
exercise deleteUserTree with mocked expo-file-system behavior, mirroring the
existing deleteRecordingFile cases for existing and missing paths. Also add a
buildRecordingKey/recordingKeySegments case that uses unsafe traversal inputs
like userId or bookCode set to "." or ".." to verify the sanitize logic prevents
path traversal, using the same symbols deleteUserTree, buildRecordingKey, and
recordingKeySegments to locate the affected code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: df75ea37-0995-45b1-abdc-aea6f73c71e1
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (42)
.cursor/rules/project-overview.mdcapp.config.tsdocs/AGENT_ONBOARDING.mdpackage.jsonsrc/app/tabs/ViewChapter.tsxsrc/app/tabs/drafting/BibleTab.tsxsrc/app/tabs/drafting/ChapterAudioPlayerBar.tsxsrc/app/tabs/drafting/DraftingPage.tsxsrc/app/tabs/drafting/DraftingTabBar.tsxsrc/app/tabs/drafting/record/RecordTab.test.tsxsrc/app/tabs/drafting/record/RecordTab.tsxsrc/app/tabs/drafting/record/components/RecordingControls.test.tsxsrc/app/tabs/drafting/record/components/RecordingControls.tsxsrc/app/tabs/drafting/record/components/RecordingWaveform.test.tsxsrc/app/tabs/drafting/record/components/RecordingWaveform.tsxsrc/app/tabs/drafting/record/components/SourceTextPanel.test.tsxsrc/app/tabs/drafting/record/components/SourceTextPanel.tsxsrc/app/tabs/drafting/record/components/VerseNav.test.tsxsrc/app/tabs/drafting/record/components/VerseNav.tsxsrc/app/tabs/drafting/record/hooks/useRecordTabGuards.test.tssrc/app/tabs/drafting/record/hooks/useRecordTabGuards.tssrc/app/tabs/drafting/record/hooks/useVerseRecorder.test.tssrc/app/tabs/drafting/record/hooks/useVerseRecorder.tssrc/app/tabs/drafting/record/utils/recordTabUtils.test.tssrc/app/tabs/drafting/record/utils/recordTabUtils.tssrc/db/index.tssrc/db/queries.recordings.test.tssrc/db/queries.tssrc/db/repository.recordings.test.tssrc/db/repository.tssrc/db/schema.tssrc/hooks/useDraftPlayback.test.tssrc/hooks/useDraftPlayback.tssrc/hooks/useRecorder.test.tssrc/hooks/useRecorder.tssrc/navigation/AppNavigator.tsxsrc/services/recordingStorage.test.tssrc/services/recordingStorage.tssrc/services/storage.test.tssrc/services/storage.tssrc/theme/tokens.tssrc/types/db/types.ts
💤 Files with no reviewable changes (1)
- src/app/tabs/ViewChapter.tsx
Precompute static bar heights in StyleSheet and split live active/paused styles so react-native/no-inline-styles lint passes cleanly. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Install SDK 56-aligned expo-asset and register its config plugin so expo-doctor passes and native prebuild deduplicates expo-asset/constants. Refs #49 Co-authored-by: Cursor <cursoragent@cursor.com>
Update the kill-resilience doc for the home recovery prompt (findPausedTake, Continue/Discard) and the marker-at-start persistence, and note the timer-vs-audio duration trade-off on kill. Refs #49
The committed durationMs came from the wall-clock timer, which undercounts the real audio length (worst after a hard kill, where a Stop-from-prompt could commit elapsedMs = 0). Parse the merged file's ADTS frames to get the sample-accurate length and use it as the source of truth, falling back to the timer only when the probe yields nothing. Also surface the persisted duration in the Review timer via an optional resolveDurationMs adapter hook. Refs #49
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/app/tabs/drafting/record/components/RecordingControls.tsx (1)
140-212: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winDuplicated discard-button JSX; inconsistent
onDiscardguard.The discard button markup at Lines 176-189 and 192-207 is identical (same testID/label/icon/styles) but only the second copy guards on
onDiscardbeing defined. IfonDiscardis omitted whilecanResumeisfalse, the button still renders and silently no-ops on press.Extract a single discard-button render and reuse it in both branches, guarding on
onDiscardin both places.♻️ Proposed consolidation
+ const discardButton = onDiscard ? ( + <TouchableOpacity + style={styles.destructiveButton} + onPress={onDiscard} + accessibilityRole="button" + accessibilityLabel="Discard recovered take" + testID="record-discard-button" + > + <Trash2 + size={18} + color={theme.colors.destructive} + strokeWidth={listIconStrokeWidth} + /> + <Text style={styles.destructiveLabel}>Discard take</Text> + </TouchableOpacity> + ) : null; + {status === RecorderStatus.Paused && ( <View style={styles.captureGroup}> <Text style={styles.duration} testID="record-duration"> {formatDuration(elapsedMs)} </Text> <View style={styles.captureButtonsRow}> {canResume ? ( <> <TouchableOpacity ... testID="record-stop-button">...</TouchableOpacity> <TouchableOpacity ... testID="record-resume-button">...</TouchableOpacity> </> - ) : ( - <TouchableOpacity - style={styles.destructiveButton} - onPress={onDiscard} - accessibilityRole="button" - accessibilityLabel="Discard recovered take" - testID="record-discard-button" - > - <Trash2 size={18} color={theme.colors.destructive} strokeWidth={listIconStrokeWidth} /> - <Text style={styles.destructiveLabel}>Discard take</Text> - </TouchableOpacity> - )} + ) : ( + discardButton + )} </View> - {canResume && isRecovered && onDiscard ? ( - <TouchableOpacity - style={styles.destructiveButton} - onPress={onDiscard} - accessibilityRole="button" - accessibilityLabel="Discard recovered take" - testID="record-discard-button" - > - <Trash2 size={18} color={theme.colors.destructive} strokeWidth={listIconStrokeWidth} /> - <Text style={styles.destructiveLabel}>Discard take</Text> - </TouchableOpacity> - ) : null} + {canResume && isRecovered ? discardButton : null} <Text style={styles.captureTip} testID="record-tip"> {pausedTip(canResume, isRecovered)} </Text> </View> )}🤖 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/app/tabs/drafting/record/components/RecordingControls.tsx` around lines 140 - 212, The paused-state JSX in RecordingControls contains duplicated discard-button markup, and the first branch renders a discard action without checking whether onDiscard exists. Consolidate the discard button into a single reusable render path in RecordingControls and apply the same onDiscard guard everywhere it is used so the button only appears when the handler is available, while keeping the existing testID, accessibilityLabel, and styling consistent.src/hooks/useRecorder.test.ts (1)
1-99: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winLGTM!
Good coverage of rehydration, kill-recovery, background/foreground auto-resume counteraction, and error paths. Consider adding a case where
mockRecorder.uriisnull/unset at the momentstart()fires and only appears later, thenpause()is called — this would catch thepauseInternalsegment-registration gap flagged inuseRecorder.ts.Also applies to: 101-205, 206-240, 242-333, 334-454, 455-579, 580-733
🤖 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/useRecorder.test.ts` around lines 1 - 99, Add a test in useRecorder.test.ts that covers starting recording when mockRecorder.uri is initially null or unset and only becomes available afterward, then calling pause() to verify the paused segment is still registered correctly. Focus the scenario around useRecorder, pause(), and the pauseInternal path so it catches the segment-registration gap caused by late URI availability. Use the existing mockRecorder and makeAdapter setup to simulate the delayed URI update and assert the paused state/commit behavior reflects a recorded segment.
🤖 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.
Inline comments:
In `@src/app/screens/hooks/useRecordingRecovery.ts`:
- Around line 49-62: The promptRecovery flow in useRecordingRecovery currently
bails out when getVerseDetailNavByChapterAssignment returns null or
take.verseNumber is undefined, which leaves unrecoverable paused takes and their
segment files behind forever. Update this branch to explicitly clear the
PausedTakeMarker and remove the associated segment storage before returning,
using the existing recovery/cleanup helpers around promptRecovery so orphaned
takes can be discarded even when no verse resolution is possible.
In `@src/services/recordingStorage.ts`:
- Around line 148-165: concatenateAacSegments currently leaves the temporary
merge file behind if a bytes() or write() call fails during the append loop.
Update concatenateAacSegments to wrap the merged file creation/append work in a
try/catch, and in the catch path delete the temporary merged.uri before
rethrowing. Keep the fix localized to concatenateAacSegments and the merged File
handling so the cleanup always runs on mid-loop failure.
---
Nitpick comments:
In `@src/app/tabs/drafting/record/components/RecordingControls.tsx`:
- Around line 140-212: The paused-state JSX in RecordingControls contains
duplicated discard-button markup, and the first branch renders a discard action
without checking whether onDiscard exists. Consolidate the discard button into a
single reusable render path in RecordingControls and apply the same onDiscard
guard everywhere it is used so the button only appears when the handler is
available, while keeping the existing testID, accessibilityLabel, and styling
consistent.
In `@src/hooks/useRecorder.test.ts`:
- Around line 1-99: Add a test in useRecorder.test.ts that covers starting
recording when mockRecorder.uri is initially null or unset and only becomes
available afterward, then calling pause() to verify the paused segment is still
registered correctly. Focus the scenario around useRecorder, pause(), and the
pauseInternal path so it catches the segment-registration gap caused by late URI
availability. Use the existing mockRecorder and makeAdapter setup to simulate
the delayed URI update and assert the paused state/commit behavior reflects a
recorded segment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 2b8cc883-ea59-424a-aeab-fd6ce398abb2
📒 Files selected for processing (28)
docs/recording-kill-resilience.mdsrc/app/screens/DraftingScreen.tsxsrc/app/screens/HomeScreen.tsxsrc/app/screens/hooks/useRecordingRecovery.test.tssrc/app/screens/hooks/useRecordingRecovery.tssrc/app/tabs/RecordTab.tsxsrc/app/tabs/drafting/record/RecordTab.test.tsxsrc/app/tabs/drafting/record/RecordTab.tsxsrc/app/tabs/drafting/record/components/RecordingControls.test.tsxsrc/app/tabs/drafting/record/components/RecordingControls.tsxsrc/app/tabs/drafting/record/components/RecordingWaveform.test.tsxsrc/app/tabs/drafting/record/components/RecordingWaveform.tsxsrc/app/tabs/drafting/record/hooks/useRecordTabGuards.test.tssrc/app/tabs/drafting/record/hooks/useRecordTabGuards.tssrc/app/tabs/drafting/record/hooks/useVerseRecorder.test.tssrc/app/tabs/drafting/record/hooks/useVerseRecorder.tssrc/components/layout/DraftingTabBar.tsxsrc/db/queries.nav.test.tssrc/db/queries.tssrc/hooks/useRecorder.test.tssrc/hooks/useRecorder.tssrc/services/recordingStorage.test.tssrc/services/recordingStorage.tssrc/services/storage.test.tssrc/services/storage.tssrc/types/drafting/types.tssrc/types/navigation/types.tssrc/types/recording/types.ts
✅ Files skipped from review due to trivial changes (1)
- docs/recording-kill-resilience.md
🚧 Files skipped from review as they are similar to previous changes (4)
- src/app/tabs/drafting/record/components/RecordingWaveform.test.tsx
- src/app/tabs/drafting/record/hooks/useVerseRecorder.ts
- src/app/tabs/drafting/record/RecordTab.tsx
- src/app/tabs/drafting/record/RecordTab.test.tsx
Comment on lines
+49
to
+62
| async function promptRecovery(take: PausedTakeMarker) { | ||
| const nav = | ||
| take.chapterAssignmentId !== undefined | ||
| ? await getVerseDetailNavByChapterAssignment(take.chapterAssignmentId) | ||
| : null; | ||
|
|
||
| // Without a resolvable verse we can't act on Continue, so don't prompt a | ||
| // dead end — leave the marker to retry on a later launch. | ||
| if (!nav || take.verseNumber === undefined) { | ||
| log.warn('Skipping recovery prompt; take could not be resolved', { | ||
| bibleTextId: take.bibleTextId, | ||
| }); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Unresolvable paused takes are never cleared, leaking storage forever.
When chapterAssignmentId no longer resolves (e.g. the chapter assignment was deleted) or verseNumber is missing, the function only logs a warning and returns — the marker and its segment files stay in storage indefinitely. Every future app launch will repeat the same silent failure with no way for the user to discard the orphaned take, since the recovery prompt (and thus the "Discard" action) never fires for it.
Consider clearing the marker (and deleting its segment files) in this branch when the take is unrecoverable, rather than leaving it to "retry on a later launch" forever.
♻️ Proposed fix
if (!nav || take.verseNumber === undefined) {
- log.warn('Skipping recovery prompt; take could not be resolved', {
- bibleTextId: take.bibleTextId,
- });
- return;
+ log.warn('Discarding unrecoverable paused take', {
+ bibleTextId: take.bibleTextId,
+ });
+ discardPausedTake(take);
+ return;
}📝 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.
Suggested change
| async function promptRecovery(take: PausedTakeMarker) { | |
| const nav = | |
| take.chapterAssignmentId !== undefined | |
| ? await getVerseDetailNavByChapterAssignment(take.chapterAssignmentId) | |
| : null; | |
| // Without a resolvable verse we can't act on Continue, so don't prompt a | |
| // dead end — leave the marker to retry on a later launch. | |
| if (!nav || take.verseNumber === undefined) { | |
| log.warn('Skipping recovery prompt; take could not be resolved', { | |
| bibleTextId: take.bibleTextId, | |
| }); | |
| return; | |
| } | |
| async function promptRecovery(take: PausedTakeMarker) { | |
| const nav = | |
| take.chapterAssignmentId !== undefined | |
| ? await getVerseDetailNavByChapterAssignment(take.chapterAssignmentId) | |
| : null; | |
| // Without a resolvable verse we can't act on Continue, so don't prompt a | |
| // dead end — leave the marker to retry on a later launch. | |
| if (!nav || take.verseNumber === undefined) { | |
| log.warn('Discarding unrecoverable paused take', { | |
| bibleTextId: take.bibleTextId, | |
| }); | |
| discardPausedTake(take); | |
| return; | |
| } |
🤖 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/app/screens/hooks/useRecordingRecovery.ts` around lines 49 - 62, The
promptRecovery flow in useRecordingRecovery currently bails out when
getVerseDetailNavByChapterAssignment returns null or take.verseNumber is
undefined, which leaves unrecoverable paused takes and their segment files
behind forever. Update this branch to explicitly clear the PausedTakeMarker and
remove the associated segment storage before returning, using the existing
recovery/cleanup helpers around promptRecovery so orphaned takes can be
discarded even when no verse resolution is possible.
Contributor
Author
There was a problem hiding this comment.
This will be addressed in future with a routine cache clear and an explicit clear cache option in settings.
There was a problem hiding this comment.
@fel-cesar, understood — deferring this to the planned cache-clear routine and settings-based manual clear makes sense as a broader solution. Would you like me to open a follow-up issue to track this so it isn't lost?
Wrap the merged-file creation and byte-append loop in concatenateAacSegments so a mid-loop bytes()/write() failure best-effort deletes the orphaned temporary merge file before rethrowing, instead of leaking it in the document directory. Refs #49
Update expo, expo-asset, and expo-build-properties to their latest 56.0.x patches (with transitive bumps to @expo/* tooling, expo-modules-core, and iconv-lite) to keep the SDK aligned. Refs #49
3 tasks
Contributor
Author
Scope reconciliation with the #116 recording-engine decompositionWhile reviewing follow-ups, I checked this PR against #95 ("Recording engine on expo-audio") and its sibling tickets #97 (state machine) and #98 (multi-take / DB wiring), which came out of the #116 epic decomposition. Finding: #95's acceptance criteria are all functionally met here.
Caveat — architecture differs from how those tickets were sliced. #95 asked for a thin engine ( Recommendation: merge as-is. The behavior and anti-pattern avoidance the epic wanted are delivered; re-slicing into a thin engine now would be churn with no user-facing benefit and no second caller to justify the split. Follow-up on #95 / #97 / #98: for each, the team should either
My vote is (1) for all three. I'll add a short cross-reference on each ticket pointing here. Note: this is the opposite of #96 (playback engine + real waveform / scrubbing), which this PR genuinely defers — that stays open. |
This was referenced Jul 7, 2026
mattrace-gloo
requested changes
Jul 9, 2026
Collaborator
There was a problem hiding this comment.
Good work on a hard ticket. ADTS kill recovery makes sense, Expo deps look right, layers are mostly clean, tests are solid, CI is green. A few #49 gaps before merge though.
Source audio player - not this PR
I checked the ticket chain. SourceAudioPlayerBar is still a stub from #47 (PR #131). Source audio files are #51/#50. #48 (PR #153) skips the player too. Epic #116 is draft recordings, not source listening.
#49 only needs to hook into the source player on verse nav (scrub to verse start). That is not wired here, but there is also nothing to wire until the player actually works.
Probably worth a follow-up ticket for #51 -> player wiring so #48/#49 integration has a clear owner. Not blocking this PR.
| Piece | Ticket | Status |
|---|---|---|
| Source player bar shell | #47 | Closed in PR #131 as stub (loadState never reaches ready) |
| Source audio files on device | #51 / #50 | Open |
| Bible tab + player integration | #48 | Open (PR #153 defers player) |
| Draft recording (this PR) | #49 | Here |
Blockers for #49
- Draft review scrubbing - #49 wants a scrubbable draft waveform and play from the scrub position. Review is play/pause from start with fake bars. A basic slider on
useDraftPlaybackwould probably be enough for v1. - Android device QA - needs a fresh dev client (
prebuild+expo-audio). Record, pause, background, kill recovery, permissions, re-record, delete. Note results on the PR. bibleTextIdnull - TEMP toast helps debug but recording cannot work without it. Fix the lookup or open a follow-up.
Non-blocking
RecorderAdapter<T>with one caller. Fine if another use case is coming soon, otherwise could fold intouseVerseRecorder- Re-record demotes
is_latestbut does not delete the old file
Nice work on these
- ADTS segments for kill recovery
- Tab keep-alive in
DraftingScreen - Permission, tab, and navigation guards
expo-cryptofor UUIDs, not a custom crypto dep
Can re-review once scrubbing and device QA are in, or ticket-waived with follow-ups linked.
Line-specific comments
src/app/tabs/drafting/record/components/RecordingControls.tsx ~L214
#49 Review AC wants scrubbing and play from scrub position. This is play/pause only. A slider wired to useDraftPlayback.seek() would close the gap. Or ticket it with a waiver.
src/hooks/useDraftPlayback.ts ~L45
toggle always plays from start. Need seek(ms) here for review scrubbing. Player already has seekTo.
src/app/tabs/drafting/record/components/RecordingWaveform.tsx ~L42
Live animation is fine. Review bars are decorative (sin/cos). #49 mockups show a scrubbable waveform. Placeholder is ok if there is a scrub control. Real amplitude can be a follow-up (#96).
src/app/tabs/drafting/record/RecordTab.tsx ~L132
Good that this is not a silent no-op. What is causing null bibleTextId? Sync timing, missing row, API gap? Worth fixing or tracking before merge since recording depends on it.
src/app/tabs/drafting/record/RecordTab.tsx ~L119
Paused-take guard on verse nav looks right. #49 also says scrub the source player on verse change (see #47). Not blocking since the player is still a stub. Add a TODO or follow-up for when #51 lands.
src/hooks/useRecorder.ts ~L59
Adapter pattern is clean. Only one caller though. Keep it if another use case is coming, otherwise could live in useVerseRecorder and cut a lot of generic surface area.
src/hooks/useRecorder.ts ~L166
ADTS over M4A for kill-resilience is the right call. Comments explaining why are helpful.
src/app/tabs/drafting/record/hooks/useVerseRecorder.ts ~L66
Good place for verse-specific DB, storage, and marker wiring.
src/services/recordingStorage.ts ~L203
Hand-rolled ADTS duration parser. Worth validating on real device files. expo-audio player duration is a simpler fallback if this gets flaky.
src/db/repository.ts ~L507
Transaction and is_latest demotion look right. Re-record does not delete the demoted file. Small orphan leak over time.
src/app/screens/DraftingScreen.tsx ~L194
Tab keep-alive via display: none + pointerEvents. Right approach. Avoids tearing down the recorder on tab switch.
AGENTS.md / .cursor/
Looks like #160 scope, not #49. Fine if it came from a main merge.
12 tasks
Support seeking during review: expose position/duration and a seek() on useDraftPlayback, and make the decorative review waveform tappable and draggable to seek (no new dependency; real amplitude stays deferred to #96). Group the recorder's playback surface into a nested `playback` sub-object so it reads as a concern the recorder only coordinates rather than one it owns. Refs #49
Rename useDraftPlayback to useAudioPlayback (and its API type) so the single-source player reads as reusable for any clip — draft review now, source/original verse audio later — rather than being draft-specific. No behavior change; the recorder still coordinates it via its gated playback sub-object. Refs #49
Drop the duplicate RecorderPlaybackApi interface — it was structurally identical to the hook's type with a single use site — and type recorder.playback as UseAudioPlaybackApi directly. Refs #49
Revert the review waveform to a non-interactive decorative bar: drop the touch-responder seek handlers, position/duration-driven progress fill, and the props that fed them. The underlying useAudioPlayback seek/position/ duration capability stays on the recorder's playback surface for later use. Refs #49
6 tasks
Contributor
Author
|
We've created follow-up issues for the items that were not addressed in this PR and will be handled separately:
|
DraftingProvider seeded selectedVerse from initialVerse only on mount, so a later prop change (e.g. recording recovery) would not update the selection if the provider stayed mounted. Sync selectedVerse when initialVerse changes so recovery no longer depends on the loading-gated remount. Add a provider test covering the re-point behavior.
…-playback # Conflicts: # src/app/context/DraftingContext.tsx # src/app/tabs/BibleTab.tsx
This was referenced Jul 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#49: Build Record tab with draft recording, playback, and kill-resilient recovery
Branch:
49-build-record-tab-with-draft-recording-and-playback· Suggested type: ✨ New feature🔥 TLDR
Replaces the legacy
ViewChapterscreen with a drafting screen shell and a fully wired Record tab where translators can record verse drafts, pause/resume takes, review playback, re-record, and delete — with committed recordings persisted locally in SQLite and on disk. Partial takes are now kill-resilient: recording is captured as segmented ADTS AAC, a recovery marker is persisted the moment recording starts, and an unfinished take is surfaced for Continue / Discard from Home after an app kill or crash. Server upload remains follow-up work.📋 Summary
ViewChapterscreen only simulated recording in component state; there was no audio capture, durable storage, DB-backed draft lifecycle, or crash recovery.DraftingScreenshell (routeVerseDetail) hostingRecordTabandBibleTab, a genericuseRecorderstate machine composed by a verse-scopeduseVerseRecorder, auseAudioPlaybackhook, durable file storage viaexpo-file-system, and arecordingstable with take/is_latestinvariants. Recording is captured as segmented ADTS AAC so partial takes survive a process kill, with a persisted recovery marker and a Home-screen recovery prompt.Related Issue: #49
Type of Change:
🔧 Technical Changes
Key files
Navigation & screens
src/navigation/AppNavigator.tsx— routeVerseDetailnow rendersDraftingScreen(replacesViewChapter).src/app/screens/DraftingScreen.tsx— parent shell: loads chapter/verse data, owns selected verse and tab state, and renders both tabs simultaneously (inactive tab hidden viadisplay: 'none'+pointerEvents) so the recorder is never torn down on a tab switch.src/app/screens/HomeScreen.tsx+src/app/screens/hooks/useRecordingRecovery.ts— surface a killed/unfinished take after sync and force a Continue / Discard decision, navigating to the recovered verse.src/app/tabs/RecordTab.tsx/src/app/tabs/BibleTab.tsx— tab mount points wired throughDraftingContext.src/app/tabs/drafting/record/RecordTab.tsx— orchestrates verse nav, recording controls, waveform, and source-text panel.src/app/tabs/drafting/record/components/—RecordingControls,RecordingWaveform,SourceTextPanel,VerseNav.src/app/tabs/drafting/record/hooks/useRecordTabGuards.ts— permission gating + navigation/tab-switch guard for paused takes.src/app/tabs/drafting/record/hooks/useVerseRecorder.ts— verse-specific adapter: DB attribution, paused-take markers, merge → derive duration → move → cleanup → insert, and playback URI resolution.src/app/tabs/ViewChapter.tsx— removed (replaced by the drafting module).Recording pipeline
src/hooks/useRecorder.ts— generic recorder state machine (idle → recording → paused → review); wall-clock elapsed timer; background auto-pause; ADTS AAC capture (extension: '.aac',android: { outputFormat: 'aac_adts', audioEncoder: 'aac' }); segment tracking + resume-after-kill;isRecoveredflag; marker persisted at recording start.src/hooks/useAudioPlayback.ts— generic play/pause/seek + position/duration hook (formerlyuseDraftPlayback); backs committed draft-take review playback (rewind on finish).src/services/recordingStorage.ts— durable directory layout, relative storage keys, ADTS segment concatenation (concatenateAacSegments), audio-derived duration (aacDurationMs), partial-take cleanup.src/services/storage.ts— per-verse paused-take recovery marker (PausedTakeMarkerwithsegments,elapsedMs,startedAt,sessionToken, nav context) plus get/set/clear/find helpers.Data layer
src/db/schema.ts/src/db/index.ts— recording attribution columns onrecordings.src/db/repository.ts/src/db/queries.ts— insert/update/delete recordings with take numbering and theis_latestinvariant; verse-nav-by-chapter-assignment lookup for the recovery prompt.src/types/db/types.ts,src/types/recording/types.ts,src/types/drafting/types.ts,src/types/navigation/types.ts— new/extended types.Config & deps
app.config.ts—expo-audioplugin with microphone permission string;expo-assetplugin.package.json—expo-audio(~56.0.12),expo-asset(~56.0.19),expo-crypto(~56.0.4),expo-file-system(~56.0.8).Dependencies & Configuration
expo-audio,expo-asset,expo-crypto,expo-file-systemexpo-audiomicrophone permission +expo-assetplugin inapp.config.ts; requiresnpm run prebuild(or a fresh EAS dev client) before device testingrecordingstable attribution fields; local audio files under the app document directory (no migration framework — schema applies on fresh DB init)🧠 Kill-resilient recording (how it works)
expo-audio's default.m4acontainer is only valid once itsmoovatom is written on a cleanstop(), so a process kill mid-recording leaves an unplayable file. To make partial takes recoverable:.aac) — a self-framing bitstream that stays playable up to the last complete frame and concatenates by plain byte append.Implementation lives in
src/hooks/useRecorder.ts(segment tracking, ADTS capture,isRecovered),src/services/recordingStorage.ts(concatenateAacSegments,aacDurationMs),src/services/storage.ts(PausedTakeMarker), andsrc/app/screens/hooks/useRecordingRecovery.ts(Home prompt).✅ Testing & Quality
Testing completed
expo-audio; see "How to test")Code quality (fluent-mobile gates)
npm run format:checknpm run lintnpm run typechecknpm test -- --ci🧪 How to test
Prerequisites: Node 24+,
npm install, copy.env.example→.envwith a reachableEXPO_PUBLIC_API_BASE_URL,npm run prebuild(new native module), EAS dev client ornpm run android.Steps:
MM:SS:CS); Pause and Stop available; verse chevrons disabled.Expected: Record → pause → resume → stop → play → re-record → delete works per verse. In-progress takes survive backgrounding and process kill. Committed drafts survive leaving/re-entering the chapter. No server upload (
sync_status='pending'stays local). Waveforms are decorative (not real amplitude); review playback is play/pause from the start (no scrubber — see follow-ups).Seekable playback / Review scrubbing (deferred from #49 → #176)
.aac, kept deliberately for kill-safe resumable capture, which is not reliably seekable. An interactive scrub waveform was prototyped in this branch and then intentionally removed (RecordingWaveformno longer exposesonSeek) because accurate scrubbing needs a seekable container.useAudioPlaybackseek path. Build Record Tab with Draft Recording and Playback #49's scrubbing AC is waived accordingly.Real amplitude waveform + playback engine (deferred → #96)
Null
bibleTextIdin the Record tab (deferred → #177)bible_text_id; for some versesgetBibleTextId(...)currently resolvesnull.src/app/tabs/drafting/record/RecordTab.tsxships a TEMPToastAndroid("No bible text id for this verse yet.") instead of a dead button. Resolve nullbibleTextIdfor verses in the Record tab (sync/lookup gap) #177 owns the sync/lookup root-cause fix and the real disabled/"still syncing" UX.Recovery caveats
.aac.Drafting shell
BibleTaband the chapter audio bar are minimal for Build Record Tab with Draft Recording and Playback #49's E2E; richer behavior lands with Build Drafting Page Shell with Tab Navigation #47 / Build Bible Tab with Verse List and Sequential Playback #48.Upload
sync_status='pending'); no upload worker ([Epic] Recording sync & cloud storage (R2) #117 epic).📌 Reviewer follow-up notes
Follow-up issues were filed for the waived scope (see the follow-up comment):
DraftingScreenkeeps both tabs mounted and hides the inactive one, souseRecorderis no longer torn down on tab switch.