feat(codex): show the credit limit on credit-metered ChatGPT workspaces - #833
Conversation
iamtoruk
left a comment
There was a problem hiding this comment.
Thanks for this — it's a thorough contribution, and the testing notes (including confirming the pre-existing cli-durable-totals failure on main — that's a known time-of-day flake, fix inbound with the next batch) made review much easier. I ran the suites locally since fork PRs don't get CI here: app 453/453, swift-testing 120/120, builds and typechecks green, and the security-sensitive surfaces (endpoints, token handling, request count) all check out clean.
Three changes needed before merge — the first two are one-liners:
1. Unpinned NumberFormatter locale fails XCTest on non-US-grouping locales — mac/Sources/CodeBurnMenubar/Data/CodexUsage.swift:99 (CreditLimit.displayLabel) builds a bare NumberFormatter(). On a machine whose locale groups with periods this renders Monthly usage limit · 12.000 / 10.000 credits and testPercentOnlyOverageKeepsTheImpliedCount fails (CodexPlanParsingTests.swift:182) — XCTest came out 65/66 here. Fix is the repo's existing convention (8 sites, including CodexSubscriptionService.swift:469 in this same PR's file set):
formatter.locale = Locale(identifier: "en_US")Note en_US_POSIX is the wrong pin for this one — it drops grouping entirely (12000).
2. Same missing pin in the new credits-footer branch — mac/Sources/CodeBurnMenubar/AppStore.swift:1397, the inCredits ? .decimal branch, renders 3.410 instead of 3,410 on the same locales, and no test exercises it. Same one-line fix, plus a test would be great. (The TS side hardcodes toLocaleString('en-US'), and your own doc note says the two implementations should stay in sync.)
3. The "next expires" caption silently disappears for existing users — inlineResetCredits in CodexSubscriptionService.swift returns ResetCredits(availableCount:, nextExpiresAt: nil) whenever the usage payload carries rate_limit_reset_credits.available_count, and the companion request is then skipped. HeatmapSection.resetCreditsLabel degrades "3 available · next expires in 2 days" to "3 available" — and that row only renders when availableCount > 0, i.e. exactly the users who lose the information. Suggestion that keeps the saved request on the common path: take the inline shortcut only when available_count == 0, and fall through to the existing fetch otherwise.
Two non-blocking nits, take or leave:
- The comment on
spendControlWindow("a garbage value is not null, so??would stop at it") overstates the code slightly —findstops at the first object-typed candidate, so an object-shaped garbage alias (e.g.individual_limit: { limit: "abc" }in the first position) still returns null. Swift behaves identically so parity holds; only the comment misleads. The test covers a string garbage alias, which is the casefinddoes skip. has_creditssemantics are documented from a workspace where it'sfalse. If a seat-based account ever reportshas_credits: truenext to a dollar balance, the footer drops the$and rounds to whole units — worth a defensive thought, though blast radius is small.
Happy to re-review as soon as the three fixes land.
Signed-off-by: Richard Boisvert <rboisvert@devolutions.net>
a81d4a8 to
3e400bf
Compare
|
Thanks for the detailed review, and for running the suites locally given fork PRs don't get CI here. All three are in, pushed as an amend to the same commit. 1. Locale pin — 2. Credits footer — same pin at Added 3. Reset credits — implemented as you suggested. New Nit 1 — corrected. The comment now says object-shaped garbage still wins the position and that Swift commits to the first candidate that decodes, rather than implying Nit 2 — I looked at this and decided against a logic guard, because the payload argues the opposite way. My workspace is credit-metered (it has a Suites after the changes: XCTest 67/67 (was 66, +1 for the shortcut test), swift-testing 125/125 (was 120, +5 for the new suite), app 453/453, typecheck clean. Also, good to hear |
|
small poke @iamtoruk |
…menubar The desktop dollar-balance footer used toFixed, which drops thousands separators, while the new menubar footer groups via a currency NumberFormatter, so the two clients showed $12500.00 vs $12,500.00 for the same balance. The menubar credit footer also rounded half-even while the desktop uses Math.round (half-up), disagreeing on exact-half values. Render the desktop dollar path through the en_US currency locale, and pin the menubar footer formatter to half-up. Adds a grouping assertion (TS) and an exact-half rounding test (Swift).
iamtoruk
left a comment
There was a problem hiding this comment.
Re-reviewed on 53ecb36. All three prior blockers are fixed and verified: both NumberFormatter locale pins (CodexUsage.swift, AppStore.swift) and the reset-credits inline shortcut now falls through to the fetch unless available_count == 0.
An adversarial pass on the credit/dollar footer surfaced two remaining desktop/menubar divergences: the desktop dollar path used toFixed (no thousands separators) while the menubar grouped via a currency formatter ($12500.00 vs $12,500.00 for the same balance), and the menubar credit footer rounded half-even while the desktop uses Math.round (half-up). I pushed a fix (75a0285) aligning both: the desktop dollar path now renders through the en_US currency locale, and the menubar footer formatter pins half-up. Added a grouping assertion (TS) and an exact-half rounding test (Swift).
Ran locally (fork PRs get no CI here): app npm test 453/453, swift test 126/126 incl. all XCTest suites at 0 failures, tsc clean. Merged main and resolved the CHANGELOG conflict. Good to merge.
…he all-provider view buildDurablePeriod derived the today slice of the multi-day, all-provider headline from the unsliced whole-range parse, so a turn spanning local midnight kept its category and turn count anchored on its yesterday start. The per-call cost and calls bucketed onto today correctly, but By Activity and the JSON daily turn count lost the post-midnight half — categories summed to only the pre-midnight cost while the headline, By Model and By Project were right. Slice the today parse with filterProjectsByDays first, which re-anchors the straddling turn to its surviving today calls, so today's category cost lands on today. Category cost is the sum of the slice's own calls, so day-N + day-N+1 still equals the whole-range total (no over-count); the per-day turn-count split matches the cache side and the documented per-day semantics. Adds a regression test in the straddling-turn conservation suite (mutation-checked: fails on the pre-fix code). Also fills in the CHANGELOG Unreleased entries for the batch (#853, #856, #872, #846/#859, #866/#867, #833).
Summary
rate_limit: null, so CodeBurn rendered an empty quota card with a blank menubar percentage and tab badge. Their only limit is the admin-set monthly credit allowance inspend_control.individual_limit, which was never decoded.app/electron/quota/codex.ts(desktop) andCodexSubscriptionService.swift(menubar).$prefix, composite tiers (enterprise_cbp_usage_based) normalize to their base tier, and reset credits come from the inlinerate_limit_reset_creditsblock instead of a second HTTP request.Payload quirks are documented in
docs/providers/codex.md. Notablyplan_typereports"business"even on a true Enterprise workspace, and the accounts endpoints the ChatGPT workspace switcher reads reject the Codex OAuth token with 403 while/wham/usagereturns 200 for the same token, so the displayed label is faithfully what OpenAI returns.Testing
npm testpasses - one pre-existing failure intests/cli-durable-totals.test.tsand 26 jsdom resolution errors, both of which reproduce unmodified onmain. This PR touches no files insrc/ortests/. The suites covering this change are green:npm --prefix app test453/453 andswift test --package-path mac66 + 120.npm run buildsucceedsbefore:

after (did not reconnect claude):

matches my usage:
