Repository navigation
Extract sidebar drag state machine from ContentView into CmuxSidebarDragCoordinator - #6172
Closed
azooz2003-bit wants to merge 1 commit into
Closed
azooz2003-bit wants to merge 1 commit into
azooz2003-bit wants to merge 1 commit into
Conversation
Moves the sidebar drag/drop state machine out of ContentView.swift into a new domain package, CmuxSidebarDragCoordinator (depends only on CmuxFoundation): - SidebarDragState: @mainactor @observable Coordinator (begin/clear drag, drop-indicator state). Inverts its former static reach into the cross-window registry via a constructor-injected `any SidebarWorkspaceDragRegistering`. - SidebarWorkspaceDragRegistry: the process-wide cross-window drag identity, converted from a caseless static-namespace enum into a real @mainactor class behind a protocol seam, owned by AppDelegate (composition root) and injected. - SidebarDragStateRegistry (#if DEBUG): the windowId->SidebarDragState map used by the debug.sidebar.simulate_drag profiling handler, likewise converted from a static-namespace enum to an AppDelegate-owned instance. Seams inverted: the former `SidebarWorkspaceDragRegistry.currentWorkspaceId` reads in SidebarTabDropDelegate now go through the injected coordinator (`dragState.currentWorkspaceDragId`); the DEBUG registry calls in ContentView/TerminalController forward to `AppDelegate.shared?.sidebar*Registry`. An app-side `SidebarDragState.convenience init()` keeps the `@State` call site byte-identical by injecting AppDelegate's shared registry. Byte-identical: no Defaults keys, NotificationCenter names, wire formats, or logic changed; only declaration site + injection of an already-process-wide value. The pure drop predicates/planner (CmuxFoundation) and autoscroll service (CmuxAppKitSupportUI) were already extracted by prior waves; the entangled SwiftUI SidebarTabDropDelegate (welded to TabManager/AppDelegate/@binding) is left in the app target intentionally. Adds behavior unit tests with a fake registry seam (6 tests). New package wired into cmux.xcodeproj (6 pbxproj entries mirroring CmuxSidebarGit) and added as an app-target dependency. Budget ratcheted: ContentView.swift -112 lines. NOTE: app build validated by CI (not run locally per refactor protocol). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughA new local Swift package ChangesCmuxSidebarDragCoordinator Package Extraction and App Wiring
Sequence Diagram(s)sequenceDiagram
participant Sidebar as VerticalTabsSidebar (ContentView)
participant State as SidebarDragState
participant AppRegistry as SidebarWorkspaceDragRegistry (AppDelegate)
participant DropDelegate as SidebarTabDropDelegate
Sidebar->>State: SidebarDragState() [convenience init]
State->>AppRegistry: injects AppDelegate.shared?.sidebarWorkspaceDragRegistry
Sidebar->>State: beginDragging(tabId:)
State->>AppRegistry: begin(workspaceId: tabId)
DropDelegate->>State: currentWorkspaceDragId
State->>AppRegistry: currentWorkspaceId
AppRegistry-->>State: UUID?
State-->>DropDelegate: UUID? (used as effectiveDraggedTabId)
Sidebar->>State: clearDrag()
alt originatedActiveDrag == true
State->>AppRegistry: end(workspaceId: tabId)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (19 passed)
✨ 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 |
Contributor
Greptile SummaryThis PR extracts the sidebar drag/drop state machine from
Confidence Score: 4/5Safe to merge; the extraction is clean, all semantics are preserved, and the only concern is a silent fallback path in the convenience init that isn't expected to fire in production. The refactor is well-scoped and correct: actor isolation, the originating-vs-mirrored-foreign guard, and the stale-clear no-op are all preserved. The one gap is the convenience The app-side Important Files Changed
Sequence DiagramsequenceDiagram
participant AD as AppDelegate (composition root)
participant CV as ContentView (@State dragState)
participant SDS as SidebarDragState
participant SWDR as SidebarWorkspaceDragRegistry
participant SDD as SidebarTabDropDelegate
AD->>AD: init sidebarWorkspaceDragRegistry
AD->>CV: create window / ContentView
CV->>SDS: SidebarDragState() [convenience init]
SDS->>AD: AppDelegate.shared?.sidebarWorkspaceDragRegistry
AD-->>SDS: shared SidebarWorkspaceDragRegistry instance
SDS->>SDS: store workspaceDragRegistry
Note over CV,SDS: Drag begins in originating window
CV->>SDS: beginDragging(tabId:)
SDS->>SWDR: begin(workspaceId:)
SWDR->>SWDR: "activeWorkspaceId = tabId"
Note over SDD: Drop in destination window
SDD->>SDS: dragState.currentWorkspaceDragId
SDS->>SWDR: currentWorkspaceId
SWDR-->>SDS: tabId (cross-window identity)
SDS-->>SDD: foreignId resolved
Note over CV,SDS: Drag ends
CV->>SDS: clearDrag()
SDS->>SWDR: end(workspaceId:) [only if originated]
SWDR->>SWDR: "activeWorkspaceId = nil"
Reviews (1): Last reviewed commit: "Extract sidebar drag state machine into ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+10072
to
10077
| convenience init() { | ||
| self.init( | ||
| workspaceDragRegistry: AppDelegate.shared?.sidebarWorkspaceDragRegistry | ||
| ?? SidebarWorkspaceDragRegistry() | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
init() falls back to a brand-new SidebarWorkspaceDragRegistry() when AppDelegate.shared is nil. That orphaned instance is not the same object stored in AppDelegate.sidebarWorkspaceDragRegistry, so any SidebarDragState created through this path would have cross-window drag detection silently broken: currentWorkspaceDragId would always return nil when a drag originates in another window. The original static enum never had this failure mode. Adding an assertionFailure in DEBUG guards against this regressing silently in future test harnesses or early-lifecycle code paths.
Suggested change
| convenience init() { | |
| self.init( | |
| workspaceDragRegistry: AppDelegate.shared?.sidebarWorkspaceDragRegistry | |
| ?? SidebarWorkspaceDragRegistry() | |
| ) | |
| } | |
| convenience init() { | |
| let registry: any SidebarWorkspaceDragRegistering | |
| if let shared = AppDelegate.shared { | |
| registry = shared.sidebarWorkspaceDragRegistry | |
| } else { | |
| assertionFailure("SidebarDragState() called before AppDelegate is available; cross-window drag detection will be broken for this instance") | |
| registry = SidebarWorkspaceDragRegistry() | |
| } | |
| self.init(workspaceDragRegistry: registry) | |
| } |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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/ContentView.swift`:
- Around line 10068-10077: The convenience initializer for SidebarDragState
silently falls back to creating an isolated SidebarWorkspaceDragRegistry when
AppDelegate.shared is unavailable, which masks cross-window drag coordination
bugs rather than surfacing the unexpected state. Since the code comment asserts
this should never happen after a sidebar mounts, add a defensive assertion
before the fallback to catch any unexpected early initialization scenarios and
alert developers to this problematic condition, while keeping the fallback for
safety.
🪄 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: 8e90e795-2645-4a35-9903-b6adeac777cf
⛔ Files ignored due to path filters (1)
.github/swift-file-length-budget.tsvis excluded by!**/*.tsv
📒 Files selected for processing (10)
Packages/CmuxSidebarDragCoordinator/Package.swiftPackages/CmuxSidebarDragCoordinator/Sources/CmuxSidebarDragCoordinator/SidebarDragState.swiftPackages/CmuxSidebarDragCoordinator/Sources/CmuxSidebarDragCoordinator/SidebarDragStateRegistry.swiftPackages/CmuxSidebarDragCoordinator/Sources/CmuxSidebarDragCoordinator/SidebarWorkspaceDragRegistry.swiftPackages/CmuxSidebarDragCoordinator/Tests/CmuxSidebarDragCoordinatorTests/SidebarDragStateTests.swiftSources/AppDelegate.swiftSources/ContentView.swiftSources/SidebarWorkspaceGroupHeaderView.swiftSources/TerminalController.swiftcmux.xcodeproj/project.pbxproj
Comment on lines
+10068
to
10077
| extension SidebarDragState { | ||
| /// Builds a drag state wired to the app's process-wide cross-window drag | ||
| /// registry. Falls back to a fresh registry only if `AppDelegate.shared` is | ||
| /// not yet available (never the case once a sidebar has mounted). | ||
| convenience init() { | ||
| self.init( | ||
| workspaceDragRegistry: AppDelegate.shared?.sidebarWorkspaceDragRegistry | ||
| ?? SidebarWorkspaceDragRegistry() | ||
| ) | ||
| } |
There was a problem hiding this comment.
Silent fallback to isolated registry could mask cross-window drag bugs.
The fallback ?? SidebarWorkspaceDragRegistry() creates an isolated registry if AppDelegate.shared is unavailable. This silently breaks cross-window drag coordination rather than surfacing the unexpected state. Since the comment claims this is "never the case once a sidebar has mounted," consider an assertion to catch any unexpected early initialization:
Proposed defensive assertion
extension SidebarDragState {
/// Builds a drag state wired to the app's process-wide cross-window drag
/// registry. Falls back to a fresh registry only if `AppDelegate.shared` is
/// not yet available (never the case once a sidebar has mounted).
convenience init() {
+ let registry = AppDelegate.shared?.sidebarWorkspaceDragRegistry
+ `#if` DEBUG
+ if registry == nil {
+ assertionFailure("SidebarDragState initialized before AppDelegate.shared available")
+ }
+ `#endif`
self.init(
- workspaceDragRegistry: AppDelegate.shared?.sidebarWorkspaceDragRegistry
- ?? SidebarWorkspaceDragRegistry()
+ workspaceDragRegistry: registry ?? SidebarWorkspaceDragRegistry()
)
}
}🤖 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/ContentView.swift` around lines 10068 - 10077, The convenience
initializer for SidebarDragState silently falls back to creating an isolated
SidebarWorkspaceDragRegistry when AppDelegate.shared is unavailable, which masks
cross-window drag coordination bugs rather than surfacing the unexpected state.
Since the code comment asserts this should never happen after a sidebar mounts,
add a defensive assertion before the fallback to catch any unexpected early
initialization scenarios and alert developers to this problematic condition,
while keeping the fallback for safety.
Collaborator
Author
|
Superseded by #6226 — consolidated into the existing CmuxSidebar package (no new micro-package) per over-engineering review. The extraction is preserved there. |
This branch was successfully deployed
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.
What moved
The sidebar drag/drop state machine moves out of the
ContentView.swiftgod file into a new domain packagePackages/CmuxSidebarDragCoordinator(depends only onCmuxFoundation):SidebarDragState—@MainActor @ObservableCoordinator owning transient drag state (draggedTabId,dropIndicator,isSimulated,foreignDraggedIsPinned;beginDragging/setDropIndicator/clearDrag). Its former static reach into the cross-window registry is inverted via a constructor-injectedany SidebarWorkspaceDragRegistering.SidebarWorkspaceDragRegistry— the process-wide cross-window dragged-workspace identity, converted from a caseless static-namespaceenuminto a real@MainActor final classbehind a protocol seam, owned byAppDelegate(the composition root) and injected.SidebarDragStateRegistry(#if DEBUG) — thewindowId -> SidebarDragStatemap read by thedebug.sidebar.simulate_dragprofiling handler, likewise converted from a static-namespaceenumto anAppDelegate-owned instance.Seams inverted
SidebarTabDropDelegate's formerSidebarWorkspaceDragRegistry.currentWorkspaceIdreads now go through the injected coordinator (dragState.currentWorkspaceDragId).SidebarDragStateRegistry.{register,unregister,state}calls inContentView/TerminalControllerforward toAppDelegate.shared?.sidebar*Registry.SidebarDragState.convenience init()keeps the@State var dragState = SidebarDragState()call site byte-identical by injectingAppDelegate's shared registry (fresh-registry fallback only ifAppDelegate.sharedis nil, never the case once a sidebar has mounted).Byte-identical notes
No
Defaultskeys,NotificationCenternames, wire formats, or logic changed. The only change is the declaration site of these types plus injection of an already-process-wide value (a singleton in all but name).clearDrag's originated-vs-foreign guard, theend()stale-clear no-op, and the simulate-drag flow are preserved exactly.Scope decision
The pure drop predicates/planner (
SidebarTabDropIndicatorPredicate,SidebarDropPlanner,SidebarDropIndicator,SidebarWorkspaceSelectionSyncPolicyinCmuxFoundation) and the autoscroll service (SidebarDragAutoScroll*inCmuxAppKitSupportUI) were already extracted by prior refactor waves, so this PR takes the remaining clean subset: the state machine + cross-window identity. The SwiftUISidebarTabDropDelegateis aDropDelegatewelded toTabManager,AppDelegate.sharedcross-window methods,@Bindings, andDropInfo; moving it would require inverting ~10 collaborator methods at high behavior-drift risk, so it is intentionally left in the app target (it now consumes the package types).Tests
6 behavior unit tests with a fake
SidebarWorkspaceDragRegisteringseam covering begin/clear, originating-vs-mirrored-foreign clear semantics, the top-level indicator flag, and the registry stale-clear no-op.swift build+swift testpass in the package.Verification
scripts/lint-ios-package-conventions.sh: OK, zero newlint:allow.Packages/CmuxSidebarDragCoordinatorswift build+swift test: pass..github/swift-file-length-budget.tsvreconciled (ContentView.swift ratcheted down ~112 lines).cmux.xcodeproj(6 pbxproj entries mirroringCmuxSidebarGit) and added as an app-target dependency.Est. lines removed from the ContentView giant: ~112.
NOTE: the full app build + XCUITests are validated by GitHub CI; per refactor protocol they were not run locally.
🤖 Generated with Claude Code
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Extracted the sidebar drag/drop state machine into a new
CmuxSidebarDragCoordinatorpackage and replaced static globals with injected registries for better modularity and cross-window drag handling. Behavior is unchanged.Refactors
SidebarDragState,SidebarWorkspaceDragRegistry, and DEBUGSidebarDragStateRegistrytoCmuxSidebarDragCoordinator.SidebarWorkspaceDragRegisteringandSidebarWorkspaceDragRegistry(@MainActor); owned byAppDelegateand injected.SidebarTabDropDelegatenow reads cross-window drag viadragState.currentWorkspaceDragId.SidebarDragStateconvenience init to keep@State var dragState = SidebarDragState()call sites unchanged.ContentView.swiftby ~112 lines.Dependencies
Packages/CmuxSidebarDragCoordinator(depends onCmuxFoundation) and wired it intocmux.xcodeproj.swift buildandswift testpass.Written for commit 8f2fe27. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Tests