Skip to content

mac: match upstream quit and search behavior - #181

Merged
khoi merged 3 commits into
mainfrom
fix/macos-quit-search-parity
Aug 8, 2026
Merged

mac: match upstream quit and search behavior#181
khoi merged 3 commits into
mainfrom
fix/macos-quit-search-parity

Conversation

@khoi

@khoi khoi commented Aug 8, 2026

Copy link
Copy Markdown
Contributor

Why

Quitting while Supaterm was hidden skipped confirmation because no window was visible. Search also intercepted Return before AppKit could commit marked IME text.

What changed

Quit planning now considers hidden app windows and unhides the app before presenting confirmation. Search delegates command handling through AppKit so marked text commits first; Shift-Return navigates backward and Escape closes search.

Verification

  • Focused quit, search, and menu-focus suites passed 67 tests
  • make mac-check, dead-code, dependency, web, and docs gates passed
  • Full Mac suite passed 1,700 tests; only the unrelated key-window test failed

khoi added 2 commits August 8, 2026 01:07
Hidden windows still own sessions that need a quit decision. Distinguish an
app with no windows from a hidden app, then reveal the existing app-wide
chooser before asking what to do with those sessions.
The search field intercepted Return before its field editor could commit
marked text. Route text commands through the delegate so plain Return
finishes composition while Shift-Return still moves to the prior match.
@khoi khoi added the bug Something isn't working label Aug 8, 2026
@github-actions

github-actions Bot commented Aug 8, 2026

Copy link
Copy Markdown

SupaReview

Findings

The quit predicate now includes retained auxiliary and minimized windows, but the confirmation presenter only works with visible, non-miniaturized parents; this makes normal Quit cancel indefinitely after common window-lifecycle states. The search tests also bypass the production representable wiring, leaving the new AppKit integration under-tested.

Findings

  • [P1] Do not treat every retained NSWindow as a quit-confirmation target
    hasAppWindows: !NSApp.windows.isEmpty,

    !NSApp.windows.isEmpty also matches windows that do not represent a live terminal session. ConfigurationDiagnosticsWindowController creates and retains a hidden window eagerly (and the settings controller retains one after first use); after the last terminal window closes, that window remains in NSApp.windows. A normal Quit then enters confirmation, but QuitConfirmationPresenter cannot find a visible, non-miniaturized parent and returns .cancel, so the app cannot quit. The same failure occurs with only a minimized terminal window because unhide does not deminiaturize it. Base this predicate on live terminal windows (or otherwise provide a presentable parent) rather than the raw application window list.
  • [P3] Exercise the production search-field wiring in the new command tests
    try withSearchField(modifierFlags: .shift) { _, _, recorder, window in
    window.sendEvent(
    try makeKeyEvent(
    keyCode: kVK_Return,
    characters: "\r",
    modifierFlags: .shift,

    The Shift-Return test manually constructs GhosttySearchFieldDelegate through withSearchField and injects { .shift }, so it does not exercise GhosttySearchField.makeCoordinator/updateNSView or the production default that reads NSApp.currentEvent. A regression in installing the delegate, refreshing the callbacks, or reading real event modifiers would still leave these tests green even though the AppKit behavior being changed is broken. Add an integration path using the representable/default initializer (or a real-event test without the injected modifier closure) in addition to this focused delegate test.

Review type: Full review

Reviewers: thermo-nuclear-review-subagent, thermo-nuclear-code-quality-review-subagent

@tuist

tuist Bot commented Aug 8, 2026

Copy link
Copy Markdown

🛠️ Tuist Run Report 🛠️

Tests 🧪

Scheme Status Cache hit rate Tests Skipped Ran Commit
supatermUITests 0 % 76 0 76 14db29967

Builds 🔨

Scheme Status Duration Commit
supatermUITests 2m 30s 14db29967

@khoi

khoi commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Accept. I will restrict quit confirmation to a presentable terminal window, make that window visible before presenting, and add coverage through the production search-field wiring. I will run the macOS checks and tests after the fix.

@khoi

khoi commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Changes since last review:

  • Quit confirmation now uses a registered terminal window, so retained settings and diagnostics windows cannot trigger it.
  • The terminal window is restored before the panel appears.
  • Search coverage now mounts the production field and sends a Shift-Return through the AppKit event loop.

Commits: ef58686
Tests:

  • make mac-check
  • make mac-test-e2e
  • make mac-scan-dead-code
  • make mac-inspect-dependencies
  • make web-install web-check web-test web-build
  • make docs-install docs-check docs-validate docs-build
  • make mac-test: 1,700 passed; 3 unrelated macOS 26.5.1 Edit-menu assertions failed

Only terminal windows now trigger the confirmation flow, so retained
settings and diagnostics windows cannot block an ordinary quit. The
selected terminal window is restored before the panel appears.

The search regression mounts the production field and runs its app event
through a modal loop, covering the AppKit modifier path without changing
the process menu state.
@khoi
khoi force-pushed the fix/macos-quit-search-parity branch from ef58686 to c1e86c7 Compare August 8, 2026 12:43
@khoi

khoi commented Aug 8, 2026

Copy link
Copy Markdown
Contributor Author

Changes since last review:

  • Scoped the production search-field event regression to its test window with NSApp.runModal(for:) and NSApp.stopModal().
  • This preserves the NSApp.currentEvent path while preventing AppKit from adding system Edit-menu items for later tests.

Commit: c1e86c7

Tests:

  • Focused xcodebuild: searchFieldShiftReturnUsesProductionWiring() followed by all 24 SupatermMenuControllerTests — passed
  • make mac-check — passed
  • make mac-test — 1703 passed, 0 failed
  • Pre-push gauntlet — passed: dependency inspection, dead-code scan, mac tests, web check/tests, docs check/validate/build

@khoi
khoi merged commit 3239a85 into main Aug 8, 2026
19 checks passed
@khoi
khoi deleted the fix/macos-quit-search-parity branch August 8, 2026 12:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant