Harden CodexBar cold-start menu readiness#1150
Conversation
|
@steipete I could not request you through GitHub's reviewer API from this account, so tagging you here for review. |
|
Codex review: needs maintainer review before merge. Reviewed May 29, 2026, 6:10 PM ET / 22:10 UTC. Summary Reproducibility: no. this read-only review did not run a local cold-boot/AppKit bundle reproduction. The PR body provides credible packaged-app reboot proof, and source/tests support the cold-start readiness path, but exact first-after-boot behavior remains macOS-runtime-dependent. Review metrics: 2 noteworthy metrics.
Merge readiness Overall follows the weaker of proof and patch quality, so missing proof can cap an otherwise strong patch. Rank-up moves:
Mantis proof suggestion Risk before merge
Maintainer options:
Next step before merge
Security Review detailsBest possible solution: Land the menu-readiness hardening after maintainers either accept the oldest-process duplicate-launch policy and validation harness as supported behavior or narrow those parts before merge. Do we have a high-confidence way to reproduce the issue? No; this read-only review did not run a local cold-boot/AppKit bundle reproduction. The PR body provides credible packaged-app reboot proof, and source/tests support the cold-start readiness path, but exact first-after-boot behavior remains macOS-runtime-dependent. Is this the best way to solve the issue? Unclear as a final product decision. The menu-readiness implementation is focused and covered by tests, but the duplicate-launch policy and LaunchAgent/Login Item harness need maintainer acceptance or narrowing before merge. AGENTS.md: found and applied where relevant. Codex review notes: model gpt-5.5, reasoning high; reviewed against 01528f5ee6eb. Label changesLabel justifications:
Evidence reviewedWhat I checked:
Likely related people:
What the crustacean ranks mean
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
|
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ee755640c
ℹ️ 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".
| try register() | ||
| } else { | ||
| try service.unregister() | ||
| guard status() == .enabled else { return } |
There was a problem hiding this comment.
Treating setEnabled(false) as a no-op unless status() == .enabled leaves the login item registered when Service Management reports .requiresApproval. In that state the service is already registered but awaiting user approval, so users who toggle Launch at Login off will not actually remove it and can still see the pending/background item in System Settings on next login. This regression was introduced by the new status guard and should also call unregister() for .requiresApproval (or for any state except .notRegistered).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 31a37207: setEnabled(false) now calls unregister() for any ServiceManagement state except .notRegistered, so pending .requiresApproval login items are removed instead of left registered. Added a focused regression test for the .requiresApproval disable path and verified with swift test --filter LaunchAtLoginManagerTests plus git diff --check.
clawsweeper
Bot
added
proof: sufficient
|
Fixed the hosted submenu provider-identity regression in What changed:
Verification run:
Duplicate-launch note: I left @clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
AmrMohamad
force-pushed
the
enhance/UI-performance-issues
branch
from
63f3689 to
351dd19
Compare
clawsweeper
Bot
added
rating: 🐚 platinum hermit
AmrMohamad
force-pushed
the
enhance/UI-performance-issues
branch
from
351dd19 to
090937e
Compare
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
|
Thanks @AmrMohamad. I could not update this PR branch directly because maintainer edits are disabled, so I extracted the low-risk cold-start menu readiness subset and landed it in #1231 / d7db992. The landed change refreshes open Codex parent and hosted adjunct menus when dashboard, credits, token-cost, and plan-history data become ready after launch, and adds cached Codex token hydration with root-fingerprint validation and cached Pi session merging. I left the duplicate launch guards, launch validation harnesses, keychain/login-item changes, docs churn, and broad cleanup out because they need separate review and proof. Please enable Allow edits by maintainers and split those pieces into smaller PRs if you want to continue them. |
2 participants
Summary
This PR hardens CodexBar’s first-launch menu readiness path after macOS boot. The issue we investigated was an unstable first-open Codex tab where expensive adjunct UI, especially charts and hosted submenu content, could remain absent until the user manually refreshed or switched tabs.
The core fix is not a visual redesign. It keeps the UI shape intact and improves the lifecycle behind it: menu readiness invalidation, hosted submenu hydration, cached token data availability, duplicate-launch protection, and cold-start validation.
Why this changed
CodexBar’s menu is an AppKit
NSStatusItem/NSMenusurface with SwiftUI-hosted rows and lazily hydrated submenus. On first launch after boot, several data sources can become ready after the menu has already been opened:Before this change, the menu invalidation path was too coarse for those adjunct readiness transitions. A parent menu could remain open with stale structure, and a hosted submenu could stay stuck on its placeholder even after the backing data became available.
What changed
Validation
Source and focused behavior checks:
swift test --filter StatusMenuHostedSubmenuRefreshTestsswift test --filter StatusMenuOpenRefreshTestsswift test --filter LaunchAtLoginManagerTestsmake checkgit diff --checkRuntime proof:
.build/package/CodexBar.appcodesign --verify --deep --strict --verbose=2 .build/package/CodexBar.appCold-start artifact:
/Users/amrmohamad/.codex/artifacts/CodexBar/cold-start-20260526-025603Key artifact evidence:
post_boot_first_launch_candidate=trueexisting_codexbar_process_count=0codexbar_login_item_present=falseat runner startuptime_seconds=319completeBuy Credits...,Cost | submenu=true,Usage Dashboard,Status Page, andRefreshCost | submenu=trueandCost submenu item count=1Scripts/prepare_next_boot_cold_start_validation.sh check-resultpassed against the captured artifactNotes
The LaunchAgent’s original summary recorded an older checker failure because the validator used the stdlib PNG decoder fallback instead of Pillow metadata. The artifact itself is valid and immutable; after correcting the checker to accept
image_decoder=stdlib_png, the same first-boot artifact validates successfully.This PR intentionally does not change the visible UI design. The changes are lifecycle, data-readiness, validation, and performance hardening around the existing menu hierarchy.