Cache menu card heights and skip closed merged-menu rebuild#1314
Merged
steipete merged 6 commits intoJun 5, 2026
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR updates menu-card height caching to support a “fingerprinted” cache key so stable card content can reuse measured heights even across invalidateMenus() calls, reducing unnecessary re-measurement.
Changes:
- Replace content-version-based cache invalidation with a fingerprint-driven cache key strategy.
- Add new tests covering fingerprinted vs unfingerprinted cache behavior across menu invalidation.
- Thread
heightCacheFingerprintthrough menu-card item creation and apply fingerprints to several menu sections.
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/CodexBarTests/StatusMenuHeightCacheTests.swift | Updates existing cache test expectation and adds new fingerprinted/unfingerprinted cache tests plus a helper controller factory. |
| Sources/CodexBar/StatusItemController+ZaiHourlyChartMenu.swift | Adds a stable fingerprint for the hourly-usage submenu height caching. |
| Sources/CodexBar/StatusItemController+UsageHistoryMenu.swift | Adds a stable fingerprint for the usage-history submenu height caching. |
| Sources/CodexBar/StatusItemController+MenuTracking.swift | Stops clearing the height cache on invalidateMenus(), relying on versioned/fingerprinted keys instead. |
| Sources/CodexBar/StatusItemController+MenuCardItems.swift | Adds heightCacheFingerprint plumbing into makeMenuCardItem and the cache lookup. |
| Sources/CodexBar/StatusItemController+MenuCardHeightCache.swift | Changes cache key from version to fingerprint and adds an optional fingerprint parameter. |
| Sources/CodexBar/StatusItemController+Menu.swift | Uses model-derived fingerprints for multiple menu-card sections to stabilize caching across invalidations. |
| Sources/CodexBar/StatusItemController+CodexStackedMenu.swift | Adds model-derived fingerprints for stacked cards. |
| Sources/CodexBar/MenuCardHeightFingerprint.swift | Introduces a fingerprint generator based on UsageMenuCardView.Model content. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+6
to
+8
| "name=\(self.providerName)", | ||
| "email=\(self.email)", | ||
| "subtitle=\(self.subtitleText)", |
Comment on lines
+2
to
+22
| func heightFingerprint(section: String, additional: [String] = []) -> String { | ||
| MenuCardHeightFingerprint.join([ | ||
| "section=\(section)", | ||
| "provider=\(self.provider.rawValue)", | ||
| "name=\(self.providerName)", | ||
| "email=\(self.email)", | ||
| "subtitle=\(self.subtitleText)", | ||
| "subtitleStyle=\(self.subtitleStyle.heightFingerprint)", | ||
| "plan=\(self.planText ?? "")", | ||
| "placeholder=\(self.placeholder ?? "")", | ||
| "credits=\(self.creditsText ?? "")", | ||
| "creditsHint=\(self.creditsHintText ?? "")", | ||
| "creditsCopy=\(self.creditsHintCopyText ?? "")", | ||
| "metrics=\(MenuCardHeightFingerprint.join(self.metrics.map(\.heightFingerprint)))", | ||
| "notes=\(MenuCardHeightFingerprint.join(self.usageNotes))", | ||
| "dashboard=\(self.inlineUsageDashboard?.heightFingerprint ?? "")", | ||
| "providerCost=\(self.providerCost?.heightFingerprint ?? "")", | ||
| "tokenUsage=\(self.tokenUsage?.heightFingerprint ?? "")", | ||
| "openaiAPI=\(self.openAIAPIUsage == nil ? "0" : "1")", | ||
| ] + additional) | ||
| } |
Comment on lines
+553
to
+555
| heightCacheFingerprint: row.model.heightFingerprint( | ||
| section: "overview", | ||
| additional: ["storage=\(storageText ?? "")"]), |
Comment on lines
+199
to
+212
| private func makeHeightCacheController() -> StatusItemController { | ||
| let settings = self.makeSettings() | ||
| settings.statusChecksEnabled = false | ||
| settings.refreshFrequency = .manual | ||
| settings.mergeIcons = false | ||
| let store = self.makeCodexStore(settings: settings, dashboardAuthorized: false) | ||
| return StatusItemController( | ||
| store: store, | ||
| settings: settings, | ||
| account: UsageFetcher().loadAccountInfo(), | ||
| updater: DisabledUpdaterController(), | ||
| preferencesSelection: PreferencesSelection(), | ||
| statusBar: self.makeStatusBarForTesting()) | ||
| } |
Comment on lines
34
to
37
| self.menuContentVersion &+= 1 | ||
| self.menuCardHeightCache.removeAll(keepingCapacity: true) | ||
| if !allowStaleContentDuringDataRefresh { | ||
| self.latestRequiredMenuRebuildVersion = self.menuContentVersion | ||
| } |
|
Codex review: needs real behavior proof before merge. Reviewed June 5, 2026, 3:33 PM ET / 19:33 UTC. Summary Reproducibility: yes. for the targeted redundant rebuild path from source: current main invalidates closed menus and prewarms attached menus, while the PR defers the merged menu until the next open. I did not reproduce the macOS 26.x lag live in this read-only review. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Proof guidance:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land this after redacted current-head packaged macOS 26.x Merge Icons proof covers both repeated measured-height cache hits and skipped closed merged-menu rebuilds, with maintainer acceptance or adjustment of the account-cache freshness window. Do we have a high-confidence way to reproduce the issue? Yes for the targeted redundant rebuild path from source: current main invalidates closed menus and prewarms attached menus, while the PR defers the merged menu until the next open. I did not reproduce the macOS 26.x lag live in this read-only review. Is this the best way to solve the issue? Unclear until final runtime proof is added. The implementation is a plausible narrow performance slice with focused tests, but the auth-cache freshness tradeoff and current-head packaged behavior need maintainer review before merge. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against de55f4850b8d. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
Shiny media proof means a screenshot, video, or linked artifact directly shows the changed behavior. Runtime, network, CSP, and security claims still need visible diagnostics. How this review workflow works
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
Contributor
Author
|
@clawsweeper re-review Updated the PR body with packaged macOS 26.5 / Merge Icons proof from the local PR-head bundle. Patch-quality blockers already addressed in
Packaged runtime proof now added to the PR body:
Local validation after the update:
Remaining scope: this is still a targeted cache-policy slice. It reduces the repeated unchanged measured-height path on one macOS 26.5 packaged Merge Icons run, but does not remove the first |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
P1
hhh2210
changed the title
clawsweeper
Bot
added
rating: 🦐 gold shrimp
Menu cards size from semantic text styles (.body/.footnote/.headline/…) that scale with the macOS system text-size / Dynamic Type setting. That scale was in neither the content fingerprint nor the cache key, and no observer clears the cache when it changes, so a runtime text-size change could return a height measured at the old scale (clipped or over-tall cards) until the next data refresh or the 256-entry flush. Add the resolved .body point size to MenuCardHeightCacheKey. Widening the key is the safe direction: identical scale keeps hitting as before, a changed scale forces a fresh measurement. It can never return a stale height, only (at worst) re-measure once after a scale change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
In Merge Icons mode prepareAttachedClosedMenusIfNeeded eagerly rebuilt the closed merged menu on every store tick, running a full main-thread populateMenu (incl. SwiftUI hosting-view layout) that menuWillOpen redoes on display anyway. Defer the merged menu until next open instead, taking the residual close-path freeze to zero. Refs steipete#1274.
steipete
force-pushed
the
codex/menu-card-height-fingerprint
branch
from
ef527e8 to
400f98a
Compare
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
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.
Refs #1274.
Summary
UsageMenuCardView.Modelheight fingerprint and pass it through the merged, stacked, history, and chart menu-card call sitesWhy
The latest #1274 samples after #1297 and #1314 split the remaining Merge Icons lag into two pieces:
populateMenu -> addMenuCardSections -> MenuCardItemHostingView.measuredHeight, where each cache miss allocates a freshNSHostingViewjust to measure card height.heightFingerprintstring formatting/hash work plusmenuCardModel -> accountInfo -> loadAuthBackedCodexAccount -> CodexOAuthCredentialsStore.parseon repeated rebuilds.#1286 moved close rebuild work off the synchronous dismiss path, and #1297 targets icon-observation churn. This PR remains the next separate measured-height slice, but now also addresses the obvious cost regressions called out in the #1314 retest. It still does not claim to close #1274; affected macOS 26.x / Merge Icons machines should retest the packaged build.
Claw/Copilot Follow-up
chars,utf8,lines, hash), so cache keys no longer retain raw PII or user-visible stringsstorageText, soniland""no longer collapse into the same height identityversion:fallback keys oninvalidateMenus()while retaining content-fingerprinted keys, avoiding old version-scoped entries accumulating until the global 256-entry flushAccountInfo(email: nil, plan: nil)fixtures instead ofUsageFetcher().loadAccountInfo()Latest #1274 Retest Follow-up
@giuseppebisemi's #1314 retest showed the measured-height leaf dropped, but part of the work moved into the new fingerprint and the broader model construction still reparsed Codex OAuth credentials. Current head
254412f0responds to that specific critique:MenuCardHeightFingerprint.stringShapeno longer uses CryptoKit digest formatting,String(format:), orwithVaList; it uses a per-process saltedHashervalue plus cheap UTF-8 newline counting.Metric.heightFingerprintno longer callspercentLabel, so the fingerprint path no longer reachesPercentStyle.labelSuffix -> L(...) -> ProcessInfo.environmentper metric.Localization.isRunningTestsProcess()now caches the startup result instead of readingProcessInfo.processInfo.environmenton repeated localization calls.UsageStore.accountInfo(for:)now has a shortconfigRevision-scoped cache, so close/open rebuild bursts do not re-read and re-parseauth.json/ JWT payloads each time.menuCardModelnow requests fallback account info only for providers whose metadata uses account fallback; non-Codex provider cards no longer pay for Codex auth parsing they will not display.This is intentionally not full menu-model memoization. The first
populateMenu, SwiftUI hosting/view construction, and actual model reuse remain separate future work for #1274.Packaged macOS 26.5 Proof From The Measured-Height Slice
Local packaged-app run at earlier PR head
74528926, redacted for account/path data:Repeated menu-open proof:
Observed result across the 12 repeated open/close rounds:
Interpretation: on this macOS 26.5 packaged Merge Icons run, repeated unchanged merged-menu opens did not show the old
MenuCardItemHostingView.measuredHeight/addMenuCardSectionshot path in the sampled main thread. There are still ordinary SwiftUINSHostingViewattach/layout frames, so this proof supports the targeted measured-height cache slice rather than claiming all menu work is gone.Current Head Validation
Current head:
254412f0.swift test --filter UsageStoreCoverageTests(21 tests)swift test --filter MenuCardHeightFingerprintTests(3 tests)swift test --filter StatusMenuHeightCacheTests(6 tests)make check(SwiftFormat lint + SwiftLint: 0 violations)swift test(3246 tests / 385 suites)Earlier CI had passed for PR head
74528926(GitGuardian,build-linux-cliarm64/x64, andlint-build-test). Current head CI is expected to rerun, but I am not treating slow CI or Claw re-review as part of this local proof.Remaining Scope
I would still keep #1274 open after this PR. #1314 now proves the repeated unchanged measured-height cache path on one local macOS 26.5 Merge Icons packaged run, and current head removes the most obvious fingerprint/account parse regressions from that slice. It does not remove the first
populateMenu, does not reuse actual menu item hosting views, and does not fully memoizeUsageMenuCardView.Modelconstruction.Skip closed merged-menu rebuild (folded in, head
b6d1129e, #1274)@giuseppebisemi's post-254412f0retest confirmed this slice's targeted costs collapsed (heightFingerprint76→3,CodexOAuthCredentialsStore.parse~50→5,menuCardModel105→33,addMenuCardSections83→6, and theProcessInfo.environment/percentLabelreads gone) and isolated the remaining freeze to a single structural path:Root cause:
menuDidCloseonly defers the merged menu when it is already stale at close time, butobserveStoreChangesinvalidates withallowStaleContentDuringDataRefresh: trueand still falls through toprepareAttachedClosedMenusIfNeeded()— so a merged menu that closed fresh is rebuilt while closed on the next background tick.Rather than ship this as a third open PR, it is folded in here (same Merge Icons menu-rebuild domain). The fix: in
prepareAttachedClosedMenusIfNeeded, when the menu being prepared is the merged menu, defer it unconditionally (insert intoclosedMenusDeferredUntilNextOpenand skiprebuildClosedMenuIfNeeded). Guarding insideprepareAttachedClosedMenusIfNeededitself — rather than only the stale-refresh path — also coversallowStale=falseinvalidations (config / provider-order / manual), so the eager closed rebuild is gone on every path. That takes the 388-sample subtree to 0.Correctness:
menuWillOpenrunsrefreshMenuForOpenIfNeededsynchronously before the menu is registered open and before AppKit displays it, so the deferred merged menu is fully repopulated before it is shown — no stale flash or width jump — and the deferred-set entry is cleared on open, so it does not leak. The only remaining merged-menu populate is the one on open, the boundary@giuseppebisemiand I drew.Validation at head
b6d1129e:swift test --filter StatusMenuOpenRefreshTests(29 tests) — includes the rewritten "closed merged menu defers rebuild until next open instead of pre-warming" assertion and the two pre-warm-deferral tests re-pointed to the non-merged fallback menumake check(SwiftFormat lint + SwiftLint: 0 violations)swift test(3246 tests / 385 suites)This still does not remove the first
populateMenuor reuse hosting views; it eliminates the redundant closed rebuild only.