Open data-stale dropdowns in two phases (populateMenu off the click path) - #1375
Open data-stale dropdowns in two phases (populateMenu off the click path)#1375hhh2210 wants to merge 3 commits into
Conversation
When the merged menu is invalidated by data-refresh ticks while closed (the deferred-until-next-open path from steipete#1274/steipete#1314), menuWillOpen ran the full populateMenu - including SwiftUI hosting-view creation and layout - synchronously on the click path before the dropdown could appear. PR steipete#1314 deliberately scoped this out: "This still does not remove the first populateMenu." Attach the existing stale content immediately instead, and schedule the rebuild from current store data through the open-menu rebuild scheduler so it runs right after the menu is on screen. Structural, privacy, and localization invalidations bump latestRequiredMenuRebuildVersion and still rebuild synchronously before display, and the in-flight-refresh stale-preserve path keeps precedence. Closing the menu before the deferred rebuild runs cancels it and re-defers the merged menu until its next open. Refs steipete#1274, steipete#1325 https://claude.ai/code/session_01ATo2T7154YGB9ey1TuPt6B
SwiftFormat's redundantAsync rule flags the required-invalidation test because it never awaits. https://claude.ai/code/session_01ATo2T7154YGB9ey1TuPt6B
The non-merged attached menu now also keeps stale content on open and rebuilds right after display, so assert the deferred rebuild instead of a synchronous one. https://claude.ai/code/session_01ATo2T7154YGB9ey1TuPt6B
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds “two-phase open” behavior for data-stale merged status menus (show existing content immediately, then rebuild after display), with tests and changelog entry to validate/describe the change.
Changes:
- Defer stale (data-refresh-only) merged-menu rebuild until after the menu is displayed.
- Add/extend tests covering two-phase open and cancellation behavior.
- Document the fix in the changelog.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| Tests/CodexBarTests/StatusMenuTwoPhaseOpenTests.swift | New tests validating deferred rebuild behavior and cancellation on close. |
| Tests/CodexBarTests/StatusMenuOpenRefreshTests.swift | Extends existing test to assert the two-phase open sequence. |
| Sources/CodexBar/StatusItemController+MenuTracking.swift | Implements the “defer stale rebuild until after display” decision logic and scheduling. |
| CHANGELOG.md | Notes the user-visible menu-bar behavior change/fix. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if self.canDeferStaleMenuRebuildUntilAfterDisplay(menu) { | ||
| #if DEBUG | ||
| self.menuLogger.debug( | ||
| "menu open deferred stale rebuild until after display", | ||
| metadata: [ | ||
| "items": "\(menu.items.count)", | ||
| "provider": provider?.rawValue ?? "nil", | ||
| ]) | ||
| #endif | ||
| self.scheduleOpenMenuRebuildIfStillVisible(menu, provider: provider) | ||
| return | ||
| } |
| private func canDeferStaleMenuRebuildUntilAfterDisplay(_ menu: NSMenu) -> Bool { | ||
| // Without open-menu tracking the post-display rebuild would never run, leaving stale content. | ||
| guard self.isMenuRefreshEnabled else { return false } | ||
| guard !menu.items.isEmpty else { return false } | ||
| let key = ObjectIdentifier(menu) | ||
| guard let menuVersion = self.menuVersions[key] else { return false } | ||
| return menuVersion >= self.latestRequiredMenuRebuildVersion | ||
| } |
| for _ in 0..<40 where controller.menuVersions[key] != controller.menuContentVersion { | ||
| await Task.yield() | ||
| } |
| for _ in 0..<40 { | ||
| await Task.yield() | ||
| } |
|
Codex review: needs real behavior proof before merge. Reviewed June 10, 2026, 12:36 AM ET / 04:36 UTC. Summary Reproducibility: yes. for the introduced correctness problem by source inspection: a closed data tick leaves the baseline old, the PR defers the root-open rebuild, and a later open-menu change back to that old signature can skip refresh. The original click-latency improvement still needs real app proof. 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 findings
Review detailsBest possible solution: Keep the two-phase open shape, but re-anchor readiness after the deferred rebuild, add a reverted-live-data regression test, and post redacted packaged-app timing or sample proof before merge. Do we have a high-confidence way to reproduce the issue? Yes for the introduced correctness problem by source inspection: a closed data tick leaves the baseline old, the PR defers the root-open rebuild, and a later open-menu change back to that old signature can skip refresh. The original click-latency improvement still needs real app proof. Is this the best way to solve the issue? No; deferring data-only rebuilds is a plausible direction, but the current implementation needs to preserve the readiness-baseline semantics from current main and provide real behavior proof. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 6f6cb097dc58. 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
|
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Closing this approach after landing #1376 in #1376 fixes the observed WindowServer stall by deferring parent-menu recomposition caused by data refreshes until AppKit menu tracking ends. This PR instead schedules the full #1374 remains open. Its first-open stale-content latency still needs a design that does not rebuild the full parent menu during tracking. Thanks @hhh2210 for the investigation, tests, and clearly documented tradeoffs. |
The two-phase open returns from refreshMenuForOpenIfNeeded while the menu is still marked stale, so menuWillOpen skips the root-open baseline resync. Anchor the baseline in rebuildOpenMenuIfStillVisible once the deferred rebuild renders current store data; otherwise an open-menu store change reverting to the pre-rebuild signature compares equal against the stale baseline and the visible menu keeps the deferred-rebuild data. Addresses the ClawSweeper P2 on steipete#1375.
|
Pushed d85192a addressing the ClawSweeper P2 (readiness-baseline re-anchor) — the analysis was correct, thanks. What was wrong: the two-phase open returns from Fix: Regression coverage: added Packaged behavior proof (peekaboo open-click timing + |
Refs #1374, #1274, #1325.
Summary
menuWillOpenattaches the menu's existing content immediately and schedules the rebuild through the existingscheduleOpenMenuRebuildIfStillVisiblemachinery, so it runs right after the menu is on screen instead of synchronously on the click pathlatestRequiredMenuRebuildVersion) still rebuild synchronously before display, so stale structure is never shownforgetClosedMenucleanup, and the merged menu re-defers until next openisMenuRefreshEnabled == false) keeps the synchronous rebuild, since the post-display rebuild would never run without open-menu trackingWhy
#1314 deferred the closed merged menu until next open and explicitly scoped out the consequence: "This still does not remove the first
populateMenuor reuse hosting views; it eliminates the redundant closed rebuild only."Since every background data tick while the menu is closed leaves it deferred-stale, the next click pays the full
populateMenu(MenuDescriptor build +NSHostingViewcreation + SwiftUI layout) synchronously insidemenuWillOpenbefore the dropdown can appear — withrefreshFrequency = oneMinuteand Merge Icons on, effectively every open after ≥1 idle minute. Root-cause walkthrough in #1374; #1325's sample caughtpopulateMenu/addMenuCardsas the hot frames on that path, and the post-#1297 retest in #1274 measured it at~306 menuWillOpen → ~153 refreshMenuForOpenIfNeededinclusive main-thread samples.Scope / honesty
Validation
StatusMenuTwoPhaseOpenTests.swift: data-stale open keeps existing items and rebuilds after display; required invalidation still rebuilds synchronously before display; closing before the deferred rebuild cancels it and re-defers the merged menuStatusMenuOpenRefreshTests.swift: the non-merged stale open now asserts the two-phase deferral instead of the synchronous rebuild (the mechanism is shared, and the deferral is the same win there)swift test(full suite) + SwiftFormat/SwiftLint were green on the fork mirror of this branch at the pre-rebase head (abe46ca3); the rebase onto6f6cb097only resolved a CHANGELOG collision with Fix Antigravity summaries for untracked quotas #1369, no code changesBehavior proof status
Packaged before/after proof on macOS 26.5 (peekaboo open-click timing +
samplestack identity:populateMenuundermenuWillOpenvs under the deferred-task thunk, #1314-style) is being collected and will be posted as a follow-up comment on this PR. A retest from the #1274/#1314 measurement setup would be very welcome — asked in #1374.