Repository navigation
Add snooze/hide workspace - #4265
Closed
austinywang wants to merge 58 commits into
Closed
austinywang wants to merge 58 commits into
austinywang wants to merge 58 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 persistent workspace hidden state, TabManager visible-only APIs and invariants, sidebar hidden-workspaces UI and drag/drop handling, V2 and CLI hide/show/include_hidden semantics, localization, and tests. ChangesWorkspace Visibility (Hide/Snooze)
Sequence Diagram(s)(omitted — changes are primarily internal UI/model/handler updates; no new external multi-component sequential flow that needs visualization) Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning, 1 inconclusive)
✅ Passed checks (12 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?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: 5
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)
5698-5710:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSkip hidden workspaces in history without deleting them.
These loops currently treat hidden workspaces like closed workspaces and remove them from
tabHistory. After that workspace is woken again, back/forward navigation can no longer return to it because its history entry was discarded. Only prune entries when the workspace no longer exists.Suggested fix
func navigateBack() { guard historyIndex > 0 else { return } // Find the previous valid tab in history (skip closed tabs) var targetIndex = historyIndex - 1 while targetIndex >= 0 { let tabId = tabHistory[targetIndex] - if tabs.contains(where: { $0.id == tabId && !$0.isHidden }) { - isNavigatingHistory = true - historyIndex = targetIndex - selectedTabId = tabId - isNavigatingHistory = false - return + if let workspace = tabs.first(where: { $0.id == tabId }) { + if !workspace.isHidden { + isNavigatingHistory = true + historyIndex = targetIndex + selectedTabId = tabId + isNavigatingHistory = false + return + } + targetIndex -= 1 + continue } // Remove closed tab from history tabHistory.remove(at: targetIndex) historyIndex -= 1 targetIndex -= 1 @@ func navigateForward() { guard historyIndex < tabHistory.count - 1 else { return } // Find the next valid tab in history (skip closed tabs) - let targetIndex = historyIndex + 1 + var targetIndex = historyIndex + 1 while targetIndex < tabHistory.count { let tabId = tabHistory[targetIndex] - if tabs.contains(where: { $0.id == tabId && !$0.isHidden }) { - isNavigatingHistory = true - historyIndex = targetIndex - selectedTabId = tabId - isNavigatingHistory = false - return + if let workspace = tabs.first(where: { $0.id == tabId }) { + if !workspace.isHidden { + isNavigatingHistory = true + historyIndex = targetIndex + selectedTabId = tabId + isNavigatingHistory = false + return + } + targetIndex += 1 + continue } // Remove closed tab from history tabHistory.remove(at: targetIndex) // Don't increment targetIndex since we removed the element }Also applies to: 5718-5730
🤖 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/TabManager.swift` around lines 5698 - 5710, The loop that walks tabHistory currently treats hidden workspaces as closed by removing their entries; change the logic in the block using tabHistory/targetIndex/historyIndex to skip entries where a tab exists but isHidden (i.e., if tabs.contains(where: { $0.id == tabId }) then just decrement targetIndex/historyIndex without removing), and only remove an entry when no tab with that id exists (tabs.contains(where: { $0.id == tabId }) == false). Preserve the existing state updates (isNavigatingHistory = true/false and selectedTabId = tabId) when you find a non-hidden tab, and apply the same fix to the other analogous loop handling history around the code that also manipulates tabHistory and historyIndex.
🤖 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/cmuxApp.swift`:
- Around line 1070-1074: The Close Other Workspaces button currently uses
manager.tabs and manager.tabs.count and thus can act on hidden workspaces;
update the enablement and action to operate only on visible tabs and ignore
hidden workspaces: change the disabled check to use workspace == nil ||
workspace?.isHidden == true || manager.visibleTabs.count <= 1 (or equivalent API
that returns only non-hidden tabs) and modify or replace the call to
closeOtherSelectedWorkspacePeers(in:) so that it either filters manager.tabs for
only non-hidden/visible peers before closing or create a new helper (e.g.,
closeOtherSelectedVisibleWorkspacePeers) that resolves peers from
manager.visibleTabs and only closes those. Ensure the Button label still calls
the new/updated helper so hidden workspaces are not affected.
In `@Sources/ContentView.swift`:
- Around line 9818-9833: The ForEach in workspaceRows still lets views created
below it (e.g., TabItemView, SidebarEmptyArea) capture live ObservableObjects
like TabManager/NotificationStore via renderContext, so convert the data passed
into immutable snapshot structs and closure bundles: create a lightweight value
type (e.g., WorkspaceRowSnapshot and DropGapSnapshot) with only the fields the
row needs plus action closures (select, close, openContextMenu, onDrop), change
workspaceRow(...) to accept these snapshots instead of
Workspace/WorkspaceListRenderContext, and update TabItemView and
SidebarEmptyArea to read only from those snapshots and invoke closures for
mutations; ensure no ObservableObject instances or NotificationStore references
are captured below the ForEach boundary.
- Around line 9909-9917: Replace the hard-coded plural header by adding ICU
plural entries (one/other) under a new key like "sidebar.hiddenWorkspaces" in
your .stringsdict, then call the pluralized localization with the numeric
argument instead of formatting manually: switch the current String(localized:
"sidebar.hiddenWorkspaces.title", defaultValue: "Hidden Workspaces (%d)") usage
to String(localized: "sidebar.hiddenWorkspaces", locale: .current,
renderContext.hiddenWorkspaceCount) (keeping renderContext.hiddenWorkspaceCount
as the numeric parameter) so the OS picks the correct plural form for all
locales.
In `@Sources/TabManager.swift`:
- Around line 4311-4318: The current
visibleWorkspaceIdForFocus(afterRemovingAt:) falls back to the first visible tab
overall, causing focus to jump to a non-adjacent workspace; change the fallback
to search backward from just before the removed index for the nearest visible
workspace. In function visibleWorkspaceIdForFocus(afterRemovingAt removedIndex:
Int), after trying the forward search (startIndex..<tabs.count), add a backwards
search (e.g., stride(from: startIndex - 1, through: 0, by: -1) or a reverse
loop) to find the first tabs[i] where !tabs[i].isHidden and return its id; only
if that also fails keep the existing tabs.first(where: { !$0.isHidden })?.id (or
return nil if no visible tabs).
- Around line 4054-4069: The fallback search in nextVisibleWorkspaceId currently
uses beforeRange.prefix(hiddenIndex).first(where:) which finds the first visible
tab from the start of the list instead of the nearest previous visible tab;
change the before-range search to iterate backward from hiddenIndex so it
returns the closest visible workspace before hiddenIndex (e.g., use a
reversed/prior-indices search such as reversing the prefix or using last(where:)
on the prefix) and return tabs[thatIndex].id; refer to nextVisibleWorkspaceId,
hiddenIndex and beforeRange to locate the site to change.
---
Outside diff comments:
In `@Sources/TabManager.swift`:
- Around line 5698-5710: The loop that walks tabHistory currently treats hidden
workspaces as closed by removing their entries; change the logic in the block
using tabHistory/targetIndex/historyIndex to skip entries where a tab exists but
isHidden (i.e., if tabs.contains(where: { $0.id == tabId }) then just decrement
targetIndex/historyIndex without removing), and only remove an entry when no tab
with that id exists (tabs.contains(where: { $0.id == tabId }) == false).
Preserve the existing state updates (isNavigatingHistory = true/false and
selectedTabId = tabId) when you find a non-hidden tab, and apply the same fix to
the other analogous loop handling history around the code that also manipulates
tabHistory and historyIndex.
🪄 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: 38a20f72-bbbd-43ea-9d05-b1dabbfa1d08
📒 Files selected for processing (12)
CLI/cmux.swiftResources/Localizable.xcstringsSources/AppDelegate.swiftSources/CmuxSocketEventMapper.swiftSources/ContentView.swiftSources/SessionPersistence.swiftSources/TabManager.swiftSources/TerminalController.swiftSources/Workspace.swiftSources/cmuxApp.swiftcmux.xcodeproj/project.pbxprojcmuxTests/WorkspaceVisibilityTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 12 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/TabManager.swift">
<violation number="1" location="Sources/TabManager.swift:4317">
P2: Close/detach focus fallback jumps to the first visible workspace instead of the nearest previous visible workspace.</violation>
</file>
<file name="Sources/TerminalController.swift">
<violation number="1" location="Sources/TerminalController.swift:4326">
P2: `workspace.list` should reject malformed `workspace_id` values; it currently treats them as absent and returns the normal list.</violation>
</file>
<file name="Sources/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:14951">
P2: After drag/drop, the sidebar stores selection index against `tabManager.tabs` instead of visible tabs, which breaks subsequent Shift-range selection when hidden workspaces exist.</violation>
</file>
<file name="Sources/cmuxApp.swift">
<violation number="1" location="Sources/cmuxApp.swift:1073">
P2: Use `visibleWorkspaceTabs.count` for this disable condition so hidden workspaces do not affect whether “Close Other Workspaces” is enabled.</violation>
</file>
<file name="Resources/Localizable.xcstrings">
<violation number="1" location="Resources/Localizable.xcstrings:3088">
P3: Define this label with pluralized localization variants (e.g., `.one` / `.other`) instead of a single hard-coded plural string so counts like 1 render correctly across locales.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
Contributor
Greptile SummaryThis PR introduces workspace snooze/hide across the full stack: a new
Confidence Score: 4/5Safe to merge with one socket API inconsistency to fix: workspace.action move_top returns a raw all-tabs index while move_up/move_down now return visible indices. The workspace.action move_top handler was not updated alongside move_up/move_down: it still calls tabManager.tabs.firstIndex (raw position) while the other two actions now call visibleWorkspaceTabs.firstIndex (visible position). Any script or extension that chains move_top with move_up/move_down or compares the result against workspace.list indices will get inconsistent workspace positions. All other previously flagged issues (workspace.reorder, workspace.list workspace_id filtering, workspace.move_to_window last-visible error code, lastSidebarSelectionIndex raw-index overwrite, and translation coverage) are addressed in this iteration. Sources/TerminalController.swift — the workspace.action move_top case at line 7141. Important Files Changed
Sequence DiagramsequenceDiagram
participant CLI as CLI / Socket caller
participant TC as TerminalController
participant TM as TabManager
participant WS as Workspace
CLI->>TC: "workspace.hide {workspace_id}"
TC->>TM: canHideWorkspaces([id])
TM-->>TC: false → last_visible_workspace error
TM-->>TC: true → proceed
TC->>TM: setWorkspaceHidden(workspace, hidden: true)
TM->>WS: "isHidden = true"
TM->>TM: nextVisibleWorkspaceId(afterHiding:)
TM->>TM: selectWorkspaceId(nextId)
TM-->>TC: true
TC-->>CLI: "{hidden: true, selected_workspace_id: nextId, index: nil}"
CLI->>TC: "workspace.show {workspace_id}"
TC->>TM: setWorkspaceHidden(workspace, hidden: false)
TM->>WS: "isHidden = false"
TM-->>TC: true
TC-->>CLI: "{hidden: false, index: visibleIndex}"
CLI->>TC: "workspace.list {include_hidden: false}"
TC->>TM: workspaceTabs(includeHidden: false)
TM-->>TC: visible workspaces with 0-based visible indices
TC-->>CLI: "{workspaces: [...], include_hidden: false}"
Reviews (27): Last reviewed commit: "Route socket workspace.action move/close..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
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 `@Sources/TabManager.swift`:
- Around line 5409-5441: When selectedTabId refers to a hidden workspace the
current logic falls back to visibleTabs.first, causing jumps to the start
instead of the adjacent visible workspace; update selectNextTab() and
selectPreviousTab() so that if currentId is not found in visibleWorkspaceTabs
you locate currentId in the full ordered tab list (the app's master tab array,
e.g. workspaceTabs/allTabs), then search forward (for next) or backward (for
prev) from that position wrapping around until you find the next
visibleWorkspaceTabs element, and set selectedTabId to that id (instead of
visibleTabs.first). Keep using activateWorkspaceCycleHotWindow() and preserve
the DEBUG debugPrepareWorkspaceSwitch calls.
- Around line 4031-4051: The setWorkspaceHidden(_ tab: Workspace, hidden: Bool)
function mutates the passed Workspace instance which may be a stale detached
object after restoreSessionSnapshot() replaces tabs with fresh Workspace
instances; instead, resolve the live workspace from the tabs array by finding
the workspace with the same id (use tabs.first(where: { $0.id == tab.id }) or an
index lookup) and perform all checks and mutations (isHidden,
sidebarSelectedWorkspaceIds, selectedTabId update, nextVisibleWorkspaceId call)
on that resolved live instance before assigning tabs = tabs and returning;
ensure you still respect canHideWorkspaces([tab.id]) by checking against the
resolved workspace id and preserve the existing early returns and nextSelection
logic but operate on the live workspace object.
🪄 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: 879e403a-0cd2-4585-a512-ab3f6e8f1a0d
📒 Files selected for processing (6)
Resources/Localizable.xcstringsSources/ContentView+MoveTabToNewWorkspace.swiftSources/ContentView.swiftSources/TabManager.swiftSources/cmuxApp.swiftcmuxTests/WorkspaceVisibilityTests.swift
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 dec325c. Configure here.
…e-workspace # Conflicts: # CLI/cmux.swift # Sources/CmuxSocketEventMapper.swift # Sources/ContentView.swift # Sources/TabManager.swift # Sources/Workspace.swift
The snooze/hide workspace feature adds lines to existing files (ContentView, TabManager, CLI/cmux, TerminalController, etc.); refresh the tracked budget to the post-merge actuals. Also picks up reductions for files that shrank on main (TerminalNotificationStore, FeedCoordinator). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…e-workspace # Conflicts: # .github/swift-file-length-budget.tsv
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The visible-index reorder plan rejected out-of-range indices before clampedVisibleReorderIndex could clamp them, regressing CLI/socket reorder callers (e.g. workspace.reorder --index 999 meaning 'move to bottom') that historically clamped via workspaceReorderPlan(toIndex:). Remove the bounds guard and rely on the clamp; add regression coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Two interactions surfaced after merging main's workspace-groups subsystem: 1. Snoozing a group's anchor left the anchor visible: the visible-only render still emitted a group header keyed on the hidden anchor id, so the snoozed workspace stayed in the sidebar as the header. renderItems now suppresses a group header when its anchor is not in the visible set and renders the visible children as plain rows until woken. 2. Reveal-on-select only ran in selectWorkspace(_:), so focus paths that route through selectWorkspaceId directly (focusTab / notification jump-to-unread, move-with-focus) could make a hidden workspace the active terminal with no sidebar row. Move the reveal into the shared selectWorkspaceId choke point. Adds regression coverage for both. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The feature branch was pinned to an older ghostty (f78189a, an ancestor of main's 34cbf18); the main merges failed to fast-forward the gitlink. Point it at main's current SHA so builds pick up the cmd-click link, tmux bridge, and surface registry fixes already on main. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
workspace.action move_up/move_down and close_others/above/below operated on raw tabManager.tabs, so a snoozed (hidden) workspace between visible rows made move_down a visual no-op and close-others/above/below could close snoozed workspaces that should keep running. Route them through the visible workspace list, matching the UI context-menu/keyboard path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Comment on lines
7139
to
7141
| case "move_top": | ||
| tabManager.moveTabToTop(workspace.id) | ||
| finish(["index": v2OrNull(tabManager.tabs.firstIndex(where: { $0.id == workspace.id }))]) |
Contributor
There was a problem hiding this comment.
move_top response index not updated to visible space
move_up and move_down were both updated to return visibleWorkspaceTabs.firstIndex (visible index) in this PR, but move_top still returns tabManager.tabs.firstIndex (raw all-tabs index). After any workspace.action move_top, the response index diverges from the index returned by workspace.list (default) and from the index returned by move_up/move_down — all three actions now promise a consistent visible-space index, but move_top silently breaks that contract.
Concrete failure: [HiddenX(0), A(1), B(2)]. workspace.action workspace_id=B action=move_top moves B to visible slot 0 and raw slot 1. The response reports index: 1 (raw). A follow-up workspace.list shows B at index: 0 (visible). A script that feeds the move_top response index back into a workspace.reorder or workspace.action move_up/move_down call will target the wrong workspace.
lawrencecchen
added
the
stale-revisit
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
3 participants
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 #4261
Summary
Notes
Verification
Note
Medium Risk
Changes core workspace navigation, sidebar selection, and CLI/socket contracts across a large surface area, though behavior is guarded (e.g. last-visible workspace) and mostly additive.
Overview
Introduces workspace snooze/hide: workspaces can be removed from the sidebar without closing them or stopping notifications, with state persisted in session snapshots (
isHidden).CLI / socket: New
hide-workspace/show-workspace(andworkspace hide/show) call v2workspace.hide/workspace.show;list-workspacesandworkspace listgain--include-hidden. Workspace handle resolution can passinclude_hiddenfor UUID/ref targeting while numeric indices stay on the visible list. Hide/show commands skip pre-dispatch window focus. Socket mapper emitsworkspace.visibility_changedandworkspace.reordered.App UI: Sidebar lists visible workspaces only; a collapsible Hidden Workspaces section shows snoozed rows (non-draggable, click-to-wake). Context menus and the command palette add Snooze/Wake with guards so the last visible workspace cannot be hidden. Number shortcuts, reorder/move/close batch actions, command-palette switcher, and drag-drop use visible indices, with mapping back to raw order for inserts.
Supporting fixes:
SidebarWorkspaceSelectionSyncPolicyreconciles multi-select after visibility changes; keyboard shortcut matching allows shifted ⌘ punctuation via physical key fallback; find shortcut bails out before focusing the window when there is no target.Reviewed by Cursor Bugbot for commit 255ffc4. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Adds snooze/hide for workspaces and a collapsible “Hidden Workspaces (n)” section. All menus, shortcuts, history, drag/drop, and socket actions resolve against the visible list; selecting by click, ID/ref, or focus jumps reveals a hidden workspace and keeps selection stable.
New Features
hide-workspace/show-workspace,list-workspaces --include-hidden; v2workspace.hide/workspace.show; emitworkspace.visibility_changedandworkspace.reordered; hide/show skip pre-dispatch window focus.isHidden; localized Snooze/Wake and Hidden section titles; tests cover visibility filtering, session restore, visible-index resolution, hidden ID/ref selection, reorder clamping, and group/anchor behavior.ghosttysubmodule to main SHA for latest editor link, tmux bridge, and surface registry fixes.Bug Fixes
workspace.actionmove_up/move_down and close_* now operate over the visible list to avoid acting on hidden workspaces.Written for commit 315c6c3. Summary will update on new commits.
Summary by CodeRabbit