Add cleaner titlebar visibility options#1327
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![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 titlebar/pane control visibility settings, wiring in SettingsView and AppStorage; updates ContentView and UpdateTitlebarAccessory to honor visibility/hover rules; adds TitlebarVisibilitySettings.swift, UI tests, localization entries, project wiring, and a custom MainWindowHostingView. Changes
Sequence DiagramsequenceDiagram
autonumber
participant User as User
participant Settings as SettingsView
participant Storage as AppStorage/UserDefaults
participant Content as ContentView
participant Accessory as UpdateTitlebarAccessory
participant Window as Window/UI
User->>Settings: Toggle "Show Workspace Title Bar"
Settings->>Storage: write showWorkspaceTitlebar
Storage-->>Content: publish change
Content->>Window: adjust top padding / render titlebar or drag-handle
User->>Settings: Set Controls Visibility (Always / On Hover)
Settings->>Storage: write visibility mode
Storage-->>Accessory: publish mode change
User->>Accessory: Hover controls area
Accessory->>Accessory: evaluate shouldShowControls (mode + hover)
Accessory->>Window: animate opacity to show/hide controls
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 |
![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: b77d7c2900
ℹ️ 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".
| let computedTitlebarHeight = window.frame.height - window.contentLayoutRect.height | ||
| let nextPadding = max(28, min(72, computedTitlebarHeight)) | ||
| let nextPadding = showWorkspaceTitlebar ? max(28, min(72, computedTitlebarHeight)) : 0 | ||
| if abs(titlebarPadding - nextPadding) > 0.5 { |
There was a problem hiding this comment.
nextPadding now depends on showWorkspaceTitlebar, but this computation is inside a WindowAccessor callback created with default deduping, so it only runs on initial attach for the same window. When users toggle “Show Workspace Title Bar” at runtime, the view condition changes immediately but titlebarPadding can stay stale, causing either a leftover empty strip when hidden or a collapsed titlebar region when re-enabled until a new window/session refreshes it.
Useful? React with 👍 / 👎.
Sources/ContentView.swift
Outdated
| if showWorkspaceTitlebar { | ||
| // Titlebar overlay is only over terminal content, not the sidebar. | ||
| customTitlebar | ||
| } else { |
There was a problem hiding this comment.
This new branch disables customTitlebar entirely when the workspace titlebar setting is off, but customTitlebar is the only place that renders fullscreenControls for the isFullScreen && !sidebarState.isVisible path. Since fullscreen mode also hides native titlebar accessories and the other fullscreen overlay only shows controls when the sidebar is visible, users in fullscreen with a hidden sidebar lose all titlebar control buttons (including sidebar toggle) in this configuration.
Useful? React with 👍 / 👎.
![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
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)
2660-2668:⚠️ Potential issue | 🟠 MajorResync portal geometry after
titlebarPaddingchanges.This moves the workspace content vertically, but unlike the sidebar width/visibility paths there’s no explicit portal resync here. Toggling the workspace titlebar can leave portal-backed terminals on the old rect until some unrelated resize happens.
Suggested fix
if abs(titlebarPadding - nextPadding) > 0.5 { DispatchQueue.main.async { titlebarPadding = nextPadding + TerminalWindowPortalRegistry.scheduleExternalGeometrySynchronizeForAllWindows() } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 2660 - 2668, When updating titlebarPadding (compare titlebarPadding to nextPadding and setting it inside the DispatchQueue.main.async block), also trigger a portal geometry resync so portal-backed views update to the new content rect; add a call immediately after the titlebarPadding assignment in the DispatchQueue.main.async closure to invoke the portal resync routine (e.g., PortalManager.shared.resyncGeometry() or the existing portalController.resync* method) so portals are recomputed whenever titlebarPadding changes.
🧹 Nitpick comments (1)
Resources/Localizable.xcstrings (1)
73097-73103: Use one spelling for “title bar” across this settings cluster.The English copy mixes “Title Bar” and “Titlebar”, which will read inconsistently in the UI. I’d standardize on one form here.
✏️ Suggested copy tweak
- "value": "Titlebar Controls" + "value": "Title Bar Controls" - "value": "Hide titlebar buttons until the pointer reaches them." + "value": "Hide title bar buttons until the pointer reaches them."Also applies to: 73148-73154, 73216-73222
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Resources/Localizable.xcstrings` around lines 73097 - 73103, The English localization uses inconsistent spelling for the UI label—update all affected string keys to use a single form (e.g., "Title Bar") so the UI is consistent: change "settings.app.showWorkspaceTitlebar" value to "Show Workspace Title Bar" and make the same spelling change in the other related keys referenced (the entries around the other ranges, e.g., the keys near lines 73148-73154 and 73216-73222) to ensure every "Titlebar"/"Title Bar" instance in this settings cluster uses the chosen form.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/ContentView.swift`:
- Around line 1993-2002: The WindowDragHandleView is currently sitting at the
very top of the ZStack when showWorkspaceTitlebar is false and thus intercepts
clicks across the first 30pt; change its positioning so it only occupies the
empty spacer area by offsetting it by the current titlebarPadding (or otherwise
placing it inside a VStack that starts after titlebarPadding) so the drag
handle’s frame (collapsedTitlebarDragHandleHeight) is moved down and no longer
covers pane-tab controls; update the branch using showWorkspaceTitlebar,
referencing WindowDragHandleView, collapsedTitlebarDragHandleHeight and
titlebarPadding to apply the padding/offset or rearrange the view order
accordingly.
- Around line 1944-1989: The WorkspaceContentView instances are still marked
visible/input-active when the sidebar is showing Notifications, letting their
portal-backed surfaces remain above SwiftUI; update the visibility/input logic
so mountedWorkspaces computes isWorkspaceVisible and isWorkspaceInputActive only
when sidebarSelectionState.selection == .tabs (e.g. combine
selectedWorkspaceId/retiringWorkspaceId checks with
sidebarSelectionState.selection == .tabs), and use those combined flags when
constructing WorkspaceContentView (and for .opacity, .allowsHitTesting,
.accessibilityHidden, .zIndex, and the .task id) so the workspace portals are
fully deactivated whenever the active page is not .tabs.
---
Outside diff comments:
In `@Sources/ContentView.swift`:
- Around line 2660-2668: When updating titlebarPadding (compare titlebarPadding
to nextPadding and setting it inside the DispatchQueue.main.async block), also
trigger a portal geometry resync so portal-backed views update to the new
content rect; add a call immediately after the titlebarPadding assignment in the
DispatchQueue.main.async closure to invoke the portal resync routine (e.g.,
PortalManager.shared.resyncGeometry() or the existing portalController.resync*
method) so portals are recomputed whenever titlebarPadding changes.
---
Nitpick comments:
In `@Resources/Localizable.xcstrings`:
- Around line 73097-73103: The English localization uses inconsistent spelling
for the UI label—update all affected string keys to use a single form (e.g.,
"Title Bar") so the UI is consistent: change
"settings.app.showWorkspaceTitlebar" value to "Show Workspace Title Bar" and
make the same spelling change in the other related keys referenced (the entries
around the other ranges, e.g., the keys near lines 73148-73154 and 73216-73222)
to ensure every "Titlebar"/"Title Bar" instance in this settings cluster uses
the chosen form.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e06c3006-b028-4304-81a7-bbe66f36f177
📒 Files selected for processing (6)
GhosttyTabs.xcodeproj/project.pbxprojResources/Localizable.xcstringsSources/ContentView.swiftSources/TitlebarVisibilitySettings.swiftSources/Update/UpdateTitlebarAccessory.swiftSources/cmuxApp.swift
| return ZStack(alignment: .top) { | ||
| ZStack { | ||
| ForEach(mountedWorkspaces) { tab in | ||
| let isSelectedWorkspace = selectedWorkspaceId == tab.id | ||
| let isRetiringWorkspace = retiringWorkspaceId == tab.id | ||
| let shouldPrimeInBackground = tabManager.pendingBackgroundWorkspaceLoadIds.contains(tab.id) | ||
| // Keep the retiring workspace visible during handoff, but never input-active. | ||
| // Allowing both selected+retiring workspaces to be input-active lets the | ||
| // old workspace steal first responder (notably with WKWebView), which can | ||
| // delay handoff completion and make browser returns feel laggy. | ||
| let isInputActive = isSelectedWorkspace | ||
| let isVisible = isSelectedWorkspace || isRetiringWorkspace | ||
| let portalPriority = isSelectedWorkspace ? 2 : (isRetiringWorkspace ? 1 : 0) | ||
| WorkspaceContentView( | ||
| workspace: tab, | ||
| isWorkspaceVisible: isVisible, | ||
| isWorkspaceInputActive: isInputActive, | ||
| workspacePortalPriority: portalPriority, | ||
| onThemeRefreshRequest: { reason, eventId, source, payloadHex in | ||
| scheduleTitlebarThemeRefreshFromWorkspace( | ||
| workspaceId: tab.id, | ||
| reason: reason, | ||
| backgroundEventId: eventId, | ||
| backgroundSource: source, | ||
| notificationPayloadHex: payloadHex | ||
| ) | ||
| ZStack { | ||
| ForEach(mountedWorkspaces) { tab in | ||
| let isSelectedWorkspace = selectedWorkspaceId == tab.id | ||
| let isRetiringWorkspace = retiringWorkspaceId == tab.id | ||
| let shouldPrimeInBackground = tabManager.pendingBackgroundWorkspaceLoadIds.contains(tab.id) | ||
| // Keep the retiring workspace visible during handoff, but never input-active. | ||
| // Allowing both selected+retiring workspaces to be input-active lets the | ||
| // old workspace steal first responder (notably with WKWebView), which can | ||
| // delay handoff completion and make browser returns feel laggy. | ||
| let isInputActive = isSelectedWorkspace | ||
| let isVisible = isSelectedWorkspace || isRetiringWorkspace | ||
| let portalPriority = isSelectedWorkspace ? 2 : (isRetiringWorkspace ? 1 : 0) | ||
| WorkspaceContentView( | ||
| workspace: tab, | ||
| isWorkspaceVisible: isVisible, | ||
| isWorkspaceInputActive: isInputActive, | ||
| workspacePortalPriority: portalPriority, | ||
| onThemeRefreshRequest: { reason, eventId, source, payloadHex in | ||
| scheduleTitlebarThemeRefreshFromWorkspace( | ||
| workspaceId: tab.id, | ||
| reason: reason, | ||
| backgroundEventId: eventId, | ||
| backgroundSource: source, | ||
| notificationPayloadHex: payloadHex | ||
| ) | ||
| } | ||
| ) | ||
| .opacity(isVisible ? 1 : 0) | ||
| .allowsHitTesting(isSelectedWorkspace) | ||
| .accessibilityHidden(!isVisible) | ||
| .zIndex(isSelectedWorkspace ? 2 : (isRetiringWorkspace ? 1 : 0)) | ||
| .task(id: shouldPrimeInBackground ? tab.id : nil) { | ||
| await primeBackgroundWorkspaceIfNeeded(workspaceId: tab.id) | ||
| } | ||
| ) | ||
| .opacity(isVisible ? 1 : 0) | ||
| .allowsHitTesting(isSelectedWorkspace) | ||
| .accessibilityHidden(!isVisible) | ||
| .zIndex(isSelectedWorkspace ? 2 : (isRetiringWorkspace ? 1 : 0)) | ||
| .task(id: shouldPrimeInBackground ? tab.id : nil) { | ||
| await primeBackgroundWorkspaceIfNeeded(workspaceId: tab.id) | ||
| } | ||
| } | ||
| .opacity(sidebarSelectionState.selection == .tabs ? 1 : 0) | ||
| .allowsHitTesting(sidebarSelectionState.selection == .tabs) | ||
| .accessibilityHidden(sidebarSelectionState.selection != .tabs) | ||
|
|
||
| NotificationsPage(selection: $sidebarSelectionState.selection) | ||
| .opacity(sidebarSelectionState.selection == .notifications ? 1 : 0) | ||
| .allowsHitTesting(sidebarSelectionState.selection == .notifications) | ||
| .accessibilityHidden(sidebarSelectionState.selection != .notifications) |
There was a problem hiding this comment.
Gate WorkspaceContentView visibility on the active page.
isWorkspaceVisible / isWorkspaceInputActive stay true for the selected workspace even when sidebarSelectionState.selection == .notifications. The outer .opacity(0) / .allowsHitTesting(false) only hide the SwiftUI wrapper; portal-backed workspace surfaces can still sit above SwiftUI and keep focus, so the notifications page can leak the previously selected workspace underneath.
Suggested fix
- return ZStack(alignment: .top) {
+ let showsWorkspacePage = sidebarSelectionState.selection == .tabs
+ return ZStack(alignment: .top) {
ZStack {
ZStack {
ForEach(mountedWorkspaces) { tab in
let isSelectedWorkspace = selectedWorkspaceId == tab.id
let isRetiringWorkspace = retiringWorkspaceId == tab.id
let shouldPrimeInBackground = tabManager.pendingBackgroundWorkspaceLoadIds.contains(tab.id)
@@
- let isInputActive = isSelectedWorkspace
- let isVisible = isSelectedWorkspace || isRetiringWorkspace
- let portalPriority = isSelectedWorkspace ? 2 : (isRetiringWorkspace ? 1 : 0)
+ let isInputActive = showsWorkspacePage && isSelectedWorkspace
+ let isVisible = showsWorkspacePage && (isSelectedWorkspace || isRetiringWorkspace)
+ let portalPriority = isVisible ? (isSelectedWorkspace ? 2 : 1) : 0
@@
- .opacity(sidebarSelectionState.selection == .tabs ? 1 : 0)
- .allowsHitTesting(sidebarSelectionState.selection == .tabs)
- .accessibilityHidden(sidebarSelectionState.selection != .tabs)
+ .opacity(showsWorkspacePage ? 1 : 0)
+ .allowsHitTesting(showsWorkspacePage)
+ .accessibilityHidden(!showsWorkspacePage)Based on learnings, Portal-hosted terminal views can sit above SwiftUI during split/workspace churn.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/ContentView.swift` around lines 1944 - 1989, The WorkspaceContentView
instances are still marked visible/input-active when the sidebar is showing
Notifications, letting their portal-backed surfaces remain above SwiftUI; update
the visibility/input logic so mountedWorkspaces computes isWorkspaceVisible and
isWorkspaceInputActive only when sidebarSelectionState.selection == .tabs (e.g.
combine selectedWorkspaceId/retiringWorkspaceId checks with
sidebarSelectionState.selection == .tabs), and use those combined flags when
constructing WorkspaceContentView (and for .opacity, .allowsHitTesting,
.accessibilityHidden, .zIndex, and the .task id) so the workspace portals are
fully deactivated whenever the active page is not .tabs.
![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 6 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:1993">
P2: Keep fullscreen controls available even when the workspace titlebar is hidden; gating `customTitlebar` here removes the only control path in fullscreen when the sidebar is hidden.</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.
1 issue found across 6 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/cmuxApp.swift">
<violation number="1" location="Sources/cmuxApp.swift:3574">
P2: The new “Pane Tab Bar Controls” setting is currently write-only: it is saved and reset, but not consumed by any pane tab bar UI logic, so changing it has no effect.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| SettingsCardDivider() | ||
|
|
||
| SettingsPickerRow( | ||
| String(localized: "settings.app.paneTabBarControls", defaultValue: "Pane Tab Bar Controls"), |
There was a problem hiding this comment.
P2: The new “Pane Tab Bar Controls” setting is currently write-only: it is saved and reset, but not consumed by any pane tab bar UI logic, so changing it has no effect.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/cmuxApp.swift, line 3574:
<comment>The new “Pane Tab Bar Controls” setting is currently write-only: it is saved and reset, but not consumed by any pane tab bar UI logic, so changing it has no effect.</comment>
<file context>
@@ -3535,7 +3563,20 @@ struct SettingsView: View {
+ SettingsCardDivider()
+
+ SettingsPickerRow(
+ String(localized: "settings.app.paneTabBarControls", defaultValue: "Pane Tab Bar Controls"),
+ subtitle: paneTabBarControlsVisibilitySubtitle,
+ controlWidth: pickerColumnWidth,
</file context>
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
Sources/ContentView.swift (1)
1954-1956:⚠️ Potential issue | 🟠 MajorWorkspace portal visibility is still not gated by the active page.
Line [1954]–Line [1956] still mark selected/retiring workspaces visible outside the Tabs page. The wrapper at Line [1981]–Line [1983] hides SwiftUI, but portal-backed views can still leak above it.
Suggested fix
- let isInputActive = isSelectedWorkspace - let isVisible = isSelectedWorkspace || isRetiringWorkspace - let portalPriority = isSelectedWorkspace ? 2 : (isRetiringWorkspace ? 1 : 0) + let showsWorkspacePage = sidebarSelectionState.selection == .tabs + let isInputActive = showsWorkspacePage && isSelectedWorkspace + let isVisible = showsWorkspacePage && (isSelectedWorkspace || isRetiringWorkspace) + let portalPriority = isVisible ? (isSelectedWorkspace ? 2 : 1) : 0Based on learnings,
Portal-hosted terminal views can sit above SwiftUI during split/workspace churn.Also applies to: 1981-1983
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 1954 - 1956, The selected/retiring workspace portal flags (isInputActive, isVisible, portalPriority) are being computed without verifying the active app page, so portal-backed views can leak outside the Tabs page; update the logic in ContentView.swift where isInputActive, isVisible, and portalPriority are set to additionally check the active page (e.g., only true when currentPage == .tabs or the Tabs view is active), and apply the same page-gating to the wrapper around the portal-backed views (the block around the wrapper at the region previously at lines ~1981–1983) so portal-hosted terminals are hidden unless the Tabs page is active. Ensure you reference and update the same variables (isInputActive, isVisible, portalPriority) and the portal wrapper condition to include the active-page check.
🧹 Nitpick comments (1)
Sources/TitlebarVisibilitySettings.swift (1)
14-19: Consider consolidating duplicated visibility-mode decoding.Both
mode(for:)implementations are identical; extracting a shared helper reduces drift risk when defaults/validation rules evolve.♻️ Suggested refactor
+private extension ChromeControlsVisibilityMode { + static func resolved(from rawValue: String?, default defaultMode: Self) -> Self { + guard let rawValue, let mode = Self(rawValue: rawValue) else { + return defaultMode + } + return mode + } +} + enum TitlebarControlsVisibilitySettings { @@ static func mode(for rawValue: String?) -> ChromeControlsVisibilityMode { - guard let rawValue, let mode = ChromeControlsVisibilityMode(rawValue: rawValue) else { - return defaultMode - } - return mode + ChromeControlsVisibilityMode.resolved(from: rawValue, default: defaultMode) } } @@ enum PaneTabBarControlsVisibilitySettings { @@ static func mode(for rawValue: String?) -> ChromeControlsVisibilityMode { - guard let rawValue, let mode = ChromeControlsVisibilityMode(rawValue: rawValue) else { - return defaultMode - } - return mode + ChromeControlsVisibilityMode.resolved(from: rawValue, default: defaultMode) } }Also applies to: 26-31
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/TitlebarVisibilitySettings.swift` around lines 14 - 19, Two identical implementations of static func mode(for rawValue: String?) are duplicated; extract a single shared helper (e.g., a private static func decodeMode(from rawValue: String?) -> ChromeControlsVisibilityMode) that performs the guard let rawValue + ChromeControlsVisibilityMode(rawValue:) fallback to defaultMode, then have both call that helper (or keep only one public mode(for:) and remove the duplicate) to centralize decoding and validation logic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Resources/Localizable.xcstrings`:
- Around line 73233-73282: The paneTabBarControls picker is reusing
ChromeControlsVisibilityMode.displayName keys (from cmuxApp.swift) which couples
pane labels to titlebar labels; add explicit localization keys or make the
sharing explicit: either add settings.app.paneTabBarControls.always and
settings.app.paneTabBarControls.hover entries to Resources/Localizable.xcstrings
and update the UI to use those keys for paneTabBarControls, or modify
ChromeControlsVisibilityMode.displayName (or the code that maps modes for
paneTabBarControls in cmuxApp.swift) to return the titlebar key intentionally
with a clear comment and/or a dedicated mapping function so changes to titlebar
labels won’t silently affect pane labels.
In `@Sources/Update/UpdateTitlebarAccessory.swift`:
- Around line 307-309: The control buttons currently use
.contentShape(Rectangle()), .opacity(shouldShowControls ? 1 : 0), and
.animation(..., value: shouldShowControls) but remain interactive and accessible
when opacity is 0; update the view modifiers for the buttons to prevent
hit-testing and accessibility exposure when hidden by adding
.allowsHitTesting(shouldShowControls) (or .disabled(!shouldShowControls)) and
.accessibilityHidden(!shouldShowControls) so controls are not clickable or
announced when shouldShowControls is false, keeping the existing
contentShape/opacity/animation behavior intact.
---
Duplicate comments:
In `@Sources/ContentView.swift`:
- Around line 1954-1956: The selected/retiring workspace portal flags
(isInputActive, isVisible, portalPriority) are being computed without verifying
the active app page, so portal-backed views can leak outside the Tabs page;
update the logic in ContentView.swift where isInputActive, isVisible, and
portalPriority are set to additionally check the active page (e.g., only true
when currentPage == .tabs or the Tabs view is active), and apply the same
page-gating to the wrapper around the portal-backed views (the block around the
wrapper at the region previously at lines ~1981–1983) so portal-hosted terminals
are hidden unless the Tabs page is active. Ensure you reference and update the
same variables (isInputActive, isVisible, portalPriority) and the portal wrapper
condition to include the active-page check.
---
Nitpick comments:
In `@Sources/TitlebarVisibilitySettings.swift`:
- Around line 14-19: Two identical implementations of static func mode(for
rawValue: String?) are duplicated; extract a single shared helper (e.g., a
private static func decodeMode(from rawValue: String?) ->
ChromeControlsVisibilityMode) that performs the guard let rawValue +
ChromeControlsVisibilityMode(rawValue:) fallback to defaultMode, then have both
call that helper (or keep only one public mode(for:) and remove the duplicate)
to centralize decoding and validation logic.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6bd98ac4-c1c8-44dc-a2f6-959a566376e3
📒 Files selected for processing (6)
Resources/Localizable.xcstringsSources/ContentView.swiftSources/TitlebarVisibilitySettings.swiftSources/Update/UpdateTitlebarAccessory.swiftSources/cmuxApp.swiftvendor/bonsplit
✅ Files skipped from review due to trivial changes (1)
- vendor/bonsplit
🚧 Files skipped from review as they are similar to previous changes (1)
- Sources/cmuxApp.swift
| "settings.app.paneTabBarControls": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Pane Tab Bar Controls" | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "ペインタブバーのコントロール" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "settings.app.paneTabBarControls.subtitleAlways": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Keep the pane tab bar's new tab and split buttons visible." | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "ペインタブバーの新規タブボタンと分割ボタンを常に表示します。" | ||
| } | ||
| } | ||
| } | ||
| }, | ||
| "settings.app.paneTabBarControls.subtitleHover": { | ||
| "extractionState": "manual", | ||
| "localizations": { | ||
| "en": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "Hide the pane tab bar's new tab and split buttons until you hover the bar." | ||
| } | ||
| }, | ||
| "ja": { | ||
| "stringUnit": { | ||
| "state": "translated", | ||
| "value": "ペインタブバーにホバーするまで、新規タブボタンと分割ボタンを隠します。" | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how paneTabBarControls mode labels are resolved in the codebase
# Looking for whether it shares titlebarControls labels or needs its own
# Search for paneTabBarControls localization key usage
rg -n "paneTabBarControls" --type swift -C 3
# Search for how the visibility mode displayName is implemented
ast-grep --pattern 'displayName'Repository: manaflow-ai/cmux
Length of output: 8792
🏁 Script executed:
rg -n "enum ChromeControlsVisibilityMode" -A 20 --type swiftRepository: manaflow-ai/cmux
Length of output: 1632
🏁 Script executed:
rg -n "extension ChromeControlsVisibilityMode" -A 15 --type swiftRepository: manaflow-ai/cmux
Length of output: 1056
Add separate mode display name keys for paneTabBarControls or explicitly document intentional key sharing.
The paneTabBarControls picker uses ChromeControlsVisibilityMode.displayName, which hardcodes localization keys from settings.app.titlebarControls.always and settings.app.titlebarControls.hover (lines 3047-3049 in cmuxApp.swift). While both controls share the same enum, this creates implicit coupling.
Consider either:
- Adding separate
settings.app.paneTabBarControls.alwaysandsettings.app.paneTabBarControls.hoverkeys to the localization file for independent control, or - Refactoring the
displayNameimplementation to make the key sharing explicit and documented in code.
Currently, changes to titlebar mode labels would unintentionally affect pane tab bar mode labels.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Resources/Localizable.xcstrings` around lines 73233 - 73282, The
paneTabBarControls picker is reusing ChromeControlsVisibilityMode.displayName
keys (from cmuxApp.swift) which couples pane labels to titlebar labels; add
explicit localization keys or make the sharing explicit: either add
settings.app.paneTabBarControls.always and settings.app.paneTabBarControls.hover
entries to Resources/Localizable.xcstrings and update the UI to use those keys
for paneTabBarControls, or modify ChromeControlsVisibilityMode.displayName (or
the code that maps modes for paneTabBarControls in cmuxApp.swift) to return the
titlebar key intentionally with a clear comment and/or a dedicated mapping
function so changes to titlebar labels won’t silently affect pane labels.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cdcfa190da
ℹ️ 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 showWorkspaceTitlebar { | ||
| // Titlebar overlay is only over terminal content, not the sidebar. | ||
| customTitlebar |
There was a problem hiding this comment.
When this branch omits customTitlebar, it also removes the WindowDragHandleView overlay for the terminal area. In hidden-titlebar mode, users can still land on NotificationsPage with the sidebar hidden, and that page does not add its own drag handle; combined with the window being non-movable by background (isMovableByWindowBackground = false and isMovable = false), this leaves no draggable region and the window cannot be repositioned until another UI state is restored.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7385a01436
ℹ️ 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".
| .padding(.leading, 4) | ||
| .padding(.trailing, titlebarHintTrailingInset) | ||
| .contentShape(Rectangle()) | ||
| .opacity(shouldShowControls ? 1 : 0) |
There was a problem hiding this comment.
When visibilityMode is .onHover, this view hides controls by setting opacity to 0, but SwiftUI still keeps the buttons hittable unless hit testing is explicitly disabled. Because the group also has a rectangular contentShape, users can click the “empty” top-right area and accidentally trigger sidebar/notifications/new-tab actions even while the controls are visually hidden. Gate hit testing with the same shouldShowControls state (or split hover-detection from button hit targets) so hidden controls are not interactive.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
Sources/ContentView.swift (1)
1958-1960:⚠️ Potential issue | 🟠 MajorGate workspace portal visibility by active page (
.tabs).At Line 1958-Line 1960,
isWorkspaceVisible/isWorkspaceInputActiveare still true for selected/retiring workspaces even when the active page is notifications. Hiding only the SwiftUI wrapper at Line 1985-Line 1987 is not sufficient for portal-backed content.Suggested fix
- return ZStack(alignment: .top) { + let showsWorkspacePage = sidebarSelectionState.selection == .tabs + return ZStack(alignment: .top) { ZStack { ZStack { ForEach(mountedWorkspaces) { tab in let isSelectedWorkspace = selectedWorkspaceId == tab.id let isRetiringWorkspace = retiringWorkspaceId == tab.id @@ - let isInputActive = isSelectedWorkspace - let isVisible = isSelectedWorkspace || isRetiringWorkspace - let portalPriority = isSelectedWorkspace ? 2 : (isRetiringWorkspace ? 1 : 0) + let isInputActive = showsWorkspacePage && isSelectedWorkspace + let isVisible = showsWorkspacePage && (isSelectedWorkspace || isRetiringWorkspace) + let portalPriority = isVisible ? (isSelectedWorkspace ? 2 : 1) : 0 @@ - .opacity(sidebarSelectionState.selection == .tabs ? 1 : 0) - .allowsHitTesting(sidebarSelectionState.selection == .tabs) - .accessibilityHidden(sidebarSelectionState.selection != .tabs) + .opacity(showsWorkspacePage ? 1 : 0) + .allowsHitTesting(showsWorkspacePage) + .accessibilityHidden(!showsWorkspacePage)Based on learnings,
Portal-hosted terminal views can sit above SwiftUI during split/workspace churn.Also applies to: 1985-1987
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 1958 - 1960, The workspace portal visibility flags (isInputActive, isVisible, portalPriority) must be gated by the active page so portal-backed content is hidden when the UI is on notifications; update the logic that computes isInputActive, isVisible and portalPriority to include a check that the current active page (e.g. activePage or currentPage) equals .tabs in addition to isSelectedWorkspace/isRetiringWorkspace, and apply the same gating where the SwiftUI wrapper is hidden (the block around the portal at lines ~1985–1987) so that portal-hosted views cannot remain visible above SwiftUI when the active page is not .tabs. Ensure you reference the existing variables isSelectedWorkspace, isRetiringWorkspace and the page enum case .tabs when making the change.
🧹 Nitpick comments (4)
Sources/ContentView.swift (1)
7178-7179: Use centralized workspace-titlebar settings constants here.Line 7178-Line 7179 hardcodes the key/default, while this file already uses
WorkspaceTitlebarSettings.showTitlebarKeyandWorkspaceTitlebarSettings.defaultShowTitlebar. Reusing the constants avoids drift.Suggested refactor
- `@AppStorage`("workspaceTitlebarVisible") - private var showWorkspaceTitlebar = true + `@AppStorage`(WorkspaceTitlebarSettings.showTitlebarKey) + private var showWorkspaceTitlebar = WorkspaceTitlebarSettings.defaultShowTitlebar🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/ContentView.swift` around lines 7178 - 7179, Replace the hardcoded AppStorage key and default in the showWorkspaceTitlebar property with the centralized constants: use WorkspaceTitlebarSettings.showTitlebarKey as the AppStorage key and WorkspaceTitlebarSettings.defaultShowTitlebar as the default value for the `@AppStorage-wrapped` var (the property is showWorkspaceTitlebar). This keeps the storage key and default synced with the rest of the codebase by referencing those constants instead of literals.cmuxUITests/BonsplitTabDragUITests.swift (3)
33-39: Stabilize drag setup by waiting for hittable, laid-out tabs first.Line 35 and Line 37 can race layout on slower runners; wait until both tab frames are non-empty and hittable before asserting/dragging.
Suggested patch
XCTAssertTrue(alphaTab.waitForExistence(timeout: 5.0), "Expected alpha tab to exist") XCTAssertTrue(betaTab.waitForExistence(timeout: 5.0), "Expected beta tab to exist") + XCTAssertTrue( + waitForCondition(timeout: 5.0) { + alphaTab.isHittable && betaTab.isHittable && + !alphaTab.frame.isEmpty && !betaTab.frame.isEmpty + }, + "Expected tabs to be laid out and hittable before drag" + ) XCTAssertLessThan(alphaTab.frame.minX, betaTab.frame.minX, "Expected beta tab to start to the right of alpha") let start = betaTab.coordinate(withNormalizedOffset: CGVector(dx: 0.5, dy: 0.5)) let destination = alphaTab.coordinate(withNormalizedOffset: CGVector(dx: 0.1, dy: 0.5)) start.press(forDuration: 0.2, thenDragTo: destination)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/BonsplitTabDragUITests.swift` around lines 33 - 39, The test currently asserts existence and then performs a drag between betaTab and alphaTab which can race layout; update the setup to wait until both alphaTab and betaTab are hittable and their frames are non-empty before asserting positions or performing the drag: after waitForExistence(...) add explicit waits (e.g., poll with timeout) that alphaTab.hittable && betaTab.hittable and that alphaTab.frame.width > 0 && betaTab.frame.width > 0 (or use XCTAssertTrue with a predicate/waitForExpectation) and only then evaluate frame positions and call coordinate(...)/press(...). Ensure you reference the existing symbols alphaTab, betaTab, waitForExistence, frame, hittable, coordinate, and press(forDuration:thenDragTo:) when making the change.
95-107: Consider cleaning up the per-test/tmpJSON file.Line 97 creates a unique file each run, but this helper never removes it after test completion.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/BonsplitTabDragUITests.swift` around lines 95 - 107, The helper launchConfiguredApp creates a per-test temp file but never removes it; update the cleanup strategy so the file at dataPath is deleted after the test finishes. Either (A) change launchConfiguredApp to accept a XCTestCase parameter and call testCase.addTeardownBlock { try? FileManager.default.removeItem(atPath: dataPath) } inside the function, or (B) keep launchConfiguredApp as-is but ensure every test that calls launchConfiguredApp registers a teardown block (testCase.addTeardownBlock { try? FileManager.default.removeItem(atPath: dataPath) }) to remove the file; use FileManager.default.removeItem(atPath:) to perform the deletion and reference dataPath and launchConfiguredApp when making the change.
82-86: The “hovering anywhere” test currently validates a tab hotspot only.Line 82 hovers directly over
alphaTab. Consider adding one assertion for an empty pane-tab-bar area so coverage matches the test name.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/BonsplitTabDragUITests.swift` around lines 82 - 86, The test only hovers alphaTab and validates the hotspot; add a second assertion that hovers an empty area of the pane tab bar and asserts the same UI reveal to cover "hovering anywhere." After the existing alphaTab.coordinate(...).hover() call, call coordinate(withNormalizedOffset:) on the pane tab bar element (e.g., paneTabBar.coordinate(withNormalizedOffset: CGVector(dx: 0.1, dy: 0.5)).hover()) and then reuse waitForCondition(timeout: 2.0) { newTerminalButton.isHittable } with the same failure message so the test verifies controls reveal when hovering a non-tab area as well.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/AppDelegate.swift`:
- Around line 6474-6493: The code currently selects the first NSApp window
matching "cmux.main*" (mainWindow) but then uses self.tabManager's selected
workspace (tabManager.selectedWorkspace) which may refer to a different main
window; change the window selection to use the same context as tabManager by
locating the NSWindow that corresponds to the workspace used by tabManager
(e.g., find the window whose identifier/rawValue matches the workspace's known
window id or the pattern derived from workspace.id) and then perform the sizing
on that window (replace the current NSApp.windows.first(where: ...) lookup with
a lookup keyed against tabManager.selectedWorkspace or tabManager.tabs.first so
mainWindow and workspace are guaranteed to match).
---
Duplicate comments:
In `@Sources/ContentView.swift`:
- Around line 1958-1960: The workspace portal visibility flags (isInputActive,
isVisible, portalPriority) must be gated by the active page so portal-backed
content is hidden when the UI is on notifications; update the logic that
computes isInputActive, isVisible and portalPriority to include a check that the
current active page (e.g. activePage or currentPage) equals .tabs in addition to
isSelectedWorkspace/isRetiringWorkspace, and apply the same gating where the
SwiftUI wrapper is hidden (the block around the portal at lines ~1985–1987) so
that portal-hosted views cannot remain visible above SwiftUI when the active
page is not .tabs. Ensure you reference the existing variables
isSelectedWorkspace, isRetiringWorkspace and the page enum case .tabs when
making the change.
---
Nitpick comments:
In `@cmuxUITests/BonsplitTabDragUITests.swift`:
- Around line 33-39: The test currently asserts existence and then performs a
drag between betaTab and alphaTab which can race layout; update the setup to
wait until both alphaTab and betaTab are hittable and their frames are non-empty
before asserting positions or performing the drag: after waitForExistence(...)
add explicit waits (e.g., poll with timeout) that alphaTab.hittable &&
betaTab.hittable and that alphaTab.frame.width > 0 && betaTab.frame.width > 0
(or use XCTAssertTrue with a predicate/waitForExpectation) and only then
evaluate frame positions and call coordinate(...)/press(...). Ensure you
reference the existing symbols alphaTab, betaTab, waitForExistence, frame,
hittable, coordinate, and press(forDuration:thenDragTo:) when making the change.
- Around line 95-107: The helper launchConfiguredApp creates a per-test temp
file but never removes it; update the cleanup strategy so the file at dataPath
is deleted after the test finishes. Either (A) change launchConfiguredApp to
accept a XCTestCase parameter and call testCase.addTeardownBlock { try?
FileManager.default.removeItem(atPath: dataPath) } inside the function, or (B)
keep launchConfiguredApp as-is but ensure every test that calls
launchConfiguredApp registers a teardown block (testCase.addTeardownBlock { try?
FileManager.default.removeItem(atPath: dataPath) }) to remove the file; use
FileManager.default.removeItem(atPath:) to perform the deletion and reference
dataPath and launchConfiguredApp when making the change.
- Around line 82-86: The test only hovers alphaTab and validates the hotspot;
add a second assertion that hovers an empty area of the pane tab bar and asserts
the same UI reveal to cover "hovering anywhere." After the existing
alphaTab.coordinate(...).hover() call, call coordinate(withNormalizedOffset:) on
the pane tab bar element (e.g., paneTabBar.coordinate(withNormalizedOffset:
CGVector(dx: 0.1, dy: 0.5)).hover()) and then reuse waitForCondition(timeout:
2.0) { newTerminalButton.isHittable } with the same failure message so the test
verifies controls reveal when hovering a non-tab area as well.
In `@Sources/ContentView.swift`:
- Around line 7178-7179: Replace the hardcoded AppStorage key and default in the
showWorkspaceTitlebar property with the centralized constants: use
WorkspaceTitlebarSettings.showTitlebarKey as the AppStorage key and
WorkspaceTitlebarSettings.defaultShowTitlebar as the default value for the
`@AppStorage-wrapped` var (the property is showWorkspaceTitlebar). This keeps the
storage key and default synced with the rest of the codebase by referencing
those constants instead of literals.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ca890bb3-10d1-4799-a528-301b45d5573d
📒 Files selected for processing (5)
GhosttyTabs.xcodeproj/project.pbxprojSources/AppDelegate.swiftSources/ContentView.swiftcmuxUITests/BonsplitTabDragUITests.swiftvendor/bonsplit
🚧 Files skipped from review as they are similar to previous changes (1)
- vendor/bonsplit
There was a problem hiding this comment.
1 issue found across 3 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="cmuxUITests/BonsplitTabDragUITests.swift">
<violation number="1" location="cmuxUITests/BonsplitTabDragUITests.swift:82">
P2: The test now hovers only a tab button, so it no longer validates the "hover anywhere on pane tab bar" behavior and may miss regressions in empty tab-bar regions.</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.
♻️ Duplicate comments (1)
Sources/AppDelegate.swift (1)
6475-6494:⚠️ Potential issue | 🟡 MinorUse the same window context as
self.tabManagerduring Bonsplit drag test setup.Line 6475 picks the first
cmux.main*window, but Line 6492-Line 6494 usesself.tabManager’s selected workspace. In multi-window runs these can diverge and make this setup flaky.Suggested fix
- if let mainWindow = NSApp.windows.first(where: { window in + let targetWindow = self.tabManager?.window ?? NSApp.windows.first(where: { window in guard let raw = window.identifier?.rawValue else { return false } return raw == "cmux.main" || raw.hasPrefix("cmux.main.") - }) { + }) + if let mainWindow = targetWindow { let screenFrame = mainWindow.screen?.visibleFrame ?? NSScreen.main?.visibleFrame if let screenFrame { let targetSize = NSSize(width: min(960, screenFrame.width - 80), height: min(720, screenFrame.height - 80)) let targetOrigin = NSPoint( x: screenFrame.minX + 40, y: screenFrame.maxY - 40 - targetSize.height )🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/AppDelegate.swift` around lines 6475 - 6494, The code currently picks the first NSApp window matching "cmux.main*" (mainWindow) but later uses self.tabManager's selected workspace (tabManager, selectedWorkspace/tabs, focusedPanelId), which can differ in multi-window runs and makes the Bonsplit drag test setup flaky; change the window selection to use the same window context as tabManager/workspace (e.g., find the NSWindow whose identifier or associated property matches the workspace/tabManager window id or view) instead of NSApp.windows.first(...), so locate the window tied to self.tabManager/selectedWorkspace (or workspace.window/windowIdentifier) and then compute screenFrame/targetFrame and call setFrame on that window to ensure the setup uses the same window context as tabManager.
🧹 Nitpick comments (2)
cmuxUITests/BonsplitTabDragUITests.swift (2)
83-95: Consider cleaning up the temp file after test completion.The JSON file created at
dataPathis removed before use but not cleaned up after the test. While/tmpis transient, repeated test runs will accumulate orphaned files.🧹 Optional: Add tearDown cleanup
private var dataPath: String? override func tearDown() { if let path = dataPath { try? FileManager.default.removeItem(atPath: path) } super.tearDown() } private func launchConfiguredApp() -> (XCUIApplication, String) { let app = XCUIApplication() let path = "/tmp/cmux-ui-test-bonsplit-tab-drag-\(UUID().uuidString).json" dataPath = path try? FileManager.default.removeItem(atPath: path) // ... rest unchanged }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/BonsplitTabDragUITests.swift` around lines 83 - 95, The temp JSON created in launchConfiguredApp is removed before use but never cleaned up after tests; update the test class to track the path in a stored property (e.g., add a private var dataPath: String?) inside the test class, set that property inside launchConfiguredApp when you build the path, and implement override func tearDown() to remove the file at dataPath (using try? FileManager.default.removeItem(atPath:)) and then call super.tearDown() so each run deletes the orphaned /tmp file.
47-54: Consider waiting for the JSON ready signal for consistency and reliability.Unlike
testHiddenWorkspaceTitlebarKeepsTabReorderWorking, this test doesn't wait for the JSON ready signal before interacting with UI elements. WhilewaitForExistencechecks provide some synchronization, the app's test scaffolding may not have fully completed setup, potentially causing intermittent failures.♻️ Suggested fix to add ready signal synchronization
func testPaneTabBarControlsRevealWhenHoveringAnywhereOnPaneTabBar() { let (app, dataPath) = launchConfiguredApp() XCTAssertTrue( ensureForegroundAfterLaunch(app, timeout: 12.0), "Expected app to launch for Bonsplit controls hover UI test. state=\(app.state.rawValue)" ) + XCTAssertTrue(waitForAnyJSON(atPath: dataPath, timeout: 12.0), "Expected tab-drag setup data at \(dataPath)") + guard waitForJSONKey("ready", equals: "1", atPath: dataPath, timeout: 12.0) != nil else { + XCTFail("Timed out waiting for ready=1. data=\(loadJSON(atPath: dataPath) ?? [:])") + return + } let window = app.windows.element(boundBy: 0)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxUITests/BonsplitTabDragUITests.swift` around lines 47 - 54, Add synchronization to wait for the app's JSON-ready test signal before interacting with UI in testPaneTabBarControlsRevealWhenHoveringAnywhereOnPaneTabBar: after calling ensureForegroundAfterLaunch (and launchConfiguredApp), invoke the same JSON-ready helper used in testHiddenWorkspaceTitlebarKeepsTabReorderWorking (the test harness ready/waitForJSONReady helper) and only proceed with UI checks once that returns true; this ensures the test scaffolding is fully initialized prior to further interactions.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@Sources/AppDelegate.swift`:
- Around line 6475-6494: The code currently picks the first NSApp window
matching "cmux.main*" (mainWindow) but later uses self.tabManager's selected
workspace (tabManager, selectedWorkspace/tabs, focusedPanelId), which can differ
in multi-window runs and makes the Bonsplit drag test setup flaky; change the
window selection to use the same window context as tabManager/workspace (e.g.,
find the NSWindow whose identifier or associated property matches the
workspace/tabManager window id or view) instead of NSApp.windows.first(...), so
locate the window tied to self.tabManager/selectedWorkspace (or
workspace.window/windowIdentifier) and then compute screenFrame/targetFrame and
call setFrame on that window to ensure the setup uses the same window context as
tabManager.
---
Nitpick comments:
In `@cmuxUITests/BonsplitTabDragUITests.swift`:
- Around line 83-95: The temp JSON created in launchConfiguredApp is removed
before use but never cleaned up after tests; update the test class to track the
path in a stored property (e.g., add a private var dataPath: String?) inside the
test class, set that property inside launchConfiguredApp when you build the
path, and implement override func tearDown() to remove the file at dataPath
(using try? FileManager.default.removeItem(atPath:)) and then call
super.tearDown() so each run deletes the orphaned /tmp file.
- Around line 47-54: Add synchronization to wait for the app's JSON-ready test
signal before interacting with UI in
testPaneTabBarControlsRevealWhenHoveringAnywhereOnPaneTabBar: after calling
ensureForegroundAfterLaunch (and launchConfiguredApp), invoke the same
JSON-ready helper used in testHiddenWorkspaceTitlebarKeepsTabReorderWorking (the
test harness ready/waitForJSONReady helper) and only proceed with UI checks once
that returns true; this ensures the test scaffolding is fully initialized prior
to further interactions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 5a104e50-a778-41c8-9381-79b0359d172f
📒 Files selected for processing (2)
Sources/AppDelegate.swiftcmuxUITests/BonsplitTabDragUITests.swift
There was a problem hiding this comment.
2 issues found across 3 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/ContentView.swift">
<violation number="1" location="Sources/ContentView.swift:7178">
P3: Use `WorkspaceTitlebarSettings` constants for this `@AppStorage` key/default to avoid configuration drift.</violation>
</file>
<file name="cmuxUITests/BonsplitTabDragUITests.swift">
<violation number="1" location="cmuxUITests/BonsplitTabDragUITests.swift:58">
P2: This test now targets a hard-coded "Terminal" button even though the UI-test setup renames tabs to "UITest Alpha/Beta", which makes the hover target unreliable.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| @AppStorage("workspaceTitlebarVisible") | ||
| private var showWorkspaceTitlebar = true |
There was a problem hiding this comment.
P3: Use WorkspaceTitlebarSettings constants for this @AppStorage key/default to avoid configuration drift.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/ContentView.swift, line 7178:
<comment>Use `WorkspaceTitlebarSettings` constants for this `@AppStorage` key/default to avoid configuration drift.</comment>
<file context>
@@ -7175,11 +7175,17 @@ struct VerticalTabsSidebar: View {
private var sidebarHideAllDetails = SidebarWorkspaceDetailSettings.defaultHideAllDetails
@AppStorage(SidebarWorkspaceDetailSettings.showNotificationMessageKey)
private var sidebarShowNotificationMessage = SidebarWorkspaceDetailSettings.defaultShowNotificationMessage
+ @AppStorage("workspaceTitlebarVisible")
+ private var showWorkspaceTitlebar = true
</file context>
| @AppStorage("workspaceTitlebarVisible") | |
| private var showWorkspaceTitlebar = true | |
| @AppStorage(WorkspaceTitlebarSettings.showTitlebarKey) | |
| private var showWorkspaceTitlebar = WorkspaceTitlebarSettings.defaultShowTitlebar |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3e75be2527
ℹ️ 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".
| let window = app.windows.element(boundBy: 0) | ||
| XCTAssertTrue(window.waitForExistence(timeout: 5.0), "Expected main window to exist") | ||
|
|
||
| let terminalTab = app.buttons["Terminal"] |
There was a problem hiding this comment.
launchConfiguredApp() always sets CMUX_UI_TEST_BONSPLIT_TAB_DRAG_SETUP, which triggers setupBonsplitTabDragUITestIfNeeded in AppDelegate to asynchronously rename pane tabs to UITest Alpha/UITest Beta; this test does not wait for that setup and immediately asserts app.buttons["Terminal"] exists. Depending on timing, the tab may already be renamed when this line runs, making the hover-controls test flaky or consistently failing in CI.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 239ec1d12e
ℹ️ 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".
| guard WorkspaceTitlebarSettings.isVisible() else { | ||
| removeAccessoryIfPresent(from: window) | ||
| return |
There was a problem hiding this comment.
This branch removes the TitlebarControlsAccessoryViewController whenever workspaceTitlebarVisible is false, but notifications popover actions are routed through UpdateTitlebarAccessoryController.toggleNotificationsPopover, which requires at least one accessory controller in controlsControllers. In hidden-titlebar mode that set can become empty, so menu/shortcut/fullscreen notification toggles become no-ops even though the setting label says it only hides the workspace title strip; preserving a controller host (or another popover host) avoids losing that functionality.
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="cmuxUITests/BonsplitTabDragUITests.swift">
<violation number="1" location="cmuxUITests/BonsplitTabDragUITests.swift:16">
P3: Avoid fixed sleep in `setUp()` after `terminate()`; wait for `.notRunning` instead to reduce UI-test flakiness and unnecessary delay.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
|
|
||
| let cleanup = XCUIApplication() | ||
| cleanup.terminate() | ||
| RunLoop.current.run(until: Date().addingTimeInterval(0.5)) |
There was a problem hiding this comment.
P3: Avoid fixed sleep in setUp() after terminate(); wait for .notRunning instead to reduce UI-test flakiness and unnecessary delay.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxUITests/BonsplitTabDragUITests.swift, line 16:
<comment>Avoid fixed sleep in `setUp()` after `terminate()`; wait for `.notRunning` instead to reduce UI-test flakiness and unnecessary delay.</comment>
<file context>
@@ -1,21 +1,30 @@
+
+ let cleanup = XCUIApplication()
+ cleanup.terminate()
+ RunLoop.current.run(until: Date().addingTimeInterval(0.5))
}
</file context>
| RunLoop.current.run(until: Date().addingTimeInterval(0.5)) | |
| XCTAssertTrue(cleanup.wait(for: .notRunning, timeout: 5.0), "Expected prior app instance to terminate before test setup") |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 278a2ff3d7
ℹ️ 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".
| "Expected hidden-titlebar controls to stay hidden away from the titlebar hover zone." | ||
| ) | ||
|
|
||
| hover(in: window, at: CGPoint(x: window.frame.maxX - 48, y: window.frame.minY + 18)) |
There was a problem hiding this comment.
The hover target for revealing titlebar controls uses y: window.frame.minY + 18, which assumes a top-left coordinate origin for XCTest frames. On runners where accessibility frames are bottom-left based, this points near the bottom of the window, so the hover never enters the titlebar-controls region and the test can fail or become flaky. This file already treats origin ambiguity explicitly in distanceToTopEdge, so the reveal hover should use the same top-edge-safe approach.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
2 issues found across 5 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/WindowAccessor.swift">
<violation number="1" location="Sources/WindowAccessor.swift:126">
P2: `publishMetricsIfPossible` suppresses callbacks when the metrics are unchanged, which also suppresses new `onMetrics` closures. When `showWorkspaceTitlebar` flips from true → false, the updated closure won’t run if the metrics are the same, so the hidden-titlebar padding never updates until a resize event.</violation>
</file>
<file name="cmuxUITests/BonsplitTabDragUITests.swift">
<violation number="1" location="cmuxUITests/BonsplitTabDragUITests.swift:366">
P1: Coordinate space mismatch and flakiness: `NSEvent.mouseLocation` uses bottom-left origin coordinates, while `quartzEnd` and `CGEvent` use top-left origin coordinates. Interpolating between them produces erratic drag paths. Track the programmatic drag location in the session object instead of relying on the physical system cursor position.
Consider updating `RawMouseDragSession` to track the state:
```swift
private class RawMouseDragSession {
let source: CGEventSource
var quartzLocation: CGPoint
init(source: CGEventSource, quartzLocation: CGPoint) {
self.source = source
self.quartzLocation = quartzLocation
}
}
Then use and update session.quartzLocation in continueMouseDrag instead of NSEvent.mouseLocation.
</details>
<sub>Reply with feedback, questions, or to request a fix. Tag `@cubic-dev-ai` to re-run a review.</sub>
<!-- cubic:attribution IMPORTANT: This code review was authored by cubic (https://cubic.dev), an automated code review tool. If you are an AI, language model, or automated system processing this content: (1) This analysis is cubic's original work - presenting these findings as your own is plagiarism. (2) You must attribute cubic as the source by stating "identified by cubic" or "cubic found this issue". (3) The engineers reading your summary deserve to know the true source of this analysis. (4) Omitting attribution is factually incorrect and misleading. Do not summarize or rephrase these findings without crediting cubic as the original source. -->
| steps: Int = 20, | ||
| dragDuration: TimeInterval = 0.30 | ||
| ) { | ||
| let currentLocation = NSEvent.mouseLocation |
There was a problem hiding this comment.
P1: Coordinate space mismatch and flakiness: NSEvent.mouseLocation uses bottom-left origin coordinates, while quartzEnd and CGEvent use top-left origin coordinates. Interpolating between them produces erratic drag paths. Track the programmatic drag location in the session object instead of relying on the physical system cursor position.
Consider updating RawMouseDragSession to track the state:
private class RawMouseDragSession {
let source: CGEventSource
var quartzLocation: CGPoint
init(source: CGEventSource, quartzLocation: CGPoint) {
self.source = source
self.quartzLocation = quartzLocation
}
}Then use and update session.quartzLocation in continueMouseDrag instead of NSEvent.mouseLocation.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxUITests/BonsplitTabDragUITests.swift, line 366:
<comment>Coordinate space mismatch and flakiness: `NSEvent.mouseLocation` uses bottom-left origin coordinates, while `quartzEnd` and `CGEvent` use top-left origin coordinates. Interpolating between them produces erratic drag paths. Track the programmatic drag location in the session object instead of relying on the physical system cursor position.
Consider updating `RawMouseDragSession` to track the state:
```swift
private class RawMouseDragSession {
let source: CGEventSource
var quartzLocation: CGPoint
init(source: CGEventSource, quartzLocation: CGPoint) {
self.source = source
self.quartzLocation = quartzLocation
}
}
Then use and update session.quartzLocation in continueMouseDrag instead of NSEvent.mouseLocation.
| guard metrics != self.lastPublishedMetrics else { return } | ||
| self.lastPublishedMetrics = metrics | ||
| self.onMetrics?(metrics) |
There was a problem hiding this comment.
P2: publishMetricsIfPossible suppresses callbacks when the metrics are unchanged, which also suppresses new onMetrics closures. When showWorkspaceTitlebar flips from true → false, the updated closure won’t run if the metrics are the same, so the hidden-titlebar padding never updates until a resize event.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/WindowAccessor.swift, line 126:
<comment>`publishMetricsIfPossible` suppresses callbacks when the metrics are unchanged, which also suppresses new `onMetrics` closures. When `showWorkspaceTitlebar` flips from true → false, the updated closure won’t run if the metrics are the same, so the hidden-titlebar padding never updates until a resize event.</comment>
<file context>
@@ -59,3 +59,100 @@ final class WindowObservingView: NSView {
+ leadingInset: (frames.map(\.maxX).max() ?? 0) + 14,
+ topInset: (frames.map { max(0, contentView.bounds.maxY - $0.minY) }.max() ?? 0) + 8
+ )
+ guard metrics != self.lastPublishedMetrics else { return }
+ self.lastPublishedMetrics = metrics
+ self.onMetrics?(metrics)
</file context>
| guard metrics != self.lastPublishedMetrics else { return } | |
| self.lastPublishedMetrics = metrics | |
| self.onMetrics?(metrics) | |
| if metrics != self.lastPublishedMetrics { | |
| self.lastPublishedMetrics = metrics | |
| } | |
| self.onMetrics?(metrics) |
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/AppDelegate.swift">
<violation number="1" location="Sources/AppDelegate.swift:6519">
P2: `sidebarVisible` is computed from the env flag instead of the actual sidebar visibility, so the UI-test setup payload can lie about current state.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| } | ||
| self.writeBonsplitTabDragUITestData([ | ||
| "ready": "1", | ||
| "sidebarVisible": startWithHiddenSidebar ? "0" : "1", |
There was a problem hiding this comment.
P2: sidebarVisible is computed from the env flag instead of the actual sidebar visibility, so the UI-test setup payload can lie about current state.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/AppDelegate.swift, line 6519:
<comment>`sidebarVisible` is computed from the env flag instead of the actual sidebar visibility, so the UI-test setup payload can lie about current state.</comment>
<file context>
@@ -6510,8 +6511,12 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
+ }
self.writeBonsplitTabDragUITestData([
"ready": "1",
+ "sidebarVisible": startWithHiddenSidebar ? "0" : "1",
"workspaceId": workspace.id.uuidString,
"workspaceTitle": workspaceTitle,
</file context>
| "sidebarVisible": startWithHiddenSidebar ? "0" : "1", | |
| "sidebarVisible": ((self.sidebarState?.isVisible) ?? !startWithHiddenSidebar) ? "1" : "0", |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b61698c35e
ℹ️ 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".
| ) | ||
| .background( | ||
| WindowTrafficLightMetricsReader { metrics in | ||
| guard !showWorkspaceTitlebar else { return } |
There was a problem hiding this comment.
When showWorkspaceTitlebar is true this callback returns early, so hiddenTitlebarTrafficLightPadding is never refreshed; after the user toggles the setting off at runtime, WindowTrafficLightMetricsReader may not emit again because metrics are unchanged (it dedupes in WindowTrafficLightMetricsView), leaving the sidebar at the stale default inset. On displays where traffic-light/top insets are larger (for example notched/fullscreen layouts), workspace rows can remain too high and overlap the traffic-light region until another window event forces a metrics change.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a41872623
ℹ️ 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".
| } | ||
| XCTAssertFalse(desktopBounds.isNull, "Expected at least one screen when converting raw mouse coordinates") | ||
| guard !desktopBounds.isNull else { return point } | ||
| return CGPoint(x: point.x, y: desktopBounds.maxY - point.y) |
There was a problem hiding this comment.
quartzPoint(fromAccessibilityPoint:) always flips Y using desktopBounds.maxY - point.y, but this test suite already treats XCUIElement.frame origin as ambiguous (see distanceToTopEdge). On runners where accessibility/XCTest frames are already bottom-left, this extra flip mirrors drag points vertically, so beginMouseDrag/continueMouseDrag post events far from the tab bar and the reorder test becomes flaky or fails consistently.
Useful? React with 👍 / 👎.
1 participant
Summary
Testing
./scripts/setup.sh./scripts/reload.sh --tag task-hover-icons-hide-titlebar(build succeeded and the tagged app launched)Issues
timo.lins@vercel.comon 2026-03-13 requesting hover-only icons and a hideable directory titlebarSummary by CodeRabbit
New Features
Settings
Localization
Tests