Give every window its own independent Dock - #7144
Merged
Merged
Conversation
Replace the single app-wide Global Dock with per-window DockSplitStores: each main window lazily creates its own Dock (seeded fresh from ~/.config/cmux/dock.json, owner id == window id), renders it live in its right sidebar, and tears it down — panels, PTYs, portals — when the window unregisters. No window ever shows the "Global Dock is active in another window" placeholder; DockInactiveHostView and the renderHostId first-claim gating are removed. CLI/socket routing resolves per window: a Dock-scoped workspace_id names the owning window (results are self-describing), the legacy global-dock constant keeps routing as an alias for "the Dock of the routed window", and surface/pane containment scans every window Dock. Shortcut routing, focused-close, drag/drop moves, and browser tab commands all target the window's own store. Fixes #7142 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR replaces the shared Dock with per-window Dock storage, updates rendering and lifecycle cleanup to use window-scoped Dock instances, and rewires shortcut, control, browser, and socket routing to resolve and close the correct window Dock. Tests, docs, localization, and project membership were updated to match. ChangesPer-window Dock lifecycle and cleanup
Routing and control paths
Tests and wiring
Estimated code review effort: 5 (Critical) | ~120 minutes Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (22 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
Greptile SummaryThis PR replaces the single app-wide
Confidence Score: 5/5This PR is safe to merge. The per-window Dock lifecycle is correctly scoped to MainWindowContext, teardown is unconditional and ordered (nil before closeAllPanels), and all routing paths are fail-closed on contradictory selectors. The core lifecycle invariant — one DockSplitStore per window, created lazily, torn down on unregister — is upheld at every call site. Routing conflict detection, browser-command resolution, quit-confirmation, and Ghostty runtime-close callbacks are all updated consistently. The test seams were correctly moved to the test target per the canonical pattern. No correctness bugs were found in routing, teardown ordering, or notification cleanup. No files require special attention. DockScope.swift carries an acknowledged naming mismatch (the global case now represents a per-window store) but the comment is clear and the raw value is stable for coding compatibility. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant W as NSWindow
participant AD as AppDelegate
participant MWC as MainWindowContext
participant DSS as DockSplitStore
participant SW as Socket Worker
W->>AD: registerMainWindowContext
AD->>MWC: "create (windowDock=nil, lazy)"
W->>MWC: RightSidebar renders dock mode
MWC->>DSS: windowDockStore() creates store
Note over DSS: workspaceId == windowId
SW->>AD: socket command with routing selectors
AD->>AD: windowDockForRouting
AD->>DSS: route to owning store
DSS-->>SW: "result (workspace_id = windowId)"
W->>AD: window closes
AD->>MWC: teardownWindowDock
MWC->>DSS: "windowDock=nil then closeAllPanels"
AD->>AD: remove context from mainWindowContexts
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
participant W as NSWindow
participant AD as AppDelegate
participant MWC as MainWindowContext
participant DSS as DockSplitStore
participant SW as Socket Worker
W->>AD: registerMainWindowContext
AD->>MWC: "create (windowDock=nil, lazy)"
W->>MWC: RightSidebar renders dock mode
MWC->>DSS: windowDockStore() creates store
Note over DSS: workspaceId == windowId
SW->>AD: socket command with routing selectors
AD->>AD: windowDockForRouting
AD->>DSS: route to owning store
DSS-->>SW: "result (workspace_id = windowId)"
W->>AD: window closes
AD->>MWC: teardownWindowDock
MWC->>DSS: "windowDock=nil then closeAllPanels"
AD->>AD: remove context from mainWindowContexts
Reviews (32): Last reviewed commit: "Align Dock conflict tests with read-rout..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 26 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/Workspace+DockBrowserLookup.swift (1)
80-89: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winDo not close the legacy workspace Dock from a focused window-Dock shortcut.
If the right sidebar is in Dock mode but the per-window store has not been registered yet, this fallback can close
selectedWorkspace’s old Dock state. Use the window-owned Dock as the single target and no-op when it has no focused panel.As per path instructions, Dock/window ownership must use one authoritative routing path.
Proposed fix
- if let windowDock = existingWindowDock(forWindowId: context.windowId) { - guard let panelId = windowDock.focusedPanelId else { return true } - if windowDock.closePanel(panelId, force: false) { - notificationStore?.clearNotifications(forTabId: windowDock.workspaceId, surfaceId: panelId) - } - return true - } - guard let workspace = context.tabManager.selectedWorkspace, - let panelId = workspace.focusedDockPanelId else { return true } - _ = workspace.closeDockPanelAndClearNotifications(panelId, force: false) + let windowDock = windowDock(forWindowId: context.windowId) + guard let panelId = windowDock.focusedPanelId else { return true } + if windowDock.closePanel(panelId, force: false) { + notificationStore?.clearNotifications(forTabId: windowDock.workspaceId, surfaceId: panelId) + } return true🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/Workspace`+DockBrowserLookup.swift around lines 80 - 89, The fallback in Workspace+DockBrowserLookup’s focused-window Dock handling is incorrectly closing selectedWorkspace’s legacy Dock state when the per-window store is missing. Update the logic so the shortcut only targets the window-owned Dock via existingWindowDock(forWindowId:) and returns immediately if that Dock has no focusedPanelId; do not consult tabManager.selectedWorkspace or call closeDockPanelAndClearNotifications in this path.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/AppDelegate`+DockShortcutRouting.swift:
- Around line 24-28: Stop falling back to the legacy workspace Dock in the dock
shortcut routing path. Update the Dock resolution in
AppDelegate+DockShortcutRouting so the focused window always uses the
window-owned DockSplitStore via existingWindowDock(forWindowId:) and, if none
exists, returns nil instead of selectedWorkspace?.dockSplit; keep the change
localized to the dock lookup helper used for Dock shortcuts.
In `@Sources/AppDelegate`+WindowDock.swift:
- Around line 91-98: The dock lookup in dockReferenceTabManager(for:) currently
falls back from tabManagerFor(windowId:) to
preferredRegisteredMainWindowContext()?.tabManager when dock.scope is .global,
which can route a move into the wrong window’s Dock. Update this method to fail
closed by returning nil when the owning window context cannot be resolved, and
keep the tabManagerFor(tabId:) path unchanged for non-global docks.
In `@Sources/TerminalController.swift`:
- Around line 5276-5278: The window-Dock routing in
v2ResolveWindowDockBrowserPanelContext and v2ResolveWindowDockBrowserTabStore
currently accepts mixed selectors and then prefers dockBySurface/dockByPane over
dockByOwner, which can route commands to the wrong Dock. Add the same
resolvedDocks agreement check in both helpers so workspace_id, surface_id,
pane_id, and owner-derived resolution must all agree before treating the request
as handled. If they disagree, fail closed by returning an unhandled/error result
instead of overriding the caller’s tabManager or context.
In `@Sources/TerminalController`+ControlSurfaceContext.swift:
- Line 86: The routing helpers are allowing a Dock found by surfaceID/paneID to
be acted on even when it belongs to a different resolved window/workspace than
the one selected by resolveTabManager. Update
windowDockForRouting(_:tabManager:) and the related windowDockContainingPanel
paths to verify the Dock’s owner matches the resolved tab manager/window
identity before mutating or focusing it, using the existing
v2ResolveWindowId(tabManager:) / explicit windowID sources as the authority. If
the resolved owner does not match, skip that Dock and continue routing so
actions stay scoped to the correct window.
In `@Sources/TerminalController`+ControlSurfaceContext2.swift:
- Around line 425-447: The close flow in
TerminalController+ControlSurfaceContext2 should reject cases where the Dock
resolved from surfaceID disagrees with the explicitly resolved tab/window
context. In the windowDock close branch and the other close branch at the
referenced close helpers, add a guard that compares Dock ownership and surface
identity before calling closePanel, and return a failure if the
windowDock/tabManager sources do not match. Ensure the returned closed payload
uses a windowID derived from the same resolved window/workspace context used to
close the panel.
---
Outside diff comments:
In `@Sources/Workspace`+DockBrowserLookup.swift:
- Around line 80-89: The fallback in Workspace+DockBrowserLookup’s
focused-window Dock handling is incorrectly closing selectedWorkspace’s legacy
Dock state when the per-window store is missing. Update the logic so the
shortcut only targets the window-owned Dock via existingWindowDock(forWindowId:)
and returns immediately if that Dock has no focusedPanelId; do not consult
tabManager.selectedWorkspace or call closeDockPanelAndClearNotifications in this
path.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3aac6ffb-fe22-4c96-a39d-aed17bd1ad2c
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (25)
Resources/Localizable.xcstringsSources/AppDelegate+DockShortcutRouting.swiftSources/AppDelegate+DockSurfaceMove.swiftSources/AppDelegate+GlobalDock.swiftSources/AppDelegate+WindowDock.swiftSources/AppDelegate.swiftSources/DockInactiveHostView.swiftSources/DockPanelView.swiftSources/DockScope.swiftSources/DockSplitStore+Config.swiftSources/DockSplitStore+SurfaceTransfer.swiftSources/DockSplitStore.swiftSources/RightSidebarPanelView.swiftSources/TerminalController+ControlPaneContext.swiftSources/TerminalController+ControlPaneDock.swiftSources/TerminalController+ControlSurfaceContext.swiftSources/TerminalController+ControlSurfaceContext2.swiftSources/TerminalController+ControlSurfaceContext3.swiftSources/TerminalController+ControlSurfaceDock.swiftSources/TerminalController.swiftSources/Workspace+DockBrowserLookup.swiftcmux.xcodeproj/project.pbxprojcmuxTests/DockSocketLifecycleTests.swiftcmuxTests/WindowDockLifecycleTests.swiftdocs/dock.md
💤 Files with no reviewable changes (3)
- Sources/DockInactiveHostView.swift
- Resources/Localizable.xcstrings
- Sources/AppDelegate+GlobalDock.swift
Review follow-ups: the Dock registry is the single source of truth for which window renders a Dock surface, so Dock-scoped commands now focus, reveal, and report the dock's OWNING window (owner id == window id) instead of whatever window the caller's routed context resolved. Explicit selectors naming two different windows' Docks fail closed with invalid_params. The focused-Dock shortcut path no longer falls back to the never-rendered workspace Dock, dockReferenceTabManager fails closed instead of retargeting the active window, and per-window dock stores now live in a dedicated WindowDockRegistry so all lifecycle mutation is centralized. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/TerminalController+ControlSurfaceDock.swift (1)
100-117: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFail closed when Dock routing selectors disagree.
windowDockForRoutingreturns the owner/alias Dock before checkingsurfaceID/paneIDcontainment, so calls that don’t later guard withcontainsPanel/containsPanecan return the wrong Dock snapshot for mixed selectors. Resolve all Dock candidates first, let the legacy alias stay non-specific, and returnnilwhen explicit owner/surface/pane selectors point at different window Docks.Proposed fix
func windowDockForRouting(_ routing: ControlRoutingSelectors, tabManager: TabManager) -> DockSplitStore? { - if let workspaceID = routing.workspaceID { - if workspaceID == AppDelegate.windowDockAliasWorkspaceId { - return AppDelegate.shared?.windowDock(for: tabManager) - } - if let dock = AppDelegate.shared?.existingWindowDock(forWindowId: workspaceID) { - return dock - } - } + let dockByOwner: DockSplitStore? = routing.workspaceID.flatMap { workspaceID in + guard workspaceID != AppDelegate.windowDockAliasWorkspaceId else { return nil } + return AppDelegate.shared?.existingWindowDock(forWindowId: workspaceID) + } + let dockBySurface = routing.surfaceID.flatMap { windowDockContainingPanel($0) } + let dockByPane = routing.paneID.flatMap { windowDockContainingPane($0) } + + let resolved = [dockByOwner, dockBySurface, dockByPane].compactMap { $0 } + if let first = resolved.first, resolved.contains(where: { $0 !== first }) { + return nil + } + if let dock = resolved.first { + return dock + } + if routing.workspaceID == AppDelegate.windowDockAliasWorkspaceId { + return AppDelegate.shared?.windowDock(for: tabManager) + } - if let surfaceID = routing.surfaceID, - let dock = windowDockContainingPanel(surfaceID) { - return dock - } - if let paneID = routing.paneID, - let dock = windowDockContainingPane(paneID) { - return dock - } return nil }As per path instructions, correctness-critical routing must avoid disagreeing sources of truth and fail closed when authoritative mappings disagree.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/TerminalController`+ControlSurfaceDock.swift around lines 100 - 117, windowDockForRouting currently returns the workspace/alias Dock before fully validating surfaceID and paneID, which can let mixed routing selectors resolve to the wrong Dock. Update TerminalController+ControlSurfaceDock.windowDockForRouting to gather the candidate Docks for workspaceID, surfaceID, and paneID first, treat AppDelegate.windowDockAliasWorkspaceId as the only non-specific legacy case, and compare the explicit owner/surface/pane results. If the explicit selectors disagree, return nil instead of picking one arbitrarily; only return a Dock when the selectors consistently identify the same window Dock.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@Sources/TerminalController`+ControlSurfaceDock.swift:
- Around line 100-117: windowDockForRouting currently returns the
workspace/alias Dock before fully validating surfaceID and paneID, which can let
mixed routing selectors resolve to the wrong Dock. Update
TerminalController+ControlSurfaceDock.windowDockForRouting to gather the
candidate Docks for workspaceID, surfaceID, and paneID first, treat
AppDelegate.windowDockAliasWorkspaceId as the only non-specific legacy case, and
compare the explicit owner/surface/pane results. If the explicit selectors
disagree, return nil instead of picking one arbitrarily; only return a Dock when
the selectors consistently identify the same window Dock.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5a6ec9b8-6e9e-4d8a-ba5b-7ab8749c2237
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (10)
Sources/AppDelegate+DockShortcutRouting.swiftSources/AppDelegate+WindowDock.swiftSources/AppDelegate.swiftSources/TerminalController+ControlPaneContext.swiftSources/TerminalController+ControlSurfaceContext.swiftSources/TerminalController+ControlSurfaceContext2.swiftSources/TerminalController+ControlSurfaceContext3.swiftSources/TerminalController+ControlSurfaceDock.swiftSources/TerminalController.swiftcmuxTests/DockSocketLifecycleTests.swift
There was a problem hiding this comment.
1 issue found across 11 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…routing Busy window-Dock terminals now make the quit / last-window-close warning appear, matching workspace Docks (the retired Global Dock was never counted). An explicit window_id that contradicts the Dock a command resolves (by owner workspace_id, surface, or pane) now fails closed in windowDockForRouting, the browser resolvers' conflict check, and the focus/close containment branches, so explicit window routing is honored or rejected — never silently overridden. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…licy A recoverable (already-closed) window's manager can no longer lazily create a NEW Dock store through CLI paths — it would have no teardown owner, leaving headless panels running until quit. Manager-based Dock creation now requires a live registered window; only an existing store stays addressable during close races. The unconditional teardown on window unregister is documented as deliberate: a busy Dock panel does not veto its window's close, matching the window's workspace surfaces, while the menu path keeps its unconditional dialog and quit counts window Docks via hasQuitConfirmationDirtyWorkspaces. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/TerminalController.swift`:
- Around line 5326-5334: The Dock routing conflict check in TerminalController
should also validate the requested workspace scope, not just the resolved Dock
stores. Update the conflict logic around windowDockSelectorsConflict so it takes
requestedWorkspaceID into account and fails closed when workspace_id disagrees
with surface_id/pane_id, except for the legacy alias or the Dock owner ID. Keep
the existing routing flow in TerminalController’s browser-command handling, but
ensure the guard before selecting dockBySurface/dockByPane/dockByOwner rejects
mixed workspace/Dock selectors instead of falling back.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 6baab771-d53e-405b-9cb8-64b4603e0ed2
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (7)
Sources/QuitConfirmationAlertPresenter.swiftSources/TerminalController+ControlSurfaceContext.swiftSources/TerminalController+ControlSurfaceContext2.swiftSources/TerminalController+ControlSurfaceDock.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/DockSocketLifecycleTests.swift
👮 Files not reviewed due to content moderation or server errors (4)
- cmux.xcodeproj/project.pbxproj
- Sources/QuitConfirmationAlertPresenter.swift
- Sources/TerminalController+ControlSurfaceDock.swift
- Sources/TerminalController+ControlSurfaceContext.swift
…browser resolvers "Conflicting Dock routing selectors" now goes through String(localized:) with en/ja entries like the other Dock socket errors. The browser resolvers' conflict check also fails closed when a workspace_id names a NON-Dock scope while surface/pane selectors point into a window Dock (browser CLI commands never inject caller workspace context, unlike close-surface, so this cannot break first-party flows). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
No budget increases: the window-Dock browser resolvers, selector conflict check, owner-anchored payload, and Dock browser close move to a new TerminalController+WindowDockBrowserRouting.swift; the shared focus/reveal owner-anchoring and the Dock close branch move into TerminalController+ControlSurfaceDock.swift as reusable helpers; the main-window ForTesting seams move to a dedicated AppDelegate+MainWindowTestingSupport.swift per the debug-seam policy; and the cross-window routing socket tests move to a new WindowDockRoutingSocketTests.swift. Behavior is unchanged. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Emptying a window's only workspace closes the window, and window close now tears down its Dock — so accepting that move would destroy the just-moved surface. Mirrors the existing workspace-Dock self-move guard; windows with more workspaces are unaffected (only the emptied workspace closes). Regression-covered in WindowDockLifecycleTests. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/RightSidebarPanelView.swift (1)
419-433: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winMove window-dock creation out of the render path
Sources/RightSidebarPanelView.swift:420
AppDelegate.windowDock(for:)lazily creates theDockSplitStore, so this call can mutate registry state duringbodyevaluation. Trigger the first access from an explicit lifecycle hook (.onAppear, mode change, ordidSet) and keep the view path read-only.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@Sources/RightSidebarPanelView.swift` around lines 419 - 433, The dock lookup in dockPanel(windowAppearance:) is mutating state during view rendering because AppDelegate.windowDock(for:) lazily creates the DockSplitStore; move that first-access side effect out of the body path. Use an explicit lifecycle trigger tied to RightSidebarPanelView, such as .onAppear, a mode change handler, or a didSet on the relevant state, so DockPanelView only reads an already-created dock and the render path stays read-only.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@Sources/TerminalController.swift`:
- Around line 3169-3173: The Dock-owner routing in TerminalController should not
depend on an already-created Dock, because first-use requests can miss the
intended window and fall back incorrectly. In the workspace-id handling path,
keep the existing Dock-scoped lookup, but when it fails, resolve the owner
directly through tabManagerFor(windowId:) using the workspace lookup result
before any fallback to the active window. Apply the same fix in both the Dock
owner resolution and dockSurfaceCreate-related paths so tab selection is always
based on the authoritative window identity.
In `@Sources/TerminalController`+WindowDockBrowserRouting.swift:
- Around line 17-19: Explicit Dock routing selectors are currently being treated
as absent when v2UUID(...) returns nil for empty or invalid values, which allows
unintended fallback to the focused/owner Dock. Update the routing validation in
TerminalController+WindowDockBrowserRouting, including the selector handling in
v2ResolveWindowDockBrowserTabStore, so that presence of keys like surface_id,
pane_id, target_surface_id, target_pane_id, and window_id is checked before
parsing and any present-but-invalid UUID returns invalid_params instead of
falling back. Use the existing requestedWorkspaceID, requestedSurfaceID, and
requestedPaneID style variables as the entry points for this fail-closed
validation.
---
Outside diff comments:
In `@Sources/RightSidebarPanelView.swift`:
- Around line 419-433: The dock lookup in dockPanel(windowAppearance:) is
mutating state during view rendering because AppDelegate.windowDock(for:) lazily
creates the DockSplitStore; move that first-access side effect out of the body
path. Use an explicit lifecycle trigger tied to RightSidebarPanelView, such as
.onAppear, a mode change handler, or a didSet on the relevant state, so
DockPanelView only reads an already-created dock and the render path stays
read-only.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 75bf9968-8c5d-4287-abc2-10b19a66bce5
📒 Files selected for processing (18)
Resources/Localizable.xcstringsSources/AppDelegate+DockSurfaceMove.swiftSources/AppDelegate+MainWindowTestingSupport.swiftSources/AppDelegate+WindowDock.swiftSources/AppDelegate.swiftSources/DockSplitStore+SurfaceTransfer.swiftSources/RightSidebarPanelView.swiftSources/TerminalController+ControlPaneContext.swiftSources/TerminalController+ControlSurfaceContext.swiftSources/TerminalController+ControlSurfaceContext2.swiftSources/TerminalController+ControlSurfaceContext3.swiftSources/TerminalController+ControlSurfaceDock.swiftSources/TerminalController+WindowDockBrowserRouting.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxprojcmuxTests/DockSocketLifecycleTests.swiftcmuxTests/WindowDockLifecycleTests.swiftcmuxTests/WindowDockRoutingSocketTests.swift
💤 Files with no reviewable changes (1)
- cmuxTests/DockSocketLifecycleTests.swift
Per the debug-seam policy's preferred fix: the register/unregister ForTesting helpers now live in cmuxTests and reach internal AppDelegate state via @testable import, instead of shipping in production Sources. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…uting The CLI injects the caller's CMUX_WORKSPACE_ID unconditionally on the surface/pane command family even with explicit surface UUIDs, so the browser-style non-Dock-workspace conflict rule cannot apply here without breaking Dock surface targeting from main-area terminals. Encode the invariant where the precedence lives. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
A supplied-but-unresolvable surface/pane/workspace/window handle (empty string, stale ref) no longer degrades to the focused/owner Dock fallback: both window-Dock browser resolvers now apply the canonical v2RejectUnresolvedHandles guard once the request routes to a Dock, matching the methods that already pre-validate. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
The containment-based focus branch now fails closed when an explicit Dock-owner workspace_id (or window_id) names a different window's Dock than the one containing the surface, matching the other window-Dock resolvers. Non-Dock workspace ids stay non-conflicting for CLI caller-context injection. Regression-covered in the two-window test. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Runtime closes route by the surface's owner id, which for a window Dock is a window id no TabManager tab matches — the close and child-exit callbacks silently no-opped (as they already did for the retired Global Dock's synthetic owner id). Both callbacks now route window-Dock surfaces to their owning store first, so Ctrl-D and close bindings tear the panel down instead of leaving a dead surface retained. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Contributor
Author
|
@coderabbitai resume |
✅ Action performedReviews resumed. |
There was a problem hiding this comment.
1 issue found across 5 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmuxTests/WindowDockRoutingSocketTests.swift (1)
249-249: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winReuse
expectInvalidParamsfor all fail-closed assertions instead of checkingok==falsealone.Several fail-closed assertions in this test only check
envelope["ok"] as? Bool == false(e.g. Lines 249, 297, 320, 325, 330, 335, 343, 352, 357, 362), while theexpectInvalidParamshelper (Lines 192-196) already exists and is used elsewhere in the same test (Lines 227, 238, 312, 380, 391) to also asserterror["code"] == "invalid_params". A prior review on this file flagged a similar weakness (checking the localized message instead of the stable error code), which was fixed for one call site — but these newer conflict assertions reintroduce a related gap: they only prove some failure occurred, not that it's specifically the intended selector-conflict rejection. If an unrelated bug caused a different error path to fire, these assertions would still pass.♻️ Suggested fix
- `#expect`(lazyOwnerReadConflict["ok"] as? Bool == false) + try expectInvalidParams(lazyOwnerReadConflict)Apply the same substitution at the other
["ok"] as? Bool == false-only checks (Lines 297, 320, 325, 330, 335, 343, 352, 357, 362).Also applies to: 293-297, 308-343, 348-362
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cmuxTests/WindowDockRoutingSocketTests.swift` at line 249, The fail-closed assertions in WindowDockRoutingSocketTests are only checking that the response is not ok, which doesn’t verify the intended invalid_params failure path. Replace the remaining envelope["ok"] as? Bool == false-only checks with the existing expectInvalidParams helper so the tests also assert error["code"] == "invalid_params". Use expectInvalidParams consistently at the conflict-selector call sites in this test, matching the same pattern already used elsewhere.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@cmuxTests/WindowDockRoutingSocketTests.swift`:
- Line 249: The fail-closed assertions in WindowDockRoutingSocketTests are only
checking that the response is not ok, which doesn’t verify the intended
invalid_params failure path. Replace the remaining envelope["ok"] as? Bool ==
false-only checks with the existing expectInvalidParams helper so the tests also
assert error["code"] == "invalid_params". Use expectInvalidParams consistently
at the conflict-selector call sites in this test, matching the same pattern
already used elsewhere.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 3b8cf487-9df6-47eb-967d-618a264b42e9
📒 Files selected for processing (1)
cmuxTests/WindowDockRoutingSocketTests.swift
azooz2003-bit
added a commit
that referenced
this pull request
Jul 3, 2026
…dowRegistry (dissolved MainWindowContext); wire Dock teardown into context removal (round 41-fix)
azooz2003-bit
added a commit
that referenced
this pull request
Jul 3, 2026
… dropped from Workspace.swift during merge (#7144) (round 43-fix)
azooz2003-bit
added a commit
that referenced
this pull request
Jul 3, 2026
…kspace terminal-creation chain; wire workspace-Dock teardown (round 44-fix)
azooz2003-bit
added a commit
that referenced
this pull request
Jul 3, 2026
… SidebarBonsplitTabDropDelegate (#7144 sidebar tab-move), wire pbxproj (round 46-fix)
This was referenced Jul 7, 2026
This was referenced Jul 15, 2026
This was referenced Jul 17, 2026
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #7142 — every cmux window now hosts its own independent Dock. Opening a second window no longer shows the "Global Dock is active in another window" placeholder: each window's right sidebar mounts its own live
DockSplitStore, so N windows can run N Docks (terminals + browsers) simultaneously.Behavior
~/.config/cmux/dock.json,.cmux/dock.jsonsemantics unchanged). No state migrates between windows.closeAllPanels()(terminals, browsers, portals — no leaked PTYs). The old app-lifetime persistence of the single Global Dock is intentionally retired. Quit and last-window close go through the same unregister path.DockInactiveHostView,renderHostId, and its first-claim/handoff logic are removed (dead code under per-window stores). ThevisibleUIHostIdsunion survives to keep visibility stable across transient SwiftUI remounts within a window. The removeddock.inactiveHost.titlestring is deleted fromLocalizable.xcstrings.Design: owner id == window id
Each per-window store's
workspaceIdis its owning window'swindowId(registry:AppDelegate.windowDocksById, seeAppDelegate+WindowDock.swift). That makes Dock-scoped CLI results self-describing (workspace_idnames the window whose Dock was hit) and makes routing a dictionary lookup.CLI/socket compatibility:
D0CCD0CC-…) keeps routing as an alias meaning "the Dock of the routed window" (explicitwindow_id, else the caller's window), so existing scripts keep working.workspace_idroutes commands to that window (resolveTabManagerprefers the owning window over the caller).surface.focus,surface.close, browser commands, etc. resolve the owning window cross-window.Call-site audit (every user of the old
app.globalDock/existingGlobalDock/isGlobalDockOwnerId)RightSidebarPanelViewapp.windowDock(for: tabManager)— the window's own store.dockSurfaceCreate/dockPaneCreate(--placement dock)windowDockForRouting(wasglobalDockForRouting)focusedDockStoreForShortcut) + focused close (closeFocusedDockPanelForCommand)context.windowId).dockForPane,locateDockSurface,moveSurfaceIntoDock,moveDockSurfaceTo*)dockReferenceTabManagerv2ResolveWindowDockBrowser*)tabManagerso the alias resolves per window.resolvedSurfaceIdForClosedock branchwindowDockForRoutinghandles owner-id routing first (comment explains).Workspace.openDockBrowserLinkInNewTabIfNeededWorkspace-scoped Docks (
Workspace.dockSplit, project.cmux/dock.jsontrees) are untouched.Tests
cmuxTests/WindowDockLifecycleTests.swift(wired intoproject.pbxproj): independent store per window (identity, owner ids, routing ids), teardown on window unregister (registry drop + panels closed + not visible), and both windows' Docks visible/portal-active simultaneously (the exact regression the render-host gate used to cause).DockSocketLifecycleTestsupdated to the new semantics: results report the owning window id; owner-id routing for list/current/close/pane-mutation/browser commands; a dedicated legacy-alias compat test; the two-window focused-close test now proves Cmd+W in window B closes B's Dock panel while window A's Dock is untouched.Localization audit
Removed surface only:
DockInactiveHostViewand itsdock.inactiveHost.titlekey (en + ja) deleted fromResources/Localizable.xcstrings. No new user-facing strings were introduced (verified viargover the touched Swift files for newText(/String(localized:usages — none added).docs/dock.md(English-only CLI doc) gained a per-window paragraph; the localized web docs catalogs (web/messages/*) contain no window-sharing claims, so no catalog changes were needed.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Replaces the single app-wide Dock with independent per-window Docks. Each window owns, renders, and tears down its Dock seeded from
~/.config/cmux/dock.json; routing, shortcuts, browser/socket commands, and quit/close flows now target the owning window. Fixes #7142.New Features
workspace_idequals itswindow_id; the legacy global‑dock alias means “the Dock of the routed window”; explicitwindow_id/pane_idare honored; surface/pane containment and drag/move scan across windows; browser shortcuts route to the window Dock.Bug Fixes
Written for commit 0c1c671. Summary will update on new commits.
Summary by CodeRabbit