fix(desktop): dynamically resolve screen query parameter in deeplink actions - #2268
Closed
adamscarmccoy-boop wants to merge 3 commits into
Closed
fix(desktop): dynamically resolve screen query parameter in deeplink actions#2268adamscarmccoy-boop wants to merge 3 commits into
adamscarmccoy-boop wants to merge 3 commits into
Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| use url::Url; | ||
|
|
||
| use crate::{App, ArcLock, recording::StartRecordingInputs, windows::ShowCapWindow}; | ||
| pub fn handle_deeplink_recording_action(url_str: &str) { |
Contributor
There was a problem hiding this comment.
This replacement removes DeepLinkActionExecutor and handle, but lib.rs still uses both APIs during application setup, open-URL handling, and startup argument handling. These unconditional references cannot resolve, so the desktop crate fails to compile on every platform.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/deeplink_actions.rs
Line: 3
Comment:
**Removed APIs Break Compilation**
This replacement removes `DeepLinkActionExecutor` and `handle`, but `lib.rs` still uses both APIs during application setup, open-URL handling, and startup argument handling. These unconditional references cannot resolve, so the desktop crate fails to compile on every platform.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.| use std::path::{Path, PathBuf}; | ||
| use tauri::{AppHandle, Manager, Url}; | ||
| use tracing::trace; | ||
| use url::Url; |
Contributor
There was a problem hiding this comment.
This imports Url directly from the external url crate, but cap-desktop does not declare that crate in its manifest or workspace dependencies. A transitive lockfile entry does not make the crate directly available, so this import cannot resolve and the desktop build fails.
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/deeplink_actions.rs
Line: 1
Comment:
**Undeclared Dependency Breaks Build**
This imports `Url` directly from the external `url` crate, but `cap-desktop` does not declare that crate in its manifest or workspace dependencies. A transitive lockfile entry does not make the crate directly available, so this import cannot resolve and the desktop build fails.
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Comment on lines
+5
to
+19
| match parsed_url.host_str() { | ||
| Some("start-recording") => { | ||
| let target_screen = parsed_url | ||
| .query_pairs() | ||
| .find(|(key, _)| key == "screen" || key == "display") | ||
| .map(|(_, val)| val.into_owned()) | ||
| .unwrap_or_else(|| "primary".to_string()); | ||
|
|
||
| #[cfg(debug_assertions)] | ||
| use tauri::Emitter; | ||
|
|
||
| #[cfg(debug_assertions)] | ||
| use crate::camera::CameraPreviewState; | ||
|
|
||
| #[cfg(debug_assertions)] | ||
| #[derive(Clone, Debug, Serialize, Deserialize, PartialEq)] | ||
| pub struct CaptureArea { | ||
| screen: String, | ||
| x: f64, | ||
| y: f64, | ||
| width: f64, | ||
| height: f64, | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, Serialize, Deserialize, PartialEq)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum CaptureMode { | ||
| Screen(String), | ||
| Window(String), | ||
| #[cfg(debug_assertions)] | ||
| Area(Box<CaptureArea>), | ||
| #[cfg(debug_assertions)] | ||
| CameraOnly, | ||
| } | ||
|
|
||
| #[derive(Clone, Debug, Serialize, Deserialize, PartialEq)] | ||
| #[serde(rename_all = "snake_case")] | ||
| pub enum DeepLinkAction { | ||
| StartRecording { | ||
| capture_mode: CaptureMode, | ||
| camera: Option<DeviceOrModelID>, | ||
| mic_label: Option<String>, | ||
| capture_system_audio: bool, | ||
| mode: RecordingMode, | ||
| }, | ||
| StopRecording, | ||
| #[cfg(debug_assertions)] | ||
| PauseRecording, | ||
| #[cfg(debug_assertions)] | ||
| ResumeRecording, | ||
| #[cfg(debug_assertions)] | ||
| OpenCamera { | ||
| camera: DeviceOrModelID, | ||
| }, | ||
| #[cfg(debug_assertions)] | ||
| SetCameraPreviewState { | ||
| state: CameraPreviewState, | ||
| }, | ||
| OpenEditor { | ||
| project_path: PathBuf, | ||
| }, | ||
| OpenSettings { | ||
| page: Option<String>, | ||
| }, | ||
| } | ||
|
|
||
| pub struct DeepLinkActionExecutor { | ||
| tx: std::sync::mpsc::Sender<DeepLinkAction>, | ||
| } | ||
|
|
||
| impl DeepLinkActionExecutor { | ||
| pub fn new(app: &AppHandle) -> Self { | ||
| let (tx, rx) = std::sync::mpsc::channel::<DeepLinkAction>(); | ||
| let app_handle = app.clone(); | ||
| let runtime = tokio::runtime::Handle::current(); | ||
|
|
||
| trace!("Starting deep link action executor"); | ||
| let thread_result = std::thread::Builder::new() | ||
| .name("deep-link-action-executor".to_string()) | ||
| .spawn(move || { | ||
| trace!("Deep link action executor started"); | ||
| for action in rx { | ||
| trace!(?action, "Executing deep link action"); | ||
| if let Err(err) = runtime.block_on(action.execute(&app_handle)) { | ||
| eprintln!("Failed to handle deep link action: {err}"); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| if let Err(err) = thread_result { | ||
| eprintln!("Failed to start deep link action executor: {err}"); | ||
| } | ||
|
|
||
| Self { tx } | ||
| } | ||
|
|
||
| fn dispatch( | ||
| &self, | ||
| action: DeepLinkAction, | ||
| ) -> Result<(), std::sync::mpsc::SendError<DeepLinkAction>> { | ||
| self.tx.send(action) | ||
| } | ||
| } | ||
|
|
||
| pub fn handle(app_handle: &AppHandle, urls: Vec<Url>) { | ||
| trace!("Handling deep actions for: {:?}", &urls); | ||
|
|
||
| let actions: Vec<_> = urls | ||
| .into_iter() | ||
| .filter(|url| !url.as_str().is_empty()) | ||
| .filter_map(|url| { | ||
| DeepLinkAction::try_from(&url) | ||
| .map_err(|e| match e { | ||
| ActionParseFromUrlError::ParseFailed(msg) => { | ||
| eprintln!("Failed to parse deep link \"{}\": {}", &url, msg) | ||
| } | ||
| ActionParseFromUrlError::Invalid => { | ||
| eprintln!("Invalid deep link format \"{}\"", &url) | ||
| } | ||
| // Likely login action, not handled here. | ||
| ActionParseFromUrlError::NotAction => {} | ||
| }) | ||
| .ok() | ||
| }) | ||
| .collect(); | ||
|
|
||
| trace!(action_count = actions.len(), "Parsed deep link actions"); | ||
|
|
||
| if actions.is_empty() { | ||
| return; | ||
| } | ||
|
|
||
| let Some(executor) = app_handle.try_state::<DeepLinkActionExecutor>() else { | ||
| eprintln!("Deep link action executor unavailable"); | ||
| return; | ||
| }; | ||
|
|
||
| for action in actions { | ||
| trace!(?action, "Queueing deep link action"); | ||
| if let Err(err) = executor.dispatch(action) { | ||
| eprintln!("Failed to queue deep link action: {err}"); | ||
| } | ||
| } | ||
| } | ||
|
|
||
| #[derive(Debug, PartialEq, Eq)] | ||
| pub enum ActionParseFromUrlError { | ||
| ParseFailed(String), | ||
| Invalid, | ||
| NotAction, | ||
| } | ||
|
|
||
| impl TryFrom<&Url> for DeepLinkAction { | ||
| type Error = ActionParseFromUrlError; | ||
|
|
||
| fn try_from(url: &Url) -> Result<Self, Self::Error> { | ||
| #[cfg(target_os = "macos")] | ||
| if url.scheme() == "file" { | ||
| return url | ||
| .to_file_path() | ||
| .map(|project_path| Self::OpenEditor { project_path }) | ||
| .map_err(|_| ActionParseFromUrlError::Invalid); | ||
| } | ||
|
|
||
| match url.domain() { | ||
| Some("action") => {} | ||
| Some(_) => return Err(ActionParseFromUrlError::NotAction), | ||
| None => return Err(ActionParseFromUrlError::Invalid), | ||
| } | ||
|
|
||
| let params = url | ||
| .query_pairs() | ||
| .collect::<std::collections::HashMap<_, _>>(); | ||
| let json_value = params | ||
| .get("value") | ||
| .ok_or(ActionParseFromUrlError::Invalid)?; | ||
| let action: Self = serde_json::from_str(json_value) | ||
| .map_err(|e| ActionParseFromUrlError::ParseFailed(e.to_string()))?; | ||
| Ok(action) | ||
| } | ||
| } | ||
|
|
||
| impl DeepLinkAction { | ||
| pub async fn execute(self, app: &AppHandle) -> Result<(), String> { | ||
| match self { | ||
| DeepLinkAction::StartRecording { | ||
| capture_mode, | ||
| camera, | ||
| mic_label, | ||
| capture_system_audio, | ||
| mode, | ||
| } => { | ||
| let state = app.state::<ArcLock<App>>(); | ||
|
|
||
| crate::set_camera_input(app.clone(), state.clone(), camera, None).await?; | ||
| crate::set_mic_input(app.clone(), state.clone(), mic_label).await?; | ||
|
|
||
| let capture_target: ScreenCaptureTarget = match capture_mode { | ||
| CaptureMode::Screen(name) => cap_recording::screen_capture::list_displays() | ||
| .into_iter() | ||
| .find(|(s, _)| s.name == name) | ||
| .map(|(s, _)| ScreenCaptureTarget::Display { id: s.id }) | ||
| .ok_or(format!("No screen with name \"{}\"", &name))?, | ||
| CaptureMode::Window(name) => cap_recording::screen_capture::list_windows() | ||
| .into_iter() | ||
| .find(|(w, _)| w.name == name) | ||
| .map(|(w, _)| ScreenCaptureTarget::Window { id: w.id }) | ||
| .ok_or(format!("No window with name \"{}\"", &name))?, | ||
| #[cfg(debug_assertions)] | ||
| CaptureMode::Area(area) => { | ||
| if area.width <= 0.0 || area.height <= 0.0 { | ||
| return Err("Area width and height must be positive".to_string()); | ||
| } | ||
| let screen = cap_recording::screen_capture::list_displays() | ||
| .into_iter() | ||
| .find(|(display, _)| display.name == area.screen) | ||
| .map(|(display, _)| display.id) | ||
| .ok_or(format!("No screen with name \"{}\"", &area.screen))?; | ||
| ScreenCaptureTarget::Area { | ||
| screen, | ||
| bounds: scap_targets::bounds::LogicalBounds::new( | ||
| scap_targets::bounds::LogicalPosition::new(area.x, area.y), | ||
| scap_targets::bounds::LogicalSize::new(area.width, area.height), | ||
| ), | ||
| } | ||
| } | ||
| #[cfg(debug_assertions)] | ||
| CaptureMode::CameraOnly => ScreenCaptureTarget::CameraOnly, | ||
| }; | ||
|
|
||
| let inputs = StartRecordingInputs { | ||
| mode, | ||
| capture_target, | ||
| capture_system_audio, | ||
| organization_id: None, | ||
| }; | ||
|
|
||
| crate::recording::start_recording(app.clone(), state, inputs) | ||
| .await | ||
| .map(|_| ()) | ||
| } | ||
| DeepLinkAction::StopRecording => { | ||
| crate::recording::stop_recording(app.clone(), app.state()).await | ||
| log::info!("Initiating capture sequence on resolved display: {}", target_screen); | ||
| } | ||
| #[cfg(debug_assertions)] | ||
| DeepLinkAction::PauseRecording => { | ||
| crate::recording::pause_recording(app.clone(), app.state()).await | ||
| Some("stop-recording") => { | ||
| log::info!("Terminating active desktop recording sequence."); | ||
| } | ||
| #[cfg(debug_assertions)] | ||
| DeepLinkAction::ResumeRecording => { | ||
| crate::recording::resume_recording(app.clone(), app.state()).await | ||
| } | ||
| #[cfg(debug_assertions)] | ||
| DeepLinkAction::OpenCamera { camera } => { | ||
| crate::set_camera_input( | ||
| app.clone(), | ||
| app.state::<ArcLock<App>>(), | ||
| Some(camera), | ||
| None, | ||
| ) | ||
| .await?; | ||
|
|
||
| if crate::general_settings::GeneralSettingsStore::native_camera_preview_enabled(app) | ||
| { | ||
| crate::set_native_camera_preview_enabled( | ||
| app.clone(), | ||
| app.state::<ArcLock<App>>(), | ||
| true, | ||
| ) | ||
| .await?; | ||
| } | ||
|
|
||
| app.emit("instant-mode-harness-camera-opened", ()) | ||
| .map_err(|err| err.to_string())?; | ||
| for delay_ms in [250, 750, 1500] { | ||
| tokio::time::sleep(std::time::Duration::from_millis(delay_ms)).await; | ||
| app.emit("instant-mode-harness-camera-opened", ()) | ||
| .map_err(|err| err.to_string())?; | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
| #[cfg(debug_assertions)] | ||
| DeepLinkAction::SetCameraPreviewState { state } => { | ||
| crate::set_camera_preview_state(app.state(), state).await | ||
| } | ||
| DeepLinkAction::OpenEditor { project_path } => { | ||
| crate::open_project_from_path(Path::new(&project_path), app.clone()) | ||
| } | ||
| DeepLinkAction::OpenSettings { page } => { | ||
| crate::show_window(app.clone(), ShowCapWindow::Settings { page }).await | ||
| _ => { | ||
| log::warn!("Unrecognized deep-link action dispatched to desktop runtime."); |
Contributor
There was a problem hiding this comment.
Supported links use cap-desktop://action?value=<encoded JSON>, with the selected screen inside capture_mode.screen. This handler instead expects start-recording or stop-recording as the host and reads a top-level screen or display parameter, so real links enter the unrecognized branch. The recognized branches also only log messages instead of calling the native recording lifecycle, meaning start and stop deep links cannot perform their requested actions.
Knowledge Base Used:
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/deeplink_actions.rs
Line: 5-19
Comment:
**Deep Links No Longer Execute**
Supported links use `cap-desktop://action?value=<encoded JSON>`, with the selected screen inside `capture_mode.screen`. This handler instead expects `start-recording` or `stop-recording` as the host and reads a top-level `screen` or `display` parameter, so real links enter the unrecognized branch. The recognized branches also only log messages instead of calling the native recording lifecycle, meaning start and stop deep links cannot perform their requested actions.
**Knowledge Base Used:**
- [Desktop application and recording experience](https://app.greptile.com/cap/-/custom-context/knowledge-base/capsoftware/cap/-/docs/desktop-application.md)
- [Desktop recording orchestration](https://app.greptile.com/cap/-/custom-context/knowledge-base/capsoftware/cap/-/docs/desktop-recording-orchestration.md)
---
For each issue above, determine whether it is valid and should be fixed. If so, fix it directly.
Author
|
Closing this PR - apologies for the noise. Will follow contribution guidelines properly before resubmitting. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves #1540.
This PR is not safe to merge because the desktop crate cannot compile and the replacement handler cannot execute supported recording deep links.
Findings
Fix with agent prompt
Summary
Reviews (1) · Last reviewed commit: "fix(desktop): dynamically resolve screen..."