Fix open menu refresh glitch - #923
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 967e66ce50
ℹ️ 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 let self else { return } | ||
| self.observeStoreChanges() | ||
| self.invalidateMenus(refreshOpenMenus: !self.store.isRefreshing) | ||
| self.invalidateMenus() |
There was a problem hiding this comment.
Preserve open-menu updates for explicit store actions
When the user triggers an explicit action that only mutates UsageStore—for example Augment's menu action Refresh Session (appendActionMenuEntries wires it to refreshAugmentSession, which just awaits store.forceRefreshAugmentSession() and store.refresh())—the open menu now only has its version invalidated. Because this observer no longer passes refreshOpenMenus: !self.store.isRefreshing, the isRefreshing transition back to false will not rebuild the still-open menu, so the action's updated usage/error state remains invisible until the user closes and reopens the menu; the new refreshStore(..., refreshOpenMenusWhenComplete:) path only covers the generic Refresh action, not these store-observed explicit actions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in 290120c2.
I kept the store observer conservative so automatic/background updates still only invalidate open menus, which preserves the AppKit menu-tracking fix. For explicit user-triggered store actions, I added refreshOpenMenusAfterExplicitStoreAction() and wired refreshAugmentSession() to call it after the refresh completes.
Added coverage in StatusMenuOpenRefreshTests to verify explicit store actions refresh a visible open menu while store observation alone still does not rebuild during tracking.
Validated with:
swift test --filter StatusMenuOpenRefreshTestsswift test --filter StatusMenuTestsmake check
967e66c to
290120c
Compare
…links, cost estimate clarification, menu refresh glitch fix)
290120c to
2eaa246
Compare
|
Thanks @AmrMohamad! I verified this is covered on current main now: commit 36db0c6 added the deferred open-menu refresh path, keeps store-observation invalidation from rebuilding visible menus during tracking, and added StatusMenuOpenRefreshTests. I also re-ran: swift test --filter 'StatusMenuTests|StatusMenuOpenRefreshTests'on current main at 6423275 and it passed: 38 tests. Closing because the branch is now dirty against main and the fix is already landed. Appreciate the patch; the changelog entry is in Unreleased as #923. |
Summary
Why this fixes the glitch
CodexBar runs as a macOS menu bar app built around
NSStatusItem/NSMenu, with several menu rows hosted through SwiftUI/AppKit views. The recorded glitch showed the middle hosted rows blanking into a large gap while native AppKit rows remained visible. That matches an AppKit menu-tracking hazard: mutating or rebuilding anNSMenuwhile it is currently open can leave hosted view rows in a transient broken visual state.The existing open-menu refresh path refreshed provider data and then repopulated the currently visible menu after store observation. This patch lets the refresh update the backing store and mark menu content stale, but defers the visible rebuild until the menu is explicitly refreshed or opened again. That keeps the menu stable during AppKit tracking while preserving fresh data for the next render.
Validation
swift test --filter StatusMenuTestsswift test --filter StatusMenuOpenRefreshTestsmake checkScope
Only the staged menu-refresh fix is included: three status item controller files plus focused menu regression tests.