Repository navigation
Reconcile Dock portal bindings at reparent time instead of on focus - #7786
Conversation
Two regression tests for the class of bug where surfaces moved into or revealed in the Dock stay blank until the Dock gains focus: - dockBrowserAttachIntoVisibleDockShowsPortal: attaching a live browser panel into a visible Dock must mark its window-portal entry visible at attach time. Fails today because DockSplitStore.applyVisibility only implements the browser hide side; the show side is a no-op. - dockBrowserRevealRestoresPortalVisibility: hiding and re-revealing the Dock must restore the browser portal entry's visibility. Fails today for the same reason (the #5435 family, browser flavor). CI is expected to go red on this commit; the fix lands in the next commit per the repo's two-commit regression policy. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Fixes #7780. Fixes #7305. Root cause: terminal and browser content views are window-level portal-hosted, and a portal bind silently no-ops when the target anchor is momentarily off-window during reparent churn (both registries guard on anchorView.window). The main split area self-heals after topology changes via Workspace's event-driven reconcile passes, but the Dock had no reconciler at all: terminals only got a synchronous reattach-token bump at attach time (which can itself race the churn), and browsers had no show path whatsoever — DockSplitStore.applyVisibility only implemented the browser hide side. The only reliable recovery was the SwiftUI re-render caused by rightSidebarOwnsInputFocus flipping, which is why blanked surfaces came back exactly when the Dock gained focus. Fix: - New DockSplitStore+PortalReconcile.swift: an idempotent Dock portal reconcile pass covering terminals and browsers, mirroring the Workspace reconcile idiom — coalesced zero-delay first attempt, follow-ups re-armed by the structural notifications that fire when reparent churn settles, wall-clock stall backoff, and a hard deadline that tears down observers. Healthy state is a pure-check no-op. - Browser show parity in DockSplitStore.applyVisibility: a visible Dock browser now marks its portal entry visible and schedules recovery only when the binding is actually damaged. - Reconcile scheduling from every Dock mutation path: attach (including rollback attaches), internal pane drops and drag-to-splits, bonsplit didMoveTab/didSplitPane, Dock reveal, and zoom toggles. - Cross-container moves reconcile both sides: moveSurfaceIntoDock schedules the destination Dock; moveDockSurfaceToWorkspace and moveDockSurfaceToNewWorkspace schedule the source Dock and run the destination workspace's terminal and browser reconcile after the attach/split legs. The regression tests from the previous commit now pass; additional coverage exercises the reconciler directly (stale terminal bind without focus, unbound browser imperative bind, healthy-browser no-op, and move-path scheduling in both directions). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughDock portal reconciliation now repairs terminal and browser visibility, bindings, geometry, and refresh state after Dock mutations and structural events. Surface moves, focus changes, pane operations, and visibility updates schedule reconciliation, with retry handling and regression tests. ChangesDock portal reconciliation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DockSplitStore
participant TerminalWindowPortalRegistry
participant BrowserWindowPortalRegistry
DockSplitStore->>DockSplitStore: Schedule reconciliation after Dock mutation
DockSplitStore->>DockSplitStore: Flush layouts and inspect panels
DockSplitStore->>TerminalWindowPortalRegistry: Reattach and reconcile terminal portal
DockSplitStore->>BrowserWindowPortalRegistry: Bind and update browser portal
DockSplitStore->>DockSplitStore: Retry until portal readiness is complete
Possibly related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (4 errors, 1 warning)
✅ Passed checks (20 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 |
Greptile SummaryThis PR adds Dock portal reconciliation so moved Dock surfaces recover without relying on focus. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (3): Last reviewed commit: "Abort browser waitUntil timeouts without..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| private func refreshDockPortalReconcileTimeout() { | ||
| let state = dockPortalReconcileState | ||
| state.timeoutWorkItem?.cancel() | ||
| let workItem = DispatchWorkItem { [weak self] in | ||
| self?.clearDockPortalReconcile() | ||
| } | ||
| state.timeoutWorkItem = workItem | ||
| DispatchQueue.main.asyncAfter( | ||
| deadline: .now() + Self.dockPortalReconcileTimeout, | ||
| execute: workItem | ||
| ) |
There was a problem hiding this comment.
When a Dock portal anchor or hosted view becomes ready after this fixed 2-second window, clearDockPortalReconcile() removes the observers and invalidates pending attempts before the real move-to-window or registry-change notification arrives. A slow reparent, busy main thread, or delayed WebKit/terminal attach can then leave the Dock surface blank until another unrelated Dock mutation schedules a fresh reconcile.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
There was a problem hiding this comment.
By design, matching the repo's sanctioned mechanism: the 2s deadline is byte-for-byte the bound Workspace.beginEventDrivenLayoutFollowUp uses for the identical class of race in the main split area (Sources/Workspace.swift, event-driven layout follow-up). The deadline is a leak/wakeup backstop, not the primary signal — recovery is driven by the four structural notifications that fire when SwiftUI actually commits the reparent, and the browser/terminal view-side onDidMoveToWindow bind paths remain the first line of recovery independent of this reconciler. Any subsequent Dock mutation (attach, drop, split, selection, reveal, zoom) re-arms a fresh chain with a fresh deadline, so an anchor that becomes ready later than 2s without any of those signals is the same residual the main area already accepts. Keeping observers armed indefinitely is what the bound exists to prevent.
| private func scheduleDockPortalReconcileAttempt() { | ||
| let state = dockPortalReconcileState | ||
| guard state.timeoutWorkItem != nil else { return } | ||
| guard !state.attemptScheduled else { return } | ||
|
|
||
| state.attemptScheduled = true | ||
| let delay = dockPortalReconcileBackoffDelay() | ||
| let version = state.attemptVersion | ||
| DispatchQueue.main.asyncAfter(deadline: .now() + delay) { [weak self] in | ||
| guard let self else { return } | ||
| let state = self.dockPortalReconcileState | ||
| guard state.attemptVersion == version, | ||
| state.timeoutWorkItem != nil else { return } | ||
| state.attemptScheduled = false | ||
| self.attemptDockPortalReconcile() | ||
| } |
There was a problem hiding this comment.
When dockBrowserPortalAnchorReady or the terminal bounds/surface checks stay false, this loop retries with DispatchQueue.main.asyncAfter rather than waiting only for the structural notifications that prove readiness changed. That can both keep doing repeated main-thread layout/portal work while nothing changed and still miss recovery once the hard timeout clears the observers.
Rule Used: Flag new blocking or timing-based synchronization ... (source)
There was a problem hiding this comment.
The loop is event-driven first: the four structural notifications (terminalSurfaceHostedViewDidMoveToWindow, terminalPortalVisibilityDidChange, browserPortalRegistryDidChange, terminalSurfaceDidBecomeReady) immediately re-arm a zero-delay attempt when readiness actually changes. The 10ms→250ms stall backoff only spaces retries while the anchor is not yet ready — that path performs no registry mutations and posts nothing, so it cannot spin — and it mirrors Workspace.beginEventDrivenLayoutFollowUp's stall backoff exactly, bounded by the same 2s deadline. A purely notification-driven loop would hang forever on the one case this exists to repair: a bind dropped because the commit produced no observable notification.
There was a problem hiding this comment.
That addresses the concern. The critical insight — that the silent-drop case is precisely the one that produces no notification — justifies having a backoff path rather than relying on notifications alone. And with no registry mutations on the stall path, the cost is just a few cheap readiness checks until the 2s deadline fires.
Withdrawn.
| #if DEBUG | ||
| extension DockSplitStore { | ||
| func debugDockPortalReconcileScheduleCountForTesting() -> Int { | ||
| dockPortalReconcileState.scheduledRequestCount | ||
| } | ||
|
|
||
| func debugResetDockPortalReconcileStateForTesting() { | ||
| clearDockPortalReconcile() | ||
| dockPortalReconcileState.scheduledRequestCount = 0 | ||
| } | ||
|
|
||
| @discardableResult | ||
| func debugReconcileDockPortalsForTesting(reason: String = "dock.portal.test") -> Bool { | ||
| reconcileDockPortalPass(reason: reason) | ||
| } | ||
| } |
There was a problem hiding this comment.
These #if DEBUG ForTesting methods expose and drive private reconciler state from a file under Sources/, and the new tests call them to reset schedule state, count requests, and run the reconcile pass. The repo rule keeps this kind of test-only surface out of production Swift; the tests should reach internal state through @testable import or a test-support target instead.
Rule Used: Do not add new test/debug seams (ForTesting-styl... (source)
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Fixed in a26c14a: the #if DEBUG debug…ForTesting extension is removed per .github/review-bot-rules/no-test-debug-seam-in-production-source.md. Tests now call the internal reconcileDockPortalPass(reason:) / clearDockPortalReconcile() directly and read the store-owned dockPortalReconcileState via @testable import (the rule's canonical fix).
| private var dockPortalReconcileState: DockPortalReconcileState { | ||
| if let state = objc_getAssociatedObject(self, &dockPortalReconcileStateKey) as? DockPortalReconcileState { | ||
| return state | ||
| } | ||
| let state = DockPortalReconcileState() | ||
| objc_setAssociatedObject(self, &dockPortalReconcileStateKey, state, .OBJC_ASSOCIATION_RETAIN_NONATOMIC) | ||
| return state | ||
| } |
There was a problem hiding this comment.
scheduleDockPortalReconcile reaches this property on DockSplitStore, but the inspected declaration is a pure Swift final class DockSplitStore: BonsplitDelegate, not an NSObject subclass. If that protocol does not make the class Objective-C-compatible in every app build, the first reconcile schedule passes a non-ObjC object to objc_getAssociatedObject/objc_setAssociatedObject, which can crash the Dock move/reveal path instead of recovering the portal.
There was a problem hiding this comment.
Resolved by a26c14a — the associated-object pattern is gone entirely; the state is now a declared stored property on DockSplitStore, so no ObjC-runtime dependency remains.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/DockSplitStore+PaneFocus.swift (1)
92-100: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winRoute Bonsplit selection callbacks through this scheduled path.
Lines 119-129 still call
applyDockSelectiondirectly, bypassing this new scheduling path. A user selecting or focusing a stale terminal portal therefore only receives the lightweight visibility update, not the full reconcile pass. Have both delegate callbacks callapplyFocusedDockSelection()(or a shared helper that schedules afterward).Proposed fix
func splitTabBar(_ controller: BonsplitController, didSelectTab tab: Bonsplit.Tab, inPane pane: PaneID) { - applyDockSelection(tabId: tab.id, inPane: pane) + applyFocusedDockSelection() } func splitTabBar(_ controller: BonsplitController, didFocusPane pane: PaneID) { - guard let tab = controller.selectedTab(inPane: pane) else { - applyVisibilityToAllPanels() - return - } - applyDockSelection(tabId: tab.id, inPane: pane) + applyFocusedDockSelection() }As per coding guidelines, use one shared action path rather than wiring behavior separately across surfaces.
🤖 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/DockSplitStore`+PaneFocus.swift around lines 92 - 100, Route both Bonsplit selection/focus delegate callbacks through applyFocusedDockSelection() instead of calling applyDockSelection directly. Update the callbacks around the existing direct calls so they use this shared scheduled path, ensuring stale terminal portal selections trigger the full dock portal reconcile.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/DockSplitStore`+PortalReconcile.swift:
- Around line 276-292: Remove the DEBUG-only DockSplitStore testing extension
and its methods debugDockPortalReconcileScheduleCountForTesting(),
debugResetDockPortalReconcileStateForTesting(), and
debugReconcileDockPortalsForTesting(). Update tests to use the existing internal
reconcileDockPortalPass(reason:) via `@testable` import and assert portal-registry
outcomes directly.
- Around line 4-31: Move DockPortalReconcileState ownership into DockSplitStore:
remove the mutable file-global dockPortalReconcileStateKey and Objective-C
association logic, declare the reconciliation state as an injected/store-owned
property initialized by DockSplitStore’s constructor, and update the
dockPortalReconcileState accessor and all reconciliation methods to use that
property directly.
- Around line 74-84: The portal repair logic in
refreshDockPortalReconcileTimeout and the related retry/layout methods relies on
delayed dispatch, exponential retries, and forced polling. Remove the timeout
and all DispatchQueue.main.asyncAfter-based coordination, retry counters, and
layout polling; instead, trigger one coalesced reconciliation pass from scoped
attachment, layout, and registry-completion lifecycle events, using the existing
portal reconciliation state and clearDockPortalReconcile symbols.
---
Outside diff comments:
In `@Sources/DockSplitStore`+PaneFocus.swift:
- Around line 92-100: Route both Bonsplit selection/focus delegate callbacks
through applyFocusedDockSelection() instead of calling applyDockSelection
directly. Update the callbacks around the existing direct calls so they use this
shared scheduled path, ensuring stale terminal portal selections trigger the
full dock portal reconcile.
🪄 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: b7005de3-0b47-4ff7-a4ab-ed866f84b30f
📒 Files selected for processing (7)
Sources/AppDelegate+DockSurfaceMove.swiftSources/DockSplitStore+PaneFocus.swiftSources/DockSplitStore+PortalDrop.swiftSources/DockSplitStore+PortalReconcile.swiftSources/DockSplitStore+SurfaceTransfer.swiftcmux.xcodeproj/project.pbxprojcmuxTests/DockPortalReconcileTests.swift
- Replace the Objective-C associated-object state (and its top-level association key) with a declared DockSplitStore-owned dockPortalReconcileState property, per the no-ambient-global-state review rule. DockSplitStore.swift stays under its length budget by relocating the focus-ownership browserPanel(owning:in:) lookup to DockSplitStore+PaneFocus.swift. - Remove the #if DEBUG debug…ForTesting extension, per the no-test-debug-seam-in-production-source rule. Tests now drive the internal reconcileDockPortalPass(reason:)/clearDockPortalReconcile() API and read the store-owned state directly via @testable import. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
BrowserSessionHistoryRestoreTests' waitUntil recorded an XCTFail and then also threw BrowserTestTimeout. The thrown error is tallied by XCTest as an "unexpected" failure, which is the one class the app-host unit-test gate does not tolerate — so whenever this pre-existing timeout (it reproduces identically on main; see runs 29062349460 and 29063767894, BrowserConfigTests.swift:2667) lands in the final test-run segment, the whole shard goes red. On main it is usually masked because an earlier app-host crash restarts the run and the gate parses only the last summary; this PR's added test file re-packs the shards and put the suite in a segment that completes cleanly. Abort via continueAfterFailure = false instead: the timeout is still recorded as a visible test failure and still stops the test at the same point, but is no longer misclassified as a crash-grade failure. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This branch was successfully deployed
1 participant
Fixes #7780. Fixes #7305.
Bug
Moving surfaces (terminals AND browsers) between the main area and the right-sidebar Dock intermittently left them blank or invisible until the Dock gained focus — across both directions, plain drops, and drops into splits.
Root cause
Terminal and browser content views are window-level portal-hosted; a portal bind silently no-ops when the target anchor is momentarily off-window during reparent churn (
TerminalWindowPortalRegistry.bind/BrowserWindowPortalRegistry.bindboth guard onanchorView.window). The main split area self-heals after topology changes viaWorkspace's event-driven reconcile passes (reconcileTerminalGeometryPass,reconcileBrowserPortalVisibilityForCurrentRenderedLayout), but the Dock had no reconciler at all:DockSplitStore.applyVisibilityonly implemented the browser hide side.The only reliable recovery was the SwiftUI re-render caused by
rightSidebarOwnsInputFocusflipping — which is exactly why blanked surfaces came back when the Dock gained focus. Prior point fixes (#7054, #5435, #7529) each patched one trigger of that re-render without closing the underlying gap.Fix
Give the Dock the same authoritative portal reconciliation the main area already has:
Sources/DockSplitStore+PortalReconcile.swift: idempotent Dock portal reconcile pass for terminals and browsers, mirroring the Workspace idiom — coalesced zero-delay first attempt, follow-ups re-armed by the structural notifications that fire when reparent churn settles (.terminalSurfaceHostedViewDidMoveToWindow,.terminalPortalVisibilityDidChange,.browserPortalRegistryDidChange,.terminalSurfaceDidBecomeReady), wall-clock stall backoff (10ms→250ms), and a 2s hard deadline that tears down observers. Healthy state is a pure-check no-op (no portal mutations, no refresh, no registry posts).DockSplitStore.applyVisibility: a visible Dock browser now marks its portal entry visible and schedules recovery only when the binding is actually damaged.performPortalPaneDrop), bonsplitdidMoveTab/didSplitPane, Dock reveal, zoom toggles.moveSurfaceIntoDockschedules the destination Dock;moveDockSurfaceToWorkspace/moveDockSurfaceToNewWorkspaceschedule the source Dock and run the destination workspace's terminal + browser reconcile after the attach/split legs.No focus-semantics changes; recovery no longer depends on the Dock gaining focus in any path.
Commits (two-commit regression policy)
aac686c— failing regression tests only (browser attach-into-visible-Dock and Dock reveal). CI is expected red on this commit.2661432— the fix + reconciler-specific coverage (stale terminal bind without focus, unbound browser imperative bind, healthy-browser no-op, move-path scheduling both directions).Verification
cmuxTests/DockPortalReconcileTests.swift(wired into pbxproj;scripts/lint-pbxproj-test-wiring.shpasses).python3 scripts/swift_file_length_budget.pypasses; no budget TSV touched; new files are 292/327 lines; no new warnings.-derivedDataPath /tmp/cmux-fable-7780).🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Touches window-level portal registries and many Dock mutation paths; incorrect reconcile timing could cause flicker or duplicate binds, but behavior mirrors an existing Workspace pattern and is covered by new tests.
Overview
Fixes Dock terminals and browsers staying blank after moves or layout changes until the Dock gained focus, by adding event-driven portal reconciliation parallel to the main workspace.
New
DockSplitStore+PortalReconcile.swiftintroducesscheduleDockPortalReconcile/reconcileDockPortalPass: coalesced attempts, notification-driven wakeups (terminal hosted-view moves, portal visibility, browser registry), exponential backoff, and a 2s timeout. Visible panels get terminal reattach/geometry refresh and imperative browser portal bind/sync when anchors are ready.applyVisibilitynow shows Dock browsers (registry visibility + reconcile when binding is damaged), not only hide.Reconcile is hooked from Dock topology paths: attach/detach, pane drops, tab move/split, selection, zoom, and
AppDelegatecross-container moves (moveSurfaceIntoDock; workspace moves also run destination workspace terminal/browser reconcile and schedule source Dock reconcile).browserPanel(owning:in:)moves toDockSplitStore+PaneFocus.swift.DockPortalReconcileTestsand a smallBrowserConfigTeststimeout failure style tweak round out the change.Reviewed by Cursor Bugbot for commit 023cd5e. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes Dock surfaces going blank after moves or reveal by reconciling terminal and browser portal bindings during reparenting instead of on focus. Restores portal visibility immediately and makes recovery deterministic. Fixes #7780 and #7305.
DockSplitStore+PortalReconcile.swift: idempotent, event‑driven reconcile for terminals and browsers with coalesced attempts, structural wakeups, short backoff, and a 2s timeout.DockSplitStore.applyVisibility; marksBrowserWindowPortalRegistryentries visible and only schedules recovery when the binding is damaged.DockSplitStore.dockPortalReconcileState; removed debug seams and moved focus‑ownershipbrowserPanel(owning:in:)toDockSplitStore+PaneFocus.swift.BrowserSessionHistoryRestoreTests.waitUntilnow aborts viacontinueAfterFailure = falseinstead of throwing to avoid false shard failures.Written for commit 023cd5e. Summary will update on new commits.
Summary by CodeRabbit