Repository navigation
Fix minimal mode tab bar disappearing in fullscreen - #2375
Conversation
Three issues caused the Bonsplit horizontal tab bar to be hidden when entering fullscreen with minimal mode enabled: 1. ignoresSafeArea(.container, edges: .top) was applied unconditionally in minimal mode, pushing content behind the fullscreen menu bar area. Now gated on !isFullScreen. 2. effectiveTitlebarPadding returned -titlebarPadding in minimal mode regardless of fullscreen state. In fullscreen there is no native titlebar to compensate for, so the negative offset pushed content off the top of the screen. Now returns 0 in fullscreen. 3. Traffic light leading inset (80px) was applied in fullscreen minimal mode even though there are no traffic light buttons. Now gated on !isFullScreen, and syncTrafficLightInset is called on fullscreen enter/exit. Closes #2317 Based on #2341
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughThe PR fixes a layout issue where the horizontal tab bar disappears in fullscreen mode with minimal mode enabled. Changes thread the Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 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 three related bugs that caused the Bonsplit horizontal tab bar to disappear or mis-render when entering fullscreen with minimal mode enabled: unconditional Key changes:
Issue found:
Confidence Score: 4/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant App as App Launch / Restore
participant OA as onAppear
participant WO as WindowObserver (async)
participant FSN as FullScreen Notification
App->>OA: view appears
OA->>OA: isFullScreen = false (default)
OA->>OA: syncTrafficLightInset() → inset=80px ❌ (if in FS)
App->>WO: DispatchQueue.main.async
WO->>WO: isFullScreen = styleMask.contains(.fullScreen)
Note over WO: syncTrafficLightInset() NOT called ← gap
App->>FSN: NSWindow.didEnterFullScreenNotification
FSN->>FSN: isFullScreen = true
FSN->>FSN: syncTrafficLightInset() → inset=0 ✅
App->>FSN: NSWindow.didExitFullScreenNotification
FSN->>FSN: isFullScreen = false
FSN->>FSN: syncTrafficLightInset() → inset=80px ✅
|
![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: 05b6751729
ℹ️ 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".
|
|
||
| private func syncTrafficLightInset() { | ||
| let inset: CGFloat = (isMinimalMode && !sidebarState.isVisible) ? 80 : 0 | ||
| let inset: CGFloat = (isMinimalMode && !sidebarState.isVisible && !isFullScreen) ? 80 : 0 |
There was a problem hiding this comment.
syncTrafficLightInset() now depends on isFullScreen, but isFullScreen is also initialized asynchronously from window.styleMask.contains(.fullScreen) (outside the enter/exit fullscreen notifications). Since syncTrafficLightInset() is only called on appear and on specific change handlers, a window restored already in fullscreen can keep the old 80px tab leading inset until another trigger occurs. Please also resync when the initial fullscreen state is set (for example via an onChange(of: isFullScreen) handler or immediately after that assignment).
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
Sources/ContentView.swift (1)
3052-3068:⚠️ Potential issue | 🟡 MinorAdd a sync path for non-notification fullscreen state changes.
Enter/exit fullscreen notifications are handled, but
isFullScreencan also change from window attachment state initialization. In that path, inset sync may stay stale until another event fires.💡 Proposed fix
+ view = AnyView(view.onChange(of: isFullScreen) { _, _ in + syncTrafficLightInset() + })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 3052 - 3068, The fullscreen enter/exit handlers update isFullScreen and call syncTrafficLightInset() but the code path that initializes or attaches the window (where observedWindow is set and isFullScreen may be derived) doesn’t perform the same sync, so inset/state can remain stale; in the window attachment/initialization path (the code that sets observedWindow or initializes the view/window) ensure you mirror the notification handlers by setting isFullScreen appropriately, calling setTitlebarControlsHidden(_:in:) with the current window, assigning or clearing AppDelegate.shared?.fullscreenControlsViewModel, and calling syncTrafficLightInset() so the traffic light inset is always synced regardless of whether fullscreen change came from notifications or initial attachment.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 3052-3068: The fullscreen enter/exit handlers update isFullScreen
and call syncTrafficLightInset() but the code path that initializes or attaches
the window (where observedWindow is set and isFullScreen may be derived) doesn’t
perform the same sync, so inset/state can remain stale; in the window
attachment/initialization path (the code that sets observedWindow or initializes
the view/window) ensure you mirror the notification handlers by setting
isFullScreen appropriately, calling setTitlebarControlsHidden(_:in:) with the
current window, assigning or clearing
AppDelegate.shared?.fullscreenControlsViewModel, and calling
syncTrafficLightInset() so the traffic light inset is always synced regardless
of whether fullscreen change came from notifications or initial attachment.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 99d336dc-4851-44be-935a-f426d404fdbb
📒 Files selected for processing (2)
Sources/ContentView.swiftSources/WorkspaceContentView.swift
![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 2 files
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/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:2521">
P2: Because this inset now depends on `isFullScreen`, restored windows that start in fullscreen can keep a stale 80px leading gap if `isFullScreen` is initialized later without triggering another sync. Resync the tab inset immediately after initial fullscreen-state assignment (or observe `isFullScreen` changes) so startup/restored fullscreen state is applied consistently.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| private func syncTrafficLightInset() { | ||
| let inset: CGFloat = (isMinimalMode && !sidebarState.isVisible) ? 80 : 0 | ||
| let inset: CGFloat = (isMinimalMode && !sidebarState.isVisible && !isFullScreen) ? 80 : 0 |
There was a problem hiding this comment.
P2: Because this inset now depends on isFullScreen, restored windows that start in fullscreen can keep a stale 80px leading gap if isFullScreen is initialized later without triggering another sync. Resync the tab inset immediately after initial fullscreen-state assignment (or observe isFullScreen changes) so startup/restored fullscreen state is applied consistently.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/ContentView.swift, line 2521:
<comment>Because this inset now depends on `isFullScreen`, restored windows that start in fullscreen can keep a stale 80px leading gap if `isFullScreen` is initialized later without triggering another sync. Resync the tab inset immediately after initial fullscreen-state assignment (or observe `isFullScreen` changes) so startup/restored fullscreen state is applied consistently.</comment>
<file context>
@@ -2514,7 +2518,7 @@ struct ContentView: View {
private func syncTrafficLightInset() {
- let inset: CGFloat = (isMinimalMode && !sidebarState.isVisible) ? 80 : 0
+ let inset: CGFloat = (isMinimalMode && !sidebarState.isVisible && !isFullScreen) ? 80 : 0
for tab in tabManager.tabs {
if tab.bonsplitController.configuration.appearance.tabBarLeadingInset != inset {
</file context>
Three issues caused the Bonsplit horizontal tab bar to be hidden when entering fullscreen with minimal mode enabled: 1. ignoresSafeArea(.container, edges: .top) was applied unconditionally in minimal mode, pushing content behind the fullscreen menu bar area. Now gated on !isFullScreen. 2. effectiveTitlebarPadding returned -titlebarPadding in minimal mode regardless of fullscreen state. In fullscreen there is no native titlebar to compensate for, so the negative offset pushed content off the top of the screen. Now returns 0 in fullscreen. 3. Traffic light leading inset (80px) was applied in fullscreen minimal mode even though there are no traffic light buttons. Now gated on !isFullScreen, and syncTrafficLightInset is called on fullscreen enter/exit. Closes manaflow-ai#2317 Based on manaflow-ai#2341 Co-authored-by: Lawrence Chen <lawrencecchen@users.noreply.github.com>
This branch was successfully deployed
1 participant
Summary
Fixes the Bonsplit horizontal tab bar disappearing when entering fullscreen with minimal mode enabled.
Three root causes:
ignoresSafeArea(.container, edges: .top)was applied unconditionally in minimal mode, pushing content behind the fullscreen menu bar areaeffectiveTitlebarPaddingreturned a negative offset in fullscreen minimal mode even though there is no native titlebar to compensate forBased on community PR #2341 by @solssak, which identified the
ignoresSafeAreaissue but missed the padding and inset problems.Closes #2317
Test plan
Summary by cubic
Fixes the horizontal tab bar disappearing in fullscreen when minimal mode is on. The tab bar now stays visible and aligned, with no wasted leading space, in both fullscreen and windowed modes.
.ignoresSafeArea(.container, edges: .top)in fullscreen minimal mode.effectiveTitlebarPaddingin fullscreen minimal mode; keep negative padding only in windowed minimal mode.Written for commit 05b6751. Summary will update on new commits.
Summary by CodeRabbit