diff --git a/mac/Sources/CodeBurnMenubar/CodeBurnApp.swift b/mac/Sources/CodeBurnMenubar/CodeBurnApp.swift index fefdffac..e60cf3c1 100644 --- a/mac/Sources/CodeBurnMenubar/CodeBurnApp.swift +++ b/mac/Sources/CodeBurnMenubar/CodeBurnApp.swift @@ -49,11 +49,14 @@ struct CodeBurnApp: App { } @MainActor -final class AppDelegate: NSObject, NSApplicationDelegate, NSPopoverDelegate { +final class AppDelegate: NSObject, NSApplicationDelegate, NSPopoverDelegate, NSMenuDelegate { private var statusItem: NSStatusItem! private var popover: NSPopover! private var rightClickMonitor: Any? private var lastContextMenuPresentedAt: Date = .distantPast + /// Held only while the right-click menu is open. Cleared in menuDidClose so + /// left-click goes back to the popover action instead of re-showing the menu. + private var contextMenu: NSMenu? fileprivate let store = AppStore() let updateChecker = UpdateChecker() /// True while the displays are asleep. Refresh ticks skip spawning @@ -909,15 +912,25 @@ final class AppDelegate: NSObject, NSApplicationDelegate, NSPopoverDelegate { button.sendAction(on: [.leftMouseUp, .rightMouseUp]) // macOS 27 no longer routes any right-mouse event to the status-item - // button's target/action. A global monitor still observes right-mouse-down; + // button's target/action. A global monitor still observes right-mouse-up; // we hit-test it against our own status-item window and present the menu - // ourselves. Harmless and stable on 15/26 too (the debounce in - // showContextMenu prevents a double-present if the legacy path also fires). - rightClickMonitor = NSEvent.addGlobalMonitorForEvents(matching: [.rightMouseDown]) { [weak self] _ in + // ourselves. + // + // Must be mouse-*up*, not mouse-down: presenting on down starts menu + // tracking while the button is still held, so the matching rightMouseUp + // is treated as an outside click and the menu flashes then dismisses. + // Presenting on up (after the click completes) keeps it open. Harmless + // on 15/26 too (the debounce in showContextMenu prevents a double-present + // if the legacy path also fires). + rightClickMonitor = NSEvent.addGlobalMonitorForEvents(matching: StatusItemContextMenuPolicy.presentEventMask) { [weak self] _ in guard let self, let button = self.statusItem.button, let window = button.window, window.frame.contains(NSEvent.mouseLocation) else { return } + // Defer one turn so menu presentation is not nested inside the + // monitor callback. Safe on mouse-*up* (the click is already + // complete); on mouse-down a deferred present was killed by the + // matching up event and the menu only flashed. DispatchQueue.main.async { self.showContextMenu(from: button) } } @@ -1126,9 +1139,16 @@ final class AppDelegate: NSObject, NSApplicationDelegate, NSPopoverDelegate { private func showContextMenu(from button: NSStatusBarButton) { // Debounce: on macOS <= 26 both the legacy action path and the global // monitor can fire for a single right-click. Present at most once per click. - let now = Date() - guard now.timeIntervalSince(lastContextMenuPresentedAt) > 0.3 else { return } - lastContextMenuPresentedAt = now + // Policy lives in StatusItemContextMenuPolicy so the gate is unit-tested (#802). + guard StatusItemContextMenuPolicy.acceptPresent( + now: Date(), + lastPresentedAt: &lastContextMenuPresentedAt + ) else { return } + + // Don't let an open popover steal the click / sit under the menu. + if popover?.isShown == true { + popover.performClose(nil) + } let menu = NSMenu() @@ -1160,12 +1180,39 @@ final class AppDelegate: NSObject, NSApplicationDelegate, NSPopoverDelegate { quitItem.target = self menu.addItem(quitItem) - // Present directly. The previous `statusItem.menu = menu; button.performClick` - // trick relies on the click -> action path that macOS 27 changed; popUp is - // version-stable. Open a few px below the status item so the menu clears the - // menu bar: anchoring flush clips the top edge and makes macOS engage menu - // scrolling (a scroll chevron appears and the first row slides up on hover). - menu.popUp(positioning: nil, at: NSPoint(x: 0, y: button.bounds.height + 6), in: button) + // Present via the status item's own menu slot. AppKit positions and tracks + // that menu correctly under the status item (no scroll chevron / first-row + // jump). Manual NSMenu.popUp(at:in:) is what caused the jump: the menu was + // tracked against a point while the cursor still sat on the status item + // above it, so the first mouse move engaged scroll mode. + // + // #472 dropped this pattern because assigning `statusItem.menu` from the + // right-mouse *action* never ran on macOS 27 (right-clicks no longer reach + // the button action). We still open from our global rightMouseUp monitor + // (or the legacy action on ≤26); once we set `statusItem.menu` ourselves, + // performClick is just "open the attached menu" and works on 27 too. + // + // menuDidClose clears `statusItem.menu` so the next left-click hits our + // action (popover) instead of re-opening this menu. + menu.delegate = self + contextMenu = menu + statusItem.menu = menu + button.performClick(nil) + } + + // MARK: - NSMenuDelegate + + // AppKit invokes menu callbacks on the main thread. Clear the status-item + // menu slot so the next left-click hits our action (popover) again. + nonisolated func menuDidClose(_ menu: NSMenu) { + // Hop explicitly — don't assumeIsolated across the NSMenuDelegate boundary + // under Swift 6 strict concurrency (NSMenu isn't Sendable). + DispatchQueue.main.async { [weak self] in + guard let self else { return } + // Always clear: we only ever attach our own context menu to the item. + self.statusItem.menu = nil + self.contextMenu = nil + } } /// One-line "today" summary for the context menu's usage row. diff --git a/mac/Sources/CodeBurnMenubar/StatusItemContextMenuPolicy.swift b/mac/Sources/CodeBurnMenubar/StatusItemContextMenuPolicy.swift new file mode 100644 index 00000000..845a9f31 --- /dev/null +++ b/mac/Sources/CodeBurnMenubar/StatusItemContextMenuPolicy.swift @@ -0,0 +1,47 @@ +import AppKit +import Foundation + +/// Pure policy for the status-item right-click menu (#802). +/// +/// Keeps the event-mask, debounce, and presentation choices out of AppDelegate +/// so they can be unit-tested without spinning up an NSStatusItem. The bugs this +/// encodes: +/// +/// 1. **Flash** — presenting on `rightMouseDown` lets the matching `rightMouseUp` +/// dismiss the menu immediately. Monitor (and legacy action) use mouse-*up*. +/// 2. **Jump/scroll** — manual `NSMenu.popUp(at:in:)` tracks poorly while the +/// cursor still sits on the status item above the menu. Attach via +/// `statusItem.menu` + `performClick` instead. +/// 3. **Double-present** — on macOS ≤26 both the button action and the global +/// monitor can fire for one click; debounce collapses them. +enum StatusItemContextMenuPolicy { + /// Global-monitor / action event. Must be mouse-up (see flash note above). + static let presentEventMask: NSEvent.EventTypeMask = .rightMouseUp + + /// Minimum gap between presents. Covers the dual-path race on macOS ≤26. + static let presentDebounceSeconds: TimeInterval = 0.3 + + /// How the menu is shown once a present is accepted. + enum Presentation: Equatable { + /// Assign `statusItem.menu` then `button.performClick`. AppKit positions + /// and tracks the menu under the status item. Clear menu in menuDidClose. + case statusItemMenu + /// Manual `NSMenu.popUp(at:in:)`. Causes scroll-chevron jump on mouse move + /// when the cursor starts above the menu. Kept only as a named anti-pattern + /// so tests can lock that we do *not* use it. + case manualPopUp + } + + static let presentation: Presentation = .statusItemMenu + + /// Returns true and advances `lastPresentedAt` when a new present is allowed. + static func acceptPresent( + now: Date, + lastPresentedAt: inout Date, + debounce: TimeInterval = presentDebounceSeconds + ) -> Bool { + guard now.timeIntervalSince(lastPresentedAt) > debounce else { return false } + lastPresentedAt = now + return true + } +} diff --git a/mac/Tests/CodeBurnMenubarTests/StatusItemContextMenuPolicyTests.swift b/mac/Tests/CodeBurnMenubarTests/StatusItemContextMenuPolicyTests.swift new file mode 100644 index 00000000..e601fa88 --- /dev/null +++ b/mac/Tests/CodeBurnMenubarTests/StatusItemContextMenuPolicyTests.swift @@ -0,0 +1,87 @@ +import AppKit +import XCTest +@testable import CodeBurnMenubar + +/// Locks the right-click menu policy that fixed flash + scroll-jump (#802). +/// Does not drive a real NSStatusItem — that needs manual/AppKit integration. +final class StatusItemContextMenuPolicyTests: XCTestCase { + func testPresentEventMaskIsRightMouseUpNotDown() { + // Presenting on mouse-down lets the matching up dismiss the menu (flash). + XCTAssertEqual( + StatusItemContextMenuPolicy.presentEventMask, + NSEvent.EventTypeMask.rightMouseUp + ) + XCTAssertNotEqual( + StatusItemContextMenuPolicy.presentEventMask, + NSEvent.EventTypeMask.rightMouseDown + ) + // Mask must include up and must not include down (single-bit masks here). + XCTAssertTrue(StatusItemContextMenuPolicy.presentEventMask.contains(.rightMouseUp)) + XCTAssertFalse(StatusItemContextMenuPolicy.presentEventMask.contains(.rightMouseDown)) + } + + func testPresentationUsesStatusItemMenuNotManualPopUp() { + // Manual popUp tracks against a point while the cursor sits on the status + // item above the menu → scroll chevron / Today-row jump on mouse move. + XCTAssertEqual( + StatusItemContextMenuPolicy.presentation, + .statusItemMenu + ) + XCTAssertNotEqual( + StatusItemContextMenuPolicy.presentation, + .manualPopUp + ) + } + + func testDebounceAcceptsFirstPresent() { + var last = Date.distantPast + let now = Date(timeIntervalSince1970: 1_000) + XCTAssertTrue( + StatusItemContextMenuPolicy.acceptPresent(now: now, lastPresentedAt: &last) + ) + XCTAssertEqual(last, now) + } + + func testDebounceRejectsWithinWindow() { + let t0 = Date(timeIntervalSince1970: 1_000) + var last = t0 + // Just inside the 0.3s window + let t1 = t0.addingTimeInterval(0.299) + XCTAssertFalse( + StatusItemContextMenuPolicy.acceptPresent(now: t1, lastPresentedAt: &last) + ) + XCTAssertEqual(last, t0, "reject must not advance lastPresentedAt") + } + + func testDebounceAcceptsAfterWindow() { + let t0 = Date(timeIntervalSince1970: 1_000) + var last = t0 + let t1 = t0.addingTimeInterval(StatusItemContextMenuPolicy.presentDebounceSeconds + 0.001) + XCTAssertTrue( + StatusItemContextMenuPolicy.acceptPresent(now: t1, lastPresentedAt: &last) + ) + XCTAssertEqual(last, t1) + } + + func testDebounceBoundaryIsStrictlyGreaterThan() { + // Gate uses `>` not `>=`: exactly debounce seconds later is still rejected. + let t0 = Date(timeIntervalSince1970: 1_000) + var last = t0 + let exact = t0.addingTimeInterval(StatusItemContextMenuPolicy.presentDebounceSeconds) + XCTAssertFalse( + StatusItemContextMenuPolicy.acceptPresent(now: exact, lastPresentedAt: &last) + ) + XCTAssertEqual(last, t0) + } + + func testCustomDebounceOverride() { + var last = Date(timeIntervalSince1970: 0) + let now = Date(timeIntervalSince1970: 0.5) + XCTAssertFalse( + StatusItemContextMenuPolicy.acceptPresent(now: now, lastPresentedAt: &last, debounce: 1.0) + ) + XCTAssertTrue( + StatusItemContextMenuPolicy.acceptPresent(now: now, lastPresentedAt: &last, debounce: 0.4) + ) + } +}