feat(lifecycle): keep Staged running when its last window closes - #936
Open
matt2e wants to merge 2 commits into
Open
feat(lifecycle): keep Staged running when its last window closes#936matt2e wants to merge 2 commits into
matt2e wants to merge 2 commits into
Conversation
Closing the window (red button / Cmd+W) terminated the app, taking every running agent session with it — and Cmd+Q was worse: `PredefinedMenuItem::quit` maps to `NSApp terminate:`, which reaches no Tauri hook, so it skipped the action-shutdown handler entirely and left agent CLIs (spawned with their own process group and only `kill_on_drop`) orphaned. New `app_lifecycle` module owns both halves of the fix, adapted to the multi-window model (#928) this lands on top of: - Closing a window with peers still live (visible or hidden) is just a close: sessions belong to the process, so the window is destroyed normally and the existing `Destroyed` hook does the per-window cleanup. Closing the *last* window is where the interception bites: on macOS `CloseRequested` is prevented and the window hidden, so sessions keep streaming; the Dock icon (`RunEvent::Reopen`), `Window ▸ Staged`, or a quit arriving while hidden brings it back — `show_a_window` prefers `main` for its restored geometry but recovers any surviving `win-N` peer. Hiding also drops that window's `tauri-{label}` PR-poll client to its unfocused tier, which a hidden window's missing webview blur would not. Other platforms have no Dock or tray to recover a hidden window, so closing the last window still quits there — now through the confirmation gate. - A custom Quit menu item makes Cmd+Q routable, so `request_quit` can gate it on active sessions and raise a confirmation dialog; confirming cancels each session with `CompletionReason::AppQuit` (the cancel is what runs the ACP child's graceful stop), waits for sessions and actions inside one shared 2s budget, sweeps any still-active rows to cancelled/app_quit, then exits. Queued sessions count as active; running actions are reported but don't gate. Quit and `Window ▸ Staged` route through the shared `dispatch_menu_event` router as focus-independent backend actions — every window being hidden is exactly when they matter. The dialog is addressed to exactly one window (`emit_to` plus a window-scoped frontend listener, the same pattern as menu routing): where the user is, or a window revealed for the purpose. A broadcast would raise one dialog per window, each unaware of the others' answers. The pending-prompt flag remembers its host window and is cleared when that window is destroyed, so the force-on-second- request escape hatch can't fire with no dialog on screen. `RunEvent::Exit` now runs the same idempotent cleanup, which is the only hook on the terminate: path — Dock ▸ Quit and logout stop sessions and actions instead of orphaning them. Ownership is checked against `owner_pid`, so a quit never prompts about or cancels another Staged instance's work. The quit commands are refused in the web-mode dispatch table: a browser client must not be able to terminate the desktop host. The store-incompatibility screens' Close buttons now quit rather than closing a window that would only hide. Phase 4 of the plan (routing Dock ▸ Quit through the prompt via a runtime `applicationShouldTerminate:`) is deliberately left out: the shared cleanup already prevents the process and data damage there, only the prompt is missing. Verified with `just check-all`. Signed-off-by: Matt Toohey <contact@matttoohey.com>
The quit confirmation lived in the webview: `request_quit` emitted `app:quit-requested` to one chosen window and a Svelte `AlertDialog` rendered it. That forced a window into the quit path, and the case it hurt is the central scenario of this branch — the whole point of closing-window-is-not- quitting is that sessions keep streaming with everything hidden, so "all windows hidden, sessions running, user hits Cmd+Q" isn't a corner to tolerate, it's the primary path the confirmation exists to serve. Reaching it materialised a full application window (restored geometry, hydrating project tree) to host a two-button question, and `cancel_quit` did nothing but clear a flag — so Cmd+Q then Cancel left the user with a visible window they had to close a second time, having asked for neither. Parenting a native alert would not have fixed that: `.parent()` is exactly what makes `tauri-plugin-dialog` render an `NSAlert` as a window-modal *sheet*, so the reveal would have stayed. Unparented is a different widget, not a different modality of the same one — rfd 0.16 routes a parentless dialog to `CFUserNotificationDisplayAlert`, displayed by the system rather than by AppKit. That's what buys window-independence, so the reveal drops out of the quit path entirely: quitting from a hidden state stays hidden, cancelling returns the app to exactly the state the user left it in, and the branch where no window could be revealed and the app quit *without asking* disappears rather than being preserved. Structurally this resolves the review finding about a pending prompt outliving its host window by removing the concept of a host window. `QuitState.prompt_host` collapses to `prompt_pending: AtomicBool`; `clear_prompt_if_host`, the `Destroyed` arm of `on_window_event`, and `reveal_a_window` (now inlined into its one caller, `show_a_window`) all go away. The frontend half goes with them: `QuitConfirmDialog`, the `quitPrompt` store, `quitListener`, `quitPromptCopy`, the `QuitRequestedPayload` type, and the `confirm_quit` / `cancel_quit` commands. `quit_app` stays — the store-incompatibility screens still need it — and stays refused in the web-mode dispatch table. The prompt copy ports to Rust, where `get_branch` / `get_project` resolve the names the Svelte dialog used to read from its stores; the review's suggestion to fold the session list into the preceding sentence with a colon is taken while the wording moves. `OkCancelCustom`'s ok slot is the default (Return) button, so it holds "Keep Running" and the *cancel* slot holds "Quit & Stop Sessions" — a stray Return must not be what kills running agents. Accepted costs, all inherent to the widget: the alert carries generic system chrome rather than Staged's icon; the "Stopping sessions…" progress state is gone, since a native alert dismisses on click while `shutdown_cleanup` runs out its 2s budget; and it is not modal to the app, so work can start behind it. The last resolves correctly — `shutdown_cleanup` re-queries active sessions instead of trusting the prompt's snapshot — and it keeps the force-quit escape hatch dispatchable, which a second Cmd+Q needs. Because that dismissal leaves nothing on screen, `request_quit` now returns early when a shutdown is already under way instead of raising a second alert about sessions the first one is stopping. Verified with `just check-all`. Signed-off-by: Matt Toohey <contact@matttoohey.com>
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 941614d388
ℹ️ About Codex in GitHub
Codex has been enabled to automatically review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| /// signalled first and waited on against this one deadline, because the | ||
| /// `RunEvent::Exit` path runs inside `applicationWillTerminate:`, where the OS | ||
| /// gives us limited time before killing the process outright. | ||
| const SHUTDOWN_BUDGET: Duration = Duration::from_secs(2); |
There was a problem hiding this comment.
When an active ACP agent does not acknowledge session/cancel within two seconds, this deadline expires and spawn_quit calls app.exit(0) while the agent child is still alive. The cancellation path in crates/acp-client/src/driver.rs can wait five seconds for the prompt response and, for remote agents, another five seconds in graceful_stop; because process exit does not run the child's kill_on_drop, slow local agents or remote proxy process groups can be orphaned and continue consuming resources or modifying the workspace after Staged quits. The shutdown path must either wait through the driver's teardown bounds or explicitly force-kill remaining session processes before exiting.
Useful? React with 👍 / 👎.
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.
Closing the window (red button / Cmd+W) terminated Staged, taking every running agent session with it. Cmd+Q was worse:
PredefinedMenuItem::quitmaps toNSApp terminate:, which reaches no Tauri hook, so it skipped the action-shutdown handler entirely and left agent CLIs orphaned.A new
app_lifecyclemodule owns both halves, adapted to the multi-window model from #928.Close-to-hide
Destroyedhook does the per-window cleanup.CloseRequestedand hides the window instead, so sessions keep streaming. The Dock icon (RunEvent::Reopen), the newWindow ▸ Stageditem, or a quit brings it back;show_a_windowprefersmainfor its restored geometry but recovers any survivingwin-Npeer.tauri-{label}PR-poll client to its unfocused tier, which a hidden window's missing webview blur would not.Gated quit
request_quitcan gate it on active sessions. The confirmation is an unparented native alert —rfdroutes a parentless dialog toCFUserNotificationDisplayAlert, which the system displays rather than AppKit, so quitting from a fully hidden state never materialises a window and cancelling returns the app to exactly the state the user left it in.OkCancelCustom's default (Return) button holds "Keep Running"; a stray Return must not kill running agents.CompletionReason::AppQuit, waits for sessions and actions inside one shared 2s budget, sweeps any still-active rows to cancelled/app_quit, then exits. Queued sessions count as active; running actions are reported but don't gate. Ownership is checked againstowner_pid, so a quit never touches another Staged instance's work.Window ▸ Stagedroute through the shareddispatch_menu_eventrouter as focus-independent backend actions — every window being hidden is exactly when they matter.RunEvent::Exitruns the same idempotent cleanup, the only hook on theterminate:path, so Dock ▸ Quit and logout stop sessions and actions instead of orphaning them.quit_appis refused in the web-mode dispatch table: a browser client must not be able to terminate the desktop host. The store-incompatibility screens' Close buttons now quit rather than closing a window that would only hide.Known gaps
applicationShouldTerminate:) is deliberately left out — the shared cleanup already prevents the process and data damage there, only the prompt is missing.shutdown_cleanupre-queries active sessions instead of trusting the prompt's snapshot, and it keeps the second-Cmd+Q force-quit escape hatch dispatchable.Verified with
just check-all.