Consolidate workspace extractions into CmuxWorkspaces (no new packages) - #6222
Conversation
… new package) Folds the per-workspace surface-list derivation from feat-workspace-surface-list-model into the EXISTING CmuxWorkspaces domain package instead of standing up a new top-level CmuxWorkspaceSurfaceList package. The owner rejected the per-sliver micro-packages; the extraction is good, so it lives in the workspace domain package. What moved (byte-identical logic): - WorkspaceSurfaceListModel (the @mainactor @observable derivation model: orderedPanelIds, focusedPanelId, representativePanelIdForWorkspaceManualUnread, effectiveSelectedPanelId, the tabIdsTo* pane queries, and the paneLayoutVersion reorder bump) and its WorkspaceSurfaceTreeReading seam protocol now live in Packages/CmuxWorkspaces/Sources/CmuxWorkspaces/SurfaceList/. Both files are byte-identical to the member branch; they only import Foundation/Observation, so CmuxWorkspaces's Package.swift needs no new dependency. - The 12 behavior tests move into CmuxWorkspacesTests (only the @testable import target changed from CmuxWorkspaceSurfaceList to CmuxWorkspaces). App-side seam unchanged in shape: Workspace+WorkspaceSurfaceTreeReading.swift conforms Workspace to the seam and Workspace holds the model (weak back-ref via the seam to avoid a retain cycle), with the legacy accessors as one-line forwards. Every `import CmuxWorkspaceSurfaceList` became `import CmuxWorkspaces` (removed in Workspace.swift, which already imports CmuxWorkspaces). No new top-level package: Packages/CmuxWorkspaceSurfaceList/ is deleted and its 6 pbxproj package-reference entries are removed. The pbxproj only gains the 4 source-file wiring entries for Workspace+WorkspaceSurfaceTreeReading.swift. Verified: scripts/lint-ios-package-conventions.sh adds zero new violations (the one pre-existing namespace-enum ERROR in CmuxMobileShellUI is untouched debt on main); swift build + swift test green in Packages/CmuxWorkspaces (32 tests, 4 suites). Budget for Sources/Workspace.swift ratcheted to 12978. Supersedes the standalone CmuxWorkspaceSurfaceList package PR from feat-workspace-surface-list-model. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
📝 WalkthroughWalkthroughIntroduces ChangesWorkspaceSurfaceListModel extraction
Sequence Diagram(s)sequenceDiagram
participant Workspace
participant WorkspaceSurfaceListModel
participant WorkspaceSurfaceTreeReading
Note over Workspace: init
Workspace->>WorkspaceSurfaceListModel: attach(tree: self)
Note over Workspace: query orderedPanelIds
Workspace->>WorkspaceSurfaceListModel: orderedPanelIds
WorkspaceSurfaceListModel->>WorkspaceSurfaceTreeReading: surfaceIdsInTabOrderAcrossAllPanes
WorkspaceSurfaceListModel->>WorkspaceSurfaceTreeReading: panelId(forSurfaceId:), panelExists(_:), allPanelIds
WorkspaceSurfaceListModel-->>Workspace: deduped + orphan-appended [UUID]
Note over Workspace: didChangeGeometry
Workspace->>WorkspaceSurfaceListModel: registerGeometryChange()
WorkspaceSurfaceListModel->>WorkspaceSurfaceTreeReading: lastOrderedPanelIds (read)
alt panel order changed
WorkspaceSurfaceListModel->>WorkspaceSurfaceTreeReading: lastOrderedPanelIds = new (write)
WorkspaceSurfaceListModel->>WorkspaceSurfaceTreeReading: bumpPaneLayoutVersion()
WorkspaceSurfaceListModel-->>Workspace: true
else unchanged
WorkspaceSurfaceListModel-->>Workspace: false
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Poem
🚥 Pre-merge checks | ✅ 20 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (20 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 SummaryConsolidates the workspace surface-list extraction into the existing
Confidence Score: 4/5The consolidation is faithful and well-tested, but two open design questions on this PR — the @observable observation no-op in WorkspaceSurfaceListModel and the compactMap UUID conversion in the seam — should be resolved before merging. The logic extraction is behavior-preserving: the 12 tests cover all derivation paths, the delegation chain in Workspace.swift is straightforward, and the pbxproj delta is minimal. The two concerns already active in the PR thread — that WorkspaceSurfaceListModel's @observable fires no updates (all properties compute from an @ObservationIgnored field), and that spatiallyOrderedPaneIds silently drops panes via compactMap(UUID.init(uuidString:)) — remain unresolved and affect correctness for any callers that bypass the Workspace forwarding wrappers or encounter a UUID format edge case. WorkspaceSurfaceListModel.swift (observation design) and Workspace+WorkspaceSurfaceTreeReading.swift (spatiallyOrderedPaneIds UUID conversion) Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant SwiftUI as SwiftUI View
participant WS as Workspace (ObservableObject)
participant SL as WorkspaceSurfaceListModel (@Observable)
participant Seam as WorkspaceSurfaceTreeReading (protocol)
participant BC as BonsplitController
participant PT as PaneTreeModel
Note over WS,SL: init() — attach(tree: self)
SwiftUI->>WS: orderedPanelIds
WS->>SL: surfaceList.orderedPanelIds
SL->>Seam: surfaceIdsInTabOrderAcrossAllPanes
Seam->>BC: allTabIds.map(\.uuid)
BC-->>Seam: [UUID]
Seam-->>SL: [UUID]
SL->>Seam: panelId(forSurfaceId:) / panelExists(_:)
Seam->>WS: panelIdFromSurfaceId / panels[panelId]
WS-->>Seam: UUID?
SL-->>WS: [UUID]
WS-->>SwiftUI: [UUID]
Note over WS,SL: geometry change callback
WS->>SL: surfaceList.registerGeometryChange()
SL->>SL: compute orderedPanelIds
SL->>Seam: lastOrderedPanelIds (compare)
Seam->>PT: paneTree.lastOrderedPanelIds
PT-->>Seam: [UUID]
alt order changed
SL->>Seam: "lastOrderedPanelIds = currentOrder"
SL->>Seam: bumpPaneLayoutVersion()
Seam->>WS: "paneLayoutVersion &+= 1"
WS-->>SwiftUI: objectWillChange fires
end
%%{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"}}}%%
sequenceDiagram
participant SwiftUI as SwiftUI View
participant WS as Workspace (ObservableObject)
participant SL as WorkspaceSurfaceListModel (@Observable)
participant Seam as WorkspaceSurfaceTreeReading (protocol)
participant BC as BonsplitController
participant PT as PaneTreeModel
Note over WS,SL: init() — attach(tree: self)
SwiftUI->>WS: orderedPanelIds
WS->>SL: surfaceList.orderedPanelIds
SL->>Seam: surfaceIdsInTabOrderAcrossAllPanes
Seam->>BC: allTabIds.map(\.uuid)
BC-->>Seam: [UUID]
Seam-->>SL: [UUID]
SL->>Seam: panelId(forSurfaceId:) / panelExists(_:)
Seam->>WS: panelIdFromSurfaceId / panels[panelId]
WS-->>Seam: UUID?
SL-->>WS: [UUID]
WS-->>SwiftUI: [UUID]
Note over WS,SL: geometry change callback
WS->>SL: surfaceList.registerGeometryChange()
SL->>SL: compute orderedPanelIds
SL->>Seam: lastOrderedPanelIds (compare)
Seam->>PT: paneTree.lastOrderedPanelIds
PT-->>Seam: [UUID]
alt order changed
SL->>Seam: "lastOrderedPanelIds = currentOrder"
SL->>Seam: bumpPaneLayoutVersion()
Seam->>WS: "paneLayoutVersion &+= 1"
WS-->>SwiftUI: objectWillChange fires
end
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| @MainActor | ||
| @Observable | ||
| public final class WorkspaceSurfaceListModel { | ||
| @ObservationIgnored | ||
| private weak var tree: (any WorkspaceSurfaceTreeReading)? |
There was a problem hiding this comment.
@Observable with no tracked storage — observation silently no-ops
WorkspaceSurfaceListModel is @Observable, but its only stored property (tree) is @ObservationIgnored. Every computed property (orderedPanelIds, focusedPanelId, representativePanelIdForWorkspaceManualUnread, etc.) reads exclusively from tree. The @Observable macro generates _$observationRegistrar.access calls only for stored properties that are NOT @ObservationIgnored; computed properties that read only ignored storage register no observation dependency at all.
The consequence: any SwiftUI view body that reads workspace.surfaceList.orderedPanelIds (or any other derived property) via the @Observable machinery registers no dependency, will never be re-invalidated when tree data changes, and silently shows stale data. surfaceList is internal let (not private), so it is accessible to all SwiftUI views in the app target — the missing-update footgun is live.
Current callers route through workspace.orderedPanelIds (the ObservableObject forwarding path), so nothing is broken today. But the @Observable annotation advertises an observability contract the model cannot honour for any of its useful properties. Either drop @Observable here (and document that callers must observe through Workspace), or add a stored cached snapshot that IS tracked and that the registerGeometryChange() path invalidates.
| var spatiallyOrderedPaneIds: [UUID] { | ||
| bonsplitController.treeSnapshot().orderedPaneIds.compactMap(UUID.init(uuidString:)) | ||
| } |
There was a problem hiding this comment.
orderedPaneIds strings don't round-trip through UUID
spatiallyOrderedPaneIds uses compactMap(UUID.init(uuidString:)) to convert bonsplit's treeSnapshot().orderedPaneIds strings. compactMap silently drops any string that fails UUID parsing, whereas the original code keyed its dictionary by paneId.id.uuidString and looked up by the same string type — a mismatch just missed the key, but every pane was iterated. If bonsplit ever emits a pane id string in a format that UUID.init(uuidString:) rejects (e.g., non-standard casing or extra wrapper characters), the representative-panel fallback will silently skip that pane. allPaneIds takes \.id directly from PaneID, so the two UUID sources are structurally different; a precondition or an assertion that the parsed count equals allPaneIds.count would catch any future format divergence early.
ComposerDictationTextMerge (landed in #6197) is a caseless static-member enum with no lint:allow marker, so package-conventions-lint (a required check) has been red on main and on every PR branched off it. It is a genuine stateless pure-function namespace with no instance state to own, so the sanctioned inline lint:allow escape hatch is the correct fix rather than reshaping it into an instantiated type. Unblocks this consolidation PR's required CI; the consolidation diff itself is unchanged. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…space # Conflicts: # .github/swift-file-length-budget.tsv # Packages/CmuxMobileShellUI/Sources/CmuxMobileShellUI/ComposerDictationTextMerge.swift
This branch was successfully deployed
1 participant
Folds the workspace surface-list extraction into the EXISTING
CmuxWorkspacesdomain package instead of standing up a new top-level package. The owner rejected the per-sliver micro-packages; the extraction itself is good, so it now lives in the workspace domain package.Slivers folded
CmuxWorkspaceSurfaceListpackage is gone; its contents live underPackages/CmuxWorkspaces/Sources/CmuxWorkspaces/SurfaceList/.Byte-identical
WorkspaceSurfaceListModel.swift(the@MainActor @Observablederivation:orderedPanelIds,focusedPanelId,representativePanelIdForWorkspaceManualUnread,effectiveSelectedPanelId, thetabIdsTo*pane queries, and thepaneLayoutVersionreorder bump) and itsWorkspaceSurfaceTreeReading.swiftseam protocol are byte-identical to the member branch (verified bydiff). They import only Foundation/Observation, soCmuxWorkspaces/Package.swiftneeds no new dependency. The 12 behavior tests moved intoCmuxWorkspacesTestswith only the@testable importtarget changed (CmuxWorkspaceSurfaceList→CmuxWorkspaces).App-side seam is unchanged in shape:
Workspace+WorkspaceSurfaceTreeReading.swiftconformsWorkspaceto the seam,Workspaceowns the model and is referenced weakly via the seam (no retain cycle), and the legacy accessors stay one-line forwards. Everyimport CmuxWorkspaceSurfaceListbecameimport CmuxWorkspaces(removed inWorkspace.swift, which already importsCmuxWorkspaces).No new packages
Packages/CmuxWorkspaceSurfaceList/is deleted and its 6 pbxproj package-reference entries removed. The pbxproj diff only adds the 4 source-file wiring entries forWorkspace+WorkspaceSurfaceTreeReading.swift.Verification
scripts/lint-ios-package-conventions.sh: zero new violations (one pre-existingnamespace-enumERROR inCmuxMobileShellUI/ComposerDictationTextMerge.swiftis untouched debt already onmain).swift build+swift testgreen inPackages/CmuxWorkspaces: 32 tests across 4 suites pass.Sources/Workspace.swiftratcheted to 12978.xcodebuildintentionally left to CI per task instructions.Supersedes the standalone
CmuxWorkspaceSurfaceListpackage PR fromfeat-workspace-surface-list-model.🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Consolidates the workspace surface-list logic into
CmuxWorkspacesand removesCmuxWorkspaceSurfaceListwith no behavior changes. Also fixes a pre-existing package-conventions lint to unblock CI.Refactors
WorkspaceSurfaceListModelandWorkspaceSurfaceTreeReadingunderPackages/CmuxWorkspaces/Sources/CmuxWorkspaces/SurfaceList/to own ordered panel ids, focused/representative/selected panel,tabIdsTo*, and the layout-version bump.Workspacenow conforms to the seam and forwards legacy accessors; geometry-change handling callssurfaceList.registerGeometryChange().Packages/CmuxWorkspaceSurfaceList/and replacedimport CmuxWorkspaceSurfaceListwithimport CmuxWorkspaces; moved 12 behavior tests toCmuxWorkspacesTests.swift build/swift testpass.Sources/Workspace.swiftfile-length budget set to 12621.Bug Fixes
package-conventions-lintby adding// lint:allow namespace-enumtoComposerDictationTextMerge.swift(fixes a failure inherited frommain).Written for commit faebd3b. Summary will update on new commits.
Summary by CodeRabbit