Conversation
|
@willfanguy is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
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. |
|
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 persisted tab-group snapshots and panel→group assignments to session layout; introduces a workspace-scoped auto-created group tied to Workspace customColor with sync/create/delete logic; wires group assignment into tab creation and context actions; updates vendored bonsplit, localization, and .gitignore. Changes
Sequence Diagram(s)sequenceDiagram
participant Workspace
participant SessionPersistence
participant Pane
participant Storage
participant BonsplitController
Workspace->>SessionPersistence: request sessionLayoutSnapshot()
SessionPersistence-->>Workspace: layoutSnapshot (includes groups, panelGroupAssignments)
Workspace->>Storage: persist snapshot
Storage->>Workspace: load snapshot
Workspace->>Workspace: adopt workspace group (match customColor) or setCustomColor()
Workspace->>Pane: restorePane(paneSnapshot)
Pane->>Pane: recreate groups from snapshot
Pane-->>Workspace: return newPanelId mapping
Workspace->>Pane: reassign tabs to groups using panelId mapping + panelGroupAssignments
BonsplitController->>Workspace: splitTabBar(_:didCreateTab:inPane:)
Workspace->>Pane: if workspaceGroupId exists, assign new tab to workspace group
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~30 minutes 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 |
Greptile SummaryThis PR adds Chrome-style tab groups to the horizontal tab bar: named/colored group pills, context menu integration (new group, add/remove from group, rename/recolor/collapse/delete via pill), session persistence, and workspace color sync. Two P1 issues need resolution before merge: the bonsplit submodule pointer references a commit from Confidence Score: 4/5Not safe to merge: submodule branch policy violation will break CI, and multi-pane group ID bug corrupts workspace group state in split workspaces Two P1 findings remain: the bonsplit submodule commit is on a feature branch of a personal fork (against explicit CLAUDE.md policy, breaks clones/CI), and syncWorkspaceGroup() silently fails for all panes after the first in split workspaces due to per-pane UUID divergence vendor/bonsplit (must be merged to manaflow-ai/bonsplit:main before merging this PR), Sources/Workspace.swift (syncWorkspaceGroup multi-pane UUID logic, non-localized group name) Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant Workspace
participant BonsplitController
participant SessionPersistence
User->>Workspace: Right-click tab → New Group
Workspace->>BonsplitController: createGroup(name:colorHex:inPane:)
BonsplitController-->>Workspace: groupId (UUID)
Workspace->>BonsplitController: assignTab(tab.id, toGroup: groupId)
User->>Workspace: Set sidebar color
Workspace->>Workspace: syncWorkspaceGroup()
loop For each paneId in allPaneIds
Workspace->>BonsplitController: createGroup(name:colorHex:inPane:)
BonsplitController-->>Workspace: gid (different UUID per pane!)
note over Workspace: workspaceGroupId = gid (first pane only)
Workspace->>BonsplitController: assignTab(all tabs → gid)
end
User->>Workspace: Quit
Workspace->>SessionPersistence: save(groups, panelGroupAssignments)
note over SessionPersistence: workspace group excluded from save
User->>Workspace: Relaunch
SessionPersistence->>Workspace: restorePaneLayout(snapshot)
Workspace->>BonsplitController: createGroup(id:name:colorHex:inPane:) [saved groups]
Workspace->>BonsplitController: assignTab(tabId, toGroup:) [saved assignments]
Workspace->>Workspace: syncWorkspaceGroup() [recreates workspace group from customColor]
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| workspaceGroupId = nil | ||
| } | ||
| return | ||
| } | ||
|
|
||
| let groupName = customTitle ?? title | ||
| if let gid = workspaceGroupId { | ||
| // Update existing group color and name in all panes | ||
| for paneId in bonsplitController.allPaneIds { | ||
| bonsplitController.renameGroup(gid, to: groupName, inPane: paneId) | ||
| bonsplitController.changeGroupColor(gid, to: colorHex, inPane: paneId) | ||
| } | ||
| } else { | ||
| // Create workspace group in all panes and assign all tabs | ||
| for paneId in bonsplitController.allPaneIds { | ||
| if let gid = bonsplitController.createGroup(name: groupName, colorHex: colorHex, inPane: paneId) { | ||
| if workspaceGroupId == nil { workspaceGroupId = gid } | ||
| for tab in bonsplitController.tabs(inPane: paneId) { | ||
| bonsplitController.assignTab(tab.id, toGroup: gid) | ||
| } | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
createGroup(name:colorHex:inPane:) is called once per pane and each call returns a fresh UUID. Only the first pane's UUID is stored in workspaceGroupId (guarded by if workspaceGroupId == nil). Every subsequent deleteGroup, renameGroup, and changeGroupColor call passes that first-pane UUID to all panes — for pane 2 and beyond the group is never found and operations silently no-op. The didCreateTab delegate has the same flaw: it assigns a newly created tab to the first-pane UUID even when the tab lives in a different pane.
Fix: use a per-pane map ([PaneID: UUID]) instead of a single workspaceGroupId, or pass the desired UUID into createGroup using the id-bearing overload (already present for restore) so all panes share one stable UUID.
| case .addToGroup: | ||
| // Handled via direct bonsplitController.assignTab() from the submenu |
There was a problem hiding this comment.
The default group name "Group \(existingCount + 1)" is a bare string literal shown directly as a user-visible tab group title. CLAUDE.md requires every user-facing string to use String(localized:defaultValue:) with a key in Resources/Localizable.xcstrings (English + Japanese).
| case .addToGroup: | |
| // Handled via direct bonsplitController.assignTab() from the submenu | |
| let name = String(localized: "tab.group.defaultName", defaultValue: "Group \(existingCount + 1)") |
PLAN.md
Outdated
| # Tab Group Feature Plan | ||
|
|
||
| Chrome-style tab grouping for cmux's horizontal tab bar. | ||
|
|
||
| ## Status |
There was a problem hiding this comment.
This file contains local filesystem paths (/Users/will/Repos/personal/cmux-color-tabs), personal setup notes, and phase-by-phase implementation tracking. It is a development planning artifact and should not be checked into the production repository.
| case .removeFromGroup: | ||
| bonsplitController.removeTabFromGroup(tab.id) | ||
| @unknown default: |
There was a problem hiding this comment.
.addToGroup context action is a silent no-op
The .addToGroup case breaks without calling bonsplitController.assignTab. The comment claims assignment is "handled via direct bonsplitController.assignTab() from the submenu", but no such call site appears in this diff. If the submenu sends a TabContextAction.addToGroup delegate callback, the tab assignment will be silently dropped here.
Sources/Workspace.swift
Outdated
| if !assignments.isEmpty { | ||
| panelGroupAssignments = Dictionary(uniqueKeysWithValues: assignments) |
There was a problem hiding this comment.
Dictionary(uniqueKeysWithValues:) will trap on duplicate panel IDs
If panelIdFromSurfaceId returns the same UUID for two different tabs (possible during pane reconstruction edge cases), this call raises a fatal duplicate key error. Prefer the merging initializer to handle duplicates safely:
| if !assignments.isEmpty { | |
| panelGroupAssignments = Dictionary(uniqueKeysWithValues: assignments) | |
| panelGroupAssignments = Dictionary(assignments, uniquingKeysWith: { _, last in last }) |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (1)
PLAN.md (1)
21-27: Add language identifier to fenced code block.The rendering chain diagram should specify a language (e.g.,
textorplaintext) for accessibility and linting compliance.Proposed fix
-``` +```text cmux TabManager (workspaces)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@PLAN.md` around lines 21 - 27, The fenced code block containing the rendering chain diagram (the block that begins with the lines "cmux TabManager (workspaces)" and includes "WorkspaceContentView", "BonsplitView", "PaneContainerView", "TabBarView", "TabItemView") should have a language identifier added to its opening fence; replace the opening "```" with "```text" (or "```plaintext") so the fenced code block includes a language token for linting/accessibility.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@PLAN.md`:
- Line 6: Remove the developer-specific absolute path string from the checklist
entry in PLAN.md (the line that reads “[x] Repo cloned to
`/Users/will/Repos/personal/cmux-color-tabs`”); replace it with a neutral or
templated statement such as “[x] Repo cloned locally” or “[x] Repo cloned to
<local-path>” so the document no longer contains machine-specific information.
In `@Sources/SessionPersistence.swift`:
- Around line 292-294: The properties groups and panelGroupAssignments on
SessionPaneLayoutSnapshot lack default values, forcing the synthesized
memberwise initializer to require all fields and breaking existing callers that
only pass panelIds and selectedPanelId; update the SessionPaneLayoutSnapshot
definition to give groups and panelGroupAssignments sensible defaults (e.g., nil
or empty collection as appropriate) so the synthesized initializer remains
backward-compatible with existing call sites that construct it using only
panelIds and selectedPanelId.
In `@Sources/Workspace.swift`:
- Around line 7543-7578: restoreSessionSnapshot/setCustomColor currently
reassigns every tab into the workspace group after restorePane reapplies saved
panelGroupAssignments, which clobbers user-restored named group memberships; fix
by ensuring workspace-group creation/update does not reassign tabs during
restore: either call syncWorkspaceGroup() (or a new
createOrUpdateWorkspaceGroupWithoutReassignment helper) before restorePane
applies panelGroupAssignments, or change setCustomColor/syncWorkspaceGroup to
only create or update workspaceGroupId/name/color (using
bonsplitController.createGroup/renameGroup/changeGroupColor) and skip the
assignTab loop when a restore flag is set; reference restoreSessionSnapshot,
restorePane, setCustomColor, syncWorkspaceGroup, panelGroupAssignments,
workspaceGroupId and bonsplitController.assignTab to locate the code to modify.
- Around line 12252-12259: Replace the hard-coded user-facing name "Group
\(existingCount + 1)" in the .newGroup case with a localized lookup using
String(localized: "group.defaultName", defaultValue: "Group %d") (format with
existingCount + 1 when passing into createGroup), updating the name variable
assignment used with bonsplitController.createGroup(name: ..., colorHex: ...,
inPane: pane) and preserving existing behavior; also add the "group.defaultName"
key to Resources/Localizable.xcstrings with the English default "Group %d" and
provide the Japanese translation entry in the same file.
- Around line 12196-12201: The new-pane tab gets created before the pane has the
workspace group, so auto-assign in splitTabBar(_:didCreateTab:inPane:) can fail;
call syncWorkspaceGroup() immediately after creating a pane (e.g. inside
didSplitPane) so the new pane is seeded with the workspace group before any tabs
are created, or alternately ensure the pane gets the workspace group by
proactively creating/assigning it in didSplitPane using the existing
workspaceGroupId before any tab creation; update didSplitPane in
BonsplitController to invoke syncWorkspaceGroup() (or perform the explicit group
creation) right after the pane is created so splitTabBar can safely assign the
first tab.
- Around line 7567-7576: syncWorkspaceGroup() currently creates groups per pane
via createGroup(name:colorHex:inPane:) which can produce distinct IDs so storing
only the first returned workspaceGroupId will make subsequent renameGroup(),
changeGroupColor(), and deleteGroup() calls target the wrong group in other
panes; change the logic to generate or obtain a single canonical group ID (e.g.,
create the group once without a pane-specific ID or provide an explicit id
parameter) and pass that same ID into createGroup for each pane so the group
identity is consistent across panes, and ensure didCreateTab() seeds the
workspace group in newly added panes when they appear; additionally, replace the
hard-coded user-facing string "Group \(existingCount + 1)" with a localized
variant using String(localized:..., defaultValue:...) and add the
key/translation to Resources/Localizable.xcstrings.
---
Nitpick comments:
In `@PLAN.md`:
- Around line 21-27: The fenced code block containing the rendering chain
diagram (the block that begins with the lines "cmux TabManager (workspaces)" and
includes "WorkspaceContentView", "BonsplitView", "PaneContainerView",
"TabBarView", "TabItemView") should have a language identifier added to its
opening fence; replace the opening "```" with "```text" (or "```plaintext") so
the fenced code block includes a language token for linting/accessibility.
🪄 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: 9fc79407-8313-4a7d-af33-0cefa403ebdd
📒 Files selected for processing (4)
PLAN.mdSources/SessionPersistence.swiftSources/Workspace.swiftvendor/bonsplit
| case .newGroup: | ||
| let colors = ["4F46E5", "DC2626", "059669", "D97706", "7C3AED", "DB2777"] | ||
| let existingCount = bonsplitController.groups(inPane: pane).count | ||
| let color = colors[existingCount % colors.count] | ||
| let name = "Group \(existingCount + 1)" | ||
| if let groupId = bonsplitController.createGroup(name: name, colorHex: color, inPane: pane) { | ||
| bonsplitController.assignTab(tab.id, toGroup: groupId) | ||
| } |
There was a problem hiding this comment.
Localize the default group name.
"Group \(existingCount + 1)" is user-facing and bypasses the new Japanese localization work. Please switch it to String(localized:..., defaultValue: ...) and add the key to Resources/Localizable.xcstrings. 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."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Workspace.swift` around lines 12252 - 12259, Replace the hard-coded
user-facing name "Group \(existingCount + 1)" in the .newGroup case with a
localized lookup using String(localized: "group.defaultName", defaultValue:
"Group %d") (format with existingCount + 1 when passing into createGroup),
updating the name variable assignment used with
bonsplitController.createGroup(name: ..., colorHex: ..., inPane: pane) and
preserving existing behavior; also add the "group.defaultName" key to
Resources/Localizable.xcstrings with the English default "Group %d" and provide
the Japanese translation entry in the same file.
- syncWorkspaceGroup() now generates a single stable UUID and passes it via createGroup(id:) to all panes, ensuring consistent identity - New panes from splits get seeded with the workspace group on next sync - Dictionary(uniqueKeysWithValues:) replaced with uniquingKeysWith to avoid fatal trap on duplicate panel IDs during snapshot capture Addresses review feedback on manaflow-ai#2641. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (3)
Sources/Workspace.swift (3)
12260-12265:⚠️ Potential issue | 🟡 MinorLocalize the default group name.
"Group \(existingCount + 1)"is user-facing text, so it should go throughString(localized:..., defaultValue:...)and get a key inResources/Localizable.xcstrings.Suggested fix
- let name = "Group \(existingCount + 1)" + let name = String( + localized: "group.defaultName", + defaultValue: "Group \(existingCount + 1)" + )Also add
group.defaultNametoResources/Localizable.xcstringswith English and Japanese entries. As per coding guidelines, "All user-facing strings must be localized usingString(localized: "key.name", defaultValue: "English text")for every string shown in the UI."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 12260 - 12265, The default group name "Group \(existingCount + 1)" is not localized; replace its construction in the Workspace.swift case .newGroup branch with a localized lookup (use String(localized: "group.defaultName", defaultValue: "Group %d") or similar) and format it with existingCount + 1 before calling bonsplitController.createGroup(name:..., colorHex:..., inPane: pane); add a "group.defaultName" entry to Resources/Localizable.xcstrings with English and Japanese translations per guidelines.
673-678:⚠️ Potential issue | 🟠 MajorDon't clobber restored named-group memberships.
Workspace.restorePane(...)replayspanelGroupAssignmentshere, butWorkspace.restoreSessionSnapshot(...)later callssetCustomColor(...). WhenworkspaceGroupIdis still nil,syncWorkspaceGroup()recreates the workspace group and bulk-reassigns every tab into it, so colored workspaces lose any saved named-group memberships on restore. Seed/update the workspace group without the initial assign-all pass during restore, or move the workspace-group sync earlier.Also applies to: 7537-7544, 7576-7583
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 673 - 678, The restore path replays panelGroupAssignments in Workspace.restorePane but later Workspace.restoreSessionSnapshot calls setCustomColor which triggers syncWorkspaceGroup to (re)create a workspace group and bulk-reassign all tabs, clobbering named-group memberships; fix by ensuring the workspace group is seeded before or without the assign-all pass during restore: either move the syncWorkspaceGroup call earlier (before replaying panelGroupAssignments) or add a "isRestoring" flag/parameter to syncWorkspaceGroup or setCustomColor so that when workspaceGroupId is nil during restore it creates/sets workspaceGroupId without performing the bulk bonsplitController.assignTab(...) reassignment, preserving the per-tab assignments from panelGroupAssignments.
7563-7569:⚠️ Potential issue | 🟠 MajorSeed fresh panes before
didCreateTabassigns into the workspace group.This
assignTabcall assumes the destination pane already containsworkspaceGroupId, but the missing-pane create path insyncWorkspaceGroup()is never run when a split creates a new pane.splitPane(...withTab:)also bypassesdidCreateTab, so the first split tab can miss workspace-group membership entirely. CallsyncWorkspaceGroup()fromWorkspace.splitTabBar(_:didSplitPane:newPane:orientation:), or explicitly create the group in the new pane before adding/assigning tabs.Also applies to: 12204-12209
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 7563 - 7569, The code assigns tabs into a workspace group in a pane without guaranteeing the group exists in newly-split panes; ensure the group is created before any assignTab calls by either invoking syncWorkspaceGroup() from Workspace.splitTabBar(_:didSplitPane:newPane:orientation:) so the new pane gets seeded, or explicitly call bonsplitController.createGroup(id: gid, name: groupName, colorHex: colorHex, inPane: newPaneId) in the split handler before looping over tabs and calling bonsplitController.assignTab(tab.id, toGroup: gid); also ensure the same fix is applied to the other occurrence (lines 12204-12209) and that didCreateTab no longer needs to be relied on for split-created panes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/Workspace.swift`:
- Around line 12260-12265: The default group name "Group \(existingCount + 1)"
is not localized; replace its construction in the Workspace.swift case .newGroup
branch with a localized lookup (use String(localized: "group.defaultName",
defaultValue: "Group %d") or similar) and format it with existingCount + 1
before calling bonsplitController.createGroup(name:..., colorHex:..., inPane:
pane); add a "group.defaultName" entry to Resources/Localizable.xcstrings with
English and Japanese translations per guidelines.
- Around line 673-678: The restore path replays panelGroupAssignments in
Workspace.restorePane but later Workspace.restoreSessionSnapshot calls
setCustomColor which triggers syncWorkspaceGroup to (re)create a workspace group
and bulk-reassign all tabs, clobbering named-group memberships; fix by ensuring
the workspace group is seeded before or without the assign-all pass during
restore: either move the syncWorkspaceGroup call earlier (before replaying
panelGroupAssignments) or add a "isRestoring" flag/parameter to
syncWorkspaceGroup or setCustomColor so that when workspaceGroupId is nil during
restore it creates/sets workspaceGroupId without performing the bulk
bonsplitController.assignTab(...) reassignment, preserving the per-tab
assignments from panelGroupAssignments.
- Around line 7563-7569: The code assigns tabs into a workspace group in a pane
without guaranteeing the group exists in newly-split panes; ensure the group is
created before any assignTab calls by either invoking syncWorkspaceGroup() from
Workspace.splitTabBar(_:didSplitPane:newPane:orientation:) so the new pane gets
seeded, or explicitly call bonsplitController.createGroup(id: gid, name:
groupName, colorHex: colorHex, inPane: newPaneId) in the split handler before
looping over tabs and calling bonsplitController.assignTab(tab.id, toGroup:
gid); also ensure the same fix is applied to the other occurrence (lines
12204-12209) and that didCreateTab no longer needs to be relied on for
split-created panes.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/Workspace.swift (1)
12212-12216:⚠️ Potential issue | 🟠 MajorSeed the workspace group in a new pane before assigning the first tab.
This assumes
gidalready exists inpane, but freshly split panes are not seeded here. The first tab created in a colored workspace’s new pane can therefore stay ungrouped until some latersyncWorkspaceGroup()call.Possible fix
func splitTabBar(_ controller: BonsplitController, didCreateTab tab: Bonsplit.Tab, inPane pane: PaneID) { // Auto-assign new tabs to the workspace group when the workspace has a color - if let gid = workspaceGroupId, tab.groupId == nil { + if let gid = workspaceGroupId, + tab.groupId == nil, + let colorHex = customColor?.replacingOccurrences(of: "#", with: "") { + if !bonsplitController.groups(inPane: pane).contains(where: { $0.id == gid }) { + bonsplitController.createGroup( + id: gid, + name: customTitle ?? title, + colorHex: colorHex, + inPane: pane + ) + } bonsplitController.assignTab(tab.id, toGroup: gid) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 12212 - 12216, In splitTabBar(_:didCreateTab:inPane:) the code assumes workspaceGroupId is already present in the newly split pane before calling bonsplitController.assignTab(...), so the first tab can remain ungrouped; fix this by seeding/ensuring the workspace group exists in that pane before assigning the tab (e.g., invoke the controller API that creates/attaches a group to a pane — call it before bonsplitController.assignTab(tab.id, toGroup: gid)); update splitTabBar(_:didCreateTab:inPane:) to first ensure the group (using the BonsplitController method that seeds or attaches a group to a PaneID) and then call assignTab, referencing workspaceGroupId, bonsplitController.assignTab, and the pane parameter.
🤖 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/Workspace.swift`:
- Around line 7565-7578: The workspace rename path isn't updating managed group
headers because the code that syncs groupName (derived from customTitle ??
title) only runs in setCustomColor(_:) and session restore; to fix, invoke the
same group sync/update logic whenever the workspace title or customTitle is
changed (e.g., in the workspace rename setter or setCustomTitle method): if
workspaceGroupId is non-nil compute let groupName = customTitle ?? title and
iterate bonsplitController.allPaneIds to create missing groups
(bonsplitController.createGroup), rename existing ones
(bonsplitController.renameGroup), change colors
(bonsplitController.changeGroupColor), and reassign ungrouped tabs
(bonsplitController.assignTab) just like the existing block so group headers
reflect the new workspace name immediately.
- Around line 380-398: The code currently filters out workspaceGroupId when
building panelGroupAssignments which loses the distinction between intentionally
ungrouped tabs and tabs in the auto-managed workspace group; update the logic in
the block that constructs panelGroupAssignments (the use of
bonsplitController.tabs(inPane:), the compactMap that references tab.groupId and
panelIdFromSurfaceId) to preserve workspaceGroupId entries instead of dropping
them, or alternatively record a separate explicit workspace-group mapping (e.g.,
add a workspaceGroupId field to SessionWorkspaceSnapshot and store assignments
including workspaceGroupId) so restore can accurately reconstruct group
membership.
---
Duplicate comments:
In `@Sources/Workspace.swift`:
- Around line 12212-12216: In splitTabBar(_:didCreateTab:inPane:) the code
assumes workspaceGroupId is already present in the newly split pane before
calling bonsplitController.assignTab(...), so the first tab can remain
ungrouped; fix this by seeding/ensuring the workspace group exists in that pane
before assigning the tab (e.g., invoke the controller API that creates/attaches
a group to a pane — call it before bonsplitController.assignTab(tab.id, toGroup:
gid)); update splitTabBar(_:didCreateTab:inPane:) to first ensure the group
(using the BonsplitController method that seeds or attaches a group to a PaneID)
and then call assignTab, referencing workspaceGroupId,
bonsplitController.assignTab, and the pane parameter.
🪄 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: fd5d24bd-1587-46b6-8284-c634365d7172
📒 Files selected for processing (4)
.gitignoreResources/Localizable.xcstringsSources/SessionPersistence.swiftSources/Workspace.swift
✅ Files skipped from review due to trivial changes (2)
- .gitignore
- Resources/Localizable.xcstrings
There was a problem hiding this comment.
♻️ Duplicate comments (3)
Sources/Workspace.swift (3)
12214-12219:⚠️ Potential issue | 🟠 MajorAuto-assign can fail in fresh panes that don't yet contain
workspaceGroupId.
didCreateTabassigns immediately, but it does not ensure the target pane is seeded with the workspace group first. In split/new-pane flows, this can leave the first tab ungrouped.🔧 Suggested guard before assign
func splitTabBar(_ controller: BonsplitController, didCreateTab tab: Bonsplit.Tab, inPane pane: PaneID) { // Auto-assign new tabs to the workspace group when the workspace has a color if let gid = workspaceGroupId, tab.groupId == nil { + if bonsplitController.groups(inPane: pane).first(where: { $0.id == gid }) == nil, + let colorHex = customColor?.replacingOccurrences(of: "#", with: "") { + bonsplitController.createGroup( + id: gid, + name: customTitle ?? title, + colorHex: colorHex, + inPane: pane + ) + } bonsplitController.assignTab(tab.id, toGroup: gid) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 12214 - 12219, splitTabBar(_:didCreateTab:inPane:) may assign the workspace group before the new pane has been seeded, leaving the first tab ungrouped; change the block that checks workspaceGroupId and tab.groupId to first ensure the target pane contains/has been seeded with workspaceGroupId (e.g. add or call a helper like ensurePaneHasGroup(_ gid: , inPane: ) which creates or adds the workspace group to the pane if missing) and only then call bonsplitController.assignTab(tab.id, toGroup: gid); update or add the helper to use existing bonsplitController APIs to add the group to the pane if necessary.
7556-7581:⚠️ Potential issue | 🟡 MinorManaged workspace group names can still drift after workspace rename.
The sync path computes
groupName, but it only runs from color-sync/restore flows. Renamingtitle/customTitlelater won’t refresh existing group headers until another color-triggered sync occurs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 7556 - 7581, syncWorkspaceGroup currently only runs name updates as part of the color-sync path, so renaming title/customTitle later doesn’t propagate; update syncWorkspaceGroup (and call sites) so it always reconciles group names across panes: when workspaceGroupId exists iterate bonsplitController.allPaneIds and call bonsplitController.renameGroup(gid, to: groupName, inPane:) for each pane even if colorHex didn't change (and keep the existing createGroup/assignTab and changeGroupColor logic when needed), and ensure any setters or rename actions for title/customTitle invoke syncWorkspaceGroup() (or a new force-sync param) so name changes immediately refresh group headers.
342-347:⚠️ Potential issue | 🟠 MajorColor-only workspace-group adoption can bind to the wrong restored group.
This restoration logic picks the first group with matching
colorHex. If the user has multiple same-color groups,workspaceGroupIdcan point to a non-workspace group and subsequent sync/assign operations target the wrong group.💡 Safer direction
- if let colorHex = snapshot.customColor?.replacingOccurrences(of: "#", with: ""), - let firstPane = bonsplitController.allPaneIds.first { - let restored = bonsplitController.groups(inPane: firstPane) - if let match = restored.first(where: { $0.colorHex == colorHex }) { - workspaceGroupId = match.id - } - } + // Prefer an explicitly persisted workspaceGroupId in SessionWorkspaceSnapshot. + // Keep color/name inference only as backward-compatible fallback.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 342 - 347, The code currently assigns workspaceGroupId by taking the first restored group matching snapshot.customColor (via restored.first(where: { $0.colorHex == colorHex })), which can bind to the wrong group when multiple groups share the same color; change the logic in the Workspace restoration path to collect all matches from bonsplitController.groups(inPane: firstPane) that match colorHex and only set workspaceGroupId if there is exactly one unique match (e.g., let matches = restored.filter { $0.colorHex == colorHex }; if matches.count == 1 { workspaceGroupId = matches[0].id }) — otherwise do not adopt by color-only (or fall back to a safer disambiguator such as a stored group id/name/metadata if available).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/Workspace.swift`:
- Around line 12214-12219: splitTabBar(_:didCreateTab:inPane:) may assign the
workspace group before the new pane has been seeded, leaving the first tab
ungrouped; change the block that checks workspaceGroupId and tab.groupId to
first ensure the target pane contains/has been seeded with workspaceGroupId
(e.g. add or call a helper like ensurePaneHasGroup(_ gid: , inPane: ) which
creates or adds the workspace group to the pane if missing) and only then call
bonsplitController.assignTab(tab.id, toGroup: gid); update or add the helper to
use existing bonsplitController APIs to add the group to the pane if necessary.
- Around line 7556-7581: syncWorkspaceGroup currently only runs name updates as
part of the color-sync path, so renaming title/customTitle later doesn’t
propagate; update syncWorkspaceGroup (and call sites) so it always reconciles
group names across panes: when workspaceGroupId exists iterate
bonsplitController.allPaneIds and call bonsplitController.renameGroup(gid, to:
groupName, inPane:) for each pane even if colorHex didn't change (and keep the
existing createGroup/assignTab and changeGroupColor logic when needed), and
ensure any setters or rename actions for title/customTitle invoke
syncWorkspaceGroup() (or a new force-sync param) so name changes immediately
refresh group headers.
- Around line 342-347: The code currently assigns workspaceGroupId by taking the
first restored group matching snapshot.customColor (via restored.first(where: {
$0.colorHex == colorHex })), which can bind to the wrong group when multiple
groups share the same color; change the logic in the Workspace restoration path
to collect all matches from bonsplitController.groups(inPane: firstPane) that
match colorHex and only set workspaceGroupId if there is exactly one unique
match (e.g., let matches = restored.filter { $0.colorHex == colorHex }; if
matches.count == 1 { workspaceGroupId = matches[0].id }) — otherwise do not
adopt by color-only (or fall back to a safer disambiguator such as a stored
group id/name/metadata if available).
Integrates bonsplit's new tab group system into cmux: - Context menu: New Group (rotating color palette), Add to Group submenu, Remove from Group — wired through BonsplitDelegate - Session persistence: groups and panel→group assignments saved/restored via SessionTabGroupSnapshot in SessionPaneLayoutSnapshot - Workspace color sync: setting a workspace sidebar color auto-creates a matching tab group and assigns all tabs; new tabs auto-join via didCreateTab delegate; workspace group excluded from session snapshots (recreated from customColor on restore) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- syncWorkspaceGroup() now generates a single stable UUID and passes it via createGroup(id:) to all panes, ensuring consistent identity - New panes from splits get seeded with the workspace group on next sync - Dictionary(uniqueKeysWithValues:) replaced with uniquingKeysWith to avoid fatal trap on duplicate panel IDs during snapshot capture Addresses review feedback on manaflow-ai#2641. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
… PLAN.md
- SessionPaneLayoutSnapshot: add default nil values for groups and
panelGroupAssignments to preserve backward-compatible memberwise init
- Localize default group name ("Group N") via String(localized:) with
key in Localizable.xcstrings (en + ja)
- Add isRestoringSession flag so syncWorkspaceGroup skips tab
auto-assignment during restore, preventing saved group assignments
from being clobbered
- Remove PLAN.md from repo (gitignored, kept locally)
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The previous approach filtered workspace groups from session snapshots and used an isRestoringSession flag to skip auto-assignment during restore. This caused workspace-grouped tabs to lose their assignment after quit/relaunch. New approach: - Persist all groups and assignments including workspace-managed ones - syncWorkspaceGroup only assigns tabs with groupId == nil, so it never clobbers existing group memberships (saved or user-created) - On restore, adopt the workspace group ID from the snapshot before calling setCustomColor so syncWorkspaceGroup updates rather than duplicates - Remove isRestoringSession flag (no longer needed) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- didCreateTab now creates the workspace group in the target pane if missing (handles splits creating new panes before syncWorkspaceGroup) - setCustomTitle syncs the workspace group name across all panes so renaming a workspace updates the group pill immediately Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
willfanguy
force-pushed
the
feat/tab-groups
branch
from
5a1ea3f to
d5c9110
Compare
1 participant
Closes #2689
Summary
Depends on bonsplit changes: https://github.com/willfanguy/bonsplit/tree/feat/tab-groups
What's included
bonsplit (submodule):
TabGroupmodel with name, colorHex, isCollapsedTabItem.groupIdfor group membershipTabBarViewgrouped rendering withTabGroupHeaderViewpillBonsplitControllerpublic API: createGroup, assignTab, removeTabFromGroup, deleteGroup, renameGroup, changeGroupColor, toggleGroupCollapsedcmux:
SessionPersistence:SessionTabGroupSnapshot, groups + panel→group assignments inSessionPaneLayoutSnapshotWorkspace.syncWorkspaceGroup(): ties sidebar color to tab groups, auto-assigns new tabsTest plan
🤖 Generated with Claude Code