Skip to content

Comments

Deeplinks: pause/resume/restart recording + switch mic/camera#1621

Open
wh0amibjm wants to merge 5 commits intoCapSoftware:mainfrom
wh0amibjm:algora-1540-deeplink-actions
Open

Deeplinks: pause/resume/restart recording + switch mic/camera#1621
wh0amibjm wants to merge 5 commits intoCapSoftware:mainfrom
wh0amibjm:algora-1540-deeplink-actions

Conversation

@wh0amibjm
Copy link

@wh0amibjm wh0amibjm commented Feb 20, 2026

Follow-up implementation for #1540.

Proposed Changes

  • Add new deeplink actions to the desktop app (cap-desktop://action?value=... legacy JSON/string format):
    • PauseRecording, ResumeRecording, TogglePauseRecording, RestartRecording
    • SwitchMicrophone (by label)
    • SwitchCamera
  • Align semantics with existing recording APIs:
    • pause/resume/toggle return "Recording not in progress" when no active recording exists
    • restart maps the underlying restart_recording() -> Result<RecordingAction, String> into Result<(), String> to match the deeplink executor return type.

Proof

  • Added unit coverage for deeplink parsing in apps/desktop/src-tauri/src/deeplink_actions.rs:
    • parse PauseRecording
    • parse SwitchMicrophone payload decoding
    • reject non-action domains (cap-desktop://signin?...)

Local note: running the full cap-desktop test target on this machine is currently blocked by missing full Xcode toolchain (xcodebuild), but the change is isolated to deeplink action parsing/execution and follows existing recording::* APIs.

Checklist

  • PR targets the correct repo/branch
  • Backwards compatibility preserved for existing legacy action deeplinks
  • Unit tests added for parsing behavior

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

1 file reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

crate::recording::toggle_pause_recording(app.clone(), app.state()).await
}
DeepLinkAction::RestartRecording => {
crate::recording::restart_recording(app.clone(), app.state()).await
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

restart_recording returns Result<RecordingAction, String> but this match arm must return Result<(), String>. The existing StartRecording action (line 152-154) and hotkeys.rs:156-158 both use .map(|_| ()) to convert the return type.

Suggested change
crate::recording::restart_recording(app.clone(), app.state()).await
crate::recording::restart_recording(app.clone(), app.state()).await.map(|_| ())
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/deeplink_actions.rs
Line: 169

Comment:
`restart_recording` returns `Result<RecordingAction, String>` but this match arm must return `Result<(), String>`. The existing `StartRecording` action (line 152-154) and hotkeys.rs:156-158 both use `.map(|_| ())` to convert the return type.

```suggestion
                crate::recording::restart_recording(app.clone(), app.state()).await.map(|_| ())
```

How can I resolve this? If you propose a fix, please make it concise.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in commit e02ac71RestartRecording now maps Result<RecordingAction, String> to Result<(), String> with .map(|_| ()).

.map(|_| ())
}
DeepLinkAction::SwitchMicrophone { mic_label } => {
crate::set_mic_input(app.state(), mic_label).await
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: it’s a bit easier to follow (and a little less inference-dependent) if this matches the StartRecording pattern and makes the state type explicit.

Suggested change
crate::set_mic_input(app.state(), mic_label).await
crate::set_mic_input(app.state::<ArcLock<App>>(), mic_label).await

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 7bd37b3: SwitchMicrophone now treats empty/whitespace labels as unset (None) and uses explicit app.state::<ArcLock<App>>().

Comment on lines 159 to 161
DeepLinkAction::PauseRecording => {
crate::recording::pause_recording(app.clone(), app.state()).await
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For deeplinks, do you want pause/resume/toggle to behave like StopRecording and error when there’s no active recording? Right now these will succeed silently when nothing is recording.

Suggested change
DeepLinkAction::PauseRecording => {
crate::recording::pause_recording(app.clone(), app.state()).await
}
DeepLinkAction::PauseRecording => {
let state = app.state::<ArcLock<App>>();
if state.read().await.current_recording().is_none() {
return Err("Recording not in progress".to_string());
}
crate::recording::pause_recording(app.clone(), state).await
}

.map(|_| ())
}
DeepLinkAction::SwitchMicrophone { mic_label } => {
crate::set_mic_input(app.state(), mic_label).await
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small robustness thing: treat empty mic labels as None so callers that send "" don’t end up trying to select a non-existent device.

Suggested change
crate::set_mic_input(app.state(), mic_label).await
crate::set_mic_input(app.state(), mic_label.filter(|label| !label.trim().is_empty())).await

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 7bd37b3: SwitchMicrophone now treats empty/whitespace labels as unset (None) and uses explicit app.state::<ArcLock<App>>().

@wh0amibjm
Copy link
Author

Correction (escaped markdown got mangled in previous comment):

Follow-up pushed in cf8787f to address the deeplink behavior concern for pause/resume/toggle:

  • PauseRecording, ResumeRecording, and TogglePauseRecording now return "Recording not in progress" when there is no active recording (mirrors StopRecording semantics).
  • Kept the actual recording command calls unchanged once an active recording exists.

Validation attempt:

  • Tried running desktop deeplink tests locally.
  • Blocked on local toolchain: cidre build requires full Xcode (xcodebuild), while this machine currently has Command Line Tools only.

@wh0amibjm
Copy link
Author

Pushed follow-up b6264a0 to tighten deeplink parsing safety with unit coverage:

  • added parsing tests for:
    • PauseRecording
    • SwitchMicrophone payload decoding
    • rejecting non-action domains (cap-desktop://signin?...)

Validation note:

  • attempted cargo test -p cap-desktop deeplink_actions::tests
  • still blocked in this environment by full macOS toolchain requirements (xcodebuild from full Xcode + ffmpeg/cidre headers/tooling).

.await
}
DeepLinkAction::SwitchCamera { camera } => {
crate::set_camera_input(app.clone(), app.state(), camera, None).await
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor robustness: mirror the mic label handling and treat an empty DeviceID("") as unset so it doesn't trigger 3 init retries.

Suggested change
crate::set_camera_input(app.clone(), app.state(), camera, None).await
let camera = camera.filter(|id| match id {
DeviceOrModelID::DeviceID(device_id) => !device_id.trim().is_empty(),
DeviceOrModelID::ModelID(_) => true,
});
crate::set_camera_input(app.clone(), app.state(), camera, None).await

let error = DeepLinkAction::try_from(&url).expect_err("signin deeplink is not action");
assert!(matches!(error, ActionParseFromUrlError::NotAction));
}
}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice to have: quick parse smoke tests for the other new actions, just to guard the serde rename plumbing.

Suggested change
}
#[test]
fn parses_restart_recording_action() {
let url = action_url(r#""restart_recording""#);
let action = DeepLinkAction::try_from(&url).expect("parse restart action");
assert!(matches!(action, DeepLinkAction::RestartRecording));
}
#[test]
fn parses_switch_camera_action() {
let url = action_url(r#"{"switch_camera":{"camera":{"DeviceID":"camera-1"}}}"#);
let action = DeepLinkAction::try_from(&url).expect("parse switch camera action");
match action {
DeepLinkAction::SwitchCamera { camera } => {
assert!(camera.is_some());
}
other => panic!("unexpected action: {other:?}"),
}
}
}

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants