Repository navigation
Fix sidebar row-height layout feedback - #6111
Merged
Merged
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughReplaces per-row ChangesSnapshot-based drop-hit height refactor
Sequence Diagram(s)sequenceDiagram
rect rgba(70, 130, 180, 0.5)
Note over TabItemView,SidebarTabDropDelegate: New computed drop-hit height flow
end
TabItemView->>TabItemViewDropSupport: workspaceDropTargetHeight(snapshot, effectiveSubtitle)
TabItemViewDropSupport->>SidebarWorkspaceRowDropMetrics: dropTargetHeight(snapshot, settings, metadataEntryIsExpanded, metadataBlocksAreExpanded)
SidebarWorkspaceRowDropMetrics->>SidebarWorkspaceRowDropMetrics: shouldUsePointerEdgeHeight(wrapsWorkspaceTitles, hasDescription, hasMetadataBlocks)
alt pointer-edge height needed
SidebarWorkspaceRowDropMetrics-->>TabItemViewDropSupport: CGFloat (computed targetHeight)
else not needed
SidebarWorkspaceRowDropMetrics-->>TabItemViewDropSupport: nil
end
TabItemViewDropSupport-->>TabItemView: CGFloat?
TabItemView->>SidebarTabDropDelegate: tabDropDelegateFactory(CGFloat?)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Poem
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (2 errors, 1 warning)
✅ Passed checks (18 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 |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
Contributor
Greptile SummaryThis PR eliminates a main-thread hang in the sidebar by removing
Confidence Score: 5/5Safe to merge; the GeometryReader removal is complete and the drop delegate nil-height fallback was already an established pattern in the codebase. The core change removes layout-feedback state writes from LazyVStack rows, matched by deterministic arithmetic in the new metrics types and backed by pinned tests. The dropTargetHeight guard correctly returns nil for variable-height rows rather than returning an inaccurate estimate, preserving existing pointer-edge fallback behavior. No new concurrency, blocking, or architectural regressions were introduced. Sources/ContentView.swift — the title line-limit cap (nil → 8 lines) and the new description line-limit are user-visible display changes worth a quick review. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[TabItemView.body] --> B{workspaceDropTargetHeight}
B --> C[SidebarWorkspaceRowDropMetrics.dropTargetHeight]
C --> D{shouldUsePointerEdgeHeight?}
D -- wrapsTitle OR hasDesc OR hasMetadataBlocks --> E[return nil]
D -- none of above --> F[targetHeight - pure arithmetic]
F --> G[CGFloat value]
E --> H[tabDropDelegateFactory nil - pointer-edge fallback]
G --> I[tabDropDelegateFactory height - half-row split drop zones]
L[SidebarWorkspaceGroupHeaderView.body] --> M[metrics.dropTargetHeight - always CGFloat]
M --> N[SidebarWorkspaceGroupHeaderDropDelegate targetRowHeight]
Reviews (3): Last reviewed commit: "Fix sidebar row-height layout feedback" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
13297
to
13379
| var pendingWorkspaceSnapshot: SidebarWorkspaceSnapshotBuilder.Snapshot? | ||
| } | ||
|
|
||
| struct SidebarWorkspaceRowDropMetrics { | ||
| static func targetHeight( | ||
| fontScale: CGFloat, | ||
| wrapsWorkspaceTitles: Bool, | ||
| hasDescription: Bool, | ||
| hasSubtitle: Bool, | ||
| hasRemoteStatus: Bool, | ||
| hasMetadataEntries: Bool, | ||
| hasMetadataBlocks: Bool, | ||
| hasLog: Bool, | ||
| hasProgress: Bool, | ||
| hasBranchDirectory: Bool, | ||
| hasPullRequests: Bool, | ||
| hasPorts: Bool | ||
| ) -> CGFloat { | ||
| let scale = max(fontScale, 0.5) | ||
| var height = 16 + (wrapsWorkspaceTitles ? 32 : 16) * scale | ||
| if hasDescription { | ||
| height += 28 * scale | ||
| } | ||
| if hasSubtitle { | ||
| height += 24 * scale | ||
| } | ||
| if hasRemoteStatus { | ||
| height += 18 * scale | ||
| } | ||
| if hasMetadataEntries { | ||
| height += 20 * scale | ||
| } | ||
| if hasMetadataBlocks { | ||
| height += 34 * scale | ||
| } | ||
| if hasLog { | ||
| height += 16 * scale | ||
| } | ||
| if hasProgress { | ||
| height += 16 * scale | ||
| } | ||
| if hasBranchDirectory { | ||
| height += 18 * scale | ||
| } | ||
| if hasPullRequests { | ||
| height += 18 * scale | ||
| } | ||
| if hasPorts { | ||
| height += 16 * scale | ||
| } | ||
| return max(34, ceil(height)) | ||
| } | ||
|
|
||
| static func targetHeight( | ||
| snapshot: SidebarWorkspaceSnapshotBuilder.Snapshot, | ||
| settings: SidebarTabItemSettingsSnapshot, | ||
| effectiveSubtitle: String? | ||
| ) -> CGFloat { | ||
| let visibleDetails = settings.visibleAuxiliaryDetails | ||
| return targetHeight( | ||
| fontScale: settings.sidebarFontScale, | ||
| wrapsWorkspaceTitles: settings.wrapsWorkspaceTitles, | ||
| hasDescription: snapshot.customDescription != nil, | ||
| hasSubtitle: effectiveSubtitle != nil, | ||
| hasRemoteStatus: !settings.hidesAllDetails && settings.showsSSH && snapshot.remoteWorkspaceSidebarText != nil, | ||
| hasMetadataEntries: visibleDetails.showsMetadata && !snapshot.metadataEntries.isEmpty, | ||
| hasMetadataBlocks: visibleDetails.showsMetadata && !snapshot.metadataBlocks.isEmpty, | ||
| hasLog: visibleDetails.showsLog && snapshot.latestLog != nil, | ||
| hasProgress: visibleDetails.showsProgress && snapshot.progress != nil, | ||
| hasBranchDirectory: visibleDetails.showsBranchDirectory && | ||
| (snapshot.compactGitBranchSummaryText != nil || | ||
| !snapshot.compactDirectoryCandidates.isEmpty || | ||
| !snapshot.compactBranchDirectoryCandidates.isEmpty || | ||
| !snapshot.branchDirectoryLines.isEmpty), | ||
| hasPullRequests: visibleDetails.showsPullRequests && !snapshot.pullRequestRows.isEmpty, | ||
| hasPorts: visibleDetails.showsPorts && !snapshot.listeningPorts.isEmpty | ||
| ) | ||
| } | ||
| } | ||
|
|
||
| struct TabItemView: View, Equatable { | ||
| private static let workspaceObservationCoalesceInterval: RunLoop.SchedulerTimeType.Stride = .milliseconds(40) | ||
| private static let legacyVMWebSocketDescription = "VM WebSocket PTY" |
Contributor
There was a problem hiding this comment.
SidebarWorkspaceRowDropMetrics placed in the already-oversized ContentView.swift
SidebarWorkspaceGroupHeaderMetrics has its own dedicated file (SidebarWorkspaceGroupHeaderMetrics.swift), which is the correct pattern for drop-metrics types. Adding 83 lines of a parallel type to ContentView.swift — already at ~16 900 lines — worsens the sprawl that the file-package-boundaries rule flags. Moving SidebarWorkspaceRowDropMetrics to a new SidebarWorkspaceRowDropMetrics.swift would keep it consistent with the existing sibling, independently testable (no import burden from the app target's god-file), and free of any accidental UI coupling.
Rule Used: Flag Swift changes that add too much unrelated res... (source)
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!
![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: f650303f83
ℹ️ 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
+13316
to
+13319
| var height = 16 + (wrapsWorkspaceTitles ? 32 : 16) * scale | ||
| if hasDescription { | ||
| height += 28 * scale | ||
| } |
There was a problem hiding this comment.
When wrapsWorkspaceTitles is enabled with a long title, or a workspace has a multi-line description, this estimate can be far shorter than the rendered row: the title is allowed to wrap without a line limit, and SidebarWorkspaceDescriptionText has no line limit and uses .fixedSize(...). SidebarDropPlanner.edgeForPointer then clamps info.location.y to this smaller targetHeight, so dragging over much of the actual upper half of a tall row is treated as a bottom-edge drop and reorders after the workspace instead of before. Please derive the metric from bounded line counts or otherwise keep the hit height at least as large as the rendered row.
Useful? React with 👍 / 👎.
lawrencecchen
force-pushed
the
feat-sidebar-rowheight-livelock
branch
from
June 14, 2026 08:09
f650303 to
b43386f
Compare
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit b43386f. Configure here.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b43386fdd9
ℹ️ 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".
| !snapshot.compactDirectoryCandidates.isEmpty || | ||
| !snapshot.compactBranchDirectoryCandidates.isEmpty || | ||
| !snapshot.branchDirectoryLines.isEmpty), | ||
| hasPullRequests: visibleDetails.showsPullRequests && !snapshot.pullRequestRows.isEmpty, |
There was a problem hiding this comment.
When a workspace exposes multiple pull requests, the rendered sidebar uses ForEach(workspaceSnapshot.pullRequestRows) to add one visible row per PR, but this metric collapses the whole non-empty array to a single 18pt increment. In that case the synthetic dropTargetHeight can be far shorter than the actual row; SidebarDropPlanner.edgeForPointer then clamps pointer y to the shorter height, so dragging over much of a tall multi-PR row is interpreted as a bottom-edge drop instead of the intended top/center position. Please base this section on the displayed row count or cap the rendered rows to match the metric.
Useful? React with 👍 / 👎.
![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 `@Sources/SidebarWorkspaceGroupHeaderMetrics.swift`:
- Around line 76-80: The dropTargetHeight property in
SidebarWorkspaceGroupHeaderMetrics uses raw fontScale without enforcing a
minimum floor value, while SidebarWorkspaceRowDropMetrics floors fontScale to
0.5. To ensure consistent drop-hit heights between headers and rows, apply the
same 0.5 floor to fontScale in the dropTargetHeight calculation by using
max(0.5, fontScale) before multiplying by 24 in the return statement.
🪄 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: a4e6e987-5c84-4dce-8c00-4e81f91d7bbc
📒 Files selected for processing (6)
Sources/ContentView.swiftSources/SidebarWorkspaceGroupHeaderMetrics.swiftSources/SidebarWorkspaceGroupHeaderView.swiftSources/SidebarWorkspaceRowDropMetrics.swiftcmux.xcodeproj/project.pbxprojcmuxTests/SidebarWorkspaceDropPlannerTests.swift
Comment on lines
+76
to
+80
| /// Stable drop-hit height for the group header, without reading SwiftUI layout. | ||
| var dropTargetHeight: CGFloat { | ||
| let contentHeight = max(chevronFrame, iconFrame, plusFrame, nameFontSize + 4) | ||
| return max(24 * fontScale, contentHeight + 10) | ||
| } |
There was a problem hiding this comment.
Align header scale floor with row drop metrics.
SidebarWorkspaceRowDropMetrics floors fontScale to 0.5, but this header path uses raw fontScale. If settings/config ever provide values below that floor, row/header drop-hit heights become inconsistent.
Suggested fix
/// Stable drop-hit height for the group header, without reading SwiftUI layout.
var dropTargetHeight: CGFloat {
- let contentHeight = max(chevronFrame, iconFrame, plusFrame, nameFontSize + 4)
- return max(24 * fontScale, contentHeight + 10)
+ let scale = max(fontScale, 0.5)
+ let contentHeight = max(
+ Self.baseChevronFrame * scale,
+ Self.baseIconFrame * scale,
+ Self.basePlusFrame * scale,
+ Self.baseNameFontSize * scale + 4
+ )
+ return max(24 * scale, contentHeight + 10)
}🤖 Prompt for 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.
In `@Sources/SidebarWorkspaceGroupHeaderMetrics.swift` around lines 76 - 80, The
dropTargetHeight property in SidebarWorkspaceGroupHeaderMetrics uses raw
fontScale without enforcing a minimum floor value, while
SidebarWorkspaceRowDropMetrics floors fontScale to 0.5. To ensure consistent
drop-hit heights between headers and rows, apply the same 0.5 floor to fontScale
in the dropTargetHeight calculation by using max(0.5, fontScale) before
multiplying by 24 in the return statement.
lawrencecchen
force-pushed
the
feat-sidebar-rowheight-livelock
branch
from
June 14, 2026 08:52
b43386f to
bc784d6
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bc784d6674
ℹ️ 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
+193
to
+198
| guard shouldUsePointerEdgeHeight( | ||
| wrapsWorkspaceTitles: settings.wrapsWorkspaceTitles, | ||
| hasDescription: snapshot.customDescription != nil, | ||
| hasMetadataBlocks: visibleDetails.showsMetadata && !snapshot.metadataBlocks.isEmpty | ||
| ) else { | ||
| return nil |
There was a problem hiding this comment.
With wrapsWorkspaceTitles enabled, or whenever a row has a description/metadata block, this guard returns nil, so the row delegate passes no height to SidebarDropPlanner.indicator; that planner then ignores info.location.y and falls back to source-direction preferredEdge. In those configurations, dragging downward over the top half of a row still targets the bottom edge (and dragging upward over the bottom half still targets the top), so users cannot place the drop based on the pointer on exactly these detailed rows. Fresh evidence in this version is that the new guard disables the bounded targetHeight path rather than using it for these cases.
Useful? React with 👍 / 👎.
This was referenced Jun 21, 2026
This was referenced Jun 22, 2026
lawrencecchen
added a commit
that referenced
this pull request
Jul 3, 2026
…-view guard (#7221) * Extend sidebar lazy-layout guard to the row views (TabItemView, group header) The source-scan guard from #6870 protected only the two container functions, but four of the five historical livelock regressions entered through the row views: the #2586/#6556 GeometryReader -> @State rowHeight probes lived in TabItemView and SidebarWorkspaceGroupHeaderView (removed by #6111, reintroduced by #4385, removed again by #7117) and shipped in stable v0.64.17, which livelocked in the wild on 2026-07-02 with exactly that signature (#2586 (comment)). Scan the TabItemView region of ContentView.swift and the whole group header file for per-row geometry feedback: GeometryReader, onGeometryChange, manual sizeThatFits, ProposedViewSize(nil), per-row anchorPreference/overlayPreferenceValue, and any discovered custom Layout. Rows must stay measurement-free; the only sanctioned geometry path is the container's drag-gated reader. Missing row types fail loudly so a rename cannot rot the guard into a no-op. Verified the extended guard retroactively flags both v0.64.17 row views. New self-test cases (j)-(m) cover clean-pass, the #6556 probe shape, the #5323 anchorPreference shape, and rename protection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Add behavioral scale gate for the sidebar lazy-layout contract The lazy-layout contract (sidebar layout/diff work stays O(visible rows), never O(all workspaces)) has regressed five times through five different mechanisms (#5323 anchorPreference aggregation, #5764 String ids, #5845 animated height interpolation, #6210 force-measuring custom Layout, #6556 GeometryReader -> @State feedback), each shipping to stable before detection because nothing exercises the sidebar at the 100+ workspace scale where O(N) per pass livelocks the main thread (#2586). SidebarLazyLayoutScaleTests mounts the real VerticalTabsSidebar with 300 workspaces in an NSHostingView and counts actual row body evaluations through a DEBUG-only environment probe (SidebarLazyContractProbe, same pattern as MinimalModeInvalidationProbe): - mount must realize only viewport rows (catches any virtualization defeat, present or future, regardless of mechanism) - a 40-burst unread-model storm (the sidebar's highest-frequency whole-body invalidation path) must stay row-scoped and go quiet when the burst stops (catches feedback loops the way #6556 manifested) - a harness canary reproduces the GeometryReader -> @State shape in divergent form and asserts the harness detects it, so the gate cannot silently rot This is the mechanism-independent backstop behind the source-shape scan in scripts/check-sidebar-lazy-layout.py. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix scale-test autorelease avalanche that hung/crashed the app host Creating 300 workspaces inside one main-actor job accumulated every autoreleased object from the O(N)-per-add snapshot work into a single autorelease pool; the closing objc_autoreleasePoolPop then crashed CI (Signal 11 in AutoreleasePoolPage::releaseUntil, masked as a green run, see #5641) and hung for hours when reproduced on an AWS M4 Pro (sampled: main thread pinned in releaseUntil). The app never does this; real workspace creation happens one per event-loop turn with AppKit popping the pool between turns. Make the harness match real cadence: per-iteration autoreleasepool around addWorkspace and a run-loop turn every 20 creations. Also hoist the RunLoop.run call into a synchronous helper (fixes the Swift 6 unavailable-from-async warning) wrapped in its own pool. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix harness NSWindow double-release that killed the app host NSZombies named the corpse: "-[NSKVONotifying_NSWindow release]: message sent to deallocated instance". The harness windows used the NSWindow default isReleasedWhenClosed=true, so tearDown's close() performed AppKit's own release on top of ARC's; the double-release SEGV'd the host at the next autorelease-pool pop, before the pass was recorded, and CI masked the crash as a green run (#5641 (comment)). With zombies absorbing the over-release, all assertions pass in under a second, isolating the crash entirely to window teardown. Set isReleasedWhenClosed = false on both harness windows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Guard --file mode: require container functions only when one is present Addresses Greptile P2 on the PR: --file against a row-view source (no workspaceScrollContent/workspaceRows) emitted false could-not-locate violations that masked real row findings. In --file mode the container checks now apply only when at least one guarded function exists in the source, so ad-hoc row-view scans are clean while a fixture that renamed one function still fails loudly. Self-test cases added for both sides. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Split probe env key + extension into their own files (Aziz policy) One major type per Swift file, matching the MinimalModeInvalidationProbe three-file layout exactly. No content changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Cover the group-header row wrapper (guard target + grouped scale fixture) Codex review found the blind spot: the group-header row is assembled by sidebarWorkspaceGroupHeader(...) in VerticalTabsSidebar+WorkspaceGroups.swift, where modifiers wrap the header before it enters the LazyVStack — a GeometryReader or anchorPreference added there defeats laziness exactly like one inside the row view (the #4385 regression entered through the header path). The guard now scans that whole file for the row-forbidden shapes with a rename-protected marker, and the scale fixture groups the first 20 workspaces into 5 groups so group-header realization and convergence are asserted by the behavioral backstop (bounds on groupHeaderBodies at mount and in the quiet check). Verified on the AWS M4 Pro runner: 3/3 pass in 2.7s, no host restarts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Make the scale harness hermetic against persisted sidebar provider Codex review: VerticalTabsSidebar selects between the workspace list and extension/built-in sidebars via @AppStorage(CmuxExtensionSidebarSelection.defaultsKey), so a host with a persisted non-default provider would mount the wrong sidebar and the probes would never fire. Use a scratch UserDefaults suite pinned to the default provider via .defaultAppStorage, cleaned in tearDown — the WorkspaceContentViewVisibilityTests pattern. Verified on the AWS M4 Pro runner: 3/3 pass in 2.7s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com>
mochiexists
pushed a commit
to mochiexists/cmux-mochi
that referenced
this pull request
Jul 7, 2026
…-view guard (manaflow-ai#7221) * Extend sidebar lazy-layout guard to the row views (TabItemView, group header) The source-scan guard from manaflow-ai#6870 protected only the two container functions, but four of the five historical livelock regressions entered through the row views: the manaflow-ai#2586/manaflow-ai#6556 GeometryReader -> @State rowHeight probes lived in TabItemView and SidebarWorkspaceGroupHeaderView (removed by manaflow-ai#6111, reintroduced by livelocked in the wild on 2026-07-02 with exactly that signature (manaflow-ai#2586 (comment)). Scan the TabItemView region of ContentView.swift and the whole group header file for per-row geometry feedback: GeometryReader, onGeometryChange, manual sizeThatFits, ProposedViewSize(nil), per-row anchorPreference/overlayPreferenceValue, and any discovered custom Layout. Rows must stay measurement-free; the only sanctioned geometry path is the container's drag-gated reader. Missing row types fail loudly so a rename cannot rot the guard into a no-op. Verified the extended guard retroactively flags both v0.64.17 row views. New self-test cases (j)-(m) cover clean-pass, the manaflow-ai#6556 probe shape, the manaflow-ai#5323 anchorPreference shape, and rename protection. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Add behavioral scale gate for the sidebar lazy-layout contract The lazy-layout contract (sidebar layout/diff work stays O(visible rows), never O(all workspaces)) has regressed five times through five different mechanisms (manaflow-ai#5323 anchorPreference aggregation, manaflow-ai#5764 String ids, manaflow-ai#5845 animated height interpolation, manaflow-ai#6210 force-measuring custom Layout, manaflow-ai#6556 GeometryReader -> @State feedback), each shipping to stable before detection because nothing exercises the sidebar at the 100+ workspace scale where O(N) per pass livelocks the main thread (manaflow-ai#2586). SidebarLazyLayoutScaleTests mounts the real VerticalTabsSidebar with 300 workspaces in an NSHostingView and counts actual row body evaluations through a DEBUG-only environment probe (SidebarLazyContractProbe, same pattern as MinimalModeInvalidationProbe): - mount must realize only viewport rows (catches any virtualization defeat, present or future, regardless of mechanism) - a 40-burst unread-model storm (the sidebar's highest-frequency whole-body invalidation path) must stay row-scoped and go quiet when the burst stops (catches feedback loops the way manaflow-ai#6556 manifested) - a harness canary reproduces the GeometryReader -> @State shape in divergent form and asserts the harness detects it, so the gate cannot silently rot This is the mechanism-independent backstop behind the source-shape scan in scripts/check-sidebar-lazy-layout.py. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix scale-test autorelease avalanche that hung/crashed the app host Creating 300 workspaces inside one main-actor job accumulated every autoreleased object from the O(N)-per-add snapshot work into a single autorelease pool; the closing objc_autoreleasePoolPop then crashed CI (Signal 11 in AutoreleasePoolPage::releaseUntil, masked as a green run, see manaflow-ai#5641) and hung for hours when reproduced on an AWS M4 Pro (sampled: main thread pinned in releaseUntil). The app never does this; real workspace creation happens one per event-loop turn with AppKit popping the pool between turns. Make the harness match real cadence: per-iteration autoreleasepool around addWorkspace and a run-loop turn every 20 creations. Also hoist the RunLoop.run call into a synchronous helper (fixes the Swift 6 unavailable-from-async warning) wrapped in its own pool. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Fix harness NSWindow double-release that killed the app host NSZombies named the corpse: "-[NSKVONotifying_NSWindow release]: message sent to deallocated instance". The harness windows used the NSWindow default isReleasedWhenClosed=true, so tearDown's close() performed AppKit's own release on top of ARC's; the double-release SEGV'd the host at the next autorelease-pool pop, before the pass was recorded, and CI masked the crash as a green run (manaflow-ai#5641 (comment)). With zombies absorbing the over-release, all assertions pass in under a second, isolating the crash entirely to window teardown. Set isReleasedWhenClosed = false on both harness windows. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Guard --file mode: require container functions only when one is present Addresses Greptile P2 on the PR: --file against a row-view source (no workspaceScrollContent/workspaceRows) emitted false could-not-locate violations that masked real row findings. In --file mode the container checks now apply only when at least one guarded function exists in the source, so ad-hoc row-view scans are clean while a fixture that renamed one function still fails loudly. Self-test cases added for both sides. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Split probe env key + extension into their own files (Aziz policy) One major type per Swift file, matching the MinimalModeInvalidationProbe three-file layout exactly. No content changes. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Cover the group-header row wrapper (guard target + grouped scale fixture) Codex review found the blind spot: the group-header row is assembled by sidebarWorkspaceGroupHeader(...) in VerticalTabsSidebar+WorkspaceGroups.swift, where modifiers wrap the header before it enters the LazyVStack — a GeometryReader or anchorPreference added there defeats laziness exactly like one inside the row view (the manaflow-ai#4385 regression entered through the header path). The guard now scans that whole file for the row-forbidden shapes with a rename-protected marker, and the scale fixture groups the first 20 workspaces into 5 groups so group-header realization and convergence are asserted by the behavioral backstop (bounds on groupHeaderBodies at mount and in the quiet check). Verified on the AWS M4 Pro runner: 3/3 pass in 2.7s, no host restarts. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * Make the scale harness hermetic against persisted sidebar provider Codex review: VerticalTabsSidebar selects between the workspace list and extension/built-in sidebars via @AppStorage(CmuxExtensionSidebarSelection.defaultsKey), so a host with a persisted non-default provider would mount the wrong sidebar and the probes would never fire. Use a scratch UserDefaults suite pinned to the default provider via .defaultAppStorage, cleaned in tearDown — the WorkspaceContentViewVisibilityTests pattern. Verified on the AWS M4 Pro runner: 3/3 pass in 2.7s. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> (cherry picked from commit 2071529)
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
2 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.
Fixes the nightly sidebar hang path captured from
cmux NIGHTLYafter sidebar interaction.The hang sample was on the main thread in SwiftUI LazyVStack/ForEach placement and AttributeGraph updates. This removes the per-row GeometryReader height probes that wrote back into SwiftUI state from sidebar rows during layout.
Changes:
SidebarWorkspaceGroupHeaderMetrics.Verification:
CMUX_SKIP_ZIG_BUILD=1 xcodebuild -project cmux.xcodeproj -scheme cmux-unit -configuration Debug -destination 'platform=macOS' -derivedDataPath /tmp/cmux-rhl-test -only-testing:cmuxTests/SidebarWorkspaceDropPlannerTests test./scripts/reload-cloud.sh --tag rhlrhlbuild: created workspaces withCMUX_TAG=rhl ./scripts/cmux-debug-cli.sh new-workspace, ranCMUX_TAG=rhl ./scripts/cmux-debug-cli.sh simulate-sidebar-drag --window window:1 --from workspace:3 --to workspace:1 --duration-ms 250 --steps 8, then verified real reorder withCMUX_TAG=rhl ./scripts/cmux-debug-cli.sh reorder-workspace --workspace workspace:3 --before workspace:1 --window window:1and screenshot state via Computer Use.Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Note
Medium Risk
Sidebar drag-and-drop hit zones now rely on heuristics instead of measured row height, so reorder/group-drop UX could drift on complex rows even though the layout-feedback hang risk is reduced.
Overview
Addresses a main-thread SwiftUI hang tied to LazyVStack rows writing measured heights back into state during layout by removing per-row
GeometryReaderheight probes from workspace rows and group headers.Workspace rows now feed tab drop delegates an optional
dropTargetHeightfromSidebarWorkspaceRowDropMetrics, which estimates row height from font scale, title wrapping, subtitles, metadata expansion, and other visible sidebar details instead of live layout. For simple rows (single-line title, no description/metadata blocks), that estimate is passed through; richer rows passnilso drop planning can fall back to pointer-based behavior. Metadata expand/collapse state moves to the parent row so height math stays aligned with what is shown, and description/metadata views gain shared line limits tied to the same metrics.Group headers use a computed
dropTargetHeightonSidebarWorkspaceGroupHeaderMetrics(font-scale–based, no layout read).TabItemViewDropSupportcentralizes close, Finder-open, and snapshot-invalidation helpers that were inlined in the giant view file.Tests in
SidebarWorkspaceDropMetricsTestslock scaling and expansion behavior for row/header drop heights.Reviewed by Cursor Bugbot for commit bc784d6. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes a SwiftUI sidebar hang by eliminating layout-driven height probes and using deterministic, optional drop-target heights for rows and stable heights for group headers. This removes main-thread layout feedback and keeps drag-and-drop stable across font scales and content.
GeometryReaderprobes; rows and headers no longer write layout to state. Headers useSidebarWorkspaceGroupHeaderMetrics.dropTargetHeight.SidebarWorkspaceRowDropMetricsandTabItemViewDropSupport. Row drop delegates now takeCGFloat?; rows pass a height only for width‑independent rows, computed from font scale and visible content (no layout reads).TabItemView. Collapsed limits and max line caps moved into metrics; applied.lineLimitto title, description, and markdown blocks to match the heuristic.SidebarWorkspaceDropMetricsTeststo cover header scaling and row height heuristics (base/rich/scaled, expanded metadata, pointer‑edge gating).Written for commit bc784d6. Summary will update on new commits.
Summary by CodeRabbit
Refactor
Tests