Reserve scroller inset so terminal text isn't clipped on the right#3001
Reserve scroller inset so terminal text isn't clipped on the right#3001rdsciv wants to merge 1 commit intomanaflow-ai:mainfrom
Conversation
… right When macOS renders a persistent vertical scroller (Show Scroll Bars = Always, or Automatic with a mouse attached), cmux was still telling libghostty the full scrollview width, so the rightmost terminal column rendered underneath the scrollbar. cmux forces the NSScrollView to overlay style to avoid a reserved legacy gutter, which means AppKit does not subtract the scroller width for us. Shrink surfaceView.frame by the scroller width when preferredScrollerStyle is .legacy and a vertical scroller is mounted; keep the full terminal width for transient overlay scrollers that fade out. Re-run the geometry pass on preferredScrollerStyleDidChangeNotification so the terminal resizes live when the user plugs in a mouse or toggles the system preference. Fixes manaflow-ai#1082 Fixes manaflow-ai#2997
|
@rdsciv is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis change adds vertical scroller inset width calculation to prevent legacy scrollbars from overlapping terminal content. The implementation computes the reserved right-edge space based on scroller presence and style, adjusts terminal width synchronization accordingly, and introduces comprehensive unit tests for the new inset logic. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 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 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 SummaryFixes the long-standing clip-under-scroller bug (#1082, #2997) by introducing Confidence Score: 5/5Safe to merge; all remaining findings are P2 style suggestions that do not affect correctness. The core fix is a small, well-tested, pure function. The geometry integration is straightforward and the live-resize path through No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant Sys as macOS (NSScroller)
participant Handler as handlePreferredScrollerStyleChange
participant Geom as synchronizeGeometryAndContent
participant Helper as verticalScrollerInsetWidth()
participant Surface as surfaceView.frame
participant Core as synchronizeCoreSurface → libghostty
Sys->>Handler: preferredScrollerStyleDidChange notification
Handler->>Geom: synchronizeGeometryAndContent()
Geom->>Helper: verticalScrollerInsetWidth()
Note over Helper: preferredScrollerStyle == .legacy<br/>AND hasVerticalScroller?
alt Legacy + scroller mounted
Helper-->>Geom: NSScroller.scrollerWidth (e.g. 15pt)
else Overlay or no scroller
Helper-->>Geom: 0
end
Geom->>Surface: setFrame(width: scrollView.bounds.width − inset)
Geom->>Core: synchronizeCoreSurface()
Core-->>Sys: libghostty resizes terminal columns
Reviews (1): Last reviewed commit: "Reserve vertical scroller inset so termi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| func testClampsNegativeScrollerWidthToZero() { | ||
| let inset = GhosttySurfaceScrollView.verticalScrollerInsetWidth( | ||
| hasVerticalScroller: true, | ||
| preferredScrollerStyle: .legacy, | ||
| scrollerWidth: -4 | ||
| ) | ||
| XCTAssertEqual(inset, 0) | ||
| } |
There was a problem hiding this comment.
testClampsNegativeScrollerWidthToZero exercises a max(0, ...) guard that Apple's NSScroller.scrollerWidth(for:scrollerStyle:) can never trigger — the API is documented to return a non-negative CGFloat. Per the CLAUDE.md test quality policy, tests should verify observable runtime behavior through executable paths, not defensive implementation detail. Consider replacing this case with a test that observes a meaningful boundary condition (e.g. scrollerWidth == 0 with a legacy scroller mounted, confirming the inset is 0 and the surface retains its full width), which would remain useful if scrollerWidth semantics ever change.
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!
| private func verticalScrollerInsetWidth() -> CGFloat { | ||
| let style = NSScroller.preferredScrollerStyle | ||
| let width = NSScroller.scrollerWidth(for: .regular, scrollerStyle: style) | ||
| return Self.verticalScrollerInsetWidth( | ||
| hasVerticalScroller: scrollView.hasVerticalScroller, | ||
| preferredScrollerStyle: style, | ||
| scrollerWidth: width | ||
| ) | ||
| } |
There was a problem hiding this comment.
scrollerWidth query for overlay style
NSScroller.scrollerWidth(for: .regular, scrollerStyle: style) is called unconditionally, but the result is only used when style == .legacy. When style == .overlay the static helper returns 0 before reading scrollerWidth, so the scrollerWidth call is dead work. Consider querying the width only when needed:
| private func verticalScrollerInsetWidth() -> CGFloat { | |
| let style = NSScroller.preferredScrollerStyle | |
| let width = NSScroller.scrollerWidth(for: .regular, scrollerStyle: style) | |
| return Self.verticalScrollerInsetWidth( | |
| hasVerticalScroller: scrollView.hasVerticalScroller, | |
| preferredScrollerStyle: style, | |
| scrollerWidth: width | |
| ) | |
| } | |
| private func verticalScrollerInsetWidth() -> CGFloat { | |
| let style = NSScroller.preferredScrollerStyle | |
| guard style == .legacy else { return 0 } | |
| guard scrollView.hasVerticalScroller else { return 0 } | |
| let width = NSScroller.scrollerWidth(for: .regular, scrollerStyle: .legacy) | |
| return max(0, width) | |
| } |
This also removes the indirection through the static helper for the hot-path call and makes the guard order match the static helper's logic explicitly.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
1 participant
Summary
Fixes #1082 and #2997. When macOS renders a persistent vertical scroller — either because Show Scroll Bars is set to Always, or because the user has a mouse attached with Automatic — the rightmost terminal column currently renders underneath the scrollbar. cmux forces the
NSScrollViewto overlay style to avoid a reserved legacy gutter, which means AppKit does not subtract the scroller width for us; meanwhilesynchronizeGeometryAndContentwas handing libghostty the full scrollview width.This PR shrinks
surfaceView.frame.widthby the scroller width whenNSScroller.preferredScrollerStyle == .legacyand a vertical scroller is mounted. Transient overlay scrollers that fade out keep the full terminal width — the brief overlap during a scroll is the explicit tradeoff for not losing a column on every trackpad-only session.handlePreferredScrollerStyleChangenow re-runs the full geometry pass, so the terminal resizes live when the user plugs in a mouse or flips the system preference.GhosttySurfaceScrollView.verticalScrollerInsetWidth(...)— small static helper, pure function, easy to unit-testsynchronizeGeometryAndContentuses it to computetargetSizesynchronizeCoreSurfacereadssurfaceView.frame.widthas before, which is now pre-shrunkNote on commit structure
The CLAUDE.md regression-test policy recommends a two-commit structure (test red → fix green). That doesn't cleanly apply here because the testable seam (
verticalScrollerInsetWidth) is introduced by the fix itself, so the test cannot compile at commit-1. The new tests verify observable runtime behavior of the pure helper (per the "no source-text assertions" rule) rather than shape.Test plan
xcodebuild -scheme cmux-unitpasses locally (newTerminalScrollerInsetTestscovers all four branches of the helper)Summary by cubic
Fixes #1082 and #2997 by reserving a right-edge inset when macOS shows a persistent vertical scrollbar so the rightmost terminal column isn’t clipped. The inset applies only for legacy scrollers; overlay scrollers keep full width, and the terminal resizes live when the style changes.
synchronizeGeometryAndContent, shrinkingsurfaceView.frame.widthonly when a vertical scroller is present andNSScroller.preferredScrollerStyle == .legacy.GhosttySurfaceScrollView.verticalScrollerInsetWidth(...)(pure helper) with unit tests for absent, overlay, legacy, and negative-width cases.Written for commit 943c221. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests