From 3c8ce32bf38383255306ca85af7bea44f4b77628 Mon Sep 17 00:00:00 2001 From: iamtoruk Date: Wed, 6 May 2026 10:52:55 -0700 Subject: [PATCH] Fix popover anchor, tab strip flicker, and stale-data refresh Five interleaving menubar regressions traced back to the cache-wipe and showLoading additions in 18c3c8f, surfaced by adversarial multi-agent review against the v0.9.6 baseline. - forceRefresh no longer calls store.invalidateCache(). Wiping the whole cache on every wake or manual refresh emptied todayPayload, flipped showAgentTabs to false, and made cache[key] == nil for all keys, which forced the full-popover loading overlay over already rendered data. The day-rollover guard inside refresh() still wipes the cache when the calendar date changes, so the legitimate part of 18c3c8f is preserved. - Overlay condition is now !store.hasCachedData. Without this, the popover briefly rendered $0.00 placeholders before the overlay slid in on a cold key, and reflashed the overlay on every manual refresh even when fresh data was on screen. - refreshStatusButton skips while popover is anchored. Rewriting the button's attributedTitle changes its intrinsic width, which makes macOS reflow the status item and detaches the anchored popover to the screen's top-left default position. popoverDidClose runs the refresh once so the menubar title catches up immediately on dismiss. - showAgentTabs is sticky via hasAnyProvidersInCache. Prevents the one-frame flicker where the tab strip vanished while the new key's payload had not yet arrived. - observeStore tracks store.currency. Without this, switching currency did not propagate to refreshStatusButton until the next 30s payload tick, leaving the menubar showing the old currency symbol and rate. - Day-rollover race in refresh and refreshQuietly: capture cacheDate at fetch start, drop the write if the calendar date changed during the await. Prevents an in-flight fetch from yesterday polluting today's freshly cleared cache. - Manual refresh button passes showLoading: true again. Safe now that the overlay is gated on cache state instead of isLoading; the refresh button icon swaps to the spinner glyph for visible feedback, while the popover body keeps the existing data and updates when the fetch lands. --- mac/Sources/CodeBurnMenubar/AppStore.swift | 19 +++++++++++++ mac/Sources/CodeBurnMenubar/CodeBurnApp.swift | 27 ++++++++++++++++++- .../Views/MenuBarContent.swift | 19 ++++++++++++- 3 files changed, 63 insertions(+), 2 deletions(-) diff --git a/mac/Sources/CodeBurnMenubar/AppStore.swift b/mac/Sources/CodeBurnMenubar/AppStore.swift index af1947a..00b4283 100644 --- a/mac/Sources/CodeBurnMenubar/AppStore.swift +++ b/mac/Sources/CodeBurnMenubar/AppStore.swift @@ -61,6 +61,14 @@ final class AppStore { cache[currentKey] != nil } + /// True if any cached payload reports at least one provider. Used to keep the + /// AgentTabStrip visible across period/provider switches even when the current + /// key's payload is briefly empty (e.g. immediately after a `switchTo` and + /// before the new fetch lands). + var hasAnyProvidersInCache: Bool { + cache.values.contains { !$0.payload.current.providers.isEmpty } + } + var findingsCount: Int { payload.optimize.findingCount } @@ -117,6 +125,7 @@ final class AppStore { func refresh(includeOptimize: Bool, force: Bool = false, showLoading: Bool = false) async { invalidateStaleDayCache() let key = currentKey + let cacheDateAtStart = cacheDate if !force, cache[key]?.isFresh == true { return } if !force, inFlightKeys.contains(key) { return } inFlightKeys.insert(key) @@ -131,6 +140,11 @@ final class AppStore { do { let fresh = try await DataClient.fetch(period: key.period, provider: key.provider, includeOptimize: includeOptimize) guard !Task.isCancelled else { return } + // Day-rollover race guard: if the calendar date changed during the + // fetch, this payload was computed against yesterday's date and + // would pollute today's freshly-cleared cache. Drop it; the next + // tick will refetch with today's data. + if cacheDate != cacheDateAtStart { return } cache[key] = CachedPayload(payload: fresh, fetchedAt: Date()) lastError = nil } catch { @@ -140,6 +154,7 @@ final class AppStore { do { let fallback = try await DataClient.fetch(period: key.period, provider: key.provider, includeOptimize: false) guard !Task.isCancelled else { return } + if cacheDate != cacheDateAtStart { return } cache[key] = CachedPayload(payload: fallback, fetchedAt: Date()) lastError = nil return @@ -162,8 +177,12 @@ final class AppStore { /// Always uses the .all provider since the menubar badge shows total spend. func refreshQuietly(period: Period) async { invalidateStaleDayCache() + let cacheDateAtStart = cacheDate do { let fresh = try await DataClient.fetch(period: period, provider: .all, includeOptimize: false) + // Same day-rollover guard as refresh(): drop yesterday's payload if + // the calendar rolled over during the fetch. + if cacheDate != cacheDateAtStart { return } cache[PayloadCacheKey(period: period, provider: .all)] = CachedPayload(payload: fresh, fetchedAt: Date()) } catch { NSLog("CodeBurn: quiet refresh failed for \(period.rawValue): \(error)") diff --git a/mac/Sources/CodeBurnMenubar/CodeBurnApp.swift b/mac/Sources/CodeBurnMenubar/CodeBurnApp.swift index bf5bacf..4e9d07b 100644 --- a/mac/Sources/CodeBurnMenubar/CodeBurnApp.swift +++ b/mac/Sources/CodeBurnMenubar/CodeBurnApp.swift @@ -170,7 +170,16 @@ final class AppDelegate: NSObject, NSApplicationDelegate, NSPopoverDelegate { guard now.timeIntervalSince(lastRefreshTime) > 5 else { return } lastRefreshTime = now - store.invalidateCache() + // Note: do NOT call store.invalidateCache() here. The day-rollover guard + // inside refresh() already wipes the cache when the calendar date has + // changed; wiping unconditionally on every wake/manual-refresh empties + // todayPayload, makes showAgentTabs go false, and triggers the + // full-popover loading overlay (because cache[key] == nil after wipe). + // That's the regression chain documented in the multi-agent review. + // + // showLoading: true is fine now that the overlay condition is + // `!hasCachedData`: it lights up the refresh-button spinner glyph + // without covering the popover body. Task { async let main: Void = store.refresh(includeOptimize: false, force: true, showLoading: true) async let today: Void = store.refreshQuietly(period: .today) @@ -219,6 +228,9 @@ final class AppDelegate: NSObject, NSApplicationDelegate, NSPopoverDelegate { withObservationTracking { _ = store.payload _ = store.todayPayload + // Track currency too so the menubar title catches up immediately on + // currency switch instead of waiting for the next 30s payload tick. + _ = store.currency } onChange: { [weak self] in DispatchQueue.main.async { guard let self else { return } @@ -270,6 +282,13 @@ final class AppDelegate: NSObject, NSApplicationDelegate, NSPopoverDelegate { /// flow as one typographic unit with a single, controllable gap. private func refreshStatusButton() { guard let button = statusItem.button else { return } + // Skip while the popover is anchored to this button. Rewriting the + // attributedTitle changes the button's intrinsic width, which makes + // macOS reflow the status item in the menubar and detaches the + // anchored popover (it pops to a stale default position). The + // popoverDidClose delegate calls back through here once the popover + // is dismissed so the menubar cost catches up immediately on close. + if popover != nil && popover.isShown { return } // Clear any previously-set image so the attachment is the only glyph rendered. button.image = nil @@ -398,4 +417,10 @@ final class AppDelegate: NSObject, NSApplicationDelegate, NSPopoverDelegate { func popoverShouldDetach(_ popover: NSPopover) -> Bool { false } + + func popoverDidClose(_ notification: Notification) { + // Catch up on any menubar title updates that were skipped while the + // popover was anchored. + refreshStatusButton() + } } diff --git a/mac/Sources/CodeBurnMenubar/Views/MenuBarContent.swift b/mac/Sources/CodeBurnMenubar/Views/MenuBarContent.swift index 2315353..6e3719f 100644 --- a/mac/Sources/CodeBurnMenubar/Views/MenuBarContent.swift +++ b/mac/Sources/CodeBurnMenubar/Views/MenuBarContent.swift @@ -41,7 +41,15 @@ struct MenuBarContent: View { } } - if store.isLoading || (providerHasCostInAllPayload && !store.hasCachedData) { + // Overlay fires only on cold cache for the current key. This + // avoids the 1-frame `$0.00` flash on first-time period/provider + // switches (the body would otherwise render the empty payload + // for the runloop tick before the overlay slides in). With the + // cache no longer being wiped on every wake/manual-refresh, + // hasCachedData==false now means "we have never fetched this + // key before in this session", which is the right time to + // cover the popover. + if !store.hasCachedData { BurnLoadingOverlay(periodLabel: store.selectedPeriod.rawValue) .transition(.opacity) } @@ -79,6 +87,10 @@ struct MenuBarContent: View { /// on this machine. Hidden only when nothing is detected, which means there's /// nothing to filter by anyway. private var showAgentTabs: Bool { + // Sticky: once any cached payload has reported providers, keep the tab strip + // visible. Without this, the strip disappears for one frame on a period + // switch when the new key's payload is still empty. + if store.hasAnyProvidersInCache { return true } let payload = store.todayPayload ?? store.payload return !payload.current.providers.isEmpty } @@ -407,6 +419,11 @@ struct FooterBar: View { .fixedSize() Button { + // showLoading: true is safe now that the overlay condition uses + // `!hasCachedData` instead of `isLoading`. The button icon swaps + // to the spinner glyph (driven by store.isLoading), giving the + // user visible feedback the click was registered, but the + // popover body keeps the existing data instead of blanking out. Task { await store.refresh(includeOptimize: false, force: true, showLoading: true) } } label: { Image(systemName: store.isLoading ? "arrow.triangle.2.circlepath" : "arrow.clockwise")