Repository navigation
Fix slow scrolled tab strip interactions - #147
azooz2003-bit wants to merge 3 commits into
Conversation
📝 WalkthroughWalkthroughTab-bar scroll affordance plumbing and coordinate space are refactored to support horizontal scrolling. A new ChangesTab Bar Scroll-Aware Hit-Testing and Frame Coordinate Refactoring
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. Comment |
Greptile SummaryThis PR fixes incorrect tab-bar interactions when the tab strip is horizontally scrolled by keeping all tab geometry in scroll-content coordinates and injecting a
Confidence Score: 4/5The core hit-test and drag-reorder logic is correct and well-tested; the one gap is that the open-hand cursor zone in minimal mode will drift out of position after the user scrolls without triggering a tab-layout change. All three hit-test paths (drag zone, hover background, manual reorder) correctly translate viewport points into content space using the injected closure, and the new tests confirm the boundary cases. The only unhandled situation is cursor rect staleness in DragNSView after a mid-session scroll: the .openHand rect is recalculated only when hitRegion or isMinimalMode changes, but since tab frames are now content-stable, a pure scroll no longer triggers that path. Sources/Bonsplit/Internal/Views/TabBarView.swift — specifically the DragNSView cursor rect invalidation path around line 2762. Important Files Changed
Sequence DiagramsequenceDiagram
participant SV as NSScrollView
participant Bridge as TabBarScrollViewBridge
participant TV as TabBarView (@State)
participant DZV as DragNSView (DragZone)
participant DHV as TabBarBackgroundNSView (Hover)
participant TR as TabBarManualReorderTrackingView
Note over SV,TV: Content geometry update (scroll or layout)
SV->>TV: updateTabScrollContent(frame:)
TV->>Bridge: currentScrollOffset() → CGFloat
TV->>TV: "tabScrollAffordances = tabScrollAffordances(scrollOffset, contentWidth, containerWidth)"
Note over TV,DZV: SwiftUI render pass
TV->>DZV: "updateNSView — hitRegion = .trailingEmptyChrome(tabFramesInContent, …)"
TV->>DZV: "scrollOffsetProvider = { bridge.currentScrollOffset() }"
TV->>DHV: "updateNSView — scrollOffsetProvider = { bridge.currentScrollOffset() }"
TV->>TR: "updateNSView — scrollOffsetProvider = { bridge.currentScrollOffset() }"
Note over DZV: Hit test (mouse event)
DZV->>DZV: "contentPoint(for: viewportPoint) = point + scrollOffset"
DZV->>DZV: compare against content-space tabFrames
Note over DHV: Tab item hit check (nonisolated)
DHV->>DHV: containsBonsplitTabItemHit — localPoint + scrollOffset vs hitBounds + scrollOffset
Note over TR: Drag reorder (mouse drag)
TR->>TR: "contentPoint(for: dragPoint) = point + scrollOffset"
TR->>TR: dropTargetIndex(for: contentPoint, in: pane)
Reviews (1): Last reviewed commit: "test: cover scrolled tab strip hit regio..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| var hitRegion = HitRegion.entireBounds { | ||
| didSet { invalidateWindowDragCursorRects() } |
There was a problem hiding this comment.
resetCursorRects is only triggered by hitRegion.didSet and isMinimalMode.didSet. Before this PR, tabFramesInBar were in viewport coordinates, so every scroll event produced a new set of frames, changed hitRegion, and automatically re-fired didSet → invalidateWindowDragCursorRects(). Now tabFramesInContent are stable under scrolling, hitRegion never changes mid-scroll, and the .openHand cursor rect stays anchored to the position computed at the last tab-layout change. A user who scrolls the tab bar mid-session will see the drag cursor painted over the wrong area (covering a visible tab or missing the actual empty-chrome region) until some unrelated tab event refreshes the frames.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/Bonsplit/Internal/Views/TabBarView.swift`:
- Around line 2450-2452: contentPoint(for:) currently only adds the scroll
offset but doesn't convert a point from the tab-bar's coordinate space into the
tab content coordinate space (tabContentCoordinateSpaceName), so
ManualReorderNSView hit-testing is using mixed spaces; fix by converting the
incoming point into the tab-content coordinate space before applying
scrollOffsetProvider() (use the view-to-coordinate-space conversion
corresponding to tabContentCoordinateSpaceName), then add the scroll offset as
before so detection and drop-index math use consistent coordinates; update any
ManualReorderNSView hit-test callers to pass points in the tab-bar coordinate
space into contentPoint(for:) (no other callsite changes needed if they already
do).
- Around line 1647-1650: The fallback separator mask is computed using
selectedTabFrameInContent (content-space) but totalWidth is in viewport-space,
causing drift after horizontal scroll; fix by translating the selected tab frame
into viewport coordinates before calling tabBarLayout.selectedSeparatorGap —
e.g. compute a selectedTabFrameInViewport by subtracting the current horizontal
content scroll/offset (or apply your content->viewport transform) from
selectedTabFrameInContent, then pass that selectedTabFrameInViewport and
totalWidth into tabBarLayout.selectedSeparatorGap so the mask aligns after
scrolling.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb07b175-82ba-4d66-8ba4-aef99baae55e
📒 Files selected for processing (3)
Sources/Bonsplit/Internal/Views/TabBarView.swiftSources/Bonsplit/Internal/Views/TabItemView.swiftTests/BonsplitTests/BonsplitTests.swift
| let selectedGap = tabBarLayout.selectedSeparatorGap( | ||
| selectedTabFrame: selectedTabFrameInBar, | ||
| selectedTabFrame: selectedTabFrameInContent, | ||
| totalWidth: totalWidth | ||
| ) |
There was a problem hiding this comment.
Fallback separator masking needs viewport translation.
Line 1648 uses selectedTabFrameInContent directly, but this fallback separator is rendered in viewport-space (totalWidth from outer geometry). After horizontal scroll, the mask drifts by the scroll offset.
Suggested patch
- let selectedGap = tabBarLayout.selectedSeparatorGap(
- selectedTabFrame: selectedTabFrameInContent,
- totalWidth: totalWidth
- )
+ let selectedTabFrameInViewport = selectedTabFrameInContent?.offsetBy(
+ dx: -scrollViewBridge.currentScrollOffset(),
+ dy: 0
+ )
+ let selectedGap = tabBarLayout.selectedSeparatorGap(
+ selectedTabFrame: selectedTabFrameInViewport,
+ totalWidth: totalWidth
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let selectedGap = tabBarLayout.selectedSeparatorGap( | |
| selectedTabFrame: selectedTabFrameInBar, | |
| selectedTabFrame: selectedTabFrameInContent, | |
| totalWidth: totalWidth | |
| ) | |
| let selectedTabFrameInViewport = selectedTabFrameInContent?.offsetBy( | |
| dx: -scrollViewBridge.currentScrollOffset(), | |
| dy: 0 | |
| ) | |
| let selectedGap = tabBarLayout.selectedSeparatorGap( | |
| selectedTabFrame: selectedTabFrameInViewport, | |
| totalWidth: totalWidth | |
| ) |
🤖 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/Bonsplit/Internal/Views/TabBarView.swift` around lines 1647 - 1650,
The fallback separator mask is computed using selectedTabFrameInContent
(content-space) but totalWidth is in viewport-space, causing drift after
horizontal scroll; fix by translating the selected tab frame into viewport
coordinates before calling tabBarLayout.selectedSeparatorGap — e.g. compute a
selectedTabFrameInViewport by subtracting the current horizontal content
scroll/offset (or apply your content->viewport transform) from
selectedTabFrameInContent, then pass that selectedTabFrameInViewport and
totalWidth into tabBarLayout.selectedSeparatorGap so the mask aligns after
scrolling.
| private func contentPoint(for point: NSPoint) -> NSPoint { | ||
| NSPoint(x: point.x + max(0, scrollOffsetProvider?() ?? 0), y: point.y) | ||
| } |
There was a problem hiding this comment.
Manual reorder hit-testing still mixes coordinate spaces.
Line 1380 stores tab frames in tabContentCoordinateSpaceName, but Line 2451 only adds scroll offset to local points. ManualReorderNSView points are in tab-bar view coordinates, so a non-zero scroll viewport origin (for example, when a leading inset is present) shifts source-tab detection and drop-index thresholds.
🤖 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/Bonsplit/Internal/Views/TabBarView.swift` around lines 2450 - 2452,
contentPoint(for:) currently only adds the scroll offset but doesn't convert a
point from the tab-bar's coordinate space into the tab content coordinate space
(tabContentCoordinateSpaceName), so ManualReorderNSView hit-testing is using
mixed spaces; fix by converting the incoming point into the tab-content
coordinate space before applying scrollOffsetProvider() (use the
view-to-coordinate-space conversion corresponding to
tabContentCoordinateSpaceName), then add the scroll offset as before so
detection and drop-index math use consistent coordinates; update any
ManualReorderNSView hit-test callers to pass points in the tab-bar coordinate
space into contentPoint(for:) (no other callsite changes needed if they already
do).
1 participant
Summary
Validation
Downstream cmux PR: manaflow-ai/cmux#6053
Need help on this PR? Tag
/codesmithwith what you need. Autofix is disabled.Summary by cubic
Fix sluggish tab strip interactions while scrolled by keeping tab geometry in content-space and translating interactions by the current scroll offset. Addresses the tab strip scroll performance issue and fixes scrolled hit regions.
Written for commit 674339b. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Style