fix: implement deterministic pause/resume and optimize deep link handling#1713
fix: implement deterministic pause/resume and optimize deep link handling#1713Angelebeats wants to merge 14 commits intoCapSoftware:mainfrom
Conversation
…urity notifications
…or deep-link security
…command to toggle
… security notifications
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| pub fn send_always(self, app: &tauri::AppHandle) { | ||
| send_notification_always(app, self); | ||
| } |
There was a problem hiding this comment.
send_always bypasses the user's notification preference
send_always sets always = true, short-circuiting the enable_notifications guard at line 110 regardless of user settings. Deep links are commonly triggered programmatically (e.g., Raycast, scripts) at high frequency, so unexpected notifications on every action are especially disruptive to users who have opted out. The notification sound is already suppressed via the skip_sound match on DeepLinkTriggered — send_always is not needed to achieve the PR's stated goal of preventing audio alerts.
| pub fn send_always(self, app: &tauri::AppHandle) { | |
| send_notification_always(app, self); | |
| } | |
| pub fn send_always(self, app: &tauri::AppHandle) { | |
| send_notification(app, self); | |
| } |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/notifications.rs
Line: 96-98
Comment:
**`send_always` bypasses the user's notification preference**
`send_always` sets `always = true`, short-circuiting the `enable_notifications` guard at line 110 regardless of user settings. Deep links are commonly triggered programmatically (e.g., Raycast, scripts) at high frequency, so unexpected notifications on every action are especially disruptive to users who have opted out. The notification sound is already suppressed via the `skip_sound` match on `DeepLinkTriggered` — `send_always` is not needed to achieve the PR's stated goal of preventing audio alerts.
```suggestion
pub fn send_always(self, app: &tauri::AppHandle) {
send_notification(app, self);
}
```
How can I resolve this? If you propose a fix, please make it concise.| match &self { | ||
| DeepLinkAction::StartRecording { .. } | ||
| | DeepLinkAction::StartDefaultRecording | ||
| | DeepLinkAction::StopRecording | ||
| | DeepLinkAction::ResumeRecording | ||
| | DeepLinkAction::PauseRecording | ||
| | DeepLinkAction::TogglePauseRecording => { | ||
| crate::notifications::NotificationType::DeepLinkTriggered.send_always(app); | ||
| } | ||
| _ => {} | ||
| } |
There was a problem hiding this comment.
DeepLinkTriggered.send_always(app) is called unconditionally before the action runs. Both pause_recording and resume_recording return Ok(()) silently when no recording is active, so the user receives "Action Triggered" even though nothing changed. Moving the notification after a confirmed successful result avoids this misleading feedback.
let result = match self {
// ... existing arms ...
};
if result.is_ok() {
crate::notifications::NotificationType::DeepLinkTriggered.send(app);
}
resultPrompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/deeplink_actions.rs
Line: 139-149
Comment:
**Notification fires before action executes — false confirmation on no-op**
`DeepLinkTriggered.send_always(app)` is called unconditionally before the action runs. Both `pause_recording` and `resume_recording` return `Ok(())` silently when no recording is active, so the user receives "Action Triggered" even though nothing changed. Moving the notification after a confirmed successful result avoids this misleading feedback.
```rust
let result = match self {
// ... existing arms ...
};
if result.is_ok() {
crate::notifications::NotificationType::DeepLinkTriggered.send(app);
}
result
```
How can I resolve this? If you propose a fix, please make it concise.| _send_notification(app, notification_type, true); | ||
| } | ||
|
|
||
| fn _send_notification(app: &tauri::AppHandle, notification_type: NotificationType, always: bool) { |
There was a problem hiding this comment.
In Rust, a _ prefix conventionally marks an item as intentionally unused (suppressing dead-code lints). _send_notification is called from two places and is not unused. A name like send_notification_inner or send_notification_impl communicates the intent more clearly.
| fn _send_notification(app: &tauri::AppHandle, notification_type: NotificationType, always: bool) { | |
| fn send_notification_inner(app: &tauri::AppHandle, notification_type: NotificationType, always: bool) { |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/notifications.rs
Line: 109
Comment:
**Misleading leading-underscore on an actively-used private function**
In Rust, a `_` prefix conventionally marks an item as intentionally unused (suppressing dead-code lints). `_send_notification` is called from two places and is not unused. A name like `send_notification_inner` or `send_notification_impl` communicates the intent more clearly.
```suggestion
fn send_notification_inner(app: &tauri::AppHandle, notification_type: NotificationType, always: bool) {
```
How can I resolve this? If you propose a fix, please make it concise.
|
Updated the PR to address Greptile bot's P2 suggestions (renamed functions, fixed notification logic). Ready for re-review. |
1 participant
Resolves deep link action corruption and implements deterministic pause and resume handlers for the cap:// protocol. This ensures reliable remote control from external tools like Raycast and optimizes the notification experience by preventing audio alerts during deep link execution.
Greptile Summary
This PR introduces a new
cap://deep link scheme for deterministic recording control (pause, resume, toggle-pause, stop, start) alongside the existingcap-desktop://scheme, and registers it intauri.conf.json. URL sanitization before logging andok_or_elsefor lazy allocation are welcome improvements. All three remaining findings are P2 style/UX concerns — no blocking issues were found.Confidence Score: 5/5
Safe to merge — all findings are P2 style/UX concerns with no blocking correctness or security issues
All three findings are P2: the send_always bypass is a minor UX concern, the pre-action notification is cosmetic, and the _send_notification naming is pure style. None affect data integrity or security. Per the confidence guidance, PRs with only P2 findings default to 5/5.
notifications.rs deserves a second look around send_always and the _send_notification naming before the next refactor
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[cap:// URL received] --> B[try_from URL] B -->|scheme == cap| C{match host/path} C -->|record| D[StartDefaultRecording] C -->|stop| E[StopRecording] C -->|pause| F[PauseRecording] C -->|resume| G[ResumeRecording] C -->|toggle-pause| H[TogglePauseRecording] C -->|unknown| I[ActionParseFromUrlError::Invalid] B -->|scheme == cap-desktop| J{domain == action?} J -->|yes| K[Parse JSON value param] K --> L[StartRecording / OpenSettings / etc.] D & E & F & G & H & L --> M[execute] M --> N[DeepLinkTriggered.send_always BEFORE action] N --> O{action result} O -->|Ok| P[Success] O -->|Err| Q[error! log] style N fill:#f96,color:#000Prompt To Fix All With AI
Reviews (1): Last reviewed commit: "fix: resolve deep link corruption and im..." | Re-trigger Greptile
Context used: