Repository navigation
Warn or disable X-button tab close (#4622) - #4626
austinywang wants to merge 10 commits into
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 ChangesX-Button Tab Close Warning
Sequence DiagramsequenceDiagram
participant User
participant TabUI as Tab UI
participant Workspace
participant TabManager
participant Policy as CloseTabConfirmationPolicy
User->>TabUI: Click X button
TabUI->>Workspace: onTabCloseRequest(tabID)
Workspace->>Workspace: markExplicitClose(tabID, trigger: .tabCloseButton)
Workspace->>TabManager: closeWorkspaceFromCloseTabGesture(workspace, trigger: .tabCloseButton)
TabManager->>Policy: shouldConfirm(..., trigger: .tabCloseButton)
Policy-->>TabManager: (true/false)
TabManager-->>User: show confirmation or close immediately
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (2 errors, 1 warning, 3 inconclusive)
✅ Passed checks (11 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 |
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 `@Resources/Localizable.xcstrings`:
- Around line 60964-61014: The new localization keys
settings.app.warnBeforeClosingTabXButton,
settings.app.warnBeforeClosingTabXButton.subtitleOff, and
settings.app.warnBeforeClosingTabXButton.subtitleOn only include "en" and "ja";
update each key to include an entry for every locale supported by this
Resources/*.xcstrings catalog (add the missing locale slots and supply
translated text or fallback English values), ensuring each locale has a
stringUnit/state and extractionState left as in other entries; apply the same
fix to the other new keys reported in the review (the other set of added keys
mentioned) so every new Swift localization key has entries for all catalog
locales.
In `@Sources/Workspace.swift`:
- Around line 9891-9892: The function markExplicitClose(surfaceId: TabID,
trigger: CloseTabConfirmationTrigger = .tabCloseButton) still has a default
parameter allowing callers to omit the trigger; remove the default so the
signature becomes markExplicitClose(surfaceId: TabID, trigger:
CloseTabConfirmationTrigger) and update all callsites to pass an explicit
CloseTabConfirmationTrigger value (e.g. .tabCloseButton, .keyboardShortcut,
.programmatic, etc.) to avoid silently routing non-X-button closes through the
X-button preference; search for markExplicitClose(...) usages and adjust each to
provide the correct CloseTabConfirmationTrigger.
🪄 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: 2c3fbb80-5be4-4a5f-8072-ca80045ef641
📒 Files selected for processing (12)
Resources/Localizable.xcstringsSources/CmuxSettingsJSONPathSupport.swiftSources/CommandPalette/CommandPaletteSettingsToggle.swiftSources/KeyboardShortcutSettingsFileStore+Template.swiftSources/KeyboardShortcutSettingsFileStore.swiftSources/SettingsNavigation.swiftSources/SettingsSearchAliases.swiftSources/TabManager.swiftSources/Workspace.swiftSources/cmuxApp.swiftcmuxTests/TabManagerUnitTests.swiftweb/data/cmux.schema.json
Greptile SummaryThis PR introduces
Confidence Score: 3/5Not safe to merge as-is: the default-on setting causes a double confirmation dialog for single-workspace windows, and middle-click ignores the new warning entirely. Two concrete defects affect the changed close paths. When warnBeforeClosingTabXButton is on (the default) and the user X-button-closes the only workspace in a window, the app shows Close workspace? and then immediately shows Close window? — a double prompt the unit tests miss because they run TabManager without a real NSWindow. The sidebar middle-click gesture still routes through closeWorkspaceWithConfirmation so the new setting has no effect on middle-click. Both defects are on the primary changed paths and the first will affect nearly every user on first launch. Sources/AppDelegate.swift (handleMainTerminalWindowShouldClose interaction with closeWorkspaceIfRunningProcess) and Sources/ContentView.swift (middle-click call site at line 13902). Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["User closes tab"] --> B{Gesture?}
B -->|"Sidebar X-button"| C["closeWorkspaceFromCloseTabGesture\ntrigger: .tabCloseButton"]
B -->|"Keyboard shortcut"| D["closeWorkspaceFromCloseTabGesture\ntrigger: .shortcut"]
B -->|"Middle-click"| E["closeWorkspaceWithConfirmation\nsource: .workspace ignores settings"]
B -->|"Red window button"| F["windowShouldClose\nhandleMainTerminalWindowShouldClose"]
C --> G{tabCloseButton policy}
G -->|"warnBeforeClosingTabXButton=true"| H["Close workspace? dialog"]
G -->|"warnBeforeClosingTabXButton=false"| I["Close immediately"]
D --> J{shortcutTabClose policy}
J -->|"warnBeforeClosingTab=true AND requiresConfirmation=true"| K["Close workspace? dialog"]
J -->|"else"| L["Close immediately"]
H --> M{Last workspace in window?}
K --> M
I --> M
L --> M
M -->|"Yes"| N["window.performClose nil"]
M -->|"No"| O["closeWorkspace done"]
N --> F
F --> P["confirmCloseMainWindow: Close window? dialog --- SECOND DIALOG on X-button path"]
|
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/cmuxApp.swift (1)
4733-4745:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep the X-button warning default independent from the shortcut setting.
Line 4733 makes the new X-button preference inherit the shortcut default, and Line 4744 falls back to
isEnabled(...)when the X-button key is unset. That makes the new setting effectively on-by-default and tied towarnBeforeClosingTab, which contradicts the separate opt-in behavior described forapp.warnBeforeClosingTabXButton. It also meansReset All Settingswill not restore X-button close behavior independently.🐛 Proposed fix
enum CloseTabWarningSettings { static let warnBeforeClosingTabKey = "warnBeforeClosingTabShortcut" static let warnBeforeClosingTabXButtonKey = "warnBeforeClosingTabXButton" static let defaultWarnBeforeClosingTab = true - static let defaultWarnBeforeClosingTabXButton = defaultWarnBeforeClosingTab + static let defaultWarnBeforeClosingTabXButton = false @@ static func isXButtonWarningEnabled(defaults: UserDefaults = .standard) -> Bool { if defaults.object(forKey: warnBeforeClosingTabXButtonKey) == nil { - return isEnabled(defaults: defaults) + return defaultWarnBeforeClosingTabXButton } return defaults.bool(forKey: warnBeforeClosingTabXButtonKey) }🤖 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/cmuxApp.swift` around lines 4733 - 4745, The isXButtonWarningEnabled method currently falls back to isEnabled(...) causing the X-button preference to inherit warnBeforeClosingTab; change it to treat warnBeforeClosingTabXButtonKey independently by returning defaultWarnBeforeClosingTabXButton when the X-button key is unset instead of calling isEnabled(...). Update isXButtonWarningEnabled(defaults:) to check defaults.object(forKey: warnBeforeClosingTabXButtonKey) and return defaults.bool(forKey: warnBeforeClosingTabXButtonKey) or defaultWarnBeforeClosingTabXButton, leaving isEnabled(...) and defaultWarnBeforeClosingTab untouched.
🤖 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 `@web/data/cmux.schema.json`:
- Around line 301-302: The schema entry currently sets "default": true which
contradicts the intended opt-in behavior; change the default to false (or remove
the "default" key) so the setting is not enabled by default, and keep the
description that unset values inherit app.warnBeforeClosingTab; update the
description text if needed to make explicit that users must opt in to enable the
X-button warning.
---
Outside diff comments:
In `@Sources/cmuxApp.swift`:
- Around line 4733-4745: The isXButtonWarningEnabled method currently falls back
to isEnabled(...) causing the X-button preference to inherit
warnBeforeClosingTab; change it to treat warnBeforeClosingTabXButtonKey
independently by returning defaultWarnBeforeClosingTabXButton when the X-button
key is unset instead of calling isEnabled(...). Update
isXButtonWarningEnabled(defaults:) to check defaults.object(forKey:
warnBeforeClosingTabXButtonKey) and return defaults.bool(forKey:
warnBeforeClosingTabXButtonKey) or defaultWarnBeforeClosingTabXButton, leaving
isEnabled(...) and defaultWarnBeforeClosingTab untouched.
🪄 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: c01d5df1-de9d-4686-a802-5e1698b126a4
📒 Files selected for processing (4)
Sources/CommandPalette/CommandPaletteSettingsToggle.swiftSources/cmuxApp.swiftcmuxTests/TabManagerUnitTests.swiftweb/data/cmux.schema.json
Dismissed after requested changes were addressed in later commits; CodeRabbit current check is passing and review threads are resolved.
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/cmuxApp.swift (1)
4759-4795:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftAdd a dedicated non-user close trigger instead of defaulting to
.shortcut.
Workspace.splitTabBar(...shouldCloseTab:)falls back tocloseConfirmationTrigger = .shortcutwhen a close isn’t explicitly marked viamarkExplicitClose(..., trigger:). Any programmatic/internal close path that callsbonsplitController.closeTab(...)/tab.closePanel(...)without marking (e.g. closing the default “Welcome” tabs) will therefore use the shortcut warning preference instead of a distinct non-user policy. Add a newCloseTabConfirmationTriggercase for non-user/programmatic closes and route those close requests through it.🤖 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/cmuxApp.swift` around lines 4759 - 4795, Add a new non-user/programmatic trigger to CloseTabConfirmationTrigger (e.g., case nonUser) and wire it through the confirmation logic: update CloseTabConfirmationPolicy.decision(...) to handle .nonUser explicitly (use the appropriate warning setting check or a new CloseTabWarningSettings API for non-user closes), change any fallback in Workspace.splitTabBar(...shouldCloseTab:) so it no longer defaults to .shortcut but uses .nonUser for unmarked programmatic closes, and ensure programmatic close call sites (bonsplitController.closeTab(...), tab.closePanel(...), or markExplicitClose(..., trigger:)) mark programmatic closes with .nonUser; also update any tests or settings references accordingly.
🤖 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 `@Resources/Localizable.xcstrings`:
- Around line 60979-61085: The localized entries for the string value "Warn
Before X-Button Tab Close" are marked as "translated" but still contain English
for multiple non-exempt locales; update the locale-specific values for the
listed locale keys ("de", "es", "fr", "it", "ko", "nb", "pt-BR", "ru", "uk",
"zh-Hans", "zh-Hant") replacing the copied-English text with proper translations
(no placeholders, machine markers, TODOs or empty strings) so each locale's
"stringUnit"."value" contains the correct translation while keeping the "state"
as "translated".
- Around line 77747-77853: The localization entries for
settings.search.alias.setting.app.warn-before-closing-tab-x-button in
Resources/Localizable.xcstrings incorrectly duplicate the English token
"app.warnBeforeClosingTabXButton ..." across many locales; replace those cloned
English values with real localized search-alias tokens (or mark locale as
exempt) so each non-exempt locale has an actual translated alias value instead
of the English placeholder, updating the stringUnit.value for the locales (e.g.,
"de", "fr", "ko", "zh-Hans", etc.) accordingly and ensuring state remains
"translated" only when a proper localized token is provided.
---
Outside diff comments:
In `@Sources/cmuxApp.swift`:
- Around line 4759-4795: Add a new non-user/programmatic trigger to
CloseTabConfirmationTrigger (e.g., case nonUser) and wire it through the
confirmation logic: update CloseTabConfirmationPolicy.decision(...) to handle
.nonUser explicitly (use the appropriate warning setting check or a new
CloseTabWarningSettings API for non-user closes), change any fallback in
Workspace.splitTabBar(...shouldCloseTab:) so it no longer defaults to .shortcut
but uses .nonUser for unmarked programmatic closes, and ensure programmatic
close call sites (bonsplitController.closeTab(...), tab.closePanel(...), or
markExplicitClose(..., trigger:)) mark programmatic closes with .nonUser; also
update any tests or settings references accordingly.
🪄 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: 1b3db8a4-3b0f-49f8-b1fb-a544564fa3c9
📒 Files selected for processing (7)
Resources/Localizable.xcstringsSources/TabManager.swiftSources/Workspace.swiftSources/WorkspaceCloseTabsBatching.swiftSources/cmuxApp.swiftcmuxTests/TabManagerUnitTests.swiftweb/data/cmux.schema.json
| "ar": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Warn Before X-Button Tab Close" | ||
| } | ||
| }, | ||
| "bs": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Warn Before X-Button Tab Close" | ||
| } | ||
| }, | ||
| "da": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Warn Before X-Button Tab Close" | ||
| } | ||
| }, | ||
| "de": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Warn Before X-Button Tab Close" | ||
| } | ||
| }, | ||
| "es": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Warn Before X-Button Tab Close" | ||
| } | ||
| }, | ||
| "fr": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Warn Before X-Button Tab Close" | ||
| } | ||
| }, | ||
| "it": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Warn Before X-Button Tab Close" | ||
| } | ||
| }, | ||
| "km": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Warn Before X-Button Tab Close" | ||
| } | ||
| }, | ||
| "ko": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Warn Before X-Button Tab Close" | ||
| } | ||
| }, | ||
| "nb": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Warn Before X-Button Tab Close" | ||
| } | ||
| }, | ||
| "pl": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Warn Before X-Button Tab Close" | ||
| } | ||
| }, | ||
| "pt-BR": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Warn Before X-Button Tab Close" | ||
| } | ||
| }, | ||
| "ru": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Warn Before X-Button Tab Close" | ||
| } | ||
| }, | ||
| "th": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Warn Before X-Button Tab Close" | ||
| } | ||
| }, | ||
| "tr": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Warn Before X-Button Tab Close" | ||
| } | ||
| }, | ||
| "uk": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Warn Before X-Button Tab Close" | ||
| } | ||
| }, | ||
| "zh-Hans": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Warn Before X-Button Tab Close" | ||
| } | ||
| }, | ||
| "zh-Hant": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Warn Before X-Button Tab Close" | ||
| } |
There was a problem hiding this comment.
Replace copied-English locale values with real translations for non-exempt locales.
These three new user-facing keys are marked "translated" but still use English copy for locales beyond the repo’s known fallback-exempt set. Please provide locale-specific translations (at least for de, es, fr, it, ko, nb, pt-BR, ru, uk, zh-Hans, zh-Hant) instead of duplicated English values.
As per coding guidelines: Placeholder, copied English, machine marker, TODO, or empty translations must not be used to satisfy a locale slot in **/Resources/*.xcstrings. Based on learnings: fallback-English is intentionally accepted only for ar, bs, da, pl, th, km, tr.
Also applies to: 61104-61210, 61229-61335
🤖 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 `@Resources/Localizable.xcstrings` around lines 60979 - 61085, The localized
entries for the string value "Warn Before X-Button Tab Close" are marked as
"translated" but still contain English for multiple non-exempt locales; update
the locale-specific values for the listed locale keys ("de", "es", "fr", "it",
"ko", "nb", "pt-BR", "ru", "uk", "zh-Hans", "zh-Hant") replacing the
copied-English text with proper translations (no placeholders, machine markers,
TODOs or empty strings) so each locale's "stringUnit"."value" contains the
correct translation while keeping the "state" as "translated".
| "ar": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "app.warnBeforeClosingTabXButton close tab confirmation x button mouse click tab strip terminal surface" | ||
| } | ||
| }, | ||
| "bs": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "app.warnBeforeClosingTabXButton close tab confirmation x button mouse click tab strip terminal surface" | ||
| } | ||
| }, | ||
| "da": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "app.warnBeforeClosingTabXButton close tab confirmation x button mouse click tab strip terminal surface" | ||
| } | ||
| }, | ||
| "de": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "app.warnBeforeClosingTabXButton close tab confirmation x button mouse click tab strip terminal surface" | ||
| } | ||
| }, | ||
| "es": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "app.warnBeforeClosingTabXButton close tab confirmation x button mouse click tab strip terminal surface" | ||
| } | ||
| }, | ||
| "fr": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "app.warnBeforeClosingTabXButton close tab confirmation x button mouse click tab strip terminal surface" | ||
| } | ||
| }, | ||
| "it": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "app.warnBeforeClosingTabXButton close tab confirmation x button mouse click tab strip terminal surface" | ||
| } | ||
| }, | ||
| "km": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "app.warnBeforeClosingTabXButton close tab confirmation x button mouse click tab strip terminal surface" | ||
| } | ||
| }, | ||
| "ko": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "app.warnBeforeClosingTabXButton close tab confirmation x button mouse click tab strip terminal surface" | ||
| } | ||
| }, | ||
| "nb": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "app.warnBeforeClosingTabXButton close tab confirmation x button mouse click tab strip terminal surface" | ||
| } | ||
| }, | ||
| "pl": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "app.warnBeforeClosingTabXButton close tab confirmation x button mouse click tab strip terminal surface" | ||
| } | ||
| }, | ||
| "pt-BR": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "app.warnBeforeClosingTabXButton close tab confirmation x button mouse click tab strip terminal surface" | ||
| } | ||
| }, | ||
| "ru": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "app.warnBeforeClosingTabXButton close tab confirmation x button mouse click tab strip terminal surface" | ||
| } | ||
| }, | ||
| "th": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "app.warnBeforeClosingTabXButton close tab confirmation x button mouse click tab strip terminal surface" | ||
| } | ||
| }, | ||
| "tr": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "app.warnBeforeClosingTabXButton close tab confirmation x button mouse click tab strip terminal surface" | ||
| } | ||
| }, | ||
| "uk": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "app.warnBeforeClosingTabXButton close tab confirmation x button mouse click tab strip terminal surface" | ||
| } | ||
| }, | ||
| "zh-Hans": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "app.warnBeforeClosingTabXButton close tab confirmation x button mouse click tab strip terminal surface" | ||
| } | ||
| }, | ||
| "zh-Hant": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "app.warnBeforeClosingTabXButton close tab confirmation x button mouse click tab strip terminal surface" | ||
| } |
There was a problem hiding this comment.
Localize the search-alias tokens per locale instead of cloning English text.
settings.search.alias.setting.app.warn-before-closing-tab-x-button currently fills most locale slots with identical English tokens. For non-exempt locales, this does not satisfy full i18n requirements for catalog entries marked translated.
As per coding guidelines: for Resources/*.xcstrings, copied English cannot be used as locale-fill translation.
🤖 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 `@Resources/Localizable.xcstrings` around lines 77747 - 77853, The localization
entries for settings.search.alias.setting.app.warn-before-closing-tab-x-button
in Resources/Localizable.xcstrings incorrectly duplicate the English token
"app.warnBeforeClosingTabXButton ..." across many locales; replace those cloned
English values with real localized search-alias tokens (or mark locale as
exempt) so each non-exempt locale has an actual translated alias value instead
of the English placeholder, updating the stringUnit.value for the locales (e.g.,
"de", "fr", "ko", "zh-Hans", etc.) accordingly and ensuring state remains
"translated" only when a proper localized token is provided.
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 57b81f7. Configure here.
| case .shortcutTabClose: | ||
| return CloseTabConfirmationPolicy.shouldConfirm(requiresConfirmation: requiresConfirmation, trigger: .shortcut) | ||
| case .tabCloseButton: | ||
| return CloseTabConfirmationPolicy.shouldConfirm(requiresConfirmation: requiresConfirmation, trigger: .tabCloseButton) |
There was a problem hiding this comment.
Pinned workspace skips confirmation
Medium Severity
Closing a pinned workspace from the sidebar X now uses the tab-close-button confirmation source. When app.warnBeforeClosingTabXButton is off, confirmPinnedWorkspaceClose treats that as “no confirmation needed” and closes immediately, unlike the previous closeWorkspaceWithConfirmation path that always showed the pinned-workspace dialog.
Additional Locations (1)
Reviewed by Cursor Bugbot for commit 57b81f7. Configure here.
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 `@web/data/cmux.schema.json`:
- Around line 299-302: Update the JSON schema description for the boolean
property warnBeforeClosingTabXButton to explicitly state the semantics for true
and false; change the description to something like: "When true, show a
close-tab confirmation before a tab-strip X button closes a tab; when false, do
not show the confirmation." Ensure this exact property name
(warnBeforeClosingTabXButton) is updated so consumers understand the boolean
conditions.
🪄 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: 71d9833a-9ca8-4ca4-bed4-473ee02486da
📒 Files selected for processing (4)
Sources/ContentView.swiftSources/cmuxApp.swiftcmuxTests/TabManagerUnitTests.swiftweb/data/cmux.schema.json
| "warnBeforeClosingTabXButton": { | ||
| "type": "boolean", | ||
| "default": true, | ||
| "description": "Always show the close-tab confirmation before a tab-strip X button closes a tab." |
There was a problem hiding this comment.
Clarify boolean semantics in the schema description.
The phrase “Always show…” reads as unconditional behavior. For a boolean setting, describe the true condition explicitly (for example, “When true, show…”), so users don’t interpret this as non-configurable behavior.
Suggested edit
"warnBeforeClosingTabXButton": {
"type": "boolean",
"default": true,
- "description": "Always show the close-tab confirmation before a tab-strip X button closes a tab."
+ "description": "When true, show the close-tab confirmation before a tab-strip X button closes a tab."
},🤖 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 `@web/data/cmux.schema.json` around lines 299 - 302, Update the JSON schema
description for the boolean property warnBeforeClosingTabXButton to explicitly
state the semantics for true and false; change the description to something
like: "When true, show a close-tab confirmation before a tab-strip X button
closes a tab; when false, do not show the confirmation." Ensure this exact
property name (warnBeforeClosingTabXButton) is updated so consumers understand
the boolean conditions.
Summary
app.warnBeforeClosingTabXButtonfor tab-strip X-button close confirmations.app.warnBeforeClosingTabas the existing shortcut/close-command preference.Preference keys
app.warnBeforeClosingTab: existing key, unchanged; still maps towarnBeforeClosingTabShortcutinternally.app.warnBeforeClosingTabXButton: new boolean key, maps towarnBeforeClosingTabXButtonin UserDefaults. Defaults totrue, so tab-strip X closes ask for confirmation by default. Users can set it tofalseto keep X-button closes immediate.Localization keys
settings.app.warnBeforeClosingTabXButtonsettings.app.warnBeforeClosingTabXButton.subtitleOnsettings.app.warnBeforeClosingTabXButton.subtitleOffsettings.search.alias.setting.app.warn-before-closing-tab-x-buttonMigration
No explicit migration is required. Existing Cmd+W/close-command behavior is unchanged because
app.warnBeforeClosingTabis unchanged. The newapp.warnBeforeClosingTabXButtonkey defaults totruewhen unset, so existing users get confirmation from the tab-strip X button by default and can opt out in Settings or cmux.json.Closes #4622.
Verification
CMUX_SKIP_ZIG_BUILD=1 ./scripts/reload.sh --tag issue-4622-tab-x-button-warngit diff --checkjq empty web/data/cmux.schema.jsonUnit tests were not run locally per repo policy; CI is the test verification path.
Summary by CodeRabbit