Skip to content

Reduce idle menu power usage - #1292

Closed
Nicolas0315 wants to merge 2 commits into
steipete:mainfrom
Nicolas0315:optimize-idle-power
Closed

Reduce idle menu power usage#1292
Nicolas0315 wants to merge 2 commits into
steipete:mainfrom
Nicolas0315:optimize-idle-power

Conversation

@Nicolas0315

@Nicolas0315 Nicolas0315 commented Jun 3, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Stop eager prewarming for closed menus by default so idle invalidations do not spend SwiftUI layout work on menu cards.
  • Keep the closed-menu rebuild path covered behind the existing DEBUG-only test hooks, and update the main behavior test to refresh stale content on next open.
  • Preserve the existing OpenAI Web battery saver absent-key behavior for upgrades; this PR no longer flips users without a stored openAIWebBatterySaverEnabled value into battery saver mode.

Background

A local pre-fix CPU sample of the running menu bar app showed idle CPU dominated by StatusItemController.rebuildClosedMenuIfNeeded(_:) -> populateMenu(_:provider:) -> SwiftUI NSHostingController.sizeThatFits(in:), while the menu was closed. WebKit helper processes were not the hot path in that sample.

Review updates

  • Addressed the ClawSweeper P1 compatibility finding in 3f907011 by restoring the openAIWebBatterySaverEnabled fallback to false and matching tests to that upgrade-safe behavior.
  • Existing installs with OpenAI Web extras enabled but no stored battery saver key keep their previous effective regular background refresh behavior.

After-fix profiler proof

  • Built the PR head 3f9070117d350e73c6e90c784c670a7bbdafe953 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.
  • Captured an idle closed-menu sample: sample <pid> 15 -file pr1292-afterfix-idle.sample.txt.
  • Sample result: process CPU was 0.0% before and after the 15s 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 the AppKit event loop waiting for events; the only CodexBar activity observed was status icon observation/update work, not closed menu rebuild or SwiftUI menu card measurement.

Verification

  • git diff --check passed
  • .build/lint-tools/bin/swiftformat --lint . passed: 0/1003 files require formatting, 87 files skipped
  • swift build --product CodexBarCLI passed
  • 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 swift build --product CodexBar is blocked before this change by dependency SwiftUI preview macros under CLT-only setup: PreviewsMacros is unavailable in KeyboardShortcuts.
  • Exact-source swift test --filter SettingsStoreTests is blocked before the target tests by the same PreviewsMacros issue plus the local CLT test compile not finding module Testing for TestsLinux.
  • make check ran SwiftFormat successfully, then SwiftLint failed locally because SourceKitten could not load sourcekitdInProc under the CLT-only setup.
  • Maintainer CI or a full-Xcode validation run is still useful for exact-source macOS tests.

@clawsweeper

clawsweeper Bot commented Jun 3, 2026

Copy link
Copy Markdown

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

Summary
The PR disables closed-menu preparation by default, keeps the old prewarm path behind DEBUG test hooks, and updates menu/settings tests for stale-until-open and battery-saver upgrade behavior.

Reproducibility: not applicable. as an open PR review. The PR body provides after-fix profiler proof for the closed-menu idle path, but I did not run a local reproduction in this read-only review.

Review metrics: 1 noteworthy metric.

  • Changed surface: 5 files, +37/-7. The diff is small, but it touches central menu refresh scheduling where runtime responsiveness matters.

Merge readiness
Overall: 🦞 diamond lobster
Proof: 🦞 diamond lobster
Patch quality: 🦞 diamond lobster
Result: ready for maintainer review.

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

Risk before merge

  • [P1] Closed persistent menus now skip idle prewarming, so stale menu content is rebuilt synchronously on next open; maintainers should explicitly accept the idle-power versus first-open latency tradeoff.
  • [P1] The contributor's runtime proof used a local macro-only compile workaround because their CLT-only setup could not build exact source, so maintainer CI or a full-Xcode run should confirm exact-source build/test behavior before merge.

Maintainer options:

  1. Accept the idle-power tradeoff (recommended)
    Merge after maintainer/full-Xcode validation if the project is comfortable moving closed-menu SwiftUI layout work from idle time to the next menu open.
  2. Keep prewarming configurable
    If next-open latency is not acceptable for some users, ask for an explicit opt-in path before changing the default globally.
  3. Pause on reproduced open stalls
    If maintainer profiling shows unacceptable first-open stalls, pause this branch and keep the current prewarm behavior while a narrower strategy is designed.

Next step before merge

  • [P2] The remaining action is maintainer acceptance of the global closed-menu prewarm default and exact-source/full-Xcode validation, not an automated code repair.

Security
Cleared: No concrete security or supply-chain concern found; the diff is limited to menu scheduling source and focused tests.

Review details

Best possible solution:

Land the narrow idle-power change only if maintainers accept the first-open latency tradeoff and exact-source full-Xcode validation stays clean.

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

Not applicable as an open PR review. The PR body provides after-fix profiler proof for the closed-menu idle path, but I did not run a local reproduction in this read-only review.

Is this the best way to solve the issue?

Yes, conditionally: the code change is narrow and keeps upgrade-sensitive battery-saver behavior intact. The remaining question is whether maintainers want this runtime tradeoff as the default.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P2: This is a normal-priority performance improvement with limited blast radius but a real menu responsiveness tradeoff.
  • merge-risk: 🚨 availability: Disabling closed-menu prewarming could shift SwiftUI layout work onto the next menu open and cause visible stalls on some setups.
  • rating: 🦞 diamond lobster: Overall readiness is 🦞 diamond lobster; proof is 🦞 diamond lobster and patch quality is 🦞 diamond lobster.
  • status: 👀 ready for maintainer look: ClawSweeper has no concrete contributor-facing blocker left for this PR. Sufficient (logs): The PR body/comment include after-fix profiler output showing the PR-built app at 0.0% CPU with zero pre-fix closed-menu hot-path symbols during an idle sample.
  • proof: sufficient: Contributor real behavior proof is sufficient. The PR body/comment include after-fix profiler output showing the PR-built app at 0.0% CPU with zero pre-fix closed-menu hot-path symbols during an idle sample.
Evidence reviewed

Acceptance criteria:

  • [P1] Run exact-source full-Xcode validation through maintainer CI or local make check.
  • [P1] Optionally profile closed-menu idle and first-open latency on a freshly built app bundle.

What I checked:

  • Repository policy read: AGENTS.md was read fully and its guidance on focused menu tests, avoiding live Keychain/provider probes, and using repository validation expectations was applied to this review. (AGENTS.md:1, 3387cc8b2d47)
  • Closed-menu preparation disabled by default: The PR head adds a false default for closed-menu preparation and gates preparation through isClosedMenuPreparationEnabled. (Sources/CodexBar/StatusItemController+MenuTracking.swift:6, 3f9070117d35)
  • Close path no longer prewarms unless enabled: The persistent-menu close path only calls rebuildClosedMenuIfNeeded when closed-menu preparation is enabled. (Sources/CodexBar/StatusItemController+Menu.swift:168, 3f9070117d35)
  • Focused menu coverage updated: The updated test asserts a closed attached menu stays stale after invalidation, schedules no closed rebuild task, and refreshes on the next menu open. (Tests/CodexBarTests/StatusMenuOpenRefreshTests.swift:172, 3f9070117d35)
  • Battery-saver absent-key behavior preserved: Current source still resolves a missing openAIWebBatterySaverEnabled default to false, and the PR's cumulative diff leaves SettingsStore source unchanged. (Sources/CodexBar/SettingsStore.swift:378, 3387cc8b2d47)
  • Real behavior proof in discussion: The PR body and follow-up comment report an after-fix local sample of the PR-built app with 0.0% CPU and zero occurrences of rebuildClosedMenuIfNeeded, populateMenu, NSHostingController, sizeThatFits, or the closed-menu rebuild log during the closed-menu idle sample. (3f9070117d35)

Likely related people:

  • Peter Steinberger: Current-main blame attributes the menu open/close and closed rebuild code to the release snapshot, and recent commits by this author changed menu refresh and rebuild behavior in the same files. (role: recent area contributor; confidence: high; commits: 482f1da5adbb, 07ed3facdd4e, 723734ef3422; files: Sources/CodexBar/StatusItemController+Menu.swift, Sources/CodexBar/StatusItemController+MenuTracking.swift, Tests/CodexBarTests/StatusMenuOpenRefreshTests.swift)
  • pickaxe: Commit 085319c deferred closed-menu rebuilds during refreshes and touched the same menu tracking code and tests affected by this PR. (role: recent adjacent contributor; confidence: medium; commits: 085319c5e9ae; files: Sources/CodexBar/StatusItemController+Menu.swift, 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.

@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. labels Jun 3, 2026
@Nicolas0315

Copy link
Copy Markdown
Contributor Author

Addressed the P1 compatibility finding in 3f907011 by preserving the existing absent-key battery saver behavior (openAIWebBatterySaverEnabled remains false when unset) and updating the related settings/provider tests. Validation and local Xcode limitations are now in the PR body.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 3, 2026

Copy link
Copy Markdown

🦞🧹
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 the P2 Normal priority bug or improvement with limited blast radius. label Jun 3, 2026
@Nicolas0315

Copy link
Copy Markdown
Contributor Author

Added after-fix idle sample proof to the PR body. The PR-built app 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 also documented the local CLT macro-only compile workaround and restored the working tree afterward.

@clawsweeper re-review

@clawsweeper

clawsweeper Bot commented Jun 3, 2026

Copy link
Copy Markdown

🦞👀
ClawSweeper picked this up.

Command router queued. I will update this comment with the next step.

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. rating: 🦞 diamond lobster Very strong PR readiness with only minor maintainer review expected. merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. 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. rating: 🐚 platinum hermit Good normal PR readiness with ordinary maintainer review expected. labels Jun 3, 2026
@steipete

Copy link
Copy Markdown
Owner

Thanks for the focused power-usage patch. Closing because its idle-versus-next-open tradeoff has been superseded by the current design: closed-menu rebuild suppression, hosting-view reuse in #1394, and instant cached presentation with asynchronous refresh in #1398. That path shipped in v0.33.0 and avoids moving the full rebuild cost back into the next menu open.

@steipete steipete closed this Jun 11, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge-risk: 🚨 availability 🚨 Merging this PR could cause crashes, hangs, restart loops, stalls, or process outages. P2 Normal priority bug or improvement with limited blast radius. proof: sufficient Contributor real behavior proof is sufficient. rating: 🦞 diamond lobster Very strong PR readiness with only minor 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.

2 participants