Add configurable close confirmation settings#2609
Add configurable close confirmation settings#2609arieltobiana wants to merge 3 commits intomanaflow-ai:mainfrom
Conversation
Each close confirmation dialog can now be independently toggled in Settings > App: - Confirm Close Running Process (workspace with running command) - Confirm Close Pinned Workspace - Confirm Close Window - Confirm Batch Close (multiple tabs/workspaces) All default to enabled, preserving existing behavior. Also configurable via `defaults write com.cmuxterm.app <key> -bool false`.
|
@arieltobiana is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds four UserDefaults-backed toggles to control close-confirmation dialogs (running processes, pinned workspaces, main window, batch closes) and gates confirmation flows in AppDelegate, TabManager, and Workspace based on those toggles. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant SettingsView as "SettingsView\n(UserDefaults)"
participant App as "AppDelegate"
participant TabMgr as "TabManager"
participant Workspace as "Workspace"
User->>SettingsView: Toggle close-confirmation settings
SettingsView->>SettingsView: Write to UserDefaults
User->>App: Close main window
App->>SettingsView: Read CloseWindowSettings.isEnabled()
alt CloseWindowSettings disabled
App-->>User: Close proceeds (no dialog)
else CloseWindowSettings enabled
App-->>User: Show confirmation dialog
end
User->>TabMgr: Request batch close / close others
TabMgr->>SettingsView: Read BatchCloseSettings.isEnabled()
alt BatchClose disabled
TabMgr->>Workspace: Close each workspace (per-workspace confirmation may apply)
else BatchClose enabled
TabMgr-->>User: Show batch confirmation dialog
TabMgr->>Workspace: Close each workspace (suppress per-workspace confirmation)
end
Workspace->>SettingsView: Read CloseRunningProcessSettings.isEnabled()
alt Process running & setting enabled
Workspace-->>User: Show running-process confirmation
else
Workspace-->>User: Close proceeds
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/TabManager.swift (1)
2904-2915:⚠️ Potential issue | 🟠 MajorBatch-close toggle currently suppresses running-process confirmations as a side effect.
When batch confirmation is disabled on Line 2904, the loop still closes each workspace with
requiresConfirmation: falseon Line 2914. That means the running-process confirmation path is bypassed for multi-workspace closes, so toggling batch-close impacts another confirmation behavior.💡 Proposed fix
let plan = closeWorkspacesPlan(for: workspaces) - if BatchCloseSettings.isEnabled() { + let shouldShowBatchConfirmation = BatchCloseSettings.isEnabled() + if shouldShowBatchConfirmation { guard confirmClose( title: plan.title, message: plan.message, acceptCmdD: plan.acceptCmdD ) else { return } } for workspace in plan.workspaces { guard tabs.contains(where: { $0.id == workspace.id }) else { continue } - closeWorkspaceIfRunningProcess(workspace, requiresConfirmation: false) + closeWorkspaceIfRunningProcess( + workspace, + requiresConfirmation: !shouldShowBatchConfirmation + ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 2904 - 2915, The batch-close toggle currently forces requiresConfirmation: false when calling closeWorkspaceIfRunningProcess, bypassing per-workspace running-process confirmation; change the call so requiresConfirmation is based on BatchCloseSettings.isEnabled() (i.e., pass requiresConfirmation: !BatchCloseSettings.isEnabled()) so that if batch confirmation was taken no per-workspace prompts occur, but if batch confirmation was not taken each workspace still prompts via closeWorkspaceIfRunningProcess(workspace, requiresConfirmation: !BatchCloseSettings.isEnabled()).
🤖 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 2830-2839: Replace the hardcoded English title and message in the
batch-close dialog (the strings used around BatchCloseSettings.isEnabled() where
you compute count from plan.panelIds and titleLines from plan.titles and call
confirmClose) with localized ICU-style keys using String(localized: "key.name",
defaultValue: "..."), creating plural keys (e.g. "batchClose.message.one" /
"batchClose.message.other") for the count-dependent line and a localized key for
the dialog title (e.g. "batchClose.title"); update the confirmClose call to use
those localized values so the dialog uses String(localized: ..., defaultValue:
...) for both the title and the message and supports .one/.other pluralization
for English/Japanese.
---
Outside diff comments:
In `@Sources/TabManager.swift`:
- Around line 2904-2915: The batch-close toggle currently forces
requiresConfirmation: false when calling closeWorkspaceIfRunningProcess,
bypassing per-workspace running-process confirmation; change the call so
requiresConfirmation is based on BatchCloseSettings.isEnabled() (i.e., pass
requiresConfirmation: !BatchCloseSettings.isEnabled()) so that if batch
confirmation was taken no per-workspace prompts occur, but if batch confirmation
was not taken each workspace still prompts via
closeWorkspaceIfRunningProcess(workspace, requiresConfirmation:
!BatchCloseSettings.isEnabled()).
🪄 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: 60c06ad1-f8a3-425b-bfb9-bb4696183569
📒 Files selected for processing (4)
Sources/AppDelegate.swiftSources/TabManager.swiftSources/Workspace.swiftSources/cmuxApp.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 4 files
You’re at about 83% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
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:2830">
P2: Skipping the batch-close dialog still forces `requiresConfirmation: false`, which bypasses per-workspace running-process confirmation when batch confirmation is disabled.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Greptile SummaryThis PR adds 4 independently togglable close confirmation settings (
Confidence Score: 4/5Two P1 findings need to be addressed before merge: a logic bug in the batch-close path and missing localization catalog entries Most of the implementation is correct and follows established patterns; the requiresConfirmation hardcoding bug and missing Localizable.xcstrings entries are P1 issues that should be fixed first Sources/TabManager.swift (requiresConfirmation bug in batch loop) and Sources/cmuxApp.swift (missing Localizable.xcstrings entries for 12 keys) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
CW[Close Window] --> CWCheck{CloseWindowSettings\n.isEnabled?}
CWCheck -->|false| CWSkip[Skip — close immediately]
CWCheck -->|true| CWDlg[Show Close window? dialog]
CP[Close Pinned Workspace] --> CPCheck{ClosePinnedWorkspaceSettings\n.isEnabled?}
CPCheck -->|false| CPFall[Fall through to\nrunning-process check]
CPCheck -->|true| CPDlg[Show Close pinned workspace? dialog]
BC[Batch Close 2+ workspaces] --> BCCheck{BatchCloseSettings\n.isEnabled?}
BCCheck -->|false| BCBug["⚠ Bug: skips batch dialog\nAND running-process check\nrequiresConfirmation: false hardcoded"]
BCCheck -->|true| BCDlg[Show batch dialog]
BCDlg --> BCLoop["Close each workspace\nrequiresConfirmation: false\n(correct — user already confirmed)"]
SW[Close Single Workspace] --> RPCheck{CloseRunningProcessSettings\n.isEnabled?}
RPCheck -->|false| RPSkip[Skip running-process dialog]
RPCheck -->|true| RPDlg[Show Close workspace? dialog]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| SettingsCardRow( | ||
| String(localized: "settings.app.confirmCloseRunningProcess", defaultValue: "Confirm Close Running Process"), | ||
| subtitle: confirmCloseRunningProcess | ||
| ? String(localized: "settings.app.confirmCloseRunningProcess.subtitleOn", defaultValue: "Show a confirmation before closing a workspace with a running command.") | ||
| : String(localized: "settings.app.confirmCloseRunningProcess.subtitleOff", defaultValue: "Close workspaces without confirmation, even with running commands.") | ||
| ) { | ||
| Toggle("", isOn: $confirmCloseRunningProcess) | ||
| .labelsHidden() | ||
| .controlSize(.small) | ||
| } | ||
|
|
||
| SettingsCardDivider() | ||
|
|
||
| SettingsCardRow( | ||
| String(localized: "settings.app.confirmClosePinnedWorkspace", defaultValue: "Confirm Close Pinned Workspace"), | ||
| subtitle: confirmClosePinnedWorkspace | ||
| ? String(localized: "settings.app.confirmClosePinnedWorkspace.subtitleOn", defaultValue: "Show a confirmation before closing a pinned workspace.") | ||
| : String(localized: "settings.app.confirmClosePinnedWorkspace.subtitleOff", defaultValue: "Close pinned workspaces without extra confirmation.") | ||
| ) { | ||
| Toggle("", isOn: $confirmClosePinnedWorkspace) | ||
| .labelsHidden() | ||
| .controlSize(.small) | ||
| } | ||
|
|
||
| SettingsCardDivider() | ||
|
|
||
| SettingsCardRow( | ||
| String(localized: "settings.app.confirmCloseWindow", defaultValue: "Confirm Close Window"), | ||
| subtitle: confirmCloseWindow | ||
| ? String(localized: "settings.app.confirmCloseWindow.subtitleOn", defaultValue: "Show a confirmation before closing a window with workspaces.") | ||
| : String(localized: "settings.app.confirmCloseWindow.subtitleOff", defaultValue: "Close windows without confirmation.") | ||
| ) { | ||
| Toggle("", isOn: $confirmCloseWindow) | ||
| .labelsHidden() | ||
| .controlSize(.small) | ||
| } | ||
|
|
||
| SettingsCardDivider() | ||
|
|
||
| SettingsCardRow( | ||
| String(localized: "settings.app.confirmBatchClose", defaultValue: "Confirm Batch Close"), | ||
| subtitle: confirmBatchClose | ||
| ? String(localized: "settings.app.confirmBatchClose.subtitleOn", defaultValue: "Show a confirmation before closing multiple tabs or workspaces at once.") | ||
| : String(localized: "settings.app.confirmBatchClose.subtitleOff", defaultValue: "Close multiple tabs or workspaces without confirmation.") | ||
| ) { | ||
| Toggle("", isOn: $confirmBatchClose) | ||
| .labelsHidden() |
There was a problem hiding this comment.
Localizable.xcstrings
All 12 new String(localized:defaultValue:) keys (settings.app.confirmCloseRunningProcess, .subtitleOn, .subtitleOff, and equivalents for confirmClosePinnedWorkspace, confirmCloseWindow, confirmBatchClose) are absent from Resources/Localizable.xcstrings. Per the project policy, every UI string key must be registered in the catalog with translations for all supported languages — including Japanese. Comparable existing keys (settings.app.warnBeforeQuit, settings.app.telemetry, etc.) all have full catalog entries. Until these are added, Japanese users will always see English text for all 4 new toggle rows.
There was a problem hiding this comment.
The String(localized:defaultValue:) keys are auto-generated by Xcode's string catalog extraction on build. Japanese translations would be a separate localization effort. Out of scope for this feature PR.
When batch confirmation is disabled, each workspace should still get its own running-process confirmation check. Previously, requiresConfirmation was always false after the batch dialog, which silently skipped per-workspace checks when the batch dialog was disabled.
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/TabManager.swift (1)
2866-2878:⚠️ Potential issue | 🟠 MajorDon't skip the running-process warning after confirming a pinned close.
Passing
requiresConfirmation: falsehere bypasses the running-process check entirely. A pinned workspace with an active command will ignoreconfirmCloseRunningProcess, so the new toggles are no longer independent on the single-workspace path.💡 Proposed fix
- closeWorkspaceIfRunningProcess(workspace, requiresConfirmation: false) + closeWorkspaceIfRunningProcess(workspace)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 2866 - 2878, The code bypasses the running-process confirmation by calling closeWorkspaceIfRunningProcess(workspace, requiresConfirmation: false) after a pinned-workspace confirm; change this so the running-process check still runs—e.g., call closeWorkspaceIfRunningProcess(workspace, requiresConfirmation: true) or call the variant that uses the default behavior (omit the false override) so confirmCloseRunningProcess still triggers for workspace.isPinned paths; update the call site referencing closeWorkspaceIfRunningProcess and the confirmClose(...) block accordingly.
♻️ Duplicate comments (1)
Sources/TabManager.swift (1)
2830-2838:⚠️ Potential issue | 🟠 MajorLocalize the new batch-close dialog strings.
This alert still hardcodes English copy. Please move the title/message to
String(localized:..., defaultValue:...), add the keys toResources/Localizable.xcstrings, and use.one/.otherkeys instead of manual"s"pluralization.As per coding guidelines: “All user-facing strings must be localized using
String(localized: "key.name", defaultValue: "English text")for every string shown in the UI. Keys must go inResources/Localizable.xcstrings…” Based on learnings: “when handling pluralized strings, prefer using localization keys with the ICU-style plural forms.oneand.other.”🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager.swift` around lines 2830 - 2838, The batch-close alert currently hardcodes English text; change the title and message passed to confirmClose into localized strings using String(localized:..., defaultValue:...) and add corresponding keys to Resources/Localizable.xcstrings; for the plural tab count use an ICU-style plural entry (with .one and .other variants) instead of manually appending "s" — locate the block guarded by BatchCloseSettings.isEnabled() that builds count, titleLines and calls confirmClose and replace the literal title and message with localized variants that reference the plural key for the tab count and a separate key for the pane listing.
🤖 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 2904-2918: Batch closes skip pinned-workspace confirmation because
the loop always calls closeWorkspaceIfRunningProcess(...) even when pinned and
confirmations are required; change the loop to call
closeWorkspaceWithConfirmation(workspace) for pinned workspaces when
confirmations are required (i.e., when !batchConfirmed) and only call
closeWorkspaceIfRunningProcess(workspace, requiresConfirmation: !batchConfirmed)
for non-pinned or when batchConfirmed is true. Update the for loop around
plan.workspaces to check workspace.isPinned (or the equivalent property) and
branch accordingly so confirmClosePinnedWorkspace logic is honored.
---
Outside diff comments:
In `@Sources/TabManager.swift`:
- Around line 2866-2878: The code bypasses the running-process confirmation by
calling closeWorkspaceIfRunningProcess(workspace, requiresConfirmation: false)
after a pinned-workspace confirm; change this so the running-process check still
runs—e.g., call closeWorkspaceIfRunningProcess(workspace, requiresConfirmation:
true) or call the variant that uses the default behavior (omit the false
override) so confirmCloseRunningProcess still triggers for workspace.isPinned
paths; update the call site referencing closeWorkspaceIfRunningProcess and the
confirmClose(...) block accordingly.
---
Duplicate comments:
In `@Sources/TabManager.swift`:
- Around line 2830-2838: The batch-close alert currently hardcodes English text;
change the title and message passed to confirmClose into localized strings using
String(localized:..., defaultValue:...) and add corresponding keys to
Resources/Localizable.xcstrings; for the plural tab count use an ICU-style
plural entry (with .one and .other variants) instead of manually appending "s" —
locate the block guarded by BatchCloseSettings.isEnabled() that builds count,
titleLines and calls confirmClose and replace the literal title and message with
localized variants that reference the plural key for the tab count and a
separate key for the pane listing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
When batch confirmation is disabled, route each workspace through closeWorkspaceWithConfirmation so pinned workspace and running-process confirmations are each independently honored.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
You’re at about 85% of the monthly review limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
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:2921">
P2: Non-batch multi-close now bypasses running-process confirmation for pinned workspaces by routing through a helper that forces `requiresConfirmation: false` after pinned confirmation.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if batchConfirmed { | ||
| closeWorkspaceIfRunningProcess(workspace, requiresConfirmation: false) | ||
| } else { | ||
| _ = closeWorkspaceWithConfirmation(workspace) |
There was a problem hiding this comment.
P2: Non-batch multi-close now bypasses running-process confirmation for pinned workspaces by routing through a helper that forces requiresConfirmation: false after pinned confirmation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/TabManager.swift, line 2921:
<comment>Non-batch multi-close now bypasses running-process confirmation for pinned workspaces by routing through a helper that forces `requiresConfirmation: false` after pinned confirmation.</comment>
<file context>
@@ -2915,7 +2915,11 @@ class TabManager: ObservableObject {
+ if batchConfirmed {
+ closeWorkspaceIfRunningProcess(workspace, requiresConfirmation: false)
+ } else {
+ _ = closeWorkspaceWithConfirmation(workspace)
+ }
}
</file context>
There was a problem hiding this comment.
The requiresConfirmation: false after pinned-workspace confirmation in closeWorkspaceWithConfirmation is a pre-existing pattern from before this PR. The only change on that line was adding && ClosePinnedWorkspaceSettings.isEnabled() to the condition. The running-process bypass after pinned confirm predates this PR and is out of scope — can be addressed separately if desired.
1 participant
Summary
New settings
confirmCloseRunningProcessconfirmClosePinnedWorkspaceconfirmCloseWindowconfirmBatchCloseSettings are also configurable via
defaults write:defaults write com.cmuxterm.app confirmCloseRunningProcess -bool falseFiles changed
Sources/cmuxApp.swift— Settings enums,@AppStorageproperties, UI toggles, reset defaultsSources/Workspace.swift— GateresolveCloseConfirmationrunning-process caseSources/TabManager.swift— Gate pinned workspace and batch close confirmationsSources/AppDelegate.swift— Gate close window confirmationTest plan
Summary by cubic
Added four per-action toggles in Settings > App to control close confirmations. Users can turn off prompts for running processes, pinned workspaces, window close, and batch closes; defaults stay on.
New Features
confirmCloseRunningProcess), Confirm Close Pinned Workspace (confirmClosePinnedWorkspace), Confirm Close Window (confirmCloseWindow), Confirm Batch Close (confirmBatchClose).defaults write com.cmuxterm.app <key> -bool <true|false>.Bug Fixes
Written for commit d11667e. Summary will update on new commits.
Summary by CodeRabbit
New Features
Behavior Changes