Reduce merged menu rebuild latency - #1286
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Updates menu refresh behavior so that when the merged status menu closes with stale content, its rebuild is deferred until the next time the user opens it (avoiding background work on the dismiss path).
Changes:
- Add tracking for merged menus whose refresh should be deferred until next open.
- Route “closed persistent menu needs refresh” through a new handler that defers merged-menu rebuilds.
- Add a test covering the deferred rebuild behavior for merged menus.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/CodexBarTests/StatusMenuOpenRefreshTests.swift | Adds coverage for deferred rebuild behavior when closing a stale merged menu. |
| Sources/CodexBar/StatusItemController.swift | Introduces a set to track menus deferred until next open. |
| Sources/CodexBar/StatusItemController+Shutdown.swift | Clears the new deferral tracking state on shutdown. |
| Sources/CodexBar/StatusItemController+MenuTracking.swift | Skips background rebuild for deferred menus; removes deferral on open; adds deferral handler. |
| Sources/CodexBar/StatusItemController+Menu.swift | Uses the new handler when a persistent closed menu is stale. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| self.menuProviders.removeValue(forKey: key) | ||
| self.menuVersions.removeValue(forKey: key) | ||
| } else if self.menuNeedsRefresh(menu) { | ||
| self.rebuildClosedMenuIfNeeded(menu) | ||
| self.handleClosedPersistentMenuNeedingRefresh(menu) | ||
| } | ||
| self.parentMenuRebuildsDeferredDuringTracking.remove(key) | ||
| self.scheduleDeferredMenuInteractionRefreshIfNeeded() |
| for _ in 0..<20 { | ||
| await Task.yield() | ||
| } |
|
Codex review: needs maintainer review before merge. Reviewed June 3, 2026, 6:45 PM ET / 22:45 UTC. Summary Reproducibility: yes. Source inspection shows current main can schedule populateMenu from the stale persistent-menu close path, and the PR body provides macOS 26.5 packaged proof that the branch removes those frames from dismiss. 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. Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the focused fix if maintainers accept the lazy-rebuild tradeoff, then keep the broader macOS 26.x lag report open for shipped-build validation. Do we have a high-confidence way to reproduce the issue? Yes. Source inspection shows current main can schedule populateMenu from the stale persistent-menu close path, and the PR body provides macOS 26.5 packaged proof that the branch removes those frames from dismiss. Is this the best way to solve the issue? Yes. The patch is a narrow maintainable fix for the reported close-path cost while leaving the broader first-rebuild SwiftUI hosting cost tracked by the related lag issue. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 3387cc8b2d47. Label changesLabel changes:
Label justifications:
Evidence reviewedAcceptance criteria:
What 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
|
|
Nice — this lands squarely on the close-path stack @giuseppebisemi profiled, and deferring the merged-menu rebuild off the dismiss path is the right first cut. Two notes before this goes up for review: 1. Worth confirming the cost didn't just move to the open path. Since 2. Maybe Behavior + test look correct to me otherwise; the lifecycle test pins the no-rebuild-on-close / rebuild-on-open contract well. |
04d7193 to
d6dcd23
Compare
|
Addressed the two Copilot review points in the latest push:
Re-ran locally:
|
|
Updated the PR body with the local macOS/AppKit lifecycle proof and changed the issue reference from @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review — added live runtime proof to the PR body: the close/open-path menu lifecycle contract (6 tests) run on the exact reported platform, macOS 26.5 (build 25F71). The key case ( |
|
🦞👀 Command router queued. I will update this comment with the next step. Re-review progress:
|
|
@clawsweeper re-review — added a packaged-app live menu proof on macOS 26.5 (25F71): 5× open/dismiss of the merged |
|
🦞👀 Command router queued. I will update this comment with the next step. Re-review progress:
|
|
Follow-up after the newer #1274 samples/traces: pushed What changed in this follow-up:
Validation rerun locally:
Scope note: this still does not claim @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
Refs #1274.
This targets the remaining close/dismiss freeze reported after #1277. cc @giuseppebisemi because this PR specifically addresses the close-path stack you profiled in #1274.
Summary
menuDidClose, including through deferred interaction refresh follow-up.populateMenu/ SwiftUI hosting teardown and rebuild do not run on the user dismiss path.Behavior proof
Live runtime proof on the exact reported platform — macOS 26.5 (build 25F71), Apple Swift 6.3.2 — run against this branch head (
d6dcd23d) on 2026-06-03 14:13 +0800. These drive the productionmenuWillOpen/menuDidClose/populateMenupaths directly:What this establishes on the dismiss path:
merged menu close defers stale rebuild until next open— aftermenuDidClose, no closed-menu rebuild task is scheduled andmenuVersions[menu]stays at the opened version, i.e.populateMenu/ SwiftUI hosting teardown does not run on dismiss; the rebuild moves to the nextmenuWillOpen. This is the exact main-thread work @giuseppebisemi sampled (rebuildClosedMenu → populateMenu → MenuCardItemHosting+NSHostingView/AttributeGraph teardown).closed attached menu is prepared … after invalidationand… waits for store refresh to finish— non-merged closed menus and non-dismiss invalidations still prepare in the background, so the deferral is scoped to the user dismiss path only (covers the stale-content tradeoff).Scope / honesty: this is deterministic runtime proof of the AppKit menu lifecycle on macOS 26.5, not a packaged-app screen recording. The complementary field evidence is @giuseppebisemi's before-fix close-path
samplein #1274, which this change removes from the dismiss path. A maintainer-side packaged-build check on macOS 26.x remains the right gate before closing #1274 — kept asRefs, not auto-close.Maintainer live menu proof
Packaged
CodexBar.appbuilt from this branch (d6dcd23d) and launched on macOS 26.5 (build 25F71), merged status item (mergeIconson by default). Peekaboo opens and dismisses the merged menu 5x; on each dismiss the process issampled to inspect the main thread on the close path:Each dismiss settles in ~0.15s with no
populateMenu/rebuildClosedMenuframes on the main thread — the exact stack @giuseppebisemi sampled in #1274 before the fix (rebuildClosedMenu → populateMenu → MenuCardItemHosting+NSHostingView/AttributeGraph teardown, parked for seconds). That work no longer runs on the user dismiss path; it moves to the next open (open stays ~0.1s here because data was already fresh).Notes
This does not claim to make
populateMenuitself cheap. It removes the close/dismiss-path rebuild that can block the main thread; the deeperMenuDescriptor.build/ SwiftUI hosting cost is still a separate optimization target.Validation
swift test --filter '<StatusMenuTests close/open-path contract group, 6 tests>'swift test --filter StatusMenuOpenRefreshTestsmake check