Skip to content

Avoid closed menu rebuilds during data refresh#1291

Open
Nicolas0315 wants to merge 1 commit into
steipete:mainfrom
Nicolas0315:codex/reduce-closed-menu-rebuilds
Open

Avoid closed menu rebuilds during data refresh#1291
Nicolas0315 wants to merge 1 commit into
steipete:mainfrom
Nicolas0315:codex/reduce-closed-menu-rebuilds

Conversation

@Nicolas0315
Copy link
Copy Markdown

@Nicolas0315 Nicolas0315 commented Jun 3, 2026

Summary

  • skip closed attached menu preparation for data-refresh invalidations that allow stale menu content
  • cancel pending closed-menu rebuilds in that path so background refreshes do not spend CPU measuring SwiftUI menu cards
  • keep open-time rebuild behavior covered with a regression test

Evidence

  • Pre-fix local sample of CodexBar 0.32.4 showed CPU under StatusItemController.rebuildClosedMenuIfNeeded -> populateMenu -> SwiftUI menu card height measurement while the menu was closed.

After-fix profiler proof

  • Built PR head c0d5952a04eb60860747436724b84481c89cf98b locally with swift build --product CodexBar.
  • Local build note: this machine has only Command Line Tools, so the proof build used a local-only, uncommitted compile workaround that removed dependency #Preview declarations from KeyboardShortcuts and replaced SwiftUI @Entry with the equivalent EnvironmentKey implementation. The app/menu refresh code under review was unchanged, and the working tree was restored afterward.
  • Ran the PR-built app with Keychain/OpenAI Web disabled via runtime defaults: .build/arm64-apple-macosx/debug/CodexBar -debugDisableKeychainAccess YES -openAIWebAccessEnabled NO -refreshFrequency oneMinute -debugLogLevel debug.
  • Waited 70 seconds to cross the oneMinute refresh cadence with the menu closed, then captured sample <pid> 20 -file pr1291-afterfix-idle-after-refresh.sample.txt.
  • Sample result: process CPU was 0.0% before and after the sample window, and the sample contained zero occurrences of the pre-fix hot-path symbols:
    • rebuildClosedMenuIfNeeded: 0
    • populateMenu: 0
    • NSHostingController: 0
    • sizeThatFits: 0
    • closed menu rebuild completed: 0
  • The sample call graph was dominated by AppKit/CFRunLoop waiting for events; no closed menu rebuild or SwiftUI menu card measurement appeared after the refresh cadence elapsed.

Tests

  • git diff --check
  • swift test --filter StatusMenuTests attempted, but local Command Line Tools environment failed while compiling dependency KeyboardShortcuts because PreviewsMacros for #Preview was unavailable.
  • swift build --product CodexBar passed for the local proof build with the macro-only compile workaround described above.

Local limitations

  • xcode-select -p is /Library/Developer/CommandLineTools; this machine has no /Applications/Xcode*.app, and home-mac-main also has no full Xcode install.
  • Exact-source macOS app build/tests still need CI or a full-Xcode machine because CLT lacks the SwiftUI macro plugins needed by dependencies.

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 3, 2026

Codex review: needs maintainer review before merge. Reviewed June 3, 2026, 12:21 PM ET / 16:21 UTC.

Summary
The PR skips closed attached-menu preparation for stale data-refresh invalidations, captures the required rebuild version for delayed closed-menu tasks, and adds two StatusMenu regression tests.

Reproducibility: yes. there is a high-confidence source reproduction path: current main routes stale store observations into invalidateMenus with allowStaleContentDuringDataRefresh: true, then still prepares closed attached menus when no menu is open. I did not run a live macOS profiler in this read-only review.

Review metrics: 2 noteworthy metrics.

  • Patch size: 2 files changed, +92/-1. The implementation surface is small and concentrated in the menu refresh path plus its focused tests.
  • Regression coverage: 2 tests added. The tests cover both skipping stale data-refresh closed rebuilds and preserving required closed-menu preparation across later stale invalidations.

Merge readiness
Overall: 🐚 platinum hermit
Proof: 🐚 platinum hermit
Patch quality: 🐚 platinum hermit
Result: ready for maintainer review.

Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch.

Rank-up moves:

  • Confirm swift test --filter StatusMenuTests in CI or on a full-Xcode machine.

Risk before merge

  • [P1] The contributor could not run the exact-source focused StatusMenuTests locally because the Command Line Tools environment lacks required SwiftUI macro plugins; CI or a full-Xcode machine should confirm the new tests.
  • [P1] The after-fix profiler proof used a local-only compile workaround outside the app/menu code, so maintainers should still prefer exact-source build/test signal before merge.

Maintainer options:

  1. Decide the mitigation before merge
    Land the narrow menu-refresh change after CI or a full-Xcode run confirms the focused StatusMenu tests on the exact PR source.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • No ClawSweeper repair is needed; the remaining action is normal maintainer review and CI/full-Xcode validation.

Security
Cleared: The diff only changes app menu-refresh logic and tests; I found no concrete security or supply-chain concern.

Review details

Best possible solution:

Land the narrow menu-refresh change after CI or a full-Xcode run confirms the focused StatusMenu tests on the exact PR source.

Do we have a high-confidence way to reproduce the issue?

Yes, there is a high-confidence source reproduction path: current main routes stale store observations into invalidateMenus with allowStaleContentDuringDataRefresh: true, then still prepares closed attached menus when no menu is open. I did not run a live macOS profiler in this read-only review.

Is this the best way to solve the issue?

Yes, the PR is the narrowest maintainable shape I found: skip closed preparation only for stale data-refresh invalidations and compare delayed tasks against the captured required rebuild version. The remaining validation need is exact-source CI/full-Xcode test confirmation, not a different implementation.

AGENTS.md: found and applied where relevant.

Codex review notes: model gpt-5.5, reasoning high; reviewed against 3387cc8b2d47.

Label changes

Label changes:

  • add proof: sufficient: Contributor real behavior proof is sufficient. The PR body now includes after-fix profiler/sample output from a real PR-built app session showing no closed-menu rebuild hot-path work after the refresh cadence elapsed.
  • add rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • add status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (logs): The PR body now includes after-fix profiler/sample output from a real PR-built app session showing no closed-menu rebuild hot-path work after the refresh cadence elapsed.
  • remove rating: 🧂 unranked krab: Current PR rating is rating: 🐚 platinum hermit, so this older rating label is no longer current.
  • remove status: 📣 needs proof: Current PR status label is status: 👀 ready for maintainer look.

Label justifications:

  • P2: This is a normal-priority performance/behavior bugfix for menu refresh CPU work with limited blast radius.
  • rating: 🐚 platinum hermit: Overall readiness is 🐚 platinum hermit; proof is 🐚 platinum hermit and patch quality is 🐚 platinum hermit.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (logs): The PR body now includes after-fix profiler/sample output from a real PR-built app session showing no closed-menu rebuild hot-path work after the refresh cadence elapsed.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body now includes after-fix profiler/sample output from a real PR-built app session showing no closed-menu rebuild hot-path work after the refresh cadence elapsed.
Evidence reviewed

What I checked:

  • Repository policy read: The full target AGENTS.md was read; its guidance to keep changes small, prefer focused menu/model tests, and avoid validation that can trigger Keychain prompts shaped this review. (AGENTS.md:1, 3387cc8b2d47)
  • Current main still prepares closed menus after stale data invalidation: On current main, invalidateMenus increments menuContentVersion and, when no menus are open, unconditionally calls prepareAttachedClosedMenusIfNeeded even when allowStaleContentDuringDataRefresh is true. (Sources/CodexBar/StatusItemController+MenuTracking.swift:47, 3387cc8b2d47)
  • Stale data-refresh source path: Store observation calls invalidateMenus with allowStaleContentDuringDataRefresh: true, which is the path the PR narrows for closed menu background work. (Sources/CodexBar/StatusItemController.swift:444, 3387cc8b2d47)
  • PR diff scope: The PR patch changes two files: one guard and one captured required-version check in StatusItemController+MenuTracking.swift, plus two regression tests in StatusMenuOpenRefreshTests.swift. (Sources/CodexBar/StatusItemController+MenuTracking.swift:44, c0d5952a04eb)
  • After-fix runtime proof: The PR body now reports a PR-built app run past the one-minute refresh cadence with the menu closed, 0.0% process CPU, and zero sampled occurrences of rebuildClosedMenuIfNeeded, populateMenu, NSHostingController, sizeThatFits, or the closed-menu rebuild log marker. (c0d5952a04eb)
  • Menu refresh history: Commit 085319c introduced the current stale-content and delayed closed-menu rebuild machinery that this PR adjusts. (Sources/CodexBar/StatusItemController+MenuTracking.swift:26, 085319c5e9ae)

Likely related people:

  • steipete: Peter Steinberger authored adjacent menu refresh performance work and the current 0.32.5 development baseline, and has the strongest shortlog presence in the touched menu files. (role: recent area contributor; confidence: high; commits: 482f1da5adbb, 07ed3facdd4e, 3387cc8b2d47; files: Sources/CodexBar/StatusItemController+Menu.swift, Sources/CodexBar/StatusItemController+MenuTracking.swift, Tests/CodexBarTests/StatusMenuOpenRefreshTests.swift)
  • pickaxe: The current delayed closed-menu rebuild and stale-content data-refresh machinery appears to date to their recent commit. (role: introduced behavior; confidence: high; commits: 085319c5e9ae; files: Sources/CodexBar/StatusItemController+MenuTracking.swift, Tests/CodexBarTests/StatusMenuOpenRefreshTests.swift)
What the crustacean ranks mean
  • 🦀 challenger crab: rare, exceptional readiness with strong proof, clean implementation, and convincing validation.
  • 🦞 diamond lobster: very strong readiness with only minor maintainer review expected.
  • 🐚 platinum hermit: good normal PR, likely mergeable with ordinary maintainer review.
  • 🦐 gold shrimp: useful signal, but proof or patch confidence is still limited.
  • 🦪 silver shellfish: thin signal; proof, validation, or implementation needs work.
  • 🧂 unranked krab: not merge-ready because proof is missing/unusable or there are serious correctness or safety concerns.
  • 🌊 off-meta tidepool: rating does not apply to this item.

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 keeps one durable marker-backed review comment per issue or PR.
  • Re-runs edit this comment so the latest verdict, findings, and automation markers stay together instead of adding duplicate bot comments.
  • A fresh review can be triggered by eligible @clawsweeper re-review comments, exact-item GitHub events, scheduled/background review runs, or manual workflow dispatch.
  • PR/issue authors and users with repository write access can comment @clawsweeper re-review or @clawsweeper re-run on an open PR or issue to request a fresh review only.
  • Maintainers can also comment @clawsweeper review to request a fresh review only.
  • Fresh-review commands do not start repair, autofix, rebase, CI repair, or automerge.
  • Maintainer-only repair and merge flows require explicit commands such as @clawsweeper autofix, @clawsweeper automerge, @clawsweeper fix ci, or @clawsweeper address review.
  • Maintainers can comment @clawsweeper explain to ask for more context, or @clawsweeper stop to stop active automation.

@Nicolas0315 Nicolas0315 force-pushed the codex/reduce-closed-menu-rebuilds branch from 85f952a to c0d5952 Compare June 3, 2026 13:05
@clawsweeper clawsweeper Bot added rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. P2 Normal priority bug or improvement with limited blast radius. labels Jun 3, 2026
@Nicolas0315
Copy link
Copy Markdown
Author

Added after-fix idle sample proof to the PR body. The PR-built app ran past the one-minute refresh cadence with the menu closed, sampled at 0.0% CPU, and the sample had zero occurrences of the pre-fix closed-menu hot-path symbols (rebuildClosedMenuIfNeeded, populateMenu, NSHostingController, sizeThatFits). I documented the local CLT macro-only compile workaround and restored the working tree afterward.

@clawsweeper re-review

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented Jun 3, 2026

🦞🧹
ClawSweeper re-review requested.

I asked ClawSweeper to review this item again.
Action: item re-review queued (workflow sweep.yml, event repository_dispatch).
Result: the existing ClawSweeper review comment will be edited in place when the review finishes.

Re-review progress:

@clawsweeper clawsweeper Bot added proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR. and removed rating: 🧂 unranked krab Not merge-ready due to missing proof or serious correctness/safety concerns. status: 📣 needs proof The PR needs real behavior proof before ClawSweeper can clear the contributor ask. labels Jun 3, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. status: 👀 ready for maintainer look ClawSweeper has no concrete contributor-facing blocker left for this PR.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant