Fix status item menu bar observation for token account snapshots (#2283) - #2511
Fix status item menu bar observation for token account snapshots (#2283)#2511Yuxin-Qiao wants to merge 3 commits into
Conversation
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs real behavior proof before merge. Reviewed July 30, 2026, 11:27 AM ET / 15:27 UTC. ClawSweeper reviewWhat this changesThe branch adds account-snapshot dependencies to CodexBar’s menu-bar icon observation token and icon work signature, plus Swift Observation tests for snapshot-collection invalidation. Merge readiness⛔ Blocked until real behavior proof is added - 7 items remain Keep this PR open: its current signature still ignores mutable usage, error, and source fields when an existing account snapshot is refreshed, so the menu-bar icon can remain stale in the reported scenario. It also needs redacted after-fix runtime proof from a freshly built CodexBar bundle. Likely related people: steipete is a low-confidence routing candidate for this menu-bar ownership path. Priority: P2 Review scores
Verification
How this fits togetherCodexBar refreshes provider usage into flowchart LR
A[Provider refresh] --> B[Usage and account snapshots]
B --> C[Usage store observation token]
C --> D[Status item work signature]
D --> E{Signature changed?}
E -->|Yes| F[Redraw menu-bar icon]
E -->|No| G[Keep existing icon]
Before merge
Findings
Agent review detailsSecurityNone. Review metrics
Root-cause clusterRelationship: Members:
Proposal only: this assessment does not dispatch repair, suppress jobs, mutate sibling items, close, or merge anything. Merge-risk optionsMaintainer options:
Technical reviewBest possible solution: Make the icon work signature depend on all snapshot state that can change the rendered icon, add a same-account replacement regression test, then attach redacted after-fix proof from a freshly built bundle showing the icon and opened menu converge after refresh. Do we have a high-confidence way to reproduce the issue? Yes, from source: replace an existing ordinary or Codex account snapshot while preserving its ID and displayed identity fields but changing usage, error, or source data. Observation fires, yet the newly added projection remains equal and the controller can skip icon redraw work. Is this the best way to solve the issue? No; reading the collections is necessary, but projecting only stable identity fields does not cover the mutable snapshot state that the reported icon should reflect. A complete render-state projection or an explicit revision that changes on every icon-relevant snapshot update is the narrower reliable fix. Full review comments:
Overall correctness: patch is incorrect AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 8ef86077e70a. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (3 earlier review cycles)
|
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
![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: ce475ecc1b
ℹ️ 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".
| _ = self.accountSnapshots | ||
| _ = self.codexAccountSnapshots |
There was a problem hiding this comment.
Once the initial icon signature is stored, a refresh that changes only either per-account collection triggers this observation but still does no icon work: observeStoreIconChanges() recomputes storeIconObservationSignature() and returns because that signature reads store.snapshot(for:) and never accountSnapshots or codexAccountSnapshots. The status indicator therefore remains stale in the scenario these dependencies are intended to fix; include the relevant account state in the signature/render source (or publish it into snapshots) and test through the controller's signature gate rather than only asserting raw Observation invalidation.
AGENTS.md reference: AGENTS.md:L4-L5
Useful? React with 👍 / 👎.
…status item updates (steipete#2283)
|
@clawsweeper re-review |
|
🦞🧹 I asked ClawSweeper to review this item again. Re-review progress:
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b7e280360
ℹ️ 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".
| let accountPart = accountSnapshots.map { snap in | ||
| let label = snap.account.label | ||
| return "\(snap.account.id.uuidString):\(label)" |
There was a problem hiding this comment.
The new signature projection is fresh evidence that the previously reported gate remains incomplete: an ordinary refresh replaces an existing TokenAccountUsageSnapshot with updated snapshot, error, or sourceLabel values while preserving its ID and label, so Observation fires but this string remains unchanged and observeStoreIconChanges() returns without updating the status item. The Codex projection has the same problem because it retains only ID, email, and active state; include the mutable fields that are intended to trigger icon work and cover a same-account refresh rather than only insertion into an empty collection.
Useful? React with 👍 / 👎.
|
@clawsweeper re-review Here is the technical summary and evidence for PR #2511: Problem & Fix Summary
Test Coverage & Evidence
|
clawsweeper
Bot
added
the
merge-risk: 🚨 other
1 participant
Summary
When account snapshots (
accountSnapshotsorcodexAccountSnapshots) were updated, the status item menu bar observation token (iconObservationToken) inUsageStorewas missing dependencies on these collections. This caused the status item menu bar icon to remain stale until another tracked property was invalidated.This PR adds
accountSnapshotsandcodexAccountSnapshotsobservation reads toiconObservationTokeninUsageStore.swift.Changes Made
_ = self.accountSnapshotsand_ = self.codexAccountSnapshotstoiconObservationToken.withObservationTracking) invalidation unit tests verifying that mutatingaccountSnapshotsorcodexAccountSnapshotsinvalidatesiconObservationToken.Verification
swift test --filter StatusItemIconObservationSignatureTests(22/22 passed).make check(0 format/lint violations).Fixes #2283