Skip to content

Lay out standalone quit confirmation before display - #9620

Merged
austinywang merged 2 commits into
mainfrom
issue-9595-ghost-quit-dialog-v2
Aug 25, 2026
Merged

austinywang merged 2 commits into
mainfrom
issue-9595-ghost-quit-dialog-v2

Conversation

@austinywang

@austinywang austinywang commented Aug 5, 2026 •

Copy link
Copy Markdown
Contributor

Closes #9595

Summary

  • force NSAlert to resolve its lazy layout before the standalone raw window is shown
  • preserve the asynchronous presenter and the no-nested-modal-loop invariant from Avoid nested quit confirmation modal loops #6461
  • add behavior-level coverage for the malformed placeholder button geometry

Root cause

The no-host-window fallback orders alert.window front while NSAlert still has its lazy placeholder layout. On macOS 26.4.1 that left the standalone panel at 260×328, with vertically stacked full-width buttons. NSAlert.layout() is the AppKit API that forces immediate layout; using runModal() here would reintroduce the nested modal-loop hang that #6461 removed.

The normal host-window path is unchanged and still uses beginSheetModal(for:).

Regression proof

  • Test-only commit 533bbb47e9 adds the standalone button-alignment assertion.
  • Local before fix: focused suite reported 4 passes and 1 failure; the two buttons were separated by 80 points vertically.
  • CI on the test-only SHA reproduced the failure with a 71-point separation at the new assertion: run 30969717154, shard job 92191739399. The app-host wrapper currently tolerates ordinary Swift Testing failures and printed All failures are expected, treating as pass, so the GitHub job is marked successful despite the recorded test failure.
  • Fix commit c98c6708a0 adds alert.layout() before standalone presentation.
  • Local after fix: all 5 focused tests pass, including both sheet and standalone assertions that runModal() is never called.

Validation

  • Tagged Debug build succeeded on pushed HEAD c98c6708a0: dogfood build.
  • Hidden-host repro: confirmed zero on-screen tagged windows, sent Ctrl+D to the sole terminal, and captured the standalone alert at 260×250, modal layer 8, alpha 1.0.
  • Escape dismissed the standalone panel and ran the cancellation recovery path, respawning the sole terminal.
  • git diff --check passes; the PR contains exactly the presenter and its existing test file.
  • ./scripts/lint-pbxproj-test-wiring.sh passes for all 652 test files.

Visual evidence

A trustworthy screenshot/video could not be recorded. The cloud Mac run 30968244690 reached its SSH hold but never appeared in Tailscale or the Admin API. Local Sky CUA lacks its executable/app-server, the alternate CUA daemon is unavailable, and macOS Screen Recording is denied for the available helper. Runtime geometry and window-layer metadata were captured directly instead.

Localization

No user-facing strings changed. The changed Swift diff was audited for added English literals; no localization catalog updates are needed.


View with [code]smith Autofix with [code]smith
Need help on this PR? Tag @codesmith-bot with what you need. Autofix is disabled.


Summary by cubic

Fixes the standalone quit confirmation dialog on macOS by calling NSAlert.layout() before the window is shown. Prevents stacked, full-width buttons and keeps the async presenter (no nested modal loops). Addresses #9595.

  • Bug Fixes
    • Call alert.layout() in the standalone path before ordering the window front to avoid placeholder button geometry.
    • Add a behavior test asserting the two buttons have the same midY (no vertical stack).

Written for commit c98c670. Summary will update on new commits.

Review in cubic

Summary by CodeRabbit

  • Bug Fixes

    • Improved the quit confirmation alert’s presentation so its buttons are correctly aligned when displayed.
    • Ensured the alert layout is fully resolved before the window becomes visible.
  • Tests

    • Added coverage confirming the quit confirmation alert’s buttons appear horizontally aligned.

@cursor

cursor Bot commented Aug 5, 2026

Copy link
Copy Markdown

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

@coderabbitai

coderabbitai Bot commented Aug 5, 2026 •

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The standalone quit confirmation presenter now performs an alert layout pass before presentation. Its test verifies that the two alert buttons have vertically aligned centers.

Changes

Quit confirmation layout

Layer / File(s) Summary
Standalone alert button alignment
Sources/QuitConfirmationAlertPresenter.swift, cmuxTests/QuitConfirmationAlertPresenterTests.swift
The presenter lays out the alert before configuring and presenting it. The test verifies that both buttons have vertically aligned centers within 0.5 points.

Estimated code review effort: 1 (Trivial) | ~2 minutes

Suggested reviewers: lawrencecchen

🚥 Pre-merge checks | ✅ 25
✅ Passed checks (25 passed)
Check name Status Explanation
Linked Issues check ✅ Passed The changes address [#9595] by forcing NSAlert.layout() before standalone display, preserving asynchronous presentation, and adding regression coverage.
Out of Scope Changes check ✅ Passed All reported changes support the linked issue and PR objectives; no unrelated code changes are identified.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Cmux Swift Actor Isolation ✅ Passed The production diff only adds alert.layout() inside the existing @MainActor presenter; it adds no models, protocols, Sendable types, or background UI access. The other changes are tests.
Cmux Swift Blocking Runtime ✅ Passed The production diff adds only alert.layout() before display. The test adds geometry assertions only. No semaphore, wait, sleep, delayed dispatch, sync, polling, or lock was introduced.
Cmux Browser Automation Off-Main ✅ Passed The full PR range changes only QuitConfirmationAlertPresenter.swift and its test; no browser socket, policy, worker-router, WebKit, or AppKit automation paths are modified.
Cmux Expensive Synchronous Load ✅ Passed The production diff only adds synchronous NSAlert.layout() in presentStandalone(); it adds no agent-history loader, file scan, JSON/transcript parse, or per-record syscall.
Cmux Cache Substitution Correctness ✅ Passed The PR only adds NSAlert.layout() in transient standalone UI presentation and a geometry test; it does not replace an authoritative read with a cache in persistence, history, undo, or snapshot code.
Cmux No Hacky Sleeps ✅ Passed The two-commit PR changes only Swift files; the rule scopes non-Swift runtime code and explicitly routes Swift timing to a separate check.
Cmux Algorithmic Complexity ✅ Passed The production diff adds only alert.layout() before fixed two-button handling; it adds no collection scan, nested loop, sort, filter, join, or slower scalable algorithm.
Cmux Swift Concurrency ✅ Passed The PR adds only synchronous AppKit layout and a geometry assertion; it introduces no Dispatch, Combine, Task, async/await, or new completion-handler pattern.
Cmux Swift @Concurrent ✅ Passed The full PR adds only synchronous alert.layout() and geometry assertions; it adds no async, nonisolated, Task, or @concurrent code, and the presenter remains @MainActor.
Cmux Swift Package Boundaries ✅ Passed The PR adds only alert.layout() to an AppKit NSAlert presenter; this is small UI/AppKit and app-lifecycle glue, not independently reusable domain logic requiring a SwiftPM target.
Cmux Swiftpm Lockfiles ✅ Passed The PR changes only Sources/QuitConfirmationAlertPresenter.swift and its test; no Package.swift, Package.resolved, Xcode project, .gitignore, workflow, or dependency path changed.
Cmux Swift Logging ✅ Passed The production diff adds only alert.layout() and contains no print, debugPrint, dump, NSLog, file logging, Logger declaration, or sensitive-data logging.
Cmux User-Facing Error Privacy ✅ Passed Production diff adds only alert.layout(); user-facing alert text is unchanged, and added geometry text is test-only.
Cmux Full Internationalization ✅ Passed The two-commit diff only adds NSAlert.layout() and test geometry assertions; it adds no user-facing text or catalog/web changes, and existing alert text already uses localized keys with catalog ent...
Cmux Swiftui State Layout ✅ Passed The PR changes only AppKit NSAlert presentation and AppKit tests; no SwiftUI state, layout measurement, lazy-row store reference, or render-time mutation is introduced.
Cmux Architecture Rethink ✅ Passed The two-commit diff adds only NSAlert.layout() before existing standalone presentation and a geometry test; it adds no timing workaround, state owner, duplicate wiring, or lifecycle split.
Cmux Swift Auxiliary Window Close Shortcuts ✅ Passed The PR only calls alert.layout() on an existing NSAlert window and adds test-only geometry coverage; it adds no window or shortcut routing, and the auxiliary-window lint passes.
Cmux Source Artifacts ✅ Passed The PR changes only a product Swift source file and a Swift test file; no logs, media, caches, scratch directories, or other source-control artifacts appear in the diff.
Cmux No Test Or Debug Seam In Production Source ✅ Passed The production diff adds only alert.layout() in presentStandalone(); it adds no test/debug seam, widened visibility, or test-build-guarded member.
Cmux No Ambient Global State ✅ Passed The production diff adds only alert.layout() inside the existing private presentStandalone() method; it adds no global function, mutable global, static namespace, or singleton.
Title check ✅ Passed The title clearly and concisely describes the main change: laying out the standalone quit confirmation before display.
Description check ✅ Passed The description clearly covers the change, root cause, testing, regression proof, validation, and unavailable visual evidence.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch issue-9595-ghost-quit-dialog-v2

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@austinywang
austinywang merged commit 8c6143b into main Aug 25, 2026
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Quit confirmation dialog renders as a washed-out, mispositioned 'ghost' window (regressed reachability from #9492)

1 participant