feat: Introduce new and improved recording picker #1388
Conversation
|
Warning Rate limit exceeded@richiemcilroy has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 2 minutes and 13 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (9)
WalkthroughAdds device disconnect/reconnect handling/events for microphone and camera, macOS shareable-content refresh/retry, window-exclusion filtering and selection hints, device-selection controls in overlays, recording-window preference tracking, multiple output-pipeline timing adjustments, feed reconnection messages, and a minor version bump. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App
participant DeviceWatcher
participant RecordingActor
participant Overlay
User->>App: Start recording
App->>RecordingActor: spawn/init recording
activate RecordingActor
Note over App,DeviceWatcher: device watchers running
DeviceWatcher->>App: periodic availability check
alt device disconnected
DeviceWatcher->>App: handle_input_disconnect(kind)
App->>RecordingActor: pause/detach feed
App->>Overlay: emit RecordingEvent::InputLost(kind)
App->>User: show issue overlay/notification
else device restored
DeviceWatcher->>App: detected available
App->>App: ensure_selected_mic_ready / ensure_selected_camera_ready
App->>RecordingActor: reattach/restore feed
App->>Overlay: emit RecordingEvent::InputRestored(kind)
end
deactivate RecordingActor
sequenceDiagram
participant Settings
participant App
participant User
Settings->>App: load general settings
alt first run (recording_picker_preference_set == false)
App->>App: enable_new_recording_flow = true
App->>App: recording_picker_preference_set = true
App->>Settings: persist change
else subsequent runs
App->>Settings: keep stored preferences
end
User->>Settings: choose Recording window variant
Settings->>Settings: updateRecordingWindowVariant(variant) sets enableNewRecordingFlow and recordingPickerPreferenceSet
Settings->>App: persist settings
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
apps/desktop/src/routes/in-progress-recording.tsx (1)
52-103: Handle theStoppedrecording event to auto-close the windowThe window-close effect (lines 203–211) correctly watches for
state().variant === "stopped", but therecordingEventlistener (lines 139–167) doesn't have a case for theStoppedevent. When the backend stops recording normally, it emitsRecordingEvent::Stopped(confirmed inrecording.rs:749), but this event is unhandled, sostate().variantnever transitions to"stopped"and the window won't close automatically.Add a case in the
recordingEventswitch:case "Stopped": setState({ variant: "stopped" }); break;
🧹 Nitpick comments (10)
crates/recording/src/output_pipeline/win.rs (1)
353-365: LGTM: Duration to TimeSpan conversion.The conversion correctly handles Windows' 100-nanosecond tick representation. Saturating arithmetic and clamping to
i64::MAXappropriately handle overflow for extremely large durations.Consider adding a doc comment explaining the tick rate and overflow behavior for future maintainers:
+/// Converts a [`Duration`] to a Windows [`TimeSpan`]. +/// +/// Windows TimeSpan uses 100-nanosecond ticks (10,000,000 ticks per second). +/// Overflows are saturated to `i64::MAX`. fn duration_to_timespan(duration: Duration) -> TimeSpan {apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (1)
304-322: Picker-driven main-window hide/show flow is well-containedTying the main window’s visibility to
rawOptions.targetModewithhasHiddenMainWindowForPickeris a clear way to ensure you only hide/show once per picker session and always restore the window on cleanup. The onCleanup guard is a good safeguard for unexpected unmounts. If you notice any flicker, a minor follow-up would be to cacheconst currentWindow = getCurrentWindow()instead of re-calling it, but it’s not essential.apps/desktop/src-tauri/src/target_select_overlay.rs (2)
66-71: Graceful fallback to default exclusions, but settings errors are silently droppedUsing
GeneralSettingsStore::get(&app).ok().flatten().map_or_else(...)correctly falls back todefault_excluded_windowswhen settings are missing or invalid, keeping the overlay functional. The downside is deserialization/read errors are silently ignored; consider logging at debug/warn so unexpected corruption ofgeneral_settingsis diagnosable.
92-95: Window exclusion filter looks correct; consider avoiding duplicate owner name lookupsThe
should_skip_windowlogic cleanly matches against bundle identifier, owner name, and title, and the call site short‑circuits correctly to suppress excluded windows. Note thatowner_name()is called both insideshould_skip_windowand again when constructingWindowUnderCursor.app_name, which may duplicate relatively expensive OS calls every 50 ms; if this ever shows up in profiling, you could thread the already-fetchedowner_namethrough instead of recomputing it.Also applies to: 129-150
crates/recording/src/feeds/camera.rs (1)
176-181: Locked-camera reconnection flow mirrors microphone behavior and looks correctThe
LockedCameraInputReconnectedmessage plus theState::Lockedbranch inSetInputcorrectly: (1) enforce that only the currently locked camera ID can reconnect, (2) shut down the previous capture viainner.done_tx.send(())before starting a new one, (3) reuse the samesetup_camerapipeline and ready future wiring as the Open path, and (4) update the lockedAttachedStatewith freshcamera_info,video_info, anddone_tx. The duplication between the Open and Locked branches is acceptable, but if this grows further you might consider extracting a shared helper to reduce repetition.Also applies to: 403-468
crates/recording/src/feeds/microphone.rs (1)
333-654: Consider extracting common stream building logic.The Open and Locked state branches contain substantial code duplication (~140 lines). The stream building logic (lines 386-474 and 540-627) is nearly identical.
While functional, extracting the common stream setup into a helper function would improve maintainability and reduce the risk of inconsistencies when making future changes.
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (1)
66-78: Defaults andhandleChangeextra payload look consistent, but local store isn’t fully updated
- Changing
enableNewRecordingFlowdefault totrueand addingrecordingPickerPreferenceSet: falsematches the new “new picker by default” behavior and the backendGeneralSettingsStoreshape.handleChange’sextra?: Partial<GeneralSettingsStore>is a nice way to batch related updates into persistence, but you only applyextratogeneralSettingsStore.set, not to the localsettingsstore. If you later read any of those extra keys (likerecordingPickerPreferenceSet) fromsettings, they’ll be stale until the window reloads. Consider also mergingextraintosettingsfor consistency.Also applies to: 203-212
apps/desktop/src/routes/in-progress-recording.tsx (1)
182-200: Device-change handling during recording is careful and rollback-safe
refreshCameraWindowStateplus the periodic timer keepcameraWindowOpenin sync without spamming the backend.pauseRecordingForDeviceChangeand its use fromupdateMicInput/updateCameraInputensure:
- Device changes only pause if currently recording,
- Pause segments are tracked in
pauseResumes,- The in‑progress UI state remains consistent (variant switched to
"paused"and time updated once).updateMicInput/updateCameraInput:
- Respect
startedWithMicrophone/startedWithCameraInputso you can’t enable a new input mid‑recording that wasn’t present at start,- No‑op when selecting the same input,
- Optimistically update
optionsQuery.rawOptions, but roll back to the previous value if the underlyingset*Inputcommand fails.openRecordingSettingsMenu’s dynamic menu wiring (including “No Microphone”/“No Webcam” items) is consistent with these rules and won’t offer actions that are locked for this recording.Also applies to: 269-329, 330-425
apps/desktop/src-tauri/src/lib.rs (1)
106-122: Input disconnect/restoration pipeline and device watchers are well-integrated
Appnow tracksselected_camera_id,camera_in_use, and aHashSet<RecordingInputKind>ofdisconnected_inputs, which is a clean internal model for device state.handle_input_disconnect:
- Idempotently marks an input as disconnected,
- Pauses any active recording via
current_recording_mut().pause().await,- Emits a
NewNotificationwith a clear pause message per input kind,- Emits
RecordingEvent::InputLost { input }for the frontend.handle_input_restored:
- Clears the disconnected flag once,
- Calls
ensure_selected_mic_ready/ensure_selected_camera_readyto re‑apply the selected input,- Emits
RecordingEvent::InputRestored { input }.spawn_mic_error_handlerand the microphone/camera watchers poll at 1s intervals only while a recording is active (and, for camera, only whencamera_in_useis true), then call the above handlers based on actual device availability. This ties feed‑level errors and physical device add/remove into the same state machine.- Minor concern: the watchers and mic error handler currently hold a write lock on
ArcLock<App>while awaitinghandle_input_disconnect/handle_input_restored, which themselvesawait(pause recording, set inputs). It works but serializes any other operations needing the app state. You may want to clone just the data needed for the decision, drop the read or write guard, and then call the async handler to reduce lock contention.Also applies to: 250-327, 492-567, 569-623
apps/desktop/src/routes/target-select-overlay.tsx (1)
152-159: Area-mode state flow works but doesn’t reuse an existing area selection in the cropper
- For area target mode you now track:
pendingAreaTarget(the logical"area"ScreenCaptureTarget),initialAreaBounds(seeded when converting a window to an area via “Adjust recording area”),crop/committedCropandisValid/committedIsValidto enforce a 150×150 minimum size.- When entering area mode:
- If
options.captureTargetis already"area"for this display,pendingAreaTargetis seeded from it.- When leaving area mode, any
pendingAreaTargetis committed back intocaptureTarget.- While idle (not interacting) and with a valid
committedCrop,pendingAreaTargetis kept in sync with the current crop bounds.- The cropper visual, though, is only initialized from
initialAreaBounds(initialCrop={() => initialAreaBounds() ?? CROP_ZERO}). If you arrive in area mode with an existing area target but without having gone through “Adjust recording area” (soinitialAreaBoundsis stillundefined), the crop starts empty, and the start button stays disabled until the user draws a new area—even though we do have a usablependingAreaTarget.- Consider also seeding
initialAreaBoundsfrom the existing"area"capture target when you populatependingAreaTarget, so the cropper reflects the saved selection and the user can refine instead of redrawing.Also applies to: 160-183, 184-211, 335-368, 384-405, 504-519, 567-576
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lockpackages/ui-solid/icons/cursor-macos.svgis excluded by!**/*.svgpackages/ui-solid/icons/cursor-windows.svgis excluded by!**/*.svgpackages/ui-solid/icons/monitor.svgis excluded by!**/*.svg
📒 Files selected for processing (24)
apps/desktop/src-tauri/Cargo.toml(1 hunks)apps/desktop/src-tauri/src/general_settings.rs(5 hunks)apps/desktop/src-tauri/src/lib.rs(9 hunks)apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs(2 hunks)apps/desktop/src-tauri/src/recording.rs(7 hunks)apps/desktop/src-tauri/src/target_select_overlay.rs(4 hunks)apps/desktop/src-tauri/src/windows.rs(1 hunks)apps/desktop/src/components/SelectionHint.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/new-main/index.tsx(17 hunks)apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx(1 hunks)apps/desktop/src/routes/(window-chrome)/settings/general.tsx(3 hunks)apps/desktop/src/routes/camera.tsx(7 hunks)apps/desktop/src/routes/capture-area.tsx(6 hunks)apps/desktop/src/routes/in-progress-recording.tsx(9 hunks)apps/desktop/src/routes/target-select-overlay.tsx(14 hunks)apps/desktop/src/styles/theme.css(1 hunks)apps/desktop/src/utils/tauri.ts(2 hunks)crates/recording/src/feeds/camera.rs(3 hunks)crates/recording/src/feeds/microphone.rs(5 hunks)crates/recording/src/output_pipeline/macos.rs(1 hunks)crates/recording/src/output_pipeline/win.rs(6 hunks)crates/recording/src/sources/screen_capture/macos.rs(1 hunks)crates/recording/src/studio_recording.rs(4 hunks)packages/ui-solid/src/auto-imports.d.ts(3 hunks)
🧰 Additional context used
📓 Path-based instructions (5)
**/*.{ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx}: Use a 2-space indent for TypeScript code.
Use Biome for formatting and linting TypeScript/JavaScript files by runningpnpm format.
Files:
apps/desktop/src/routes/(window-chrome)/settings/experimental.tsxapps/desktop/src/routes/in-progress-recording.tsxapps/desktop/src/routes/capture-area.tsxpackages/ui-solid/src/auto-imports.d.tsapps/desktop/src/routes/camera.tsxapps/desktop/src/components/SelectionHint.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src/routes/(window-chrome)/settings/general.tsxapps/desktop/src/routes/(window-chrome)/new-main/index.tsx
**/*.{ts,tsx,js,jsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{ts,tsx,js,jsx}: Use kebab-case for filenames for TypeScript/JavaScript modules (e.g.,user-menu.tsx).
Use PascalCase for React/Solid components.
Files:
apps/desktop/src/routes/(window-chrome)/settings/experimental.tsxapps/desktop/src/routes/in-progress-recording.tsxapps/desktop/src/routes/capture-area.tsxpackages/ui-solid/src/auto-imports.d.tsapps/desktop/src/routes/camera.tsxapps/desktop/src/components/SelectionHint.tsxapps/desktop/src/utils/tauri.tsapps/desktop/src/routes/target-select-overlay.tsxapps/desktop/src/routes/(window-chrome)/settings/general.tsxapps/desktop/src/routes/(window-chrome)/new-main/index.tsx
**/*.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/output_pipeline/macos.rsapps/desktop/src-tauri/src/windows.rscrates/recording/src/sources/screen_capture/macos.rsapps/desktop/src-tauri/src/target_select_overlay.rscrates/recording/src/feeds/microphone.rsapps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rsapps/desktop/src-tauri/src/general_settings.rscrates/recording/src/feeds/camera.rsapps/desktop/src-tauri/src/recording.rsapps/desktop/src-tauri/src/lib.rscrates/recording/src/output_pipeline/win.rscrates/recording/src/studio_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/output_pipeline/macos.rscrates/recording/src/sources/screen_capture/macos.rscrates/recording/src/feeds/microphone.rscrates/recording/src/feeds/camera.rscrates/recording/src/output_pipeline/win.rscrates/recording/src/studio_recording.rs
**/tauri.ts
📄 CodeRabbit inference engine (AGENTS.md)
Do not edit auto-generated files named
tauri.ts.
Files:
apps/desktop/src/utils/tauri.ts
🧠 Learnings (4)
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/recording/src/output_pipeline/macos.rscrates/recording/src/feeds/camera.rsapps/desktop/src-tauri/src/recording.rscrates/recording/src/output_pipeline/win.rs
📚 Learning: 2025-10-17T05:58:22.586Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1219
File: crates/enc-avfoundation/src/mp4.rs:350-373
Timestamp: 2025-10-17T05:58:22.586Z
Learning: In crates/enc-avfoundation/src/mp4.rs, the `finish()` method intentionally skips video extension when `is_paused` is true. This is correct behavior because if recording is paused, the video should not be extended beyond the pause point—the pause is user-initiated, unlike the case where ScreenCaptureKit stops providing frames during static content.
Applied to files:
crates/recording/src/output_pipeline/macos.rscrates/recording/src/output_pipeline/win.rs
📚 Learning: 2025-09-22T14:19:56.010Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T14:19:56.010Z
Learning: Applies to apps/web/**/*.{ts,tsx,js,jsx} : On the client, always use `useEffectQuery` or `useEffectMutation` from `@/lib/EffectRuntime`; never call `EffectRuntime.run*` directly in components.
Applied to files:
apps/desktop/src/routes/camera.tsx
📚 Learning: 2025-09-22T14:19:56.010Z
Learnt from: CR
Repo: CapSoftware/Cap PR: 0
File: AGENTS.md:0-0
Timestamp: 2025-09-22T14:19:56.010Z
Learning: Applies to apps/desktop/**/src-tauri/gen/** : Do not edit auto-generated files in `apps/desktop/src-tauri/gen/`.
Applied to files:
apps/desktop/src-tauri/Cargo.toml
🧬 Code graph analysis (15)
crates/recording/src/output_pipeline/macos.rs (3)
crates/recording/src/output_pipeline/core.rs (2)
timestamp(838-838)send_audio_frame(861-861)crates/recording/src/output_pipeline/ffmpeg.rs (3)
timestamp(22-24)send_audio_frame(110-116)send_audio_frame(153-155)crates/recording/src/output_pipeline/win.rs (1)
send_audio_frame(340-350)
apps/desktop/src-tauri/src/target_select_overlay.rs (5)
apps/desktop/src/utils/tauri.ts (1)
WindowExclusion(497-497)crates/scap-targets/src/platform/win.rs (3)
windows(260-260)windows(1150-1150)owner_name(443-483)apps/desktop/src-tauri/src/general_settings.rs (2)
get(208-219)default_excluded_windows(50-59)crates/scap-targets/src/platform/macos.rs (2)
owner_name(320-332)bundle_identifier(350-378)crates/scap-targets/src/lib.rs (1)
owner_name(134-136)
apps/desktop/src/routes/in-progress-recording.tsx (2)
apps/desktop/src/utils/tauri.ts (6)
RecordingInputKind(452-452)events(297-341)commands(7-292)CameraInfo(365-365)LogicalPosition(425-425)DeviceOrModelID(394-394)apps/desktop/src/utils/createEventListener.ts (1)
createTauriEventListener(30-44)
apps/desktop/src/routes/capture-area.tsx (2)
apps/desktop/src/components/SelectionHint.tsx (1)
SelectionHint(10-47)apps/desktop/src/components/Cropper.tsx (1)
CROP_ZERO(32-32)
crates/recording/src/feeds/microphone.rs (2)
crates/timestamp/src/lib.rs (1)
from_cpal(35-44)crates/recording/src/feeds/camera.rs (11)
handle(327-470)handle(476-492)handle(498-500)handle(506-523)handle(529-535)handle(541-557)handle(573-609)handle(615-639)handle(645-661)handle(667-679)handle(685-698)
apps/desktop/src/routes/camera.tsx (2)
apps/desktop/src/utils/createEventListener.ts (1)
createTauriEventListener(30-44)apps/desktop/src/utils/tauri.ts (1)
events(297-341)
apps/desktop/src-tauri/src/general_settings.rs (3)
apps/desktop/src/utils/tauri.ts (1)
GeneralSettingsStore(405-405)apps/desktop/src-tauri/src/auth.rs (1)
get(51-57)apps/desktop/src-tauri/src/presets.rs (1)
get(24-38)
crates/recording/src/feeds/camera.rs (1)
crates/recording/src/feeds/microphone.rs (10)
new(88-98)handle(330-654)handle(660-672)handle(678-680)handle(686-706)handle(722-758)handle(764-776)handle(782-798)handle(804-817)handle(823-836)
apps/desktop/src-tauri/src/recording.rs (5)
apps/desktop/src/utils/tauri.ts (3)
ScreenCaptureTarget(469-469)RecordingInputKind(452-452)RecordingEvent(451-451)apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (3)
get_shareable_content(67-83)refresh_shareable_content(30-32)state(22-24)crates/recording/src/sources/screen_capture/mod.rs (1)
display(66-72)crates/scap-targets/src/platform/macos.rs (1)
display(519-539)crates/scap-targets/src/platform/win.rs (1)
display(1044-1051)
apps/desktop/src-tauri/src/lib.rs (5)
apps/desktop/src/utils/tauri.ts (5)
RecordingEvent(451-451)RecordingInputKind(452-452)DeviceOrModelID(394-394)Camera(364-364)NewNotification(432-432)crates/recording/src/feeds/microphone.rs (7)
handle(330-654)handle(660-672)handle(678-680)handle(686-706)handle(722-758)drop(262-266)list(105-127)crates/recording/src/studio_recording.rs (6)
handle(110-142)handle(150-173)handle(181-205)handle(213-221)handle(231-242)handle(252-267)crates/camera/src/lib.rs (4)
drop(238-242)list_cameras(42-44)device_id(29-31)model_id(33-35)apps/desktop/src-tauri/src/recording.rs (1)
list_cameras(291-293)
crates/recording/src/output_pipeline/win.rs (3)
crates/recording/src/output_pipeline/core.rs (7)
new(307-325)new(675-677)new(718-720)new(751-753)new(795-812)timestamp(838-838)send_audio_frame(861-861)crates/recording/src/output_pipeline/ffmpeg.rs (3)
timestamp(22-24)send_audio_frame(110-116)send_audio_frame(153-155)crates/recording/src/sources/screen_capture/windows.rs (1)
timestamp(63-65)
apps/desktop/src/routes/target-select-overlay.tsx (5)
apps/desktop/src/components/Cropper.tsx (3)
CropBounds(26-31)CROP_ZERO(32-32)Cropper(233-1266)apps/desktop/src/components/SelectionHint.tsx (1)
SelectionHint(10-47)apps/desktop/src/utils/queries.ts (3)
listVideoDevices(81-86)listAudioDevices(100-107)createCameraMutation(195-228)apps/desktop/src/routes/(window-chrome)/new-main/CameraSelect.tsx (1)
CameraSelect(13-27)apps/desktop/src/routes/(window-chrome)/new-main/MicrophoneSelect.tsx (1)
MicrophoneSelect(21-36)
apps/desktop/src/routes/(window-chrome)/settings/general.tsx (3)
apps/desktop/src/utils/tauri.ts (1)
GeneralSettingsStore(405-405)apps/desktop/src/store.ts (1)
generalSettingsStore(61-62)apps/desktop/src/routes/(window-chrome)/settings/Setting.tsx (1)
SettingItem(3-22)
crates/recording/src/studio_recording.rs (2)
crates/recording/src/feeds/camera.rs (2)
handle(327-470)handle(476-492)crates/recording/src/feeds/microphone.rs (9)
handle(330-654)handle(660-672)handle(678-680)handle(686-706)handle(722-758)handle(764-776)handle(782-798)handle(804-817)handle(823-836)
apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (2)
apps/desktop/src/store.ts (1)
authStore(59-59)apps/desktop/src/utils/tauri.ts (2)
CaptureDisplay(377-377)CaptureWindow(379-379)
🪛 GitHub Actions: CI
crates/recording/src/feeds/microphone.rs
[error] 809-816: this if statement can be collapsed
crates/recording/src/feeds/camera.rs
[error] 672-678: this if statement can be collapsed
🪛 GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
crates/recording/src/feeds/microphone.rs
[failure] 809-816: this if statement can be collapsed
error: this if statement can be collapsed
--> crates/recording/src/feeds/microphone.rs:809:9
|
809 | / if let State::Locked { inner } = &mut self.state {
810 | | if inner.label == msg.label {
811 | | inner.id = msg.id;
812 | | inner.config = msg.config;
... |
816 | | }
| |_________^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.91.0/index.html#collapsible_if
help: collapse nested if block
|
809 ~ if let State::Locked { inner } = &mut self.state
810 ~ && inner.label == msg.label {
811 | inner.id = msg.id;
...
814 | inner.done_tx = msg.done_tx;
815 ~ }
|
crates/recording/src/feeds/camera.rs
[failure] 672-678: this if statement can be collapsed
error: this if statement can be collapsed
--> crates/recording/src/feeds/camera.rs:672:9
|
672 | / if let State::Locked { inner } = &mut self.state {
673 | | if inner.id == msg.id {
674 | | inner.camera_info = msg.camera_info;
675 | | inner.video_info = msg.video_info;
... |
678 | | }
| |_________^
|
= help: for further information visit https://rust-lang.github.io/rust-clippy/rust-1.91.0/index.html#collapsible_if
= note: requested on the command line with -D clippy::collapsible-if
help: collapse nested if block
|
672 ~ if let State::Locked { inner } = &mut self.state
673 ~ && inner.id == msg.id {
674 | inner.camera_info = msg.camera_info;
675 | inner.video_info = msg.video_info;
676 | inner.done_tx = msg.done_tx;
677 ~ }
|
⏰ 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). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (46)
apps/desktop/src-tauri/src/windows.rs (1)
628-629: LGTM! Window dimensions updated to accommodate enhanced recording controls.The increased dimensions (320×150 vs. the previous 250×40) appropriately support the new inline camera/mic selectors and recording controls mentioned in the PR objectives. The positioning calculations at lines 646-647 correctly adapt to these new values, maintaining proper centering and bottom margin.
crates/recording/src/output_pipeline/macos.rs (2)
66-110: Explicit retry logic is clearer, but fix the unbounded loops first.The shift from retry-crate macros to explicit loops improves readability and maintains the intentional blocking behavior required to prevent frame interleaving. However, the unbounded retry loops must be fixed before merging.
Based on learnings: The lock-holding during retries is correct and intentional to ensure sequential frame processing.
98-109: Add a retry limit to prevent infinite blocking.Similar to
send_video_frame, this loop will retry indefinitely whenNotReadyForMoreis returned. Apply the same fix to prevent thread hangs.+ const MAX_RETRIES: u32 = 500; // 500 * 2ms = 1 second max wait + let mut retry_count = 0; + loop { match mp4.queue_audio_frame(&frame.inner, timestamp) { Ok(()) => break, Err(QueueFrameError::NotReadyForMore) => { + retry_count += 1; + if retry_count >= MAX_RETRIES { + return Err(anyhow!("send_audio_frame/timeout after {} retries", MAX_RETRIES)); + } std::thread::sleep(Duration::from_millis(2)); continue; } Err(e) => return Err(anyhow!("send_audio_frame/{e}")), } }⛔ Skipped due to learnings
Learnt from: Brendonovich Repo: CapSoftware/Cap PR: 1305 File: crates/recording/src/output_pipeline/macos.rs:80-90 Timestamp: 2025-10-28T08:39:42.230Z Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.packages/ui-solid/src/auto-imports.d.ts (1)
28-29: All icon files verified to exist—no issues found.The referenced icon files are confirmed: cursor-macos.svg, cursor-windows.svg, and monitor.svg exist in packages/ui-solid/icons/, and lucide includes the alert-triangle icon. The auto-generated declarations correctly reflect the discoverable icons from both the local cap collection and the lucide package.
apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs (4)
26-32: LGTM! Clean delegation pattern.The refactoring to extract common logic into
prewarm_shareable_content_innerwith aforce_refreshparameter follows the DRY principle while maintaining a clear public API surface.
34-40: Approve the force-refresh logic with a verification note.The cache-clearing logic is correct: the write lock on Line 36 ensures exclusive access when clearing, and the early-return path on Line 37 uses a read lock for concurrent access. The function properly coordinates with in-flight warmup tasks via the async
Mutexacquired later (Line 43).One subtle point: when
force_refresh=trueand an in-flight warmup exists, this function waits for that warmup to complete. Sincesc::ShareableContent::current()is fetched at execution time (Line 87 inrun_warmup), not spawn time, the data will be fresh even if the warmup task was spawned before the force-refresh request.Please verify the concurrent force-refresh behavior with a test or manual review, particularly the scenario where:
- Thread A starts a prewarm (spawns warmup task)
- Thread B calls refresh (clears cache, waits for A's warmup)
- Both threads receive the fresh fetch result
Expected: Both get fresh data from the system at warmup execution time.
174-178: LGTM! Correct conditional delegation.The logic correctly routes to
refresh_shareable_content()when a forced refresh is requested, otherwise falls back to the standardprewarm_shareable_content()path.
1-200: Verify rustfmt and clippy compliance manually.The sandbox environment cannot execute cargo commands. Please verify locally before merging:
cargo fmt -- --check apps/desktop/src-tauri/src/platform/macos/sc_shareable_content.rs cargo clippy --workspace -- -D warningsBoth commands must pass without errors per the coding guidelines.
crates/recording/src/output_pipeline/win.rs (4)
232-248: LGTM: Native encoder timestamp handling.The refactored timestamp logic correctly computes relative frame times starting from zero. Since timestamps are already pause-adjusted when sent through
video_tx(line 332), the encoder receives a continuous timeline with pause durations removed.The
unwrap_or(Duration::ZERO)fallback on line 243 handles potential non-monotonic timestamps gracefully, though such cases should be rare in practice.
326-336: LGTM: Pause-aware video frame dispatch.Correctly routes frames through
PauseTracker.adjustand only sends to the encoder when an adjusted timestamp is available. Frames arriving during pause are appropriately skipped.
340-350: LGTM: Pause-aware audio frame dispatch.Correctly integrates with
PauseTrackerto skip frames during pause and adjust timestamps. The let-chain syntax (Rust 1.65+) cleanly handles the multiple conditions.Note that if
output.lock()fails, the audio frame is silently dropped—consistent with the video path (line 266), but consider whether poisoned-mutex cases warrant additional logging or monitoring.
60-60: Verification confirmed: All Muxer implementations updated consistently.All four
Muxertrait implementations have been correctly updated with thepause_flag: Arc<AtomicBool>parameter:
- Windows (win.rs:80)
- macOS (macos.rs:33)
- Mp4Muxer (ffmpeg.rs:41)
- OggMuxer (ffmpeg.rs:128)
The trait definition in core.rs:849 matches all implementations. The
PauseTrackerintegration is complete and consistent across the codebase.apps/desktop/src/routes/(window-chrome)/new-main/index.tsx (9)
32-35: Mode/auth integration and header restructuring look consistentUsing
authStore.createQuery()to drive the dashboard link and introducing<Mode />in the header follows existing store/query patterns and keeps concerns localized to this route. The new top bar grouping (settings, recordings, changelog, license badge, Mode) is coherent and the conditional dashboard URL based onauth.datareads cleanly. I don’t see functional issues here.Also applies to: 770-865
76-80: Window size bump appears safeUpdating
getWindowSize()to return 290×310 should give the new layout more breathing room. Given that you also clamp the window size on focus/resize elsewhere in this file, this change looks self‑consistent.
195-229: TargetMenuPanel flex/min-h-0 adjustments improve scrollingThe new
flex/flex-1/min-h-0structure on the panel container should resolve nested flex overflow issues and make the list area’s scrolling more reliable in constrained heights. No behavioral concerns from this change.
398-416: Overlay opening logic: verify idempotence when toggling modesPassing
{ variant: "display" | "window", id: target.id }intocommands.openTargetSelectOverlaysfrom the select handlers aligns with theScreenCaptureTargetshape and should give the overlay a precise initial focus.toggleTargetModethen usesopenTargetSelectOverlays(null)/closeTargetSelectOverlays()to manage overlay visibility based ontargetMode.One thing to double‑check: when switching between modes while the picker is already active,
toggleTargetModewill callopenTargetSelectOverlays(null)again. As long as the underlying Tauri command is idempotent (reusing or updating existing overlays rather than spawning duplicates), this is fine; if not, you might want to only callopenTargetSelectOverlayson transitions fromnull -> mode.Also applies to: 585-591
531-554: ExplicitCaptureDisplay/CaptureWindowlocals improve type safetyIntroducing
let screen: CaptureDisplay | undefinedandlet win: CaptureWindow | undefinedclarifies the return types ofoptions.screen()andoptions.window()and makes theundefinedcases explicit. The control flow and fallbacks remain the same as before, so behavior should be unchanged.
585-592: toggleTargetMode wiring into target buttons is sensibleCentralizing target-mode changes in
toggleTargetModeand reusing it from the Display/Window/AreaTargetTypeButtoncomponents keeps the logic consistent and ensures you respect theisRecording()guard everywhere. The dropdown buttons still control the inline menus independently, which matches the intent of separating picker overlays from the menu UI.Also applies to: 671-745
780-838: Confirm imports or global registration for icon componentsIn this file I see usages of
IconCapSettings,IconLucideSquarePlay,IconLucideBug,IconCapLogoFullDark, andIconCapLogoFull, but I don’t see corresponding imports. If these aren’t globally registered components via your tooling, TypeScript/TSX will fail at compile time.Please confirm they are provided globally; if not, add the appropriate imports for these icons.
824-865: License badge + dashboard link behavior looks goodThe conditional
hreffor the logo (dashboard vs marketing site) and the license badge click-through to"Upgrade"for non‑pro plans are straightforward and gated correctly onlicense.data?.type. Wrapping the badge inErrorBoundary+Suspensekeeps failures and loading states from impacting the rest of the header. This all reads cleanly.
867-918: Sign-in overlay and cancel flow are generally solid; verify abort wiringThe full-screen “Signing In…” overlay is correctly layered via
absolute inset-0on arelativeroot, and gating the underlying content on!signIn.isPendingavoids accidental interaction. UsingsignIn.variables?.abort()plussignIn.reset()on the cancel button is a reasonable pattern assumingstart-sign-inalways callssignIn.mutateAsyncwith anAbortController.Please double‑check that:
createSignInMutationconsistently setsvariablesto theAbortControllerpassed from the"start-sign-in"handler, and- there are no other call sites where
signInis used with differentvariablesshapes.If those assumptions hold, this flow should behave as expected.
apps/desktop/src-tauri/Cargo.toml (1)
3-3: Version bump to 0.3.84 looks finePackage metadata change only; no functional impact from this file.
crates/recording/src/sources/screen_capture/macos.rs (1)
48-53: ExposingSourceErrorpublicly is appropriateMaking
SourceErrorpubso other crates (e.g. tauri layer) can express macOS screen capture failures is reasonable and does not alter internal behavior.crates/recording/src/studio_recording.rs (2)
400-412: ActorHandle feed setters are thin, well-scoped wrappers
ActorHandle::set_mic_feedandset_camera_feedsimplyaskthe actor with the new messages and bubble upanyhow::Result<()>, which keeps the external API minimal while reusing the actor’s validation logic.
719-725: SegmentPipelineFactory setters correctly feed into future segmentsUpdating
self.base_inputs.mic_feedand.camera_feedhere ensures subsequentcreate_nextcalls use the new feeds without disturbing the currently running segment, which matches the paused‑then‑resume semantics.crates/recording/src/feeds/microphone.rs (1)
291-297: LGTM: Reconnection handling follows established patterns.The
LockedInputReconnectedmessage and its handler correctly implement input reconnection for locked feeds. The label-based matching ensures the correct device is being reconnected, and the implementation parallels the camera feed pattern shown incamera.rs.Also applies to: 801-818
apps/desktop/src/routes/(window-chrome)/settings/experimental.tsx (1)
20-30: LGTM: Feature successfully graduated from experimental.The new recording flow is now enabled by default and the experimental toggle has been appropriately removed. This aligns with the PR objectives to make the new recording picker the default experience.
apps/desktop/src/styles/theme.css (1)
264-375: LGTM: Selection hint animations properly implemented.The CSS for the new selection hint overlay is well-structured with appropriate animations and timing functions. The animations provide a smooth user experience to guide area selection.
apps/desktop/src-tauri/src/general_settings.rs (1)
243-263: LGTM: One-time migration logic correctly implemented.The initialization logic properly implements a one-time opt-in mechanism for the new recording flow. Existing users who haven't explicitly set their preference will be migrated to the new flow, while the flag prevents this from running on subsequent launches.
apps/desktop/src/utils/tauri.ts (1)
1-3: Auto-generated file - no manual review required.This file is auto-generated by tauri-specta and should not be manually edited. The type updates correctly reflect the Rust-side changes.
apps/desktop/src/components/SelectionHint.tsx (1)
1-47: LGTM: Selection hint component properly implemented.The component correctly implements a platform-aware selection hint overlay with appropriate accessibility attributes and fallback message. The OS detection approach is suitable since the platform doesn't change at runtime.
apps/desktop/src/routes/camera.tsx (3)
50-61: LGTM: Camera disconnect detection properly implemented.The event listener correctly tracks camera disconnect/reconnect events using the new
InputLostandInputRestoredevent variants. ThecreateTauriEventListenerutility ensures proper cleanup.
98-100: LGTM: Disconnect overlay consistently integrated.The
CameraDisconnectedOverlayis consistently integrated into both native and legacy camera preview pages, appearing when the camera is disconnected.Also applies to: 291-293
414-425: LGTM: Disconnect overlay component well-designed.The overlay component is appropriately styled with backdrop blur, proper z-index, and inherits the camera window's border radius for visual consistency. The
pointer-events-noneallows interaction with controls while showing the disconnect state.apps/desktop/src/routes/capture-area.tsx (3)
74-81: LGTM: Selection hint conditions properly implemented.The logic correctly shows the selection hint to guide users who haven't made a selection yet. The conditions ensure the hint only appears when relevant (UI visible, no stored selection, minimal crop bounds).
Also applies to: 129-134
275-275: LGTM: Selection hint properly integrated.The
SelectionHintcomponent is correctly placed and uses the reactiveshowSelectionHint()signal to control visibility. The default message is appropriate for this context.
289-298: LGTM: Initial crop logic safely handles stored selections.The
initialCropfunction correctly retrieves stored bounds when available and safely falls back toCROP_ZERO. This ensures the cropper always has a valid initial value.apps/desktop/src/routes/(window-chrome)/settings/general.tsx (1)
214-225: Recording window variant toggle wiring is correct and avoids redundant writes
recordingWindowVariant()correctly mapsenableNewRecordingFlowto"new"/"old", andupdateRecordingWindowVariantearly‑returns when the requested variant matches the current state, which prevents unnecessary writes.- The segmented control UI uses
aria-pressed, highlights the active option, and persists viaupdateRecordingWindowVariant, which also marksrecordingPickerPreferenceSet: truefor analytics/one‑time‑prompt logic. This all looks sound.Also applies to: 438-465
apps/desktop/src-tauri/src/recording.rs (2)
327-343: Recording input events and camera state tie-in are coherent
- Adding
RecordingInputKindand extendingRecordingEventwithInputLost/InputRestoredprovides a clean, typed bridge to the frontend for device issues.- Marking
state.camera_in_use = camera_feed.is_some()at actor spawn and resetting bothapp.disconnected_inputsandapp.camera_in_useinhandle_recording_endkeeps the watcher logic inlib.rsfrom running against stale state after recordings finish.- The macOS actor retry branch that checks
is_shareable_content_error(&err)and then reacquiresshareable_contentbefore retrying is integrated correctly into the mic‑retry loop.- The conditional cleanup of instant recording directories based on
delete_instant_recordings_after_uploadis guarded by both successful upload and filesystem error logging, which is safe.Also applies to: 558-563, 691-702, 985-987, 1154-1156
11-14: macOS shareable-content acquisition and retry are well-scoped
- Wrapping
ShareableContentinSendableShareableContentwith explicitSend/Syncand aretained()helper keeps the actor interface simple while still using the cached content.acquire_shareable_content_for_targetcorrectly:
- Fetches cached content (or prewarms),
- Validates that the target's display is representable in that content via
shareable_content_missing_target_display,- Performs at most one
refresh_shareable_content()before failing with a clear error.is_shareable_content_error's downcast toSourceErrorand match onAsContentFilteris a narrow, explicit retry condition.AsContentFilteris the only variant indicating transient stale content (returned when content filter creation fails on an already-resolved display), whileNoDisplayis a permanent error (display doesn't exist) and should not trigger retry.- The error‑loop in
start_recordingthat re‑acquiresshareable_contentonly whenis_shareable_content_erroris true looks correct and won't spin indefinitely.apps/desktop/src/routes/in-progress-recording.tsx (1)
479-642: Control bar UX for issues, mic status, and camera preview is coherent
- The new issue panel, mic icon with live level indicator, and issue button tied to
hasRecordingIssue/issuePanelVisiblegive clear, low‑friction visibility into input problems and backend failures.- Disabling pause/resume while
hasDisconnectedInput()is true prevents resuming a recording when inputs are in a broken state.- Camera preview toggle correctly uses
isCameraWindowOpenplusshowWindow("Camera")/WebviewWindow.getByLabel("camera")and re‑reads state afterward, so the button reflects the actual window state.- The added settings button that opens
openRecordingSettingsMenurounds out the in‑recording device management story without cluttering the main stop/pause/restart controls.apps/desktop/src-tauri/src/lib.rs (3)
330-401:set_mic_inputcorrectly coordinates feed changes with studio recordings and disconnection state
- The function now:
- Reads
mic_feed, any active studio handle, and the current selected label without holding the lock across awaits.- Short‑circuits when the requested
labelequals the current selection.- If a studio recording is active, first detaches the mic feed (
set_mic_feed(None)), then reconfigures the underlyingMicFeed(SetInputorRemoveInput), and finally re‑attaches a locked feed to the studio actor when a label is present.- Updates
selected_mic_labeland, if the mic was previously marked as disconnected, clears that flag and emits anInputRestoredevent.- Device‑not‑found errors on restart are handled explicitly and logged via
restart_mic_feed, so transient unavailability doesn’t crash the app.
409-490:set_camera_inputmirrors mic semantics and keepscamera_in_useconsistent
- Similar to
set_mic_input,set_camera_input:
- Avoids unnecessary work when
idequalscurrent_idandcamera_in_useis already true.- Detaches any studio camera feed (
set_camera_feed(None)), then appliesRemoveInputorSetInputon theCameraFeed, showing the camera preview window when a camera is selected.- For studio recordings, locks the camera feed and re‑attaches it to the actor.
- Updates
selected_camera_idandcamera_in_use, clears anyCameraentry indisconnected_inputs, and emitsInputRestoredwhen appropriate.- This ensures that studio and instant recordings see the same camera state, and that the frontend’s input‑issue UI stays in sync with backend state.
178-214: Mic error handling and watcher bootstrapping are wired correctly at startup and restart
restart_mic_feednow creates a fresh error channel and spawnsspawn_mic_error_handlerfor the new feed, so error handling continues to work after restarts.- In
run, mic errors are also wired once theAppstate is created (spawn_mic_error_handler(app.clone(), mic_error_rx)), and device watchers are started viaspawn_device_watchers(app.clone()).AudioInputLevelemission continues to use the samemic_samples_rxchannel, so the existing level meter UI stays functional.Also applies to: 2301-2317, 2477-2492, 2499-2501
apps/desktop/src/routes/target-select-overlay.tsx (2)
387-405: Area-mode RecordingControls correctly guard start and sync capture target
- In area mode,
isValidandcommittedIsValidenforce the minimum crop size, andstartDisabledis wired toprops.disabled, so the “Start Recording” button is effectively disabled when the crop is too small.- Before starting, the click handler for the primary button checks
props.target.variant === "area"and then writes the current area bounds intocaptureTargetviasetOptions, ensuring the global options reflect what’s actually sent tocommands.startRecording.- The min-size error callout and the
SelectionHintoverlay provide clear feedback when the selection is too small or hasn’t been drawn yet, which should keep users from accidentally starting a tiny or empty capture.Also applies to: 520-577, 585-642
5-6: Serialization shapes match exactly—no risk of mismatchThe verification is confirmed:
- Rust
DeviceOrModelIDenum uses default externally-tagged representation withserde::SerializeModelIDvariant contains a struct that custom-serializes to string format"vid:pid"- With externally-tagged enums, this produces JSON:
{"DeviceID": "..."} | {"ModelID": "vid:pid"}- TypeScript type is
{ DeviceID: string } | { ModelID: ModelIDType }whereModelIDType = string- These match exactly; Specta generates the TS types correctly from the Rust definitions
No serialization mismatch risk exists.
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 (5)
crates/recording/src/feeds/camera.rs(3 hunks)crates/recording/src/feeds/microphone.rs(5 hunks)crates/recording/src/sources/audio_mixer.rs(2 hunks)crates/rendering/src/lib.rs(2 hunks)crates/rendering/src/main.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/recording/src/feeds/camera.rs
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/rendering/src/lib.rscrates/recording/src/sources/audio_mixer.rscrates/recording/src/feeds/microphone.rscrates/rendering/src/main.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/rendering/src/lib.rscrates/recording/src/sources/audio_mixer.rscrates/recording/src/feeds/microphone.rscrates/rendering/src/main.rs
🧬 Code graph analysis (1)
crates/recording/src/feeds/microphone.rs (1)
crates/recording/src/feeds/camera.rs (11)
handle(327-470)handle(476-492)handle(498-500)handle(506-523)handle(529-535)handle(541-557)handle(573-609)handle(615-639)handle(645-661)handle(667-679)handle(685-698)
⏰ 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). (1)
- GitHub Check: Analyze (rust)
🔇 Additional comments (7)
crates/recording/src/sources/audio_mixer.rs (2)
102-104: LGTM: Refactoring improves readability.The switch from positional to inline named placeholders in the format string makes the code more readable and less error-prone.
119-121: LGTM: Consistent refactoring.The inline format placeholders maintain consistency with the aresample filter configuration above and improve code clarity.
crates/rendering/src/main.rs (1)
152-152: LGTM! Modern Rust formatting syntax.The inline variable formatting
format!("frame.{extension}")is more readable and idiomatic than the positional placeholder approach. This is a good modernization.crates/rendering/src/lib.rs (1)
430-430: Verification complete—rename is fully applied.All
to_uvreferences have been updated topoint_to_uvin theMotionBoundsimplementation. The method definition (line 430) and its only call site (line 516) are consistent with no remaining stale references.crates/recording/src/feeds/microphone.rs (3)
64-64: LGTM: Label tracking enables proper device reconnection.The addition of
labelfields toAttachedStateandInputConnectedallows tracking device identity across reconnection events, which is essential for the new reconnection logic.Also applies to: 81-81, 285-285
291-297: LGTM: Reconnection message struct follows established pattern.The
LockedInputReconnectedstruct mirrors the design fromcamera.rsand includes all necessary fields for updating the locked feed state during reconnection.
801-818: LGTM: Reconnection handler correctly implements the guard pattern.The handler properly uses the guard pattern (
&& inner.label == msg.label) to avoid nested if statements, which satisfies the clippy lint mentioned in previous reviews. The logic correctly updates the locked state when a device with a matching label reconnects.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
crates/recording/src/feeds/camera.rs (1)
335-498: Locked SetInput reconnection flow is sound; consider reducing duplication with Open pathThe Locked branch correctly:
- Rejects mismatched ids while locked.
- Spins a new camera setup, asks the actor to update locked state, and only resolves the ready future on success.
- Interprets
Ok(false)andErr(_)fromLockedCameraInputReconnectedas failures, returningBuildStreamCrashedand stopping the new handle.This matches the existing Open-path behavior while adding the necessary locking semantics. The Open and Locked branches now duplicate a fair amount of setup (channels, runtime/thread spawn, setup_camera call); extracting this into a shared helper would reduce surface for future drift but isn’t strictly required.
crates/recording/src/studio_recording.rs (1)
719-725: SegmentPipelineFactory mutators correctly propagate new feeds to future segmentsUpdating
self.base_inputs.mic_feed/camera_feedinsideSegmentPipelineFactoryensures that subsequentcreate_nextcalls use the latest feeds after a pause. The implementation is minimal and precise; if you later add more mutable recording inputs, a shared helper for updatingbase_inputscould keep this pattern DRY, but it’s fine as‑is.crates/recording/src/output_pipeline/win.rs (1)
258-263: Consider logging when timestamps go backward.The defensive fallback to
Duration::ZEROon line 259 prevents panics if frames arrive out of order, but duplicate zero timestamps could degrade encoding quality. Consider logging a warning to aid troubleshooting.let relative = if let Some(first) = first_timestamp { - timestamp.checked_sub(first).unwrap_or(Duration::ZERO) + timestamp.checked_sub(first).unwrap_or_else(|| { + warn!("Frame timestamp out of order: timestamp={timestamp:?}, first={first:?}"); + Duration::ZERO + }) } else { first_timestamp = Some(timestamp); Duration::ZERO };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
crates/recording/src/feeds/camera.rs(3 hunks)crates/recording/src/output_pipeline/macos.rs(2 hunks)crates/recording/src/output_pipeline/win.rs(6 hunks)crates/recording/src/studio_recording.rs(4 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/output_pipeline/win.rscrates/recording/src/studio_recording.rscrates/recording/src/feeds/camera.rscrates/recording/src/output_pipeline/macos.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/output_pipeline/win.rscrates/recording/src/studio_recording.rscrates/recording/src/feeds/camera.rscrates/recording/src/output_pipeline/macos.rs
🧠 Learnings (2)
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/recording/src/output_pipeline/win.rscrates/recording/src/studio_recording.rscrates/recording/src/feeds/camera.rscrates/recording/src/output_pipeline/macos.rs
📚 Learning: 2025-10-17T05:58:22.586Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1219
File: crates/enc-avfoundation/src/mp4.rs:350-373
Timestamp: 2025-10-17T05:58:22.586Z
Learning: In crates/enc-avfoundation/src/mp4.rs, the `finish()` method intentionally skips video extension when `is_paused` is true. This is correct behavior because if recording is paused, the video should not be extended beyond the pause point—the pause is user-initiated, unlike the case where ScreenCaptureKit stops providing frames during static content.
Applied to files:
crates/recording/src/output_pipeline/win.rscrates/recording/src/output_pipeline/macos.rs
🧬 Code graph analysis (4)
crates/recording/src/output_pipeline/win.rs (2)
crates/recording/src/output_pipeline/core.rs (7)
new(307-325)new(675-677)new(718-720)new(751-753)new(795-812)timestamp(838-838)send_audio_frame(861-861)crates/recording/src/output_pipeline/ffmpeg.rs (3)
timestamp(22-24)send_audio_frame(110-116)send_audio_frame(153-155)
crates/recording/src/studio_recording.rs (2)
crates/recording/src/feeds/camera.rs (2)
handle(327-499)handle(505-521)crates/recording/src/feeds/microphone.rs (9)
handle(330-654)handle(660-672)handle(678-680)handle(686-706)handle(722-758)handle(764-776)handle(782-798)handle(804-817)handle(823-836)
crates/recording/src/feeds/camera.rs (2)
apps/desktop/src/utils/tauri.ts (1)
DeviceOrModelID(394-394)crates/recording/src/feeds/microphone.rs (9)
handle(330-654)handle(660-672)handle(678-680)handle(686-706)handle(722-758)handle(764-776)handle(782-798)handle(804-817)handle(823-836)
crates/recording/src/output_pipeline/macos.rs (2)
crates/recording/src/output_pipeline/core.rs (2)
timestamp(838-838)send_audio_frame(861-861)crates/recording/src/output_pipeline/ffmpeg.rs (3)
timestamp(22-24)send_audio_frame(110-116)send_audio_frame(153-155)
⏰ 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). (2)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (9)
crates/recording/src/output_pipeline/macos.rs (2)
66-101: Well done implementing the retry limit for video frames.The bounded retry loop properly addresses the previous review concern about infinite blocking. The timeout of ~1 second (500 retries × 2ms) is reasonable for a queue operation, and the error message clearly indicates the failure mode.
Based on learnings: The approach of holding the mutex lock during retries remains correct for maintaining sequential frame processing.
105-120: Critical: Add a retry limit to prevent infinite blocking in audio path.The audio frame queueing has the same infinite retry issue that was fixed in the video path (lines 81-98). If the encoder becomes stuck returning
NotReadyForMore, this loop will block indefinitely and hang the recording thread.Apply a retry limit consistent with the video path:
impl AudioMuxer for AVFoundationMp4Muxer { fn send_audio_frame(&mut self, frame: AudioFrame, timestamp: Duration) -> anyhow::Result<()> { let mut mp4 = self.0.lock().map_err(|e| anyhow!("{e}"))?; + const MAX_AUDIO_QUEUE_RETRIES: u32 = 500; + let mut retry_count = 0; + loop { match mp4.queue_audio_frame(&frame.inner, timestamp) { Ok(()) => break, Err(QueueFrameError::NotReadyForMore) => { + retry_count += 1; + if retry_count >= MAX_AUDIO_QUEUE_RETRIES { + return Err(anyhow!( + "send_audio_frame/timeout after {} retries", + MAX_AUDIO_QUEUE_RETRIES + )); + } std::thread::sleep(Duration::from_millis(2)); continue; } Err(e) => return Err(anyhow!("send_audio_frame/{e}")), } } Ok(()) } }⛔ Skipped due to learnings
Learnt from: Brendonovich Repo: CapSoftware/Cap PR: 1305 File: crates/recording/src/output_pipeline/macos.rs:80-90 Timestamp: 2025-10-28T08:39:42.230Z Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.Learnt from: Brendonovich Repo: CapSoftware/Cap PR: 1219 File: crates/enc-avfoundation/src/mp4.rs:350-373 Timestamp: 2025-10-17T05:58:22.586Z Learning: In crates/enc-avfoundation/src/mp4.rs, the `finish()` method intentionally skips video extension when `is_paused` is true. This is correct behavior because if recording is paused, the video should not be extended beyond the pause point—the pause is user-initiated, unlike the case where ScreenCaptureKit stops providing frames during static content.crates/recording/src/feeds/camera.rs (2)
176-181: LockedCameraInputReconnected event wiring looks consistent with existing feed patternsThe new private event cleanly carries the id, camera/video info, and done_tx needed to refresh the locked camera state, mirroring the microphone’s reconnection message shape. No functional issues here.
693-713: LockedCameraInputReconnected handler correctly updates state and tears down the old captureThe handler’s
if letchain onState::Lockedand matchingidis appropriate, and the sequence:
- Updates
camera_info/video_info,- Swaps in the new
done_txand signals the previous one,- Returns
true/falseto reflect whether the locked state still matched,gives the caller precise feedback and ensures the old capture handle is cleanly stopped. This matches the intended reconnection semantics and aligns with the existing done_tx usage elsewhere in this feed.
crates/recording/src/studio_recording.rs (2)
13-13: Mic/camera feed change messages and error handling are well‑gatedImporting
bailandanyhowsupports the new SetMicFeed/SetCameraFeed handlers. The handlers:
- Forbid changing feeds while
Recording, with clear bail! messages.- Allow updates while
Pausedby mutatingsegment_factory’s base inputs.- Return a clear error once the recording actor is inactive.
This cleanly enforces the intended “pause before changing inputs” policy without mutating active pipelines.
Also applies to: 224-268
400-412: ActorHandle APIs for changing mic/camera feeds match existing handle patternsThe new
set_mic_feedandset_camera_feedmethods mirror the existingstop/pause/resume/cancelAPIs by wrapping anask(...)call and propagating the inneranyhow::Result. This keeps the public surface consistent and makes it straightforward for callers to react to state‑gating errors.crates/recording/src/output_pipeline/win.rs (3)
22-69: Excellent error handling for timestamp adjustments.The new
PauseTrackerimplementation properly addresses the overflow and underflow concerns from the previous review. The error propagation with descriptive context (lines 46-50, 52-57, 60-65) will aid debugging if timing issues occur, and the shared instance ensures audio/video remain synchronized during pause/unpause operations.
369-381: LGTM: Correct Duration to TimeSpan conversion.The conversion logic correctly handles Windows TimeSpan's 100-nanosecond tick rate. Saturating arithmetic and clamping to
i64::MAXprevent overflow, ensuring valid timestamps even for very long recordings.
96-96: No issues found—pause_flag is correctly shared and synchronized across the pipeline.The
pause_flagis properly shared as anArc<AtomicBool>instance created inBuildCtx, stored inOutputPipeline, and passed to the muxer. The control layer exposes publicpause()andresume()methods that atomically modify the flag withstore(), while the muxer'sPauseTrackermonitors it with atomicload()operations. Both audio and video frames respect the pause state throughPauseTracker::adjust(), ensuring consistent synchronization across the pipeline.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/recording/src/feeds/camera.rs (1)
752-777: Locked reconnection handler correctly stages and overwrites stateThe
LockedCameraInputReconnectedhandler only proceeds when still locked on the sameid, stages the existing stream for pending release, overwrites attachment data, and returns aboolso the caller can decide whether to proceed or tear down. This neatly guards against races where the state changes between reconnect start and completion.
🧹 Nitpick comments (3)
crates/recording/src/feeds/camera.rs (3)
80-132: Pending-release management inAttachedStateis coherent; minor cleanup possibleThe
new,overwrite,stage_pending_release, andfinalize_pending_releasehelpers correctly encapsulate state transitions and ensure previous streams are signaled exactly once. Now thatidis read inFinalizePendingRelease, the#[allow(dead_code)]attribute is stale and could be removed.
248-358:spawn_camera_setupunifies setup nicely; consider runtime reuseThe function cleanly centralizes camera setup, ready/error signaling, and locked vs open behavior (including reconnection and cleanup), and the
ReadyFuturewiring looks correct. One thing to watch: creating a freshtokio::Runtimeper setup call may be heavier than necessary; ifSetInputcan be invoked frequently, consider reusing a shared runtime/handle rather than constructing a new one each time.
714-718:InputConnectedhandler relies solely on the sharedreadyfutureIgnoring the
InputConnectedpayload and instead awaiting the sharedreadyfuture fromstate.connectingis a bit unusual but internally consistent: it keeps all data flow viaReadyFutureand ensures pending releases are finalized via the same helper as inLock.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/recording/src/feeds/camera.rs(9 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.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/feeds/camera.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/feeds/camera.rs
🧠 Learnings (1)
📚 Learning: 2025-10-28T08:39:42.230Z
Learnt from: Brendonovich
Repo: CapSoftware/Cap PR: 1305
File: crates/recording/src/output_pipeline/macos.rs:80-90
Timestamp: 2025-10-28T08:39:42.230Z
Learning: In `crates/recording/src/output_pipeline/macos.rs`, the `AVFoundationMp4Muxer` intentionally holds the `Mutex<MP4Encoder>` lock during retry attempts in `send_video_frame()` and `send_audio_frame()`. This blocking behavior is correct because frame processing must happen sequentially to prevent audio and video frames from being interleaved incorrectly in the encoder.
Applied to files:
crates/recording/src/feeds/camera.rs
🧬 Code graph analysis (1)
crates/recording/src/feeds/camera.rs (1)
crates/recording/src/feeds/microphone.rs (11)
handle_input_connected(58-71)new(88-98)handle(330-654)handle(660-672)handle(678-680)handle(686-706)handle(722-758)handle(764-776)handle(782-798)handle(804-817)handle(823-836)
⏰ 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). (2)
- GitHub Check: Clippy (aarch64-apple-darwin, macos-latest)
- GitHub Check: Analyze (rust)
🔇 Additional comments (9)
crates/recording/src/feeds/camera.rs (9)
6-9: Futures imports align with new readiness flow
FutureExt,BoxFuture, andSharedare all used correctly for the shared readiness future pattern; no issues here.
54-71:OpenState::handle_input_connectedlogic looks soundReturning
booland handling attach vs reconnect here keeps callers simple, and thestage_pending_release+ attach logic is coherent with later finalization calls inLockandInputConnected.
221-227: Readiness alias and flow enum are a good abstraction
ReadyFutureplusCameraSetupFlowcleanly separate open vs locked setup paths while sharing the underlying machinery; matches the microphone feed pattern well.
233-238:LockedCameraInputReconnectedpayload is appropriately scopedCarrying just
id,{camera,video}_info, anddone_txis sufficient for reconnection and keeps this message tightly focused on locked-state updates.
244-246:FinalizePendingReleasemessage keeps cleanup explicitUsing an explicit message keyed by
DeviceOrModelIDfor deferred cleanup is a clear way to finish pending releases after a successful locked reconnect.
508-549: RefactoredSetInputhandler correctly routes throughspawn_camera_setupOpen and locked flows both now use the same setup path and return a future resolving to
(CameraInfo, VideoInfo), with appropriate locked-id validation and error propagation. The mapping fromReadyFutureto the public reply type is straightforward and consistent.
563-566:RemoveInputnow also cleans up pending releasesFinalizing any
pending_releasebefore sending ondone_txensures both the current and any previously staged streams are signaled, avoiding leaked capture handles.
664-668: Lock path correctly finalizes previous stream after successful connectUsing
state.handle_input_connected(..)’s boolean plus a chainedlet Some(attached)to callfinalize_pending_releasemirrors the InputConnected flow and ensures we don’t leave the prior camera stream running when locking while a connection is in progress.
779-802:FinalizePendingReleasesafely handles both open and locked statesMatching on
State::OpenvsState::Lockedand checkingidbefore callingfinalize_pending_releasemeans the pending-release signal is only sent when the attachment still corresponds to the expected device/model, avoiding accidental teardown after unrelated state changes.
This PR sets the new and improved recording picker to default. Users can toggle back to the old picker in Settings -> General.
Overview:
apps/desktop/src/routes/target-select-overlay.tsxsurface that listens toevents.targetUnderCursor, drivesRecordingOptionsProvider, and lets users jump between display, window, and area capture with inline camera/mic selectors, recording controls, and mode switchingapps/desktop/src/routes/capture-area.tsxto share the picker experience: persisted ratios, snap-to-grid options, validation, and seamless handoff back to the overlay when confirming or cancelling an area targetSelectionHintoverlay plus the newmonitor,cursor-windows, andcursor-macosassets inpackages/ui-solid/icons, giving users immediate guidance while they drag out a regionapps/desktop/src-tauri/src/windows.rsso capture/overlay windows spawn per-display, ignore pointer events until needed, and minimize the main window when a monitor picker is active, keeping the UX distraction-freeSummary by CodeRabbit
New Features
Improvements
Bug Fixes