Skip to content

Defer tracked menu data-refresh rebuilds - #1376

Merged
steipete merged 1 commit into
steipete:mainfrom
jangisaac-dev:fix/grok-menu-beachball
Jun 10, 2026
Merged

Defer tracked menu data-refresh rebuilds#1376
steipete merged 1 commit into
steipete:mainfrom
jangisaac-dev:fix/grok-menu-beachball

Conversation

@jangisaac-dev

@jangisaac-dev jangisaac-dev commented Jun 10, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Defer parent-menu recomposition caused by provider data refreshes until AppKit menu tracking ends.
  • Keep explicit provider switching and hosted-submenu updates immediate.
  • Add focused regressions for repeated refreshes, submenu close behavior, and keyboard provider switching during refresh.

This addresses the multi-second WindowServer stalls reported when slower providers finish while the status menu is open. The branch intentionally excludes the original Grok probe cleanup and investigation document.

Validation

  • make check
  • xcrun swift test --filter 'StatusMenuOpenRefreshTests|StatusMenuSwitcherClickTests|StatusMenuSwitcherTrackingTests' — 53 tests passed
  • Full xcrun swift test — 3,403 tests passed
  • Whole-branch autoreview against current main — no actionable findings, 0.90 confidence

@clawsweeper

clawsweeper Bot commented Jun 10, 2026

Copy link
Copy Markdown

Codex review: needs real behavior proof before merge. Reviewed June 10, 2026, 3:10 AM ET / 07:10 UTC.

Summary
The PR defers parent-menu recomposition from provider data refreshes while a menu is tracked, preserves immediate provider-switcher and hosted-submenu updates, and updates focused menu tests plus a release note.

Reproducibility: no. high-confidence live reproduction was established in this read-only review. Current main source shows the direct open-menu rebuild path and the PR adds regression tests for deferral, but the reported WindowServer stall still needs packaged-app proof.

Review metrics: 2 noteworthy metrics.

  • Diff scope: 7 files changed, +71/-20. The branch is focused on menu invalidation/navigation behavior, regression tests, and one release-note line.
  • Regression coverage: 2 test files changed. The PR now covers deferred parent rebuilds and keyboard provider switching during refresh, addressing the earlier focused-test gap.

Merge readiness
Overall: 🧂 unranked krab
Proof: 🧂 unranked krab
Patch quality: 🐚 platinum hermit
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:

  • [P1] Add redacted after-fix proof from a packaged build showing repeated menu opens and slow-provider refresh completions stay responsive; screenshots, video, terminal/live output, or logs are acceptable, and updating the PR body should trigger re-review.

Proof guidance:

  • [P1] Needs real behavior proof before merge: The PR body lists test commands but has no inspectable screenshot, recording, terminal output, live logs, or linked artifact showing the after-fix packaged menu behavior. 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 recording would materially help verify the visible macOS menu responsiveness claim. 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 repeated CodexBar menu opens and slow-provider refresh completions stay responsive while the tracked menu remains open.

Risk before merge

  • [P1] The central runtime claim is performance/responsiveness under a packaged macOS menu tracking scenario, and the PR still has no inspectable after-fix proof for that behavior.
  • [P1] I did not run live provider or packaged-app validation during this read-only review; source inspection and the PR body support the patch, but real behavior proof remains the merge gate.

Maintainer options:

  1. Decide the mitigation before merge
    Land the focused defer-rebuild implementation after inspectable packaged-app proof shows slow-provider refresh completions no longer stall the tracked menu.
  2. Pause or close
    Do not merge this PR until maintainers decide whether the risk is worth taking.

Next step before merge

  • [P1] The PR needs contributor- or maintainer-provided real behavior proof; no narrow automated code repair is currently indicated.

Security
Cleared: The diff only touches menu-controller Swift code, focused tests, and a release-note line; no security or supply-chain concern was found.

Review details

Best possible solution:

Land the focused defer-rebuild implementation after inspectable packaged-app proof shows slow-provider refresh completions no longer stall the tracked menu.

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

No high-confidence live reproduction was established in this read-only review. Current main source shows the direct open-menu rebuild path and the PR adds regression tests for deferral, but the reported WindowServer stall still needs packaged-app proof.

Is this the best way to solve the issue?

Yes, with proof pending. The patch uses existing invalidation and provider-switcher rebuild seams, keeps hosted-submenu/provider switching immediate, and no narrower source-level fix was apparent.

AGENTS.md: found and applied where relevant.

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

Label changes

Label justifications:

  • P1: The PR targets a user-facing multi-second menu/WindowServer stall in the core menu-bar workflow.
  • rating: 🧂 unranked krab: Overall readiness is 🧂 unranked krab; proof is 🧂 unranked krab and patch quality is 🐚 platinum hermit.
  • status: 📣 needs proof: The PR needs real behavior proof before ClawSweeper can clear the contributor ask. Needs real behavior proof before merge: The PR body lists test commands but has no inspectable screenshot, recording, terminal output, live logs, or linked artifact showing the after-fix packaged menu behavior. 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:

  • Repository policy read: AGENTS.md was read fully; its menu-test guidance and no-live-provider-probe guidance affected the review approach. (AGENTS.md:1, 20004f3d2e81)
  • Current main behavior: On current main, refreshOpenMenuIfStillVisible schedules an open-menu rebuild directly for visible menus, which is the path this PR changes for parent menus. (Sources/CodexBar/StatusItemController+MenuTracking.swift:242, 20004f3d2e81)
  • PR diff shape: The head commit changes 7 files with 71 insertions and 20 deletions, centered on menu invalidation/navigation and focused tests. (11117f4fa02f)
  • Deferred parent rebuild implementation: The PR head routes non-hosted visible parent-menu refreshes through invalidateMenus with parent rebuild deferral and stale-content allowance. (Sources/CodexBar/StatusItemController+MenuTracking.swift:242, 11117f4fa02f)
  • Focused regression tests: The PR head updates open-menu refresh tests to assert parent rebuilds stay deferred during tracking and adds switcher keyboard coverage while refresh is in flight. (Tests/CodexBarTests/StatusMenuOpenRefreshTests.swift:631, 11117f4fa02f)
  • Proof still missing: The PR body lists make check and Swift test validation, but no screenshot, recording, terminal output, logs, or linked artifact showing after-fix packaged-app responsiveness. (11117f4fa02f)

Likely related people:

  • Peter Steinberger: Current main blame and local log for the menu tracking/provider-switcher paths point to commit 920997c; the PR head commit was also authored by Peter with external co-authorship. (role: recent area contributor; confidence: medium; commits: 920997c6a365, 11117f4fa02f; files: Sources/CodexBar/StatusItemController+MenuTracking.swift, Sources/CodexBar/StatusItemController+ProviderSwitcher.swift, Sources/CodexBar/StatusItemController+ProviderNavigation.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. P1 Urgent regression or broken agent/channel workflow affecting real users now. labels Jun 10, 2026
Defer parent-menu recomposition caused by provider data refreshes until menu tracking ends. Keep explicit provider switching and hosted submenu updates immediate.

Co-authored-by: isaacJang <dltkr717@gmail.com>
@steipete
steipete force-pushed the fix/grok-menu-beachball branch from 279518b to 11117f4 Compare June 10, 2026 07:04
@steipete steipete changed the title fix: stop menu beachball when Grok provider is enabled Defer tracked menu data-refresh rebuilds Jun 10, 2026

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

Copy link
Copy Markdown

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: 11117f4fa0

ℹ️ 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 +249 to +252
self.invalidateMenus(
refreshOpenMenus: true,
deferOpenParentMenuRebuild: true,
allowStaleContentDuringDataRefresh: true)

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 Refresh the menu after account-switch fetches finish

When a user switches a token/Codex account while the menu is open, the callers in StatusItemController+Menu.swift await the provider/account refresh and then call refreshOpenMenuIfStillVisible to update the visible menu with the newly fetched account-scoped data. This path now only invalidates with deferOpenParentMenuRebuild: true, which refreshOpenMenuIfNeeded handles by recording a deferral and returning, so the open parent menu can stay on the stale/intermediate account state until the user closes and reopens it instead of showing the completed refresh result.

Useful? React with 👍 / 👎.

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

Labels

P1 Urgent regression or broken agent/channel workflow affecting real users now. 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.

2 participants