Add sidebar sections with persistence and auto-apply#2646
Add sidebar sections with persistence and auto-apply#2646rodchristiansen wants to merge 14 commits intomanaflow-ai:mainfrom
Conversation
Users can now organize workspace tabs into named, collapsible sections in the sidebar. Sections are opt-in — the sidebar behaves identically when no sections exist. - New SidebarSection model with name, collapse state, and ordered workspace membership - Section CRUD in TabManager (create, rename, delete, reorder) - Context menu: "Move to Section" submenu on workspace tabs with "New Section..." option - SidebarSectionHeaderView with disclosure chevron, inline rename, context menu, accessibility labels, and drop target - Drag workspaces onto section headers to assign them - Section state persists across sessions via SessionPersistence - Pinned workspaces always render at top regardless of section - Backward-compatible: old session snapshots restore cleanly
The new file needs to be registered in the .pbxproj so Xcode can find the SidebarSection and SidebarLayout types.
- Forward section objectWillChange to TabManager via sectionRevision counter so sidebar re-renders immediately on any section mutation - Use @focusstate on rename TextField with delayed auto-focus so cursor lands in the text field instead of the terminal - Guard onTapGesture during rename editing to prevent conflicts - Add toggleCollapsed()/setCollapsed() helpers that bump revision - Pipe Combine observers from each section to TabManager on didSet
Update bonsplit submodule: the animated split entry path hardcoded 0.5 instead of using the configured divider ratio. Include sidebar sections (id, name, collapsed state, workspace membership) in the autosave fingerprint so that section changes trigger session persistence and survive app restart.
Preserve workspace UUIDs in session snapshots so section membership survives app restart. Include sections in the autosave fingerprint so create/rename/reorder/collapse changes trigger persistence. Auto-enter rename mode when a new section is created so the user can immediately type the name.
- Add "target": "current" to cmux.json workspace commands so layouts apply to the selected workspace in-place instead of spawning a new one - Fix section rename auto-focus race: delay pendingRenameSectionId to next run loop so SwiftUI mounts the header view before the publisher emits
- Decode 'target' field in CmuxWorkspaceDefinition custom decoder so target:current actually takes effect from cmux.json - Fix within-section drag reorder: reorder section.workspaceIds instead of the flat tabs array so visual order updates correctly - Fix section rename auto-focus timing race (from prior commit)
|
@rodchristiansen 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 (6)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughIntroduces sidebar sections for organizing workspaces, persists/restores section state and workspace IDs across sessions, adds UI for section management and drag/drop semantics, and implements cmux “autoApply” behavior that can update the current workspace in-place on tab switches. Changes
Sequence Diagram(s)mermaid 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 |
|
This review could not be run because your cubic account has exceeded the monthly review limit. If you need help restoring access, please contact contact@cubic.dev. |
There was a problem hiding this comment.
Pull request overview
This PR adds persistent, user-defined sidebar sections for organizing workspaces, extends session persistence to restore section/workspace identities across restarts, and introduces autoApply / target: "current" behavior for cmux workspace commands (including auto-application on workspace switch).
Changes:
- Add
SidebarSectionmodel + sidebar layout computation and render pinned/ungrouped/sectioned workspaces in the sidebar. - Persist/restore workspace IDs and sidebar sections via session snapshots (and support restoring workspace IDs).
- Add cmux
autoApplyandworkspace.target(including applying a layout onto the currently selected workspace).
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Sources/Workspace.swift | Include workspace id in snapshots; allow restoring a workspace with a prior UUID. |
| Sources/TabManager.swift | Add section state, sidebar layout computation, autosave fingerprint updates, and restore logic for sections/workspace IDs. |
| Sources/SidebarSection.swift | New observable model for sidebar sections + a SidebarLayout helper struct. |
| Sources/SessionPersistence.swift | Extend session snapshot schema structs to include workspace IDs and section snapshots. |
| Sources/ContentView.swift | Render sidebar sections + section headers, add “Move to Section” menu, and adjust drag/drop behavior. |
| Sources/CmuxConfigExecutor.swift | Implement target: "current" workspace application behavior. |
| Sources/CmuxConfig.swift | Add autoApply, CmuxWorkspaceTarget, and per-session auto-apply tracking on tab switches. |
| GhosttyTabs.xcodeproj/project.pbxproj | Add SidebarSection.swift to the Xcode project build sources. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Sources/ContentView.swift
Outdated
| let workspaceNumberShortcut = self.workspaceNumberShortcut | ||
| let tabItemSettings = tabItemSettingsStore.snapshot | ||
| let tabIndexById = Dictionary(uniqueKeysWithValues: tabs.enumerated().map { | ||
| let tabIndexById = Dictionary(uniqueKeysWithValues: allOrdered.enumerated().map { |
There was a problem hiding this comment.
tabIndexById is being computed from layout.allWorkspacesInOrder rather than tabManager.tabs. The index passed into TabItemView is used for numbered shortcut badges (via WorkspaceShortcutMapper) and for actions like Move Up/Down; those currently operate on tabManager.tabs order (e.g., keyboard selection by number selects tabs[targetIndex]). With sections, sidebar display order can diverge from tabs order, so this will show incorrect shortcut digits and can move/reorder the wrong workspace. Consider keeping index based on tabs ordering, or updating all index-based behaviors (shortcut mapping + move/reorder actions) to consistently use the sidebar layout order instead of tabs.
| let tabIndexById = Dictionary(uniqueKeysWithValues: allOrdered.enumerated().map { | |
| let tabIndexById = Dictionary(uniqueKeysWithValues: tabs.enumerated().map { |
Sources/ContentView.swift
Outdated
| if !tabManager.sections.isEmpty || true { | ||
| let sections = tabManager.sections | ||
| let currentSection = tabManager.sectionForWorkspace(tab.id) | ||
| Menu(String(localized: "contextMenu.moveToSection", defaultValue: "Move to Section")) { | ||
| Button(String(localized: "contextMenu.noSection", defaultValue: "No Section")) { | ||
| for id in targetIds { | ||
| tabManager.removeWorkspaceFromSection(tabId: id) | ||
| } | ||
| } | ||
| .disabled(currentSection == nil) | ||
|
|
||
| if !sections.isEmpty { | ||
| Divider() | ||
| } | ||
|
|
||
| ForEach(sections, id: \.id) { section in | ||
| Button(section.name) { | ||
| for id in targetIds { | ||
| tabManager.moveWorkspaceToSection(tabId: id, sectionId: section.id) | ||
| } | ||
| } | ||
| .disabled(currentSection?.id == section.id) | ||
| } | ||
|
|
||
| Divider() | ||
|
|
||
| Button(String(localized: "contextMenu.newSection", defaultValue: "New Section…")) { | ||
| let section = tabManager.createSection( | ||
| name: String(localized: "sidebar.newSectionDefaultName", defaultValue: "New Section") | ||
| ) | ||
| for id in targetIds { | ||
| tabManager.moveWorkspaceToSection(tabId: id, sectionId: section.id) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
The condition if !tabManager.sections.isEmpty || true is always true, which makes this branch unconditional and likely leftover debug scaffolding. Remove the || true (or the whole if) so intent is clear and behavior matches the intended UX when there are no sections.
| if !tabManager.sections.isEmpty || true { | |
| let sections = tabManager.sections | |
| let currentSection = tabManager.sectionForWorkspace(tab.id) | |
| Menu(String(localized: "contextMenu.moveToSection", defaultValue: "Move to Section")) { | |
| Button(String(localized: "contextMenu.noSection", defaultValue: "No Section")) { | |
| for id in targetIds { | |
| tabManager.removeWorkspaceFromSection(tabId: id) | |
| } | |
| } | |
| .disabled(currentSection == nil) | |
| if !sections.isEmpty { | |
| Divider() | |
| } | |
| ForEach(sections, id: \.id) { section in | |
| Button(section.name) { | |
| for id in targetIds { | |
| tabManager.moveWorkspaceToSection(tabId: id, sectionId: section.id) | |
| } | |
| } | |
| .disabled(currentSection?.id == section.id) | |
| } | |
| Divider() | |
| Button(String(localized: "contextMenu.newSection", defaultValue: "New Section…")) { | |
| let section = tabManager.createSection( | |
| name: String(localized: "sidebar.newSectionDefaultName", defaultValue: "New Section") | |
| ) | |
| for id in targetIds { | |
| tabManager.moveWorkspaceToSection(tabId: id, sectionId: section.id) | |
| } | |
| } | |
| } | |
| let sections = tabManager.sections | |
| let currentSection = tabManager.sectionForWorkspace(tab.id) | |
| Menu(String(localized: "contextMenu.moveToSection", defaultValue: "Move to Section")) { | |
| Button(String(localized: "contextMenu.noSection", defaultValue: "No Section")) { | |
| for id in targetIds { | |
| tabManager.removeWorkspaceFromSection(tabId: id) | |
| } | |
| } | |
| .disabled(currentSection == nil) | |
| if !sections.isEmpty { | |
| Divider() | |
| } | |
| ForEach(sections, id: \.id) { section in | |
| Button(section.name) { | |
| for id in targetIds { | |
| tabManager.moveWorkspaceToSection(tabId: id, sectionId: section.id) | |
| } | |
| } | |
| .disabled(currentSection?.id == section.id) | |
| } | |
| Divider() | |
| Button(String(localized: "contextMenu.newSection", defaultValue: "New Section…")) { | |
| let section = tabManager.createSection( | |
| name: String(localized: "sidebar.newSectionDefaultName", defaultValue: "New Section") | |
| ) | |
| for id in targetIds { | |
| tabManager.moveWorkspaceToSection(tabId: id, sectionId: section.id) | |
| } | |
| } |
| // Close all panels except the focused one so applyCustomLayout | ||
| // starts from a single pane and doesn't stack on existing splits. | ||
| let keep = current.focusedPanelId | ||
| for panelId in current.panels.keys where panelId != keep { | ||
| current.closePanel(panelId, force: true) |
There was a problem hiding this comment.
This loop iterates current.panels.keys while calling current.closePanel(...), which can mutate current.panels (directly or via bonsplit delegate callbacks). Mutating a dictionary while iterating its keys can trap at runtime. Snapshot the panel IDs first (e.g., let panelIds = Array(current.panels.keys)) before closing, and also handle the case where focusedPanelId is nil (pick a stable panel to keep or skip the pruning) so you don’t accidentally close every panel before applying the layout.
| // Close all panels except the focused one so applyCustomLayout | |
| // starts from a single pane and doesn't stack on existing splits. | |
| let keep = current.focusedPanelId | |
| for panelId in current.panels.keys where panelId != keep { | |
| current.closePanel(panelId, force: true) | |
| // Close all panels except one preserved panel so applyCustomLayout | |
| // starts from a single pane and doesn't stack on existing splits. | |
| let panelIds = Array(current.panels.keys) | |
| let keep = current.focusedPanelId ?? panelIds.first | |
| if let keep { | |
| for panelId in panelIds where panelId != keep { | |
| current.closePanel(panelId, force: true) | |
| } |
Sources/ContentView.swift
Outdated
| .onTapGesture { | ||
| guard !isEditing else { return } | ||
| section.toggleCollapsed() | ||
| tabManager.objectWillChange.send() |
There was a problem hiding this comment.
SidebarSectionHeaderView manually calls tabManager.objectWillChange.send() after mutating section. Since section is an ObservableObject and TabManager already bumps sectionRevision by subscribing to each section’s objectWillChange, this manual objectWillChange is redundant and can cause extra invalidations. Prefer relying on the existing sectionRevision/observer pipeline (and remove the manual send) to keep update propagation consistent.
| tabManager.objectWillChange.send() |
Sources/SidebarSection.swift
Outdated
| @@ -0,0 +1,81 @@ | |||
| import Combine | |||
| import SwiftUI | |||
There was a problem hiding this comment.
SidebarSection.swift imports SwiftUI, but this file only uses Combine/Swift concurrency types and does not reference any SwiftUI symbols. Consider removing the unused import to reduce compile work and keep dependencies minimal.
| import SwiftUI |
| ) { | ||
| section.toggleCollapsed() | ||
| tabManager.objectWillChange.send() | ||
| } |
There was a problem hiding this comment.
In this context menu action, section.toggleCollapsed() should already propagate via the section observers/sectionRevision, so tabManager.objectWillChange.send() is likely unnecessary and adds extra invalidations.
Greptile SummaryThis PR adds persistent sidebar sections (collapse, rename, reorder, drag-to-assign) and an
Confidence Score: 4/5Not safe to merge as-is: two P1 defects need resolution before the new sidebar section feature works correctly Score of 4 reflects two concrete P1 bugs in the new code paths: the always-true Sources/ContentView.swift — || true condition at line 13505 and intra-section drag off-by-one at lines 15074-15077 Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Tab switch: selectedTabId changes] --> B[Observer 1: updateLocalConfigPath]
A --> C[Observer 2: 150 ms asyncAfter]
B --> D[loadLocalConfig / finishReload]
D --> E[checkAutoApply]
C --> E
E --> F{localConfigPath set?}
F -- No --> G[Return early\nsilently skips global-config commands]
F -- Yes --> H{workspace.id in\nautoAppliedIds?}
H -- Yes --> I[Return — already applied\nthis session]
H -- No --> J{loadedCommands has\nautoApply=true AND\ntarget=current?}
J -- No --> K[Return]
J -- Yes --> L[Insert workspace.id\ninto autoAppliedIds]
L --> M[CmuxConfigExecutor.execute\napplies layout to current workspace]
Reviews (1): Last reviewed commit: "AutoApply: track per-session, fire once ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Sources/ContentView.swift
Outdated
| } | ||
| .disabled(targetIds.isEmpty) | ||
|
|
||
| if !tabManager.sections.isEmpty || true { |
There was a problem hiding this comment.
|| true condition — debug code left in
The || true makes the guard always true, so the "Move to Section" submenu renders unconditionally regardless of whether any sections exist. Remove || true so the condition reflects the intended logic. If the intent is to always show the menu (to allow section creation from scratch), the if wrapper should be removed entirely. Was the intent to always show the "Move to Section" submenu (including when no sections exist) so that "New Section…" is always accessible? If so, the entire if guard should be dropped rather than keeping || true.
Sources/ContentView.swift
Outdated
| let insertAfter = dropIndicator?.edge == .bottom | ||
| if let targetIdx = section.workspaceIds.firstIndex(of: targetTabId) { | ||
| let insertIdx = insertAfter ? targetIdx + 1 : targetIdx | ||
| section.addWorkspace(draggedTabId, at: insertIdx) |
There was a problem hiding this comment.
insertIdx is computed from the pre-removal array, but addWorkspace removes the dragged item before inserting at that index. When the dragged item sits at a lower index than the target, removal shifts all subsequent indices left by 1, so the item lands one slot too far.
Example: [A, B, C, D], drag A after C (insertAfter=true): targetIdx=2, insertIdx=3. After removeAll(A) → [B, C, D], insert at 3 → [B, C, D, A]. Expected [B, C, A, D].
| let insertAfter = dropIndicator?.edge == .bottom | |
| if let targetIdx = section.workspaceIds.firstIndex(of: targetTabId) { | |
| let insertIdx = insertAfter ? targetIdx + 1 : targetIdx | |
| section.addWorkspace(draggedTabId, at: insertIdx) | |
| let insertAfter = dropIndicator?.edge == .bottom | |
| if let targetIdx = section.workspaceIds.firstIndex(of: targetTabId), | |
| let draggedIdx = section.workspaceIds.firstIndex(of: draggedTabId) { | |
| let rawInsert = insertAfter ? targetIdx + 1 : targetIdx | |
| // After removeAll(draggedTabId), every index > draggedIdx shifts -1 | |
| let adjustedInsert = rawInsert - (draggedIdx < rawInsert ? 1 : 0) | |
| section.addWorkspace(draggedTabId, at: adjustedInsert) | |
| } |
Sources/CmuxConfig.swift
Outdated
| private func checkAutoApply() { | ||
| guard let tabManager = trackedTabManager, | ||
| let workspace = tabManager.selectedWorkspace, | ||
| !autoAppliedWorkspaceIds.contains(workspace.id), | ||
| let baseCwd = localConfigPath.map({ ($0 as NSString).deletingLastPathComponent }) | ||
| else { return } |
There was a problem hiding this comment.
autoApply silently skips global-config commands
The guard requires localConfigPath to be non-nil, so if a user defines autoApply: true with target: "current" in their global config and navigates to a directory with no local config, checkAutoApply returns early without explanation. loadedCommands already contains the global command, but baseCwd resolution blocks it. Consider falling back to the global config's parent directory when localConfigPath is nil, or documenting that autoApply only works when a local config is present.
Sources/ContentView.swift
Outdated
| // Read sectionRevision to trigger re-render when any section changes. | ||
| let _ = tabManager.sectionRevision |
There was a problem hiding this comment.
sectionRevision read
tabManager.sidebarLayout already reads sectionRevision internally (let _ = sectionRevision inside the computed property body), so SwiftUI's dependency tracking is already established by the let layout = tabManager.sidebarLayout call on the next line. The explicit read on line 9995 creates a duplicate dependency and can be removed.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (1)
Sources/SidebarSection.swift (1)
13-56: Remove the redundantrevisionproperty andbumpRevision()calls.
SidebarSection.revisionis never read anywhere in the codebase. Every mutation already changes at least one other@Publishedproperty (name,isCollapsed, orworkspaceIds), which triggersobjectWillChange. TheTabManagerlistens to that signal and bumpssectionRevisionindependently. The extra@Published revisionpath adds no functional value and creates a second publish for the same event.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/SidebarSection.swift` around lines 13 - 56, Remove the redundant `@Published` property `revision` and the helper `bumpRevision()` method and delete all calls to `bumpRevision()` inside `removeWorkspace`, `addWorkspace(at:)`, `setCollapsed(_:)`, and `toggleCollapsed()`; ensure the initializer and any comments no longer reference `revision` so the class relies solely on the existing `@Published` properties (`name`, `isCollapsed`, `workspaceIds`) for change notifications and leave `contains(_:)` untouched.
🤖 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/CmuxConfig.swift`:
- Around line 333-340: The sink currently discards the emitted selectedTabId and
re-reads selectedWorkspace later, which can apply to the wrong tab after a quick
switch; capture the emitted id in the sink closure (from
tabManager.$selectedTabId.sink { [weak self] tabId in ... }) and pass that
captured tabId into the delayed work (or call a new method like
checkAutoApply(for: tabId)); in the asyncAfter closure verify the current
selectedTabId or selectedWorkspace still corresponds to the captured tabId
before calling checkAutoApply (or have checkAutoApply(for:) only apply if ids
match) so the delayed apply is bound to the originating tab.
In `@Sources/CmuxConfigExecutor.swift`:
- Around line 94-109: The code treats wsDef.target == .current as a no-op when
tabManager.selectedWorkspace is nil and falls through to the name-based path,
risking mutating the wrong workspace; update the early branch that checks `if
wsDef.target == .current, let current = tabManager.selectedWorkspace { ... }` to
instead fail closed: if `wsDef.target == .current` and
`tabManager.selectedWorkspace` is nil, return early (do not proceed to
recreate/create by name) and log or surface an error; ensure this behavior is
applied where `wsDef.target`, `tabManager.selectedWorkspace`, and the `current`
usage (e.g., `current.setCustomTitle`, `current.setCustomColor`,
`current.applyCustomLayout`) are handled so `.current` never falls through to
the name-based path.
- Around line 99-106: The code is force-closing panels unconditionally
(current.closePanel(panelId, force: true)) before calling
current.applyCustomLayout(layout, baseCwd: resolvedCwd), which can kill
restored/running panes when this path is used by autoApply; change the logic to
avoid silent force-closes by either using non-forcing close (force: false or the
default closePanel call) or only closing panels that are safe (e.g., check panel
state via current.panels[panelId]?.isIdle or similar) and skip panels with
running processes or unsaved state, then proceed to call
applyCustomLayout(layout, baseCwd: resolvedCwd).
In `@Sources/ContentView.swift`:
- Around line 13508-13527: The "No Section" and per-section Button disabled
states currently use currentSection (reflecting only the clicked row) instead of
the whole selection; update their .disabled conditions to compute across
targetIds: disable "No Section" only if every targetId already has no section,
and disable a given section Button only if every targetId is already in that
section (use your model/tabManager lookup for each target's section id and check
allSatisfy over targetIds); keep the existing actions
(tabManager.removeWorkspaceFromSection and tabManager.moveWorkspaceToSection)
unchanged.
- Around line 15069-15087: The current drop logic only handles same-section
reorders and otherwise calls tabManager.reorderWorkspace, which fails to update
section membership and computes insert positions from the pre-removal array
(causing downward moves to overshoot); replace both branches with a single
section-aware move operation: locate source section via
tabManager.sectionForWorkspace(draggedTabId) and target section via
tabManager.sectionForWorkspace(targetTabId) (or nil for ungrouped), remove the
draggedTabId from its source section.workspaceIds first, compute the normalized
insertIdx based on the post-removal target index (accounting for bottom-edge
inserts via dropIndicator?.edge), then insert via
section.addWorkspace(draggedTabId, at: insertIdx) for the destination section
(or call the ungrouped insertion equivalent) so cross-section moves update
membership and same-section downward moves use the corrected index; keep debug
dlog calls intact.
- Around line 9956-9963: TabItemView.index must remain the flat-tab index used
by shift-selection and moveBy(_:), but the code is currently passing
sidebarLayout.allWorkspacesInOrder indices; change the call sites that pass
index (including the occurrences around TabItemView initializers and row actions
referenced) to provide two indices: keep TabItemView.index as the flat index
from tabManager.tabs and add a new sidebarDisplayIndex (or similarly named
parameter) computed from sidebarLayout.allWorkspacesInOrder for rendering only;
alternatively update row actions (shift-selection handling and moveBy(_:)) to
use the sidebar-order model, but do not repurpose TabItemView.index—use explicit
separate symbols so shift-selection and moveBy(_:) continue to operate against
tabManager.tabs while the UI renders using the sidebar order.
In `@Sources/SessionPersistence.swift`:
- Line 331: SessionWorkspaceSnapshot currently declares var id: UUID? which
allows nil and breaks workspace identity guarantees; change the model so id is a
non-optional UUID on SessionWorkspaceSnapshot, remove any default UUID
generation inside the type, and update all initializers/constructors and Codable
decoding (e.g., init(from:)) to require and validate a UUID on decode; handle
legacy/migration cases only at the persistence/migration boundary (e.g.,
SessionPersistence load/migrate routines) by supplying or deriving a UUID there
before constructing a SessionWorkspaceSnapshot, and update all call sites that
create SessionWorkspaceSnapshot to pass the live workspace id.
In `@Sources/TabManager.swift`:
- Around line 2737-2745: In moveWorkspaceToSection, validate that the
destination section exists before mutating current membership: first find let
destination = sections.first(where: { $0.id == sectionId }) and return early if
nil, then remove the tabId from other sections (call
section.removeWorkspace(tabId)) and finally call destination.addWorkspace(tabId,
at: atIndex) and notifySectionChange(); this prevents silently ungrouping a
workspace when the target section is stale or deleted.
- Around line 2758-2784: sidebarLayout now treats section.workspaceIds as the
source-of-truth for grouped order, so the flat reorder API must not mutate tabs
for sectioned workspaces; update the reorderWorkspace(tabId:toIndex:)
implementation to check sectionForWorkspace(tabId) (or equivalent) and
immediately return false (use `@discardableResult` Bool) when the workspace is in
a section, leaving grouped reorders to the group-aware APIs and preventing UI
snap-back on recompute.
---
Nitpick comments:
In `@Sources/SidebarSection.swift`:
- Around line 13-56: Remove the redundant `@Published` property `revision` and the
helper `bumpRevision()` method and delete all calls to `bumpRevision()` inside
`removeWorkspace`, `addWorkspace(at:)`, `setCollapsed(_:)`, and
`toggleCollapsed()`; ensure the initializer and any comments no longer reference
`revision` so the class relies solely on the existing `@Published` properties
(`name`, `isCollapsed`, `workspaceIds`) for change notifications and leave
`contains(_:)` 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4faae21d-7b40-4e9e-84e3-500408d5d64d
📒 Files selected for processing (8)
GhosttyTabs.xcodeproj/project.pbxprojSources/CmuxConfig.swiftSources/CmuxConfigExecutor.swiftSources/ContentView.swiftSources/SessionPersistence.swiftSources/SidebarSection.swiftSources/TabManager.swiftSources/Workspace.swift
| if let layout = wsDef.layout { | ||
| // Close all panels except the focused one so applyCustomLayout | ||
| // starts from a single pane and doesn't stack on existing splits. | ||
| let keep = current.focusedPanelId | ||
| for panelId in current.panels.keys where panelId != keep { | ||
| current.closePanel(panelId, force: true) | ||
| } | ||
| current.applyCustomLayout(layout, baseCwd: resolvedCwd) |
There was a problem hiding this comment.
Don’t silently force-close existing panels before auto-applying a layout.
This path is also used by autoApply, so force: true can tear down restored panes, kill running terminals, or discard editor/browser state on tab switch with no user intervention.
Safer direction
if let layout = wsDef.layout {
// Close all panels except the focused one so applyCustomLayout
// starts from a single pane and doesn't stack on existing splits.
let keep = current.focusedPanelId
- for panelId in current.panels.keys where panelId != keep {
- current.closePanel(panelId, force: true)
+ let panelIdsToClose = current.panels.keys.filter { $0 != keep }
+ for panelId in panelIdsToClose {
+ guard current.closePanel(panelId, force: command.autoApply != true) else { return }
}
current.applyCustomLayout(layout, baseCwd: resolvedCwd)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/CmuxConfigExecutor.swift` around lines 99 - 106, The code is
force-closing panels unconditionally (current.closePanel(panelId, force: true))
before calling current.applyCustomLayout(layout, baseCwd: resolvedCwd), which
can kill restored/running panes when this path is used by autoApply; change the
logic to avoid silent force-closes by either using non-forcing close (force:
false or the default closePanel call) or only closing panels that are safe
(e.g., check panel state via current.panels[panelId]?.isIdle or similar) and
skip panels with running processes or unsaved state, then proceed to call
applyCustomLayout(layout, baseCwd: resolvedCwd).
| Menu(String(localized: "contextMenu.moveToSection", defaultValue: "Move to Section")) { | ||
| Button(String(localized: "contextMenu.noSection", defaultValue: "No Section")) { | ||
| for id in targetIds { | ||
| tabManager.removeWorkspaceFromSection(tabId: id) | ||
| } | ||
| } | ||
| .disabled(currentSection == nil) | ||
|
|
||
| if !sections.isEmpty { | ||
| Divider() | ||
| } | ||
|
|
||
| ForEach(sections, id: \.id) { section in | ||
| Button(section.name) { | ||
| for id in targetIds { | ||
| tabManager.moveWorkspaceToSection(tabId: id, sectionId: section.id) | ||
| } | ||
| } | ||
| .disabled(currentSection?.id == section.id) | ||
| } |
There was a problem hiding this comment.
Base section-menu disablement on all selected targets.
In a mixed multi-selection, currentSection only reflects tab.id, so "No Section" or a section target can be disabled even when some of the selected workspaces still need that move. Compute the disabled state from targetIds instead of the clicked row alone.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ContentView.swift` around lines 13508 - 13527, The "No Section" and
per-section Button disabled states currently use currentSection (reflecting only
the clicked row) instead of the whole selection; update their .disabled
conditions to compute across targetIds: disable "No Section" only if every
targetId already has no section, and disable a given section Button only if
every targetId is already in that section (use your model/tabManager lookup for
each target's section id and check allSatisfy over targetIds); keep the existing
actions (tabManager.removeWorkspaceFromSection and
tabManager.moveWorkspaceToSection) unchanged.
| } | ||
|
|
||
| struct SessionWorkspaceSnapshot: Codable, Sendable { | ||
| var id: UUID? |
There was a problem hiding this comment.
Keep SessionWorkspaceSnapshot.id required to preserve workspace identity invariants.
Making id optional permits nil snapshots and weakens UUID preservation guarantees during restore. Prefer a required UUID in the model and handle legacy migration explicitly at load/migration boundaries instead of in the core snapshot type.
Suggested minimal model fix
struct SessionWorkspaceSnapshot: Codable, Sendable {
- var id: UUID?
+ var id: UUID
var processTitle: String
var customTitle: String?
var customDescription: String?Based on learnings: "Sources/SessionPersistence.swift — SessionWorkspaceSnapshot.id is required (no default UUID generation). Callers must pass the live workspace id to preserve identity across restore/migration."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/SessionPersistence.swift` at line 331, SessionWorkspaceSnapshot
currently declares var id: UUID? which allows nil and breaks workspace identity
guarantees; change the model so id is a non-optional UUID on
SessionWorkspaceSnapshot, remove any default UUID generation inside the type,
and update all initializers/constructors and Codable decoding (e.g.,
init(from:)) to require and validate a UUID on decode; handle legacy/migration
cases only at the persistence/migration boundary (e.g., SessionPersistence
load/migrate routines) by supplying or deriving a UUID there before constructing
a SessionWorkspaceSnapshot, and update all call sites that create
SessionWorkspaceSnapshot to pass the live workspace id.
| var sidebarLayout: SidebarLayout { | ||
| // Read sectionRevision to establish a SwiftUI dependency so the | ||
| // layout is recomputed whenever any section property changes. | ||
| let _ = sectionRevision | ||
| let tabById = Dictionary(uniqueKeysWithValues: tabs.map { ($0.id, $0) }) | ||
| let pinnedWorkspaces = tabs.filter { $0.isPinned } | ||
|
|
||
| // Workspace IDs that are in some section (and not pinned) | ||
| var sectionedIds = Set<UUID>() | ||
| let sectionGroups: [SidebarLayout.SectionGroup] = sections.map { section in | ||
| let workspaces = section.workspaceIds.compactMap { id -> Workspace? in | ||
| guard let ws = tabById[id], !ws.isPinned else { return nil } | ||
| return ws | ||
| } | ||
| for ws in workspaces { | ||
| sectionedIds.insert(ws.id) | ||
| } | ||
| return SidebarLayout.SectionGroup(section: section, workspaces: workspaces) | ||
| } | ||
|
|
||
| let ungroupedWorkspaces = tabs.filter { !$0.isPinned && !sectionedIds.contains($0.id) } | ||
| return SidebarLayout( | ||
| pinnedWorkspaces: pinnedWorkspaces, | ||
| ungroupedWorkspaces: ungroupedWorkspaces, | ||
| sectionGroups: sectionGroups | ||
| ) | ||
| } |
There was a problem hiding this comment.
Block flat reorders for sectioned workspaces.
sidebarLayout now treats section.workspaceIds as the source of truth for grouped workspace order. That means the flat reorderWorkspace(...) API needs to reject grouped workspaces at the boundary; otherwise any stray caller will reorder tabs only and the UI will snap back on the next recompute.
Possible guard outside this hunk
`@discardableResult`
func reorderWorkspace(tabId: UUID, toIndex targetIndex: Int) -> Bool {
guard sectionForWorkspace(tabId) == nil else { return false }
// existing flat reorder logic...
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/TabManager.swift` around lines 2758 - 2784, sidebarLayout now treats
section.workspaceIds as the source-of-truth for grouped order, so the flat
reorder API must not mutate tabs for sectioned workspaces; update the
reorderWorkspace(tabId:toIndex:) implementation to check
sectionForWorkspace(tabId) (or equivalent) and immediately return false (use
`@discardableResult` Bool) when the workspace is in a section, leaving grouped
reorders to the group-aware APIs and preventing UI snap-back on recompute.
- Remove || true debug code left in section menu guard - Fix intra-section drag off-by-one: remove before insert so indices are stable when dragging forward - Fix tabIndexById to use flat tabs order for shift-selection and shortcut badges (not section order) - Fix dictionary mutation during iteration in panel close loop - Make target:current fail closed when no workspace is selected - Only auto-apply to single-pane workspaces to avoid tearing down restored or user-customized split configurations - Validate destination section before removing workspace from all sections in moveWorkspaceToSection - Add backwards-compat comment for optional SessionWorkspaceSnapshot.id - Replace SwiftUI import with Foundation in SidebarSection (only needs Combine + Foundation) - Remove redundant revision property and bumpRevision() calls from SidebarSection (mutations already trigger objectWillChange) - Remove redundant tabManager.objectWillChange.send() in section header
- Remove redundant sectionRevision read in sidebar body (sidebarLayout already reads it internally, establishing the SwiftUI dependency) - Fix autoApply silently skipping global-config commands: fall back to global config directory when no local config is present - Fix auto-apply race condition on rapid tab switching: capture the emitted tab ID and verify it's still selected before applying, so a quick B→C switch doesn't apply B's config to C
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 2 potential issues.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 69e6439. Configure here.
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
New Section button unreachable when no sections exist
High Severity
The "Move to Section" context menu, which includes the "New Section…" button, is guarded by if !tabManager.sections.isEmpty. This prevents users from creating their first section, as createSection is only invoked from this button, effectively making the entire sidebar sections feature inaccessible.
Reviewed by Cursor Bugbot for commit 69e6439. Configure here.
| // sidebarLayout reads sectionRevision internally, establishing the | ||
| // SwiftUI dependency — no separate read needed here. | ||
| let layout = tabManager.sidebarLayout | ||
| let allOrdered = layout.allWorkspacesInOrder |
There was a problem hiding this comment.
Unused variable allOrdered computed but never referenced
Low Severity
The allOrdered variable is computed from layout.allWorkspacesInOrder but is never referenced in the body. This dead code causes unnecessary computation and memory allocation on every view update.
Reviewed by Cursor Bugbot for commit 69e6439. Configure here.
Summary
Test plan
🤖 Generated with Claude Code
Summary by cubic
Adds collapsible, user-defined sidebar sections with session persistence and predictable tab order. Also adds workspace command
autoApplyon tab switch andtarget: "current"to apply layouts to the active workspace.New Features
target: "current"applies layouts in-place (clears extra panes first). OptionalautoApply: trueruns once per workspace on switch (single‑pane only) and supports global-only configs.Bug Fixes
autoApplyavoids stale runs on rapid tab switches;target: "current"no-ops if no workspace is selected and avoids dictionary mutation when closing extra panes.bonsplitsubmodule behavior.Written for commit 69e6439. Summary will update on new commits.
Summary by CodeRabbit
Note
Medium Risk
Medium risk: introduces new persisted sidebar state (sections + workspace UUIDs) and automatic config-driven workspace mutations on tab switch, which could affect session restore and sidebar ordering/drag behavior if edge cases are missed.
Overview
Adds collapsible, user-defined sidebar sections and updates sidebar rendering to order workspaces as pinned → ungrouped → section groups, including drag/drop and context-menu actions to move workspaces between sections plus inline rename/reorder/delete controls.
Persists section state across restarts by snapshotting/restoring section IDs, names, collapse state, and workspace membership, and also persists workspace UUIDs to make section membership stable.
Extends
cmux.jsonworkspace commands withtarget: "current"(apply layout in-place to the selected workspace) andautoApply: truesupport that triggers once per workspace per session on tab switch, with guards to avoid applying to multi-pane workspaces.Reviewed by Cursor Bugbot for commit 69e6439. Bugbot is set up for automated code reviews on this repo. Configure here.