Repository navigation
Make workspace sidebar lazy with @Observable drag state - #4736
Conversation
Switches the left sidebar to LazyVStack for O(visible) layout instead of O(total) workspaces. Previously the stack had to stay eager because drag mutations (draggedTabId, dropIndicator) were @State on the ancestor, and the 60fps writes during drag fed back through the lazy layout cache and pegged the main thread (manaflow-ai#2586). Drag transient state now lives on a @observable @mainactor SidebarDragState class. Reads are tracked per property, so high-frequency writes invalidate only the tiny overlays that depend on them, never the sidebar body or the LazyVStack layout. - Add SidebarDragState (@observable) owning draggedTabId + dropIndicator. - Replace @State draggedTabId/dropIndicator/frozenTabItemPresentation on VerticalTabsSidebar with a single @State SidebarDragState. - Replace the @binding chain through TabItemView, SidebarEmptyArea, and both SidebarTabDropDelegate and SidebarBonsplitTabDropDelegate with a direct SidebarDragState reference. - Extract SidebarTabDropIndicatorOverlay and SidebarTabDragOpacityModifier so the per-frame dropIndicator/draggedTabId reads stay inside isolated views and don't invalidate the 1600-line TabItemView body. - Delete SidebarTabItemPresentationSnapshot and the frozen-presentation workflow (context-menu freeze was a workaround for binding churn that @observable's per-property tracking makes unnecessary). - Flip VStack to LazyVStack inside the sidebar ScrollView.
|
@lawrencecchen is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCentralizes sidebar drag/drop transient state into an ChangesSidebar drag-and-drop centralization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (14 passed)
✨ Finishing Touches🧪 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 SummaryThis PR reapplies the lazy-sidebar refactor (#3039) on current
Confidence Score: 5/5Safe to merge: the refactor correctly preserves all existing behaviors (modifier-hint freeze, drop-indicator placement, adapter bindings for extension sidebar) and adds no new isolation or layout hazards. The showsModifierShortcutHints freeze concern raised in the previous review is fully addressed by SidebarShortcutHintFreezePolicy and its inclusion in TabItemView.== at line 13504. SidebarTabDropDelegate now carries an explicit @mainactor annotation consistent with its @Observable-isolated state mutations. The LazyVStack + equatable + per-property @observable tracking design is internally consistent: only the dragged row and affected indicator overlays invalidate during drag, and off-screen rows are never created. New unit tests cover the pure predicate and policy logic including edge cases. No files require special attention beyond the already-noted ContentView.swift size, which is a pre-existing concern not introduced by this PR. Important Files Changed
Sequence DiagramsequenceDiagram
participant User
participant TabItemView
participant VerticalTabsSidebar
participant SidebarDragState
participant SidebarTabDropDelegate
User->>TabItemView: begins drag
TabItemView->>VerticalTabsSidebar: onDragStart()
VerticalTabsSidebar->>SidebarDragState: "draggedTabId = tabId"
VerticalTabsSidebar->>SidebarDragState: "dropIndicator = nil"
SidebarDragState-->>VerticalTabsSidebar: "@Observable invalidates body"
VerticalTabsSidebar->>TabItemView: "isBeingDragged=true (equatable skips others)"
User->>SidebarTabDropDelegate: dropUpdated(info)
SidebarTabDropDelegate->>SidebarDragState: "dropIndicator = nextIndicator"
SidebarDragState-->>VerticalTabsSidebar: "@Observable invalidates body"
VerticalTabsSidebar->>TabItemView: "topDropIndicatorVisible=true (equatable skips others)"
User->>SidebarTabDropDelegate: performDrop(info)
SidebarTabDropDelegate->>SidebarDragState: "draggedTabId = nil"
SidebarTabDropDelegate->>SidebarDragState: "dropIndicator = nil"
SidebarDragState-->>VerticalTabsSidebar: "@Observable invalidates body"
VerticalTabsSidebar->>TabItemView: "isBeingDragged=false, topDropIndicatorVisible=false"
Reviews (6): Last reviewed commit: "Freeze showsModifierShortcutHints while ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 10812-10819: The LazyVStack currently hands row children an
`@Observable` store (SidebarDragState via dragState) which violates the guideline;
instead, keep ownership of dragState in the parent scope that builds the
LazyVStack and change workspaceRow and any drop-gap children to accept only
immutable value snapshots (e.g., draggedTabId, dropIndicator) plus mutation
closures (e.g., onStartDrag(_:), onUpdateDrop(_:), onCommitDrop()) — remove any
direct SidebarDragState or `@Observable` parameters from workspaceRow and related
views and wire those closures to mutate the parent-held SidebarDragState.
- Around line 9880-9887: The requestClear handler for
SidebarDragLifecycleNotification currently only clears dragState.draggedTabId,
which can leave dragState.dropIndicator stale; update the onReceive block
handling SidebarDragLifecycleNotification.requestClear to also reset/clear
dragState.dropIndicator (e.g., set it to nil or its default empty state)
alongside dragState.draggedTabId so that any render paths keyed on dropIndicator
no longer show a lingering indicator; locate the closure attached to
NotificationCenter.default.publisher(for:
SidebarDragLifecycleNotification.requestClear) and modify it to clear both
dragState.draggedTabId and dragState.dropIndicator.
🪄 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: 31b78bcf-1719-4454-b385-c252a7e01179
📒 Files selected for processing (1)
Sources/ContentView.swift
- TabItemView and SidebarEmptyArea no longer hold `let dragState: SidebarDragState`. Per the snapshot-boundary rule in CLAUDE.md, rows under a LazyVStack must not hold @observable store references. Replaced with value snapshots (`isBeingDragged`, `topDropIndicatorVisible`) and closure/delegate bundles that the parent constructs from `dragState` ownership. TabItemView's Equatable conformance now compares the new snapshot fields so unchanged rows still skip re-render. - Deleted SidebarTabDropIndicatorOverlay and SidebarTabDragOpacityModifier (they also held @observable refs); their work is inlined or moved to a SidebarTabDropIndicatorPredicate helper evaluated in the parent. - Marked SidebarTabDropDelegate @mainactor (Greptile P2): it mutates @MainActor-isolated SidebarDragState properties, so documenting the isolation prevents accidental removal. - requestClear notification handler now also clears `dropIndicator` (CodeRabbit) so render paths keyed on dropIndicator can't linger. - Dropped SidebarTabItemPresentationResolutionPolicyTests, which still referenced the removed presentation-snapshot types and was failing CI. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed the three review threads (all resolved): Greptile P2 ( CodeRabbit Minor (requestClear / dropIndicator): CodeRabbit Major (snapshot-boundary rule): this was the real one. Failing Vercel deploy failures: these are the fork-PR auth wall ( |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
…tate
- Refactored `SidebarTabDropIndicatorPredicate.topVisible` to take pure
values (draggedTabId, dropIndicator, tabIds) instead of a SidebarDragState
+ [Tab] pair, so the per-row indicator predicate is unit-testable without
any view-state setup. Extracted `emptyAreaTopVisible` to host the
corresponding logic that used to live inline on SidebarEmptyArea.
- New `cmuxTests/SidebarTabDropIndicatorPredicateTests.swift`:
* `SidebarTabDropIndicatorPredicateTopVisibleTests` (8 cases): no drag,
no indicator, indicator on this row's top edge, this row's bottom edge,
previous row's bottom edge, unrelated row, first-row bottom edge, stray
row id not in tabIds.
* `SidebarTabDropIndicatorPredicateEmptyAreaTests` (7 cases): no drag,
no indicator, end-of-list (tabId nil), last row bottom edge, last row
top edge, non-last row bottom edge, empty list.
* `SidebarDragStateTests` (3 cases): initial cleared, independent
per-property mutation, clearing both yields idle state.
All 18 cases pass against the cmux-unit scheme.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
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 10959-10963: The code calls renderContext.tabs.map(\.id)
repeatedly inside each row to compute topDropIndicatorVisible via
SidebarTabDropIndicatorPredicate.topVisible(forTabId:draggedTabId:dropIndicator:tabIds:),
causing an O(n) allocation per row; fix by precomputing a single [UUID] snapshot
(e.g., renderedTabIds) once in the parent scope where other row snapshots are
created (before iterating/building row views) and pass renderedTabIds into the
call instead of renderContext.tabs.map(\.id), leaving the call site using
SidebarTabDropIndicatorPredicate.topVisible(forTabId: tab.id, draggedTabId:
dragState.draggedTabId, dropIndicator: dragState.dropIndicator, tabIds:
renderedTabIds).
🪄 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: a787ac0a-7ab2-4b4a-b6de-5d40b10216ca
📒 Files selected for processing (3)
Sources/ContentView.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SidebarTabDropIndicatorPredicateTests.swift
CodeRabbit flagged that `workspaceRow(...)` was calling `renderContext.tabs.map(\.id)` once per row to feed `SidebarTabDropIndicatorPredicate.topVisible(...)`, making the per-render cost O(n²) in workspace count. Store the snapshot once on `WorkspaceListRenderContext.tabIds` at the parent body level and reuse it per row. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Addressed in 186ff09: |
Removing SidebarTabItemPresentationResolutionPolicy in the lazy-sidebar refactor dropped the per-row freeze that prevented modifier-key transitions from flipping shortcut-hint badges on the row sitting behind an open context menu (Greptile flagged this regression; the PR's claim that rowInteractionState preserves the freeze only covers the workspace snapshot, not the modifier hint flag). Reintroduce the freeze with a small pure policy + per-row closures so the snapshot boundary still holds: the parent owns frozenShortcutHintsTabId/Value, rows only see Bool snapshots and call onContextMenuAppear/Disappear closures. Clear the freeze when the frozen row is removed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This branch was successfully deployed
2 participants
Summary
Reapplies #3039's lazy-sidebar refactor on top of current
main(the original PR was 924 commits behind and couldn't merge).@State draggedTabId/dropIndicator/frozenTabItemPresentationonVerticalTabsSidebarwith a single@State dragState = SidebarDragState()(an@Observable@mainactor class).VStack→LazyVStack. The eagerVStackwas a workaround for a layout-invalidation loop (Nightly freezes: sidebar LazyVStack layout loop pegs main thread at 100% CPU, deadlocks CLI #2586) caused by 60Hz@Statemutations during drag invalidating the entire sidebar body.@Observableper-property tracking means only the tiny overlays that actually read those properties re-render.SidebarTabDropIndicatorOverlay,SidebarTabDragOpacityModifier, and aSidebarEmptyAreaDropIndicatorOverlayso the high-frequencydropIndicator/draggedTabIdreads stay inside isolated leaf views, not the 1600-lineTabItemViewbody.draggedTabIdBinding/dropIndicatorBindingadapter computed properties so unmigrated consumers (extension sidebar drop delegates, bonsplit workspace overlays) keep working without further refactor.SidebarTabItemPresentationSnapshot+SidebarTabItemPresentationResolutionPolicytypes — frozen-presentation visual stability is preserved by main's existingrowInteractionStateflow.rowHeightProbefromTabItemView.bodyto keep the Swift type checker under budget after the new modifier was added.Differences from #3039
main, notmainfrom April 2026 — 924 commits later.224ea307,65749468) that route hover throughdragState.hoveredTabId. Those conflict with main'sSidebarWorkspaceRowInteractionStatedesign and are deferred to a follow-up.rowInteractionState.contextMenuDidDisappear()flow instead of Make workspace sidebar lazy with @Observable drag state #3039'scontextMenuState.isVisible = falsepath.Test plan
Activity Monitorduring a sustained drag — should be far below the eager-VStack baseline.LazyVStackshould virtualize rows.Refs #3039, #2586.
🤖 Generated with Claude Code
Need help on this PR? Tag
@codesmithwith what you need. Autofix is disabled.Summary by cubic
Make the workspace sidebar virtualized and smooth during drag by switching to
LazyVStackand centralizing drag/drop in an@ObservableSidebarDragState. Use value snapshots and pure predicates, precomputetabIdsper render, and restore stable modifier shortcut hints while a context menu is open; this fixes the drag layout loop and reduces CPU on large lists (refs #2586).Refactors
@State draggedTabId/dropIndicatorwithSidebarDragState; add adapter bindings for existing@Bindingconsumers.isBeingDragged,topDropIndicatorVisible,onDragStart, and drop-delegate closures to rows/empty area; updateTabItemViewEquatable.SidebarTabDropIndicatorPredicate; precomputetabIdsonce per render; removeSidebarTabDropIndicatorOverlayandSidebarTabDragOpacityModifier.SidebarTabDropDelegate@MainActor; cleardropIndicatoron drag-clear; move row-height probe out ofTabItemView; delete frozen presentation types and their tests.SidebarShortcutHintFreezePolicy; clear the freeze if the row is removed; add unit tests for the predicate, drag state, and freeze policy.Migration
Written for commit 4ecd35b. Summary will update on new commits. Review in cubic
Summary by CodeRabbit