Skip to content

perf: fix status item icon observer CPU loop, background web scrapes, fix switcher first-click#1073

Closed
ptstory wants to merge 4 commits into
steipete:mainfrom
ptstory:fix/status-item-icon-observer-loop
Closed

perf: fix status item icon observer CPU loop, background web scrapes, fix switcher first-click#1073
ptstory wants to merge 4 commits into
steipete:mainfrom
ptstory:fix/status-item-icon-observer-loop

Conversation

@ptstory
Copy link
Copy Markdown
Contributor

@ptstory ptstory commented May 20, 2026

Summary

Fixes a CPU hot loop where StatusItemController.observeStoreIconChanges() was
pegging a full core at ~99% continuously, even when the rendered menu bar icon
was unchanged. Also backgrounds blocking web scrape work during regular refresh
cycles and fixes a first-click acceptance bug in the Codex account switcher.

Related: #678 — this PR addresses a separate root cause (icon observer loop, not
WebKit scrape) for the same user-facing symptom (CodexBar making the machine slow).

Problem

A 5-second spindump of a stuck CodexBar process (v0.25, build 60) showed 38 of
42 main-thread dispatch samples landing in observeStoreIconChanges()
updateIcons(). The root issue is UsageStore.iconObservationToken, which
registers Swift Observation dependencies on 10+ store fields. isRefreshing
toggles on every refresh cycle regardless of icon state, causing the observer to
re-fire immediately after re-registration.

The existing render-signature caching inside applyIcon() mitigated redundant
image/title mutation but did not suppress the observer callback itself —
updateIcons() still ran on every fire, computing render inputs, checking menu
attachment, and updating animation/blink state.

Separately, the regular refresh cycle was await-ing
refreshOpenAIDashboardIfNeeded() and refreshCreditsIfNeeded() inline, which
could block the main thread for seconds during web scrapes.

Changes

Fix 1 — Icon observer loop (StatusItemController.swift, UsageStore.swift)

  • Remove isRefreshing from iconObservationToken (noisy, not icon-relevant).
  • Add storeIconObservationSignature() that derives a string signature from
    actual icon-render inputs (snapshot, status indicator, animation state, display
    text, provider visibility).
  • Guard the observer callback: skip updateIcons() when the signature is
    unchanged.

Fix 2 — Background web scrapes (UsageStore.swift)

  • For non-forced refreshes, run refreshCreditsIfNeeded() and
    refreshOpenAIDashboardIfNeeded() as unstructured background tasks so the UI
    never blocks on slow network operations.
  • Forced refreshes (manual) still await inline.
  • Tradeoff: unstructured tasks can't be cancelled if the parent refresh is
    cancelled. Acceptable for a menu bar app on a 2-minute poll cycle.

Fix 3 — Switcher first-click (StatusItemController+SwitcherViews.swift)

  • Override acceptsFirstMouse(for:)true.
  • Swallow child button hit testing so the parent view handles mouse events.
  • Implement mouseDown/mouseUp with press tracking to prevent drag-off
    misfires.
  • Extract shared applySelection(_:) to deduplicate the action path.

Testing

  • ./Scripts/lint.sh lint
  • swift test --skip-build --filter StatusItemIconObservationSignatureTests
  • swift test --skip-build --filter CodexManagedOpenAIWebRefreshTests
  • swift test --skip-build --filter StatusMenuCodexSwitcherTests
  • swift test currently hits a pre-existing unrelated failure in
    OpenAIDashboardWebViewCacheTests (Preserved page expiry is scheduled without future cache activity).

Reproduction

  1. Enable Codex provider with openAIWebAccessEnabled = true and
    historicalTrackingEnabled = true.
  2. Observe main CodexBar process in Activity Monitor — CPU pegged at ~99%.
  3. spindump <PID> 5 confirms hot path in observeStoreIconChanges().
  4. After this patch, CPU drops to idle between refresh intervals.

ptstory added 3 commits May 20, 2026 18:58
Add a derived signature guard to observeStoreIconChanges() so
updateIcons() only runs when icon-relevant state actually changes.

Remove isRefreshing from iconObservationToken — it toggles every
refresh cycle and has no bearing on the rendered menu bar icon,
causing the observer to fire continuously on the main thread.

With both changes, the main dispatch queue path drops from ~38/42
samples in a 5-second spindump to idle between refresh intervals.
…eshes

Move refreshOpenAIDashboardIfNeeded() and refreshCreditsIfNeeded()
into unstructured tasks for non-forced refreshes so the UI never
blocks on slow web scrapes during the regular polling cycle.

Forced refreshes (manual pull-to-refresh) still await inline.
The plain NSButton inside CodexAccountSwitcherView was dropping the
first click because the menu popover wasn't key. Add
acceptsFirstMouse, swallow child hit testing so the parent view
handles mouse events directly, and forward mouseDown/mouseUp with
press tracking to prevent drag-off-then-release misfires.
Copy link
Copy Markdown

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e9e9fb8d1d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread Sources/CodexBar/StatusItemController+SwitcherViews.swift Outdated
@steipete
Copy link
Copy Markdown
Owner

Thanks for this. The direction makes sense: normal refresh should not feel stuck behind slow Codex credits/OpenAI dashboard work, and reducing menu-bar icon churn is worth looking at.

I am not comfortable merging this as-is yet:

  • The switcher part is currently red locally. swift test --filter StatusMenuCodexSwitcherTests fails the new runtime-click tests: the simulated click still selects live@example.com instead of the requested managed/second-row account. Since that is exactly the first-click fix surface, this needs to be fixed before merge.
  • The icon-loop claim needs measurement. iconObservationToken currently observes isRefreshing, so refresh start/finish can call updateIcons(), but applyIcon() already has render signatures and skips unchanged icon rendering. That may be extra observer noise, not necessarily a CPU hot loop.
  • Backgrounding credits/OpenAI web is plausible, but please add proof that repeated normal refreshes do not stack unbounded credits/dashboard tasks. OpenAI dashboard has coalescing; credits currently does not look coalesced in this PR.

What would make this mergeable:

  1. Fix the switcher tests and implementation.
  2. Add before/after measurement for normal refresh wall time and icon work count/CPU. Suggested points: UsageStore.refresh, provider refresh group, refreshCreditsIfNeeded, refreshOpenAIDashboardIfNeeded, StatusItemController.updateIcons, and actual IconRenderer.makeIcon/render calls.
  3. Consider splitting this into smaller PRs: icon observation, background Codex extras, and switcher first-click handling are separate risk surfaces.

I ran:

  • swift test --filter StatusItemIconObservationSignatureTests passes.
  • swift test --filter CodexManagedOpenAIWebRefreshTests passes isolated.
  • swift test --filter StatusMenuCodexSwitcherTests fails 2 tests in the new click path.

@ptstory
Copy link
Copy Markdown
Contributor Author

ptstory commented May 21, 2026

Thanks for the detailed review. Agreed on all points.

Plan:

  • Split into three smaller PRs as suggested: icon observation, background Codex extras, and switcher first-click handling.
  • Investigate and fix the switcher click test failures.
  • Add before/after measurement for the icon observation change (normal refresh wall time, icon work count, and actual render calls).
  • Add explicit coalescing/guarding (and coverage) for credits refresh to prevent repeated normal refreshes from stacking unbounded background work.

I'll convert this PR to draft and link the split follow up PRs here.

@ptstory
Copy link
Copy Markdown
Contributor Author

ptstory commented May 21, 2026

Split into three focused PRs per review feedback:

Closing this draft in favor of the focused PRs.

@ptstory ptstory closed this May 21, 2026
steipete pushed a commit to ptstory/CodexBar that referenced this pull request May 21, 2026
Override acceptsFirstMouse, swallow child hit testing, and implement mouseDown/mouseUp with correct coordinate space conversion for multi-row account layouts.

Root cause: the runtime-click tests synthesized hit points before NSStackView laid out the switcher rows, so every button still sat at {0,0} and the simulated click always resolved to the first account.

Split from steipete#1073.
steipete added a commit that referenced this pull request May 21, 2026
* fix: accept first click in Codex account switcher

Override acceptsFirstMouse, swallow child hit testing, and implement mouseDown/mouseUp with correct coordinate space conversion for multi-row account layouts.

Root cause: the runtime-click tests synthesized hit points before NSStackView laid out the switcher rows, so every button still sat at {0,0} and the simulated click always resolved to the first account.

Split from #1073.

* fix: preserve Codex switcher tooltip hit testing

---------

Co-authored-by: Peter Steinberger <steipete@gmail.com>
steipete pushed a commit to ptstory/CodexBar that referenced this pull request May 21, 2026
Remove isRefreshing from iconObservationToken and add observer-side
signature guard to skip updateIcons() when icon-relevant state is
unchanged.

The existing render signature inside applyIcon() avoided redundant
icon rendering, but not redundant observer work. This change reduces
observer callback churn before render-signature checking is reached.

Before: updateIcons() called 10 times per refresh cycle (1 rendered)
After: updateIcons() called 6 times per refresh cycle (1 rendered)

Split from steipete#1073. Related: steipete#678.
steipete added a commit that referenced this pull request May 21, 2026
* perf: suppress redundant icon observer callbacks

Remove isRefreshing from iconObservationToken and add observer-side
signature guard to skip updateIcons() when icon-relevant state is
unchanged.

The existing render signature inside applyIcon() avoided redundant
icon rendering, but not redundant observer work. This change reduces
observer callback churn before render-signature checking is reached.

Before: updateIcons() called 10 times per refresh cycle (1 rendered)
After: updateIcons() called 6 times per refresh cycle (1 rendered)

Split from #1073. Related: #678.

* docs: update changelog for icon observer perf

---------

Co-authored-by: Peter Steinberger <steipete@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants