Repository navigation
Honor keep workspace open on last surface close - #6475
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:
📝 WalkthroughWalkthroughExtracts a ChangesLast-surface close preference gating
Sequence DiagramsequenceDiagram
participant User
participant Workspace as Workspace delegate
participant TabManager
participant UserDefaults
User->>Workspace: close last surface (tab-strip, middle-click, or other)
Workspace->>Workspace: extract tabStripClose and tabCloseButtonClose from tabStripCloseSources
Workspace->>Workspace: check panels.count <= 1 and panel exists
alt non-tab-strip close gesture
Workspace-->>User: allow workspace close unconditionally
else tab-strip or middle-click gesture
Workspace->>TabManager: closeWorkspaceOnLastSurfacePreferenceEnabled()
TabManager->>UserDefaults: read closeWorkspaceOnLastSurface setting
UserDefaults-->>TabManager: Bool
TabManager-->>Workspace: Bool
Workspace-->>User: close workspace only if preference enabled
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (21 passed)
✨ 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 |
Greptile SummaryThis PR fixes #6442 by making the "Keep Workspace Open When Closing Last Surface" preference apply to the tab-strip close button and middle-click paths, not only the keyboard shortcut path. The central change is replacing the unconditional
Confidence Score: 5/5Safe to merge; the preference-reading fix is narrowly scoped to tab-strip close paths, the keyboard-shortcut path is structurally unchanged, and the remote-tmux state machine is covered by nine new regression tests with isolated UserDefaults suites. All four supported locales are updated for both string-catalog keys. closeWorkspaceOnLastSurfacePreferenceEnabled() is exposed at internal visibility for a real production caller in Workspace.swift, not as a debug seam. The tabStripClose flag correctly threads through shouldCloseWorkspaceOnLastSurface so that tabStripClose=false (shortcut) preserves the pre-existing always-close behavior while tabStripClose=true (close button, middle-click) gates on the preference. No blocking or data-loss paths identified. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[User closes last surface] --> B{Close trigger}
B -->|Keyboard shortcut| C[TabManager.closePanelWithConfirmation]
B -->|Tab close button| D[Workspace.markTabCloseButtonClose]
B -->|Middle-click| E[Workspace.markTabStripMiddleClickClose]
C --> F{shouldCloseWorkspaceOnLastSurfaceShortcut reads keepOpen pref}
F -->|pref = close workspace| G[markExplicitClose + closeWorkspaceWithConfirmation]
F -->|pref = keep open| H[Close panel only workspace stays]
D --> I[Bonsplit shouldCloseTab tabStripClose=true]
E --> I
I --> J{lastSurfaceClosePreference tabStripClose=true reads pref}
J -->|pref = close workspace| K[closeWorkspaceFromTabCloseButton or closeWorkspaceFromCloseTabGesture]
J -->|pref = keep open| L[Close panel only workspace stays]
I --> M{isRemoteTmuxMirror?}
M -->|Yes| N[recordRemoteTmuxWorkspaceCloseAfterWindowClose]
N --> O{markRemoteTmuxWorkspaceCloseAfterWindowCloseIfNeeded}
O -->|shouldClose| P[remoteTmuxWorkspaceCloseButtonByTabId set]
O -->|shouldKeepOpen| Q[remoteTmuxKeepWorkspaceOpenAfterSessionEnd=true]
P --> R[splitTabBar didCloseTab detachMirrorWorkspaceKeptOpenLocally + closeWorkspace]
Q --> S[RemoteTmuxController handleSessionEndedRemotely handleRemoteTmuxSessionEndedKeepingWorkspaceOpenIfNeeded convert to local workspace]
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
flowchart TD
A[User closes last surface] --> B{Close trigger}
B -->|Keyboard shortcut| C[TabManager.closePanelWithConfirmation]
B -->|Tab close button| D[Workspace.markTabCloseButtonClose]
B -->|Middle-click| E[Workspace.markTabStripMiddleClickClose]
C --> F{shouldCloseWorkspaceOnLastSurfaceShortcut reads keepOpen pref}
F -->|pref = close workspace| G[markExplicitClose + closeWorkspaceWithConfirmation]
F -->|pref = keep open| H[Close panel only workspace stays]
D --> I[Bonsplit shouldCloseTab tabStripClose=true]
E --> I
I --> J{lastSurfaceClosePreference tabStripClose=true reads pref}
J -->|pref = close workspace| K[closeWorkspaceFromTabCloseButton or closeWorkspaceFromCloseTabGesture]
J -->|pref = keep open| L[Close panel only workspace stays]
I --> M{isRemoteTmuxMirror?}
M -->|Yes| N[recordRemoteTmuxWorkspaceCloseAfterWindowClose]
N --> O{markRemoteTmuxWorkspaceCloseAfterWindowCloseIfNeeded}
O -->|shouldClose| P[remoteTmuxWorkspaceCloseButtonByTabId set]
O -->|shouldKeepOpen| Q[remoteTmuxKeepWorkspaceOpenAfterSessionEnd=true]
P --> R[splitTabBar didCloseTab detachMirrorWorkspaceKeptOpenLocally + closeWorkspace]
Q --> S[RemoteTmuxController handleSessionEndedRemotely handleRemoteTmuxSessionEndedKeepingWorkspaceOpenIfNeeded convert to local workspace]
Reviews (32): Last reviewed commit: "fix: preserve window on remote last tab ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/Workspace.swift`:
- Around line 3475-3481: The functions markTabStripMiddleClickClose and the
unnamed function at line 3475 are missing the markExplicitClose side effect that
populates closeHistoryEligibleTabIds and closeHistoryEligiblePanelIds, which
breaks close-undo history functionality. Restore the missing markExplicitClose
call in both functions to ensure that tab and panel closes are properly tracked
for close history eligibility, allowing pushClosedPanelHistoryIfEligible to
capture UI-initiated closes correctly.
🪄 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: e1bc5d7f-fe5d-4d3e-91a0-fb6bd3a95a92
📒 Files selected for processing (2)
Sources/Workspace.swiftcmuxTests/LastSurfaceClosePreferenceTests.swift
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Workspace.swift (1)
11752-11759:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPass the close trigger explicitly for the gesture close path.
This branch now routes last-surface middle-click / non-close-button closes through
closeWorkspaceFromCloseTabGesturewithout an explicit trigger, so it can fall back to the wrong confirmation policy if the default changes or is not the intended.shortcuttrigger.Suggested fix
if tabCloseButtonClose { owningTabManager?.closeWorkspaceFromTabCloseButton(self) } else { - owningTabManager?.closeWorkspaceFromCloseTabGesture(self) + owningTabManager?.closeWorkspaceFromCloseTabGesture(self, trigger: .shortcut) }Based on learnings, “close-tab confirmation trigger must be explicit and wired through all close flows;
TabManager.closeWorkspaceFromCloseTabGesture(_:trigger:)must NOT rely on default parameter values for the trigger.”🤖 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/Workspace.swift` around lines 11752 - 11759, The else branch that calls owningTabManager?.closeWorkspaceFromCloseTabGesture(self) is not passing an explicit trigger parameter, causing it to rely on a default parameter value for the close confirmation trigger. Modify the closeWorkspaceFromCloseTabGesture call to include an explicit trigger parameter (the appropriate trigger value should match the close trigger semantics for gesture-based closes) rather than relying on the method's default parameter, ensuring the confirmation policy is explicitly defined for this close path.Source: Learnings
🤖 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.
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 11752-11759: The else branch that calls
owningTabManager?.closeWorkspaceFromCloseTabGesture(self) is not passing an
explicit trigger parameter, causing it to rely on a default parameter value for
the close confirmation trigger. Modify the closeWorkspaceFromCloseTabGesture
call to include an explicit trigger parameter (the appropriate trigger value
should match the close trigger semantics for gesture-based closes) rather than
relying on the method's default parameter, ensuring the confirmation policy is
explicitly defined for this close path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 7547c7e0-e091-486e-ad05-cb96f6434c11
📒 Files selected for processing (1)
Sources/Workspace.swift
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Workspace.swift (1)
11107-11115:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winInvert the keep-open preference when deciding to close the workspace.
TabManager.closeWorkspaceOnLastSurfacePreferenceEnabled()readskeepWorkspaceOpenWhenClosingLastSurface, so Line 11115 currently closes the workspace when the keep-open setting is enabled and keeps it open when disabled.🐛 Proposed fix
- return !tabStripClose || manager.closeWorkspaceOnLastSurfacePreferenceEnabled() + let keepWorkspaceOpen = manager.closeWorkspaceOnLastSurfacePreferenceEnabled() + return !tabStripClose || !keepWorkspaceOpenAlso applies to: 11753-11760
🤖 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/Workspace.swift` around lines 11107 - 11115, The logic in the shouldCloseWorkspaceOnLastSurface method is inverted because closeWorkspaceOnLastSurfacePreferenceEnabled() actually returns the value of keepWorkspaceOpenWhenClosingLastSurface. In the return statement on line 11115, negate the result of manager.closeWorkspaceOnLastSurfacePreferenceEnabled() by wrapping it with the NOT operator (!) so that when the keep-open preference is enabled, the workspace correctly stays open instead of closing. Apply the same fix to the similar code block referenced at lines 11753-11760.
🤖 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.
Outside diff comments:
In `@Sources/Workspace.swift`:
- Around line 11107-11115: The logic in the shouldCloseWorkspaceOnLastSurface
method is inverted because closeWorkspaceOnLastSurfacePreferenceEnabled()
actually returns the value of keepWorkspaceOpenWhenClosingLastSurface. In the
return statement on line 11115, negate the result of
manager.closeWorkspaceOnLastSurfacePreferenceEnabled() by wrapping it with the
NOT operator (!) so that when the keep-open preference is enabled, the workspace
correctly stays open instead of closing. Apply the same fix to the similar code
block referenced at lines 11753-11760.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 23ae6b47-d618-4c46-a9ff-043990343b7a
📒 Files selected for processing (2)
Sources/Workspace.swiftcmuxTests/LastSurfaceClosePreferenceTests.swift
|
Addressing the latest CodeRabbit summary: the source-artifacts item is a false positive for this PR. The branch diff against origin/main only changes Sources/TabManager.swift, Sources/Workspace.swift, cmux.xcodeproj/project.pbxproj, and cmuxTests/LastSurfaceClosePreferenceTests.swift; it does not add .claude, .agents, .cursor, .coderabbit.yaml, or .vercelignore. The description warning was addressed by adding explicit Testing, Demo Video, and Review Notes sections. |
This branch was successfully deployed
1 participant
Summary
Testing
Demo Video
Review Notes
git diff origin/main...HEAD --name-statusonly listsSources/TabManager.swift,Sources/Workspace.swift,cmux.xcodeproj/project.pbxproj, andcmuxTests/LastSurfaceClosePreferenceTests.swift.Fixes #6442