Repository navigation
Add warnBeforeClosingTab close-warning toggle - #2808
Merged
Merged
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:
📝 WalkthroughWalkthroughAdds a UserDefaults-backed setting that reuses the quit-warning key to control whether tab/workspace/window close confirmations are shown, threads a closeTrigger through workspace-close APIs to conditionally skip prompts, updates localization and schema text, expands tests, and adds build helper scripts. Changes
Sequence DiagramsequenceDiagram
actor User
participant SettingsUI as "Settings UI"
participant UserDefaults as "UserDefaults"
participant TabManager as "TabManager"
participant Workspace as "Workspace"
participant Dialog as "Close Dialog"
User->>SettingsUI: Toggle "Warn Before Closing Tab"
SettingsUI->>UserDefaults: Save setting state
User->>TabManager: Request close tab/workspace/window (with closeTrigger)
TabManager->>UserDefaults: Check CloseTabWarningSettings.isEnabled()
alt enabled
UserDefaults-->>TabManager: true
TabManager->>Workspace: request confirmClosePanel(closeTrigger)
Workspace->>Dialog: Present confirmation dialog
Dialog-->>User: "Confirm close?"
User-->>Dialog: Confirm/Cancel
Dialog-->>Workspace: response
Workspace-->>TabManager: proceed or cancel
else disabled
UserDefaults-->>TabManager: false
TabManager->>Workspace: Skip confirmation, close immediately
Workspace-->>TabManager: closed
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 3❌ Failed checks (2 warnings, 1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile SummaryAdds an
Confidence Score: 5/5Safe to merge; all previously identified gaps in the close-confirmation routing are addressed in this revision. The CloseTabConfirmationPolicy correctly gates every close-tab path described in the PR — tab-strip gesture, pane close, runtime surface, close-other-tabs, and multi-workspace batch close. The closeWorkspacesWithConfirmation gap flagged in earlier review rounds is now covered by shouldConfirmClose(source: .tabClose). The pinned-workspace last-surface path routes through the new closeWorkspaceFromCloseTabGesture, which consults the policy for both the pinned-workspace confirmation and the running-process check. The single-workspace Cmd+Shift+W path intentionally uses .workspace source and is not claimed by the PR to be in scope. Minor findings are style-level only. No files require special attention. Important Files Changed
Reviews (4): Last reviewed commit: "Honor cmux close-tab warning preference" | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
cmuxTests/TabManagerUnitTests.swift (1)
892-893: Use the production key constant instead of duplicating the UserDefaults key string.This test hardcodes
"warnBeforeClosingTabShortcut"even thoughCloseTabWarningSettings.warnBeforeClosingTabKeyalready exists. Reusing the constant avoids drift.Proposed diff
- private let warnBeforeClosingTabDefaultsKey = "warnBeforeClosingTabShortcut" + private let warnBeforeClosingTabDefaultsKey = CloseTabWarningSettings.warnBeforeClosingTabKey🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/TabManagerUnitTests.swift` around lines 892 - 893, Replace the hardcoded UserDefaults key string used in TabManagerUnitTests by referencing the production constant: change the private let warnBeforeClosingTabDefaultsKey to use CloseTabWarningSettings.warnBeforeClosingTabKey so the test uses the same source of truth; ensure any code that reads/writes originalWarnBeforeClosingTabPreference still uses that variable name and behavior but initialized from CloseTabWarningSettings.warnBeforeClosingTabKey.
🤖 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/TabManagerUnitTests.swift`:
- Around line 910-916: Change makeRuntimeCloseWarningTestSubject to avoid
fatalError: make it return an optional tuple (manager: TabManager, workspace:
Workspace, panelId: UUID)?; replace the guard fatalError branch with
XCTFail("Expected selected workspace and focused terminal panel") and return
nil, using the existing properties selectedWorkspace, focusedPanelId, and
terminalPanel(for:) to populate the tuple on success; update callers in tests to
either XCTUnwrap the result or handle the optional so failures are reported
per-test instead of aborting the process.
---
Nitpick comments:
In `@cmuxTests/TabManagerUnitTests.swift`:
- Around line 892-893: Replace the hardcoded UserDefaults key string used in
TabManagerUnitTests by referencing the production constant: change the private
let warnBeforeClosingTabDefaultsKey to use
CloseTabWarningSettings.warnBeforeClosingTabKey so the test uses the same source
of truth; ensure any code that reads/writes
originalWarnBeforeClosingTabPreference still uses that variable name and
behavior but initialized from CloseTabWarningSettings.warnBeforeClosingTabKey.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: d3ef3acf-024b-4945-a624-4368d60ed961
📒 Files selected for processing (7)
Resources/Localizable.xcstringsSources/KeyboardShortcutSettingsFileStore.swiftSources/TabManager.swiftSources/Workspace.swiftSources/cmuxApp.swiftcmuxTests/TabManagerUnitTests.swiftweb/data/cmux-settings.schema.json
Comment on lines
+910
to
+916
| private func makeRuntimeCloseWarningTestSubject() -> (manager: TabManager, workspace: Workspace, panelId: UUID) { | ||
| let manager = TabManager() | ||
| guard let workspace = manager.selectedWorkspace, | ||
| let panelId = workspace.focusedPanelId, | ||
| let terminalPanel = workspace.terminalPanel(for: panelId) else { | ||
| fatalError("Expected selected workspace and focused terminal panel") | ||
| } |
There was a problem hiding this comment.
Avoid fatalError in test fixture setup helpers.
If fixture creation fails, fatalError aborts the test process instead of failing just the current test. Prefer returning nil with XCTFail so failures stay isolated.
Proposed diff
- private func makeRuntimeCloseWarningTestSubject() -> (manager: TabManager, workspace: Workspace, panelId: UUID) {
+ private func makeRuntimeCloseWarningTestSubject(
+ file: StaticString = `#filePath`,
+ line: UInt = `#line`
+ ) -> (manager: TabManager, workspace: Workspace, panelId: UUID)? {
let manager = TabManager()
guard let workspace = manager.selectedWorkspace,
let panelId = workspace.focusedPanelId,
let terminalPanel = workspace.terminalPanel(for: panelId) else {
- fatalError("Expected selected workspace and focused terminal panel")
+ XCTFail("Expected selected workspace and focused terminal panel", file: file, line: line)
+ return nil
}
terminalPanel.surface.setNeedsConfirmCloseOverrideForTesting(true)
workspace.updatePanelShellActivityState(panelId: panelId, state: .unknown)
- return (manager, workspace, panelId)
+ return (manager, workspace, panelId)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/TabManagerUnitTests.swift` around lines 910 - 916, Change
makeRuntimeCloseWarningTestSubject to avoid fatalError: make it return an
optional tuple (manager: TabManager, workspace: Workspace, panelId: UUID)?;
replace the guard fatalError branch with XCTFail("Expected selected workspace
and focused terminal panel") and return nil, using the existing properties
selectedWorkspace, focusedPanelId, and terminalPanel(for:) to populate the tuple
on success; update callers in tests to either XCTUnwrap the result or handle the
optional so failures are reported per-test instead of aborting the process.
There was a problem hiding this comment.
1 issue 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="cmuxTests/TabManagerUnitTests.swift">
<violation number="1" location="cmuxTests/TabManagerUnitTests.swift:915">
P3: `fatalError` in a test fixture helper aborts the entire test process instead of failing just the current test. Use `XCTFail` and return an optional so failures remain isolated.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| guard let workspace = manager.selectedWorkspace, | ||
| let panelId = workspace.focusedPanelId, | ||
| let terminalPanel = workspace.terminalPanel(for: panelId) else { | ||
| fatalError("Expected selected workspace and focused terminal panel") |
There was a problem hiding this comment.
P3: fatalError in a test fixture helper aborts the entire test process instead of failing just the current test. Use XCTFail and return an optional so failures remain isolated.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/TabManagerUnitTests.swift, line 915:
<comment>`fatalError` in a test fixture helper aborts the entire test process instead of failing just the current test. Use `XCTFail` and return an optional so failures remain isolated.</comment>
<file context>
@@ -889,6 +889,37 @@ final class TabManagerCloseWorkspacesWithConfirmationTests: XCTestCase {
+ guard let workspace = manager.selectedWorkspace,
+ let panelId = workspace.focusedPanelId,
+ let terminalPanel = workspace.terminalPanel(for: panelId) else {
+ fatalError("Expected selected workspace and focused terminal panel")
+ }
+
</file context>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmuxTests/TabManagerUnitTests.swift (1)
910-916:⚠️ Potential issue | 🟠 MajorReplace
fatalErrorin test subject builders with per-test failure paths.Using
fatalErrorhere can terminate the entire test run instead of failing only the current test. Please return an optional (or throw) and fail viaXCTFail/XCTUnwrapat call sites.Proposed fix
- private func makeRuntimeCloseWarningTestSubject() -> (manager: TabManager, workspace: Workspace, panelId: UUID) { + private func makeRuntimeCloseWarningTestSubject( + file: StaticString = `#filePath`, + line: UInt = `#line` + ) -> (manager: TabManager, workspace: Workspace, panelId: UUID)? { let manager = TabManager() guard let workspace = manager.selectedWorkspace, let panelId = workspace.focusedPanelId, let terminalPanel = workspace.terminalPanel(for: panelId) else { - fatalError("Expected selected workspace and focused terminal panel") + XCTFail("Expected selected workspace and focused terminal panel", file: file, line: line) + return nil } terminalPanel.surface.setNeedsConfirmCloseOverrideForTesting(true) workspace.updatePanelShellActivityState(panelId: panelId, state: .unknown) return (manager, workspace, panelId) } - private func makeLastSurfaceCloseWarningTestSubject() -> ( + private func makeLastSurfaceCloseWarningTestSubject( + file: StaticString = `#filePath`, + line: UInt = `#line` + ) -> ( manager: TabManager, remainingWorkspace: Workspace, closingWorkspace: Workspace, closingPanelId: UUID - ) { + )? { let manager = TabManager() let remainingWorkspace = manager.tabs[0] let closingWorkspace = manager.addWorkspace() manager.selectWorkspace(closingWorkspace) guard let closingPanelId = closingWorkspace.focusedPanelId, let terminalPanel = closingWorkspace.terminalPanel(for: closingPanelId) else { - fatalError("Expected focused terminal panel in closing workspace") + XCTFail("Expected focused terminal panel in closing workspace", file: file, line: line) + return nil } terminalPanel.surface.setNeedsConfirmCloseOverrideForTesting(true) closingWorkspace.updatePanelShellActivityState(panelId: closingPanelId, state: .unknown) return (manager, remainingWorkspace, closingWorkspace, closingPanelId) }#!/bin/bash # Verify remaining fatalError usage and helper call-sites in this test class. rg -n 'makeRuntimeCloseWarningTestSubject|makeLastSurfaceCloseWarningTestSubject|fatalError\(' cmuxTests/TabManagerUnitTests.swift -C2Also applies to: 934-937
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/TabManagerUnitTests.swift` around lines 910 - 916, Replace fatalError in the test helper makeRuntimeCloseWarningTestSubject (and similarly makeLastSurfaceCloseWarningTestSubject) with a non-terminating failure path: change the helper to return an optional (or throw) when selectedWorkspace/focusedPanelId/terminalPanel are missing, and update all call sites to unwrap with XCTUnwrap or handle the thrown error and call XCTFail on failure; specifically modify makeRuntimeCloseWarningTestSubject and makeLastSurfaceCloseWarningTestSubject to return (manager: TabManager, workspace: Workspace, panelId: UUID)? or throws, and replace direct fatalError usages in those functions with a nil return or throw, then update tests to use XCTAssertNotNil/XCTUnwrap or try/catch + XCTFail so a single test fails instead of terminating the whole test run.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmuxTests/TabManagerUnitTests.swift`:
- Around line 910-916: Replace fatalError in the test helper
makeRuntimeCloseWarningTestSubject (and similarly
makeLastSurfaceCloseWarningTestSubject) with a non-terminating failure path:
change the helper to return an optional (or throw) when
selectedWorkspace/focusedPanelId/terminalPanel are missing, and update all call
sites to unwrap with XCTUnwrap or handle the thrown error and call XCTFail on
failure; specifically modify makeRuntimeCloseWarningTestSubject and
makeLastSurfaceCloseWarningTestSubject to return (manager: TabManager,
workspace: Workspace, panelId: UUID)? or throws, and replace direct fatalError
usages in those functions with a nil return or throw, then update tests to use
XCTAssertNotNil/XCTUnwrap or try/catch + XCTFail so a single test fails instead
of terminating the whole test run.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f2fcecaf-a0b9-494e-9394-7ccd6ed931d3
📒 Files selected for processing (3)
Sources/TabManager.swiftSources/Workspace.swiftcmuxTests/TabManagerUnitTests.swift
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
cmuxTests/TabManagerUnitTests.swift (1)
948-954:⚠️ Potential issue | 🟠 MajorAvoid
fatalErrorin fixture builders; fail the current test instead.
fatalErrorhere aborts the entire test run process instead of failing only the invoking test.Proposed diff
- private func makeRuntimeCloseWarningTestSubject() -> (manager: TabManager, workspace: Workspace, panelId: UUID) { + private func makeRuntimeCloseWarningTestSubject( + file: StaticString = `#filePath`, + line: UInt = `#line` + ) -> (manager: TabManager, workspace: Workspace, panelId: UUID)? { let manager = TabManager() guard let workspace = manager.selectedWorkspace, let panelId = workspace.focusedPanelId, let terminalPanel = workspace.terminalPanel(for: panelId) else { - fatalError("Expected selected workspace and focused terminal panel") + XCTFail("Expected selected workspace and focused terminal panel", file: file, line: line) + return nil } @@ - private func makeLastSurfaceCloseWarningTestSubject() -> ( + private func makeLastSurfaceCloseWarningTestSubject( + file: StaticString = `#filePath`, + line: UInt = `#line` + ) -> ( manager: TabManager, remainingWorkspace: Workspace, closingWorkspace: Workspace, closingPanelId: UUID - ) { + )? { @@ guard let closingPanelId = closingWorkspace.focusedPanelId, let terminalPanel = closingWorkspace.terminalPanel(for: closingPanelId) else { - fatalError("Expected focused terminal panel in closing workspace") + XCTFail("Expected focused terminal panel in closing workspace", file: file, line: line) + return nil }Also applies to: 972-975
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/TabManagerUnitTests.swift` around lines 948 - 954, Replace the fatalError calls in the fixture builder makeRuntimeCloseWarningTestSubject (and the similar occurrence around lines 972-975) with test-friendly failures: call XCTFail with a descriptive message when manager.selectedWorkspace, workspace.focusedPanelId, or workspace.terminalPanel(for:) are nil, and then return a safe fallback (e.g., create a new TabManager and unwrapable defaults or optionally use guard-let with XCTFail + return a tuple with newly created manager/workspace/UUID) so the single test fails instead of aborting the whole test run; reference the makeRuntimeCloseWarningTestSubject function and the other analogous fixture to locate and change the fatalError usage.
🧹 Nitpick comments (2)
cmuxTests/AppDelegateShortcutRoutingTests.swift (1)
1348-1352: Add failure-path cleanup for the created window.If this test exits early (e.g., guard/assertion failure), the window can stay open and affect later tests. Consider adding
defercleanup immediately after window creation.Suggested patch
let windowId = appDelegate.createMainWindow() + defer { closeWindow(withId: windowId) } guard let targetWindow = window(withId: windowId) else { XCTFail("Expected test window") return }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/AppDelegateShortcutRoutingTests.swift` around lines 1348 - 1352, The test creates a window via appDelegate.createMainWindow() but does not ensure cleanup if the guard fails; add a defer immediately after obtaining windowId to close/remove the created window (use the same API that other tests use to tear down windows) so the window is always cleaned up even on early return; update the block around createMainWindow(), window(withId:) and targetWindow to register the cleanup via defer referencing windowId (or targetWindow if available) before the guard/assertion so later tests aren't affected.cmuxTests/TabManagerUnitTests.swift (1)
930-946: Use the shared settings key constant instead of a duplicated literal.
warnBeforeClosingTabDefaultsKeyduplicatesCloseTabWarningSettings.warnBeforeClosingTabKey, which can drift over time.Proposed diff
- private let warnBeforeClosingTabDefaultsKey = "warnBeforeClosingTabShortcut" + private let warnBeforeClosingTabDefaultsKey = CloseTabWarningSettings.warnBeforeClosingTabKey🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/TabManagerUnitTests.swift` around lines 930 - 946, The test defines a duplicated key constant warnBeforeClosingTabDefaultsKey; replace usages with the shared constant CloseTabWarningSettings.warnBeforeClosingTabKey to avoid drift — update the setUp and tearDown to use CloseTabWarningSettings.warnBeforeClosingTabKey when calling UserDefaults.standard.object(forKey:), removeObject(forKey:) and set(_:forKey:), and drop the local warnBeforeClosingTabDefaultsKey declaration (keep originalWarnBeforeClosingTabPreference as-is).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/TabManager.swift`:
- Around line 3991-3995: The shared helper confirmClose currently short-circuits
all callers by returning true when CloseTabWarningSettings.isEnabled() is false;
remove that guard so confirmClose always defers to
confirmCloseHandler/confirmation logic and do not bypass prompts for
non-tab-specific flows. Ensure callers that should respect the user opt-out
(e.g., warnBeforeClosingTab and tab/surface-specific close paths) continue to
check CloseTabWarningSettings.isEnabled() themselves, and leave
workspace/window/pinned/bulk close callers untouched so they still show
confirmations via confirmClose.
---
Duplicate comments:
In `@cmuxTests/TabManagerUnitTests.swift`:
- Around line 948-954: Replace the fatalError calls in the fixture builder
makeRuntimeCloseWarningTestSubject (and the similar occurrence around lines
972-975) with test-friendly failures: call XCTFail with a descriptive message
when manager.selectedWorkspace, workspace.focusedPanelId, or
workspace.terminalPanel(for:) are nil, and then return a safe fallback (e.g.,
create a new TabManager and unwrapable defaults or optionally use guard-let with
XCTFail + return a tuple with newly created manager/workspace/UUID) so the
single test fails instead of aborting the whole test run; reference the
makeRuntimeCloseWarningTestSubject function and the other analogous fixture to
locate and change the fatalError usage.
---
Nitpick comments:
In `@cmuxTests/AppDelegateShortcutRoutingTests.swift`:
- Around line 1348-1352: The test creates a window via
appDelegate.createMainWindow() but does not ensure cleanup if the guard fails;
add a defer immediately after obtaining windowId to close/remove the created
window (use the same API that other tests use to tear down windows) so the
window is always cleaned up even on early return; update the block around
createMainWindow(), window(withId:) and targetWindow to register the cleanup via
defer referencing windowId (or targetWindow if available) before the
guard/assertion so later tests aren't affected.
In `@cmuxTests/TabManagerUnitTests.swift`:
- Around line 930-946: The test defines a duplicated key constant
warnBeforeClosingTabDefaultsKey; replace usages with the shared constant
CloseTabWarningSettings.warnBeforeClosingTabKey to avoid drift — update the
setUp and tearDown to use CloseTabWarningSettings.warnBeforeClosingTabKey when
calling UserDefaults.standard.object(forKey:), removeObject(forKey:) and
set(_:forKey:), and drop the local warnBeforeClosingTabDefaultsKey declaration
(keep originalWarnBeforeClosingTabPreference as-is).
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 228279f6-151c-4e77-951f-da10ca44e758
📒 Files selected for processing (4)
Sources/AppDelegate.swiftSources/TabManager.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/TabManagerUnitTests.swift
✅ Files skipped from review due to trivial changes (1)
- Sources/AppDelegate.swift
There was a problem hiding this comment.
4 issues 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/TabManager.swift">
<violation number="1" location="Sources/TabManager.swift:3992">
P2: The new global `CloseTabWarningSettings` check in `confirmClose` disables workspace-close confirmation dialogs as well, not only tab-close warnings.</violation>
</file>
<file name="Sources/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:5922">
P2: `warnBeforeClosingTab` is applied too broadly here: it also suppresses explicit window-close confirmation dialogs.</violation>
<violation number="2" location="Sources/AppDelegate.swift:5922">
P1: This guard skips the window-close confirmation entirely when the tab-close warning is disabled. Closing an entire window (e.g. via Cmd+Ctrl+W) may destroy multiple workspaces, but this blanket bypass removes that safety net based on a setting intended only for single-tab Cmd+W closes. Remove this guard and scope the opt-out to tab-specific close paths instead.</violation>
<violation number="3" location="Sources/AppDelegate.swift:5922">
P1: This guard in the shared `confirmClose` helper bypasses **all** confirmation dialogs (pinned-workspace protection, explicit workspace-close, multi-workspace close, "close other tabs") when the tab-close warning is disabled — not just single-tab close prompts. The opt-out should be scoped to the tab/surface-specific call sites, not the generic helper that every confirmation path funnels through.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/install-clang-wrapper.sh`:
- Around line 98-127: The current relink decision uses only
xcode_count/toolchain_count which can miss cases where entries match in count
but symlink targets are stale; instead, determine need_relink by iterating the
XCODE_DEFAULT_BIN entries and checking per-entry consistency: for each src in
the for-loop (for src in "$XCODE_DEFAULT_BIN"/*) compute name="$(basename
"$src")" and dest="$TOOLCHAIN_BIN/$name", then if dest is missing, not a
symlink, or is a symlink whose readlink result differs from src, set
need_relink=1 (and break) so the subsequent relink block runs; keep the existing
replacement logic that compares current="$(/usr/bin/readlink "$dest" || true)"
and updates/removes dest when mismatched. This uses the existing variables
need_relink, XCODE_DEFAULT_BIN, TOOLCHAIN_BIN and the readlink comparison to
reliably detect and repair stale symlinks.
In `@Sources/cmuxApp.swift`:
- Around line 4998-4999: The current localized subtitle strings for the Warn
Before Quit row ("settings.app.warnBeforeQuit.subtitleOn" and
"settings.app.warnBeforeQuit.subtitleOff") mention both quitting and closing;
update these two localized values so they only describe quit-specific behavior
(e.g., "Show a confirmation before quitting." and "Quit without confirmation.")
and move the close-confirmation wording to the separate close-warning setting
row so the quit control remains quit-scoped; change the String(localized: ...)
usages for the Warn Before Quit row to reference the revised quit-only subtitle
texts.
- Around line 3779-3789: The CloseTabWarningSettings currently aliases its
storage and defaults to QuitWarningSettings
(warnBeforeQuitKey/defaultWarnBeforeQuit), which prevents independent control;
create dedicated storage keys and default values inside CloseTabWarningSettings
(e.g., new static let warnBeforeClosingTabKey and static let
defaultWarnBeforeClosingTab) and update
CloseTabWarningSettings.isEnabled(defaults:) and
CloseTabWarningSettings.setEnabled(_:defaults:) to read/write using those new
keys in UserDefaults instead of delegating to QuitWarningSettings, ensuring
callers of CloseTabWarningSettings (isEnabled and setEnabled) use the
independent close-warning preference.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 429d5cd1-b042-41ab-8662-c44c30512e5c
📒 Files selected for processing (10)
Resources/Localizable.xcstringsSources/KeyboardShortcutSettingsFileStore.swiftSources/cmuxApp.swiftcmuxTests/AppDelegateShortcutRoutingTests.swiftcmuxTests/TabManagerUnitTests.swiftcmuxTests/WorkspaceUnitTests.swiftscripts/cmux-clang-wrapper.shscripts/install-clang-wrapper.shscripts/reload.shweb/data/cmux-settings.schema.json
✅ Files skipped from review due to trivial changes (2)
- Sources/KeyboardShortcutSettingsFileStore.swift
- web/data/cmux-settings.schema.json
🚧 Files skipped from review as they are similar to previous changes (2)
- Resources/Localizable.xcstrings
- cmuxTests/TabManagerUnitTests.swift
There was a problem hiding this comment.
5 issues found across 10 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="cmuxTests/AppDelegateShortcutRoutingTests.swift">
<violation number="1" location="cmuxTests/AppDelegateShortcutRoutingTests.swift:60">
P2: The fixture now saves/restores the quit-warning preference instead of the close-tab warning preference, so close-warning tests are no longer exercising the intended setting.</violation>
<violation number="2" location="cmuxTests/AppDelegateShortcutRoutingTests.swift:1360">
P2: This test toggles `warnBeforeQuit` instead of `warnBeforeClosingTab`, so it no longer validates the close-warning shortcut behavior described by the test name.</violation>
</file>
<file name="scripts/install-clang-wrapper.sh">
<violation number="1" location="scripts/install-clang-wrapper.sh:101">
P1: Relink detection only compares file counts, so stale symlinks are missed when Xcode changes but counts match.</violation>
</file>
<file name="Sources/cmuxApp.swift">
<violation number="1" location="Sources/cmuxApp.swift:3780">
P1: `warnBeforeClosingTab` is now aliased to `warnBeforeQuit`, so close and quit confirmations cannot be configured independently.</violation>
</file>
<file name="Sources/KeyboardShortcutSettingsFileStore.swift">
<violation number="1" location="Sources/KeyboardShortcutSettingsFileStore.swift:437">
P1: `warnBeforeClosingTab` is being mapped to the quit-warning key, which breaks independent close-vs-quit warning behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
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="cmuxTests/AppDelegateShortcutRoutingTests.swift">
<violation number="1" location="cmuxTests/AppDelegateShortcutRoutingTests.swift:1409">
P2: This test asserts the opposite of the new `warnBeforeClosingTab` behavior for Cmd+Ctrl+W, so it can lock in incorrect window-close confirmation logic.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
Bugbot Autofix is kicking off a free cloud agent to fix this issue. This run is complimentary, but you can enable autofix for all future PRs in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b19174f. Configure here.
The close-tab flow needs a regression that demonstrates cmux.json cannot yet suppress the cmux-owned Cmd+W confirmation. The test loads app.warnBeforeClosingTab=false through the same settings file store used at runtime, then closes a terminal surface that normally requires confirmation and expects an immediate close with no prompt.\n\nConstraint: Keep the first commit test-only so CI can show the regression before the fix.\nConfidence: high\nScope-risk: narrow\nDirective: Keep this coverage behavior-level; do not replace it with source-shape assertions.\nTested: Not run locally per repository instruction; expected to fail before the fix in CI.\nNot-tested: Local XCTest execution.
austinywang
force-pushed
the
issue-946-warn-before-closing-tab
branch
from
May 11, 2026 00:19
b19174f to
ffcc0b1
Compare
austinywang
force-pushed
the
issue-946-warn-before-closing-tab
branch
from
May 11, 2026 01:01
ffcc0b1 to
cce7f01
Compare
The close-tab confirmation decision now combines the existing process/dirty close risk with a cmux-owned app.warnBeforeClosingTab preference. This keeps the close-risk detection centralized and unchanged while allowing configured users to skip the cmux dialog on Cmd+W, tab-strip closes, close-other-tabs flows, runtime surface closes, and pane-close delegation.\n\nConstraint: Build and XCTest must run in CI only for this task; avoid direct xcodebuild.\nRejected: Gate TabManager.confirmClose globally | it would also suppress workspace/window/pinned-workspace confirmations that are not the close-tab setting.\nRejected: Patch only Cmd+W | tab-strip and context-menu closes would keep prompting.\nConfidence: high\nScope-risk: moderate\nDirective: New close-tab prompt paths must call CloseTabConfirmationPolicy instead of checking panelNeedsConfirmClose directly.\nTested: jq empty Resources/Localizable.xcstrings; jq empty web/data/cmux.schema.json; git diff --check.\nNot-tested: Local tests/build per repository and task instruction; CI will validate.
austinywang
force-pushed
the
issue-946-warn-before-closing-tab
branch
from
May 11, 2026 01:20
cce7f01 to
02313ed
Compare
This was referenced May 23, 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.
Closes #946.
Summary
app.warnBeforeClosingTabto cmux.json loading, generated schema data, Settings search, and Settings UIfalse,true, and default/unset config valuesValidation
jq empty Resources/Localizable.xcstringsjq empty web/data/cmux.schema.jsongit diff --checkNote
Medium Risk
Changes tab/workspace close confirmation behavior across multiple close paths (Cmd+W, tab-strip gestures, batch closes, and runtime surface closes), so regressions could lead to unintended silent closes or extra prompts. Scope is limited to close-confirmation gating and adds unit tests plus schema/UI wiring.
Overview
Adds a new
app.warnBeforeClosingTabpreference (default enabled) that can be set viacmux.json, surfaced in Settings UI, indexed in Settings search, and documented in the JSON schema and localizations.Routes tab/workspace close confirmations through a shared
CloseTabConfirmationPolicy, so Cmd+W/tab-strip closes, close-other-tabs, pane closes, runtime surface closes, and pinned-workspace last-surface behavior consistently honor the new toggle.Extends XCTest coverage to verify enabled/disabled/default behavior, including config-file parsing and pinned-workspace edge cases.
Reviewed by Cursor Bugbot for commit 02313ed. Bugbot is set up for automated code reviews on this repo. Configure here.