Skip to content

Minimal mode follow-up review fixes #1762

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
lawrencecchen wants to merge 2 commits into from
Closed

Minimal mode follow-up review fixes #1762

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1e98296
Address minimal mode follow-up review comments
lawrencecchen Mar 18, 2026
ee83108
Merge remote-tracking branch 'origin/main' into feat-minimal-mode-rev…
lawrencecchen Mar 25, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions Sources/AppDelegate.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7069,11 +7069,10 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
}

workspace.setPanelCustomTitle(panelId: betaPanelId, title: betaTitle)
if startWithHiddenSidebar {
self.sidebarState?.isVisible = false
}
self.sidebarState?.isVisible = !startWithHiddenSidebar
self.writeBonsplitTabDragUITestData([
"ready": "1",
"setupError": "",
"sidebarVisible": startWithHiddenSidebar ? "0" : "1",
"workspaceId": workspace.id.uuidString,
"workspaceTitle": workspaceTitle,
Expand Down Expand Up @@ -8562,6 +8561,10 @@ final class AppDelegate: NSObject, NSApplicationDelegate, UNUserNotificationCent
titlebarAccessoryController.isNotificationsPopoverShown()
}

func isNotificationsPopoverShown(in window: NSWindow?) -> Bool {
titlebarAccessoryController.isNotificationsPopoverShown(in: window)
}

func jumpToLatestUnread() {
guard let notificationStore else { return }
#if DEBUG
Expand Down
67 changes: 58 additions & 9 deletions Sources/Update/UpdateTitlebarAccessory.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,14 @@ struct TitlebarControlsStyleConfig {

final class TitlebarControlsViewModel: ObservableObject {
weak var notificationsAnchorView: NSView?
private(set) weak var hostWindow: NSWindow?
@Published private(set) var hostWindowNumber: Int?

func setHostWindow(_ window: NSWindow?) {
guard hostWindow !== window else { return }
hostWindow = window
hostWindowNumber = window?.windowNumber
}
}

extension Notification.Name {
Expand All @@ -127,14 +135,25 @@ private enum NotificationsPopoverVisibilityUserInfoKey {
static let isShown = "isShown"
}

private func postNotificationsPopoverVisibilityDidChange(isShown: Bool) {
private func postNotificationsPopoverVisibilityDidChange(isShown: Bool, window: NSWindow?) {
NotificationCenter.default.post(
name: .cmuxNotificationsPopoverVisibilityDidChange,
object: nil,
object: window,
userInfo: [NotificationsPopoverVisibilityUserInfoKey.isShown: isShown]
)
}

func titlebarControlsShouldHandlePopoverVisibilityChange(
hostWindowNumber: Int?,
notificationObject: Any?
) -> Bool {
guard let hostWindowNumber,
let notificationWindow = notificationObject as? NSWindow else {
return false
}
return notificationWindow.windowNumber == hostWindowNumber
}

struct NotificationsAnchorView: NSViewRepresentable {
let onResolve: (NSView) -> Void

Expand Down Expand Up @@ -320,6 +339,7 @@ struct TitlebarControlsView: View {
.animation(.easeInOut(duration: 0.14), value: shouldShowControls)
.background(
WindowAccessor { window in
viewModel.setHostWindow(window)
modifierKeyMonitor.setHostWindow(window)
}
.frame(width: 0, height: 0)
Expand All @@ -331,11 +351,20 @@ struct TitlebarControlsView: View {
shortcutRefreshTick &+= 1
}
.onAppear {
isNotificationsPopoverShown = AppDelegate.shared?.isNotificationsPopoverShown() ?? false
isNotificationsPopoverShown = AppDelegate.shared?.isNotificationsPopoverShown(in: viewModel.hostWindow) ?? false
}
.onReceive(NotificationCenter.default.publisher(for: .cmuxNotificationsPopoverVisibilityDidChange)) { notification in
guard titlebarControlsShouldHandlePopoverVisibilityChange(
hostWindowNumber: viewModel.hostWindowNumber,
notificationObject: notification.object
) else {
return
}
isNotificationsPopoverShown = (notification.userInfo?[NotificationsPopoverVisibilityUserInfoKey.isShown] as? Bool) ?? false
}
.onReceive(viewModel.$hostWindowNumber.removeDuplicates()) { _ in
isNotificationsPopoverShown = AppDelegate.shared?.isNotificationsPopoverShown(in: viewModel.hostWindow) ?? false
}
.onAppear {
modifierKeyMonitor.start()
}
Expand Down Expand Up @@ -551,7 +580,6 @@ struct HiddenTitlebarSidebarControlsView: View {
@ObservedObject var notificationStore: TerminalNotificationStore
@StateObject private var viewModel = TitlebarControlsViewModel()

private let hostWidth: CGFloat = 124
private let hostHeight: CGFloat = 28

var body: some View {
Expand All @@ -568,7 +596,8 @@ struct HiddenTitlebarSidebarControlsView: View {
onNewTab: { _ = AppDelegate.shared?.tabManager?.addTab() },
visibilityMode: .onHover
)
.frame(width: hostWidth, height: hostHeight, alignment: .leading)
.fixedSize(horizontal: true, vertical: false)
.frame(height: hostHeight, alignment: .leading)
}
}

Expand Down Expand Up @@ -780,6 +809,7 @@ final class TitlebarControlsAccessoryViewController: NSTitlebarAccessoryViewCont
private var cachedFittingSize: NSSize?
private var lastObservedViewSize: NSSize = .zero
private var lastAppliedLayoutSnapshot: TitlebarControlsLayoutSnapshot?
private weak var popoverHostWindow: NSWindow?
private let viewModel = TitlebarControlsViewModel()
private var userDefaultsObserver: NSObjectProtocol?
var popoverIsShownForTesting: Bool { notificationsPopover.isShown }
Expand Down Expand Up @@ -923,6 +953,10 @@ final class TitlebarControlsAccessoryViewController: NSTitlebarAccessoryViewCont
preferredContentSize = .zero
containerView.frame = .zero
hostingView.frame = .zero
lastAppliedLayoutSnapshot = nil
lastObservedViewSize = .zero
cachedFittingSize = nil
fittingSizeNeedsRefresh = true
}
}

Expand Down Expand Up @@ -957,9 +991,10 @@ final class TitlebarControlsAccessoryViewController: NSTitlebarAccessoryViewCont
externalAnchor.superview?.layoutSubtreeIfNeeded()
let anchorRect = externalAnchor.convert(externalAnchor.bounds, to: contentView)
if !anchorRect.isEmpty {
popoverHostWindow = window
notificationsPopover.animates = animated
notificationsPopover.show(relativeTo: anchorRect, of: contentView, preferredEdge: .maxY)
postNotificationsPopoverVisibilityDidChange(isShown: true)
postNotificationsPopoverVisibilityDidChange(isShown: true, window: window)
return
}
}
Expand All @@ -968,19 +1003,21 @@ final class TitlebarControlsAccessoryViewController: NSTitlebarAccessoryViewCont
anchorView.superview?.layoutSubtreeIfNeeded()
let anchorRect = anchorView.convert(anchorView.bounds, to: contentView)
if !anchorRect.isEmpty {
popoverHostWindow = window
notificationsPopover.animates = animated
notificationsPopover.show(relativeTo: anchorRect, of: contentView, preferredEdge: .maxY)
postNotificationsPopoverVisibilityDidChange(isShown: true)
postNotificationsPopoverVisibilityDidChange(isShown: true, window: window)
return
}
}

// Fallback: position near top-left of the window content.
let bounds = contentView.bounds
let anchorRect = NSRect(x: 12, y: bounds.maxY - 8, width: 1, height: 1)
popoverHostWindow = window
notificationsPopover.animates = animated
notificationsPopover.show(relativeTo: anchorRect, of: contentView, preferredEdge: .maxY)
postNotificationsPopoverVisibilityDidChange(isShown: true)
postNotificationsPopoverVisibilityDidChange(isShown: true, window: window)
}

func dismissNotificationsPopover() {
Expand All @@ -1002,8 +1039,10 @@ final class TitlebarControlsAccessoryViewController: NSTitlebarAccessoryViewCont

func popoverDidClose(_ notification: Notification) {
// Clear the content view controller to stop SwiftUI observers when popover is hidden
let hostWindow = popoverHostWindow
notificationsPopover.contentViewController = nil
postNotificationsPopoverVisibilityDidChange(isShown: false)
popoverHostWindow = nil
postNotificationsPopoverVisibilityDidChange(isShown: false, window: hostWindow)
}
}

Expand Down Expand Up @@ -1057,6 +1096,7 @@ private struct NotificationsPopoverView: View {
.font(.subheadline)
.foregroundColor(.secondary)
}
.accessibilityIdentifier("notificationsPopover.emptyState")
.frame(minWidth: 420, idealWidth: 520, maxWidth: 640, minHeight: 180)
} else {
ScrollView {
Expand Down Expand Up @@ -1429,6 +1469,15 @@ final class UpdateTitlebarAccessoryController {
controlsControllers.allObjects.contains(where: { $0.popoverIsShownForTesting })
}

func isNotificationsPopoverShown(in window: NSWindow?) -> Bool {
guard let window else {
return isNotificationsPopoverShown()
}
return controlsControllers.allObjects.contains { controller in
controller.popoverIsShownForTesting && controller.view.window === window
}
}
Comment on lines +1472 to +1479

@coderabbitai coderabbitai Bot Mar 18, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Per-window visibility lookup can be wrong for externally anchored popovers.

isNotificationsPopoverShown(in:) checks controller.view.window === window, but popovers may be shown in externalAnchor.window (which can differ from the controller’s window). That causes false negatives when seeding per-window popover state.

Suggested fix 
@@
 final class TitlebarControlsAccessoryViewController: NSTitlebarAccessoryViewController, NSPopoverDelegate {
@@
     var popoverIsShownForTesting: Bool { notificationsPopover.isShown }
+    func isNotificationsPopoverShown(in window: NSWindow?) -> Bool {
+        guard notificationsPopover.isShown else { return false }
+        guard let window else { return true }
+        return popoverHostWindow === window || view.window === window
+    }
@@
     func isNotificationsPopoverShown(in window: NSWindow?) -> Bool {
         guard let window else {
             return isNotificationsPopoverShown()
         }
         return controlsControllers.allObjects.contains { controller in
-            controller.popoverIsShownForTesting && controller.view.window === window
+            controller.isNotificationsPopoverShown(in: window)
         }
     }
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@Sources/Update/UpdateTitlebarAccessory.swift` around lines 1459 - 1466,
isNotificationsPopoverShown(in:) can miss popovers anchored externally; update
the per-window check to also consider the popover's external anchor window. In
the predicate over controlsControllers.allObjects (and using
controller.popoverIsShownForTesting), treat a controller as shown for the given
window if controller.view.window === window OR if the controller's popover has
an externalAnchor whose window === window (e.g. check
controller.popover?.externalAnchor?.window). This ensures externally anchored
popovers are counted in the per-window visibility lookup.


@discardableResult
func dismissNotificationsPopoverIfShown() -> Bool {
let controllers = controlsControllers.allObjects
Expand Down
16 changes: 14 additions & 2 deletions Sources/cmuxApp.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,12 +31,23 @@ enum WorkspacePresentationModeSettings {
}

static func mode(defaults: UserDefaults = .standard) -> Mode {
mode(for: defaults.string(forKey: modeKey))
if let storedMode = defaults.string(forKey: modeKey) {
return mode(for: storedMode)
}
if WorkspaceTitlebarSettings.isVisible(defaults: defaults) == false {
return .minimal
}
return defaultMode
}

static func isMinimal(defaults: UserDefaults = .standard) -> Bool {
mode(defaults: defaults) == .minimal
}

static func initializeStoredModeIfNeeded(defaults: UserDefaults = .standard) {
guard defaults.string(forKey: modeKey) == nil else { return }
defaults.set(mode(defaults: defaults).rawValue, forKey: modeKey)
}
}

enum WorkspaceButtonFadeSettings {
Expand Down Expand Up @@ -189,8 +200,9 @@ struct cmuxApp: App {
let startupAppearance = AppearanceSettings.resolvedMode()
Self.applyAppearance(startupAppearance)
_tabManager = StateObject(wrappedValue: TabManager())
// Migrate legacy and old-format socket mode values to the new enum.
let defaults = UserDefaults.standard
WorkspacePresentationModeSettings.initializeStoredModeIfNeeded(defaults: defaults)
// Migrate legacy and old-format socket mode values to the new enum.
if let stored = defaults.string(forKey: SocketControlSettings.appStorageKey) {
let migrated = SocketControlSettings.migrateMode(stored)
if migrated.rawValue != stored {
Expand Down
35 changes: 34 additions & 1 deletion cmuxTests/AppDelegateShortcutRoutingTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1064,7 +1064,7 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
)
}

func testWorkspaceMinimalModeDefaultsToStandardPresentation() {
func testWorkspaceMinimalModeFallsBackToLegacyHiddenTitlebarPreference() {
let defaults = UserDefaults.standard
let savedMode = defaults.object(forKey: WorkspacePresentationModeSettings.modeKey)
let savedLegacyTitlebar = defaults.object(forKey: WorkspaceTitlebarSettings.showTitlebarKey)
Expand All @@ -1079,6 +1079,39 @@ final class AppDelegateShortcutRoutingTests: XCTestCase {
defaults.set(false, forKey: WorkspaceTitlebarSettings.showTitlebarKey)
defaults.set(WorkspaceButtonFadeSettings.Mode.enabled.rawValue, forKey: WorkspaceButtonFadeSettings.modeKey)

WorkspacePresentationModeSettings.initializeStoredModeIfNeeded(defaults: defaults)

XCTAssertEqual(
defaults.string(forKey: WorkspacePresentationModeSettings.modeKey),
WorkspacePresentationModeSettings.Mode.minimal.rawValue
)
XCTAssertEqual(
WorkspacePresentationModeSettings.mode(defaults: defaults),
.minimal
)
}

func testWorkspaceMinimalModeDefaultsToStandardPresentationWithoutLegacyPreference() {
let defaults = UserDefaults.standard
let savedMode = defaults.object(forKey: WorkspacePresentationModeSettings.modeKey)
let savedLegacyTitlebar = defaults.object(forKey: WorkspaceTitlebarSettings.showTitlebarKey)
let savedLegacyFade = defaults.object(forKey: WorkspaceButtonFadeSettings.modeKey)
defer {
restoreDefaultsValue(savedMode, forKey: WorkspacePresentationModeSettings.modeKey, defaults: defaults)
restoreDefaultsValue(savedLegacyTitlebar, forKey: WorkspaceTitlebarSettings.showTitlebarKey, defaults: defaults)
restoreDefaultsValue(savedLegacyFade, forKey: WorkspaceButtonFadeSettings.modeKey, defaults: defaults)
}

defaults.removeObject(forKey: WorkspacePresentationModeSettings.modeKey)
defaults.removeObject(forKey: WorkspaceTitlebarSettings.showTitlebarKey)
defaults.set(WorkspaceButtonFadeSettings.Mode.enabled.rawValue, forKey: WorkspaceButtonFadeSettings.modeKey)

WorkspacePresentationModeSettings.initializeStoredModeIfNeeded(defaults: defaults)

XCTAssertEqual(
defaults.string(forKey: WorkspacePresentationModeSettings.modeKey),
WorkspacePresentationModeSettings.Mode.standard.rawValue
)
XCTAssertEqual(
WorkspacePresentationModeSettings.mode(defaults: defaults),
.standard
Expand Down
30 changes: 30 additions & 0 deletions cmuxTests/UpdatePillReleaseVisibilityTests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -191,4 +191,34 @@ final class TitlebarControlsHoverPolicyTests: XCTestCase {
XCTAssertTrue(titlebarControlsShouldTrackButtonHover(config: TitlebarControlsStyle.pillGroup.config))
XCTAssertFalse(titlebarControlsShouldTrackButtonHover(config: TitlebarControlsStyle.softButtons.config))
}

func testPopoverVisibilityNotificationsAreScopedToMatchingWindow() {
let window = NSWindow()
let otherWindow = NSWindow()

XCTAssertFalse(
titlebarControlsShouldHandlePopoverVisibilityChange(
hostWindowNumber: nil,
notificationObject: window
)
)
XCTAssertTrue(
titlebarControlsShouldHandlePopoverVisibilityChange(
hostWindowNumber: window.windowNumber,
notificationObject: window
)
)
XCTAssertFalse(
titlebarControlsShouldHandlePopoverVisibilityChange(
hostWindowNumber: window.windowNumber,
notificationObject: otherWindow
)
)
XCTAssertFalse(
titlebarControlsShouldHandlePopoverVisibilityChange(
hostWindowNumber: window.windowNumber,
notificationObject: nil
)
)
}
}
7 changes: 4 additions & 3 deletions cmuxUITests/BonsplitTabDragUITests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,11 +68,12 @@ final class BonsplitTabDragUITests: XCTestCase {
steps: 28,
dragDuration: 0.45
)
let dropIndicatorAppeared = waitForCondition(timeout: 2.0) { dropIndicator.exists }
endMouseDrag(dragSession, atAccessibilityPoint: destination)
XCTAssertTrue(
waitForCondition(timeout: 2.0) { dropIndicator.exists },
dropIndicatorAppeared,
"Expected dragging beta onto alpha to reveal the Bonsplit drop indicator."
)
endMouseDrag(dragSession, atAccessibilityPoint: destination)

XCTAssertTrue(
waitForJSONKey("trackedPaneTabTitles", equals: reorderedOrder, atPath: dataPath, timeout: 5.0) != nil,
Expand Down Expand Up @@ -362,7 +363,7 @@ final class BonsplitTabDragUITests: XCTestCase {
app.typeKey("i", modifierFlags: [.command])
XCTAssertTrue(
app.buttons["notificationsPopover.jumpToLatest"].waitForExistence(timeout: 6.0)
|| app.staticTexts["No notifications yet"].waitForExistence(timeout: 6.0),
|| app.descendants(matching: .any).matching(identifier: "notificationsPopover.emptyState").firstMatch.waitForExistence(timeout: 6.0),
"Expected notifications popover to open."
)

Expand Down
3 changes: 2 additions & 1 deletion cmuxUITests/MultiWindowNotificationsUITests.swift
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -199,7 +199,8 @@ final class MultiWindowNotificationsUITests: XCTestCase {
_ = socketCommand("clear_notifications")

app.typeKey("i", modifierFlags: [.command])
XCTAssertTrue(app.staticTexts["No notifications yet"].waitForExistence(timeout: 6.0), "Expected empty notifications popover state")
let emptyState = app.descendants(matching: .any).matching(identifier: "notificationsPopover.emptyState").firstMatch
XCTAssertTrue(emptyState.waitForExistence(timeout: 6.0), "Expected empty notifications popover state")
let jumpButton = app.buttons["notificationsPopover.jumpToLatest"]
XCTAssertTrue(jumpButton.waitForExistence(timeout: 2.0), "Expected Jump to Latest button in empty notifications popover")
XCTAssertFalse(jumpButton.isEnabled, "Expected Jump to Latest button to be disabled with no notifications")
Expand Down
Loading