Repository navigation
Sidebar: restore LazyVStack virtualization by removing whole-content height measurement (fixes 100% CPU loop) - #5852
Closed
lawrencecchen wants to merge 2 commits into
Closed
lawrencecchen wants to merge 2 commits into
lawrencecchen wants to merge 2 commits into
Conversation
…ualization The workspace-list LazyVStack was wrapped in a `.background` GeometryReader that measured its whole-content height into a PreferenceKey and `@State` to size the empty drop/tap area below the last row. Reading the LazyVStack's total height forced every row (visible and offscreen) to be placed, defeating virtualization, and writing the preference during layout fed a non-converging relayout loop that pinned the main thread at 100% CPU once enough surfaces accumulated (#2586, #5570, #5764). The existing isEquivalent dedup only stopped the @State re-write, not the full-list placement that fired on every agent-driven row update. Replace the measure -> @State -> emptyAreaHeight round-trip with a custom SidebarRowsFillLayout that places the rows at their natural height and stretches the empty area to fill the remaining space from its own finite bounds in one geometry pass. The rows are never measured into SwiftUI state, so the LazyVStack virtualizes again. It is #3241-safe by construction: the prior overflow (#3241) came from `Color.clear.frame(maxHeight: .infinity)` inside the ScrollView, while this layout only ever places into finite bounds. Verified on a tagged build: at 33 workspaces the main thread sits idle (parked in mach_msg, zero placeSubviews) instead of pegged; few-rows fill the viewport with no phantom scrollbar and many-rows scroll, both rendering correctly. Closes #5764. Complementary to #5840 (per-tick O(N) body dict rebuilds). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughReplaces PreferenceKey-driven sidebar rows height measurement with a new ChangesSidebar layout refactoring
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
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 |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cffedeca32
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| /// Expects exactly two subviews in order: `[rows, emptyArea]`. | ||
| struct SidebarRowsFillLayout: Layout { | ||
| func sizeThatFits(proposal: ProposedViewSize, subviews: Subviews, cache: inout ()) -> CGSize { | ||
| let resolved = proposal.replacingUnspecifiedDimensions() |
There was a problem hiding this comment.
In the sidebar's vertical ScrollView, the content is measured with an unspecified height, so replacingUnspecifiedDimensions() falls back to SwiftUI's default rather than the viewport floor computed by contentMinHeight. That means the layout reports roughly the rows' natural height; the later .frame(minHeight:) can expand the outer frame, but the empty-area height is derived from the layout's own shorter bounds, so when only a few workspaces are visible the blank space below the last row is no longer covered by SidebarEmptyArea for double-clicks or drops. Pass the viewport/min height into SidebarRowsFillLayout itself instead of relying on an outer frame to supply it.
Useful? React with 👍 / 👎.
Contributor
Greptile SummaryReplaces the
Confidence Score: 5/5Safe to merge. The fix replaces a well-diagnosed relayout loop with a principled single-pass layout that makes no state writes and lets the LazyVStack virtualize correctly. The change is a focused removal of the GeometryReader → PreferenceKey → @State measurement round-trip, replaced by a 53-line custom Layout whose math is fully covered by unit tests. The old feedback loop is eliminated by construction, not damped. Both the few-rows (viewport fill, #3241) and many-rows (genuine scroll) paths are correct and empirically verified. No files require special attention. The optimization opportunity in SidebarRowsFillLayout (caching the LazyVStack sizeThatFits result across sizeThatFits and placeSubviews via makeCache) was already raised in a previous review thread. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph OLD["Before (CPU loop)"]
A["LazyVStack rows rendered"] --> B[".background GeometryReader\nmeasures whole-content height"]
B --> C["SidebarWorkspaceRowsHeightPreferenceKey\nwrites preference during layout"]
C --> D["onPreferenceChange →\n@State workspaceRowsMeasurement"]
D --> E["emptyAreaHeight computed\nfrom @State"]
E --> F["VStack: rows + fixed emptyAreaHeight"]
F -->|"Agent-driven row update\nor sub-pixel jitter"| A
style C fill:#ff6b6b,color:#fff
style D fill:#ff6b6b,color:#fff
end
subgraph NEW["After (this PR)"]
G["SidebarRowsFillLayout\n(custom Layout)"] --> H["sizeThatFits: measure rows\nnatural height (width-only proposal)"]
H --> I["placeSubviews: place rows at\nnaturalHeight, empty area fills remainder\nfrom bounds (finite, viewport-anchored)"]
I --> J["LazyVStack virtualizes:\nonly visible rows placed"]
I --> K["emptyAreaFillHeight =\nmax(0, bounds.height - rowsHeight)"]
J & K --> L["No @State write, no relayout loop"]
style L fill:#51cf66,color:#fff
end
Reviews (2): Last reviewed commit: "Sidebar: test empty-area fills the whole..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+34
to
+52
| func placeSubviews(in bounds: CGRect, proposal: ProposedViewSize, subviews: Subviews, cache: inout ()) { | ||
| guard let rows = subviews.first else { return } | ||
| let rowsHeight = rows.sizeThatFits( | ||
| ProposedViewSize(width: bounds.width, height: nil) | ||
| ).height | ||
| rows.place( | ||
| at: CGPoint(x: bounds.minX, y: bounds.minY), | ||
| proposal: ProposedViewSize(width: bounds.width, height: rowsHeight) | ||
| ) | ||
| guard subviews.count > 1 else { return } | ||
| let emptyHeight = SidebarWorkspaceScrollLayout.emptyAreaFillHeight( | ||
| containerHeight: bounds.height, | ||
| rowsHeight: rowsHeight | ||
| ) | ||
| subviews[1].place( | ||
| at: CGPoint(x: bounds.minX, y: bounds.minY + rowsHeight), | ||
| proposal: ProposedViewSize(width: bounds.width, height: emptyHeight) | ||
| ) | ||
| } |
Contributor
There was a problem hiding this comment.
sizeThatFits call without a layout cache
rows.sizeThatFits(ProposedViewSize(width: …, height: nil)) is called once in sizeThatFits and again at the top of placeSubviews. SwiftUI calls both methods on the same layout pass, so this is two separate height queries against the LazyVStack per frame. The Layout protocol's makeCache/updateCache mechanism exists precisely to memoize subview measurements across these two calls. Storing rowsHeight in the cache would halve the measurement work on every layout invocation — meaningful here because querying the LazyVStack's unconstrained height is the hot operation this PR is trying to minimize.
![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 `@cmuxTests/SidebarWorkspaceScrollLayoutTests.swift`:
- Around line 60-66: Add a unit test covering the empty workspace scenario: call
SidebarWorkspaceScrollLayout.emptyAreaFillHeight with rowsHeight = 0 and assert
the returned emptyAreaHeight equals the provided containerHeight (within
tolerance). Place the new test in
cmuxTests/SidebarWorkspaceScrollLayoutTests.swift after the exact-match test
(e.g., in a new test method like emptyAreaFillsEntireContainerWhenNoRows) and
use the same CGFloat tolerance check pattern used elsewhere to compare
emptyAreaHeight and containerHeight.
🪄 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: 637035c3-ddbc-4372-a315-a830f947c714
📒 Files selected for processing (7)
Sources/ContentView.swiftSources/SidebarRowsFillLayout.swiftSources/SidebarWorkspaceRowsHeightPreferenceKey.swiftSources/SidebarWorkspaceRowsMeasurement.swiftSources/WindowChromeMetrics.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SidebarWorkspaceScrollLayoutTests.swift
💤 Files with no reviewable changes (2)
- Sources/SidebarWorkspaceRowsHeightPreferenceKey.swift
- Sources/SidebarWorkspaceRowsMeasurement.swift
Covers the empty-sidebar edge case (rowsHeight = 0) for emptyAreaFillHeight, so the drop/tap area still fills the viewport and there is no phantom scrollbar. (CodeRabbit review on #5852) Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67fa5bebca
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Comment on lines
+31
to
+33
| return CGSize(width: resolved.width, height: max(rowsHeight, resolved.height)) | ||
| } | ||
|
|
There was a problem hiding this comment.
In sessions with many workspace rows, this still asks the LazyVStack for its full natural height during layout via an unbounded height proposal. Computing that size requires SwiftUI to walk/measure the entire ForEach, which is the same whole-content measurement pattern this change is trying to remove; placeSubviews then repeats it on every layout pass, so the sidebar can still lose virtualization and hit the 100% CPU path when many surfaces update. The fresh evidence here is the new custom Layout now performs the full-height query directly instead of through the removed preference reader.
Useful? React with 👍 / 👎.
Contributor
Author
|
Closing as superseded by what landed on main:
Re-profiled current main at 33 workspaces: idle main thread is parked (0.5% CPU, no placeSubviews). During rapid create/delete the residual cost is AttributeGraph + TabItemView.body re-eval (the #5832 / #5840 dict-rebuild territory); this PR's measurement removal saved only ~35 placeSubviews frames and conflicts with #5846. Dogfood on a current-main build confirmed the sidebar feels smooth. The remaining per-op lever is #5840. |
azooz2003-bit
added a commit
that referenced
this pull request
Jun 13, 2026
…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>
ShubhamPatilsd
pushed a commit
to emergent-inc/mosaic
that referenced
this pull request
Jul 9, 2026
…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>
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.
Problem
The sidebar workspace/tab list pins the main thread at 100% CPU and beachballs once enough surfaces accumulate (the recurring force-quit-only hang: #2586, #5570, #5764). A live
sampleof the user's daily-driver showed the main thread spending 100% of its time inGraphHost.flushTransactions → LazySubviewPlacements.placeSubviews → LazyStack.place → ForEachList.applyNodesrecursing through the entireForEach, withSidebarWorkspaceRenderItem.id.getteras the hottest app frame. Reproduced on currentmain(NIGHTLY) too, just at lower intensity proportional to surface count.Root cause for this PR:
workspaceRowswrapped theLazyVStackin a.background { GeometryReader { ... .preference(SidebarWorkspaceRowsHeightPreferenceKey, rowsHeight: proxy.size.height) } }to measure the rows' whole-content height, then routed it through@State workspaceRowsMeasurementto size the empty drop/tap area below the last row. Reading the LazyVStack's total height forces every row (visible and offscreen) to be placed, defeating virtualization, and writing the preference during layout fed a non-converging relayout loop. The existingisEquivalentdedup only stopped the@Statere-write, not the full-list placement that fired on every agent-driven row update.Fix
Replace the measure→
@State→emptyAreaHeightround-trip with a small customLayout,SidebarRowsFillLayout, that places the rows at their natural height and stretches the empty area to fill the remaining space in one geometry pass. It derives the remainder from its own concretebounds(the parent.frame(minHeight:)resolves to the viewport during placement), so the rows are never measured into SwiftUI state. This removes the preference-write-during-layout feedback loop and lets theLazyVStackvirtualize again.It is #3241-safe by construction: the previous overflow (#3241) came from
Color.clear.frame(maxHeight: .infinity)inside theScrollView(a floor.frame(minHeight:)does not cap an infinite child).SidebarRowsFillLayoutonly ever places into finitebounds, so when the rows fit, rows + empty area exactly fill the viewport (no overflow, overlay scroller stays hidden); when the rows overflow, the empty area is0and the document view genuinely scrolls.Removed:
SidebarWorkspaceRowsHeightPreferenceKey,SidebarWorkspaceRowsMeasurement,@State workspaceRowsMeasurement, theonPreferenceChange/reset wiring, andSidebarWorkspaceScrollLayout.emptyAreaHeight. AddedSidebarWorkspaceScrollLayout.emptyAreaFillHeight(containerHeight:rowsHeight:)(the same remainder math, now driven by the layout's bounds) with unit coverage.SidebarRowsFillLayoutlives in its own file to keepContentView.swiftunder its length budget.Principled, not a workaround: it eliminates the whole class of preference-feedback layout loops here rather than damping the symptom, and it respects the repo's snapshot-boundary / no-mutation-in-body rules (it removes a layout-time state write).
This is complementary to #5840 (author austinywang), which removes the per-tick O(N) body dictionary rebuilds (#5832). That fix and this one touch the same hot path but different mechanisms (O(N) body setup vs the non-converging layout loop) and stack cleanly.
Verification
Built a tagged Debug app, opened 33 workspaces, and sampled the main thread:
mach_msg2_trap, zero inplaceSubviews. The original stable/NIGHTLY pegged the main thread at 100% inplaceSubviewseven sitting idle at this scale; here it is idle. (Process CPU is the 33 background terminal renderers, not the main thread.)A behavior-level CI regression test for a SwiftUI relayout loop is not practical (perf assertions are flaky and the loop is a framework-internal placement cost), so automated coverage is the pure fill-math (
SidebarWorkspaceScrollLayoutTests). The runtime win is best confirmed by dogfooding on a real many-surface session.No user-facing strings changed (no localization needed).
Note: an
issue-3241-legacy-scrollersbranch is in flight on origin and may touch the same scroller/overflow area; worth coordinating to avoid conflicts.Closes #5764. Contributes to #2586, #5570.
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Changes core sidebar scroll layout and empty-area sizing/drop targets; behavior is well-tested for fill math but SwiftUI layout regressions (scroll, drops) are possible.
Overview
Fixes sidebar 100% CPU / relayout livelock (#2586, #5764) by stopping whole-
LazyVStackheight measurement from flowing into@State.Removed the
GeometryReader+SidebarWorkspaceRowsHeightPreferenceKey+SidebarWorkspaceRowsMeasurementpipeline, related@State/onPreferenceChange/onChangewiring, and the fixedminimumHeightonSidebarEmptyArea.Added
SidebarRowsFillLayout, which lays out workspace rows at natural height and sizes the empty drop/tap area from layout bounds in one pass (viaemptyAreaFillHeight(containerHeight:rowsHeight:)), soLazyVStackcan virtualize again and the viewport still fills without phantom scrollbars (#3241).Unit tests now cover the fill math and
contentMinHeight; measurement/dedup tests were dropped with the old types.Reviewed by Cursor Bugbot for commit 67fa5be. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Restores sidebar
LazyVStackvirtualization and fixes a 100% CPU relayout loop by removing whole-content height measurement. Empty drop/tap area is now sized via a custom layout, eliminating the hang with many workspaces and keeping the overlay scroller hidden when rows fit.@Stateempty-area sizing withSidebarRowsFillLayout, which fills remaining space from layout bounds in one pass.SidebarWorkspaceRowsHeightPreferenceKeyandSidebarWorkspaceRowsMeasurement; no preference writes during layout.SidebarWorkspaceScrollLayouttoemptyAreaFillHeight(containerHeight:rowsHeight:); added a test for the zero-workspaces case to ensure the empty area fills the viewport (no phantom scrollbar).Written for commit 67fa5be. Summary will update on new commits.
Summary by CodeRabbit