Hold camera and mic locks while recording is active #1162
Conversation
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
apps/desktop/src-tauri/src/recording.rs(8 hunks)crates/recording/src/instant_recording.rs(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
crates/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
For desktop IPC, use tauri_specta derive/macros on Rust types/events; do not hand-roll bindings
Files:
crates/recording/src/instant_recording.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
crates/recording/src/instant_recording.rsapps/desktop/src-tauri/src/recording.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
crates/recording/src/instant_recording.rsapps/desktop/src-tauri/src/recording.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/recording/src/instant_recording.rs
🧬 Code graph analysis (1)
apps/desktop/src-tauri/src/recording.rs (2)
apps/desktop/src/utils/tauri.ts (2)
StartRecordingInputs(462-462)VideoUploadInfo(481-481)crates/recording/src/feeds/microphone.rs (8)
handle(245-365)handle(371-383)handle(389-391)handle(397-417)handle(433-467)handle(473-485)handle(491-507)handle(513-526)
⏰ 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). (4)
- GitHub Check: Clippy
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
apps/desktop/src-tauri/src/recording.rs (4)
11-12: LGTM! Imports support the new lock-holding mechanism.These imports are now required for the
InProgressRecordingCommonstruct fields (camera_feedandmic_feed), which hold the locks for the duration of the recording.
58-64: LGTM!InProgressRecordingCommoncorrectly holds locks throughout recording lifecycle.This struct consolidates shared recording state and—critically—holds
Arc<CameraFeedLock>andArc<MicrophoneFeedLock>for the recording's lifetime. By storing these locks in theInProgressRecordingenum (which lives in App state), they won't be dropped until the recording is stopped or cancelled, fulfilling the PR objective.
66-77: LGTM! Enum refactor properly integratesInProgressRecordingCommon.Both
InstantandStudiovariants now include thecommonfield, ensuring consistent lock-holding behavior across recording modes. The locks will be automatically dropped whenstop()orcancel()consumeself(lines 115-148).
454-460: LGTM! Lock construction ensures they're held until recording completion.By constructing
InProgressRecordingCommonwith the acquired locks and embedding it in theInProgressRecordingenum (lines 494, 534), the locks remain valid until the recording is cleared from App state. This prevents premature unlock signals to the feed actors.
| RecordingBaseInputs, | ||
| capture_pipeline::{MakeCapturePipeline, ScreenCaptureMethod, Stop, create_screen_capture}, | ||
| feeds::microphone::MicrophoneFeedLock, | ||
| feeds::{camera::CameraFeedLock, microphone::MicrophoneFeedLock}, |
There was a problem hiding this comment.
Unused import: CameraFeedLock is not referenced in this file.
CameraFeedLock is imported but never used. Instant recording doesn't currently support camera feeds (camera_feed is hardcoded to None at line 255 in RecordingBaseInputs), so this import is unnecessary.
Apply this diff to remove the unused import:
- feeds::{camera::CameraFeedLock, microphone::MicrophoneFeedLock},
+ feeds::microphone::MicrophoneFeedLock,📝 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.
| feeds::{camera::CameraFeedLock, microphone::MicrophoneFeedLock}, | |
| feeds::microphone::MicrophoneFeedLock, |
🤖 Prompt for AI Agents
In crates/recording/src/instant_recording.rs around line 4, the import
feeds::{camera::CameraFeedLock, microphone::MicrophoneFeedLock} includes
CameraFeedLock which is unused because instant recording hardcodes camera_feed
to None; remove CameraFeedLock from the use/import list so only
MicrophoneFeedLock is imported to eliminate the unused-import warning.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src-tauri/src/recording.rs(8 hunks)crates/recording/src/sources/camera.rs(3 hunks)crates/recording/src/sources/microphone.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
crates/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
For desktop IPC, use tauri_specta derive/macros on Rust types/events; do not hand-roll bindings
Files:
crates/recording/src/sources/camera.rscrates/recording/src/sources/microphone.rs
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
crates/recording/src/sources/camera.rscrates/recording/src/sources/microphone.rsapps/desktop/src-tauri/src/recording.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
crates/recording/src/sources/camera.rscrates/recording/src/sources/microphone.rsapps/desktop/src-tauri/src/recording.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/recording/src/sources/camera.rscrates/recording/src/sources/microphone.rs
🧬 Code graph analysis (2)
crates/recording/src/sources/microphone.rs (2)
crates/recording/src/sources/camera.rs (1)
setup(17-39)crates/recording/src/feeds/microphone.rs (1)
audio_info(173-175)
apps/desktop/src-tauri/src/recording.rs (2)
apps/desktop/src/utils/tauri.ts (1)
StartRecordingInputs(462-462)crates/recording/src/feeds/microphone.rs (8)
handle(245-365)handle(371-383)handle(389-391)handle(397-417)handle(433-467)handle(473-485)handle(491-507)handle(513-526)
⏰ 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: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (5)
crates/recording/src/sources/camera.rs (1)
17-39: LGTM! Clean parameter rename for consistency.The parameter rename from
configtofeed_lockimproves clarity and aligns with the parallel changes inmicrophone.rs. The implementation correctly passes the feed lock through toAddSenderand stores it in the struct to ensure it's held for the source's lifetime.crates/recording/src/sources/microphone.rs (1)
10-44: LGTM! Lock now held for recording lifetime.The struct now stores both
AudioInfoandArc<MicrophoneFeedLock>(previously justAudioInfo). This ensures the microphone lock is held for the entire lifetime of theMicrophonesource, preventing premature unlock. When the struct is dropped, the lock will be automatically released via the oneshot channel mechanism shown in the relevant code snippet.apps/desktop/src-tauri/src/recording.rs (3)
58-62: LGTM! Good refactoring to eliminate duplication.The new
InProgressRecordingCommonstruct consolidates shared fields (target_name,inputs,recording_dir) across bothInstantandStudiovariants, reducing duplication and making the code more maintainable.
64-154: LGTM! Consistent refactoring across all accessors and completion paths.All methods correctly access the shared fields via
common, and the stop/completion logic properly extractstarget_namefromcommon.target_name. The refactoring maintains the same behavior while improving code organization.
452-533: LGTM! Lock management correctly integrated.The recording start flow now properly acquires camera and microphone locks (lines 435-445), passes them to the respective builders, and stores them in the sources. This ensures locks are held for the entire recording lifetime and released when the recording completes/fails.
| use cap_recording::feeds::camera::CameraFeedLock; | ||
| use cap_recording::feeds::microphone::MicrophoneFeedLock; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
Verify that the imports are actually needed.
The imports for CameraFeedLock and MicrophoneFeedLock are not explicitly referenced by name in the code shown (types are inferred at lines 436 and 442). These might be unused imports unless they're needed elsewhere in the file.
Run the following script to check if these types are explicitly used:
🏁 Script executed:
#!/bin/bash
# Check if CameraFeedLock and MicrophoneFeedLock are explicitly referenced in recording.rs
echo "Checking for explicit references to CameraFeedLock..."
rg -n 'CameraFeedLock' apps/desktop/src-tauri/src/recording.rs | grep -v '^11:' || echo "No references found outside import"
echo ""
echo "Checking for explicit references to MicrophoneFeedLock..."
rg -n 'MicrophoneFeedLock' apps/desktop/src-tauri/src/recording.rs | grep -v '^12:' || echo "No references found outside import"Length of output: 182
Remove unused imports of CameraFeedLock and MicrophoneFeedLock
Neither type is referenced anywhere in apps/desktop/src-tauri/src/recording.rs; these imports can be deleted.
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/recording.rs around lines 11 to 12, the file
imports CameraFeedLock and MicrophoneFeedLock from cap_recording but never uses
them; remove the two unused use lines (use
cap_recording::feeds::camera::CameraFeedLock; and use
cap_recording::feeds::microphone::MicrophoneFeedLock;) to clean up unused
imports and avoid compiler warnings.
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
apps/desktop/src-tauri/src/recording.rs(8 hunks)crates/recording/src/sources/camera.rs(3 hunks)crates/recording/src/sources/microphone.rs(2 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
**/*.{ts,tsx,js,jsx,rs}
📄 CodeRabbit inference engine (CLAUDE.md)
Do not add inline, block, or docstring comments in any language; code must be self-explanatory
Files:
apps/desktop/src-tauri/src/recording.rscrates/recording/src/sources/camera.rscrates/recording/src/sources/microphone.rs
**/*.rs
📄 CodeRabbit inference engine (AGENTS.md)
**/*.rs: Format Rust code usingrustfmtand ensure all Rust code passes workspace-level clippy lints.
Rust modules should be named with snake_case, and crate directories should be in kebab-case.
Files:
apps/desktop/src-tauri/src/recording.rscrates/recording/src/sources/camera.rscrates/recording/src/sources/microphone.rs
crates/**/src/**/*.rs
📄 CodeRabbit inference engine (CLAUDE.md)
For desktop IPC, use tauri_specta derive/macros on Rust types/events; do not hand-roll bindings
Files:
crates/recording/src/sources/camera.rscrates/recording/src/sources/microphone.rs
crates/*/src/**/*
📄 CodeRabbit inference engine (AGENTS.md)
Rust crates should place tests within the
src/and/or a siblingtests/directory for each crate insidecrates/*.
Files:
crates/recording/src/sources/camera.rscrates/recording/src/sources/microphone.rs
🧬 Code graph analysis (2)
apps/desktop/src-tauri/src/recording.rs (2)
apps/desktop/src/utils/tauri.ts (1)
StartRecordingInputs(462-462)crates/recording/src/feeds/microphone.rs (8)
handle(245-365)handle(371-383)handle(389-391)handle(397-417)handle(433-467)handle(473-485)handle(491-507)handle(513-526)
crates/recording/src/sources/microphone.rs (2)
crates/recording/src/sources/camera.rs (1)
setup(17-39)crates/recording/src/feeds/microphone.rs (1)
audio_info(173-175)
⏰ 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: Build Desktop (x86_64-pc-windows-msvc, windows-latest)
- GitHub Check: Build Desktop (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (9)
crates/recording/src/sources/camera.rs (1)
17-38: LGTM!The parameter rename from
configtofeed_lockimproves clarity and establishes consistency with the microphone source pattern.crates/recording/src/sources/microphone.rs (3)
10-10: LGTM!Adding
Arc<MicrophoneFeedLock>as a second field correctly implements the PR objective of holding the microphone lock while recording is active.
16-27: LGTM!The parameter rename and updated usage are correct and consistent with the camera source.
43-43: LGTM!The constructor correctly returns both fields, keeping the feed lock alive for the duration of the
Microphoneinstance.apps/desktop/src-tauri/src/recording.rs (5)
58-62: LGTM!The
InProgressRecordingCommonstruct effectively eliminates field duplication between theInstantandStudiovariants.
75-75: LGTM!The Studio variant correctly adopts the common payload pattern.
87-92: LGTM!The accessor methods correctly delegate to the
commonfield, maintaining consistent behavior across both variants.Also applies to: 108-113
115-134: LGTM!The
stop()method correctly accessescommon.target_namefor both variants. The destructuring preserves all fields appropriately.
454-458: LGTM!The construction and propagation of
InProgressRecordingCommonis correct. The Instant variant properly holdscamera_feedto maintain the lock for the recording duration, implementing the PR objective.Also applies to: 492-492, 532-533
| common: InProgressRecordingCommon, | ||
| // camera isn't used as part of recording pipeline so we hold lock here | ||
| camera_feed: Option<Arc<CameraFeedLock>>, |
There was a problem hiding this comment.
Remove the comment to comply with coding guidelines.
The inline comment violates the guideline: "Do not add inline, block, or docstring comments in any language; code must be self-explanatory."
Apply this diff:
common: InProgressRecordingCommon,
- // camera isn't used as part of recording pipeline so we hold lock here
camera_feed: Option<Arc<CameraFeedLock>>,As per coding guidelines
📝 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.
| common: InProgressRecordingCommon, | |
| // camera isn't used as part of recording pipeline so we hold lock here | |
| camera_feed: Option<Arc<CameraFeedLock>>, | |
| common: InProgressRecordingCommon, | |
| camera_feed: Option<Arc<CameraFeedLock>>, |
🤖 Prompt for AI Agents
In apps/desktop/src-tauri/src/recording.rs around lines 69 to 71, remove the
inline comment "// camera isn't used as part of recording pipeline so we hold
lock here" so the code complies with the guideline forbidding
inline/block/docstring comments; leave the field definition and surrounding
formatting unchanged.
| feed_lock | ||
| .ask(microphone::AddSender(tx)) | ||
| .await | ||
| .map_err(|e| anyhow!("Failed to add camera sender: {e}"))?; |
There was a problem hiding this comment.
Fix error message to reference microphone instead of camera.
The error message mentions "camera sender" but this code is for the microphone source.
Apply this diff to fix the message:
- .map_err(|e| anyhow!("Failed to add camera sender: {e}"))?;
+ .map_err(|e| anyhow!("Failed to add microphone sender: {e}"))?;📝 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.
| .map_err(|e| anyhow!("Failed to add camera sender: {e}"))?; | |
| .map_err(|e| anyhow!("Failed to add microphone sender: {e}"))?; |
🤖 Prompt for AI Agents
In crates/recording/src/sources/microphone.rs around line 30, the error message
incorrectly says "Failed to add camera sender"; update the string to reference
the microphone (e.g., "Failed to add microphone sender: {e}") so the error
context matches this source, keeping the same map_err usage and error
interpolation.
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Chores