Repository navigation
Fix Return on Cmd+Ctrl+W close confirmation - #1279
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThe AppDelegate close-window confirmation flow now sets the alert's default button and focuses the alert/window without mutating the close button's keyEquivalent; a UI test was added to verify that pressing Return confirms the alert and closes the window. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant App as AppDelegate
participant AlertWin as Alert Window
participant MainWin as Main Window
User->>App: Cmd+Ctrl+W (request close)
App->>AlertWin: create/show alert
App->>AlertWin: set defaultButtonCell = closeButton.cell
App->>AlertWin: set initialFirstResponder = alertWindow
App->>AlertWin: async -> make closeButton first responder
Note right of AlertWin: Alert shown with Close button focused
User->>AlertWin: Press Return
AlertWin->>App: trigger close action
App->>MainWin: close main window
MainWin-->>User: window closed
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Comment |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes a regression where pressing Return did not confirm the Cmd+Ctrl+W close-window dialog. The previous implementation accidentally broke Return by overriding the button's Key changes:
Notes:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant AppDelegate
participant NSAlert
participant AlertWindow
participant MainQueue as DispatchQueue.main
User->>AppDelegate: Cmd+Ctrl+W
AppDelegate->>NSAlert: create alert (Close / Cancel)
AppDelegate->>AlertWindow: defaultButtonCell = closeButton.cell
AppDelegate->>AlertWindow: initialFirstResponder = closeButton
AppDelegate->>MainQueue: async { makeKeyAndOrderFront + makeFirstResponder(closeButton) }
AppDelegate->>NSAlert: runModal() [starts modal loop]
NSAlert->>AlertWindow: show window, make key
MainQueue-->>AlertWindow: makeFirstResponder(closeButton) [executes in modal run loop]
User->>AlertWindow: press Return
AlertWindow->>NSAlert: defaultButtonCell fires → .alertFirstButtonReturn
NSAlert-->>AppDelegate: runModal() returns .alertFirstButtonReturn
AppDelegate->>AppDelegate: confirmCloseMainWindow → true
AppDelegate->>AlertWindow: performClose(nil)
AlertWindow-->>User: window closed
Last reviewed commit: 39e8afb |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| waitForCloseWindowAlertToDismiss(app: app, timeout: 5.0), | ||
| "Expected Return to dismiss the close window confirmation alert" | ||
| ) | ||
| XCTAssertFalse(app.windows.firstMatch.exists, "Expected Return to confirm window close") |
There was a problem hiding this comment.
No timeout on window-close assertion — potential flakiness
waitForCloseWindowAlertToDismiss only waits until the alert UI element is gone. After that returns, window.performClose(nil) still needs to propagate through the accessibility API before app.windows.firstMatch.exists reads false. On a slow CI machine this assertion can fire before the window has actually closed, producing an intermittent failure.
Consider adding a small wait loop analogous to the other helpers:
// Wait for the app window itself to close after the alert is dismissed
let windowClosedByDeadline = {
let deadline = Date().addingTimeInterval(5.0)
while Date() < deadline {
if !app.windows.firstMatch.exists { return true }
RunLoop.current.run(until: Date().addingTimeInterval(0.05))
}
return !app.windows.firstMatch.exists
}()
XCTAssertFalse(app.windows.firstMatch.exists, "Expected Return to confirm window close")Or, at minimum, use XCTNSPredicateExpectation / expectation(for:) with a timeout before asserting.
| DispatchQueue.main.async { | ||
| alertWindow.makeKeyAndOrderFront(nil) | ||
| _ = alertWindow.makeFirstResponder(closeButton) | ||
| } |
There was a problem hiding this comment.
makeKeyAndOrderFront inside async is redundant
NSAlert.runModal() already calls the equivalent of makeKeyAndOrderFront internally when it starts the modal session, so the explicit call here is a no-op in normal conditions. It is harmless, but it introduces a subtle conceptual hazard: if runModal() returns (user clicks a button) extremely quickly — before the async block executes — the block will call makeKeyAndOrderFront on a window that is already being torn down by the modal session, which can cause unexpected visual behaviour.
The focus goal is fully achieved by initialFirstResponder (set synchronously before runModal()) together with the makeFirstResponder async call. Consider dropping the redundant makeKeyAndOrderFront:
| DispatchQueue.main.async { | |
| alertWindow.makeKeyAndOrderFront(nil) | |
| _ = alertWindow.makeFirstResponder(closeButton) | |
| } | |
| DispatchQueue.main.async { | |
| _ = alertWindow.makeFirstResponder(closeButton) | |
| } |
| "Expected Cmd+Ctrl+W to show the close window confirmation alert" | ||
| ) | ||
|
|
||
| app.typeKey(XCUIKeyboardKey.return.rawValue, modifierFlags: []) |
There was a problem hiding this comment.
Prefer the typed overload of typeKey
XCUIKeyboardKey.return.rawValue wraps and immediately unwraps the key constant; the API has a direct overload that accepts XCUIKeyboardKey:
| app.typeKey(XCUIKeyboardKey.return.rawValue, modifierFlags: []) | |
| app.typeKey(.return, modifierFlags: []) |
|
Addressed the Greptile feedback in |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxUITests/CloseWindowConfirmDialogUITests.swift`:
- Around line 78-87: The helper waitForCloseWindowAlertToDismiss currently
returns as soon as the confirmation alert disappears, which can race with the
actual window closing; update waitForCloseWindowAlertToDismiss(app:
XCUIApplication, timeout: TimeInterval) so the polling loop only returns true
when BOTH isCloseWindowAlertPresent(app: app) is false AND
app.windows.firstMatch.exists is false (i.e., wait for the alert to dismiss and
app.windows.firstMatch.exists == false in the same loop iteration), keeping the
same timeout and RunLoop sleep behavior and preserving the final return value
based on those two conditions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4d3beb4a-5674-406f-9d1f-70c9353bbad1
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxUITests/CloseWindowConfirmDialogUITests.swift
|
Followed up on the remaining CodeRabbit note. I didn't fold window-close waiting into |
* Add close-window return-key regression test * Focus close-window confirmation button * Keep Return on close-window alert * Address review feedback
This branch was successfully deployed
1 participant
Summary
Testing
./scripts/reload.sh --tag task-close-dialog-enter-focusTask
Summary by cubic
Fixes a regression where pressing Return didn’t confirm the close-window dialog after Cmd+Ctrl+W. The Close button is now focused so Return works, and a UI test verifies the alert dismisses and the window closes.
testReturnConfirmsCloseWindowDialogXCUITest with helpers to wait for the alert to dismiss and the main window to close.Written for commit 529008c. Summary will update on new commits.
Summary by CodeRabbit