-
Notifications
You must be signed in to change notification settings - Fork 2.3k
Add RunEvent::Exit event to tauri to handle macos context menu exit #6005
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
Conversation
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.
Important
Looks good to me! 👍
Reviewed everything up to f3b53a5 in 1 minute and 37 seconds. Click for details.
- Reviewed
138lines of code in1files - Skipped
0files when reviewing. - Skipped posting
4draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. src-tauri/src/lib.rs:195
- Draft comment:
Duplicate cleanup calls: The cleanup logic is triggered in both the CloseRequested event (via on_exit) and again in the RunEvent::Exit handler. Consider unifying these to avoid running cleanup twice. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 10% vs. threshold = 50% Looking at the code more carefully, these aren't exactly duplicate cleanups - they serve different purposes. The CloseRequested is for normal window closing, while RunEvent::Exit is a fallback for force quit scenarios (like quitting from dock). The comment misses that these are intentionally separate paths with different timeouts and behaviors for different scenarios. The Exit handler even has a comment explaining this: "This is called when the app is actually exiting (e.g., macOS dock quit)". I could be wrong about the necessity of having two paths - maybe they could be unified with a parameter for timeout duration. The current implementation might also have race conditions between the two cleanup paths. The separate paths appear intentional for handling different exit scenarios appropriately. The Exit handler specifically uses shorter timeouts for force-quit situations. Having separate implementations provides clearer code organization for these distinct cases. The comment should be deleted as it mischaracterizes intentionally separate cleanup paths as problematic duplication. The code is handling different exit scenarios appropriately.
2. src-tauri/src/lib.rs:24
- Draft comment:
Repeated window fetching: The webview window 'main' is retrieved twice (once for hiding and once for emitting an event). Consider storing it in a variable to improve readability and ensure consistency. - Reason this comment was not posted:
Decided after close inspection that this draft comment was likely wrong and/or not actionable: usefulness confidence = 20% vs. threshold = 50% While the comment is technically correct that the window is retrieved twice, there are good reasons for the current structure: 1) The operations are independent 2) The code is more resilient this way - if the window becomes unavailable between operations, the second operation will gracefully handle it 3) The performance impact is negligible since this is cleanup code that runs once on exit. The suggested change would make the code more brittle for minimal benefit. I may be overvaluing code resilience - storing the window reference briefly could still be safe. The code would be slightly more concise. While the code could be more concise, the current structure better handles edge cases and follows the pattern of defensive programming. The benefit of the suggested change is too small to justify. The comment should be removed. While technically accurate, the suggested change would provide minimal benefit and could potentially make the code less robust.
3. src-tauri/src/lib.rs:44
- Draft comment:
Inconsistent HTTP shutdown timeouts: The cleanup in on_exit uses a 5-second timeout for the HTTP server shutdown, whereas the RunEvent::Exit handler uses a 2-second timeout. Verify if this discrepancy is intentional. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
4. src-tauri/src/lib.rs:18
- Draft comment:
Blocking async cleanup: The use of tokio::task::block_in_place with async_runtime::block_on may block the thread. Ensure this blocking pattern is acceptable in the Tauri context and does not lead to performance issues. - Reason this comment was not posted:
Confidence changes required:50%<= threshold50%None
Workflow ID: wflow_q3ZX0wI5gFMafhNG
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
Barecheck - Code coverage reportTotal: 38.63%Your code coverage diff: -0.01% ▾ ✅ All code changes are covered |
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.
Skipped PR review on cc972c4 because no changed files had a supported extension. If you think this was in error, please contact us and we'll fix it right away.
cc972c4 to
e11b4c9
Compare
Use RunEvent::Exit to handle shutdown cleanup processes.
Also did slight refactoring
Important
Adds
RunEvent::Exithandling insrc-tauri/src/lib.rsfor cleanup on app exit, with specific handling for macOS dock quit.RunEvent::Exithandling insrc-tauri/src/lib.rsto manage cleanup on app exit, specifically for macOS dock quit.Exit, hides the main window, emitskill-mcp-servers, and performs quick cleanup with a shorter timeout.builder.build()andapp.run()phases insrc-tauri/src/lib.rsfor clarity.This description was created by
for 6ceb682. You can customize this summary. It will automatically update as commits are pushed.