Repository navigation
Fix sidebar row-height layout-feedback livelock (#6556) - #6826
austinywang wants to merge 6 commits into
Conversation
`SidebarWorkspaceGroupHeaderView` and `ContentView`'s `TabItemView` each measured their row height with a `GeometryReader` whose `onAppear`/`onChange` closures wrote `@State rowHeight` from inside the layout pass. Writing layout-derived `@State` mid-layout retriggers the SwiftUI AttributeGraph and reproduces the `StackLayout.sizeThatFits` / `ViewLayoutEngine.sizeThatFits` re-render livelock documented in #2586 / #5764 / #6556 (a `cpu_resource.diag` showed 52% CPU sustained over 173s under high workspace counts). PR #6111 intended to remove this pattern, but #4385 re-introduced it in the group header and the `TabItemView` wiring was never updated. A prior attempt (#6558) replaced the measured height with an arithmetic estimate and was reverted (#6625) because the estimate diverged from real content and made rows render too wide/tall. This redo keeps the exact measured height — so drag/drop pointer-edge metrics and visual layout are unchanged — but moves the write out of the layout pass using the established preference-key pattern (matches `BrowserAddressBarHeightPreferenceKey`): a passive `GeometryReader` publishes the height through `SidebarRowHeightPreferenceKey`, and `onPreferenceChange` applies it after layout settles. No `@State` is written from inside a `GeometryReader` anymore. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Warning Review limit reached
Next review available in: 1 minute Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (4)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Greptile SummaryThis PR fixes a SwiftUI layout-feedback livelock (#6556) in which
Confidence Score: 5/5Safe to merge — the change is a focused structural fix that eliminates mid-layout @State writes without altering any observable row height values or drag/drop behavior. The fix correctly replaces the problematic GeometryReader→@State write pattern with the standard SwiftUI preference-key approach, mirroring an established pattern already in the codebase. The new helper file is small and well-documented, actor isolation is unaffected (onPreferenceChange always fires on the main actor), and the Xcode project wiring is correct. No existing behavior is changed. No files require special attention. Important Files Changed
Sequence Diagram%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
participant Row as TabItemView/GroupHeaderView
participant GR as GeometryReader background
participant PK as SidebarRowHeightPreferenceKey
participant OPC as onPreferenceChange
participant State as rowHeight State
Note over Row,State: OLD path (livelock)
Row->>GR: layout pass
GR-->>State: onAppear/onChange writes rowHeight mid-layout
State-->>Row: State change retriggers AttributeGraph
Row->>GR: layout pass again
Note over Row,State: NEW path (this PR)
Row->>GR: layout pass (GR in background, passive)
GR->>PK: preference set with proxy.size.height
PK-->>OPC: preference bubbles up after layout settles
OPC-->>State: "rowHeight = max(height, 1) post-layout"
%%{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"}}}%%
sequenceDiagram
participant Row as TabItemView/GroupHeaderView
participant GR as GeometryReader background
participant PK as SidebarRowHeightPreferenceKey
participant OPC as onPreferenceChange
participant State as rowHeight State
Note over Row,State: OLD path (livelock)
Row->>GR: layout pass
GR-->>State: onAppear/onChange writes rowHeight mid-layout
State-->>Row: State change retriggers AttributeGraph
Row->>GR: layout pass again
Note over Row,State: NEW path (this PR)
Row->>GR: layout pass (GR in background, passive)
GR->>PK: preference set with proxy.size.height
PK-->>OPC: preference bubbles up after layout settles
OPC-->>State: "rowHeight = max(height, 1) post-layout"
Reviews (3): Last reviewed commit: "Merge remote-tracking branch 'origin/mai..." | Re-trigger Greptile |
| /// Publishes a sidebar row's measured height up to an ancestor so the value can | ||
| /// be consumed in `onPreferenceChange` — *after* the layout pass — instead of | ||
| /// being written into `@State` from inside a `GeometryReader` during layout. | ||
| /// | ||
| /// Writing layout-derived `@State` mid-layout retriggers the SwiftUI | ||
| /// AttributeGraph and reproduces the `StackLayout.sizeThatFits` / | ||
| /// `ViewLayoutEngine.sizeThatFits` re-render livelock documented in | ||
| /// https://github.com/manaflow-ai/cmux/issues/2586 and | ||
| /// https://github.com/manaflow-ai/cmux/issues/6556. The measured height only | ||
| /// feeds drag/drop hit metrics, so reporting it through a preference keeps the | ||
| /// exact pointer-edge behavior while keeping the LazyVStack row free of | ||
| /// layout-pass state writes. | ||
| struct SidebarRowHeightPreferenceKey: PreferenceKey { |
There was a problem hiding this comment.
The utility types
SidebarRowHeightPreferenceKey and sidebarRowHeightProbe() are defined at the bottom of SidebarWorkspaceGroupHeaderView.swift but consumed by both this file and ContentView.swift. Keeping shared infrastructure in a view-named file makes the dependency direction implicit: ContentView.swift silently depends on a utility defined inside another view's file. The analogous BrowserAddressBarHeightPreferenceKey is private and lives entirely within its own file. A SidebarRowHeightHelpers.swift (or folding these into a SidebarMetrics/SidebarShared file that already exists) would make the ownership explicit and keep SidebarWorkspaceGroupHeaderView.swift focused on its view.
| /// Publishes a sidebar row's measured height up to an ancestor so the value can | |
| /// be consumed in `onPreferenceChange` — *after* the layout pass — instead of | |
| /// being written into `@State` from inside a `GeometryReader` during layout. | |
| /// | |
| /// Writing layout-derived `@State` mid-layout retriggers the SwiftUI | |
| /// AttributeGraph and reproduces the `StackLayout.sizeThatFits` / | |
| /// `ViewLayoutEngine.sizeThatFits` re-render livelock documented in | |
| /// https://github.com/manaflow-ai/cmux/issues/2586 and | |
| /// https://github.com/manaflow-ai/cmux/issues/6556. The measured height only | |
| /// feeds drag/drop hit metrics, so reporting it through a preference keeps the | |
| /// exact pointer-edge behavior while keeping the LazyVStack row free of | |
| /// layout-pass state writes. | |
| struct SidebarRowHeightPreferenceKey: PreferenceKey { | |
| /// Publishes a sidebar row's measured height up to an ancestor so the value can | |
| /// be consumed in `onPreferenceChange` — *after* the layout pass — instead of | |
| /// being written into `@State` from inside a `GeometryReader` during layout. | |
| /// | |
| /// Writing layout-derived `@State` mid-layout retriggers the SwiftUI | |
| /// AttributeGraph and reproduces the `StackLayout.sizeThatFits` / | |
| /// `ViewLayoutEngine.sizeThatFits` re-render livelock documented in | |
| /// https://github.com/manaflow-ai/cmux/issues/2586 and | |
| /// https://github.com/manaflow-ai/cmux/issues/6556. The measured height only | |
| /// feeds drag/drop hit metrics, so reporting it through a preference keeps the | |
| /// exact pointer-edge behavior while keeping the LazyVStack row free of | |
| /// layout-pass state writes. | |
| // TODO: Move to a shared sidebar utilities file (e.g. SidebarRowHeightHelpers.swift) | |
| // so ContentView.swift's dependency on this type is explicit. | |
| struct SidebarRowHeightPreferenceKey: PreferenceKey { |
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
There was a problem hiding this comment.
Done in b605f9f — moved SidebarRowHeightPreferenceKey + sidebarRowHeightProbe() into a dedicated Sources/SidebarRowHeightPreferenceKey.swift (wired into the Xcode project) so the shared dependency from both SidebarWorkspaceGroupHeaderView and ContentView is explicit. Kept the two tightly-coupled members co-located in the type-named file rather than a generic Helpers file.
— Claude Code
There was a problem hiding this comment.
Addressed by removing the obsolete SidebarRowHeightPreferenceKey/sidebarRowHeightProbe diff after syncing with main. The branch diff against current origin/main is now empty, so this helper no longer exists in the PR.
— Claude Code
This comment has been minimized.
This comment has been minimized.
Addresses the cmux Aziz file-organization policy and Greptile P2 review: `SidebarRowHeightPreferenceKey` + `View.sidebarRowHeightProbe()` are consumed by both `SidebarWorkspaceGroupHeaderView` and `ContentView`'s `TabItemView`, so housing them at the bottom of a view file left the cross-file dependency implicit. Move them into a dedicated `SidebarRowHeightPreferenceKey.swift` (named for the major type, with its tightly-coupled `View` helper co-located) and wire the file into the Xcode project. No behavior change. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
This PR is now obsolete: #7117 (merged Jun 30) deleted the |
…kspacegroupheaderview-tabitemview-s # Conflicts: # Sources/ContentView.swift # Sources/SidebarWorkspaceGroupHeaderView.swift # cmux.xcodeproj/project.pbxproj
|
@lawrencecchen agreed. I removed the obsolete row-height probe diff after syncing with main; the branch now has an empty diff against current |
…kspacegroupheaderview-tabitemview-s
…kspacegroupheaderview-tabitemview-s
|
cmux-reconcile: close-candidate Proposed action: Close this empty PR without merging; preserve the branch. Evidence checked September 18, 2026: GitHub reports 0 changed files, 0 additions, and 0 deletions. I independently fetched the PR diff and it is empty. Head: There is no remaining patch in this PR against its target branch. This does not establish that the original feature shipped to Recheck the head/diff before acting in case new work arrives. Search |
|
I have read the CLA Document v2.2 and I hereby sign the CLA Warning 1 commit in this PR was authored by an email address that is not linked to any GitHub user, so we cannot tell whether the author has signed the CLA. Unlinked author:
To unblock this PR, do one of the following:
|
|
Fleet instruction update for head |
|
Closing as already on main: merging this branch into main at 8421357 produces main's own tree, so there's nothing left to land. The branch is kept; reopen if something here is still missing. Part of the backlog cleanup in manaflow-ai/cmuxterm-hq#563. |
Fixes #6556
Summary
SidebarWorkspaceGroupHeaderViewandContentView'sTabItemVieweach measured their row height with aGeometryReaderwhoseonAppear/onChangeclosures wrote@State rowHeightfrom inside the layout pass. Writing layout-derived@Statemid-layout retriggers the SwiftUI AttributeGraph and reproduces theStackLayout.sizeThatFits/ViewLayoutEngine.sizeThatFitsre-render livelock from the #6556 diagnostic (52% CPU sustained over 173 s under high workspace counts), the same class as #2586 / #5764 / #5845.PR #6111 intended to remove this pattern, but #4385 re-introduced it in the group header and the
TabItemViewwiring was never updated — exactly the still-live paths the issue enumerates.Why this differs from the reverted #6558
The first attempt (#6558) took the issue's option (1): replace the measured height with a pure arithmetic estimate (
SidebarWorkspaceRowDropMetrics.dropTargetHeight). That was reverted (#6625) because the estimate diverged from real rendered content and made rows/tab pills render too wide/tall.This PR takes the issue's option (2) instead: keep the exact measured height — so drag/drop pointer-edge metrics (
edgeForPointer, the top/bottom-half insertion split) and visual layout are byte-for-byte unchanged — but move the@Statewrite out of the layout pass.Change
SidebarRowHeightPreferenceKey+ aView.sidebarRowHeightProbe()helper, mirroring the existing in-repoBrowserAddressBarHeightPreferenceKeypattern inSources/Panels/BrowserPanelView.swift.GeometryReaderpublishes the measured height through the preference;.onPreferenceChange(...)applies it to@State rowHeightafter layout settles.GeometryReaderwrites@Stateduring layout anymore.rowHeightis still consumed by the drop-delegate factories exactly as before, so pointer-edge drop behavior is preserved.Net diff is a reduction in
ContentView.swiftand a small shared helper added toSidebarWorkspaceGroupHeaderView.swift.Verification
python3 scripts/swift_file_length_budget.py --budget .github/swift-file-length-budget.tsv→Swift file length budget respected.@Statewrite inside aGeometryReader) is the fix. Per task instruction, local builds/tests were not run — CI is the verification gate.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fixes the sidebar row-height layout livelock by moving height measurement out of the layout pass while keeping exact row height and drag/drop behavior unchanged.
Bug Fixes
GeometryReaderstate writes with a preference-key pattern: addedSidebarRowHeightPreferenceKeyand.sidebarRowHeightProbe()..onPreferenceChangeafter layout (setsrowHeight = max(height, 1)); no@Statewrites during layout.TabItemViewandSidebarWorkspaceGroupHeaderViewto use the probe; resolves the re-render loop in SidebarWorkspaceGroupHeaderView + TabItemView still have live @State rowHeight + GeometryReader layout-feedback path after v0.64.16 #6556 under heavy workspace counts.Refactors
SidebarRowHeightPreferenceKeyandView.sidebarRowHeightProbe()intoSidebarRowHeightPreferenceKey.swift.Written for commit 9c23670. Summary will update on new commits.