Repository navigation
refactor: move the surface catalog's value types into a package - #13119
Closed
teamleaderleo wants to merge 1 commit into
Closed
teamleaderleo wants to merge 1 commit into
teamleaderleo wants to merge 1 commit into
Conversation
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 and CMUXDebugLog 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. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Warning Review limit reachedNext included review available in 15 minutes. View limit detailsLimit details: You’ve used all 10 included reviews currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Review configuration: ⚙️ Run configurationConfiguration used: Repository: manaflow-ai/cmux/.coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (233)
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 ✍️ ✅ |
Contributor
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @@ -1,3 +1,4 @@ | |||
| import CmuxSurfaceCatalogModel | |||
Contributor
There was a problem hiding this comment.
This file now imports CmuxSurfaceCatalogModel, but the cmuxUITests target has neither a package product dependency nor a framework entry for that module. Building the UI-test target therefore fails with no such module 'CmuxSurfaceCatalogModel'. Link the package to cmuxUITests, or remove the unused import.
| /// Relationship changes can move many resources at once. These use the authoritative full | ||
| /// rebuild path instead of risking a partial placement update. | ||
| var requiresFullResourceRebuild = false | ||
| public var requiresFullResourceRebuild = false |
Contributor
There was a problem hiding this comment.
Moving these helpers into the package exposes CmuxTuiSnapshotParser as a public, static-only namespace. CloudWireNumber in SurfaceCatalogModel.swift is likewise exposed as a public enum containing only static conversion functions. This violates the repository directive against widening static helper namespaces merely to cross a module boundary; the behavior must instead belong to an appropriately scoped, constructable type. This repository requirement must be satisfied before merging.
Rule Used: Flag new ambient global state in production Swift: a top-level (file-scope) func used as API, a top-level mutable var or a stub class/struct holding a global flag/once-token, a caseless enum/empty struct used purely as a static func/static let namesp... (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!
Collaborator
Author
|
Continued in #13135 (in-org branch). |
This was referenced Sep 20, 2026
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
First cut from #13108. The surface catalog's value types (
SurfaceMachineID,SurfaceResourceID,SurfaceResource,SurfaceMachineInfo,SurfaceProjection, theCloudVM*state documents,VMMachineKind, and the cmux-tui snapshot parser that produces them) move from the app target into a new local package,Packages/macOS/CmuxSurfaceCatalogModel. 56 types, about 4,000 lines, depending only onCmuxCoreandCMUXDebugLog.SurfaceCatalogitself stays in the app.Why this one: these types are what holds
Sources/Cloud/(26% of all app-module edits in the last 30 days) inside the app module. About 630 of Cloud's ~1,100 references to the rest of the app are to these types. With them in a package, Cloud and Surfaces share a vocabulary that is not the app module.What changed besides the move:
public. The 27 structs that relied on the synthesized memberwise initializer get an explicitpublic initwith the same parameters and defaults; structs that already had their own initializer are untouched.CloudVMState+SnapshotComparison.swiftmoves too: it holdsCloudVMState's==.SurfaceCatalog+CloudPorts.swift) andSurfaceResourceID.portKey/desktopDisplayKey(fromSurfaceSocketCommands.swift) move intoSurfaceResourceID+Ports.swift, because the moved parser calls them. The rest of those two files stays.cmuxDebugLog(...)in the parser becomesCMUXDebugLog.logDebugEvent(...), still under#if DEBUG.VMMachineKind.swiftwas a member of both the app and thecmux-clitarget, so the CLI links the package too.import CmuxSurfaceCatalogModel. The package is added to CI's package test list.scripts/check-pbxproj.shpasses.No logic changes.
Testing
Measured on one machine (MacBook Air M5, Xcode 27.0), warm incremental
xcodebuildbuilds, the same edits toSurfaceCatalogModel.swiftwith the file in the app module (a second worktree on a slightly oldermain) and in the package (this branch). Compare within a row.SurfaceResource, sample 1main:xcodebuild -scheme cmux build(builds the CLI too):BUILD SUCCEEDED.xcodebuild -scheme cmux-unit build-for-testing:TEST BUILD SUCCEEDED(run with test: split an expression Xcode 27 cannot type-check #13073 applied locally, becausemain's tests do not type-check on Xcode 27 without it; not part of this PR).swift test --package-path Packages/macOS/CmuxSurfaceCatalogModel: passes (one new smoke test; the existing coverage of these types stays incmuxTests).CloudVMStateDeltaImpact, only constructed by tests); fixed here.Demo Video
Not applicable: code move, no behaviour change.
Checklist
🤖 Generated with Claude Code
Summary by cubic
First cut from #13108: extracts the surface catalog's value types into a new local package,
CmuxSurfaceCatalogModel, so Cloud and Surfaces share a vocabulary without depending on the app module. No logic changes;SurfaceCatalogitself stays in the app.What changed
CmuxCoreandCMUXDebugLog.public, adding explicitpublic inits where structs relied on the synthesized memberwise initializer.CloudVMState's==, the parser's port helpers, andSurfaceResourceID.portKey/desktopDisplayKeywith the parser that uses them.cmuxDebugLog(...)withCMUXDebugLog.logDebugEvent(...).VMMachineKindis shared with thecmux-clitarget.import CmuxSurfaceCatalogModelto 215 app, CLI, and test files.Verification
Written for commit 0c26fea. Summary will update on new commits.