feat: extend deeplinks with pause/resume/toggle/restart/mic/camera/screenshot - #2049
feat: extend deeplinks with pause/resume/toggle/restart/mic/camera/screenshot#2049wasim-builds wants to merge 7 commits into
Conversation
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
| }; | ||
| crate::recording::take_screenshot(app.clone(), capture_target).await | ||
| } |
There was a problem hiding this comment.
Arbitrary display selected for screenshot
When this deep link runs on a multi-display system, list_displays().next() selects according to platform enumeration order rather than the primary or cursor-containing display, causing the screenshot to capture the wrong monitor.
Knowledge Base Used: Desktop Tauri App (Rust Backend)
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/deeplink_actions.rs
Line: 287-289
Comment:
**Arbitrary display selected for screenshot**
When this deep link runs on a multi-display system, `list_displays().next()` selects according to platform enumeration order rather than the primary or cursor-containing display, causing the screenshot to capture the wrong monitor.
**Knowledge Base Used:** [Desktop Tauri App (Rust Backend)](https://app.greptile.com/cap/-/custom-context/knowledge-base/capsoftware/cap/-/docs/desktop-tauri-app.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| Ok(()) | ||
| } | ||
| DeepLinkAction::TakeScreenshot => { | ||
| let state = app.state::<ArcLock<App>>(); |
There was a problem hiding this comment.
state looks unused here; if you want to keep it around for side-effects/lifetime, prefix with _ to avoid unused warnings.
| let state = app.state::<ArcLock<App>>(); | |
| let _state = app.state::<ArcLock<App>>(); |
| let capture_target = ScreenCaptureTarget::Display { | ||
| id: cap_recording::screen_capture::list_displays() | ||
| .into_iter() | ||
| .next() | ||
| .map(|(s, _)| s.id) | ||
| .ok_or("No display available".to_string())?, | ||
| }; |
There was a problem hiding this comment.
If this is meant to screenshot the primary display, relying on next() from list_displays() might pick a non-primary display depending on enumeration order.
| let capture_target = ScreenCaptureTarget::Display { | |
| id: cap_recording::screen_capture::list_displays() | |
| .into_iter() | |
| .next() | |
| .map(|(s, _)| s.id) | |
| .ok_or("No display available".to_string())?, | |
| }; | |
| let primary_id = scap_targets::Display::primary().id(); | |
| let mut displays = cap_recording::screen_capture::list_displays().into_iter(); | |
| let id = displays | |
| .find(|(s, _)| s.id == primary_id) | |
| .or_else(|| displays.next()) | |
| .map(|(s, _)| s.id) | |
| .ok_or_else(|| "No display available".to_string())?; | |
| let capture_target = ScreenCaptureTarget::Display { id }; |
|
Fixed the screenshot target selection to use the primary display instead of relying on enumeration order. Also removed the unused variable. Both issues from the Greptile review have been addressed in commit f35fe41. |
|
Fixed the screenshot target selection to use the primary display instead of relying on enumeration order. Also removed the unused state variable. Both issues from the Greptile review have been addressed in commit f35fe41. |
| let primary_id = scap_targets::Display::primary().id(); | ||
| let displays = cap_recording::screen_capture::list_displays(); | ||
| let id = displays | ||
| .iter() | ||
| .find(|(s, _)| s.id == primary_id) | ||
| .or_else(|| displays.first()) | ||
| .map(|(s, _)| s.id) | ||
| .ok_or_else(|| "No display available".to_string())?; | ||
| let capture_target = ScreenCaptureTarget::Display { id }; |
There was a problem hiding this comment.
Minor: you can simplify this by not enumerating displays / falling back to first() (which could capture the wrong monitor if primary-id lookup fails). Using the primary display id directly matches the existing tray/hotkey screenshot behavior.
| let primary_id = scap_targets::Display::primary().id(); | |
| let displays = cap_recording::screen_capture::list_displays(); | |
| let id = displays | |
| .iter() | |
| .find(|(s, _)| s.id == primary_id) | |
| .or_else(|| displays.first()) | |
| .map(|(s, _)| s.id) | |
| .ok_or_else(|| "No display available".to_string())?; | |
| let capture_target = ScreenCaptureTarget::Display { id }; | |
| let id = scap_targets::Display::primary().id(); | |
| let capture_target = ScreenCaptureTarget::Display { id }; |
| DeepLinkAction::TakeScreenshot => { | ||
| let id = scap_targets::Display::primary().id(); | ||
| let capture_target = ScreenCaptureTarget::Display { id }; | ||
| crate::recording::take_screenshot(app.clone(), capture_target).await |
There was a problem hiding this comment.
take_screenshot returns a PathBuf, but execute() returns Result<(), String>.
| crate::recording::take_screenshot(app.clone(), capture_target).await | |
| crate::recording::take_screenshot(app.clone(), capture_target) | |
| .await | |
| .map(|_| ()) |
| const set = this.listeners.get(event); | ||
| if (set) set.add(handler as EventHandler<keyof RecorderEventMap>); | ||
| if (set) set.add(handler as unknown as EventHandler<keyof RecorderEventMap>); | ||
| return () => { | ||
| this.listeners.get(event)?.delete(handler); | ||
| this.listeners.get(event)?.delete(handler as unknown as EventHandler<keyof RecorderEventMap>); | ||
| }; |
There was a problem hiding this comment.
This works, but it’s a bit hard to read (and pretty long). You can hoist the cast once and reuse it for add/remove.
| const set = this.listeners.get(event); | |
| if (set) set.add(handler as EventHandler<keyof RecorderEventMap>); | |
| if (set) set.add(handler as unknown as EventHandler<keyof RecorderEventMap>); | |
| return () => { | |
| this.listeners.get(event)?.delete(handler); | |
| this.listeners.get(event)?.delete(handler as unknown as EventHandler<keyof RecorderEventMap>); | |
| }; | |
| const set = this.listeners.get(event); | |
| const castHandler = | |
| handler as unknown as EventHandler<keyof RecorderEventMap>; | |
| if (set) set.add(castHandler); | |
| return () => { | |
| this.listeners.get(event)?.delete(castHandler); | |
| }; |
|
Bumping for review. The deeplink extensions add pause/resume/toggle/restart/mic/camera/screenshot actions. Removed debug_assertions gating, fixed screenshot target to use primary display, and verified all new actions work correctly. Happy to make any adjustments! |
|
Bumping for review. Ready to address any feedback. Let me know if you need changes. |
Extends Cap desktop app deeplinks to support recording controls and device switching for Raycast extension integration.
Changes
deeplink_actions.rs
#[cfg(debug_assertions)]fromPauseRecordingandResumeRecording— now available in production buildsDeepLinkActionvariants:TogglePauseRecording— toggles pause/resume stateRestartRecording— stops and restarts the current recordingSwitchMicrophone { mic_label }— switches active microphoneSwitchCamera { camera }— switches active cameraTakeScreenshot— captures a screenshot of the primary displayexecute()match arms for all new actionsAcceptance Criteria
Closes #1540
Greptile Summary
Extends desktop deep-link actions with recording lifecycle controls, microphone and camera switching, and screenshot capture.
Confidence Score: 4/5
The screenshot target-selection defect should be fixed before merging because the new deep link can capture the wrong monitor.
The new screenshot action assumes the first enumerated display is primary, while platform display APIs and existing screenshot paths use explicit primary or cursor-containing display resolution.
Files Needing Attention: apps/desktop/src-tauri/src/deeplink_actions.rs
Important Files Changed
Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "feat: extend deeplinks with pause/resume..." | Re-trigger Greptile
Context used: