perf: cut menu readiness signature cost on store changes - #1351
Conversation
The menu readiness signature serializes every enabled provider's token snapshot and 30-day daily breakdown on each store mutation. Two fixes: - Skip computing it entirely when no menu is open, since the result (refreshOpenMenus) is only consulted while a menu is open. This is the common case during background refresh ticks. - Use the raw Double bit pattern instead of String(format: "%.8f", …), which is a hot per-value cost. The signature is only compared for equality, so the bit pattern is both exact and far cheaper. Reduces main-thread work that regressed popup-menu responsiveness in the 0.32 series (refs steipete#1321). Co-authored-by: Cursor <cursoragent@cursor.com>
|
Codex review: needs maintainer review before merge. Reviewed June 8, 2026, 6:11 AM ET / 10:11 UTC. Summary Reproducibility: yes. from source inspection and contributor proof, but I did not rerun it locally. Current main calls the readiness signature on each store observation, and the PR body shows a freshly built multi-provider app sampled with the menu closed and open. Review metrics: 3 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Next step before merge
Security Review detailsBest possible solution: Land this focused performance patch after normal maintainer review, and keep broader reporter-specific performance follow-up under #1321 and the complementary #1352 path. Do we have a high-confidence way to reproduce the issue? Yes, from source inspection and contributor proof, but I did not rerun it locally. Current main calls the readiness signature on each store observation, and the PR body shows a freshly built multi-provider app sampled with the menu closed and open. Is this the best way to solve the issue? Yes. The patch removes wasted closed-menu signature work while preserving open-menu refresh semantics through guarded baseline resync and targeted regression tests. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against db184430bc4b. Label changesLabel changes:
Label 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
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f351a7d6e6
ℹ️ 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".
| let refreshOpenMenus = self.openMenus.isEmpty | ||
| ? false | ||
| : self.didMenuAdjunctReadinessChange() |
There was a problem hiding this comment.
When no menu is open this now skips didMenuAdjunctReadinessChange(), so lastMenuAdjunctReadinessSignature can remain at an older value even though invalidateMenus and a later menuWillOpen have rebuilt the closed/open menu with the newer data. If the first observed store change while that menu is open brings the signature back to the old value, this branch passes refreshOpenMenus: false; invalidateMenus then returns early for open menus, leaving the visible menu showing the intermediate closed-time snapshot. Updating the stored baseline when closed, or resetting it after rebuilding on open, avoids missing this revert case.
Useful? React with 👍 / 👎.
clawsweeper
Bot
added
rating: 🧂 unranked krab
Skipping the readiness signature while all menus are closed (the idle-cost optimization) let the baseline drift from live store data. A menu reopened from new data, followed by an open-menu change reverting to the previous baseline value, was treated as unchanged and skipped the rebuild, leaving stale content visible. Re-anchor the baseline when a root menu opens (rebuilt from current data). Only the root open re-anchors; nested submenu opens must not, to avoid masking a pending parent refresh. Adds a focused regression test. Co-authored-by: Cursor <cursoragent@cursor.com>
|
Pushed a follow-up commit addressing the review:
PR body updated with the above. @clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. |
clawsweeper
Bot
added
rating: 🦪 silver shellfish
|
Added a direct runtime measurement to the PR body (mirroring what #1352 used): the real |
|
🦞👀 Command router queued. I will update this comment with the next step. |
clawsweeper
Bot
added
the
merge-risk: 🚨 availability
|
Added real-app proof to the PR body: a freshly built ad-hoc bundle of this branch with 7 providers enabled and the menu closed, sampled for 65 s across a background refresh tick ( |
|
🦞👀 Command router queued. I will update this comment with the next step. |
clawsweeper
Bot
added
proof: sufficient
…e#1351) Only re-anchor the readiness baseline on root menu open when the menu was actually rebuilt or is already fresh for the current menuContentVersion. When refreshMenuForOpenIfNeeded preserves stale content during an in-flight provider refresh, resyncing to live store data would mask the refresh-completion update. Add a focused regression test (negative-validated). Co-authored-by: Cursor <cursoragent@cursor.com>
|
Addressed the @clawsweeper P1 stale-preservation blocker:
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
clawsweeper
Bot
added
rating: 🦞 diamond lobster
clawsweeper
Bot
removed
the
merge-risk: 🚨 availability
|
Checked off the last Test plan item — menu-open proof (7 providers, same ad-hoc bundle):
PR body updated; all Test plan checkboxes are now complete. Clawsweeper already has this at diamond lobster / ready for maintainer look — the remaining step is maintainer merge. |
clawsweeper
Bot
added
rating: 🦀 challenger crab
2 participants
Summary
Targets the popup-menu lag reported in #1321. Multiple users confirmed the lag appeared in the 0.32 series and that downgrading to 0.31.0 restores responsiveness, so this focuses on a 0.32-era main-thread regression that still exists on
main.The menu readiness signature (
menuAdjunctReadinessSignature()) serializes every enabled provider's token snapshot plus its 30-day daily breakdown (and the OpenAI dashboard per-service breakdown) into a string on every store mutation, viaobserveStoreChanges. This runs on the main actor on each background refresh tick and scales with provider/account count.Three changes:
observeStoreChangesalways computeddidMenuAdjunctReadinessChange()to pass asrefreshOpenMenus, butinvalidateMenusonly consults that argument while a menu is open. When the menu is closed (the common case during background refresh) the result was computed and discarded.!menuNeedsRefresh(menu)afterrefreshMenuForOpenIfNeeded— i.e. the menu was rebuilt or was already fresh for the currentmenuContentVersion. If an in-flight provider refresh causesrefreshMenuForOpenIfNeededto preserve stale content, resync is skipped so the refresh-completion update is not masked. Nested submenu opens intentionally do not resync, so a pending parent refresh can't be masked. Thanks to @clawsweeper for catching both the original drift case and the in-flight stale-preservation case.String(format: "%.8f", …)(a hot per-value cost run for every daily/service value of every provider) with the rawDoublebit pattern. The signature is only ever compared for equality against the previous signature, so the bit pattern is both exact (no rounding collisions) and far cheaper. This also helps while a menu is open, where the signature is recomputed on each tick.Net effect: less main-thread work both at idle (closed) and during interaction (open), which is the responsiveness path that regressed in 0.32.
Proof
Real app — freshly built ad-hoc bundle of this branch, 7 providers enabled (codex, claude, cursor, gemini, antigravity, minimax, deepseek),
refreshFrequency = oneMinute, menu closed. A 65 ssampleof the runningCodexBarprocess — spanning at least one background refresh tick — shows the menu signature/rebuild path never lands on the main thread:So with a multi-provider setup and the menu closed, the readiness/rebuild work is no longer a sustained main-thread cost. (Honest caveat:
sampleis 1 ms-coarse and a single per-tick signature call is ~tens of µs, so this demonstrates the path is not a sustained closed-path cost rather than a precise per-call delta.)Real app — menu open with 7 providers enabled (same ad-hoc bundle: codex, claude, cursor, gemini, antigravity, minimax, deepseek). Opened via
osascript+ System Events (click menu bar item "CodexBar"):So the open-menu interaction path is responsive on first open, and there is no sustained signature/rebuild churn while the menu stays open across background refresh ticks.
Runtime cost of the work skipped when closed, measured on the real
menuAdjunctReadinessSignature()with a realistic multi-provider store (4 providers, each with a 30-day token daily breakdown, 5000 calls, warm):That full ~78 µs call ran on the main actor on every store mutation pre-fix, even with all menus closed (i.e. on each background refresh tick). It now runs only when a menu is actually open, and the cost grows with enabled providers/accounts.
Why the per-call cost itself also dropped — signature double-formatting micro-benchmark (
swiftc -O, 60 doubles/iter × 200k iters):Regression tests are negative-validated.
reopening root menu resyncs readiness baseline so reverted store data still refreshes— fails when resync is disabled, passes with it:root open during in flight refresh preserves stale content and does not resync baseline— fails when baseline resync is unconditional on root open, passes when gated on!menuNeedsRefresh(menu):Test plan
swift buildmake check(SwiftFormat + SwiftLint, 0 violations, 1018 files)swift test --filter StatusMenuTests— includes existing open-menu refresh coverage plus both baseline-resync regression tests (closed reopen revert + in-flight stale preservation)sample, 7 providers, menu closed — no main-thread signature/rebuild churn (see Proof)sampleshows 0 signature/rebuild hot-path hits, main thread parked (see Proof). Maintainer/reporter multi-account spot-check still welcome on merge but is not author-blocking (clawsweeper: ready for maintainer look).Notes / follow-ups (not in this PR)
removeAllItems+ rebuild theirNSHostingViewwhen underlying data changes; gating that on a per-provider content fingerprint is a possible follow-up (overlaps active menu-card-height work).mainby the recent Codex token-scan budget commits.Made with Cursor