Repository navigation
Fix minimal-mode traffic-light inset and new-window Bonsplit tab bar - #3055
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughContentView, TabManager, and AppDelegate were changed to seed, cache, and synchronize a per-window tab-bar (traffic-light) leading inset. ContentView now computes effective titlebar padding using hosting safe area, delegates inset syncing to TabManager, and listens for tab order changes; AppDelegate seeds the inset at window creation. Changes
Sequence Diagram(s)sequenceDiagram
participant App as AppDelegate
participant Window as Window / NSWindow
participant CV as ContentView
participant TM as TabManager
participant WS as Workspace/Bonsplit
App->>TM: syncWorkspaceTabBarLeadingInset(initialInset)
App->>Window: createMainWindow()
Window->>CV: instantiate ContentView (onAppear)
CV->>Window: read contentView.safeAreaInsets.top
Window-->>CV: safeAreaTop
CV->>CV: update hostingSafeAreaTop & compute effectiveTitlebarPadding
CV->>TM: syncWorkspaceTabBarLeadingInset(computedInset)
TM->>TM: normalize & cache inset
TM->>WS: applyTabBarLeadingInset(to all workspaces)
WS-->>TM: ack / updated configs
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 a regression (issue #2737) where new workspaces created in minimal-mode with a collapsed sidebar were not reserving the native traffic-light button area. The fix caches the authoritative tab-bar leading inset on Confidence Score: 5/5Safe to merge — all findings are minor style/P2; no correctness or data-integrity issues. The logic change is well-scoped: caching the inset on TabManager is the correct authority, the fallback chain is sound, and a dedicated regression test directly validates the bug scenario. The only finding is a minor inconsistency in the onChange closure arity with no runtime impact. No files require special attention. Important Files Changed
Sequence DiagramsequenceDiagram
participant CV as ContentView
participant TM as TabManager
participant WS as Workspace (new)
Note over CV: sidebar collapses / isMinimalMode changes
CV->>TM: syncWorkspaceTabBarLeadingInset(80)
TM->>TM: currentWindowTabBarLeadingInset = 80
TM->>TM: applyTabBarLeadingInset(80, to: existingTabs)
Note over CV: user creates new workspace
TM->>TM: applyCreationChromeInheritance(to: newWS, from: sourceWS)
TM->>TM: inheritedLeadingInset = currentWindowTabBarLeadingInset (80)
TM->>WS: tabBarLeadingInset = 80
Note over CV: tabs.map(id) changes - onChange fires
CV->>TM: syncWorkspaceTabBarLeadingInset(80)
TM->>WS: applyTabBarLeadingInset(80) [no-op, already correct]
Reviews (1): Last reviewed commit: "Fix minimal traffic-light inset resync" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| view = AnyView(view.onChange(of: tabManager.tabs.map(\.id)) { _ in | ||
| syncTrafficLightInset() | ||
| }) |
There was a problem hiding this comment.
onChange closure arity
The new handler uses the single-argument (old/deprecated) onChange form while the immediately-preceding handler on line 3727 uses the two-argument (oldValue, newValue) form that is consistent with the rest of this function. Both work at runtime, but mixing the two APIs may generate a deprecation warning in newer Xcode versions and is inconsistent with the surrounding code.
| view = AnyView(view.onChange(of: tabManager.tabs.map(\.id)) { _ in | |
| syncTrafficLightInset() | |
| }) | |
| view = AnyView(view.onChange(of: tabManager.tabs.map(\.id)) { _, _ in | |
| syncTrafficLightInset() | |
| }) |
| let inheritedLeadingInset = currentWindowTabBarLeadingInset | ||
| ?? sourceWorkspace?.bonsplitController.configuration.appearance.tabBarLeadingInset | ||
| guard let inheritedLeadingInset else { return } |
There was a problem hiding this comment.
If currentWindowTabBarLeadingInset is still nil (i.e. syncWorkspaceTabBarLeadingInset has not yet been called by ContentView) and sourceWorkspace is also nil, inheritedLeadingInset resolves to nil and the guard exits — leaving the new workspace with whatever default inset its BonsplitController happened to have. In practice the run-loop round-trip before workspace creation means the value is already populated, but there's no longer a hard guarantee from the type system. The old code had the same hole (guard let sourceWorkspace else { return }), so this is not a regression; just worth a comment clarifying the invariant.
![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/ContentView.swift (1)
3731-3734: Resync-on-tabs-change observer looks correct; minor redundancy on reorders.Observing
tabManager.tabs.map(\.id)correctly catches insert/remove/replace so a freshly created workspace'stabBarLeadingInsetis reconciled immediately. Note this also fires on pure reorders, where the resulting apply is a no-op perapplyTabBarLeadingInset's equality gate — negligible, just worth acknowledging.One optional tightening: if you want to fire strictly on membership changes (not order), you could observe
Set(tabManager.tabs.map(\.id))ortabManager.tabs.countplus a separate identity signal. Not necessary for correctness; current behavior is safe.Writing
@Publishedstate from anonChangeaction handler (viatabManager.syncWorkspaceTabBarLeadingInset) is fine here — the coding-guideline restriction applies to body computations, notonChangeclosures.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 3731 - 3734, Current onChange observes tabManager.tabs.map(\\.id) which also fires on pure reorders; to tighten membership-only updates, change the observed value to a set or explicit identity signal (e.g. use Set(tabManager.tabs.map(\\.id)) or observe tabManager.tabs.count combined with an identity token) so syncTrafficLightInset()/tabManager.syncWorkspaceTabBarLeadingInset only runs when membership changes; keep applyTabBarLeadingInset's equality check as-is for safety.
🤖 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/ContentView.swift`:
- Around line 3731-3734: Current onChange observes tabManager.tabs.map(\\.id)
which also fires on pure reorders; to tighten membership-only updates, change
the observed value to a set or explicit identity signal (e.g. use
Set(tabManager.tabs.map(\\.id)) or observe tabManager.tabs.count combined with
an identity token) so
syncTrafficLightInset()/tabManager.syncWorkspaceTabBarLeadingInset only runs
when membership changes; keep applyTabBarLeadingInset's equality check as-is for
safety.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7f7303c1-0140-462f-9900-c8491f4bb774
📒 Files selected for processing (3)
Sources/ContentView.swiftSources/TabManager.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Extends the #2737 regression coverage beyond the in-window new-workspace path to the new-window path (createMainWindow with a collapsed-sidebar snapshot). The initial workspace is constructed inside TabManager.init, before any source workspace or prior window inset exists to inherit from, so the Bonsplit inset remains at 0 on first render unless createMainWindow explicitly seeds it. Without that seed, a minimal-mode window restored with the sidebar collapsed can paint its first frame with the native traffic lights overlapping the pane tab bar. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Apply the minimal-mode Bonsplit tab-bar leading inset synchronously in createMainWindow before ContentView is constructed, so the initial workspace reserves traffic-light space on the very first frame when a session restore produces a collapsed-sidebar minimal-mode window. The existing resync in ContentView.onAppear and the applyCreationChromeInheritance path both run after the initial workspace has already been realized (it is created inside TabManager.init, which has no source workspace and no prior window inset to inherit from), leaving the first paint with traffic lights overlapping the pane tab bar until some later onChange fires. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Covers the case where MainWindowHostingView zeroes out the titlebar safe area: the -titlebarPadding shortcut pulls the Bonsplit strip above the window frame and the tab bar disappears. The test pins the expected padding to the hosting view's reported safe area top so non-SwiftUI main windows no longer overshoot. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
New windows created by AppDelegate host ContentView inside MainWindowHostingView, which reports a zero titlebar safe area. The previous `.padding(.top, -titlebarPadding)` shortcut therefore pulled the Bonsplit tab strip 32pt above the window frame, making the tab bar invisible while leaving the traffic lights in place. Clamp the offset to what the hosting view actually reports so it cancels a real safe-area inset when one exists (SwiftUI WindowGroup initial windows) and is a no-op when it does not (manually created windows). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
austinywang
changed the title
There was a problem hiding this comment.
🧹 Nitpick comments (1)
Sources/ContentView.swift (1)
3804-3814: Safe-area tracking looks correct; minor opportunity to batch the two@Statewrites.Clamping
safeAreaInsets.topto>= 0and gating on a> 0.5delta mirrors the existingtitlebarPaddingpath, and usingDispatchQueue.main.asyncavoids mutating@Statemid-layout — good.Optional nit: when both
titlebarPaddingandhostingSafeAreaTopchange on the same accessor tick (e.g., entering/leaving fullscreen), the two separateDispatchQueue.main.asyncblocks will cause two view updates instead of one. Trivial, but you could coalesce into a single async block if you want a single re-render:♻️ Optional: coalesce into a single main-queue hop
- if abs(titlebarPadding - nextPadding) > 0.5 { - DispatchQueue.main.async { - titlebarPadding = nextPadding - } - } - if abs(hostingSafeAreaTop - nextSafeAreaTop) > 0.5 { - DispatchQueue.main.async { - hostingSafeAreaTop = nextSafeAreaTop - } - } + let paddingChanged = abs(titlebarPadding - nextPadding) > 0.5 + let safeAreaChanged = abs(hostingSafeAreaTop - nextSafeAreaTop) > 0.5 + if paddingChanged || safeAreaChanged { + DispatchQueue.main.async { + if paddingChanged { titlebarPadding = nextPadding } + if safeAreaChanged { hostingSafeAreaTop = nextSafeAreaTop } + } + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 3804 - 3814, The two separate DispatchQueue.main.async blocks that set titlebarPadding and hostingSafeAreaTop can be coalesced into a single main-queue update to avoid two view updates; when you detect abs(titlebarPadding - nextPadding) > 0.5 or abs(hostingSafeAreaTop - nextSafeAreaTop) > 0.5, dispatch one DispatchQueue.main.async and inside it set titlebarPadding = nextPadding (if its delta exceeds 0.5) and hostingSafeAreaTop = nextSafeAreaTop (if its delta exceeds 0.5), leaving the same clamping for nextSafeAreaTop derived from window.contentView?.safeAreaInsets.top so both `@State` writes occur together.
🤖 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/ContentView.swift`:
- Around line 3804-3814: The two separate DispatchQueue.main.async blocks that
set titlebarPadding and hostingSafeAreaTop can be coalesced into a single
main-queue update to avoid two view updates; when you detect abs(titlebarPadding
- nextPadding) > 0.5 or abs(hostingSafeAreaTop - nextSafeAreaTop) > 0.5,
dispatch one DispatchQueue.main.async and inside it set titlebarPadding =
nextPadding (if its delta exceeds 0.5) and hostingSafeAreaTop = nextSafeAreaTop
(if its delta exceeds 0.5), leaving the same clamping for nextSafeAreaTop
derived from window.contentView?.safeAreaInsets.top so both `@State` writes occur
together.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: cc36002e-8f0d-4adc-9aa7-8fa7a43f21c2
📒 Files selected for processing (3)
Sources/AppDelegate.swiftSources/ContentView.swiftcmuxTests/AppDelegateShortcutRoutingTests.swift
🚧 Files skipped from review as they are similar to previous changes (1)
- cmuxTests/AppDelegateShortcutRoutingTests.swift
…anaflow-ai#3055) * Add regression test for minimal traffic-light inset * Fix minimal traffic-light inset resync * Add regression test for new-window traffic-light inset seed Extends the manaflow-ai#2737 regression coverage beyond the in-window new-workspace path to the new-window path (createMainWindow with a collapsed-sidebar snapshot). The initial workspace is constructed inside TabManager.init, before any source workspace or prior window inset exists to inherit from, so the Bonsplit inset remains at 0 on first render unless createMainWindow explicitly seeds it. Without that seed, a minimal-mode window restored with the sidebar collapsed can paint its first frame with the native traffic lights overlapping the pane tab bar. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Seed traffic-light inset for collapsed-sidebar new windows Apply the minimal-mode Bonsplit tab-bar leading inset synchronously in createMainWindow before ContentView is constructed, so the initial workspace reserves traffic-light space on the very first frame when a session restore produces a collapsed-sidebar minimal-mode window. The existing resync in ContentView.onAppear and the applyCreationChromeInheritance path both run after the initial workspace has already been realized (it is created inside TabManager.init, which has no source workspace and no prior window inset to inherit from), leaving the first paint with traffic lights overlapping the pane tab bar until some later onChange fires. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Add regression test for minimal new-window Bonsplit tab bar padding Covers the case where MainWindowHostingView zeroes out the titlebar safe area: the -titlebarPadding shortcut pulls the Bonsplit strip above the window frame and the tab bar disappears. The test pins the expected padding to the hosting view's reported safe area top so non-SwiftUI main windows no longer overshoot. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> * Bound minimal-mode titlebar offset to hosting view safe area New windows created by AppDelegate host ContentView inside MainWindowHostingView, which reports a zero titlebar safe area. The previous `.padding(.top, -titlebarPadding)` shortcut therefore pulled the Bonsplit tab strip 32pt above the window frame, making the tab bar invisible while leaving the traffic lights in place. Clamp the offset to what the hosting view actually reports so it cancels a real safe-area inset when one exists (SwiftUI WindowGroup initial windows) and is a no-op when it does not (manually created windows). Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com> --------- Co-authored-by: austinpower1258 <austinwang115@gmail.com> Co-authored-by: austinpower1258 <austin@manaflow.ai> Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
This branch was successfully deployed
2 participants
Summary
MainWindowHostingViewreports a zero titlebar safe-area inset, so the unconditional-titlebarPaddingshortcut pulled the tab strip 32pt above the window frame. Now we clamp the negative offset to what the hosting view actually reports.TabManagerand use it when creating workspaces.Testing
./scripts/reload.sh --tag issue-2737 --launchwith both existing and newly-created minimal-mode windows.Summary by CodeRabbit
New Features
Bug Fixes
Tests