-
Notifications
You must be signed in to change notification settings - Fork 1.2k
feat: add pause-recording #1546
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
feat: add pause-recording #1546
Conversation
apps/desktop/src-tauri/src/lib.rs
Outdated
| mod camera_legacy; | ||
| mod captions; | ||
| mod deeplink_actions; | ||
| mod _actions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
mod _actions; won't resolve as-is since this PR doesn't add an apps/desktop/src-tauri/src/_actions.rs module (only deeplink_actions.rs). If the intent is just to add the new deep-link action, keeping the original module name is the simplest fix.
| mod _actions; | |
| mod deeplink_actions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please apply the suggestion
apps/desktop/src-tauri/src/lib.rs
Outdated
| }; | ||
| use tauri::{AppHandle, Manager, State, Window, WindowEvent, ipc::Channel}; | ||
| use tauri_plugin_deep_link::DeepLinkExt; | ||
| use tauri_plugin_deep_link::Ext; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unless tauri-plugin-deep-link v2.2.0 actually exports Ext, this will stop compiling (the previous DeepLinkExt import works).
| use tauri_plugin_deep_link::Ext; | |
| use tauri_plugin_deep_link::DeepLinkExt; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please apply the suggestion
apps/desktop/src-tauri/src/lib.rs
Outdated
| trace!("Single instance invoked with args {args:?}"); | ||
|
|
||
| // This is also handled as a deeplink on some platforms (eg macOS), see deeplink_actions | ||
| // This is also handled as a on some platforms (eg macOS), see _actions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like this comment got truncated (missing the word after as a).
| // This is also handled as a on some platforms (eg macOS), see _actions | |
| // This is also handled as a deeplink on some platforms (eg macOS), see deeplink_actions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please apply the suggestion
apps/desktop/src-tauri/src/lib.rs
Outdated
| let app_handle = app.clone(); | ||
| app.deep_link().on_open_url(move |event| { | ||
| deeplink_actions::handle(&app_handle, event.urls()); | ||
| _actions::handle(&app_handle, event.urls()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you keep the module name as deeplink_actions, this call site should match.
| _actions::handle(&app_handle, event.urls()); | |
| deeplink_actions::handle(&app_handle, event.urls()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please apply the suggestion
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
4 files reviewed, 4 comments
apps/desktop/src-tauri/src/lib.rs
Outdated
| mod camera_legacy; | ||
| mod captions; | ||
| mod deeplink_actions; | ||
| mod _actions; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
module name uses unconventional underscore prefix _actions
| mod _actions; | |
| mod deeplink_actions; |
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/lib.rs
Line: 10:10
Comment:
module name uses unconventional underscore prefix `_actions`
```suggestion
mod deeplink_actions;
```
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please apply the suggestion
apps/desktop/src-tauri/src/lib.rs
Outdated
| }; | ||
| use tauri::{AppHandle, Manager, State, Window, WindowEvent, ipc::Channel}; | ||
| use tauri_plugin_deep_link::DeepLinkExt; | ||
| use tauri_plugin_deep_link::Ext; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import loses semantic clarity by using generic Ext instead of DeepLinkExt
| use tauri_plugin_deep_link::Ext; | |
| use tauri_plugin_deep_link::DeepLinkExt; |
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!
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/lib.rs
Line: 87:87
Comment:
import loses semantic clarity by using generic `Ext` instead of `DeepLinkExt`
```suggestion
use tauri_plugin_deep_link::DeepLinkExt;
```
<sub>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!</sub>
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please apply the suggestion
apps/desktop/src-tauri/src/lib.rs
Outdated
| trace!("Single instance invoked with args {args:?}"); | ||
|
|
||
| // This is also handled as a deeplink on some platforms (eg macOS), see deeplink_actions | ||
| // This is also handled as a on some platforms (eg macOS), see _actions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
damaged text - "handled as a on some" - missing word deleted from middle of sentence
| // This is also handled as a on some platforms (eg macOS), see _actions | |
| // This is also handled as a deeplink on some platforms (eg macOS), see deeplink_actions |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/lib.rs
Line: 2779:2779
Comment:
damaged text - "handled as a on some" - missing word deleted from middle of sentence
```suggestion
// This is also handled as a deeplink on some platforms (eg macOS), see deeplink_actions
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please apply the suggestion
apps/desktop/src-tauri/src/lib.rs
Outdated
| let app_handle = app.clone(); | ||
| app.deep_link().on_open_url(move |event| { | ||
| deeplink_actions::handle(&app_handle, event.urls()); | ||
| _actions::handle(&app_handle, event.urls()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
update module reference to match actual module name
| _actions::handle(&app_handle, event.urls()); | |
| deeplink_actions::handle(&app_handle, event.urls()); |
Prompt To Fix With AI
This is a comment left during a code review.
Path: apps/desktop/src-tauri/src/lib.rs
Line: 3073:3073
Comment:
update module reference to match actual module name
```suggestion
deeplink_actions::handle(&app_handle, event.urls());
```
How can I resolve this? If you propose a fix, please make it concise.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Please apply the suggestion
|
Thank you for the feedback. I am fixing the naming conventions and corrupted text now. I will push the update shortly. |
|
I have addressed all the comments from the bot, restored the naming conventions, and fixed the damaged comment. Ready for review! |
|
Hi Maintainer, I have addressed all the previous comments and ensured that the naming conventions are consistent with the project's standards. The PR is now free of conflicts and ready for a final review. Could you please take a look when you have a moment? Thank you for your time and guidance! Best regards, |
Greptile Overview
Greptile Summary
added
PauseRecordingaction to the desktop deep link handler, allowing recordings to be paused via URL schemeKey changes:
Action::PauseRecordingvariant to the action enum indeeplink_actions.rsexecute()handler that callsrecording::pause_recording()deeplink_actionsto_actions(unconventional naming)DeepLinkExtto genericExt(reduces code clarity)Issues found:
_actionsinstead of standarddeeplink_actionsExtinstead of explicitDeepLinkExtConfidence Score: 3/5
apps/desktop/src-tauri/src/lib.rsrequires attention for naming conventions and text restorationImportant Files Changed
PauseRecordingvariant and implementation - change is straightforward and follows existing patterns_actions, changed import to lose clarity, damaged comment textSequence Diagram
sequenceDiagram participant User participant Desktop App participant DeepLink Handler participant Action Enum participant Recording Module User->>Desktop App: Trigger pause (URL/command) Desktop App->>DeepLink Handler: on_open_url(event) DeepLink Handler->>Action Enum: try_from(url) Action Enum-->>DeepLink Handler: Action::PauseRecording DeepLink Handler->>Action Enum: execute(app_handle) Action Enum->>Recording Module: pause_recording(app, state) Recording Module-->>Action Enum: Result<(), String> Action Enum-->>DeepLink Handler: Result DeepLink Handler-->>Desktop App: Execution complete(2/5) Greptile learns from your feedback when you react with thumbs up/down!