Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
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:
📝 WalkthroughWalkthroughAdds hover-driven visibility and AppStorage-backed settings to the tab bar, replaces the inline trailing drop zone with a conditional trailingInteractionView, introduces split-button hover/drag handling and accessibility IDs, adds TabBarWindowDragRegion (NSViewRepresentable) to handle mouseDown/double-click/drag window behavior, and wraps TabBarView with an AppKit hosting wrapper. (43 words) Changes
Sequence DiagramsequenceDiagram
participant User
participant TabBar as TabBarView
participant DragRegion as TabBarWindowDragRegion
participant Window
participant System
User->>TabBar: hover enter/leave
TabBar->>TabBar: update hover state & evaluate visibility
TabBar->>DragRegion: render drag region or drop target (trailingInteractionView)
User->>DragRegion: mouseDown (click / double-click / drag)
DragRegion->>DragRegion: inspect clickCount
alt double-click
DragRegion->>System: read double-click prefs
System-->>DragRegion: prefs
DragRegion->>Window: minimize or zoom per prefs
else single-click / drag
DragRegion->>Window: enable movableIfNeeded
DragRegion->>Window: performDrag(with:)
Window-->>DragRegion: drag complete
DragRegion->>Window: restore movable state
end
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/Bonsplit/Internal/Views/TabBarView.swift (1)
78-81: Consider extracting AppStorage keys to constants.The hardcoded string keys
"paneTabBarControlsVisibilityMode"and"workspaceTitlebarVisible"could be extracted to static constants if they're referenced elsewhere in the codebase (e.g., settings UI). This helps prevent typos and makes refactoring easier.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Bonsplit/Internal/Views/TabBarView.swift` around lines 78 - 81, Extract the hardcoded AppStorage keys into named constants and use them in the property wrappers to avoid duplication/typos; e.g., add a static constant (or centralized Settings/StorageKeys struct) such as paneTabBarControlsVisibilityModeKey and workspaceTitlebarVisibleKey and replace the string literals in the `@AppStorage` properties controlsVisibilityModeRawValue and showWorkspaceTitlebar with those constants so other code (settings UI, tests) can reuse them consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Sources/Bonsplit/Internal/Views/TabBarView.swift`:
- Around line 78-81: Extract the hardcoded AppStorage keys into named constants
and use them in the property wrappers to avoid duplication/typos; e.g., add a
static constant (or centralized Settings/StorageKeys struct) such as
paneTabBarControlsVisibilityModeKey and workspaceTitlebarVisibleKey and replace
the string literals in the `@AppStorage` properties controlsVisibilityModeRawValue
and showWorkspaceTitlebar with those constants so other code (settings UI,
tests) can reuse them consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e838ad55-c977-4ea9-9b08-2feed56df4c8
📒 Files selected for processing (1)
Sources/Bonsplit/Internal/Views/TabBarView.swift
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 43f6b158fb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ZStack(alignment: .trailing) { | ||
| if !shouldShowSplitButtonsNow && shouldUseTabBarDragRegion && !isTabDragActive { | ||
| TabBarWindowDragRegion() | ||
| .frame(maxWidth: .infinity, maxHeight: .infinity) |
There was a problem hiding this comment.
In splitButtonsArea, the hidden-state drag region uses .frame(maxWidth: .infinity), which makes this HStack child flexible and able to absorb extra horizontal space instead of staying the split-buttons width. In the onHover + hidden-titlebar path, that can steal width from the scrollable tabs area and compress/hide tabs while controls are hidden. The drag region should be constrained to the controls slot width rather than allowed to expand infinitely.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fe83f44ee9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if shouldUseTabBarDragRegion && !isTabDragActive { | ||
| TabBarWindowDragRegion() | ||
| .frame(width: width, height: TabBarMetrics.tabHeight) |
There was a problem hiding this comment.
When workspaceTitlebarVisible is false, this condition switches the trailing interaction area from an onDrop target to TabBarWindowDragRegion unless isTabDragActive is set. isTabDragActive only reflects local drag state (draggingTab/activeDragTab), so cross-controller drags (the same external-drag case already handled in validateDrop) leave this region non-droppable. In that scenario, dropping after the last tab in the wide trailing gap stops working and users are forced to hit only the tiny 30px end drop zone.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 79c00997f3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !shouldShowSplitButtonsNow && shouldUseTabBarDragRegion && !isTabDragActive { | ||
| TabBarWindowDragRegion() | ||
| .frame(maxWidth: .infinity, maxHeight: .infinity) |
There was a problem hiding this comment.
This condition only adds TabBarWindowDragRegion when the split controls are hidden, so with workspaceTitlebarVisible == false and the default controls mode (always), the controls slot is never draggable; if tabs also overflow (so there is no trailing gap), the tab bar has no window-drag target at all. Because this commit also switches pane hosts to BonsplitHostingView (mouseDownCanMoveWindow = false), users in that configuration can no longer drag the window from the pane tab bar.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Bonsplit/Internal/Views/PaneContainerView.swift">
<violation number="1" location="Sources/Bonsplit/Internal/Views/PaneContainerView.swift:14">
P1: Returning `self` from `hitTest` swallows child hit targets, preventing `TabBarWindowDragRegion` from receiving mouse events for drag/double-click behavior.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3401bd9fc3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| case .leftMouseDown: | ||
| if bounds.contains(point), !hitTestRoutesToWindowDragRegion(at: point) { | ||
| suppressWindowDraggingIfNeeded(window: window) | ||
| } |
There was a problem hiding this comment.
The new local mouse monitor suppresses window.isMovable whenever the cursor is inside this view’s bounds, but it never checks whether the tab bar is currently hidden/inactive. In this codebase, inactive workspaces are hidden via isHidden (not detached), so their TabBarInteractionContainerView instances still receive monitor callbacks; if their cached frames overlap the active workspace, they can incorrectly disable window dragging for visible UI interactions. Add a visibility/hidden-ancestor guard before calling suppressWindowDraggingIfNeeded so only the active tab bar can affect window movability.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Bonsplit/Internal/Views/PaneContainerView.swift">
<violation number="1" location="Sources/Bonsplit/Internal/Views/PaneContainerView.swift:22">
P1: `viewDidMoveToWindow` clears the tracked window before restoring `isMovable`, which can leave a window permanently non-movable; it also installs an event monitor when detached (`window == nil`).</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if window !== monitoredWindow { | ||
| removeEventMonitor() | ||
| monitoredWindow = window | ||
| installEventMonitor() | ||
| } | ||
|
|
||
| if window == nil { | ||
| restoreWindowDraggingIfNeeded() | ||
| } |
There was a problem hiding this comment.
P1: viewDidMoveToWindow clears the tracked window before restoring isMovable, which can leave a window permanently non-movable; it also installs an event monitor when detached (window == nil).
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Bonsplit/Internal/Views/PaneContainerView.swift, line 22:
<comment>`viewDidMoveToWindow` clears the tracked window before restoring `isMovable`, which can leave a window permanently non-movable; it also installs an event monitor when detached (`window == nil`).</comment>
<file context>
@@ -3,15 +3,101 @@ import UniformTypeIdentifiers
+ override func viewDidMoveToWindow() {
+ super.viewDidMoveToWindow()
+
+ if window !== monitoredWindow {
+ removeEventMonitor()
+ monitoredWindow = window
</file context>
| if window !== monitoredWindow { | |
| removeEventMonitor() | |
| monitoredWindow = window | |
| installEventMonitor() | |
| } | |
| if window == nil { | |
| restoreWindowDraggingIfNeeded() | |
| } | |
| if window !== monitoredWindow { | |
| restoreWindowDraggingIfNeeded() | |
| removeEventMonitor() | |
| monitoredWindow = window | |
| if window != nil { | |
| installEventMonitor() | |
| } | |
| } |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8ebf7c8bc1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if window !== monitoredWindow { | ||
| removeEventMonitor() | ||
| monitoredWindow = window | ||
| installEventMonitor() |
There was a problem hiding this comment.
When viewDidMoveToWindow detects a window change, it assigns monitoredWindow = window (and installs a new monitor) before restoring any pending previousWindowMovableState. If this view has already suppressed dragging (window.isMovable = false) and is detached/reparented before the matching leftMouseUp arrives, restoreWindowDraggingIfNeeded() will restore against the new/nil window instead of the original one, leaving the original window stuck non-movable. Restore the old window state before overwriting monitoredWindow.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Bonsplit/Internal/Views/TabBarView.swift">
<violation number="1" location="Sources/Bonsplit/Internal/Views/TabBarView.swift:1000">
P2: `dropExited` conditionally clears the indicator using a location-derived index; with dynamic target resolution this can leave a stale drop indicator visible after exiting the tab bar. Clear `dropTargetIndex` unconditionally on exit.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7f2c13f648
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if !isEnabled { | ||
| clearDropState() | ||
| } |
There was a problem hiding this comment.
TabBarNativeDropBridge.updateNSView clears drop state every time isEnabled is false, but isEnabled is driven by local drag state (isTabDragActive). During cross-controller drags, SwiftUI onDrop delegates still handle hover/indicator updates while this bridge remains disabled, so each view update resets dropLifecycle back to .idle right after dropEntered, and dropUpdated then short-circuits without maintaining the indicator or target updates. This regresses external drag feedback and target tracking until drop.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 1 file (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="Sources/Bonsplit/Internal/Views/TabBarView.swift">
<violation number="1" location="Sources/Bonsplit/Internal/Views/TabBarView.swift:214">
P1: Wrap the `clearDropState()` call in `DispatchQueue.main.async` to avoid modifying state during view update.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if !isEnabled { | ||
| clearDropState() | ||
| } |
There was a problem hiding this comment.
P1: Wrap the clearDropState() call in DispatchQueue.main.async to avoid modifying state during view update.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Bonsplit/Internal/Views/TabBarView.swift, line 214:
<comment>Wrap the `clearDropState()` call in `DispatchQueue.main.async` to avoid modifying state during view update.</comment>
<file context>
@@ -25,6 +25,198 @@ private struct TabDropFramesPreferenceKey: PreferenceKey {
+ view.updateDropTarget = updateDropTarget
+ view.clearDropState = clearDropState
+ view.performDrop = performDrop
+ if !isEnabled {
+ clearDropState()
+ }
</file context>
| if !isEnabled { | |
| clearDropState() | |
| } | |
| if !isEnabled { | |
| DispatchQueue.main.async { | |
| clearDropState() | |
| } | |
| } |
1 participant
Summary
Testing
cmuxtagged reload using this submodule branch.Summary by cubic
Adds hover-only pane tab bar controls and full-width tab drop capture. When the workspace titlebar is hidden, empty tab‑bar areas become a draggable region that honors double‑clicks and the macOS traffic‑light inset without breaking tab drags.
New Features
@AppStoragepaneTabBarControlsVisibilityMode).workspaceTitlebarVisibleis false, aTabBarWindowDragRegionsits behind the controls and trailing space; it is disabled during tab drags; honors system double‑click actions.paneTabBar,paneTabBarControlsRegion, and each split button.Bug Fixes
Written for commit f98559d. Summary will update on new commits.
Summary by CodeRabbit
New Features
Improvements