Repository navigation
Summarize multi-workspace close confirmation - #1329
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
📝 WalkthroughWalkthroughAdds a batch workspace-closing flow: a public TabManager API to close multiple workspaces with a single confirmation, UI integration and selection-sync helpers in ContentView, AppDelegate title recognition for the new dialog, localized strings, and unit + UI tests covering the aggregated confirmation behavior. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as ContentView
participant TM as TabManager
participant Dialog as Confirmation Dialog
participant Window as App/Window
User->>UI: Select multiple workspaces & invoke close
UI->>TM: closeWorkspacesWithConfirmation(workspaceIds, allowPinned)
TM->>TM: orderedClosableWorkspaces() / build CloseWorkspacesPlan
TM->>Dialog: request single aggregated confirmation
alt user accepts
Dialog->>TM: confirmed
TM->>TM: closeWorkspaceIfRunningProcess(..., requiresConfirmation: false) for each
TM->>Window: remove workspace(s)
Window->>UI: update state/UI
else user cancels
Dialog->>TM: cancelled
TM->>UI: no changes
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![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/CloseWorkspacesConfirmDialogUITests.swift`:
- Around line 175-181: The element queries in closeWorkspacesDialog and
closeWorkspacesAlert are matching an accessibility identifier but the UI shows
the localized visible text; replace the containing(.staticText, identifier:
"Close workspaces?") calls in both closeWorkspacesDialog and
closeWorkspacesAlert with element(matching: NSPredicate(format: "ANY
staticTexts.label == %@", "Close workspaces?")) so the query matches by visible
label text rather than identifier (keep .firstMatch).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 510a2d35-8e8e-4b41-98ac-e1536a875630
📒 Files selected for processing (7)
Resources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/TabManager.swiftSources/cmuxApp.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swiftcmuxUITests/CloseWorkspacesConfirmDialogUITests.swift
Comment on lines
+175
to
+181
| private func closeWorkspacesDialog(app: XCUIApplication) -> XCUIElement { | ||
| app.dialogs.containing(.staticText, identifier: "Close workspaces?").firstMatch | ||
| } | ||
|
|
||
| private func closeWorkspacesAlert(app: XCUIApplication) -> XCUIElement { | ||
| app.alerts.containing(.staticText, identifier: "Close workspaces?").firstMatch | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for how the close-workspaces dialog title is set (localized string key)
rg -n "closeWorkspaces.title\|Close workspaces" --type swift -g '!**/cmuxUITests/**' | head -20Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Read the test file to see context around lines 175-181
cat -n cmuxUITests/CloseWorkspacesConfirmDialogUITests.swift | sed -n '160,190p'Repository: manaflow-ai/cmux
Length of output: 1027
🏁 Script executed:
# Search for where the close workspaces dialog/alert is implemented in the app code
rg -n "Close workspaces" --type swift | head -30Repository: manaflow-ai/cmux
Length of output: 873
🏁 Script executed:
# Look for how dialogs/alerts are created in the codebase to understand the pattern
rg -n "\.dialogs\.|\.alerts\." --type swift -B 2 -A 2 | head -40Repository: manaflow-ai/cmux
Length of output: 3884
Element locator should match by visible text, not accessibility identifier.
The containing(.staticText, identifier:) query parameter matches accessibility identifiers, but "Close workspaces?" is visible text from a localized string (defined in app code as String(localized: "dialog.closeWorkspaces.title", defaultValue: "Close workspaces?")). Line 144 of the same file demonstrates the correct approach: app.staticTexts["Close workspaces?"] directly accesses by text content. Update lines 176 and 180 to use text-based matching with a predicate:
private func closeWorkspacesDialog(app: XCUIApplication) -> XCUIElement {
app.dialogs.element(matching: NSPredicate(format: "ANY staticTexts.label == %@", "Close workspaces?")).firstMatch
}
private func closeWorkspacesAlert(app: XCUIApplication) -> XCUIElement {
app.alerts.element(matching: NSPredicate(format: "ANY staticTexts.label == %@", "Close workspaces?")).firstMatch
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxUITests/CloseWorkspacesConfirmDialogUITests.swift` around lines 175 -
181, The element queries in closeWorkspacesDialog and closeWorkspacesAlert are
matching an accessibility identifier but the UI shows the localized visible
text; replace the containing(.staticText, identifier: "Close workspaces?") calls
in both closeWorkspacesDialog and closeWorkspacesAlert with element(matching:
NSPredicate(format: "ANY staticTexts.label == %@", "Close workspaces?")) so the
query matches by visible label text rather than identifier (keep .firstMatch).
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f0d15b96eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // equivalents working and avoid surprising actions while the confirmation is up. | ||
| let closeConfirmationTitles = [ | ||
| String(localized: "dialog.closeWorkspace.title", defaultValue: "Close workspace?"), | ||
| String(localized: "dialog.closeWorkspaces.title", defaultValue: "Close workspaces?"), |
There was a problem hiding this comment.
Including dialog.closeWorkspaces.title in closeConfirmationTitles causes handleCustomShortcut to route Cmd+D to the alert's Close button unconditionally (the matchShortcut(... "d") branch), even when the alert did not opt into Cmd+D. For non-window multi-workspace closes, closeWorkspacesPlan sets acceptCmdD to false, so pressing Cmd+D on that new summary dialog can unexpectedly confirm a destructive close instead of doing nothing.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 7 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:7744">
P1: Gate Cmd+D handling on the dialog’s `acceptCmdD` intent instead of title matching. Including the multi-workspace title here allows Cmd+D to confirm non-window multi-workspace closes even when that confirmation flow explicitly disables Cmd+D.</violation>
</file>
<file name="cmuxTests/CmuxWebViewKeyEquivalentTests.swift">
<violation number="1" location="cmuxTests/CmuxWebViewKeyEquivalentTests.swift:5091">
P3: These tests assume `addWorkspace()` inserts after the current tab, but `addWorkspace` uses the user’s `newWorkspacePlacement` preference to choose the insert index. If the preference is `.top` or `.end`, `tabs[0]` and the bullet list order change, making the expected message flaky. Pass a placement override (or set the default) so the order is deterministic.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // equivalents working and avoid surprising actions while the confirmation is up. | ||
| let closeConfirmationTitles = [ | ||
| String(localized: "dialog.closeWorkspace.title", defaultValue: "Close workspace?"), | ||
| String(localized: "dialog.closeWorkspaces.title", defaultValue: "Close workspaces?"), |
There was a problem hiding this comment.
P1: Gate Cmd+D handling on the dialog’s acceptCmdD intent instead of title matching. Including the multi-workspace title here allows Cmd+D to confirm non-window multi-workspace closes even when that confirmation flow explicitly disables Cmd+D.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/AppDelegate.swift, line 7744:
<comment>Gate Cmd+D handling on the dialog’s `acceptCmdD` intent instead of title matching. Including the multi-workspace title here allows Cmd+D to confirm non-window multi-workspace closes even when that confirmation flow explicitly disables Cmd+D.</comment>
<file context>
@@ -7741,6 +7741,7 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
// equivalents working and avoid surprising actions while the confirmation is up.
let closeConfirmationTitles = [
String(localized: "dialog.closeWorkspace.title", defaultValue: "Close workspace?"),
+ String(localized: "dialog.closeWorkspaces.title", defaultValue: "Close workspaces?"),
String(localized: "dialog.closeTab.title", defaultValue: "Close tab?"),
String(localized: "dialog.closeOtherTabs.title", defaultValue: "Close other tabs?"),
</file context>
Comment on lines
+5091
to
+5092
| let second = manager.addWorkspace() | ||
| let third = manager.addWorkspace() |
There was a problem hiding this comment.
P3: These tests assume addWorkspace() inserts after the current tab, but addWorkspace uses the user’s newWorkspacePlacement preference to choose the insert index. If the preference is .top or .end, tabs[0] and the bullet list order change, making the expected message flaky. Pass a placement override (or set the default) so the order is deterministic.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/CmuxWebViewKeyEquivalentTests.swift, line 5091:
<comment>These tests assume `addWorkspace()` inserts after the current tab, but `addWorkspace` uses the user’s `newWorkspacePlacement` preference to choose the insert index. If the preference is `.top` or `.end`, `tabs[0]` and the bullet list order change, making the expected message flaky. Pass a placement override (or set the default) so the order is deterministic.</comment>
<file context>
@@ -5084,6 +5084,77 @@ final class TabManagerWorkspaceOwnershipTests: XCTestCase {
+final class TabManagerCloseWorkspacesWithConfirmationTests: XCTestCase {
+ func testCloseWorkspacesWithConfirmationPromptsOnceAndClosesAcceptedWorkspaces() {
+ let manager = TabManager()
+ let second = manager.addWorkspace()
+ let third = manager.addWorkspace()
+ manager.setCustomTitle(tabId: manager.tabs[0].id, title: "Alpha")
</file context>
Suggested change
| let second = manager.addWorkspace() | |
| let third = manager.addWorkspace() | |
| let second = manager.addWorkspace(placementOverride: .afterCurrent) | |
| let third = manager.addWorkspace(placementOverride: .afterCurrent) |
There was a problem hiding this comment.
1 issue found across 4 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:6025">
P2: Validate every parsed workspace index before mapping into `tabs` to avoid out-of-bounds crashes in the UI-test hook.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| guard let lastIndex = indices.last, !indices.isEmpty, lastIndex < tabs.count else { return } | ||
|
|
||
| let selectedIds = Set(indices.map { tabs[$0].id }) |
There was a problem hiding this comment.
P2: Validate every parsed workspace index before mapping into tabs to avoid out-of-bounds crashes in the UI-test hook.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/ContentView.swift, line 6025:
<comment>Validate every parsed workspace index before mapping into `tabs` to avoid out-of-bounds crashes in the UI-test hook.</comment>
<file context>
@@ -5985,6 +5997,40 @@ struct ContentView: View {
+
+ guard let lastIndex = indices.last, !indices.isEmpty, lastIndex < tabs.count else { return }
+
+ let selectedIds = Set(indices.map { tabs[$0].id })
+ selectedTabIds = selectedIds
+ lastSidebarSelectionIndex = lastIndex
</file context>
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmuxUITests/CloseWorkspacesConfirmDialogUITests.swift (1)
7-12: Consider adding tearDown to clean up socket file.The socket file created in setUp is not cleaned up after tests complete. While the unique UUID prevents conflicts between test runs, stale files may accumulate in
/tmp.♻️ Suggested tearDown implementation
override func setUp() { super.setUp() continueAfterFailure = false socketPath = "/tmp/cmux-ui-test-close-workspaces-\(UUID().uuidString).sock" try? FileManager.default.removeItem(atPath: socketPath) } + + override func tearDown() { + try? FileManager.default.removeItem(atPath: socketPath) + super.tearDown() + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/CloseWorkspacesConfirmDialogUITests.swift` around lines 7 - 12, Add a tearDown method to remove the test socket file created in setUp: implement override func tearDown() { ... } that calls try? FileManager.default.removeItem(atPath: socketPath) (or checks existence and removes) and then calls super.tearDown() so the temporary socketPath created in setUp is cleaned up after each test run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift`:
- Around line 5090-5191: The tests use manager.tabs[0] to refer to the first
workspace which is flaky; instead capture and use the explicit workspace IDs
returned by addWorkspace() and the initial workspace creation (e.g., store the
initial id in a variable like firstId = manager.tabs[0].id immediately after
creating the manager or better: let first = manager.tabs[0]; let second =
manager.addWorkspace(); let third = manager.addWorkspace()) and then pass those
explicit ids to setCustomTitle, setSidebarSelectedWorkspaceIds,
closeWorkspacesWithConfirmation, and closeCurrentWorkspaceWithConfirmation so
all references use stable identifiers (refer to addWorkspace, tabs,
setCustomTitle(tabId:), setSidebarSelectedWorkspaceIds(_:),
closeWorkspacesWithConfirmation(_:allowPinned:),
closeCurrentWorkspaceWithConfirmation()).
In `@Sources/ContentView.swift`:
- Around line 6004-6026: In applyUITestSidebarSelectionIfNeeded, the code
currently only validates indices.last before using indices to subscript tabs
which can crash for unsorted values like "3,0"; update the logic to validate
every parsed index (each element of indices) is within 0..<tabs.count (and >= 0)
before mapping to tabs, or alternatively filter out-of-range indices and
deduplicate after validation, then build selectedIds from the safe indices and
assign to selectedTabIds; reference indices, tabs, selectedIds and
selectedTabIds when making the checks and early-return on any invalid input.
---
Nitpick comments:
In `@cmuxUITests/CloseWorkspacesConfirmDialogUITests.swift`:
- Around line 7-12: Add a tearDown method to remove the test socket file created
in setUp: implement override func tearDown() { ... } that calls try?
FileManager.default.removeItem(atPath: socketPath) (or checks existence and
removes) and then calls super.tearDown() so the temporary socketPath created in
setUp is cleaned up after each test run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 779afc09-1bce-4ccb-9439-2925353a6d5e
📒 Files selected for processing (4)
Sources/ContentView.swiftSources/TabManager.swiftcmuxTests/CmuxWebViewKeyEquivalentTests.swiftcmuxUITests/CloseWorkspacesConfirmDialogUITests.swift
Comment on lines
+5090
to
+5191
| let manager = TabManager() | ||
| let second = manager.addWorkspace() | ||
| let third = manager.addWorkspace() | ||
| manager.setCustomTitle(tabId: manager.tabs[0].id, title: "Alpha") | ||
| manager.setCustomTitle(tabId: second.id, title: "Beta") | ||
| manager.setCustomTitle(tabId: third.id, title: "Gamma") | ||
|
|
||
| var prompts: [(title: String, message: String, acceptCmdD: Bool)] = [] | ||
| manager.confirmCloseHandler = { title, message, acceptCmdD in | ||
| prompts.append((title, message, acceptCmdD)) | ||
| return true | ||
| } | ||
|
|
||
| manager.closeWorkspacesWithConfirmation([manager.tabs[0].id, second.id], allowPinned: true) | ||
|
|
||
| let expectedMessage = String( | ||
| format: String( | ||
| localized: "dialog.closeWorkspaces.message", | ||
| defaultValue: "This will close %1$lld workspaces and all of their panels:\n%2$@" | ||
| ), | ||
| locale: .current, | ||
| Int64(2), | ||
| "• Alpha\n• Beta" | ||
| ) | ||
| XCTAssertEqual(prompts.count, 1, "Expected a single confirmation prompt for multi-close") | ||
| XCTAssertEqual( | ||
| prompts.first?.title, | ||
| String(localized: "dialog.closeWorkspaces.title", defaultValue: "Close workspaces?") | ||
| ) | ||
| XCTAssertEqual(prompts.first?.message, expectedMessage) | ||
| XCTAssertEqual(prompts.first?.acceptCmdD, false) | ||
| XCTAssertEqual(manager.tabs.map(\.title), ["Gamma"]) | ||
| } | ||
|
|
||
| func testCloseWorkspacesWithConfirmationKeepsWorkspacesWhenCancelled() { | ||
| let manager = TabManager() | ||
| let second = manager.addWorkspace() | ||
| manager.setCustomTitle(tabId: manager.tabs[0].id, title: "Alpha") | ||
| manager.setCustomTitle(tabId: second.id, title: "Beta") | ||
|
|
||
| var prompts: [(title: String, message: String, acceptCmdD: Bool)] = [] | ||
| manager.confirmCloseHandler = { title, message, acceptCmdD in | ||
| prompts.append((title, message, acceptCmdD)) | ||
| return false | ||
| } | ||
|
|
||
| manager.closeWorkspacesWithConfirmation([manager.tabs[0].id, second.id], allowPinned: true) | ||
|
|
||
| let expectedMessage = String( | ||
| format: String( | ||
| localized: "dialog.closeWorkspacesWindow.message", | ||
| defaultValue: "This will close the current window, its %1$lld workspaces, and all of their panels:\n%2$@" | ||
| ), | ||
| locale: .current, | ||
| Int64(2), | ||
| "• Alpha\n• Beta" | ||
| ) | ||
| XCTAssertEqual(prompts.count, 1) | ||
| XCTAssertEqual( | ||
| prompts.first?.title, | ||
| String(localized: "dialog.closeWindow.title", defaultValue: "Close window?") | ||
| ) | ||
| XCTAssertEqual(prompts.first?.message, expectedMessage) | ||
| XCTAssertEqual(prompts.first?.acceptCmdD, true) | ||
| XCTAssertEqual(manager.tabs.map(\.title), ["Alpha", "Beta"]) | ||
| } | ||
|
|
||
| func testCloseCurrentWorkspaceWithConfirmationUsesSidebarMultiSelection() { | ||
| let manager = TabManager() | ||
| let second = manager.addWorkspace() | ||
| let third = manager.addWorkspace() | ||
| manager.setCustomTitle(tabId: manager.tabs[0].id, title: "Alpha") | ||
| manager.setCustomTitle(tabId: second.id, title: "Beta") | ||
| manager.setCustomTitle(tabId: third.id, title: "Gamma") | ||
| manager.selectWorkspace(second) | ||
| manager.setSidebarSelectedWorkspaceIds([manager.tabs[0].id, second.id]) | ||
|
|
||
| var prompts: [(title: String, message: String, acceptCmdD: Bool)] = [] | ||
| manager.confirmCloseHandler = { title, message, acceptCmdD in | ||
| prompts.append((title, message, acceptCmdD)) | ||
| return false | ||
| } | ||
|
|
||
| manager.closeCurrentWorkspaceWithConfirmation() | ||
|
|
||
| let expectedMessage = String( | ||
| format: String( | ||
| localized: "dialog.closeWorkspaces.message", | ||
| defaultValue: "This will close %1$lld workspaces and all of their panels:\n%2$@" | ||
| ), | ||
| locale: .current, | ||
| Int64(2), | ||
| "• Alpha\n• Beta" | ||
| ) | ||
| XCTAssertEqual(prompts.count, 1, "Expected Cmd+Shift+W path to reuse the multi-close summary dialog") | ||
| XCTAssertEqual( | ||
| prompts.first?.title, | ||
| String(localized: "dialog.closeWorkspaces.title", defaultValue: "Close workspaces?") | ||
| ) | ||
| XCTAssertEqual(prompts.first?.message, expectedMessage) | ||
| XCTAssertEqual(prompts.first?.acceptCmdD, false) | ||
| XCTAssertEqual(manager.tabs.map(\.title), ["Alpha", "Beta", "Gamma"]) |
There was a problem hiding this comment.
Make these tests deterministic with explicit workspace references
These tests rely on manager.tabs[0] after insertions. If workspace insertion order changes (e.g., placement setting), IDs/titles can mismatch and make the tests flaky (including duplicate IDs in close requests).
✅ Suggested fix
func testCloseWorkspacesWithConfirmationPromptsOnceAndClosesAcceptedWorkspaces() {
let manager = TabManager()
+ let first = manager.tabs[0]
let second = manager.addWorkspace()
let third = manager.addWorkspace()
- manager.setCustomTitle(tabId: manager.tabs[0].id, title: "Alpha")
+ manager.setCustomTitle(tabId: first.id, title: "Alpha")
manager.setCustomTitle(tabId: second.id, title: "Beta")
manager.setCustomTitle(tabId: third.id, title: "Gamma")
@@
- manager.closeWorkspacesWithConfirmation([manager.tabs[0].id, second.id], allowPinned: true)
+ manager.closeWorkspacesWithConfirmation([first.id, second.id], allowPinned: true)
@@
func testCloseWorkspacesWithConfirmationKeepsWorkspacesWhenCancelled() {
let manager = TabManager()
+ let first = manager.tabs[0]
let second = manager.addWorkspace()
- manager.setCustomTitle(tabId: manager.tabs[0].id, title: "Alpha")
+ manager.setCustomTitle(tabId: first.id, title: "Alpha")
manager.setCustomTitle(tabId: second.id, title: "Beta")
@@
- manager.closeWorkspacesWithConfirmation([manager.tabs[0].id, second.id], allowPinned: true)
+ manager.closeWorkspacesWithConfirmation([first.id, second.id], allowPinned: true)
@@
func testCloseCurrentWorkspaceWithConfirmationUsesSidebarMultiSelection() {
let manager = TabManager()
+ let first = manager.tabs[0]
let second = manager.addWorkspace()
let third = manager.addWorkspace()
- manager.setCustomTitle(tabId: manager.tabs[0].id, title: "Alpha")
+ manager.setCustomTitle(tabId: first.id, title: "Alpha")
manager.setCustomTitle(tabId: second.id, title: "Beta")
manager.setCustomTitle(tabId: third.id, title: "Gamma")
@@
- manager.setSidebarSelectedWorkspaceIds([manager.tabs[0].id, second.id])
+ manager.setSidebarSelectedWorkspaceIds([first.id, second.id])🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/CmuxWebViewKeyEquivalentTests.swift` around lines 5090 - 5191, The
tests use manager.tabs[0] to refer to the first workspace which is flaky;
instead capture and use the explicit workspace IDs returned by addWorkspace()
and the initial workspace creation (e.g., store the initial id in a variable
like firstId = manager.tabs[0].id immediately after creating the manager or
better: let first = manager.tabs[0]; let second = manager.addWorkspace(); let
third = manager.addWorkspace()) and then pass those explicit ids to
setCustomTitle, setSidebarSelectedWorkspaceIds, closeWorkspacesWithConfirmation,
and closeCurrentWorkspaceWithConfirmation so all references use stable
identifiers (refer to addWorkspace, tabs, setCustomTitle(tabId:),
setSidebarSelectedWorkspaceIds(_:),
closeWorkspacesWithConfirmation(_:allowPinned:),
closeCurrentWorkspaceWithConfirmation()).
Comment on lines
+6004
to
+6026
| private func applyUITestSidebarSelectionIfNeeded(tabs: [Workspace]) { | ||
| #if DEBUG | ||
| guard !didApplyUITestSidebarSelection else { return } | ||
| let env = ProcessInfo.processInfo.environment | ||
| guard let rawValue = env["CMUX_UI_TEST_SIDEBAR_SELECTED_WORKSPACE_INDICES"]? | ||
| .trimmingCharacters(in: .whitespacesAndNewlines), | ||
| !rawValue.isEmpty else { | ||
| return | ||
| } | ||
|
|
||
| var indices: [Int] = [] | ||
| for token in rawValue.split(separator: ",") { | ||
| let trimmed = token.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| guard let index = Int(trimmed), index >= 0 else { return } | ||
| if !indices.contains(index) { | ||
| indices.append(index) | ||
| } | ||
| } | ||
|
|
||
| guard let lastIndex = indices.last, !indices.isEmpty, lastIndex < tabs.count else { return } | ||
|
|
||
| let selectedIds = Set(indices.map { tabs[$0].id }) | ||
| selectedTabIds = selectedIds |
There was a problem hiding this comment.
Validate all parsed UI-test indices before subscripting tabs.
Line 6023 only validates indices.last, but Line 6025 subscripts every parsed index. An unsorted value like "3,0" can still crash with out-of-bounds access.
💡 Proposed fix
- guard let lastIndex = indices.last, !indices.isEmpty, lastIndex < tabs.count else { return }
+ guard !indices.isEmpty else { return }
+ guard indices.allSatisfy({ $0 < tabs.count }) else { return }
+ guard let lastIndex = indices.last else { return }
let selectedIds = Set(indices.map { tabs[$0].id })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ContentView.swift` around lines 6004 - 6026, In
applyUITestSidebarSelectionIfNeeded, the code currently only validates
indices.last before using indices to subscript tabs which can crash for unsorted
values like "3,0"; update the logic to validate every parsed index (each element
of indices) is within 0..<tabs.count (and >= 0) before mapping to tabs, or
alternatively filter out-of-range indices and deduplicate after validation, then
build selectedIds from the safe indices and assign to selectedTabIds; reference
indices, tabs, selectedIds and selectedTabIds when making the checks and
early-return on any invalid input.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8770d106f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+6023
to
+6025
| guard let lastIndex = indices.last, !indices.isEmpty, lastIndex < tabs.count else { return } | ||
|
|
||
| let selectedIds = Set(indices.map { tabs[$0].id }) |
There was a problem hiding this comment.
applyUITestSidebarSelectionIfNeeded only checks indices.last against tabs.count, but then indexes every parsed value (tabs[$0]). A value like CMUX_UI_TEST_SIDEBAR_SELECTED_WORKSPACE_INDICES=0,99,1 passes the current guard when there are 2 tabs and then crashes on tabs[99], which can take down DEBUG/UI-test runs from a malformed env setting. Please validate every parsed index is in range before building selectedIds.
Useful? React with 👍 / 👎.
This was referenced Mar 13, 2026
This was referenced Mar 20, 2026
This was referenced Mar 29, 2026
bn-l
pushed a commit
to bn-l/cmux
that referenced
this pull request
Apr 3, 2026
* Add multi-workspace close UI regression test * Summarize multi-workspace close confirmation * Add Cmd+Shift+W multi-close UI regression test * Honor sidebar multi-select for close workspace --------- Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
This was referenced Apr 5, 2026
This was referenced Apr 18, 2026
This was referenced May 1, 2026
This branch was successfully deployed
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.
Summary
Testing
./scripts/reload.sh --tag task-multi-workspace-close-confirmation(build succeeded)Issues
Summary by cubic
Show one confirmation dialog when closing multiple workspaces, with a clear summary of what will be closed across the command palette, workspace menu, and sidebar multi-select. Meets the Linear task requirement and reduces repeated prompts, including the “Close window?” case when all workspaces are selected.
TabManager.closeWorkspacesWithConfirmation(...)that aggregates workspace titles and localizes messages (en/ja).TabManagerfor ordered multi-close, and updated key-equivalent suppression to include “Close workspaces?”.Written for commit c8770d1. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation
Tests