Repository navigation
feat(desktop): recording settings menu with mid-recording microphone and camera switching - #4262
richiemcilroy wants to merge 7 commits into
Conversation
…y's argument limit
|
Sandchest reviewed Sandchest reviews every change for vulnerabilities an attacker could use · 14 credits · Review details |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| lock: previous_lock, | ||
| subscription: previous_subscription, | ||
| } = previous; | ||
| let _ = previous_subscription.retire(); |
There was a problem hiding this comment.
On Windows, previous_subscription.retire() cancels the same cancel token used by the running source and the new subscription. The switch can report success, but the source then exits instead of recording the new microphone. Give each subscription its own cancellation token, separate from the source’s lifetime.
Prompt To Fix With AI
This is a comment left during a code review.
Path: crates/recording/src/sources/microphone.rs
Line: 654
Comment:
**Switching cuts Windows microphone audio**
On Windows, `previous_subscription.retire()` cancels the same `cancel` token used by the running source and the new subscription. The switch can report success, but the source then exits instead of recording the new microphone. Give each subscription its own cancellation token, separate from the source’s lifetime.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| self.fitted_height = height; | ||
| let Some(native) = crate::platform::native_window(window) else { | ||
| return; | ||
| }; | ||
| let scale = window.scale_factor(); | ||
| cx.spawn(async move |_, _| { | ||
| crate::app_windows::fit_controls_to_issue(&native, issue, scale); | ||
| crate::app_windows::fit_controls(&native, height, scale); |
There was a problem hiding this comment.
On Linux, native_window() returns None, so fit_window() returns without resizing. fit_controls() also has no Linux resize path. The window stays 150 pixels tall, but even the smallest settings panel plus the bar needs 182 pixels. Device lists need still more room, leaving menu rows clipped or unreachable. Resize the GPUI window on Linux when the panel opens.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/controls_window.rs
Line: 193-199
Comment:
**Linux settings menu gets clipped**
On Linux, `native_window()` returns `None`, so `fit_window()` returns without resizing. `fit_controls()` also has no Linux resize path. The window stays 150 pixels tall, but even the smallest settings panel plus the bar needs 182 pixels. Device lists need still more room, leaving menu rows clipped or unreachable. Resize the GPUI window on Linux when the panel opens.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| pub fn switch_input(&mut self, request: InputRequest, cx: &mut Context<Self>) { | ||
| let available = match request.kind() { | ||
| InputKind::Microphone => self.microphone_switch_available(), | ||
| InputKind::Camera => self.camera_switch_available(), | ||
| }; | ||
| if !available { | ||
| return; | ||
| } | ||
| if let Some(request) = self.input_switch.submit(request) { |
There was a problem hiding this comment.
Input switching and pause commands do not share a guard. switch_input() accepts requests while pause or resume is pending, and the pause shortcut calls toggle_pause() without checking input_switch_pending().
For example, pressing Pause while a Studio switch opens a device pauses the already-paused actor and marks the controls paused. The switch then calls resume() because its saved was_paused is false. Capture runs while the controls say it is paused. Block or queue both directions through the same session-level guard.
Knowledge Base Used: Desktop recording orchestration
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/session/input_switch.rs
Line: 143-151
Comment:
**Paused controls hide running capture**
Input switching and pause commands do not share a guard. `switch_input()` accepts requests while pause or resume is pending, and the pause shortcut calls `toggle_pause()` without checking `input_switch_pending()`.
For example, pressing Pause while a Studio switch opens a device pauses the already-paused actor and marks the controls paused. The switch then calls `resume()` because its saved `was_paused` is false. Capture runs while the controls say it is paused. Block or queue both directions through the same session-level guard.
**Knowledge Base Used:** [Desktop recording orchestration](https://app.greptile.com/-/switch-tenant?tenant=cap&next=%2Fcustom-context%2Fknowledge-base%2Fcapsoftware%2Fcap%2F-%2Fdocs%2Fdesktop-recording-orchestration.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| /// Rows under each heading, "None" included; a heading whose input this | ||
| /// recording cannot change shows no rows. |
There was a problem hiding this comment.
The comment above settings_rows() only describes the rows the function counts. AGENTS.md requires comments to explain non-obvious context and explicitly bans comments that narrate what code does. Remove this comment and the same pattern above microphone_choices() to satisfy the repository requirement before merging.
Context Used: AGENTS.md (source)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop-gpui/src/controls_window.rs
Line: 688-689
Comment:
**New comments repeat the code**
The comment above `settings_rows()` only describes the rows the function counts. `AGENTS.md` requires comments to explain non-obvious context and explicitly bans comments that narrate what code does. Remove this comment and the same pattern above `microphone_choices()` to satisfy the repository requirement before merging.
**Context Used:** AGENTS.md ([source](https://github.com/capsoftware/cap/blob/main/AGENTS.md))
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![capy-ai[bot]](https://avatars.githubusercontent.com/in/1915919?s=60&v=4){kind=small}
| lock: previous_lock, | ||
| subscription: previous_subscription, | ||
| } = previous; | ||
| let _ = previous_subscription.retire(); |
There was a problem hiding this comment.
Windows microphone switching cancels the running audio source
High Severity
On Windows, the original subscription and the bridge both hold clones of source_cancel, and the replacement subscription receives another clone of that same token. RecordingSubscription::retire() calls self.cancel.cancel(), so retiring the previous subscription here cancels the bridge and invalidates the replacement subscription too. The handoff can still reply successfully, but microphone forwarding stops instead of continuing on the selected device. Give each subscription an independently cancellable child token so retiring one cannot cancel the source or its replacement.
| }; | ||
| } | ||
| let resume = handle.clone(); | ||
| match gpui_tokio::Tokio::spawn_result(cx, async move { resume.resume().await }).await { |
There was a problem hiding this comment.
A pause hotkey during switching can leave capture running as paused
High Severity
The existing RecordingSession::toggle_pause hotkey path does not consult pause_unavailable(). Start a Studio device switch while recording, then press the pause hotkey while the replacement device opens: the switch has already paused the actor, so the hotkey's pause succeeds and marks the session paused. This code subsequently resumes capture because its saved was_paused is false, and the completion callback does not apply paused: Some(false). Capture therefore runs while the bar displays a paused state. Serialize input switches and pause/resume requests in both directions rather than relying only on disabled bar buttons.
| ) = this.update(cx, |_, cx| { | ||
| Feeds::global(cx).update(cx, |feeds, cx| match selection { | ||
| InputRequest::Microphone(label) => { | ||
| feeds.set_microphone(label, cx); |
There was a problem hiding this comment.
Device switches leave restart using the original input configuration
Medium Severity
RecordingSession::restart still clones last_config, but this path changes the app feed without updating that saved configuration. Start Studio with microphone A, switch successfully to B, then choose Restart: the old recording is discarded, and startup asks the same feed actor for a lock expecting A. The actor now returns B, so start_attempt_with_upload rejects it with the selected-microphone mismatch error and the replacement recording never starts. Camera switching has the equivalent mismatch; Instant switches can reopen the old microphone instead. Refresh the saved input identities, settings, readiness and feed references after successful switches, or rebuild those fields from current selections before restarting.
The recording bar's settings button was drawn disabled, and the GPUI app had no way to change the microphone or camera once a recording started. This brings back the Tauri app's recording settings menu (
in-progress-recording.tsx:737-830): show or hide the camera preview, and switch microphone and camera mid-recording, in both Studio and Instant.How a switch works
flowchart TD A[Pick a device in the bar's settings menu] --> M{Mode} M -->|Studio| S1[Pause if running: the segment closes] S1 --> S2[Detach the old feed from the segment factory] S2 --> S3[Point the app feed at the new device, wait until it is open, lock it] S3 -->|fails to open| S5[Reopen and reattach the previous device, show the error] S3 --> S4[Attach the new lock, resume if it was running] S5 --> S4b[Resume if it was running] M -->|Instant mic| I1[Open the new mic on a feed of its own] I1 -->|fails to open| I4[Nothing touched the recording, show the error] I1 --> I2[MicrophoneSwitch moves the live source onto it] I2 --> I3[Old feed released, app selection updated] M -->|Instant camera| C1[New preview selection: the bubble is what Instant records]set_mic_feed/set_camera_feed(which only work while paused). The time the cut takes counts as paused, so the bar's timer matches what was recorded. A reopened camera bubble is a new window, soadd_excluded_windows(new, macOS) keeps it out of the next segments.MicrophoneFeedrefuses a different device, so the switch happens at the source instead:MicrophoneSwitch(crates/recording/src/sources/microphone.rs) attaches the running source to another, already-open feed and drops the old one. The source already rebuilds its resampler whenever the incoming rate, channels or format change, so a 44.1 kHz stereo headset after a 48 kHz mono built-in mic needs nothing extra. The mute flag stays on the original lock, which the bar keeps using.The bar
fit_controls). Rows show a check on the device in use; "Switching…" shows on the heading while a switch runs. Clicking elsewhere or picking a device closes it.Edge cases
Tests
sources::microphone::switch_tests: a running source moves from a 48 kHz mono feed to a 44.1 kHz stereo one, the old feed is released (it can be locked again) and its samples never reach the recording; a switch onto a dead feed fails and recording continues on the current mic.session::input_switch::tests: rapid switches collapse to the newest choice; a camera request waiting behind a mic switch is never dropped.controls_windowtests for the window height with the issue card and panel, and the panel's height cap.deeplinkparse test for the debug-onlyswitch_microphoneaction used to script hardware runs.crates/recording/examples/av-sync-record.rsgains--mic,--switch-toand--switch-atfor the flash+click sync harness.Callers checked
Microphone::setup_with_channels: Studio and Instant pipelines, the Linux X11 combined source, the feed tests. All but the Instant screen pipeline pass no switch.create_pipeline(instant): one caller. Camera-only Instant gets no switch.fit_controls_to_issuereplaced byfit_controls; only the controls window called it.Feeds, and the main window's existing observer persists the selection, so the next recording uses the new device.Proof
Pending (hardware runs on the Mac mini are blocked on a microphone permission grant); this section will carry the A/V sync measurements against
mainand screenshots/recording of the menu.Fix the Windows audio cutoff, clipped Linux menu, and pause race before merging, and satisfy the repository’s comment rule.
Findings
Fix with agent prompt
Summary
Adds a recording settings menu with camera preview visibility and mid-recording device switching.
Reviews (1) · Last reviewed commit: "fix(recording): build the microphone swi..." · Reviewed by Greptile