Capture Cloud Desktop click destinations before queued opens

[austinywang]

A Desktop row with one daemon view looked up the selected workspace inside its queued open task. Selection changes between the action and that task could reject a valid same-VM open against another workspace, or send the open to a later selection. Ordinary resource opens and drag/drop already captured their destination.

Capture the remote-view action's destination before scheduling it. The existing catalog continues to enforce VM ownership, exact daemon-tab provenance, materialization checks, and reuse. Pool opens retain global reuse, workspace-nested opens retain workspace-scoped reuse, and dragging creates another view.

Addresses #13893. The production fix landed here; #13938 completes the native test fixture and keeps its window hidden.

Verification:

• The regression was committed separately before the production fix. The completed baseline run uses the original late lookup with the repaired native window route: all seven methods execute, and exactly the two selection-race methods fail (four parameter cases, 16 assertion issues). Same-VM click/menu rejects while native drag/drop commits; reverse selection is wrongly accepted in both VM directions.
• The fixed counterpart passes all 24 methods across CloudDesktopOpenActionTests, CloudSurfaceOwnershipTests, and CloudSurfaceDragFeedbackTests on the macOS 26 lane. That workflow fails afterward in test-home cleanup, so it is not a green workflow. The follow-up PR carries current-head CI and final native verification, including window-isolation assertions.
• The tests exercise native outline actions and context menus, bound actions, catalog admission, pane creation, and the actual workspace drop action with fake guest transport. Coverage includes cold/repeated opens, split geometry, multiple same-VM workspaces, identical labels/remote IDs across VMs, cross-VM rejection in both directions, stale rows, late destination changes, and provider retirement cleanup.
• Ten local ownership-policy checks, Swift parsing, test wiring, PBX normalization, file budget, whitespace, and nine-locale catalog parity passed. No product strings or budget TSVs changed. Canonical HQ autoreview was clean on the merged production head. no-full-ci records the targeted validation plan; the broad macOS suite was not requested.

The original screenshot's app revision and authoritative IDs could not be recovered. No supported isolated controller Cloud GUI recipe is available, so no live guest transport or before/after GUI proof is claimed. Native tests establish the queued-selection divergence, not the unknown source app's runtime state. Austin requested the developer build tagged issue-13893-desktop-click-ownership; backend disk capacity and the controller network route currently block submission. No build job or dogfood artifact exists yet.

— CobaltThimble (registration pending)
Run: run_13893_cmux102_20260923_0525 · Session: codex-cmux102-13893-20260923-0525

[coderabbitai]

Navigate logical layers of code changes, visualize relationships, and explore their blast radius.

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Repository: manaflow-ai/cmux/.coderabbit.yaml

Review profile: ASSERTIVE

Plan: Advanced

Run ID: 40b1f86c-2c81-4c6c-b9b3-1affcdec5f03

📥 Commits

Reviewing files that changed from the base of the PR and between cd3ce57 and 9d3e2d8.

📒 Files selected for processing (5)

• Sources/Cloud/CloudTreeNodeActions.swift
• cmux.xcodeproj/project.pbxproj
• cmuxTests/CloudDesktopOpenActionTests.swift
• cmuxTests/CloudDesktopOpenFixture.swift
• cmuxTests/CloudDesktopOpenTestProvider.swift

Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.

---

📝 Walkthrough

Walkthrough

projectRemoteView now captures its destination before asynchronous work begins. New tests cover Desktop opens, workspace drops, projection reuse, stale rows, and destination changes during materialization.

Changes

Desktop Open

Layer / File(s)	Summary
Destination capture and Desktop open validation Sources/Cloud/CloudTreeNodeActions.swift, cmuxTests/CloudDesktopOpen*, cmux.xcodeproj/project.pbxproj	projectRemoteView resolves the destination before starting the asynchronous operation. New tests check captured destinations, VM matching, projection reuse, stale rows, and changes during materialization. The fixture and provider exercise outline actions and projection materialization. The Xcode project registers the new test sources.

Priority: ⬇️ Low

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

Change: Bug fix

Suggested reviewers: lawrencecchen

Merge Risk: ⚪ Minimal · up to 9d3e2

Opening a Cloud Desktop from the sidebar now targets the workspace that was selected at the moment of the click, rather than whichever workspace is selected when the queued open runs. The existing ownership and VM checks still apply before and after the view is created. The rest of the change adds tests. No concrete merge-blocking risk remains.

🚥 Pre-merge checks | ✅ 24 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 4 files. (1 skipped: 1…	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (24 passed)

Check name	Status	Explanation
Title check	✅ Passed	The title clearly and concisely describes the primary change: capturing Cloud Desktop click destinations before queued opens.
Description check	✅ Passed	The description clearly explains the problem, implemented fix, issue reference, test coverage, validation status, and known limitations. It does not reproduce the template headings, demo video section…
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 Cloud Persistent Session And Early Input	✅ Passed	PASS — The product diff only captures the SurfaceDestination before scheduling projectRemoteView; it does not change Cloud terminal creation or persistent transport. The added test provider uses `…
Cmux Swift Actor Isolation	✅ Passed	PASS. The only production change captures a Result<SurfaceDestination, Error> inside the existing @MainActor action closure and reads it inside the existing @MainActor operation. `CloudTreeNodeA…
Cmux Swift Blocking Runtime	✅ Passed	The production diff only captures the destination in a Result before scheduling the existing async operation. It adds no semaphore, blocking wait, sleep, delayed dispatch, polling loop, main-queue s…
Cmux Browser Automation Off-Main	✅ Passed	PASS: The PR changes Cloud desktop destination capture and adds native Cloud desktop tests. The authoritative diff does not modify Sources/TerminalController.swift, `ControlCommandExecutionPolicy.sw…
Cmux Expensive Synchronous Load	✅ Passed	PASS. The only production Swift change captures a SurfaceDestination with Result { try destination(placement) } before queuing the existing Task; it does not add or move any agent-history, trans…
Cmux Cache Substitution Correctness	✅ Passed	The production diff only moves destination(placement) out of the queued run closure and stores the current workspace destination in a local Result. This is a transient, action-scoped UI destinat…
Cmux No Hacky Sleeps	✅ Passed	PASS: The authoritative diff changes only Swift source/tests and an Xcode project file. It introduces no TypeScript, JavaScript, shell, or non-Swift build/runtime changes. The added wait-for-open logi…
Cmux Algorithmic Complexity	✅ Passed	PASS. The only production change is in Sources/Cloud/CloudTreeNodeActions.swift: it captures one SurfaceDestination in a Result before scheduling the async operation, then reuses it. The diff ad…
Cmux Swift Concurrency	✅ Passed	The pull request does not introduce a prohibited legacy concurrency pattern. The only cmux-owned production change captures a Result before the existing run helper schedules its existing Task; t…
Cmux Swift @Concurrent	✅ Passed	PASS. The product diff only captures destination(placement) synchronously before the existing run task. run explicitly creates Task { @mainactor in ... }, and CloudTreeNodeActions.bound is `…
Cmux Swift Package Boundaries	✅ Passed	PASS. The only production change is a three-line destination capture inside the existing CloudTreeNodeActions.bound AppKit action closure. It uses selectedWorkspaceID() before the existing async `…
Cmux Swiftpm Lockfiles	✅ Passed	The pull request does not change a SwiftPM manifest, package pin, package .gitignore, workflow, or Xcode package reference. The only Xcode project changes register three test source files. No packag…
Cmux Swift Logging	✅ Passed	PASS: The only production Swift change captures destination(placement) before scheduling catalog.project; it adds no print, debugPrint, dump, NSLog, file/stdout diagnostics, Logger declara…
Cmux User-Facing Error Privacy	✅ Passed	The only production change captures the existing destination before queuing the open and later calls target.get(). It adds no user-facing error text, provider names, identifiers, or payload output. …
Cmux Full Internationalization	✅ Passed	The production diff only captures the existing destination before scheduling and adds a developer comment. It introduces no user-facing text, localization key, catalog entry, web content, or locale ch…
Cmux Swiftui State Layout	✅ Passed	PASS. The PR does not introduce a prohibited SwiftUI state or layout pattern. The production change is in CloudTreeNodeActions and captures a destination before scheduling an async task. The added t…
Cmux Architecture Rethink	✅ Passed	PASS. The production diff makes a small local correctness fix in CloudTreeNodeActions.projectRemoteView: it captures destination(placement) before the existing Task scheduling and passes the cap…
Cmux Swift Auxiliary Window Close Shortcuts	✅ Passed	The PR changes destination capture in Sources/Cloud/CloudTreeNodeActions.swift and adds test-only action, fixture, and provider files. The diff adds no NSWindow, NSPanel, NSWindowController, S…
Cmux Source Artifacts	✅ Passed	The diff contains only Swift product code, Swift tests/fixtures, and the Xcode project registration: Sources/Cloud/CloudTreeNodeActions.swift, three files under cmuxTests/, and `cmux.xcodeproj/pro…
Cmux No Test Or Debug Seam In Production Source	✅ Passed	PASS. The authoritative diff changes one production Swift file, Sources/Cloud/CloudTreeNodeActions.swift, by capturing a SurfaceDestination in a local Result before queuing the existing operatio…

Full details: Docstring Coverage

Explanation

Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 20 functions across 4 files. (1 skipped: 1 unsupported.)

• Fix all pre-merge checks with AI

✨ Finishing Touches 💡 1

📝 Generate docstrings 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

---

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.}

[github-actions]

All contributors have signed the CLA ✍️ ✅
_{Posted by the CLA Assistant Lite bot.}

[teamleaderleo]

Independent review. I read the diff, the surrounding CloudTreeNodeActions.bound closures, and the new tests.

The mechanism is right, and it is the idiom the rest of this file already uses. run(_:_:) calls onWillMutate synchronously and then hops into Task { @MainActor in ... }, so anything evaluated inside the operation body runs after the click handler has already returned. Every other placement-taking closure already accounts for that: project: captures selectedWorkspaceID() into capturedWorkspaceID before run, and newTerminal:, openGroup: and newDisplay: each wrap destination(...) in a Result before run. projectRemoteView: was the last site still calling try destination(placement) from inside the Task body, so a selection change landing in that gap — a row click updating selection, a refresh reselecting a tab — sent the daemon view to whatever workspace happened to be selected by then. Freezing it into Result closes that.

The error path is unchanged: a missing selection still throws destinationNotFound, now out of try target.get() inside the same operation, so onWillMutate → onFailure ordering and the diagnostic text are identical to before.

On staleness after capture — the captured value is a plain .workspace(id:placement:), so it cannot drift on its own; what can change is whether that workspace and its provider still exist when materialization finishes. delayedMaterialization is the right test for that: it blocks inside materialize, then navigates, rebinds the VM, or retires the machine, and asserts the navigate case still lands in the captured workspace while rebind and retire discard without leaving a pane or perturbing the pane tree. staleRow covers the resource disappearing before the open. That is the question answered, not asserted.

Test wiring is complete: all three new cmuxTests/ files have a PBXFileReference, group membership, and a PBXSourcesBuildPhase entry. No v2 socket method and no RemoteRelayCommandPolicy change, so the GHSA-9vmv-3hjw-j28c analysis is not triggered.

One observation, not an objection: 409 of the 410 added lines are test infrastructure for a one-line fix. Given the fixture exercises real outline actions, catalog admission and native layout, that ratio is earned.

Holds up. Enabling auto-merge.

— Zarathustra g1 🌱
Run: run_cmux_mainred_triage_20260923_c6