Repository navigation
Avoid nested quit confirmation modal loops - #6461
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughIntroduces ChangesQuit Confirmation Alert Presenter Refactor
Sequence Diagram(s)sequenceDiagram
participant User
participant AppDelegate
participant QuitConfirmationAlertPresenter
participant HostWindow
participant NSApp
User->>AppDelegate: Cmd+Q or applicationShouldTerminate(_:)
AppDelegate->>AppDelegate: Check pendingTerminateReply()
alt Presenter already active or dirty workspaces
AppDelegate->>NSApp: return .terminateLater or .terminateCancel
else Proceed with confirmation
AppDelegate->>QuitConfirmationAlertPresenter: presentQuitConfirmationAlert(ownsTerminateRequest:)
QuitConfirmationAlertPresenter->>QuitConfirmationAlertPresenter: activate app
alt Host window available
QuitConfirmationAlertPresenter->>HostWindow: beginSheetModal(completionHandler:)
User->>HostWindow: Click Quit or Cancel
HostWindow-->>QuitConfirmationAlertPresenter: completion(response, suppressionState)
else No host window
QuitConfirmationAlertPresenter->>QuitConfirmationAlertPresenter: Present standalone panel
User->>QuitConfirmationAlertPresenter: Click Quit or Cancel
QuitConfirmationAlertPresenter-->>QuitConfirmationAlertPresenter: finish() → completion()
end
QuitConfirmationAlertPresenter->>AppDelegate: completion(response, suppressionState)
AppDelegate->>AppDelegate: handleApplicationTerminateQuitConfirmationResponse()
alt User confirmed Quit
AppDelegate->>AppDelegate: Set isTerminatingApp, apply suppression
AppDelegate->>NSApp: replyToTerminate(true)
else User cancelled
AppDelegate->>NSApp: replyToTerminate(false)
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (3 errors, 1 warning)
✅ Passed checks (19 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
03f93e3 to
853726d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@cmuxTests/ShortcutAndCommandPaletteTests.swift`:
- Around line 1951-1979: The existing test
testPresenterUsesSheetCompletionWithoutRunningNestedModalLoop only covers the
sheet presentation path. Add a new test method to cover the no-host standalone
fallback scenario by creating a QuitConfirmationAlertPresenter with a
presentingWindowProvider that returns nil, then verify that the alert spy never
calls runModal() and that the completion handler still fires when the presenter
is presented in standalone mode (through button click or window close).
In `@Sources/AppDelegate.swift`:
- Around line 502-586: Extract the entire QuitConfirmationAlertPresenter class
(including all its methods, properties, and extensions) from AppDelegate.swift
and move it to a new dedicated file named
Sources/QuitConfirmationAlertPresenter.swift. This self-contained class with its
NSWindowDelegate conformance is independently testable and should not contribute
to further bloating the large AppDelegate.swift file. After moving the class,
ensure no import statements or other code changes are needed in the new file,
and verify that AppDelegate.swift can still reference and use
QuitConfirmationAlertPresenter without any compilation errors.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 78e18276-fe46-4af4-b46a-5f5c3c476619
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/ShortcutAndCommandPaletteTests.swift
| func testPresenterUsesSheetCompletionWithoutRunningNestedModalLoop() { | ||
| let alert = QuitConfirmationAlertSpy() | ||
| let hostWindow = NSWindow( | ||
| contentRect: NSRect(x: 0, y: 0, width: 480, height: 320), | ||
| styleMask: [.titled], | ||
| backing: .buffered, | ||
| defer: false | ||
| ) | ||
|
|
||
| var completedResponse: NSApplication.ModalResponse? | ||
| var completedSuppressionState: NSControl.StateValue? | ||
| let presenter = QuitConfirmationAlertPresenter( | ||
| alert: alert, | ||
| presentingWindowProvider: { hostWindow } | ||
| ) { response, suppressionState in | ||
| completedResponse = response | ||
| completedSuppressionState = suppressionState | ||
| } | ||
|
|
||
| presenter.present() | ||
|
|
||
| XCTAssertTrue(alert.didBeginSheetModal) | ||
| XCTAssertFalse(alert.didRunModal) | ||
| XCTAssertNil(completedResponse) | ||
|
|
||
| alert.capturedSheetCompletion?(.alertFirstButtonReturn) | ||
|
|
||
| XCTAssertEqual(completedResponse, .alertFirstButtonReturn) | ||
| XCTAssertEqual(completedSuppressionState, .off) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Cover the no-host standalone fallback.
This test only exercises the sheet path. A future change could reintroduce runModal() inside presentStandalone() and still pass the source scan, because the scan only checks the two AppDelegate call-site bodies. Add a provider returning nil or a host with an attached sheet and assert the spy never runs runModal() while completion still fires through the standalone button/window-close path.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@cmuxTests/ShortcutAndCommandPaletteTests.swift` around lines 1951 - 1979, The
existing test testPresenterUsesSheetCompletionWithoutRunningNestedModalLoop only
covers the sheet presentation path. Add a new test method to cover the no-host
standalone fallback scenario by creating a QuitConfirmationAlertPresenter with a
presentingWindowProvider that returns nil, then verify that the alert spy never
calls runModal() and that the completion handler still fires when the presenter
is presented in standalone mode (through button click or window close).
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| isTerminatingApp = false | ||
| clearMarkedRemoteTmuxKills() | ||
| StartupBreadcrumbLog.append("appDelegate.shouldTerminate.reply", fields: ["shouldQuit": "0"]) | ||
| } | ||
| replyToTerminateOnce(shouldQuit) |
There was a problem hiding this comment.
isTerminatingApp = false with outdated comment
In the old code, isTerminatingApp = true was set unconditionally at the top of applicationShouldTerminate (before the dialog), so the cancel branch needed to reset it. In the new code, isTerminatingApp is only set to true inside prepareForConfirmedAppTermination(), which is called only on confirmation — so by the time this else branch runs, isTerminatingApp is already false. The isTerminatingApp = false here is a no-op, and the comment ("Reset so that the next quit attempt can show the dialog again") is now misleading — the re-entry gate is activeQuitConfirmationAlertPresenter = nil (cleared in presentQuitConfirmationAlert's wrapper closure), not this flag.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| super.init() | ||
| } | ||
|
|
||
| static func makeAlert() -> NSAlert { |
There was a problem hiding this comment.
makeAlert() is a private factory helper with no caller outside QuitConfirmationAlertPresenter; it should be private rather than internal to avoid unintended visibility through @testable import.
| static func makeAlert() -> NSAlert { | |
| private static func makeAlert() -> NSAlert { |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| buttons[1].action = #selector(cancelQuit) | ||
| } | ||
|
|
||
| let window = alert.window | ||
| window.delegate = self | ||
| window.level = .modalPanel | ||
| window.center() | ||
| window.makeKeyAndOrderFront(nil) | ||
| } | ||
|
|
||
| @objc private func confirmQuit() { | ||
| finish(.alertFirstButtonReturn) | ||
| } | ||
|
|
||
| @objc private func cancelQuit() { | ||
| finish(.alertSecondButtonReturn) | ||
| } | ||
|
|
||
| func windowWillClose(_ notification: Notification) { | ||
| finish(.alertSecondButtonReturn) | ||
| } | ||
|
|
||
| private func finish(_ response: NSApplication.ModalResponse) { |
There was a problem hiding this comment.
presentStandalone fallback path has no test coverage
testPresenterUsesSheetCompletionWithoutRunningNestedModalLoop only exercises the beginSheetModal branch. The standalone fallback — used when presentingWindowProvider() returns nil or the host window already has an attached sheet — is untested. In the terminate-owned case (ownsTerminateRequest: true), a regression where button target/action are not set correctly would leave AppKit indefinitely waiting on .terminateLater with no recovery path. A test passing presentingWindowProvider: { nil } and asserting button clicks deliver the correct response and didRunModal == false would close this gap.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 535-537: The activation condition in AppDelegate.swift currently
checks only for the `.regular` activation policy before showing the quit
confirmation sheet. Change this condition from `NSApp.activationPolicy() ==
.regular` to `NSApp.activationPolicy() != .prohibited` to ensure the app
activates and brings the quit confirmation sheet to the foreground for all
non-prohibited policies, including `.accessory` mode.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 26515b15-3807-4bcc-a356-91982fc6da99
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxTests/ShortcutAndCommandPaletteTests.swift
| if NSApp.activationPolicy() == .regular { | ||
| NSApp.activate(ignoringOtherApps: true) | ||
| } |
There was a problem hiding this comment.
Activate all non-prohibited app policies before showing quit confirmation sheet
The app currently activates only .regular mode before displaying the quit-confirmation alert. When running in .accessory mode (menu-bar-only via MenuBarExtraController), the app fails to activate before the quit sheet appears, leaving it behind the user's current application. The active-presenter guard then blocks repeated quit attempts. Change the activation condition to activate all non-prohibited policies (activationPolicy != .prohibited) to ensure the quit confirmation is always presented in the foreground.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Sources/AppDelegate.swift` around lines 535 - 537, The activation condition
in AppDelegate.swift currently checks only for the `.regular` activation policy
before showing the quit confirmation sheet. Change this condition from
`NSApp.activationPolicy() == .regular` to `NSApp.activationPolicy() !=
.prohibited` to ensure the app activates and brings the quit confirmation sheet
to the foreground for all non-prohibited policies, including `.accessory` mode.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/AppDelegate.swift (1)
6085-6240: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftMove the Open Diff Viewer orchestration out of
AppDelegate.This adds diff-context lookup, baseline-store parsing, cwd selection, and process-launch setup to an already oversized
AppDelegate. Keep the shortcut wrapper here, but move the reusable orchestration into a same-module helper such asOpenDiffViewerLauncher/OpenDiffViewerContextResolverso it can be tested and maintained independently.As per coding guidelines, production Swift files over 800 lines and app-root features with separable core logic should be flagged rather than further mixing responsibilities in
Sources/AppDelegate.swift.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/AppDelegate.swift` around lines 6085 - 6240, Create a new helper class (such as OpenDiffViewerLauncher or OpenDiffViewerContextResolver) in the same module and move the orchestration logic out of AppDelegate to reduce its responsibilities. Move the private functions focusedAgentDiffContext, latestAgentTurnDiffRepoRoot, agentTurnDiffBaselineStoreURL, normalizedOpenDiffViewerIdentifier, normalizedOpenDiffViewerSessionId, normalizedOpenDiffViewerPath, and launchDiffViewerProcess into the new helper class along with the main openDiffViewerForFocusedWorkspace logic (keeping only the preferAgentContext parameter selection). In AppDelegate, keep only the thin public wrapper methods openDiffViewerForFocusedWorkspace and openDirectoryDiffViewerForFocusedWorkspace that delegate to the new helper class instance. This separates concerns, makes the code independently testable, and reduces AppDelegate's bloat.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/AppDelegate.swift`:
- Around line 6085-6240: Create a new helper class (such as
OpenDiffViewerLauncher or OpenDiffViewerContextResolver) in the same module and
move the orchestration logic out of AppDelegate to reduce its responsibilities.
Move the private functions focusedAgentDiffContext, latestAgentTurnDiffRepoRoot,
agentTurnDiffBaselineStoreURL, normalizedOpenDiffViewerIdentifier,
normalizedOpenDiffViewerSessionId, normalizedOpenDiffViewerPath, and
launchDiffViewerProcess into the new helper class along with the main
openDiffViewerForFocusedWorkspace logic (keeping only the preferAgentContext
parameter selection). In AppDelegate, keep only the thin public wrapper methods
openDiffViewerForFocusedWorkspace and openDirectoryDiffViewerForFocusedWorkspace
that delegate to the new helper class instance. This separates concerns, makes
the code independently testable, and reduces AppDelegate's bloat.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 57e933f9-e09b-4cb2-b907-3c4c062c3a49
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmux.xcodeproj/project.pbxproj
This branch was successfully deployed
1 participant
Summary
Addresses the high-frequency quit-path Sentry hang signature:
https://manaflow.sentry.io/issues/7391555405/
That issue's representative events stop in
NSAlert.runModalunderAppDelegate.applicationShouldTerminate, which means the main thread is sitting in a nested AppKit modal loop during quit confirmation.Changes
runModal()calls with a retainedQuitConfirmationAlertPresenterthat usesbeginSheetModal(for:completionHandler:)when a main window is available.runModal()when no sheet host is available..terminateLatersemantics and callsreply(toApplicationShouldTerminate:)from the async completion.Tests
xcodebuild test -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-sentryquit -only-testing:cmuxTests/QuitConfirmationModalLoopRegressionTests -only-testing:cmuxTests/QuitConfirmationAlertPresenterTests CODE_SIGNING_ALLOWED=NOThe first commit is test-only and fails against the old
runModal()quit-confirmation paths; the second commit makes it pass.Dogfood
Tagged build:
quitcfThe tagged dev app builds and launches. The exact release/stable quit-confirmation dialog path cannot be fully UI-preflighted in a tagged dev build because existing policy intentionally skips quit confirmation for dev builds (
QuitConfirmationStore.shouldShowConfirmation(... isDevBuild: true) == false). I verified the presenter behavior with the focused unit test and verified the tagged app launches and responds on its debug socket.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Prevents UI hangs on quit by removing nested AppKit modal loops and unifying quit confirmation into a non-blocking presenter. Avoids duplicate dialogs and returns the right terminate reply for both Cmd+Q and app-initiated quits.
Bug Fixes
Refactors
Written for commit 2f41a34. Summary will update on new commits.
Summary by CodeRabbit