Avoid redundant menu-open refreshes - #1277
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 provider refresh is deferred only when menu data is missing or stale, and expands test coverage to assert no deferred refresh occurs when opening an already-fresh menu.
Changes:
- Gate deferred refresh scheduling behind a “missing or stale menu data” check.
- Add a new test for “fresh menu” open behavior and adjust/extend the existing deferred-refresh test scenario.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| Tests/CodexBarTests/StatusMenuOpenRefreshTests.swift | Adds/renames tests to validate deferred refresh is (not) scheduled depending on whether menu data is present. |
| Sources/CodexBar/StatusItemController+Menu.swift | Changes open-menu refresh scheduling to defer refresh only when menu content is stale or missing. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Queue refresh work only when visible menu data is missing or stale. AppKit menu tracking is modal; | ||
| // starting provider refreshes while it is active can make the menu feel frozen and can block keyboard | ||
| // focus from returning. | ||
| if self.menuNeedsDelayedRefreshRetry(for: menu) { | ||
| self.deferMenuInteractionRefreshIfNeeded() | ||
| } | ||
| let key = ObjectIdentifier(menu) | ||
| self.menuRefreshTasks[key]?.cancel() | ||
| self.menuRefreshTasks[key] = Task { @MainActor [weak self, weak menu] in |
| for _ in 0..<20 { | ||
| await Task.yield() | ||
| } | ||
| #expect(providerRefreshCount == 0) | ||
| #expect(!controller.deferredMenuInteractionRefreshPending) | ||
|
|
||
| controller.menuDidClose(menu) | ||
| for _ in 0..<40 { | ||
| await Task.yield() | ||
| } |
|
Codex review: needs maintainer review before merge. Reviewed June 2, 2026, 9:01 AM ET / 13:01 UTC. Summary Reproducibility: yes. for the source-level behavior: current main unconditionally marks a deferred provider refresh on menu open, and the PR test split covers fresh-data and missing-data paths. I did not independently reproduce the macOS lag live during this read-only review. Review metrics: 1 noteworthy metric.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Merge the narrow refresh guard after normal maintainer check review, while keeping #1235 open for any remaining menu-lag investigation. Do we have a high-confidence way to reproduce the issue? Yes for the source-level behavior: current main unconditionally marks a deferred provider refresh on menu open, and the PR test split covers fresh-data and missing-data paths. I did not independently reproduce the macOS lag live during this read-only review. Is this the best way to solve the issue? Yes; this is a narrow fix for the redundant refresh path because it gates only provider refresh deferral and preserves the existing missing/stale retry behavior. The broader linked performance report should remain open for residual lag work. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 9e6557cc6fa6. 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
|
01af59e to
715dd2c
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
715dd2c to
d06226e
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
@clawsweeper re-review |
|
🦞👀 Command router queued. I will update this comment with the next step. Re-review progress:
|
|
Adding context as the author: I did a fair amount of local testing before settling on this narrow refresh-gate. I also tried a couple of other angles first — profiling |
Summary
Why
Refs #1235. This follows the existing open-menu refresh direction from #923/#1040/#946: menu-open/background refreshes should stay non-blocking and avoid invalidating or rebuilding the visible
NSMenuunless data is actually stale or missing.Scope note for #1235: 0.32.3 already cached provider brand icons (#1274), and 0.32.2 capped/sped up Codex token scanning. This PR is a separate, additive path: it removes the redundant deferred refresh that 0.32.1's "defer refresh until tracking ends" change queued unconditionally on every open, even when no data was stale or missing.
Evidence
Focused fresh-menu behavior
Command:
Redacted output excerpt:
Missing-data retry behavior preserved
Command:
Redacted output excerpt:
Lint / format gate
Command:
Redacted output excerpt:
ClawSweeper status
ClawSweeper's current blocker is real app behavior proof, not patch quality. I kept this PR scoped to the narrow refresh guard and did not push temporary profiling instrumentation or unrelated packaging/widget-project changes. A real macOS app proof should show:
Notes
populateMenucost is a separate performance issue.Maintainer live menu proof
Ran against a freshly packaged
CodexBar.appfrom this branch on June 2, 2026. Prompt-risk settings were temporarily disabled for the run: keychain access disabled, refresh cadence manual, OpenAI Web off, Claude Web extras off, token-cost scanning off, and storage-footprint scanning off. Defaults were restored afterwards and the test app was stopped.Packaged CLI smoke:
Peekaboo proof:
Visual check: screenshot showed the merged Overview menu rendered with provider rows and no blocking prompt.
Repeated open / click-away proof:
Runtime sample after settle: