Repository navigation
refactor: move surface ownership policy into catalog package - #13340
Closed
teamleaderleo wants to merge 3 commits into
Closed
teamleaderleo wants to merge 3 commits into
teamleaderleo wants to merge 3 commits into
Conversation
|
Important Review skippedToo many files! This PR contains 334 files, which is 34 over the limit of 300. To get a review, reduce the PR to 300 files or fewer by splitting it into smaller PRs or changing its base branch. Usage-priced reviews support at most 300 files. ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: ⛔ Files ignored due to path filters (2)
📒 Files selected for processing (334)
You can disable this status message by setting the 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
|
All contributors have signed the CLA ✍️ ✅ |
This was referenced Sep 22, 2026
teamleaderleo
marked this pull request as ready for review
September 23, 2026 03:40
Collaborator
Author
|
Parked — Thornquay 💠 (triage, 2026-09-23). Rides on #13135 — it inherits exactly the same 8-file conflict set and adds nothing of its own (its delta is 4 files / 78 insertions). |
Redone from current main instead of rebasing #13135 (1.8k commits behind, with main-side edits to the moved files and ~100 new consumers). SurfaceMachineID, SurfaceResourceID, SurfaceResource, the CloudVM state documents and the cmux-tui snapshot parser are pure values that the Cloud sidebar, SurfaceCatalog, the socket commands and the CLI all share. They move from the app target into Packages/macOS/CmuxSurfaceCatalogModel (depends on CmuxCore, CMUXDebugLog and CMUXMobileCore only). SurfaceCatalog, the owner, stays in the app. Besides the move: declarations become public, structs that relied on the synthesized memberwise initializer get an explicit public init with the same parameters, cmuxDebugLog becomes CMUXDebugLog.logDebugEvent, and the parser's port helpers and SurfaceResourceID.portKey move out of two larger app files because the parser calls them. VMMachineKind.swift was also a member of the cmux-cli target, so the CLI links the package. Since #13135 was cut, main split SurfaceMachineID, SurfaceDeviceInstanceID and SurfaceDevicePresence out of SurfaceCatalogModel.swift; they move too (hence the CMUXMobileCore dependency, for cmxCanonicalDeviceID). Main's new fields (agent, device workspace detail/unread/pin, machine presence, displayCreationMachines, displayPorts) are public and in the explicit inits. CmuxTuiSnapshotParser.mergingDisplays stays in the app's +Displays extension, where main moved it. 303 app, CLI and test files gain `import CmuxSurfaceCatalogModel`. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
teamleaderleo
force-pushed
the
integration/13276-policy
branch
from
September 24, 2026 07:03
3c33ef0 to
a5753d3
Compare
Collaborator
Author
The iOS/package conventions lint rejects public all-static types in packages. CmuxTuiSnapshotParser, CloudWireNumber and CloudVMEventFeedRecoveryDecision were internal static namespaces in the app and moved unchanged; reshaping them is a separate change, so each carries the lint's inline lint:allow justification. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
SurfaceOwnershipPolicy's pure rule (local accepts anything; a Cloud destination accepts only its own machine and rejects empty or mixed selections) moves into CmuxSurfaceCatalogModel with package tests. The app keeps the SurfaceTransferRejection mapping as an extension. Replayed onto the redone #13135; PaneDropContainer.swift (new on main since the original) gains the package import because it names the policy type. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
teamleaderleo
force-pushed
the
integration/13276-policy
branch
from
September 24, 2026 08:23
a5753d3 to
64f4f78
Compare
teamleaderleo
commented
Sep 24, 2026
teamleaderleo
left a comment
teamleaderleo
left a comment
Collaborator
Author
There was a problem hiding this comment.
Independent review of 64f4f78230: ownership decisions are unchanged. This is #13135's head plus one commit. It should merge after #13135, which leaves this commit's four files and one import as the diff.
Semantics. The old app rejection(for:) overloads and the new allows(...) rules agree on every input:
| Destination | Source / selection | Before | After |
|---|---|---|---|
local (cloudMachine == nil) |
anything, including an empty selection | allowed | allowed |
cloud a |
.cloud("a"), or a non-empty all-a selection |
allowed | allowed |
cloud a |
.local, nil, .cloud("b") |
.cloudMachineMismatch |
.cloudMachineMismatch |
cloud a |
empty selection, or mixed a/b |
.cloudMachineMismatch |
.cloudMachineMismatch |
rejection(for resources:) used to delegate to the machines overload through map(\.machine), and allows(resources:) does the same. The public init(cloudMachine:) has no default, which matches the old implicit memberwise init for a let. The app keeps the SurfaceTransferRejection mapping as an extension, so CloudTreeOutlineView+Organization.swift:80, PaneDropContainer.swift, and the existing cmuxTests/CloudDisplayCatalogTests.swift:152-154 call sites are source-compatible. The added imports in those two app files are needed because both name the type.
CI on 64f4f78230
swift-package-testsran the new suite: "9 tests in 3 suites passed", including all fiveSurface ownership policytests (job).macOS compile admissionwas still queued andcli-pipe-regressionsstill running when I checked. This head has no compile evidence yet, and it needs a green compile admission before merge.
My #13135 review covers the file move this PR inherits.
Collaborator
Author
|
Obsolete: #14390 moved the policy into CmuxCloud. |
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.
Surface ownership decisions (may this selection move into this workspace?) live in the app target, so they can only be tested by building the app, and the pure rule is tangled with localized UI errors. This PR moves
SurfaceOwnershipPolicyintoCmuxSurfaceCatalogModelwith package-level tests; the app keeps theSurfaceTransferRejectionmapping and its localized message as an extension.Depends on #13135 (the catalog package). This branch is #13135's head plus one commit; review that commit. It targets
mainand should land after #13135.Change
SurfaceOwnershipPolicy(the rule: a local destination accepts any source; a Cloud destination accepts only its own machine and rejects empty or mixed selections) moves into the package as a public value withallows(source:),allows(resources:),allows(machines:).Sources/Surfaces/SurfaceOwnershipPolicy.swiftkeeps only the app-facingrejection(for:)mapping.PaneDropContainer.swift(added onmainsince this PR was first opened) gains the package import because it names the policy type.Rebuilt on the redone #13135 head; the commit cherry-picked cleanly apart from that import.
Validation
macOS compile admissionon the pushed head; the existing app-host drag/drop, movement, restore, focus and capture tests are unchanged.Remaining gap
Should not merge before #13135; until then this PR's diff also shows #13135's commit. The app-host suite has not been run on this head.
🤖 Generated with Claude Code