Repository navigation
Fix browser tab drops into sidebar workspaces - #3430
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
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:
📝 WalkthroughWalkthroughIntroduces sidebar drag-and-drop to move Bonsplit tabs into existing or newly inserted workspaces: adds drop-planning models, per-row geometry anchors, an AppKit overlay to capture workspace drops, threads placement+insertion-index overrides through AppDelegate → TabManager (with clamping for pinned boundaries), and adds unit/UI tests and workspace portal-rendering gating. ChangesSidebar Workspace Tab Drop Feature
sequenceDiagram
actor User
participant SwiftUI as SwiftUI (ContentView)
participant AppKit as AppKit (SidebarBonsplitTabWorkspaceDropView)
participant Planner as SidebarDropPlanner
participant Delegate as AppDelegate
participant TabMgr as TabManager
User->>AppKit: Drag Bonsplit tab over sidebar row
AppKit->>Planner: workspaceAction(for: point, targets)
Planner-->>AppKit: WorkspaceDropAction (existing/new + indicator)
AppKit->>SwiftUI: update dropIndicator binding / autoscroll
User->>AppKit: Release drag
AppKit->>Delegate: call moveBonsplitTabToNewWorkspace(..., placementOverride?, insertionIndexOverride?)
Delegate->>TabMgr: addWorkspace(fromDetachedSurface:..., insertionIndexOverride: ...)
TabMgr-->>SwiftUI: workspace list updated
SwiftUI->>SwiftUI: sync selection bindings
AppKit->>SwiftUI: clear dropIndicator
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly Related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/ContentView.swift`:
- Around line 14916-14926: The code currently returns nil when beforeTarget is
nil (cursor below every WorkspaceDropTarget), preventing "drop after last
workspace"; change the insertion index calculation to treat a nil beforeTarget
as "after the last target" by using orderedTargets.count instead of 0: compute
let beforeIndex = orderedTargets.firstIndex(of: beforeTarget) ??
orderedTargets.count and pass that to legalNewWorkspaceInsertionIndex (and then
call workspaceIndicator(forInsertionIndex:orderedTargets:) with the resulting
insertionIndex) so drops below the last measured row are accepted.
- Around line 12487-12559: SidebarBonsplitTabWorkspaceDropOverlay currently
captures a live TabManager (tabManager) which violates the rule about holding
ObservableObject stores under LazyVStack/ForEach; replace that live store with
immutable value snapshots and action closures: remove the tabManager property
and instead accept a snapshot of tabs (e.g. [Tab] or a lightweight
TabsSnapshot), a snapshot selectedTabId (UUID?), and closures for move
operations and selection helpers (e.g.
moveBonsplitTab(tabId:toWorkspace:focus:focusWindow:)->Bool,
moveBonsplitTabToNewWorkspace(...)->ResultSnapshot, and a closure to compute
index or set lastSidebarSelectionIndex). Update uses inside updateNSView and
syncSidebarSelection to call those closures / read from the snapshots (replace
tabManager.selectedTabId, tabManager.tabs.firstIndex { ... }, and any move
calls) and adjust performExistingWorkspaceMove/performNewWorkspaceMove to invoke
the provided move closures and return results; ensure
selectedTabIds/lastSidebarSelectionIndex are updated via the provided selection
helper closure instead of accessing TabManager directly.
🪄 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: 6e25f054-00d9-4157-bbf6-f878cc643f3e
📒 Files selected for processing (4)
Sources/AppDelegate+MoveTabToNewWorkspace.swiftSources/ContentView.swiftSources/TabManager+DetachedWorkspace.swiftcmuxTests/SidebarOrderingTests.swift
Greptile SummaryThis PR fixes browser-tab sidebar drops by routing them through Confidence Score: 4/5Safe to merge; only P2 findings present No P0/P1 issues found. The planner logic is correct and well-tested for four of five zones. The
Important Files Changed
Sequence DiagramsequenceDiagram
participant BTab as Browser Tab Drag
participant Overlay as SidebarBonsplitTabWorkspaceDropView
participant Planner as SidebarDropPlanner
participant App as AppDelegate
participant TM as TabManager
BTab->>Overlay: draggingEntered / draggingUpdated
Overlay->>Planner: workspaceAction(for: point, targets:)
alt center zone
Planner-->>Overlay: .existingWorkspace(workspaceId)
Overlay->>App: moveBonsplitTab(tabId:toWorkspace:)
else top/bottom edge or gap
Planner-->>Overlay: .newWorkspace(insertionIndex:, indicator:)
Overlay->>App: moveBonsplitTabToNewWorkspace(tabId:insertionIndexOverride:)
App->>TM: addWorkspace(fromDetachedSurface:insertionIndexOverride:)
TM-->>TM: insert at override index (or append if out-of-bounds)
else below all rows
Planner-->>Overlay: nil → NSDragOperationNone
Note over Overlay: falls through to SidebarBonsplitTabNewWorkspaceDropOverlay
end
Overlay-->>BTab: NSDragOperation (.move or [])
Reviews (1): Last reviewed commit: "fix: use shared workspace move path for ..." | Re-trigger Greptile |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/Sidebar/SidebarDropPlanner.swift`:
- Around line 107-118: The code in workspaceAction(for:targets:) only handles
drops before a target and returns nil when the pointer is past
orderedTargets.last; change it to detect the "after-last" case and produce a
.newWorkspace insertion at the end: compute insertionIndex =
legalNewWorkspaceInsertionIndex(orderedTargets.count, orderedTargets:
orderedTargets) when the point.y is below the last target (or when the guard
fails because no beforeTarget), and return .newWorkspace with
workspaceIndicator(forInsertionIndex:insertionIndex,
orderedTargets:orderedTargets); update the existing guard/else path so
orderedTargets.count is used for end-of-list drops and prepareForDragOperation
can accept inserts at the end.
🪄 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: 6d888841-28fb-45e5-963c-c9b09ffe267e
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (5)
GhosttyTabs.xcodeproj/project.pbxprojSources/ContentView.swiftSources/Sidebar/SidebarBonsplitTabWorkspaceDropOverlay.swiftSources/Sidebar/SidebarDropPlanner.swiftcmuxTests/SidebarWorkspaceDropPlannerTests.swift
✅ Files skipped from review due to trivial changes (1)
- cmuxTests/SidebarWorkspaceDropPlannerTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/ContentView.swift
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/Sidebar/SidebarBonsplitTabWorkspaceDropOverlay.swift`:
- Around line 79-115: Add DEBUG-only logging in the overlay drag lifecycle: in
draggingEntered(_:) and draggingUpdated(_:) call
SidebarDropPlanner.workspaceAction(for: localPoint(sender), targets: targets)
(safely) and under `#if` DEBUG use dlog(...) to emit a sanitized summary of the
planner result and the computed drop indicator; in draggingExited(_:) and
concludeDragOperation(_:) call dlog(...) under `#if` DEBUG to note exit/conclude
events and that setDropIndicator(nil) was called; in prepareForDragOperation(_:)
log the boolean outcome of acceptsDrag(sender) and whether
SidebarDropPlanner.workspaceAction(...) returned non-nil; in
performDragOperation(_:) wrap debug logging in `#if` DEBUG to log the chosen
action (case .existingWorkspace(workspaceId) vs .newWorkspace(insertionIndex,
indicator)), the results of performExistingWorkspaceMove(...) or
performNewWorkspaceMove(...), and the final moved boolean before returning;
ensure all logs use the Bonsplit canonical dlog("...") and only include
sanitized IDs/indices (no sensitive data).
- Around line 21-24: The isValidTransfer closure currently always calls
AppDelegate.shared?.canMoveBonsplitTabToNewWorkspace(tabId:) which wrongly
rejects transfers planned for .existingWorkspace; change the logic to inspect
the transfer's planned action (from BonsplitTabDragPayload.currentTransfer())
and validate according to that action: if the planned action is .newWorkspace
call canMoveBonsplitTabToNewWorkspace(tabId:), if it is .existingWorkspace
validate mergeability using the same check the planner or AppDelegate provides
for merges (e.g. the canMerge/validateMerge API used elsewhere), and return
false for other/unknown planned actions; apply the same fix to the other
closures mentioned around lines 91-99 and 136-139.
In `@Sources/TabManager`+DetachedWorkspace.swift:
- Around line 55-59: The override path currently computes insertIndex via
detachedWorkspaceInsertIndex(...) using only snapshot.tabs, which can allow
inserting into a pinned segment that changed between snapshot and the live
updatedTabs; fix by re-clamping the computed insertion index against the live
pinned bounds before performing updatedTabs.insert(...): call
clampedDetachedWorkspaceInsertIndex(...) (or equivalent logic) with the live
updatedTabs (and the same insertionIndexOverride and placementOverride) to
obtain a final safe index, then use that index for updatedTabs.insert; apply the
same re-clamping change to the other override sites that mirror this logic (the
other detached-workspace insert locations referenced by the review).
🪄 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: cd3fc842-6e77-4071-b9cc-65fece8f5cf2
📒 Files selected for processing (5)
Sources/ContentView.swiftSources/Sidebar/SidebarBonsplitTabWorkspaceDropOverlay.swiftSources/TabManager+DetachedWorkspace.swiftcmuxTests/SidebarWorkspaceDropPlannerTests.swiftcmuxTests/WorkspaceUnitTests.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- cmuxTests/SidebarWorkspaceDropPlannerTests.swift
- Sources/ContentView.swift
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/TabManager+DetachedWorkspace.swift (1)
117-139: 💤 Low value
pinnedCountis computed inconsistently between the two overloads.The
tabs: [WorkspaceCreationTabSnapshot]overload (line 121) counts all pinned entries viareduce(consistent withnewTabInsertIndex), while theworkspaces: [Workspace]overload (line 133) counts only the contiguous leading pinned workspaces viaprefix { }.count. Under the pinned-first ordering invariant these produce identical results, but the asymmetry is a maintenance hazard if the invariant is ever relaxed.🔧 Suggested unification
- private static func clampedDetachedWorkspaceInsertIndex( - _ proposedInsertion: Int, - workspaces: [Workspace] - ) -> Int { - let pinnedCount = workspaces.prefix { $0.isPinned }.count - return clampedDetachedWorkspaceInsertIndex( - proposedInsertion, - totalCount: workspaces.count, - pinnedCount: pinnedCount - ) - } + private static func clampedDetachedWorkspaceInsertIndex( + _ proposedInsertion: Int, + workspaces: [Workspace] + ) -> Int { + let pinnedCount = workspaces.reduce(into: 0) { count, ws in + if ws.isPinned { count += 1 } + } + return clampedDetachedWorkspaceInsertIndex( + proposedInsertion, + totalCount: workspaces.count, + pinnedCount: pinnedCount + ) + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TabManager`+DetachedWorkspace.swift around lines 117 - 139, The two overloads clampedDetachedWorkspaceInsertIndex compute pinnedCount differently (WorkspaceCreationTabSnapshot uses reduce to count all pinned entries while Workspace uses prefix to count only leading pinned entries); make them consistent by counting all pinned workspaces in the workspaces overload (e.g., compute pinnedCount via filter or reduce over workspaces to count $0.isPinned) so both clampedDetachedWorkspaceInsertIndex overloads use the same pinned-counting logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/TabManager`+DetachedWorkspace.swift:
- Around line 117-139: The two overloads clampedDetachedWorkspaceInsertIndex
compute pinnedCount differently (WorkspaceCreationTabSnapshot uses reduce to
count all pinned entries while Workspace uses prefix to count only leading
pinned entries); make them consistent by counting all pinned workspaces in the
workspaces overload (e.g., compute pinnedCount via filter or reduce over
workspaces to count $0.isPinned) so both clampedDetachedWorkspaceInsertIndex
overloads use the same pinned-counting logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: d5424fa6-2be2-4b0d-9421-07e7851648a8
📒 Files selected for processing (3)
Sources/AppDelegate+MoveTabToNewWorkspace.swiftSources/Sidebar/SidebarBonsplitTabWorkspaceDropOverlay.swiftSources/TabManager+DetachedWorkspace.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/AppDelegate+MoveTabToNewWorkspace.swift
- Sources/Sidebar/SidebarBonsplitTabWorkspaceDropOverlay.swift
…-workspace-code-path # Conflicts: # .github/swift-file-length-budget.tsv
There was a problem hiding this comment.
🧹 Nitpick comments (3)
Sources/Workspace.swift (2)
11527-11542: ⚡ Quick winEmit a DEBUG portal-render toggle event here.
This method is now the central switch for whether a workspace may render portals at all, so it should log the transition with the workspace id, enabled flag, and reason.
🪵 Proposed logging addition
func setPortalRenderingEnabled(_ enabled: Bool, reason: String) { let changed = portalRenderingEnabled != enabled +#if DEBUG + if changed { + cmuxDebugLog( + "workspace.portalRendering workspace=\(id.uuidString.prefix(5)) " + + "enabled=\(enabled ? 1 : 0) reason=\(reason)" + ) + } +#endif portalRenderingEnabled = enabled if enabled { if changed { beginEventDrivenLayoutFollowUp(As per coding guidelines "Implement debug events in a unified log in DEBUG builds at
/tmp/cmux-debug.log... UsecmuxDebugLog("message")to log from code. All call sites must be wrapped in#if DEBUG/#endif."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 11527 - 11542, Add a DEBUG-only debug event at the top of setPortalRenderingEnabled to log the workspace id, the new enabled value, and the reason: wrap a call to cmuxDebugLog("...") in `#if` DEBUG / `#endif` and format the message to include self.id (or the workspace identifier property used in this class), the enabled Bool, and the reason string; keep the rest of the method (calls to beginEventDrivenLayoutFollowUp, clearLayoutFollowUp, hideAllTerminalPortalViews, hideAllBrowserPortalViews) unchanged.
10504-10511: ⚡ Quick winRoute teardown through
setPortalRenderingEnabled(...).
teardownAllPanels()is manually reproducing the new disable path instead of calling the helper you just introduced. That makes the teardown path easy to drift the next time portal-disable behavior changes.♻️ Proposed refactor
func teardownAllPanels() { // Hide portal-hosted content up front so a workspace being torn down // cannot keep drawing above the next selected/restored workspace while // panel close work is still unwinding. - portalRenderingEnabled = false - clearLayoutFollowUp() - hideAllTerminalPortalViews() - hideAllBrowserPortalViews() + setPortalRenderingEnabled(false, reason: "workspaceTeardown") let panelEntries = Array(panels) for (panelId, panel) in panelEntries {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Workspace.swift` around lines 10504 - 10511, teardownAllPanels() is directly mutating portalRenderingEnabled instead of using the new helper; change the first line to call setPortalRenderingEnabled(false) so teardown follows the centralized disable path you added, then keep the subsequent calls (clearLayoutFollowUp(), hideAllTerminalPortalViews(), hideAllBrowserPortalViews()) as-is to preserve behavior; ensure any parameters or side‑effects expected by setPortalRenderingEnabled(...) (e.g. completion handlers or logging) are correctly propagated.Sources/WorkspaceSurfaceConfig.swift (1)
82-94: ⚡ Quick winAdd a liveness check before the first
ghostty_surface_inherited_configcall
ghostty_surface_inherited_config(sourceSurface, context)at line 86 is called without first verifying the pointer viacmuxSurfacePointerAppearsLive, while the downstream call tocmuxCurrentSurfaceFontSizePoints(line 91) does perform that check. IfsourceSurfaceis stale at the point of the first C call, this is a crash rather than a graceful fallback.The caller in
Workspace.inheritedTerminalConfiguseswithExtendedLifetimeto keep Swift wrappers alive, which mitigates the risk in practice, but the asymmetry between the two C call sites is fragile.🛡️ Proposed defensive fix
func cmuxInheritedSurfaceConfig( sourceSurface: ghostty_surface_t, context: ghostty_surface_context_e ) -> CmuxSurfaceConfigTemplate { + guard cmuxSurfacePointerAppearsLive(sourceSurface) else { + return CmuxSurfaceConfigTemplate() + } let inherited = ghostty_surface_inherited_config(sourceSurface, context) var config = CmuxSurfaceConfigTemplate(cConfig: inherited)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/WorkspaceSurfaceConfig.swift` around lines 82 - 94, In cmuxInheritedSurfaceConfig, add a liveness check using cmuxSurfacePointerAppearsLive(sourceSurface) before calling ghostty_surface_inherited_config so you don't call into C with a stale pointer; if the pointer is not live, return a safe default CmuxSurfaceConfigTemplate (or construct one without calling ghostty_surface_inherited_config) and preserve the existing behavior of applying runtime font-size from cmuxCurrentSurfaceFontSizePoints when available. This change touches cmuxInheritedSurfaceConfig, ghostty_surface_inherited_config, cmuxSurfacePointerAppearsLive, cmuxCurrentSurfaceFontSizePoints, and the CmuxSurfaceConfigTemplate initializer—ensure the defensive early-return happens prior to the current call at the top of the function.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Workspace.swift`:
- Around line 11527-11542: Add a DEBUG-only debug event at the top of
setPortalRenderingEnabled to log the workspace id, the new enabled value, and
the reason: wrap a call to cmuxDebugLog("...") in `#if` DEBUG / `#endif` and format
the message to include self.id (or the workspace identifier property used in
this class), the enabled Bool, and the reason string; keep the rest of the
method (calls to beginEventDrivenLayoutFollowUp, clearLayoutFollowUp,
hideAllTerminalPortalViews, hideAllBrowserPortalViews) unchanged.
- Around line 10504-10511: teardownAllPanels() is directly mutating
portalRenderingEnabled instead of using the new helper; change the first line to
call setPortalRenderingEnabled(false) so teardown follows the centralized
disable path you added, then keep the subsequent calls (clearLayoutFollowUp(),
hideAllTerminalPortalViews(), hideAllBrowserPortalViews()) as-is to preserve
behavior; ensure any parameters or side‑effects expected by
setPortalRenderingEnabled(...) (e.g. completion handlers or logging) are
correctly propagated.
In `@Sources/WorkspaceSurfaceConfig.swift`:
- Around line 82-94: In cmuxInheritedSurfaceConfig, add a liveness check using
cmuxSurfacePointerAppearsLive(sourceSurface) before calling
ghostty_surface_inherited_config so you don't call into C with a stale pointer;
if the pointer is not live, return a safe default CmuxSurfaceConfigTemplate (or
construct one without calling ghostty_surface_inherited_config) and preserve the
existing behavior of applying runtime font-size from
cmuxCurrentSurfaceFontSizePoints when available. This change touches
cmuxInheritedSurfaceConfig, ghostty_surface_inherited_config,
cmuxSurfacePointerAppearsLive, cmuxCurrentSurfaceFontSizePoints, and the
CmuxSurfaceConfigTemplate initializer—ensure the defensive early-return happens
prior to the current call at the top of the function.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2d43078-5c34-417a-861c-f0731e50b8c2
📒 Files selected for processing (8)
GhosttyTabs.xcodeproj/project.pbxprojSources/ContentView.swiftSources/GhosttyTerminalView.swiftSources/Workspace.swiftSources/WorkspaceSurfaceConfig.swiftcmuxTests/GhosttyTerminalViewVisibilityPolicyTests.swiftcmuxTests/TerminalAndGhosttyTests.swiftcmuxTests/WorkspaceUnitTests.swift
💤 Files with no reviewable changes (1)
- cmuxTests/TerminalAndGhosttyTests.swift
✅ Files skipped from review due to trivial changes (1)
- cmuxTests/GhosttyTerminalViewVisibilityPolicyTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/WorkspaceUnitTests.swift
Summary
moveBonsplitTabToNewWorkspaceand the sharedmoveSurfaceToNewWorkspacepath.Issues
Verification
btabs3, user confirmed “seems good”.git diff --check origin/main..HEADxcodebuild -project GhosttyTabs.xcodeproj -scheme cmux-unit -destination 'platform=macOS' -derivedDataPath /tmp/cmux-btabpr-unit test -only-testing:cmuxTests/SidebarDropPlannerTests./scripts/reload.sh --tag btabprSummary by cubic
Fixes sidebar drag-and-drop of browser tabs so you can drop into an existing workspace or create a new one in the right spot (including below the last row), with clear before/after indicators. Also prevents stale terminal/browser portals from covering the active workspace during unmounts or handoffs.
Bug Fixes
insertionIndexOverrideclamped after pinned rows, and validate the planned drop action before accepting.GhosttyTerminalView.Refactors
SidebarDropPlannerandSidebarBonsplitTabWorkspaceDropOverlay, and move Ghostty surface-config helpers intoWorkspaceSurfaceConfig.swiftto reduceContentView/Workspacecomplexity.Written for commit 78e98dc. Summary will update on new commits.
Summary by CodeRabbit
New Features
Tests