Confirm before closing terminals with running processes#687
Conversation
…-close # Conflicts: # supacodeTests/PullRequestMergeQueueStatusTests.swift
sbertix
left a comment
sbertix
left a comment
There was a problem hiding this comment.
I played with this a bit, and I feel like there's been a misunderstanding. I should have been more specific in the issue reply (my bad, sorry 🙇♂️), but confirm-close-surface was inviting a surface-based approach, the same as Ghostty, instead of a tab-based one.
For instance, this tab-based approach doesn’t prompt for the number-one use case (imho): ⌘W on a single-surface tab, since that is always a surface close, not a tab close.
Would you be interested in reworking the PR/closing this and opening a new one so the flag actually implements a confirm-close-surface 1:1 equivalent for Supacode? Cause that's the thing we should mirror (as we don't enforce that), and adding it without tying it to Ghostty config makes it extendable in the future to any node leaf.
adamgall
changed the title
|
Thank you 🙇♂️ I'll try and get back to this tomorrow (so we can 100% have it in the next release) |
…confirm-running-tab-close
Drive the confirmation alert off the pending target's captured payload so a dismissal written back through the SwiftUINavigation item binding can no longer race the button actions: confirm closes the captured target even if the published value was cleared first, and cancel reliably clears the surface's explicit-close flag so a later unexpected zmx exit still reattaches instead of tearing down. Route every user-facing close command through the confirmation. Ghostty's palette/keybind close-tab now honors its scope, so "close others" and "close to the right" confirm like the context menu instead of collapsing to a single-tab close. Bypass the confirmation only for programmatic surface destroys, which the deeplink/CLI layer already gates upstream, so their acknowledgement is not blocked on an interactive alert. Fold the identical alert copy onto the type, drop the now-dead closeOtherTabs, closeTabsToRight, and closeAllTabs, and log the two silent close paths. Cover the bypass path, captured-payload confirm and cancel, the bulk-close wrappers, the Ghostty close-tab modes, multi-tab confirmation, and destroyed-while-pending cleanup.
sbertix
left a comment
sbertix
left a comment
There was a problem hiding this comment.
Thanks 🙇♂️ I pushed a hardening pass on top to close a confirm/cancel race in the alert and route the missing close commands (Ghostty's palette close-others / close-to-the-right, which weren't wired up) through the confirmation.
2 participants
Closes #684
Summary
confirm-close-surface, explicit surface closes consume Ghostty'sprocessAlivecallback, and tab closes preflight every pane withghostty_surface_needs_confirm_quit.Type of change
ready)How was this tested?
Automated
WorktreeTerminalManagerTests,AgentBusyStateTests,SettingsFeatureTests,SettingsFilePersistenceTests, andGhosttyRuntimeBundledOverridesTestspassed after the surface-routing rework.make checkpassed (format + lint).make testpassed: 2,671 tests, 0 failures (2,655 passed and 16 expected failures).make build-apppassed.Manual
Before the review-driven internal routing change, the feature branch was stacked temporarily onto the existing isolated Supacode Dev implementation from #510 so it could run beside production without sharing
~/.supacode, restored layouts, app identity, or CLI socket routing. The isolation commits are not included in this pull request.Confirmed end to end through the final implementation:
sleep 300shows the confirmation alert.⌘Win one pane shows the alert; focusing the other pane and invoking⌘Wmakes no visual change; confirming the existing alert closes only the originally targeted pane.The final native surface-routing and active-target-preservation behavior is covered by the focused and full automated suites and the two-pane manual scenario above.
make checkpasses (format + lint)make testpassesAI tool disclosure (optional)
Checklist
Closes #above.ready.