Repository navigation
sidebar scrollbar always visible — should only appear when content overflows - #3277
lawrencecchen wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
Caution Review failedPull request was closed or merged during review 📝 WalkthroughWalkthroughThe sidebar scroll view configuration is enhanced with dynamic behavior: the SwiftUI sidebar gains automatic scroll indicator visibility, while the resolver configures the underlying NSScrollView with overlay-style autohiding scrollers. The vertical scroller now conditionally enables based on content height comparison. Auto-scroll controller attachment is guarded by scroll view availability. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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. Review rate limit: 3/8 reviews remaining, refill in 37 minutes and 18 seconds.Comment |
|
Closing this loader dogfood PR because the downloaded videos show cmux behind the macOS FileVault confirmation sheet. The setup helper now clears the earlier panes, but it switches to hide mode during the Codex run, so late Setup Assistant sheets are not dismissed. I am changing the Codex-run helper to keep clicking Setup Assistant prompts and will rerun from a fresh branch. |
Greptile SummaryThis PR fixes the sidebar scrollbar always being visible by configuring the underlying Confidence Score: 4/5Safe to merge; the fix is self-contained and verified with recorded videos, with only minor style concerns. Only P2 findings: redundant property assignments across two call sites, and a theoretical conflict between Sources/ContentView.swift — specifically the Important Files Changed
Sequence DiagramsequenceDiagram
participant SwiftUI as SwiftUI ScrollView
participant Resolver as SidebarScrollViewResolverView
participant NSScroll as NSScrollView
SwiftUI->>Resolver: updateNSView / viewDidMoveToWindow
Resolver->>Resolver: resolveScrollView() [async main]
Resolver->>NSScroll: hasVerticalScroller = contentHeight > visibleHeight + 1
Resolver->>NSScroll: autohidesScrollers = true
Resolver->>NSScroll: scrollerStyle = .overlay
Resolver->>SwiftUI: onResolve(scrollView)
SwiftUI->>NSScroll: autohidesScrollers = true (duplicate)
SwiftUI->>NSScroll: scrollerStyle = .overlay (duplicate)
SwiftUI->>NSScroll: dragAutoScrollController.attach(scrollView)
Reviews (1): Last reviewed commit: "Address https://github.com/manaflow-ai/c..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| scrollView.autohidesScrollers = true | ||
| scrollView.scrollerStyle = .overlay |
There was a problem hiding this comment.
autohidesScrollers/scrollerStyle configuration
autohidesScrollers = true and scrollerStyle = .overlay are set here in the onResolve callback, but resolveScrollView() already sets those same two properties before calling onResolve. Since the callback runs after resolveScrollView(), these lines simply overwrite values that were just written — harmless but confusing and easy to get out of sync if one site is updated without the other.
| minHeight: geometryProxy.size.height | ||
| ) | ||
| } | ||
| .scrollIndicators(.automatic) |
There was a problem hiding this comment.
.scrollIndicators(.automatic) and manual hasVerticalScroller
.scrollIndicators(.automatic) on macOS defers scroll indicator visibility to the system preference (System Settings → Appearance → Show scroll bars). When the user has that preference set to "Always," SwiftUI may keep hasVerticalScroller = true internally and could re-assert it after resolveScrollView() sets it to false. This might cause the scroller to reappear for users with the legacy "Always show" setting, undermining the fix. Consider using .scrollIndicators(.never) paired with the overlay-style scroller as the safe default, or verifying behavior under the "Always show" scroll bar preference.
This branch was successfully deployed
1 participant
Addresses #3241
Generated by cmux-loader.
Recorded videos:
Run artifacts: https://github.com/manaflow-ai/cmux-loader/actions/runs/25089637151
Codex final message:
Implemented in branch
loader/issue-3241-sidebar-scrollbar-video18for #3241.Summary
I narrowed the fix to the workspace sidebar scroll container in Sources/ContentView.swift:
.scrollIndicators(.automatic)on the sidebarScrollView.autohidesScrollers = truescrollerStyle = .overlaySidebarScrollViewResolverView.resolveScrollView()to compute content/viewport heights and set:hasVerticalScroller = contentHeight > visibleHeight + 1This prevents a vertical scroller when sidebar rows fit in the viewport.
Verification performed
./scripts/reload.sh --tag loader./scripts/reload.sh --tag loader --launch./.cmux-loader/approve-computer-use-app "/Users/runner/Library/Developer/Xcode/DerivedData/cmux-loader/Build/Products/Debug/cmux DEV loader.app"cmux DEV loaderand inspected GUI state via computer-use after rebuild../.cmux-loader/record-video start repro-before 180→./.cmux-loader/record-video stop repro-before(status=ok)./.cmux-loader/record-video start fixed-after 180→./.cmux-loader/record-video stop fixed-after(status=ok)Recorded videos
repro-before.movfixed-after.movIf you want, I can also keep the change minimal by removing the overflow-height guard and relying only on
.overlay + autohide, but this may reintroduce the one-item visible-track behavior on some macOS settings.Summary by cubic
Fixes the workspace sidebar so its scrollbar only shows when content overflows. Uses overlay, autohiding indicators to remove the always-visible track.
.scrollIndicators(.automatic)on the sidebarScrollView.autohidesScrollers = trueandscrollerStyle = .overlayon the sidebarNSScrollView.hasVerticalScrollerbased on content vs. visible height to prevent a scroller when items fit.Written for commit 39313d6. Summary will update on new commits. Review in cubic
Summary by CodeRabbit