Repository navigation
Fix sidebar workspace frame collection virtualization - #5325
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughGates per-row frame-anchor collection so workspace drop-target frames are only published during an active tab drag or when bonsplit drop mode is enabled. Adds a centralized predicate, a transient ContentView state flag, conditional anchor application in headers/rows, overlay refactor, and pending-drop resolution in the Bonsplit drop view. ChangesGate workspace drop target collection on drag to restore sidebar virtualization
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning)
✅ Passed checks (16 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 |
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 12218-12221: Compute
SidebarDropPlanner.shouldCollectWorkspaceDropTargets once in workspaceRows
(using dragState.draggedTabId and isBonsplitWorkspaceDropTargetCollectionActive)
and pass that boolean into workspaceRow as an extra parameter (or extract it to
a shared computed property used by both workspaceRows and workspaceRow). Update
all callers that currently recompute the predicate (the occurrences around
workspaceRows and the separate workspaceRow usage) to use the new
parameter/property, and remove the duplicated calls to
SidebarDropPlanner.shouldCollectWorkspaceDropTargets to keep the decision
centralized.
- Line 10452: The SwiftLint warning is triggered because the SwiftUI state
property is not private; change the declaration of
isBonsplitWorkspaceDropTargetCollectionActive to be private (mark the `@State` var
isBonsplitWorkspaceDropTargetCollectionActive as private) inside
VerticalTabsSidebar so the state is encapsulated and satisfies the
private_swiftui_state rule.
🪄 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: 27fcb2ac-38b2-4e92-bdc3-15efdd5facea
📒 Files selected for processing (5)
Sources/ContentView.swiftSources/Sidebar/SidebarBonsplitTabWorkspaceDropOverlay.swiftSources/Sidebar/SidebarDropPlanner.swiftSources/VerticalTabsSidebar+WorkspaceGroups.swiftcmuxTests/SidebarWorkspaceDropPlannerTests.swift
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 12243-12259: The
overlayPreferenceValue(SidebarWorkspaceRowFramePreferenceKey.self) +
GeometryReader block is always mounted which forces collection/resolution of all
row anchors; instead only install the overlay when
shouldCollectWorkspaceDropTargets is true. Replace the unconditional
rows.overlayPreferenceValue { … GeometryReader { …
bonsplitWorkspaceDropOverlay(...) } } with a conditional that returns rows as-is
when shouldCollectWorkspaceDropTargets is false and applies the
overlayPreferenceValue/GeometryReader/bonsplitWorkspaceDropOverlay only when
shouldCollectWorkspaceDropTargets is true (referencing overlayPreferenceValue,
SidebarWorkspaceRowFramePreferenceKey, GeometryReader,
bonsplitWorkspaceDropOverlay, shouldCollectWorkspaceDropTargets, and
renderContext.tabs to locate the code).
In `@Sources/Sidebar/SidebarBonsplitTabWorkspaceDropOverlay.swift`:
- Line 55: The call to nsView.performPendingDropIfPossible() inside updateNSView
can synchronously invoke the closures wired there
(nsView.performExistingWorkspaceMove / performNewWorkspaceMove /
nsView.setDropIndicator) which mutate `@Binding` state (selectedTabIds,
lastSidebarSelectionIndex, dropIndicator) during the SwiftUI view update pass;
move the performPendingDropIfPossible invocation out of updateNSView so it runs
after the current runloop tick (e.g. dispatch async to main queue or schedule
from the NSView/Coordinator callback) ensuring updateNSView only wires the
closures and does not trigger them synchronously.
- Around line 136-168: performDragOperation currently accepts a transfer and
performs the move but doesn't clear the cached pendingDrop, and
draggingExited/concludeDragOperation return early without clearing it, allowing
a stale PendingDrop to be replayed by performPendingDropIfPossible; update
performDragOperation to nil-out pendingDrop after a successful accept (i.e., in
the accepted-transfer branch after perform(action:transfer:)), and also clear
pendingDrop in draggingExited and concludeDragOperation teardown paths so the
stored PendingDrop cannot outlive the gesture and be replayed later by
performPendingDropIfPossible or updateNSView.
🪄 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: 71a08416-49a1-4f24-83f1-292fbf0fdaf6
📒 Files selected for processing (2)
Sources/ContentView.swiftSources/Sidebar/SidebarBonsplitTabWorkspaceDropOverlay.swift
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Greptile SummaryThis PR gates
Confidence Score: 4/5Safe to merge with one follow-up: the teardown polling path can silently discard a Bonsplit workspace move on a loaded system before the real signal path is hardened. The collection gate, TargetBridge signal path, and NSView-identity fix are all correct and well-tested. The one issue is Sources/Sidebar/SidebarBonsplitTabWorkspaceDropOverlay.swift — specifically the Important Files Changed
Sequence DiagramsequenceDiagram
participant OS as macOS Drag Session
participant NSView as SidebarBonsplitTabWorkspaceDropView
participant Bridge as TargetBridge
participant SW as SwiftUI (TargetWriter)
participant State as VerticalTabsSidebar @State
OS->>NSView: draggingEntered
NSView->>State: setWorkspaceDropTargetCollectionActive(true)
State->>SW: overlayPreferenceValue reader installed
SW->>Bridge: updateTargets([...frames...])
Bridge-->>NSView: (stores targets)
OS->>NSView: performDragOperation (targets empty)
NSView->>NSView: "pendingDrop = PendingDrop(requestId, point, transfer)"
NSView-->>OS: return true (deferred)
SW->>Bridge: updateTargets([...frames arrive...])
Bridge->>NSView: async performPendingDropIfPossible()
NSView->>NSView: perform(action, transfer)
NSView->>State: setWorkspaceDropTargetCollectionActive(false)
State->>SW: overlayPreferenceValue reader removed
SW->>Bridge: clearTargets()
Note over OS,NSView: Teardown path (draggingExited / concludeDragOperation with pendingDrop set)
OS->>NSView: draggingExited
NSView->>NSView: "completeOrClearPendingDropAfterDragTeardown(remainingFrameWaits=3)"
loop up to 3 main-queue hops
NSView->>NSView: targets.isEmpty? recurse
end
NSView->>NSView: clearPendingDrop() or performPendingDropIfPossible()
NSView->>State: setWorkspaceDropTargetCollectionActive(false)
Reviews (3): Last reviewed commit: "fix(sidebar): keep Bonsplit drop overlay..." | Re-trigger Greptile |
There was a problem hiding this comment.
1 issue found across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
…target gate flips PR #5325 gates the row-frame anchor collection to active drag, but returns the drop-capture NSView from both branches of the if/else (it lives inside rowsWithDropOverlay). The two branches have distinct SwiftUI identity, so the gate flip on draggingEntered tears down and recreates SidebarBonsplitTabWorkspaceDropView mid-drag, orphaning the in-flight drag (the pendingDrop/requestId/frame-wait machinery exists to paper over this). Move the overlayPreferenceValue reader (the virtualization-defeating part) into a gated helper applied to the rows, and mount the drop-capture overlay at the stable outer level so its identity never changes when the gate flips. Virtualization is preserved (the reader stays conditional); only the consumer NSView becomes identity-stable. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Reviewed this against the #2586 diagnosis, built it locally, and dogfooded drag-and-drop. The fix is correct and the right one — gating the per-row One fix pushed (
|
Fresh field evidence: a real 15.5 s hang spindump, scroll-triggered, same
|
|
@azooz2003-bit heads up — the workspace-sidebar virtualization issue (#2586 / #5323) was causing real hard hangs: a user hit a 15.5s beachball just scrolling the sidebar (CPU-bound spinning in the This just merged here in #5325 ( Since you did the recent sidebar lazy + |
…lock (#2586) (#5708) * Remove sidebar row-ids preference aggregation that fed the layout livelock The workspace sidebar published SidebarWorkspaceRowIdsPreferenceKey from every row and group header, reduced it sidebar-wide on every layout pass, and wrote the aggregated set into @State from onPreferenceChange to gate selected-workspace scroll-into-view on "row is laid out". During lazy layout churn (scroll, tab switch) that preference value changes on every pass, so each pass scheduled another transaction from inside GraphHost.flushTransactions; once the cycle stopped converging the main thread never returned to the run loop (100% CPU, unbounded autorelease growth, CLI dispatch_sync deadlock). ScrollViewProxy.scrollTo resolves .id() values in lazy containers without the row being realized, and unknown ids are a no-op, so the laid-out gate is unnecessary: scroll pending requests unconditionally and delete the preference key, the per-row emitters, the sidebar-wide onPreferenceChange, and the laidOutWorkspaceRowIds state. Same class-elimination as #5325 did for the sibling SidebarWorkspaceRowFramePreferenceKey (drop-target anchors), which no longer appears in hang samples since it shipped. Issue: #2586 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Scroll to the group header when the selected workspace is in a collapsed group Codex review: a member of a collapsed group renders no row, so scrollTo with its UUID is a no-op, and clearing the pending request dropped the old deferred-scroll-on-expand behavior. Resolve the scroll target from render-context data instead: hidden members target their group header (anchor workspace id), which is always present. Never decided from what the lazy layout realized, so it cannot re-enter the preference/layout feedback cycle. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Move collapsed-group scroll target into SidebarSelectedWorkspaceScrollPolicy Keeps ContentView.swift under the Swift file length budget (workflow-guard-tests) and makes the collapsed-group fallback unit testable. Adds coverage: no group / expanded group target the workspace itself, collapsed group targets the group anchor. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Import Foundation in SidebarState for UUID Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…ions branch Dogfood verdict on the NSDraggingSession live reorder: jank. The logged root cause is structural — per-row drop delegates hit-test rows that the reorder itself animates, so every fix was a damping patch on a feedback loop, and the system drag image (the floating snapshot) is not the interaction Lawrence wants anyway: the row itself should track the pointer's Y with X pinned to the sidebar. This merges the task-sidebar-reorder-gesture engine (#5226) and adapts it to this branch: - A DragGesture owns the whole interaction. The picked-up row renders invisible in the list; a 1:1 follower copy paints over the list, glued to the cursor Y (X fixed), with implicit animations disabled. No NSDraggingSession, no drag image. - The model never mutates mid-drag: the list reflows to a preview (dragPreviewItems) so the gap animates open, and the single reorderSidebarWorkspace commit happens on release. The feedback-loop class is gone by construction — committed row frames are frozen during the drag, so hit-testing never sees its own preview. - Hysteresis (SidebarReorderIndicatorResolver) holds the landing slot near row midpoints. - Branch adaptations: ForEach identity stays on representedWorkspaceId so the promote/ungroup morph keeps working; the #5325 gated bonsplit drop-target reader and always-mounted overlay are preserved; group headers keep the rollup badge and reorder through the same gesture. - Escape cancels: a clear request latches cancelledReorderTabId so the still-alive gesture cannot re-begin, and since nothing mutated there is nothing to restore — the TabManager drag-restore snapshot API and its tests are removed as obsolete. - Autoscroll: each tick reports its actual scroll delta and the reorder advances the cursor by it, keeping the follower under the pointer and the landing slot moving while the pointer rests at a list edge. Known regression, accepted for now: dragging a workspace to another window's sidebar no longer works (there is no NSDraggingSession to carry it); the context menu's Move to Window remains. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…021 + 6026) (#6033) * Sidebar: remove whole-content rows-height measurement (fixes layout livelock) Replace the LazyVStack background GeometryReader -> SidebarWorkspaceRowsHeightPreferenceKey -> @State workspaceRowsMeasurement -> emptyAreaHeight round trip with SidebarRowsFillLayout, a custom Layout that places the rows at their natural height and stretches the empty drop/tap area to fill the remaining viewport from its own concrete bounds, in one geometry pass with no state writes. The preference write during layout fed a non-converging relayout transaction: main thread pinned 100%+ in GraphHost.flushTransactions -> LazySubviewPlacements.placeSubviews -> LazyStack.place -> ForEachList.applyNodes. A fresh 2026-06-12 capture on stable 0.64.15 (which already contains the mitigations from #5708, #5846, #5855, and #5859) shows the identical signature: 128% CPU, 400 threads, debug socket refusing connections, 3344/3715 main-thread samples inside flushTransactions. The rows-height key is the last live write-during-layout edge in the sidebar after #5325 (frame anchors) and #5708 (row IDs) removed their siblings. Same approach as #5852, re-ported on top of the #5846 pixel-alignment work (contentMinHeight flooring is kept; only the empty-area math moves into the Layout). Fixes #5764. Helps #2586, #5570, #5845. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: create bundled helper directory before install * Sidebar: replace render-item String id with an allocation-free Hashable enum ForEach(renderItems, id: \.id) gathers every row's identifier on each list diff, and the sidebar re-diffs all rows per update. The previous computed String id ("workspace.\(uuid.uuidString)") allocated and formatted a fresh 36-char string per access; SidebarWorkspaceRenderItem.id.getter was the hottest app-owned frame in the #5764 livelock spindump. SidebarWorkspaceRenderItemID is a two-case enum over UUID: identity compare and hash with zero heap allocation, and group headers can never collide with workspace rows on the same UUID (same guarantee the string prefixes gave). Identity values are unchanged in meaning, so row lifetime and animations are unaffected; nothing persisted the string form (the only consumers are the ForEach key path and scrollTo, which targets the explicit inner .id(tab.id) UUIDs, not the ForEach identity). Pure per-pass cost cut for #5764, #5845, #2586; complements the structural loop fix in #6019. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Sidebar: make rows height-stable under agent churn (no height animation, eager markdown) Two changes that stop agent activity from continuously varying sidebar row heights, which kept re-feeding the sidebar-wide layout/measurement cycle at animation frame rate (#5764, #5845): 1. Remove the three implicit .animation(value:) modifiers on agent-mutable snapshot fields (latestLog, progress, metadataBlocks.count) and reduce the four height-moving .transition(.opacity.combined(.move(edge: .top))) modifiers in TabItemView's log/progress/metadata sections to .transition(.opacity). While a row-height animation runs, every frame produces a different LazyVStack content height; with dozens of agent sessions some row is always animating. Content changes now apply in one discrete layout pass. 2. SidebarMetadataMarkdownBlockRow parsed its markdown in onAppear into @State: a guaranteed nil -> attributed swap (and height change) on every first appearance of every block scrolling in. It now renders inline via a new SidebarMetadataMarkdownRenderer with a bounded (512-entry) memo cache, so the FIRST render is already attributed and appearance performs no state write and no height change. Matches the SidebarWorkspaceDescriptionText sibling, plus memoization to keep repeat body evals cheap and growth bounded. WWDC backing: lazy rows must be height-stable after appearing; initialize row state in the initializer, not onAppear (WWDC26 "Dive into lazy stacks", 321); keep body cheap / precompute (WWDC23 10160, WWDC25 306). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Cache failed parses with updateValue (subscript assignment drops nil values) With [String: AttributedString?], `cache[markdown] = parsed` removes the key when parsed is nil, so unparseable blocks re-parsed on every body eval and appended phantom keys to insertionOrder, mis-evicting valid entries once at capacity. Caught by Greptile on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Byte-bound the metadata markdown cache (autoreview P1) The 512-entry cap bounded entry count but not retained bytes. Metadata blocks are agent/control-socket supplied and uncapped at this boundary, so a key churning large unique markdown could keep hundreds of big payloads alive after the workspace metadata was overwritten or cleared (worse than the old row-local @State, which released on update). Skip caching blocks over 4096 UTF-8 bytes: they parse inline each eval (rare, still attributed from the first frame), and total retained cache bytes are now bounded by capacity * maxCacheableBytes regardless of churn. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Plain-text fallback for oversized metadata blocks (autoreview P1) Parsing >4KB blocks inline (previous commit) removed the retention but moved the cost to CPU: TabItemView.body re-runs on snapshot changes under agent churn, so a large block reparsed each time. Return nil for oversized blocks instead, so the row falls back to the existing Text(block.markdown) plain path: no parse, no retention, and height-stable (the result never changes for a given block, so no nil->attributed swap). Small blocks still cache and render as markdown. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Size the sidebar empty area from an explicit viewport, not the layout proposal (autoreview P2) SidebarRowsFillLayout derived its container height from proposal.replacingUnspecifiedDimensions(). A vertical ScrollView leaves the scroll-axis height unspecified, so that fell back to a 10pt placeholder and the empty area collapsed to 0 whenever the rows fit the viewport — dropping the blank area below the last row out of the double-click/drop target. Pass the viewport height (minHeight, the floored content height the call site already computes from the scroll geometry) into the layout explicitly and size the empty area from it. New emptyAreaFillHeight(viewportHeight:rowsHeight:) overload encodes container = max(viewport, rows). Verified at runtime via temporary instrumentation (since removed): rows fit -> viewport=628 rows=421 empty=207 and rows=370 empty=258; rows overflow -> viewport=628 rows=676 empty=0. Added unit coverage for both the fit and overflow viewport paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…(Fix 5: F9, F11) F9 — SidebarWorkspaceFrameAnchorModifier used if/else around `content`, so the drop-target frame anchor was structurally present only while collecting. Flipping isEnabled at drag start/end (shouldCollectWorkspaceDropTargets) gave every materialized row a new structural identity, recreating each TabItemView subtree (dropping per-row @State, forcing fresh snapshot builds + relayout) inside the drag transaction. Make it branchless: always apply anchorPreference, emit `isEnabled ? [id: anchor] : [:]`. Row identity stays stable across the flip; the emitted preference is byte-identical to before while dragging, and the virtualization-defeating *reader* stays gated on the drag (#5325), so emitting an empty dict when idle costs nothing. F11 — SidebarWorkspaceDescriptionText carried `.id(description)`, keying the view's identity to agent-updated text, so every description rewrite tore down and recreated the view (markdown re-parse, layout caches) on top of the in-place re-render the text change already triggers. Delete it; the view updates in place. Verified on tag rowapat: simulated sidebar drag drives the frame-anchor path (drop indicator tracked across all steps, socket 31ms, zero layout-loop symbols); markdown description renders bold/italic and wraps with no .id. Deferred from Fix 5 with rationale: F8 (per-row rowHeight @State feeding the drop delegate) and F13 (per-row Finder .task pre-warm) — both now low-value post-#6026 (rows are height-stable) and both carry real regression surface (reorder hit-testing via a SidebarTabDropDelegate interface change; Reveal-in-Finder is `.disabled(url == nil)` gated on the pre-warm), so they warrant a focused follow-up rather than riding with these contained changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lazy Finder resolve (Fix 5: F9/F11/F13) (#6052) * Sidebar: branchless frame-anchor modifier + drop the description .id (Fix 5: F9, F11) F9 — SidebarWorkspaceFrameAnchorModifier used if/else around `content`, so the drop-target frame anchor was structurally present only while collecting. Flipping isEnabled at drag start/end (shouldCollectWorkspaceDropTargets) gave every materialized row a new structural identity, recreating each TabItemView subtree (dropping per-row @State, forcing fresh snapshot builds + relayout) inside the drag transaction. Make it branchless: always apply anchorPreference, emit `isEnabled ? [id: anchor] : [:]`. Row identity stays stable across the flip; the emitted preference is byte-identical to before while dragging, and the virtualization-defeating *reader* stays gated on the drag (#5325), so emitting an empty dict when idle costs nothing. F11 — SidebarWorkspaceDescriptionText carried `.id(description)`, keying the view's identity to agent-updated text, so every description rewrite tore down and recreated the view (markdown re-parse, layout caches) on top of the in-place re-render the text change already triggers. Delete it; the view updates in place. Verified on tag rowapat: simulated sidebar drag drives the frame-anchor path (drop indicator tracked across all steps, socket 31ms, zero layout-loop symbols); markdown description renders bold/italic and wraps with no .id. Deferred from Fix 5 with rationale: F8 (per-row rowHeight @State feeding the drop delegate) and F13 (per-row Finder .task pre-warm) — both now low-value post-#6026 (rows are height-stable) and both carry real regression surface (reorder hit-testing via a SidebarTabDropDelegate interface change; Reveal-in-Finder is `.disabled(url == nil)` gated on the pre-warm), so they warrant a focused follow-up rather than riding with these contained changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Sidebar: resolve Finder dir at context-menu time, drop per-row pre-warm (Fix 5: F13) Each TabItemView ran `.task(id: finderDirectoryCacheKey)` that stat'd the workspace's directory (FileManager.fileExists) on every row appearance, just to populate the `.disabled(finderDirectoryURL == nil)` gate on the "Show in Finder" context-menu item. Scrolling N workspaces issued N disk stats + N deferred row invalidations. "Show in Finder" already re-validates the directory at click time (WorkspaceFinderDirectoryOpener.openInFinder), so the pre-warm only gated the item's enabled state. Resolve it synchronously while the context menu builds (one stat per right-click, user-initiated) via the new WorkspaceFinderDirectoryResolver.existingDirectoryURLNow. Removes the per-row @State, the `.task`, and the now-dead WorkspaceFinderDirectoryCache / CacheKey / cache(for:). Behavior unchanged: item disabled when the dir is missing, reveals it when present. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Sidebar: read row height from a reference cell, not @State (Fix 5: F8) Each sidebar row (TabItemView) and group header (SidebarWorkspaceGroupHeaderView) kept its measured height in @State, written from a background GeometryReader on appearance (a guaranteed 1 -> realHeight write) and on every later height change. That write forced a second full body evaluation of the row inside the lazy placement pass — and the write was load-bearing: the drop delegate captured the height at body-eval time, so without the re-eval it would have been stuck at 1. Replace @State with SidebarRowHeightStore, a tiny reference cell the probe mutates in place (no invalidation). The drop delegates (SidebarTabDropDelegate, SidebarWorkspaceGroupHeaderDropDelegate) now hold the store and read `height` LAZILY at drop time via a computed property, so they always see the current measurement without a body re-eval reconstructing them. Drop targets with no measured row (empty area, first-row gap) pass a nil store; the extension-sidebar delegates keep their fixed constants. Height value at drop time is unchanged (same measurement the probe always fed), so reorder hit-testing behavior is preserved — only the read path changes. Verified at runtime: drags over rows + group header mount the delegates, app responsive (64ms), zero layout-loop symbols, sidebar renders. Real-drag drop position needs human dogfood (no socket path drives DropInfo hit-testing). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * F13 fix: gate Show-in-Finder on configured path, no body-time stat (autoreview P1) The previous F13 commit resolved the Finder directory with a synchronous existence stat in the context-menu builder, assuming it ran only on right-click. SwiftUI evaluates the `.contextMenu { }` items as part of the row body, so that reintroduced a synchronous FileManager.fileExists on the main actor for every row body eval of a configured workspace — exactly the hot-path filesystem IO the sidebar avoids. Gate the menu item on whether a directory is configured (WorkspaceFinderDirectoryResolver.path, in-memory, IO-free) and defer the stat to the click action, where WorkspaceFinderDirectoryOpener already re-validates and beeps if the directory is gone. Removes the synchronous helper. Behavior unchanged for existing directories; a configured-but-deleted directory now shows an enabled item that beeps on click instead of a disabled item. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Revert "Sidebar: read row height from a reference cell, not @State (Fix 5: F8)" This reverts commit 3aaf643. --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
…#6870) * Guard sidebar LazyVStack against re-livelock at scale (#6384) The workspace-sidebar rows render as a LazyVStack inside a vertical ScrollView. Keeping that stack lazy at *measure time* is load-bearing: the sidebar is re-diffed on every workspace/telemetry update, so any code that forces SwiftUI to realize and measure the whole row list on each layout pass turns a routine update into a multi-second GraphHost.flushTransactions() main-thread livelock once enough workspaces/surfaces are open. That is exactly the ~1s beachball reported in #6384. The root cause -- SidebarRowsFillLayout, a custom Layout that called subviews.first?.sizeThatFits(ProposedViewSize(width:, height: nil)) on the LazyVStack every pass -- was removed in #6188 (#6210), and the rows are lazy again in main. But this same class of bug has now regressed four times (#2586, #5764, #5845, #6033 -> #6210/#6384) and is defended only by inline comments, which CI cannot enforce. Add a source-scan regression guard so the contract fails CI on re-introduction: - scripts/check-sidebar-lazy-layout.py neutralizes comments/string literals (the guarded functions deliberately *name* the forbidden anti-patterns in explanatory comments), extracts the bodies of workspaceScrollContent and workspaceRows from Sources/ContentView.swift, and fails if either reintroduces a whole-list measurement signature (GeometryReader, ProposedViewSize(..., nil), .sizeThatFits(, or SidebarRowsFillLayout) or drops a lazy-fill primitive the fix relies on (LazyVStack( in workspaceRows, .frame(minHeight:) in workspaceScrollContent). A renamed/removed guarded function fails loudly rather than silently skipping. - tests/test_ci_sidebar_lazy_layout_guard.py proves the guard catches the bug: it passes the real repo and a clean fixture whose comments/strings name every forbidden token, and fails synthetic fixtures for each regression mode (force-measure, reintroduced custom Layout, GeometryReader, eager VStack, missing minHeight, renamed function). - Wire the self-test into the workflow-guard-tests CI job. The drag-only drop-target reader (rowsWithGatedDropTargetReader) is not scanned: it intentionally uses a GeometryReader to resolve per-row drop anchors and is gated behind an active drag (#5325), so it never runs during the steady-state layout this guard protects. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Guard: handle Swift multi-line string literals in neutralize_swift Greptile P2 (#6870 review): neutralize_swift treated every `"` as a regular string boundary, so a Swift multi-line string `"""..."""` parsed as two empty strings plus an unclosed string. A bare `"` inside such a literal (e.g. `"""... he said "GeometryReader" ..."""`) would close the outer string early and expose the remaining content -- including a forbidden token named in prose -- as apparent code, tripping the guard with a false positive and a spurious CI failure. Add a MULTILINE_STRING tokenizer state: `"""` opens it, only a closing `"""` ends it, and a lone `"` inside is neutralized like any other string content. Add self-test case (b2) with a multi-line string containing a bare quote plus GeometryReader / sizeThatFits(ProposedViewSize(height: nil)) / SidebarRowsFillLayout, asserting the guard still passes. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Guard: ban any custom Layout applied to the sidebar rows, not just by name Codex autoreview P2 (#6870): the guard only banned the literal deleted type name `SidebarRowsFillLayout`, so a future regression could wrap `workspaceRows(...)` in a differently named custom `Layout` (with the `subview.sizeThatFits(ProposedViewSize(... height: nil ...))` body living outside the two scanned functions) and CI would pass -- the exact #6033 shape under a new name. Generalize the guard: discover every type conforming to SwiftUI's `Layout` protocol across the whole Sources/ tree (comment/string- neutralized, pre-filtered to files that mention `Layout`), then fail if ANY of those type names is applied within `workspaceScrollContent` / `workspaceRows`. A custom Layout wrapping the LazyVStack measures it on every pass regardless of the type's name; rows must be sized by `.frame(minHeight:)` instead. The literal-name and direct force-measure token bans are kept as belt-and-suspenders. Add self-test case (d2): a `struct RowsFillLayout: Layout` (NOT the old name) whose force-measure lives in the layout type, applied to the rows in `workspaceScrollContent`; the guard must fail it. Without the generalization this is a false negative. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * Guard: discover custom Layouts across Packages/ too, document scope Codex autoreview P2 (#6870): custom-Layout discovery scanned only Sources/, but cmux migrates app code into Packages/. A force-measuring sidebar layout defined in a repo-owned package and applied in workspaceScrollContent would not be discovered, leaving the guard bypassable exactly where code is moving. - Replace the Sources-only glob with repo_owned_swift_files(), which walks both Sources/ and Packages/ and prunes build/VCS/vendored dirs (.build, .git, DerivedData, Vendor, Pods, Carthage, node_modules, ...). External-dependency Layouts remain out of scope by design. - Add self-test case (i): repo_owned_swift_files() covers Sources/ and Packages/, discovers their Layout types, and excludes a .build/checkouts vendored Layout. - Document the guard's scope boundary: it protects the rows layout as expressed in workspaceScrollContent/workspaceRows and does not chase a force-measure relocated into an arbitrary transitively-called helper (fragile to track in a lint; such an extraction should re-review this guard). Custom Layout types are the exception chased across files, since a renamed force-measuring layout is the concrete #6033 regression. Real-repo scan stays ~2s (Layout-substring pre-filter). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: cmux <cmux@cmuxs-Mac-mini.local> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…021 + 6026) (#6033) * Sidebar: remove whole-content rows-height measurement (fixes layout livelock) Replace the LazyVStack background GeometryReader -> SidebarWorkspaceRowsHeightPreferenceKey -> @State workspaceRowsMeasurement -> emptyAreaHeight round trip with SidebarRowsFillLayout, a custom Layout that places the rows at their natural height and stretches the empty drop/tap area to fill the remaining viewport from its own concrete bounds, in one geometry pass with no state writes. The preference write during layout fed a non-converging relayout transaction: main thread pinned 100%+ in GraphHost.flushTransactions -> LazySubviewPlacements.placeSubviews -> LazyStack.place -> ForEachList.applyNodes. A fresh 2026-06-12 capture on stable 0.64.15 (which already contains the mitigations from manaflow-ai/cmux#5708, manaflow-ai/cmux#5846, manaflow-ai/cmux#5855, and manaflow-ai/cmux#5859) shows the identical signature: 128% CPU, 400 threads, debug socket refusing connections, 3344/3715 main-thread samples inside flushTransactions. The rows-height key is the last live write-during-layout edge in the sidebar after manaflow-ai/cmux#5325 (frame anchors) and manaflow-ai/cmux#5708 (row IDs) removed their siblings. Same approach as manaflow-ai/cmux#5852, re-ported on top of the manaflow-ai/cmux#5846 pixel-alignment work (contentMinHeight flooring is kept; only the empty-area math moves into the Layout). Fixes manaflow-ai/cmux#5764. Helps manaflow-ai/cmux#2586, manaflow-ai/cmux#5570, manaflow-ai/cmux#5845. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix: create bundled helper directory before install * Sidebar: replace render-item String id with an allocation-free Hashable enum ForEach(renderItems, id: \.id) gathers every row's identifier on each list diff, and the sidebar re-diffs all rows per update. The previous computed String id ("workspace.\(uuid.uuidString)") allocated and formatted a fresh 36-char string per access; SidebarWorkspaceRenderItem.id.getter was the hottest app-owned frame in the manaflow-ai/cmux#5764 livelock spindump. SidebarWorkspaceRenderItemID is a two-case enum over UUID: identity compare and hash with zero heap allocation, and group headers can never collide with workspace rows on the same UUID (same guarantee the string prefixes gave). Identity values are unchanged in meaning, so row lifetime and animations are unaffected; nothing persisted the string form (the only consumers are the ForEach key path and scrollTo, which targets the explicit inner .id(tab.id) UUIDs, not the ForEach identity). Pure per-pass cost cut for manaflow-ai/cmux#5764, manaflow-ai/cmux#5845, manaflow-ai/cmux#2586; complements the structural loop fix in manaflow-ai/cmux#6019. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Sidebar: make rows height-stable under agent churn (no height animation, eager markdown) Two changes that stop agent activity from continuously varying sidebar row heights, which kept re-feeding the sidebar-wide layout/measurement cycle at animation frame rate (manaflow-ai/cmux#5764, manaflow-ai/cmux#5845): 1. Remove the three implicit .animation(value:) modifiers on agent-mutable snapshot fields (latestLog, progress, metadataBlocks.count) and reduce the four height-moving .transition(.opacity.combined(.move(edge: .top))) modifiers in TabItemView's log/progress/metadata sections to .transition(.opacity). While a row-height animation runs, every frame produces a different LazyVStack content height; with dozens of agent sessions some row is always animating. Content changes now apply in one discrete layout pass. 2. SidebarMetadataMarkdownBlockRow parsed its markdown in onAppear into @State: a guaranteed nil -> attributed swap (and height change) on every first appearance of every block scrolling in. It now renders inline via a new SidebarMetadataMarkdownRenderer with a bounded (512-entry) memo cache, so the FIRST render is already attributed and appearance performs no state write and no height change. Matches the SidebarWorkspaceDescriptionText sibling, plus memoization to keep repeat body evals cheap and growth bounded. WWDC backing: lazy rows must be height-stable after appearing; initialize row state in the initializer, not onAppear (WWDC26 "Dive into lazy stacks", 321); keep body cheap / precompute (WWDC23 10160, WWDC25 306). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Cache failed parses with updateValue (subscript assignment drops nil values) With [String: AttributedString?], `cache[markdown] = parsed` removes the key when parsed is nil, so unparseable blocks re-parsed on every body eval and appended phantom keys to insertionOrder, mis-evicting valid entries once at capacity. Caught by Greptile on the PR. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Byte-bound the metadata markdown cache (autoreview P1) The 512-entry cap bounded entry count but not retained bytes. Metadata blocks are agent/control-socket supplied and uncapped at this boundary, so a key churning large unique markdown could keep hundreds of big payloads alive after the workspace metadata was overwritten or cleared (worse than the old row-local @State, which released on update). Skip caching blocks over 4096 UTF-8 bytes: they parse inline each eval (rare, still attributed from the first frame), and total retained cache bytes are now bounded by capacity * maxCacheableBytes regardless of churn. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Plain-text fallback for oversized metadata blocks (autoreview P1) Parsing >4KB blocks inline (previous commit) removed the retention but moved the cost to CPU: TabItemView.body re-runs on snapshot changes under agent churn, so a large block reparsed each time. Return nil for oversized blocks instead, so the row falls back to the existing Text(block.markdown) plain path: no parse, no retention, and height-stable (the result never changes for a given block, so no nil->attributed swap). Small blocks still cache and render as markdown. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Size the sidebar empty area from an explicit viewport, not the layout proposal (autoreview P2) SidebarRowsFillLayout derived its container height from proposal.replacingUnspecifiedDimensions(). A vertical ScrollView leaves the scroll-axis height unspecified, so that fell back to a 10pt placeholder and the empty area collapsed to 0 whenever the rows fit the viewport — dropping the blank area below the last row out of the double-click/drop target. Pass the viewport height (minHeight, the floored content height the call site already computes from the scroll geometry) into the layout explicitly and size the empty area from it. New emptyAreaFillHeight(viewportHeight:rowsHeight:) overload encodes container = max(viewport, rows). Verified at runtime via temporary instrumentation (since removed): rows fit -> viewport=628 rows=421 empty=207 and rows=370 empty=258; rows overflow -> viewport=628 rows=676 empty=0. Added unit coverage for both the fit and overflow viewport paths. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
Fixes #5323.
Reproduce-first evidence
I reproduced the mechanism locally before changing code, using the existing production cmux process on this machine rather than a fresh VM or a new dev build.
Observed:
debug.sidebar.simulate_drag, so I used ARM-native Time Profiler sampling while applying harmless temporaryUserDefaultschurn, then deleted the temporary key./tmp/cmux-issue5323-before-arm64.trace.UserDefaultsSettingsStorenotification handling re-enteringVerticalTabsSidebar.body/VerticalTabsSidebar.workspaceRows, then SwiftUI preference/layout work includingGraphHost.updatePreferences,NSHostingView.layout,ForEachList.applyNodes, andDynamicContainerInfo.updateItems.GeometryProxy.subscript.getterfromVerticalTabsSidebar.workspaceRows(renderContext:), which is the sidebar drop overlay resolving per-row.boundsanchors into drop target frames.Expected:
.boundsanchors and the sidebar should not install the row-frameoverlayPreferenceValue/GeometryReaderreader. ALazyVStackcan then avoid requiring off-viewport row frames.Fix
SidebarDropPlanner.shouldCollectWorkspaceDropTargets(...), with a failing test committed first (test: reproduce sidebar drop target collection bug).SidebarWorkspaceRowFramePreferenceKeyemitter for workspace rows and group headers.overlayPreferenceValue(SidebarWorkspaceRowFramePreferenceKey.self)so it is only installed while workspace drop target collection is active.updateNSViewto avoid mutating SwiftUI bindings during the representable update pass.TabItemViewbody work.Before / after verification
Before:
GeometryProxy.subscript.truefor no active drag, matching the old always-on collection behavior; the new test is red against that commit.After:
false; row/group.boundsanchors and the row-frameoverlayPreferenceValuereader are absent in steady state.trueduring a sidebar workspace drag or a detected Bonsplit workspace drop, limiting all-row frame collection to actual drag sessions.reload.sh/builds before CI is green and user authorization is given.Tests / checks
cmuxTests/SidebarWorkspaceDropPlannerTests.swiftfor:Note
Medium Risk
Touches sidebar drag/drop and Bonsplit move-to-workspace flows with timing-sensitive pending-drop logic; steady-state performance improves but regressions could affect large-workspace DnD.
Overview
Stops the sidebar from collecting per-workspace row frame anchors and the
overlayPreferenceValuegeometry reader unless a drag actually needs drop targets—either a normal sidebar tab drag (draggedTabId) or an active Bonsplit workspace drop signaled separately fromSidebarDragState.Bonsplit drags now turn on that collection via AppKit pasteboard callbacks, feed targets through a
TargetBridge/TargetWriter, and can defer a drop until row frames exist—without tearing down the dropNSViewwhen collection flips on (overlay stays mounted outside the gated branch).Adds
SidebarDropPlanner.shouldCollectWorkspaceDropTargets, asidebarWorkspaceFrameAnchormodifier, and unit tests for when collection is on vs off.Reviewed by Cursor Bugbot for commit beb318e. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests