Switch menu bar to NSPopover and redesign settings - #13
Merged
Conversation
Replace SwiftUI MenuBarExtra(.window) with a manual NSStatusItem + NSPopover. The popover renders the system upward-pointing arrow centered under the status item via show(relativeTo:of:preferredEdge:.minY). The status button hosts MenuBarLabel via NSHostingView, and re-measures intrinsicContentSize on viewModel updates and UserDefaults changes so the bar resizes when content width changes (single → dual percent, $/% toggle, etc.). NSApp.activate is called before show so focusable controls (e.g. OAuth code field) receive keyboard input, and the first responder is cleared after open to suppress AppKit's auto-focus on the version-update Link.
Settings is now General / Display / Notifications / Account: - Display owns menu bar layout (5-hour vs 5-hour + 7-day), color mode, warning + critical sliders (with footer caption clarifying they drive both colors and notifications), and a conditional Enterprise display picker ($/%) shown only when viewModel.isEnterprise. - Notifications has Threshold alerts and Burn rate alert toggles, each with explanatory footer text. Both default to true. - General is reduced to Poll interval and Launch at login. - Account keeps identity, sign-out, quit (still useful given no dock). MenuBarLabel reads the new menuBarLayout preference and collapses to a single percent when set to fiveHourOnly. Default for new installs is fiveHourOnly to match the simpler glance use case.
…a settings Two changes to NotificationService: 1. checkAndNotify previously read the persisted notified array once before the loop and each iteration overwrote it with a fresh copy plus its own threshold. When 50% and 80% were crossed in the same call only [80] survived, and on the next call 50 would re-fire. Track the array in-place via var so each append is preserved. 2. Honour the new Notifications settings: checkAndNotify short-circuits when notificationThresholdAlerts is false, and shouldNotifyBurnRate returns false when notificationBurnRateAlerts is false. Both default to enabled so existing installs see no behaviour change. UserDefaults is now injected (init(defaults: UserDefaults = .standard)) so tests can pass an isolated suite-scoped instance without leaking gate keys into other parallel tests. Production call sites are unchanged. The injected property is nonisolated(unsafe) since UserDefaults is documented thread-safe but isn't formally Sendable in Swift 6.
Four new tests in NotificationGatingTests using per-test UserDefaults(suiteName:) instances so the gate keys never leak into other parallel notification suites: - Threshold alerts disabled blocks side effects (notifiedKey stays nil) - Threshold alerts default (key unset) records both 50 and 80 crossings in a single 95% call (regression guard for the overwrite bug) - Burn rate alerts disabled returns false even when criteria met - Burn rate alerts default (key unset) returns true when criteria met
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
MenuBarExtra(.window)withNSStatusItem+NSPopoverso the dropdown gets the native upward-pointing arrow centered under the icon. Status item width tracks the SwiftUI label's intrinsic content size.Displaysetting picks between5-hour(single percent) and5-hour + 7-day(current dual). Defaults to5-hourfor new installs.checkAndNotifyno longer overwrites earlier crossings when multiple thresholds fire in a single call. The persistednotifiedarray is now updated in-place, so a 95% reading correctly records both 50% and 80% instead of just 80%.checkAndNotifyandshouldNotifyBurnRate. Defaults preserve current behaviour.UserDefaults(suiteName:)so they don't leak into other parallel notification suites. Includes a regression guard for the multi-threshold record bug.Notable refactor
NotificationService.init(defaults: UserDefaults = .standard)— production unchanged, tests inject a hermetic suite. Field isnonisolated(unsafe)sinceUserDefaultsis documented thread-safe but isn't formallySendablein Swift 6.Test plan
swift buildcleanswift test --filter "BurnRate|Notification"— 21/21 pass across 4 suites