Repository navigation
Fix minimal tab drag edge hit testing - #126
Conversation
|
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 PR refines the Bonsplit tab bar with two coordinated improvements: shared-backdrop mode now uses fully transparent active tab backgrounds, and tab hit-testing tolerances are centralized and refactored to use lane-based vertical gating plus horizontal x-range checks for more precise interaction bounds. ChangesTab Bar Styling and Hit-Testing
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related issues
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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Greptile SummaryThis PR fixes tab drag hit testing in minimal/shared-backdrop tab bars so that drags along the near-titlebar edge are claimed by the tab rather than falling through to app-window dragging. It also clears the active-tab fill in shared-backdrop mode so the selected tab no longer shows a hover-like gray highlight.
Confidence Score: 4/5Safe to merge with low risk; the core hit-testing logic is straightforward and the new tests cover the intended scenario. The containsTabLaneHit function expands the accepted y-range symmetrically, adding 6 pt of slop below the tab bar bottom edge as well as above it. Whether this matters depends on whether the drag-zone view bounds extend below the visual tab bar — the test only verifies the upward direction. The rest of the changes look correct and well-targeted. Sources/Bonsplit/Internal/Views/TabBarView.swift — specifically the bounds expansion in containsTabLaneHit and the matching dy change in TabItemHitRegionView. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Mouse event at point P] --> B{shouldCaptureHit\nbounds.contains P?}
B -- No --> C[return false\nno window drag capture]
B -- Yes --> D{BonsplitTabItemHitRegionRegistry\ncontainsWindowPoint}
D -- true --> C
D -- false --> E{hitRegion type}
E -- entireBounds --> F[return true\ncapture for window drag]
E -- trailingEmptyChrome --> G{P.x < trailingLimit\nand not in tab lane?}
G -- Yes --> F
G -- No --> C
subgraph containsTabLaneHit
H[bounds.insetBy dx:0 dy:-6\nexpands plus-minus 6pt vertically] --> I{expanded bounds\ncontains P?}
I -- No --> J[return false]
I -- Yes --> K{P.x within any\ntabFrame plus-minus horizontalSlop?}
K -- Yes --> L[return true: tab hit]
K -- No --> J
end
D --> containsTabLaneHit
Reviews (1): Last reviewed commit: "Fix minimal tab drag edge hit testing" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| guard bounds.insetBy(dx: 0, dy: -verticalSlop).contains(localPoint) else { | ||
| return false | ||
| } | ||
| return tabFrames.contains { frame in | ||
| localPoint.x >= frame.minX - horizontalSlop | ||
| && localPoint.x <= frame.maxX + horizontalSlop | ||
| } |
There was a problem hiding this comment.
bounds.insetBy(dx: 0, dy: -verticalSlop) expands the accepted y-range by verticalSlop (6 pt) on both the top edge (toward the titlebar — the intended fix) and the bottom edge (toward app content). Any point within a tab's x-lane that lies up to 6 pt below the tab bar's bottom edge will be claimed as a tab hit. The old per-tab insetBy(dx: -2, dy: -2) also over-extended downward, but only by 2 pt. The new code triples that slop, and shouldCaptureHit only guards this with bounds.contains(point) on the drag-zone view, so if that view's bounds extend past the tab bar's visual bottom (e.g. it fills the full content view), the downward expansion starts suppressing window-drag events for touches in the app content area. The test only verifies the upward direction.
| let active = NSColor(TabBarColors.activeTabBackground(for: appearance)).usingColorSpace(.sRGB)! | ||
|
|
||
| var alpha: CGFloat = 1 | ||
| active.getRed(nil, green: nil, blue: nil, alpha: &alpha) |
There was a problem hiding this comment.
usingColorSpace(.sRGB) will crash the test rather than fail it cleanly if the color-space conversion ever returns nil. Color.clear resolves to NSColor.clear (calibrated gray-space), and while conversion to sRGB works today, a guarded call makes the failure mode explicit and the intent clearer.
| let active = NSColor(TabBarColors.activeTabBackground(for: appearance)).usingColorSpace(.sRGB)! | |
| var alpha: CGFloat = 1 | |
| active.getRed(nil, green: nil, blue: nil, alpha: &alpha) | |
| let active = try XCTUnwrap( | |
| NSColor(TabBarColors.activeTabBackground(for: appearance)).usingColorSpace(.sRGB), | |
| "Could not convert activeTabBackground color to sRGB" | |
| ) | |
| var alpha: CGFloat = 1 | |
| active.getRed(nil, green: nil, blue: nil, alpha: &alpha) |
1 participant
Summary
Verification
Need help on this PR? Tag
@codesmithwith what you need.Summary by cubic
Fixes tab drag hit testing so minimal/shared-backdrop tabs keep hit ownership near the titlebar edge and don’t fall through to window dragging. Also clears the active tab fill in shared-backdrop mode to remove the hover-like gray highlight.
.clearfor shared-backdrop active tab background inTabBarColors.Written for commit c39e2ce. Summary will update on new commits. Review in cubic
Summary by CodeRabbit
Bug Fixes
Refactor
Tests