Prevent detached replacements from displacing rearmed Dock portal hosts

[austinywang]

Summary

• Reproduces the Dock portal lifecycle race where SwiftUI dismantles a visible terminal host, rearms its lease, and briefly creates a detached zero-sized replacement.
• Preserves the live host as portal authority until a distinct replacement is attached to a window and has usable bounds.
• Allows that same replacement to claim normally as soon as it becomes usable.

Why this is a draft

An earlier tagged dogfood flow also exposed stale browser placement and duplicate hover targets during pane dragging. Those are not claimed as fixed by this terminal lease change. The PR stays draft until fresh terminal/browser drag dogfood confirms this patch is isolated and does not reproduce those behaviors.

Regression sequence

1. 04936dbad9 — adds the failing behavioral regression test only.
2. f7e08c0353 — rejects ownership acquisition by a distinct detached/tiny host until it becomes usable.

The test-only commit was pushed first and the manual CI workflow was dispatched. Current origin/main blocks macOS tests earlier in workflow-guard-tests because WorkspaceTodoNotificationRegressionTests.swift is not wired into the cmuxTests target, so CI could not record the intended regression failure. Run: https://github.com/manaflow-ai/cmux/actions/runs/29552840750

Verification

• Regression-only local Dock portal suite: 13 tests executed; only the new detached-replacement case failed, on both unauthorized ownership and displaced-host assertions.
• Fixed HEAD rebased onto current origin/main: 13 tests in DockPortalReconcileTests passed.
• ./scripts/reload.sh --tag detached-replacement-portal succeeded on f7e08c0353.
• git diff --check passed.
• No user-facing strings changed; localization catalogs are unaffected.

Dogfood checklist before ready for review

• Repeated terminal pane moves into, within, and out of the Dock.
• Repeated browser pane moves over the same targets.
• Confirm exactly one drop-target overlay is visible during drag.
• Confirm browser portals move with their pane and terminal portals never go blank.

Summary by CodeRabbit

• Bug Fixes
  • Improved portal-host claiming and replacement to prevent detached or undersized hosts from taking over an active portal.
  • Enhanced ownership reconciliation during detach/rearm and browser detach/attach, including correct same-pane refresh behavior.
• Tests
  • Added new Dock and browser reconciliation tests covering detached vs attached claiming and lease selection outcomes.
• Refactor
  • Centralized portal-host lease usability and replacement decisioning into a shared policy, applied across terminal surfaces and the browser panel.

[coderabbitai]

📝 Walkthrough

Walkthrough

Portal host claiming and replacement now use a shared PortalHostLeasePolicy for area, usability, and ownership decisions. Terminal and browser flows consume the policy, while reconciliation tests cover detached and attached Dock and browser replacements.

Changes

Portal host lease policy

Layer / File(s)	Summary
Centralize lease policy rules Packages/macOS/CmuxTerminalCore/..., Packages/macOS/CmuxTerminal/..., Sources/Panels/BrowserPanel.swift	PortalHostLeasePolicy computes lease area, determines usability from attachment and minimum area, and evaluates replacement eligibility.
Apply policy to terminal claims Packages/macOS/CmuxTerminal/...	TerminalSurface.claimPortalHost constructs leases through the policy, delegates replacement decisions, and records policy-derived skip causes.
Apply policy to browser claims Sources/Panels/BrowserPanel.swift	Browser claims use policy-based usability and replacement handling, preserve portal locks, track instance serials, and update forced/cause logging.
Validate detached and attached replacements cmuxTests/DockPortalReconcileTests.swift	Tests verify detached Dock and browser replacements cannot displace live hosts, while attached replacements can claim after the live owner is released.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Host as Portal host
  participant Claim as claimPortalHost
  participant Policy as PortalHostLeasePolicy
  participant Lease as Active portal lease
  Host->>Claim: Submit bounds and ownership
  Claim->>Policy: Calculate area and evaluate usability
  Policy-->>Claim: Return lease metrics and replacement decision
  Claim->>Lease: Preserve or update active host lease

Loading

Possibly related PRs

• manaflow-ai/cmux#8283: Both PRs modify portal-host lease claiming and replacement behavior.
• manaflow-ai/cmux#8325: Both PRs overlap in portal-host lease claiming and ownership handling.

Suggested reviewers: lawrencecchen, azooz2003-bit

🚥 Pre-merge checks | ✅ 25

✅ Passed checks (25 passed)

Check name	Status	Explanation
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Cmux Swift Actor Isolation	✅ Passed	PASS: The patch keeps portal-host logic on @MainActor UI classes, and the new PortalHostLeasePolicy is a pure Sendable value type with no shared mutable state.
Cmux Swift Blocking Runtime	✅ Passed	The diff only refactors portal lease policy and adds tests; no new waits, sleeps, semaphores, sync dispatch, or NSLock-based synchronization were introduced.
Cmux Browser Automation Off-Main	✅ Passed	Diff only changes BrowserPanel portal-host lease logic; TerminalController and ControlCommandExecutionPolicy are untouched, so no browser socket automation routing violation appears.
Cmux Expensive Synchronous Load	✅ Passed	BrowserPanel.claimPortalHost only adds PortalHostLeasePolicy checks; no agent-history/session loads, JSON parsing, or other expensive sync work appears in the diff.
Cmux Cache Substitution Correctness	✅ Passed	The new lease policy and lock are transient portal-claim state, not persistence/history/undo/snapshot caches, and cold/stale cases are revalidated or cleared.
Cmux No Hacky Sleeps	✅ Passed	PASS: The diff is Swift-only; the rule scopes out Swift, and no non-Swift runtime scripts were added or modified.
Cmux Algorithmic Complexity	✅ Passed	BrowserPanel’s new claim logic is constant-time predicate work; no new scans, sorts, joins, or per-target rescans over scalable collections.
Cmux Swift Concurrency	✅ Passed	PR only refactors synchronous portal-lease logic and adds tests; no new DispatchQueue, Task, Combine app-state, or completion-handler APIs were introduced.
Cmux Swift @Concurrent	✅ Passed	No changed Swift code adds invalid @concurrent or nonisolated async; the touched portal-lease paths and tests are synchronous/UI-bound.
Cmux Swift Package Boundaries	✅ Passed	Reusable lease-area/usability/replacement logic was extracted to CmuxTerminalCore; BrowserPanel and TerminalSurface now only compose surface-specific state and logging.
Cmux Swiftpm Lockfiles	✅ Passed	No Package.swift, Package.resolved, .gitignore, Xcode, or workflow files changed; diff is source/test only.
Cmux Swift Logging	✅ Passed	PASS: The only Swift diff adds lease logic in BrowserPanel; no new or changed print/debugPrint/dump/NSLog/Logger usage, and the existing DEBUG cmuxDebugLog is allowed.
Cmux User-Facing Error Privacy	✅ Passed	Patch only adjusts internal portal-lease logic and DEBUG logs; no user-facing error/alert/output copy or sensitive details were added.
Cmux Full Internationalization	✅ Passed	No production user-facing copy was added; changes are logic/tests plus DEBUG-only logs and test reasons, all allowed by the i18n rule.
Cmux Swiftui State Layout	✅ Passed	The diff only adds 5 lines inside BrowserPanel.claimPortalHost; no new @Observable/@published state, GeometryReader, lazy rows, or render-time state writes were introduced.
Cmux Architecture Rethink	✅ Passed	The only new code refreshes existing lock state on same-host reclaim; it adds no sleeps, polling, delayed dispatch, or split ownership.
Cmux Swift Auxiliary Window Close Shortcuts	✅ Passed	The PR only changes portal-lease logic and test fixtures; it adds no new standalone NSWindow/NSPanel/WindowGroup code or cmux.* identifier assignments.
Cmux Source Artifacts	✅ Passed	All changed paths are intentional source/test files; no logs, screenshots, caches, temp, build output, or other artifacts appear in the diff.
Cmux No Test Or Debug Seam In Production Source	✅ Passed	PASS: Sources/Panels/BrowserPanel.swift only adds lease-policy claim logic and DEBUG logging; no new debug/test-only accessor or seam-shaped member was added.
Cmux No Ambient Global State	✅ Passed	Diff adds a constructable PortalHostLeasePolicy and local policy uses; production files only remove ambient helpers, with no new file-scope funcs, mutable vars, or singletons.
Title check	✅ Passed	The title accurately captures the main change: preventing detached replacements from displacing Dock portal hosts.
Description check	✅ Passed	The description covers summary, rationale, regression sequence, and verification, but omits some template sections like Demo Video and Review Trigger.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix-dock-blank-after-pane-moves

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[greptile-apps]

Greptile Summary

This PR fixes a SwiftUI Dock portal lifecycle race where a detached, zero-sized replacement host could displace the still-live rearmed host, leaving the terminal surface blank. It introduces PortalHostLeasePolicy in CmuxTerminalCore to centralise the usability and replacement rules, then adopts it in both TerminalSurface+PortalLease and BrowserPanel, replacing the previously duplicated helpers.

• PortalHostLeasePolicy.shouldReplace now gates all cross-host replacements (including cross-pane) on isUsable(candidate), so a detached or zero-area representable can never steal portal authority until it is window-attached with meaningful bounds.
• BrowserPanel drops its local PortalHostLease duplicate, adopts the shared type, and adds a new no-current-lease guard so a pending distinct replacement also blocks an unusable first-time claimant.
• Two new regression tests in DockPortalReconcileTests cover the terminal detach/rearm sequence and the browser detach/attach lifecycle end-to-end.

Confidence Score: 5/5

Safe to merge after the dogfood checklist passes; the change tightens portal ownership and cannot produce a worse outcome than the existing blank-portal bug.

The core invariant is cleanly expressed in one place and verified by new targeted tests. No production logic was added that introduces new state, side channels, or timing dependencies.

No files require special attention beyond verifying the dogfood results for cross-pane terminal and browser pane drags.

Important Files Changed

Filename	Overview
Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/SurfaceValues/PortalHostLeasePolicy.swift	New shared policy struct that centralizes portal-host usability and replacement rules across terminal and browser surfaces; clean, well-documented, Sendable, and correctly gates all cross-host replacements on candidate usability.
Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+PortalLease.swift	Removes the duplicated usability helpers and delegates to PortalHostLeasePolicy; the newerSamePaneHostReady and newerModelOwnerReady flags lose their nextUsable && guard (dead code noted in prior review comments since shouldReplace gates on isUsable first) and the cross-pane hand-off comment is stale (both called out in prior comments).
Sources/Panels/BrowserPanel.swift	Removes local PortalHostLease duplicate and adopts the shared CmuxTerminalCore type; browser now hardcodes instanceSerial: 0 since it has no serial tracking — the field is unused by shouldReplace but reads misleadingly; the new no-current-lease usability guard and same-host-refresh pending-replacement consumption are correct.
cmuxTests/DockPortalReconcileTests.swift	Adds two well-structured regression tests: one for the terminal Dock portal detached-replacement race and one for the browser detach/attach lifecycle; both cover the new blocking behavior and the subsequent usable-host success path.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A([claimPortalHost called]) --> B{activePortalHostLease exists?}
    B -- No --> C{consumesPendingDistinctReplacement?}
    C -- Yes --> D{isUsable candidate?}
    D -- No --> E([return false / skip])
    D -- Yes --> F[consume pending\nclear + lock host]
    F --> G([set lease / return true])
    C -- No --> G

    B -- Yes --> H{current.hostId == hostId?\nself-refresh}
    H -- Yes --> I{pendingDistinct for this pane\n&& isUsable?}
    I -- Yes --> J[consume pending\nset lock]
    J --> K([update lease / return true])
    I -- No --> K

    H -- No --> L[compute\nallowsSamePaneReplacement]
    L --> M[leasePolicy.shouldReplace]
    M --> N{isUsable candidate?}
    N -- No --> O([return false\ncause=detachedOrTiny])
    N -- Yes --> P{cross-pane?\ncurrent.paneId != candidate.paneId}
    P -- Yes --> Q([replace / return true])
    P -- No --> R{!isUsable current\nOR allowsSamePane?}
    R -- Yes --> Q
    R -- No --> S([return false\ncause=ownerPreferred])

Loading

%%{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"}}}%%
flowchart TD
    A([claimPortalHost called]) --> B{activePortalHostLease exists?}
    B -- No --> C{consumesPendingDistinctReplacement?}
    C -- Yes --> D{isUsable candidate?}
    D -- No --> E([return false / skip])
    D -- Yes --> F[consume pending\nclear + lock host]
    F --> G([set lease / return true])
    C -- No --> G

    B -- Yes --> H{current.hostId == hostId?\nself-refresh}
    H -- Yes --> I{pendingDistinct for this pane\n&& isUsable?}
    I -- Yes --> J[consume pending\nset lock]
    J --> K([update lease / return true])
    I -- No --> K

    H -- No --> L[compute\nallowsSamePaneReplacement]
    L --> M[leasePolicy.shouldReplace]
    M --> N{isUsable candidate?}
    N -- No --> O([return false\ncause=detachedOrTiny])
    N -- Yes --> P{cross-pane?\ncurrent.paneId != candidate.paneId}
    P -- Yes --> Q([replace / return true])
    P -- No --> R{!isUsable current\nOR allowsSamePane?}
    R -- Yes --> Q
    R -- No --> S([return false\ncause=ownerPreferred])

Loading

_{Reviews (3): Last reviewed commit: "fix: consume surviving browser portal ha..." | Re-trigger Greptile}

[coderabbitai]

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/Panels/BrowserPanel.swift`:
- Around line 3452-3458: Update the shouldReplace ownership-change path to clear
pendingDistinctPortalHostReplacementPaneId after every successful distinct
replacement. Capture whether the replacement consumed the pending request before
clearing it, and set lockedPortalHost only for that consumed-request case;
preserve the existing unlock behavior for matching non-forced locks.

🪄 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: 41a18ce5-99d2-4c01-9442-4487a3983ba2

📥 Commits

Reviewing files that changed from the base of the PR and between f7e08c0 and dab8d33.

📒 Files selected for processing (4)

• Packages/macOS/CmuxTerminal/Sources/CmuxTerminal/Surface/TerminalSurface+PortalLease.swift
• Packages/macOS/CmuxTerminalCore/Sources/CmuxTerminalCore/SurfaceValues/PortalHostLeasePolicy.swift
• Sources/Panels/BrowserPanel.swift
• cmuxTests/DockPortalReconcileTests.swift

[coderabbitai]

♻️ Duplicate comments (1)

Sources/Panels/BrowserPanel.swift (1)

3452-3458: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Clear superseded pending replacement state after every successful ownership change.

A non-forced replacement currently leaves pendingDistinctPortalHostReplacementPaneId intact if it doesn't match the pending pane ID. If ownership later returns to that pending pane, the stale value can authorize an unrelated host. Clear the pending transition whenever an ownership change succeeds, then set the lock only when that replacement consumed the pending request.

• Sources/Panels/BrowserPanel.swift#L3452-L3458: Clear the pending state unconditionally before checking consumesPendingDistinctReplacement on the replacement path.
• Sources/Panels/BrowserPanel.swift#L3500-L3504: Clear the pending state unconditionally on the vacant-lease claim path as well.

This is a regression of a previously flagged issue that was introduced during the policy refactor. As per path instructions, portal ownership must remain derived from one authoritative, structured source without stale fallback state.

Proposed fixes

Sources/Panels/BrowserPanel.swift#L3452-L3458:

             if shouldReplace {
-                if consumesPendingDistinctReplacement {
-                    pendingDistinctPortalHostReplacementPaneId = nil
-                    lockedPortalHost = PortalHostLock(hostId: hostId, paneId: paneId.id)
-                } else if lockedPortalHost?.hostId == current.hostId &&
-                    lockedPortalHost?.paneId == current.paneId {
-                    lockedPortalHost = nil
-                }
+                pendingDistinctPortalHostReplacementPaneId = nil
+                if consumesPendingDistinctReplacement {
+                    lockedPortalHost = PortalHostLock(hostId: hostId, paneId: paneId.id)
+                } else if lockedPortalHost?.hostId == current.hostId &&
+                    lockedPortalHost?.paneId == current.paneId {
+                    lockedPortalHost = nil
+                }

Sources/Panels/BrowserPanel.swift#L3500-L3504:

-        if consumesPendingDistinctReplacement {
-            pendingDistinctPortalHostReplacementPaneId = nil
-            lockedPortalHost = PortalHostLock(hostId: hostId, paneId: paneId.id)
-        }
+        pendingDistinctPortalHostReplacementPaneId = nil
+        if consumesPendingDistinctReplacement {
+            lockedPortalHost = PortalHostLock(hostId: hostId, paneId: paneId.id)
+        }
         activePortalHostLease = next

🤖 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/Panels/BrowserPanel.swift` around lines 3452 - 3458, In the
ownership-change logic around the replacement path at
Sources/Panels/BrowserPanel.swift lines 3452-3458, clear
pendingDistinctPortalHostReplacementPaneId unconditionally before evaluating
consumesPendingDistinctReplacement, then set lockedPortalHost only when that
replacement consumed the pending request. Apply the same unconditional
pending-state clear on the vacant-lease claim path at
Sources/Panels/BrowserPanel.swift lines 3500-3504; retain the existing ownership
and lock behavior otherwise.

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.

Duplicate comments:
In `@Sources/Panels/BrowserPanel.swift`:
- Around line 3452-3458: In the ownership-change logic around the replacement
path at Sources/Panels/BrowserPanel.swift lines 3452-3458, clear
pendingDistinctPortalHostReplacementPaneId unconditionally before evaluating
consumesPendingDistinctReplacement, then set lockedPortalHost only when that
replacement consumed the pending request. Apply the same unconditional
pending-state clear on the vacant-lease claim path at
Sources/Panels/BrowserPanel.swift lines 3500-3504; retain the existing ownership
and lock behavior otherwise.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 57a0ed0f-979e-4146-8a96-574cfb48f936

📥 Commits

Reviewing files that changed from the base of the PR and between dab8d33 and e07fdc6.

📒 Files selected for processing (2)

• Sources/Panels/BrowserPanel.swift
• cmuxTests/DockPortalReconcileTests.swift