Repository navigation
Fix sidebar scroller visibility - #3570
Conversation
The workspace sidebar now has UI coverage for both sides of the issue: a one-workspace sidebar should not expose a vertical scroller, while an overflowing workspace list should expose one after scroll interaction. The test scopes scrollbar detection to the Sidebar accessibility frame so terminal scrollbars cannot satisfy the assertion. Constraint: Issue #3555 regressed after sidebar edge-fade chrome changed the scroll container behavior Constraint: Local tests and local app launch are disallowed before CI per task instructions Rejected: Source-shape test around safeAreaInset | it would not prove the user-visible scrollbar behavior Confidence: medium Scope-risk: narrow Tested: git diff --check Not-tested: Local UI tests not run per task instruction
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughUpdates sidebar workspace scrolling to derive top/bottom insets from a new ChangesSidebar Scroller Visibility
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
Caution Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional.
❌ Failed checks (1 error, 1 warning, 1 inconclusive)
✅ Passed checks (10 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 |
b6ce3e3 to
20f9cf4
Compare
Greptile SummaryThis PR fixes sidebar scroller visibility by routing the
Confidence Score: 5/5Safe to merge — the scroller visibility fix is complete, all three previously broken call sites are updated, and no new isolation or layout regressions are introduced. The change is a focused refactor: one deleted computed property is replaced by a value-type centralizer, and the three usage sites are updated consistently. The static let workspaceList snapshot is built entirely from static let constants so it is never stale. The regression test is well-structured and the hover/timing flakiness raised in the prior review has been addressed. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant CV as ContentView(workspaceScrollArea)
participant SI as SidebarWorkspaceScrollInsets.workspaceList
participant SL as SidebarWorkspaceScrollLayout
participant GR as GeometryReader
participant SV as ScrollView
CV->>SI: let scrollInsets = .workspaceList
Note over SI: top = scrollTopInset, bottom = bottomScrimHeight
CV->>GR: GeometryReader { proxy in ... }
GR->>SL: contentMinHeight(viewportHeight: proxy.size.height, insets: scrollInsets)
SL-->>GR: max(0, height - insets.total)
GR->>SV: workspaceScrollContent(minHeight: ...)
GR->>SV: .safeAreaInset(.top) → scrollInsets.top
GR->>SV: .safeAreaInset(.bottom) → scrollInsets.bottom
GR->>SV: drag overlay height → scrollInsets.top + 8
Reviews (7): Last reviewed commit: "Stop sidebar chrome from forcing scroll ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| sidebar.coordinate(withNormalizedOffset: CGVector(dx: 0.97, dy: 0.5)).hover() | ||
| sidebar.swipeUp() |
There was a problem hiding this comment.
macOS overlay scrollbars only appear when the pointer is over the scroll area. sidebar.coordinate(withNormalizedOffset:).hover() simulates this, but in a headless or VNC CI environment the hover event may not trigger NSScroller visibility before swipeUp() is dispatched. The 4-second poll timeout in waitForSidebarVerticalScrollerVisible gives some slack, but if the scrollbar never becomes hittable the test will time-out and emit a false negative. Consider adding a short pollUntil after the hover to confirm at least one scroller is hittable before swiping, or add a known-flaky annotation on this assertion.
There was a problem hiding this comment.
Addressed in ff7be2b: the scroller reveal path now hovers first, polls briefly for a visible scroller, then swipes and polls again instead of relying on a single hover-then-swipe timing assumption.
— Claude Code
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
20f9cf4 to
789bb2d
Compare
789bb2d to
cd9ba95
Compare
![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 `@cmuxUITests/WorkspaceSidebarScrollUITests.swift`:
- Around line 63-77: The test currently assumes 20 workspaces will induce
overflow; change the logic to programmatically add workspaces until the sidebar
scroller becomes visible or a safe upper bound is reached (e.g.,
MAX_WORKSPACES), using the same keystroke sequence (app.typeKey("n",
modifierFlags: [.command])) and reusing waitForWorkspaceRowHittable after each
addition; stop the loop early when
waitForSidebarVerticalScrollerVisible(sidebar: sidebar, app: app, timeout: 1.0)
returns true, then assert that the scroller is visible (fail if the upper bound
is reached without overflow) so the precondition is deterministic. Ensure
references to workspaceCount are replaced by the loop and the boolean check
using waitForSidebarVerticalScrollerVisible; keep existing helper calls like
waitForWorkspaceRowHittable to verify each new row is hittable.
🪄 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: 8710b9f3-27fb-43d6-afec-4518e068e95c
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/WindowChromeMetrics.swiftcmuxUITests/WorkspaceSidebarScrollUITests.swift
cd9ba95 to
ff7be2b
Compare
ff7be2b to
aa76578
Compare
aa76578 to
14811e6
Compare
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 42-63: Remove the unsupported `nonisolated` type-level qualifiers
used on SidebarWorkspaceScrollInsets and SidebarWorkspaceScrollLayout: either
update the project Swift toolchain to 6.1+ to keep SE-0449 syntax, or delete the
`nonisolated` keywords on the struct and enum declarations and, if needed, mark
individual members (e.g., the `total` computed property or `contentMinHeight`
method) with appropriate isolation attributes such as `nonisolated` on the
member or `@MainActor` to preserve the intended concurrency behavior; ensure
references to SidebarWorkspaceScrollInsets, its properties (top, bottom, total),
and SidebarWorkspaceScrollLayout.contentMinHeight compile under Swift 5.0 after
the change.
🪄 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: 3b076dd7-b134-4856-bf66-199061e404f4
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/WindowChromeMetrics.swiftcmuxUITests/WorkspaceSidebarScrollUITests.swift
14811e6 to
b7b416b
Compare
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 43-49: The stored properties on SidebarWorkspaceScrollInsets are
incorrectly annotated with nonisolated; remove the nonisolated attribute from
the static stored property workspaceList and from the instance stored properties
top and bottom so they are plain stored properties, leaving nonisolated only on
the computed property total and the static method contentMinHeight if present;
update declarations for workspaceList, top, and bottom accordingly in the
SidebarWorkspaceScrollInsets type.
🪄 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: 377134d2-3c0c-4260-9ec5-a64fb4f25128
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/WindowChromeMetrics.swiftcmuxUITests/WorkspaceSidebarScrollUITests.swift
The sidebar edge-fade restore added top and bottom safe-area chrome to the workspace scroll view, but the document baseline still filled the full viewport. That made decorative chrome count as extra scrollable height, so AppKit exposed a vertical scroller even when one workspace fit. This introduces one scroll-layout policy for the workspace sidebar: safe-area chrome still owns the titlebar/footer clearance, while the document's fit baseline is the visible viewport minus that chrome. Short workspace lists now fit exactly; long lists still overflow through the same ScrollView. Constraint: Issue #3555 reproduces on 0.64.1 and NIGHTLY after the sidebar edge-fade update Constraint: Local tests/builds are disallowed before CI by task instruction Rejected: Force autohidesScrollers in the resolver | the document would still report overflow and the bug would return under always-show scrollbars Rejected: Remove bottom clearance entirely | bottom rows could disappear behind footer chrome Confidence: medium Scope-risk: narrow Directive: Keep sidebar decorative scroll chrome and document fit height derived from one policy; do not add independent safe-area spacers without updating the fit baseline Tested: git diff --check Not-tested: Local build/UI tests not run per task instruction; remote E2E and PR CI pending
b7b416b to
acf7eac
Compare
This branch was successfully deployed
1 participant
Fixes #3555.
Summary
Commit Structure
b31e8199adds the failing regression test proving the sidebar scroller must hide when workspace content fits and appear after overflow.b7b416b6fixes the scroll container invariant and makes the overflow portion of the regression test probe only after a measured dynamic threshold.Verification
Summary by CodeRabbit
Note
Medium Risk
Adjusts sidebar scroll insets/min-height calculations that affect workspace list overflow and drag/drop hit areas, so regressions could impact scrolling or drop targets across window sizes. Adds UI coverage to reduce risk but behavior is still UI-layout sensitive.
Overview
Restores workspace sidebar scrollbar visibility to track real list overflow by centralizing top/bottom scroll insets and using them when computing the scroll content minimum height (instead of implicitly including decorative chrome).
Refactors
ContentView.workspaceScrollAreato use the newSidebarWorkspaceScrollInsets/SidebarWorkspaceScrollLayouthelpers for safe-area insets and the top drag/drop drop-zone height.Adds
WorkspaceSidebarScrollUITests.testSidebarScrollerVisibilityFollowsWorkspaceOverflowto assert the scroller is hidden when content fits and becomes visible once enough workspaces are created, using a dynamic probe threshold to reduce flakiness.Reviewed by Cursor Bugbot for commit acf7eac. Bugbot is set up for automated code reviews on this repo. Configure here.