Skip to content

Improve menu performance#1198

Open
darkamenosa wants to merge 4 commits into
steipete:mainfrom
darkamenosa:fix/menu-performance
Open

Improve menu performance#1198
darkamenosa wants to merge 4 commits into
steipete:mainfrom
darkamenosa:fix/menu-performance

Conversation

@darkamenosa
Copy link
Copy Markdown

Summary

Reduce menu lag observed on local machine. Branch bundles several main-thread cost reductions and adds a stall monitor to surface remaining hot spots.

  • Coalesce visible menu rebuilds (StatusItemController+MenuRefreshScheduling.swift) so rapid usage updates collapse into one rebuild instead of N.
  • Split menu population/sizing/prewarming/highlighting/token-cost wiring into focused extensions (StatusItemController+MenuPopulation.swift, +MenuSizing.swift, +MenuPrewarming.swift, +MenuHighlighting.swift, +TokenCostDeferral.swift, +TokenCostHydration.swift, +SwitcherMenuItems.swift) — keeps +Menu.swift smaller and avoids redundant work during open.
  • Skip menu highlight rebroadcast to unchanged rows — only the previous + current MenuCardHighlighting view receive setHighlighted(_:), tracked via highlightedMenuItems (was O(items) per move; now O(1)).
  • Defer token-cost hydration off the menu-open critical path (UsageStore+TokenCostDeferral.swift, UsageStore+TokenCostScheduling.swift, UsageStore+TokenCost.swift).
  • Reuse parsed payloads in PiSessionCostScanner / CostUsageScanner to shrink JSONL re-scan overhead.
  • Add MainRunLoopStallMonitor + StatusItemController+StallMonitoring.swift to log main-runloop stalls ≥ 350 ms with menu/refresh metadata.

Commands run

  • swift test — 3103/3103 tests pass.
  • make check — 0 SwiftFormat/SwiftLint violations across 984 files.
  • CODEXBAR_SIGNING=adhoc ./scripts/package_app.sh release + ./scripts/launch.sh — app packages and stays running.

Test plan

  • Open the menu repeatedly with multiple providers enabled; confirm no visible lag and no main runloop stall detected warnings in Console.
  • Hover across menu items; only the highlighted card animates (no full-menu flicker).
  • Trigger a usage refresh while the menu is open; the menu updates without rebuilding from scratch.

Notes

Built locally with ad-hoc signing (no Developer ID identity on the contributor machine). Upstream release signing path unchanged.

@clawsweeper
Copy link
Copy Markdown

clawsweeper Bot commented May 28, 2026

Codex review: needs real behavior proof before merge. Reviewed May 28, 2026, 6:20 AM ET / 10:20 UTC.

Summary
The PR refactors CodexBar menu construction and refresh paths, defers token-cost scans during menu interaction, adds cache hydration/loading UI and runloop stall logging, and updates related tests.

Reproducibility: yes. for the review finding: source inspection shows the new cache-only loader applies .vertexAIOnly for Vertex and does not repeat current main's .all fallback when that report is empty. I did not run Swift tests because this review must keep the checkout read-only.

Review metrics: 2 noteworthy metrics.

  • Diff surface: 53 files, +3234/-753. This is a broad runtime refactor across menu construction, scanners, scheduling, and tests, so maintainers need more than unit-test signal.
  • Real proof artifacts: 0 attached. The PR changes visible menu performance and hover behavior but provides no recording, logs, screenshot, or copied live output showing the after-fix result.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🦐 gold shrimp
Result: blocked until real behavior proof is added.

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

Rank-up moves:

  • [P2] Restore the cache-only Vertex fallback and add focused coverage for it.
  • Attach redacted runtime proof showing repeated menu opens, hover behavior after reopen, open-menu refresh behavior, and stall-monitor output or absence of warnings.

Proof guidance:

  • [P1] Needs real behavior proof before merge: Missing: the PR lists commands and an unchecked manual plan, but no after-fix runtime output, logs, screenshot, or recording; the contributor should add redacted proof in the PR body so ClawSweeper can re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.

Mantis proof suggestion
A short desktop proof would materially help because the changed behavior is visible macOS menu opening, hover highlighting, and stall logging. A maintainer can ask Mantis to capture proof by posting a new PR comment that starts with the OpenClaw Mantis account mention, followed by:

visual task: verify CodexBar menu opens repeatedly, hover highlighting works after reopen, open-menu refresh updates correctly, and no main runloop stall warnings appear.

Risk before merge

  • [P1] The PR still lacks real after-fix runtime proof for repeated menu opens, hover behavior, open-menu refreshes, and the new stall-monitor path.
  • [P1] The branch changes live menu tracking, refresh scheduling, and token-cost scheduling across 53 files, so green tests do not settle AppKit runtime and upgrade safety.
  • [P1] Current main already has a narrower coalesced menu rebuild fix, so maintainers need to decide whether the remaining broad refactor should land as one branch.

Maintainer options:

  1. Fix cache fallback and add proof (recommended)
    Preserve the full-loader Vertex fallback in loadCachedTokenSnapshot, add focused coverage for that cache-only path, and attach redacted runtime proof for the menu interaction changes.
  2. Split the runtime refactor
    Maintain the narrow main-branch menu rebuild fix and ask for smaller follow-up PRs around token-cost hydration, smart menu updates, and stall monitoring.
  3. Accept broad runtime risk
    Maintainers can choose to merge after the cache fix with tests alone, but they would own unproven AppKit menu-tracking and token-cost scheduling behavior.

Next step before merge

  • [P2] Needs contributor proof plus a narrow cache-fallback code fix; automation cannot supply the contributor's local runtime proof for this broad menu-performance change.

Security
Cleared: No concrete security or supply-chain regression was found; the diff does not add dependencies, workflows, install scripts, secret handling, or downloaded code execution.

Review findings

  • [P2] Preserve Vertex fallback when hydrating cache — Sources/CodexBarCore/CostUsageFetcher.swift:199-204
Review details

Best possible solution:

Preserve the existing Vertex fallback in the cache-only hydration path, keep the latest highlight regression fix, and require redacted live menu/runtime proof before merge.

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

Yes for the review finding: source inspection shows the new cache-only loader applies .vertexAIOnly for Vertex and does not repeat current main's .all fallback when that report is empty. I did not run Swift tests because this review must keep the checkout read-only.

Is this the best way to solve the issue?

No, not merge-ready yet. The performance direction is plausible, but the cache-only loader should preserve the existing Vertex fallback and the PR still needs real after-fix menu/runtime proof.

Full review comments:

  • [P2] Preserve Vertex fallback when hydrating cache — Sources/CodexBarCore/CostUsageFetcher.swift:199-204
    The new cache-only hydration path loads Vertex with .vertexAIOnly and returns without the full loader's retry using .all when the strict report is empty. Existing legacy or untagged Claude log caches can therefore show the loading or empty state until a slower revalidation scan finishes, even though current main would surface usable cached data.
    Confidence: 0.88

Overall correctness: patch is incorrect
Overall confidence: 0.86

AGENTS.md: found and applied where relevant.

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

Label changes

Label changes:

  • add P2: This is a normal-priority performance improvement with a concrete cache behavior regression and limited blast radius.
  • add merge-risk: 🚨 compatibility: The PR changes existing menu tracking and token-cost cache behavior, including a current Vertex fallback path used by existing setups.

Label justifications:

  • P2: This is a normal-priority performance improvement with a concrete cache behavior regression and limited blast radius.
  • merge-risk: 🚨 compatibility: The PR changes existing menu tracking and token-cost cache behavior, including a current Vertex fallback path used by existing setups.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🦐 gold shrimp.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: Missing: the PR lists commands and an unchecked manual plan, but no after-fix runtime output, logs, screenshot, or recording; the contributor should add redacted proof in the PR body so ClawSweeper can re-review. After adding proof, update the PR body; ClawSweeper should re-review automatically. If it does not, the PR author or someone with repository write access can comment @clawsweeper re-review.
Evidence reviewed

What I checked:

Likely related people:

  • steipete: Current-main history and blame for the menu refresh paths and Vertex token-cost fallback both point to Peter Steinberger as the best routing candidate. (role: recent area contributor; confidence: high; commits: 529e25538a20, 14b2b2524f7c; files: Sources/CodexBar/StatusItemController+Menu.swift, Sources/CodexBar/StatusItemController+MenuRefreshScheduling.swift, Sources/CodexBar/StatusItemController+MenuTracking.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.

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: eab7936d5a

ℹ️ 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 on lines +199 to +204
var daily = CostUsageScanner.loadDailyReport(
provider: provider,
since: since,
until: until,
now: now,
options: options)
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P2 Badge Preserve Vertex fallback when hydrating cache

When Vertex AI is enabled while Claude is also enabled, allowVertexClaudeFallback is false, so this cache-only hydration reads only .vertexAIOnly. The full token loader immediately retries with .all when that report is empty, which covers existing/legacy Claude log caches without Vertex provider tags; this new menu-hydration path skips that retry, so Vertex cost history shows the loading/empty state until the slower revalidation scan finishes even though usable cached data exists.

Useful? React with 👍 / 👎.

@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 May 28, 2026
Stale entry caused first hover after reopen to skip setHighlighted(true)
when the same row was highlighted last.
@darkamenosa
Copy link
Copy Markdown
Author

Here is the proof of that working: https://youtu.be/SANjIeV_FoQ

@okhomenko
Copy link
Copy Markdown

+1. Since last week, CodexBar has become nearly unusable. Even exiting takes around 5 seconds, and switching between tabs is extremely slow.

@steipete
Copy link
Copy Markdown
Owner

Thanks for the performance work here. I pulled out one narrower piece from this PR and landed it separately on main: ca63a8e7

What landed:

  • Cost-usage token refreshes now use cancellable scanner paths for Codex, Claude/Vertex, and Pi session logs.
  • Cancellation now propagates before cache writes, so quit/account switch/forced refresh can abandon stale scans without persisting partial cache state.
  • Added regression coverage for pre-scan cancellation and cache preservation.

I am leaving this PR open for now rather than merging the whole branch; it is still useful as an ideas branch for smaller follow-up extractions.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants