Add menu bar pace layout tokens - #2540
Conversation
![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: a5371fac64
ℹ️ 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".
| weeklyPace: self.store.menuBarLayoutPaceText(provider: provider, window: windows.weekly, now: now), | ||
| automaticPace: self.store.menuBarLayoutPaceText( | ||
| provider: provider, | ||
| window: windows.automatic, | ||
| now: now), |
There was a problem hiding this comment.
When Codex historical data is loaded or revised after this render, historicalPaceRevision wakes the icon observer, but providerStoreIconObservationSignature only hashes the legacy display text plus custom cost/account values. With a custom weekly/auto pace token and the legacy display mode left at its default percent setting, the signature is therefore unchanged and updateIcons() is skipped, leaving the token on its earlier linear/stale value until an unrelated icon-affecting change occurs. Include the active layout's pace values (or its historical revision) in that signature.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in dece955 — the active layout's pace values now contribute to the signature the same way cost and account tokens do, gated on the layout containing a pace token. Regression test in StatusItemLayoutPaceSignatureTests was verified to fail before the fix.
clawsweeper
Bot
added
rating: 🧂 unranked krab
|
Codex review: needs maintainer review before merge. Reviewed August 2, 2026, 6:13 PM ET / 22:13 UTC. ClawSweeper reviewWhat this changesAdds signed Session, Weekly, and Auto pace tokens to the menu-bar layout editor, preview, renderer, accessibility output, refresh signature, localized labels, documentation, and regression tests. Merge readinessThe branch is a coherent, tested implementation with direct native menu-bar screenshot proof, and the prior refresh defect has been addressed. It should remain open because merging would establish the broader Session/Weekly/Auto pace-token vocabulary while the canonical request still awaits a maintainer product-direction decision. Priority: P2 Review scores
Verification
How this fits togetherCodexBar converts provider usage snapshots and historical pacing data into a configurable macOS menu-bar status item. Layout tokens select which provider-scoped values are resolved for the editor preview and then rendered in the live status item. flowchart LR
A[Provider usage snapshots] --> B[Historical pace calculation]
C[Saved menu-bar layout] --> D[Token selection]
B --> E[Provider-scoped render data]
D --> E
E --> F[Layout editor preview]
E --> G[Menu-bar status item]
Decision needed
Why: The implementation is technically coherent and proof-positive, but the number of supported pace variants is a durable user-facing layout vocabulary decision that code review cannot infer from the existing contract. Before merge
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: Explicitly approve the full Session/Weekly/Auto family if matching the existing percentage-token vocabulary is desired, then merge this focused implementation; otherwise ask for a narrow session-only revision before merge. Do we have a high-confidence way to reproduce the issue? Not applicable: this PR adds a new layout capability rather than repairing a reported failing current-main runtime path. The provided screenshots nevertheless demonstrate the intended live behavior after the change. Is this the best way to solve the issue? Unclear pending product direction: the implementation is a narrow, maintainable way to add pace tokens, but a maintainer must first choose whether all three window variants are the intended supported vocabulary. AGENTS.md: found and applied where relevant. Codex review notes: model internal, reasoning high; reviewed against 5b0b9fa3e37c. LabelsLabel justifications:
EvidenceWhat I checked:
Likely related people:
Rank-up movesOptional improvements that raise the rating; they are not merge blockers.
Rating scale
Overall follows the weaker of proof and patch quality. Workflow
HistoryReview history (4 earlier review cycles)
|
clawsweeper
Bot
removed
the
merge-risk: 🚨 other
clawsweeper
Bot
added
proof: sufficient
The 0.45 layout editor replaced the old Percent/Pace/Both display modes, but no token exposes the signed pace delta the Both mode used to render. `Runs out` is not a substitute: it answers when a window ends, always estimating from the weekly (or automatic) lane, while pace answers how far off the sustainable rate usage currently runs. Add Session/Weekly/Auto pace tokens that mirror the percent tokens' window selection and reuse the existing `MenuBarDisplayText.paceText` formatting (`+11%` ahead of the rate, `-8%` behind, `0%` on pace). Each token resolves pace for its own window, so Weekly pace never borrows the session delta. Pace needs the store's historical dataset and work-day setting, so it is resolved upstream like `runsOut` through a shared `menuBarLayoutPaceText` helper that both the status item and the editor preview call. The existing 3% expected-usage floor in `weeklyPace` still applies, so a token renders the en-dash placeholder early in a window while its siblings stay visible. Refs steipete#2534
Pace values change with the historical dataset, the work-day setting, and the clock, none of which move the percent fields already hashed by providerStoreIconObservationSignature. A historicalPaceRevision bump therefore woke the icon observer but produced an unchanged signature, so updateIcons() was skipped and a custom pace token kept its stale value until an unrelated icon change forced a redraw. Contribute the active layout's pace values to the signature the same way cost and account tokens already do, gated on the layout actually containing a pace token. The regression test renders two snapshots with identical used percents but different resets: without this fix both signatures were identical. Refs steipete#2534
dece955 to
c115ae0
Compare
2 participants
Summary
Adds
Session pace,Weekly pace, andAuto pacelayout tokens so the menu bar can show the signed pace delta again, which the layout editor has had no token for since it replaced the Percent/Pace/Both display modes.Runs outdoes not cover this. It answers when does this window end, andStatusItemController+MenuBarLayout.swiftalways derives it from the weekly (or automatic) lane. Pace answers how far off the sustainable rate am I right now, and it is meaningful per window.Sign convention
Positive is deficit, negative is reserve — the same direction
MenuBarDisplayText.paceTexthas always used, so+11%means usage runs 11 points ahead of the even rate and-8%means 8 points behind it, with0%shown on pace. This mirrors the menu card's "in deficit" / "in reserve" wording in the compact form the status item has room for.Scope
The Codex review comment on #2534 recommended a session-only token with weekly and automatic deferred. I went with all three windows instead, for two reasons: the token then mirrors the existing percent tokens exactly rather than introducing a second, narrower window vocabulary in the same palette group, and weekly pace is the window I actually needed — a 7-day quota is where being ahead of the rate costs you something, since a 5-hour window refills on its own. That said, the extra windows are two enum cases and two label strings; happy to cut it down to session-only if that is the direction you prefer.
Implementation notes
UsageStore.menuBarLayoutPaceTexthelper, so the status item and the layout editor preview both apply the same historical-dataset and work-day settings the menu card uses. Precomputing upstream matches howrunsOutalready works — the renderer has no access to the store.Weekly pacenever falls back to the session delta.weeklyPacestill gates the token, so it renders the standard en-dash placeholder early in a window while its siblings keep rendering.UsagePace.weeklycalculation, so the preview is deterministic before any snapshot exists.enplus all 22 complete locale catalogs;Scripts/check-app-locales.mjspasses.Possible follow-up (not in this PR)
MenuBarLayout.migratedmaps the legacy.pacedisplay mode to therunsOuttoken and.bothto percent +runsOut. SincerunsOutis an ETA rather than a delta, installs that upgraded without ever opening the layout editor silently changed what their status item means. With this token in place that migration could map to.pace(window:)instead. I left it out because it changes behavior for existing installs and seemed like a separate decision from adding the token.Commands run
swift buildmake test(full sharded suite, exit 0)make check(0 violations)node Scripts/check-app-locales.mjsProof
Menu bar rendering from a release build of this branch, stored layout
Icon · Weekly % · Separator · Weekly pace, live provider data:Three providers side by side, one per pace state:
Claude renders
41% · +13%(13 points ahead of the even rate), AntigravityW 20% · -22%(in reserve), and CodexW 0% · –— its weekly window had just reset, so pace sits below the existing 3% expected-usage floor and the token renders the en-dash placeholder while its siblings keep rendering.Review follow-up
The Codex review correctly flagged that the icon observation signature did not cover pace values, so a
historicalPaceRevisionbump could wake the observer yet skipupdateIcons(), leaving a custom pace token stale. Fixed in dece955: the active layout's pace values now contribute toproviderStoreIconObservationSignaturethe same way cost and account tokens already do, gated on the layout actually containing a pace token. The regression test (StatusItemLayoutPaceSignatureTests) renders two snapshots with identical used percents but different resets; it was verified to fail on a5371fa (both signatures identical) and passes with the fix. Focused sweep after the fix: 381 tests across the 31 status-item, layout, and pace suites, plusmake checkwith zero violations.Note on labels
I saw the
clawsweeper:no-new-fix-prandclawsweeper:needs-product-decisionlabels on #2534. Opening this anyway as a concrete proposal to react to rather than as something expected to merge as-is — close it without ceremony if the product call goes another way.Refs #2534