Repository navigation
Sidebar: remove rows measurement that re-livelocks layout at scale - #6188
Merged
Merged
Conversation
SidebarRowsFillLayout split the rows vs the empty drop area by calling `subviews.first?.sizeThatFits(width:, height: nil)` in both sizeThatFits and placeSubviews. Asking a LazyVStack for its natural height realizes every row synchronously on every layout pass, so at high workspace counts one `GraphHost.flushTransactions` pegs the main thread (a fresh capture on 0.64.16 sat ~4s inside a single transaction under SidebarRowsFillLayout). This is the same livelock class as #2586 / #5764 / #5845. #6033 removed the old GeometryReader+PreferenceKey @State feedback loop but replaced it with this custom Layout, which still force-measures the whole list; it was only dogfooded at 12 workspaces where the O(N) measure is cheap. Remove the custom Layout. The `.frame(minHeight:)` that already wrapped it is the correct primitive: it clamps the content to max(rowsHeight, viewport) without forcing realization. The rows stay lazy and pin to the top; the empty drop/tap area becomes a full-size background filling the same frame; the end-of-list drop indicator anchors to the rows' own bottom edge. Fill semantics (rows fit -> fill viewport, no phantom scrollbar #3241; rows overflow -> scroll) are preserved, computed natively by SwiftUI. Deletes SidebarRowsFillLayout and the now-unused emptyAreaFillHeight helpers + their unit tests; keeps contentMinHeight and its #3241 tests.
|
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:
📝 WalkthroughWalkthroughRemoves the custom ChangesSidebar workspace layout simplification
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Possibly related PRs
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 |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
Greptile SummaryRemoves
Confidence Score: 5/5Safe to merge — removes a measurement-based livelock with a native SwiftUI primitive, with no state-management or concurrency changes. The change is a targeted deletion of a custom Layout whose sizeThatFits(height: nil) call on the LazyVStack was the documented root cause of the livelock. The replacement — .frame(minHeight:) plus a layered hit-target absorber — is a well-understood SwiftUI pattern that requires no new measurement. Fill semantics, per-row drop delegates, and the empty-area double-tap path are all preserved. The only acknowledged behavioral delta (inter-row gap drops consuming via the absorber instead of routing to a between-row insert) is explicitly called out in the PR description for reviewer confirmation. No files require special attention. The Xcode project file, budget TSV, and test deletions all cleanly match the Swift source changes. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[workspaceRows
LazyVStack — lazy, no height measure] --> B[.overlay alignment:.bottom
Drop indicator at last-row edge]
B --> C[.background
Color.clear absorber — rows-sized
swallows double-tap + drop over rows]
C --> D[.frame minHeight: minHeight
alignment: .top
viewport fill without measuring rows]
D --> E[.background alignment:.top
SidebarEmptyArea expandsVertically:true
fills full frame]
subgraph Hit-test order over rows area
R1[Per-row delegates — frontmost]
R2[Clear absorber — returns false for
SidebarTab + Bonsplit drops]
R3[SidebarEmptyArea — blocked by absorber]
R1 --> R2 --> R3
end
subgraph Hit-test order over empty area below rows
E1[Nothing from rows]
E2[SidebarEmptyArea — receives
double-tap + drops]
E1 --> E2
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"}}}%%
flowchart TD
A[workspaceRows
LazyVStack — lazy, no height measure] --> B[.overlay alignment:.bottom
Drop indicator at last-row edge]
B --> C[.background
Color.clear absorber — rows-sized
swallows double-tap + drop over rows]
C --> D[.frame minHeight: minHeight
alignment: .top
viewport fill without measuring rows]
D --> E[.background alignment:.top
SidebarEmptyArea expandsVertically:true
fills full frame]
subgraph Hit-test order over rows area
R1[Per-row delegates — frontmost]
R2[Clear absorber — returns false for
SidebarTab + Bonsplit drops]
R3[SidebarEmptyArea — blocked by absorber]
R1 --> R2 --> R3
end
subgraph Hit-test order over empty area below rows
E1[Nothing from rows]
E2[SidebarEmptyArea — receives
double-tap + drops]
E1 --> E2
end
Reviews (4): Last reviewed commit: "Merge origin/main into fix-sidebar-fill-..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Autoreview flagged that mounting SidebarEmptyArea as the background of the framed rows spread its end-of-list drop delegate (targetTabId: nil) behind the entire row block: workspace drops in the 2pt inter-row gaps and row padding — and the whole list when rows overflow and there is no blank area at all — could resolve to the end target instead of the hovered/adjacent row. Add a rows-sized background that claims SidebarTabDragPayload drops over the rows block and no-ops them, so only the genuine blank area below the last row routes to SidebarEmptyArea (and nothing does when rows overflow). Per-row delegates render in front and still win over their own rows. Measurement-free: the blocker is sized to the lazy rows, not the filled frame. Bumps the ContentView file-length budget by the net fix lines.
Follow-up to the drop-routing fix: the rows-sized blocker now also claims the double-tap-to-create gesture and Bonsplit new-workspace drops, not just workspace-reorder drops. Otherwise a double-tap or Bonsplit drop landing in the 2pt inter-row gaps, row padding, or anywhere over an overflowing list could still reach SidebarEmptyArea behind and create/route a workspace where the old SidebarRowsFillLayout placed a zero-height empty area. Physically placing the empty area below the rows requires measuring the LazyVStack height every layout pass (the realized-all-rows livelock this PR removes), so neutralizing every empty-area interaction over the rows block is the measurement-free equivalent.
azooz2003-bit
approved these changes
Jun 16, 2026
This was referenced Jun 22, 2026
austinywang
added a commit
that referenced
this pull request
Jun 29, 2026
…#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>
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
3 participants
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.
Problem
A fresh hang capture on stable nightly 0.64.16 (build 96) shows the main thread pegged ~122% CPU, RSS 1.25 GB (footprint peak 3.9 GB), debug socket refusing connections, with the entire ~4s sample inside one
NSHostingView.beginTransaction → GraphHost.flushTransactions → LazySubviewPlacements.placeSubviews → LazyStack.place → ForEachList.applyNodes. The app-owned frames are all the workspace sidebar:SidebarWorkspaceRenderItem.id.getter,SidebarWorkspaceRenderItemID.hash(into:), andLayout.sizeThatFits ... in conformance SidebarRowsFillLayout.This is the same livelock class as #2586, #5764, #5845.
Root cause
#6033 correctly removed the old
GeometryReader+PreferenceKey→@Statefeedback loop, but replaced it withSidebarRowsFillLayout, a customLayoutthat splits the rows vs the empty drop area by calling:in both
sizeThatFitsandplaceSubviews. Asking aLazyVStackfor its natural height (height: nil) forces SwiftUI to realize and measure every row synchronously on every layout pass, defeating laziness at measure time. With many workspaces under continuous agent-driven updates, a single layout transaction takes seconds and the main thread livelocks.#6033 was dogfooded at 12 workspaces, where that O(N) measure is cheap (its sample showed zero
flushTransactionsframes), so the cost only appears at agent-heavy scale.Fix
Remove the custom
Layout. The.frame(minHeight: minHeight, alignment: .top)that already wrapped it is the correct primitive: it clamps the content height tomax(rowsHeight, viewport)without forcing realization, so the rows stay lazy..backgroundthat fills the sameminHeightframe (drops over rows still hit the per-row delegates; the blank region below hits the empty-area delegate, as before)..overlay(alignment: .bottom)) instead of the empty area's top.Fill semantics are unchanged and now computed natively by SwiftUI: rows fit → content fills the viewport, no phantom scrollbar (#3241); rows overflow → document scrolls.
Deletes
SidebarRowsFillLayoutand the now-unusedemptyAreaFillHeighthelpers + their unit tests. KeepscontentMinHeightand its #3241 pixel-rounding tests (still the production min-height value).Verification
sbfillcompiles and launches.Review asks (@azooz2003-bit, you own this area)
SidebarRowsFillLayoutthat.frame(minHeight:)doesn't cover.tabDropDelegateFactory(rowHeight)) are unchanged. Acceptable?Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Removes the sidebar’s custom rows measurement and uses
.frame(minHeight:)to keep rows lazy and stop main-thread livelocks at scale. Preserves fill (no phantom scrollbar) and routes only the real blank space below the last row to the empty-area delegate.Bug Fixes
SidebarRowsFillLayoutand stop whole-listLazyVStackmeasurement; removeemptyAreaFillHeighthelpers and their tests; keepcontentMinHeightrounding tests (sidebar scrollbar always visible — should only appear when content overflows #3241). Addresses Nightly freezes: sidebar LazyVStack layout loop pegs main thread at 100% CPU, deadlocks CLI #2586, cmux.app 100% CPU hang: non-converging LazyHVStack layout loop over sidebar workspace list (SidebarWorkspaceRowIdsPreferenceKey feedback) — stable 0.64.14 #5764, Recurring main-thread hang/livelock with many workspaces + agent sessions: run loop stuck in LazyStack/ForEachList view-graph updates (force quit twice in 2 days) #5845 (regressed in Sidebar perf: kill the LazyVStack layout livelock (combined: 6019 + 6021 + 6026) #6033)..frame(minHeight: viewport)and draw the end-of-list drop indicator as a bottom overlay on the rows.SidebarEmptyAreaas a full-height, top-aligned background and add a rows-sized clear background that swallows double-tap, workspace-reorder drops, and Bonsplit new-workspace drops over the rows; only the blank region below the last row hits the empty-area delegate. Per-row drops are unchanged.Refactors
ContentView.swift; update the file-length budget.Written for commit f9e29b8. Summary will update on new commits.
Note
Medium Risk
Touches sidebar scroll layout, drag/drop routing, and empty-area gestures; behavior may differ slightly for drops in inter-row gaps, though per-row drops are unchanged.
Overview
Fixes a main-thread layout livelock in the workspace sidebar by dropping the custom
SidebarRowsFillLayoutthat calledsizeThatFits(height: nil)on theLazyVStackevery layout pass, which realized all rows at scale (regression from #6033; same class as #2586 / #5764 / #5845).workspaceScrollContentnow lays out lazy rows with.frame(minHeight:)only (viewport fill / scroll without measuring list height), puts the end-of-list drop indicator on a bottom overlay on the rows, mountsSidebarEmptyAreaas a full-height top-aligned background, and adds a rows-sized clear background that swallows reorder/Bonsplit drops and double-tap over the list so only the blank below the last row stays an empty-area target.Deletes
SidebarRowsFillLayout.swift,emptyAreaFillHeighthelpers inWindowChromeMetrics, their unit tests, and Xcode project references.contentMinHeightand its #3241 rounding tests are unchanged.Reviewed by Cursor Bugbot for commit f9e29b8. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
Closes #6210