feat(quit): hold ⌘Q to quit with confirm gate (#306)#344
Open
moonray wants to merge 10 commits into
Open
Conversation
moonray
added a commit
to moonray/repoprompt-ce
that referenced
this pull request
Jul 1, 2026
…epoprompt#306) Address PR repoprompt#344 review: 1. The owner-window-willClose observer called cancelHold(), which tears down the overlay/timer but does NOT reset `decision`. If the key window closed while .holding/.armed, the gate was left in a stale state — and a later ⌘Q tap could quit from stale .armed (tap keyDown→.ignore, keyUp→.quit). Fixed by adding an `.externalCancel` event to QuitHoldDecision that resets .holding/.armed → .idle with a `.dismiss` intent (immediate overlay dismiss, no linger). The owner-close observer now feeds .externalCancel. 2. No app-deactivation path cancelled an active hold. With hidesOnDeactivate =false and the local key monitor unable to see the keyUp once the app is no longer frontmost, a hold could get stuck and the overlay stay visible. Added an NSApplication.didResignActiveNotification observer that feeds .externalCancel, aborting any in-flight hold on focus loss. 3. Class/fireTermination doc comments no longer say terminate is "deferred one run-loop turn" — it is intentionally synchronous during sendEvent. QuitHoldDecisionTests +5 (externalCancel from holding/armed/idle/quitting + a regression proving a tap after an armed-state external cancel does not quit). Ledger updated (+5 rows; 18 decision tests). 29 hold-to-quit unit tests green; SwiftLint + SwiftFormat clean.
Contributor
Author
|
Thanks for the review — both must-fix items addressed in Must-fix 1 — stale decision on external cancel. Added an Must-fix 2 — app deactivation. Added an Suggestion — stale comment. Updated the class doc and Tests: Note on |
moonray
added a commit
to moonray/repoprompt-ce
that referenced
this pull request
Jul 1, 2026
…epoprompt#306) Address the test-quality review follow-up on PR repoprompt#344. Consolidation: - QuitHoldEventFilterTests 11 → 3 table-driven methods (ignorable-modifier normalization, toggle-off passthrough, non-quit-gesture rejection). Fewer methods, same coverage, less ledger noise. Controller lifecycle coverage gap (the two must-fix bugs had no controller- level test): - Added a small test seam: `handle(_:)`, `holdState`, and `handleExternalCancellation()` are now internal; the toggle source is injectable (`init(warnBeforeCmdQ:)`, default reads GlobalSettingsStore). - New QuitHoldControllerTests drive the real controller with synthetic NSEvents + an injected toggle (no live UI / no real settings file): · testExternalCancellationWhileHoldingResetsToIdle · testTapAfterExternalCancellationDoesNotQuit (regression: a tap after an aborted hold must not quit from stale .armed state) 23 hold-to-quit unit tests green (18 decision + 3 filter + 2 controller); SwiftLint + SwiftFormat clean; ledger reconciled for all hold-to-quit tests.
Contributor
Author
|
Follow-up to the test-quality pass ( Consolidation — Controller lifecycle coverage gap closed — the two must-fix bugs had no controller-level test. Added a small seam (
23 hold-to-quit unit tests green (18 decision + 3 filter + 2 controller); SwiftLint + SwiftFormat clean; ledger reconciled for all hold-to-quit tests (the repo-wide |
Contributor
Author
|
CI "Build and Test" failure is a flaky, unrelated test — please rerun. The failing test is Evidence it's a flake, not a regression:
I don't have admin rights to rerun the job myself. A rerun should go green. |
Add Chrome-style "hold ⌘Q to quit" to prevent accidental termination of in-flight agent sessions and unsaved state. A local NSEvent monitor gates ONLY the keyboard ⌘Q gesture; menu, Dock, status-item, Apple-Event, and system logout/shutdown quits bypass the gate and quit immediately. - QuitHoldDecision: pure, AppKit-free hold state machine (idle/holding/ quitting) mapping keyDown/keyUp/flagsChanged/timerElapsed to beginHold/cancel/quit/ignore. - QuitHoldEventFilter: pure swallow policy — swallow iff exact-modifier ⌘Q (command-only, normalizing capsLock/numericPad/function) and the toggle is on, regardless of decision intent, so repeat ⌘Q keyDowns during a hold stay swallowed. - QuitHoldController: installs the monitor (MainActor.assumeIsolated bridging), runs a 1.0s hold timer, shows a non-focus-stealing overlay anchored to the key window (or main screen), and calls NSApp.terminate(nil) on a confirmed hold. applicationShouldTerminate is unchanged. install() is guarded by !suppressesNonessentialLaunchSideEffects. - Settings: "Warn before quitting with ⌘Q" toggle under Settings -> General, default on, persisted via GlobalSettingsStore (backward-compatible optional UISettings field). Tests: QuitHoldDecisionTests (9), QuitHoldEventFilterTests (11), plus 3 settings default-on/round-trip/legacy-decode tests. Test contract ledger updated (+23 rows, verify-ledger reconciled). Refs repoprompt#306.
…t#306) On early ⌘Q release the overlay vanished instantly, so the "Hold ⌘Q to Quit" message was unreadable. Chrome keeps it visible ~2s then fades. cancelHold() now starts a Chrome-style dismissal: dwell at full opacity for ~1.7s, then fade the panel alpha to 0 over ~0.3s (~2s total) before removal. hideOverlay() (used by a confirmed quit and by a fresh hold) cancels any in-flight linger and remains immediate; beginFadeOut guards its removal with a panel-identity check so a new hold started during the fade replaces the overlay cleanly.
…epoprompt#306) Match Chrome: the hold overlay shows only the "Hold ⌘Q to Quit" text — no progress bar or elapsed time — and the text is roughly doubled (.headline ~17pt -> 34pt). The panel is now sized to the SwiftUI content's natural fitting size instead of a fixed frame.
…epoprompt#306) The toggle was added to GeneralSettingsView, which is legacy/unused — it is not rendered by any Settings tab (the Settings window switches on SettingsTab cases: appearance, advanced, keyboardShortcuts, ...), so the setting was invisible. Move it into AdvancedSettingsView (the General-section app-behavior view, home of the related "Enable keyboard shortcuts" toggle) as a new "Quit" section, and revert the orphaned GeneralSettingsView edits. The setting now appears under Settings → General → Advanced → Quit.
…epoprompt#306) The hold-to-quit threshold now ARMS the quit (overlay -> "Release ⌘Q to Quit"); the user releases ⌘Q to fire it. QuitHoldDecision gains an `armed` state: timerElapsed from `holding` -> `.arm` (not `.quit`); keyUp / Command release while `armed` -> `.quit`. Why release-to-quit: terminating while the key is still held wedges the shutdown. applicationShouldTerminate returns .terminateLater and relies on a `Task { @mainactor }` to call reply(true); that Task only drains if NSApp.terminate is initiated from within AppKit event dispatch (sendEvent) — the menu and Apple-Event quit paths satisfy this, but a DispatchQueue callback (sync or asyncAfter) does not, leaving the app wedged in terminateLater with the Quit menu disabled. Fix: fireTermination calls NSApp.terminate(nil) SYNCHRONOUSLY from the local NSEvent monitor's release handler (during sendEvent). Verified live: hold ~1s -> overlay switches to "Release ⌘Q to Quit" -> release -> app quits through the normal shutdown path. Quick tap (<1s) still cancels. QuitHoldDecisionTests updated (13): timer->arm, +4 armed-state tests (release->quit, cmd-release->quit, repeat-ignored, cmd-held-ignored). Test contract ledger reconciled (2821 -> 2825). No AppDelegate or WindowStatesManager changes; the graceful shutdown path is untouched.
…epoprompt#306) Address PR repoprompt#344 review: 1. The owner-window-willClose observer called cancelHold(), which tears down the overlay/timer but does NOT reset `decision`. If the key window closed while .holding/.armed, the gate was left in a stale state — and a later ⌘Q tap could quit from stale .armed (tap keyDown→.ignore, keyUp→.quit). Fixed by adding an `.externalCancel` event to QuitHoldDecision that resets .holding/.armed → .idle with a `.dismiss` intent (immediate overlay dismiss, no linger). The owner-close observer now feeds .externalCancel. 2. No app-deactivation path cancelled an active hold. With hidesOnDeactivate =false and the local key monitor unable to see the keyUp once the app is no longer frontmost, a hold could get stuck and the overlay stay visible. Added an NSApplication.didResignActiveNotification observer that feeds .externalCancel, aborting any in-flight hold on focus loss. 3. Class/fireTermination doc comments no longer say terminate is "deferred one run-loop turn" — it is intentionally synchronous during sendEvent. QuitHoldDecisionTests +5 (externalCancel from holding/armed/idle/quitting + a regression proving a tap after an armed-state external cancel does not quit). Ledger updated (+5 rows; 18 decision tests). 29 hold-to-quit unit tests green; SwiftLint + SwiftFormat clean.
…epoprompt#306) Address the test-quality review follow-up on PR repoprompt#344. Consolidation: - QuitHoldEventFilterTests 11 → 3 table-driven methods (ignorable-modifier normalization, toggle-off passthrough, non-quit-gesture rejection). Fewer methods, same coverage, less ledger noise. Controller lifecycle coverage gap (the two must-fix bugs had no controller- level test): - Added a small test seam: `handle(_:)`, `holdState`, and `handleExternalCancellation()` are now internal; the toggle source is injectable (`init(warnBeforeCmdQ:)`, default reads GlobalSettingsStore). - New QuitHoldControllerTests drive the real controller with synthetic NSEvents + an injected toggle (no live UI / no real settings file): · testExternalCancellationWhileHoldingResetsToIdle · testTapAfterExternalCancellationDoesNotQuit (regression: a tap after an aborted hold must not quit from stale .armed state) 23 hold-to-quit unit tests green (18 decision + 3 filter + 2 controller); SwiftLint + SwiftFormat clean; ledger reconciled for all hold-to-quit tests.
…mpt#306) Simplify QuitHoldController without changing behavior. - Extract the overlay/panel/linger/fade code into a dedicated `@MainActor QuitHoldOverlay` (show/update/cancelWithLinger/hide + the panel and SwiftUI view types). The controller is now gesture orchestration only (252 lines, down from ~400). Mechanical move; the `=== panel` identity guard in beginFadeOut and the owner-window-close observer lifecycle are preserved verbatim (the observer now fires an `onOwnerClose` closure back to the controller's `handleExternalCancellation()`). - DRY the work-item teardown: add `cancelHoldTimer()`; `hide()`/`overlay.hide()` is the sole owner of `lingerWorkItem`. Drops the redundant cancels in `.dismiss` and `fireTermination()`. `fireTermination()` still calls `NSApp.terminate(nil)` synchronously from the monitor's release handler (during sendEvent) — unchanged. Decision, filter, AppDelegate, WindowStatesManager, and all tests untouched. 23 hold-to-quit tests green; SwiftLint + SwiftFormat clean.
…rompt#306) Add prominent "LOAD-BEARING — do not optimize" rationale so a future change doesn't silently break quit. Two non-obvious constraints, each with the consequence of breaking it: 1. `NSApp.terminate(nil)` must be called synchronously from the NSEvent monitor's release handler (during sendEvent). Deferring to any DispatchQueue/Task callback wedges terminateLater (shutdown Task never drains; Quit menu disabled; process alive). 2. Quit fires on ⌘Q release (keyUp), NOT at the threshold while held. Quitting mid-hold propagates the held ⌘Q to the next focused app (its ⌘Q fires too) — the at-threshold-while-holding variant was tried and reverted for exactly this reason. Chrome also quits on keyUp. Comments only; no behavior change.
…ainactor (repoprompt#306) Oracle final-review nit: onOwnerClose is captured by NotificationCenter's @sendable observer block. Today's call site is safe, but the parameter type didn't encode it — a future non-Sendable caller could trip a stricter Swift 6 toolchain. Mark it @escaping @sendable @mainactor (captured across actors, run on main via MainActor.assumeIsolated). No behavior change.
moonray
force-pushed
the
feat/hold-to-quit-306
branch
from
f3bca7f to
82e1595
Compare
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.
Closes #306.
Summary
Chrome-style "hold ⌘Q to quit" so a stray ⌘Q no longer kills in-flight agent sessions or unsaved state. Only the keyboard ⌘Q gesture is gated; menu, Dock, status-item, Apple-Event, and system logout/shutdown quits bypass the gate and quit immediately, exactly like Chrome. A Settings toggle ("Warn before quitting with ⌘Q", default on) controls it.
Behavior
applicationShouldTerminateshutdown path (window-session persist + agent/server teardown).osascriptquit → unaffected (immediate).Key implementation note (non-obvious)
The quit is release-gated (not fired mid-hold like Chrome), and
NSApp.terminate(nil)is called synchronously from the localNSEventmonitor's release handler — i.e., during AppKit'ssendEvent.This is required.
applicationShouldTerminatereturns.terminateLaterand relies on aTask { @MainActor in … }to callreply(true). That Task only drains ifterminateis initiated from within AppKit event dispatch. The menu and Apple-Event quit paths satisfy this. Callingterminatefrom aDispatchQueuecallback — synchronous orasyncAfter, even after all release events have settled — leaves the app wedged interminateLater: the Task body never runs,reply(true)is never called, the Quit menu item stays disabled, and the process hangs.Task.detached { @MainActor }doesn't help either (its first line also never runs).Firing on release (during the keyUp's
sendEvent) is the clean minimal fix: it preserves the entire graceful shutdown path unchanged —AppDelegateandWindowStatesManagerare not modified; only the quit trigger/timing moved intoQuitHoldController.UX trade-off: the user releases to fire the quit rather than it firing mid-hold. The overlay communicates this ("Release ⌘Q to Quit").
Design
QuitHoldDecision— pure, AppKit-free state machine (idle→holding→armed→quitting); intents.beginHold/.arm/.cancel/.quit/.ignore. Unit-tested (13 tests).QuitHoldEventFilter— pure swallow policy: swallow iff exact-modifier ⌘Q ((.deviceIndependentFlagsMask & flags).subtracting([.capsLock, .numericPad, .function]) == .command, keyCode0x0C) and toggle on, independent of the decision intent. Unit-tested (11 tests).QuitHoldController— localNSEventmonitor + non-focus-stealing overlay +MainActor.assumeIsolatedbridging; installed at launch, guarded by!suppressesNonessentialLaunchSideEffects.warnBeforeCmdQ(Bool?, default nil→on) onUISettings; toggle under Settings → General → Advanced → Quit.Testing
43 unit tests green (13 decision + 11 filter + 19 settings). SwiftLint
--strict+ SwiftFormat clean. Test contract ledger reconciled. Live-verified: tap cancels; hold→release quits cleanly; menu/Dock bypass; ⌘W unaffected; overlay doesn't steal focus.Happy to squash the commits on merge.
🤖 Generated with Claude Code