Repository navigation
Fix sidebar scrollbar: hide when content fits, fade overlay knob when idle (#3241) - #4767
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughMeasures workspace rows height, computes contentMinHeight and emptyAreaHeight with SidebarWorkspaceScrollLayout, detects overflow, configures NSScrollView scroller visibility consistently, adapts SidebarEmptyArea to fixed/minimum heights, and adds tests for completeness and viewport edge cases. ChangesSidebar Scrollbar Overflow Detection & Layout
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 16 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (16 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
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/WindowChromeMetrics.swift`:
- Around line 64-70: The static function emptyAreaHeight in WindowChromeMetrics
is missing a return for the computed value; update nonisolated static func
emptyAreaHeight(contentMinHeight:rowsHeight:) so that after the guard you
explicitly return the computed CGFloat (e.g., add "return" before the max(...)
expression), keeping the existing guard case that returns 0 when rowsHeight is
nil.
🪄 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: d0ce901a-1b67-40dd-94e0-6db6f29c31fe
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/WindowChromeMetrics.swiftcmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift
Greptile SummaryFixes two sidebar scrollbar bugs from #3241: a phantom scrollbar when the workspace list fits the viewport (caused by
Confidence Score: 5/5Safe to merge — both bug fixes are deterministic, the measurement-to-layout feedback loop converges in one pass by design, and the stable-config no-op-on-reapply invariant is directly unit-tested. The phantom-scrollbar fix replaces an unbounded empty area with a finite measured remainder, which is straightforward and fully unit-tested at the layout layer. The never-fades fix eliminates re-render-driven NSScrollView property churn via guard-before-write, also directly tested. The single whole-content GeometryReader pattern avoids the per-row preference aggregation that caused the #2586 livelock, and sub-pixel jitter is deduped so @State is not written on every agent-driven re-render. No pre-existing invariants are weakened. No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["workspaceRows (ForEach + padding)"] --> B["GeometryReader in .background measures rows container height"]
B --> C["SidebarWorkspaceRowsHeightPreferenceKey emits SidebarWorkspaceRowsMeasurement"]
C --> D{"onPreferenceChange isEquivalent dedup?"}
D -->|"different (> 0.5pt)"| E["@State workspaceRowsMeasurement updated"]
D -->|"equivalent (≤ 0.5pt)"| F["skip write — no @State mutation (livelock-safe)"]
E --> G["emptyAreaHeight = max(0, contentMinHeight − rowsHeight)"]
H["GeometryReader viewport size.height"] --> G
G --> I["SidebarEmptyArea minHeight: emptyAreaHeight (finite, not infinity)"]
I --> J{"rows fit viewport?"}
J -->|"Yes: rows + emptyArea == viewport"| K["document == viewport — no overflow — scroller hidden"]
J -->|"No: rows > contentMinHeight"| L["emptyArea = 0 — document overflows — real scroll"]
M["SidebarScrollViewResolver fires on every SwiftUI re-render"] --> N["SidebarScrollViewConfigurator.apply"]
N --> O{"each property already correct?"}
O -->|"no drift"| P["no-op write skipped — fade timer uninterrupted"]
O -->|"drifted"| Q["minimal write to restore overlay config"]
P --> R["AppKit owns overlay knob appear / idle fade"]
Q --> R
Reviews (13): Last reviewed commit: "merge: resolve ContentView conflict with..." | Re-trigger Greptile |
| emptyAreaHeight: emptyAreaHeight | ||
| ) | ||
| } | ||
| .scrollIndicators(.automatic) |
There was a problem hiding this comment.
.scrollIndicators(.automatic) is inconsistent with the extension sidebar and redundant
.automatic is the SwiftUI default, so adding it here is a no-op at the SwiftUI level. More importantly, the extension sidebar's ScrollView doesn't carry this modifier even though both sidebars then manually override hasVerticalScroller via SidebarScrollViewResolver. The inconsistency makes the intent unclear — if the goal is to suppress SwiftUI's own indicator overlay before the NSScrollView override fires, .never would be the correct choice; if there's no intent, the modifier should be removed. Either way both sidebars should agree.
| .scrollIndicators(.automatic) | |
| .scrollIndicators(.never) |
There was a problem hiding this comment.
Fixed in b59e4a3 by removing the redundant workspace .scrollIndicators(.automatic) modifier so the workspace and extension sidebars both rely on the shared NSScrollView configuration path.
— Claude Code
…rollbar-always-visible-should
…rollbar-always-visible-should
…rollbar-always-visible-should
…rollbar-always-visible-should # Conflicts: # Sources/ContentView.swift
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 41501bf. Configure here.
#5708 removed SidebarWorkspaceRowIdsPreferenceKey, its per-row .preference emitters, the sidebar-wide onPreferenceChange aggregation, and laidOutWorkspaceRowIds — the cmux-owned edge of the #2586 sidebar layout livelock — and made selected-workspace scroll-into-view call scrollTo unconditionally. This branch's overflow fix had layered a row-layout *completeness* gate on top of that same removed per-row IDs aggregation. Reconcile by dropping the completeness machinery entirely (it depended on the deleted key) and keeping only: - a single whole-content GeometryReader that measures the rows container height once (not a per-row preference reduce), deduped by SidebarWorkspaceRowsMeasurement.isEquivalent so sub-pixel jitter from agent-driven row re-renders never writes @State — so it cannot re-feed the #2586 transaction cycle; and - the finite empty-area height that fixes the original #3241 phantom scrollbar. Also simplify SidebarWorkspaceScrollLayout (drop rowsOverflow, rowsLayoutCompleteness, and the completeness enum), restore the ghostty pointer to origin/main, and update the layout tests for the simplified API. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Issue #3241 has two symptoms. The merge reconciliation fixed the first (phantom scrollbar when content fits, via the finite empty-area height). This addresses the second, reported live on macOS 26.4.1 with ~139 workspaces and many active agents: the overlay scroller is *always* visible and never fades, even though the content legitimately overflows. Root cause (code-level): the sidebar reconfigured the backing NSScrollView on every SwiftUI re-render — toggling `hasVerticalScroller` from a re-render-driven overflow flag (and, in the extension sidebar, from a bounds-change observer). Those re-renders fire constantly while agents update workspace rows (badges, titles, PR sub-rows). Re-adding / re-styling the scroller re-shows the overlay knob and restarts its idle fade timer, so with continuous activity the knob never reaches the timeout and never fades. The autohide/overlay flags were correct; the *churn* was the defect. Fix: apply one stable configuration — `scrollerStyle = .overlay`, `autohidesScrollers = true`, `hasVerticalScroller = true` — and never toggle it. AppKit then owns visibility natively: the knob appears on scroll or real overflow and fades when idle, and stays hidden when the content fits (guaranteed by the finite empty-area height). Forcing `.overlay` also overrides a system "always show scroll bars" setting, so no permanent track is reserved either. This removes the re-render/bounds-driven reconfiguration entirely: the `contentOverflows` helper, `configureSidebarScrollViewFromDocument`, and the `observesLayoutChanges` observer infrastructure (all PR-introduced) are deleted, restoring SidebarScrollViewResolver to its upstream shape. The single whole-content rows measurement is kept only to size the empty area. Note: the fade itself is AppKit/NSScrollerImp animation behavior and is not cleanly unit-testable (NSScroller.alphaValue does not reflect the overlay knob's internal fade), so it is covered by platform-behavior reasoning + a real-app visual check rather than a fabricated test, per the test-quality policy. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
The reconciliation + stable-scroller redesign nets ~+103 lines in ContentView.swift and pushes two test files over their tracked budgets. Trim the scroller-config doc comment and record the accepted growth: - Sources/ContentView.swift 19052 -> 19155 - cmuxTests/WorkspaceGroupTests.swift 853 -> 860 (rowWorkspaceId coverage) - add cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift 557 (now >500 with the layout tests) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…g for the scroll-layout tests Aziz file-organization policy: SidebarWorkspaceRowsMeasurement and SidebarWorkspaceRowsHeightPreferenceKey move to their own TypeName.swift files instead of growing WindowChromeMetrics.swift and ContentView.swift. Aziz test-framework policy: the new SidebarWorkspaceScrollLayoutTests become a Swift Testing suite in their own file, restoring SidebarWorkspaceSnapshotRefreshPolicyTests.swift to its pre-PR XCTest-only state. No behavior change; focused cmux-unit run passes the new suite 5/5. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Empty commit; tree is identical to b43052b. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ing) Tree remains identical to b43052b. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…lView properties Extracts the sidebar overlay-scroller setup into SidebarScrollViewConfigurator (behavior unchanged: still unconditional writes) and adds a regression test asserting that re-applying the configuration to an already-configured scroll view performs zero property writes. SidebarScrollViewResolver re-resolves on every SwiftUI update, and a same-value scroller write re-tiles the overlay scrollers and can cancel an in-flight knob fade without rescheduling it, leaving the sidebar scrollbar permanently visible. Red commit: the test fails (4 writes per re-apply) until the no-op guards land in the follow-up commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…knob fade SidebarScrollViewConfigurator.apply(to:) now writes each NSScrollView property only when the value actually differs. After the first configuration, the per-SwiftUI-update re-applies from SidebarScrollViewResolver become pure reads, so AppKit fully owns the overlay scroller's appear/scroll/fade lifecycle and an in-flight knob fade can no longer be cancelled by a same-value re-tile — the mechanism that left the sidebar scrollbar permanently visible during interactive workspace creation. Green commit for the regression test added in the previous commit. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Union of both sides in workspaceScrollContent: main's emptyAreaTabDropDelegate(renderContext:) signature plus this branch's finite empty-area sizing (expandsVertically/minimumHeight). Budget TSV trued up for main-side growth (ContentView, TabManager, WorkspaceGroupTests). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

Fixes #3241.
Scope
Issue #3241 has two symptoms in the workspace sidebar. This PR fixes both, and reconciles the fix with #5708 (which removed the per-row preference aggregation that fed the #2586 layout livelock).
Root causes & fixes
1. Phantom scrollbar — finite empty area
The empty drop/tap area below the last row used
maxHeight: .infinity, so the scroll document view always reported more height than the viewport → always "overflowing" → scroller always present. Fixed by sizing the empty area to the finite remaining viewport (emptyAreaHeight = max(0, contentMinHeight - rowsHeight)). When the rows fit, content height == viewport, so the overlay scroller stays hidden. The empty area still fills the blank region as a drop/tap target.2. Never-fades — stable scroller configuration (no churn)
The sidebar reconfigured the backing
NSScrollViewon every SwiftUI re-render, togglinghasVerticalScrollerfrom a re-render-driven overflow flag (and, in the extension sidebar, from a bounds-change observer). Those re-renders fire constantly while agents update workspace rows (badges, titles, PR sub-rows). Re-adding/restyling the scroller re-shows the overlay knob and restarts its idle fade timer, so under continuous activity the knob never reaches the timeout and never fades. The autohide/overlay flags were already correct — the churn was the defect.Fix: apply one stable configuration and never toggle it:
AppKit then owns visibility natively — the knob appears on scroll or real overflow and fades when idle, and stays hidden when the content fits (guaranteed by the finite empty area). Forcing
.overlayalso overrides a system "always show scroll bars" setting, so no permanent track is reserved regardless of the user's preference. The re-render/bounds-driven reconfiguration is removed entirely (contentOverflows,configureSidebarScrollViewFromDocument, and theobservesLayoutChangesobserver infra are deleted;SidebarScrollViewResolveris back to its upstream shape).3. Livelock-safe measurement (reconcile with #5708)
#5708 removed
SidebarWorkspaceRowIdsPreferenceKey, its per-row.preferenceemitters, the sidebar-wideonPreferenceChangereduce, andlaidOutWorkspaceRowIds— the cmux-owned edge of the #2586 sidebar layout livelock — and made selected-workspace scroll-into-view callscrollTounconditionally.This branch had layered a row-layout completeness gate on top of that same removed aggregation. It is dropped entirely. The only measurement kept is a single whole-content
GeometryReaderin the rows container's.backgroundthat emits oneSidebarWorkspaceRowsMeasurement, deduped byisEquivalent(0.5pt tolerance). Why this cannot re-feed the #2586 livelock at 139-workspace scale:formUnionacross every row on every layout pass.@State, so there is no transaction scheduled from inside the preference/layout update cycle.This is the "single whole-content
GeometryReaderrather than per-row preferences" option, which is the livelock-safe shape.Tests
cmuxTests/SidebarWorkspaceSnapshotRefreshPolicyTests.swift(SidebarWorkspaceScrollLayoutTests):The fade itself is AppKit/
NSScrollerImpanimation behavior and is not cleanly unit-testable —NSScroller.alphaValuedoes not reflect the overlay knob's internal fade, and a scripted accessory window can't drive the real fade timer (I verified this with a minimal AppKit harness on the host; the control case didn't fade either, confirming the metric is the wrong seam, not that the fix is wrong). Per the test-quality policy, I did not fabricate a fade test; it is covered by the platform-behavior reasoning above and a real-app visual check.Verification status