Repository navigation
feat(desktop): add multi-account AI usage meter - #5844
Closed
AlchemistChaos wants to merge 12 commits into
Closed
AlchemistChaos wants to merge 12 commits into
AlchemistChaos wants to merge 12 commits into
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds Claude and Codex usage collection with validation, caching, tRPC exposure, Codex profile management, persistent visibility settings, and dashboard indicators for capacity, reset times, and provider status. ChangesProvider usage visibility
Estimated code review effort: 5 (Critical) | ~90 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant AIUsageIndicator
participant providerUsage
participant collectProviderUsage
participant Claude
participant CodexAppServer
User->>AIUsageIndicator: Open usage popover
AIUsageIndicator->>providerUsage: getSnapshot(force)
providerUsage->>collectProviderUsage: Collect provider snapshot
collectProviderUsage->>Claude: Fetch OAuth usage
collectProviderUsage->>CodexAppServer: Read rate limits
Claude-->>collectProviderUsage: Claude usage windows
CodexAppServer-->>collectProviderUsage: Codex rate limits
collectProviderUsage-->>providerUsage: Cached snapshot
providerUsage-->>AIUsageIndicator: Validated snapshot
AIUsageIndicator-->>User: Capacity and provider rows
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 164ab093b8
ℹ️ 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
+24
to
+25
| const child = spawn("codex", ["app-server", "--stdio"], { | ||
| env: process.env, |
There was a problem hiding this comment.
When the packaged desktop app is launched from Finder/Dock on macOS, process.env.PATH is usually the minimal GUI PATH, and the startup applyShellEnvToProcess() merge does not replace an existing PATH. This means users who have codex installed through Homebrew, npm/nvm, asdf, etc. will be reported as not configured even though codex app-server works in their terminal; the existing getProcessEnvWithShellPath/execWithShellEnv helpers are the path used elsewhere for child processes that must resolve terminal-installed CLIs.
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.test.tsx (1)
39-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd coverage for the "unavailable" status branch.
Tests cover "ok" and "not-configured" but not "unavailable", which renders
provider.errorMessageinstead of the sign-in copy — a distinct, currently untested code path.✅ Suggested additional test
test("renders the error message when usage is temporarily unavailable", () => { const markup = renderToStaticMarkup( <ProviderUsageRow provider={{ ...claudeProvider, status: "unavailable", windows: [], errorMessage: "Claude usage is temporarily unavailable.", }} />, ); expect(markup).toContain("Claude usage is temporarily unavailable."); expect(markup).not.toContain('role="progressbar"'); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.test.tsx` around lines 39 - 53, Add a test covering the unavailable status branch in ProviderUsageRow tests, constructing the provider with status "unavailable" and a representative errorMessage. Assert the rendered markup contains that error message and does not contain a progressbar role, while preserving the existing ok and not-configured coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/lib/provider-usage/providers/claude.ts`:
- Around line 18-21: Update usageBucketSchema to accept numeric as well as
string or nullish resets_at values, then update parseResetAt to normalize
numeric timestamps into the existing reset-time representation. Preserve current
handling for strings, null, and undefined, ensuring numeric endpoint values do
not invalidate the usage bucket.
---
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.test.tsx`:
- Around line 39-53: Add a test covering the unavailable status branch in
ProviderUsageRow tests, constructing the provider with status "unavailable" and
a representative errorMessage. Assert the rendered markup contains that error
message and does not contain a progressbar role, while preserving the existing
ok and not-configured coverage.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 44ec9e76-5dfc-40e6-9843-a579fa4ff47c
📒 Files selected for processing (20)
apps/desktop/src/lib/trpc/routers/index.tsapps/desktop/src/lib/trpc/routers/provider-usage.schema.tsapps/desktop/src/lib/trpc/routers/provider-usage.test.tsapps/desktop/src/lib/trpc/routers/provider-usage.tsapps/desktop/src/main/lib/provider-usage/index.test.tsapps/desktop/src/main/lib/provider-usage/index.tsapps/desktop/src/main/lib/provider-usage/providers/claude.test.tsapps/desktop/src/main/lib/provider-usage/providers/claude.tsapps/desktop/src/main/lib/provider-usage/providers/codex-app-server.test.tsapps/desktop/src/main/lib/provider-usage/providers/codex-app-server.tsapps/desktop/src/main/lib/provider-usage/providers/codex.test.tsapps/desktop/src/main/lib/provider-usage/providers/codex.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/TopBar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/AIUsageIndicator.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.test.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.ts
AlchemistChaos
changed the title
Contributor
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (16)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.ts (2)
76-85: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
findWindowByMinutesto match its unit.The parameter is
windowSecondsand both call sites pass seconds (5 * 60 * 60and7 * 24 * 60 * 60). The name states minutes. Rename it tofindWindowBySeconds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.ts` around lines 76 - 85, Rename the function findWindowByMinutes to findWindowBySeconds and update both call sites to use the new name, preserving the existing windowSeconds matching behavior.
142-151: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCompute the lowest percentage from structured values, not from formatted label strings.
getLowestSelectedRemainingPercentre-parses the numeric value out of the display strings produced bygetMenuBarSummaryParts. Any future change to the label format, for example adding a%suffix or a different separator, silently breaks this calculation. Extract the selection logic into a helper that returns{ label, value }pairs, then derive both the summary strings and the minimum from it.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.ts` around lines 142 - 151, Refactor getMenuBarSummaryParts to use a helper that returns structured { label, value } pairs for the selected providers and metrics. Build the displayed summary strings from the pair labels, and update getLowestSelectedRemainingPercent to compute the minimum directly from the pair values instead of parsing formatted strings, preserving the existing fallback when no finite values exist.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.test.tsx (1)
121-158: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a negative case for the Codex-only Switch button.
ProviderUsageRow.tsxline 169 renders the Switch button only whenprovider.providerId === "codex". This test adds an inactive Claude account but never asserts that the button is absent, so the provider guard is untested. Addexpect(markup).not.toContain("Switch")to this test.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.test.tsx` around lines 121 - 158, The test “renders inactive cached accounts separately from the active account” should also verify that Claude providers do not render the Codex-only Switch button. Add a negative assertion against “Switch” using the existing markup result, while preserving the current account and inactive-state assertions.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/AIUsageIndicator.tsx (1)
77-91: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the redundant fallback and reuse the derived
providersvalue.
getLowestSelectedRemainingPercentalready falls back togetLowestRemainingPercent(providers)when no metric is selected (usageIndicatorPolicy.tsline 149). The??branch on line 80 is therefore unreachable. Line 229 also re-readsdata?.providersalthough line 77 already derivesproviders.♻️ Proposed cleanup
const providers = data?.providers ?? []; - const remaining = - getLowestSelectedRemainingPercent(providers, selection) ?? - getLowestRemainingPercent(providers); + const remaining = getLowestSelectedRemainingPercent(providers, selection);- {data?.providers.map((provider) => ( + {providers.map((provider) => (Then drop
getLowestRemainingPercentfrom the import list on line 16.Also applies to: 229-229
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/AIUsageIndicator.tsx` around lines 77 - 91, In AIUsageIndicator, simplify the remaining calculation to use getLowestSelectedRemainingPercent(providers, selection) directly, removing the unreachable getLowestRemainingPercent fallback and its import. At the later usage around the component’s line 229, reuse the already-derived providers value instead of reading data?.providers again.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.tsx (2)
61-78: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueSimplify the synthesized default account and check
planLabel.The whole object literal is already inside the
provider.status === "ok"branch, soisActive: provider.status === "ok"on line 72 and the ternary on line 73 always evaluate totrueand"ok". Line 71 also assignsprovider.accountLabeltoplanLabel, which duplicates the account label into the plan field.planLabelis not rendered today, so there is no visible defect, but the value is wrong if a future row displays it.♻️ Proposed cleanup
{ id: `${provider.providerId}:default`, providerId: provider.providerId, profileName: "default", accountLabel: provider.accountLabel, - planLabel: provider.accountLabel, - isActive: provider.status === "ok", - status: provider.status === "ok" ? "ok" : "no-data", + planLabel: null, + isActive: true, + status: "ok", statusMessage: provider.errorMessage, windows: provider.windows, } satisfies ProviderUsageAccount,🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.tsx` around lines 61 - 78, In the synthesized default account within ProviderUsageRow, simplify the status fields to their branch-invariant values: set isActive to true and status to "ok". Do not copy provider.accountLabel into planLabel; use the appropriate absent/default plan value while preserving the existing accountLabel and other account properties.
139-224: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTwo files each declare a second React component. The coding guidelines require one component per file, organized as
ComponentName/ComponentName.tsxwith anindex.tsbarrel export. Both new files declare a helper component alongside the exported one.
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.tsx#L139-L224: moveAccountUsageBlockintocomponents/AccountUsageBlock/AccountUsageBlock.tsxwith anindex.tsbarrel export, co-located underProviderUsageRowsince it is its only consumer.apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/AIUsageIndicator.tsx#L267-L290: moveMetricToggleintocomponents/MetricToggle/MetricToggle.tsxwith anindex.tsbarrel export, co-located underAIUsageIndicator.As per coding guidelines: "Use one component per file and organize components as
ComponentName/ComponentName.tsxwith anindex.tsbarrel export" and "Co-locate components, utilities, hooks, constants, configuration, tests, and stories with the code that uses them."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.tsx` around lines 139 - 224, Move AccountUsageBlock from apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.tsx:139-224 into a co-located components/AccountUsageBlock/AccountUsageBlock.tsx with an index.ts barrel export, and update ProviderUsageRow imports. Move MetricToggle from apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/AIUsageIndicator.tsx:267-290 into components/MetricToggle/MetricToggle.tsx with an index.ts barrel export, and update AIUsageIndicator imports; leave each parent file with only its primary component.Source: Coding guidelines
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.test.ts (1)
118-178: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case where the active account windows differ from the provider windows.
In every fixture the account windows either match
provider.windowsor the accounts array is empty. The precedence rule inactiveWindows(usageIndicatorPolicy.tslines 56-65), where active-account windows win over provider windows with the sameid, is therefore never exercised. Add one provider whose active account reports a differentremainingPercentfor the same windowid, and assert the account value is used.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.test.ts` around lines 118 - 178, Add a test fixture with an active account window sharing an id with a provider window but having a different remainingPercent, then assert the relevant summary or lowest-remaining result uses the account value. Keep the provider window and active account association explicit so activeWindows precedence for the matching id is exercised.apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.test.ts (3)
154-191: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a case for a window with
resetAt: null.
projectCachedWindowskeeps a window whenwindow.resetAt === null. No test covers that branch. The branch decides whether a window with no reset time stays visible forever, which is the opposite of the "no fabricated capacity" intent this test states. Add a third window withresetAt: nulland assert it survives.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.test.ts` around lines 154 - 191, Add a third window with resetAt: null to the snapshot in the projectCachedWindows test, then include that window unchanged in the expected result alongside the non-expired secondary window. This should cover the branch that preserves windows without a reset time.
77-91: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winConsider asserting the file mode on the imported credential.
The test verifies the content of the stored
auth.jsonbut not its permissions.copySensitiveFileAtomicallyappliesSUPERSET_SENSITIVE_FILE_MODE, which is the security property that matters for a stored OAuth token. AstatSync(...).mode & 0o777assertion would prevent a silent regression if thechmodstep is ever dropped.♻️ Proposed addition
+ expect( + statSync( + join( + rootDir, + "provider-usage", + "codex", + "profiles", + "person-example-com", + "auth.json", + ), + ).mode & 0o777, + ).toBe(SUPERSET_SENSITIVE_FILE_MODE);Import
statSyncfromnode:fsandSUPERSET_SENSITIVE_FILE_MODEfrommain/lib/app-environment.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.test.ts` around lines 77 - 91, Extend the credential test around parseCodexIdentity to import statSync and SUPERSET_SENSITIVE_FILE_MODE, then assert the stored auth.json permission bits (statSync(...).mode & 0o777) equal SUPERSET_SENSITIVE_FILE_MODE, alongside the existing accountId assertion.
192-192: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering
putSnapshotandaccountRows.The store exposes two behaviors with real logic and no test:
putSnapshotdrops a write when an existing snapshot has acapturedAtgreater than or equal to the new one. A regression here would silently freeze usage data.accountRowsbuilds the exactProviderUsageAccountcontract that the dashboard renders, including theok/cached/no-datastatus mapping.Both accept the injected
rootDir, so they need no new test scaffolding.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.test.ts` at line 192, Add tests covering the store’s putSnapshot and accountRows behaviors using the existing injected rootDir setup. Verify putSnapshot rejects writes whose capturedAt is less than or equal to the existing snapshot and accepts newer snapshots, then verify accountRows produces the expected ProviderUsageAccount fields and ok/cached/no-data status mapping.apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts (3)
382-409: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRead the snapshots file once, and use
async/awaithere.Two points:
getSnapshotat line 385 runs inside themap, and each call re-reads and re-parses the wholesnapshots.jsonfile. With N profiles this performs N synchronous reads of the same file on the Electron main thread.- This is the only function in the file that uses a
.then()chain. Every other function usesasync/await.♻️ Proposed refactor
- function accountRows(): Promise<ProviderUsageAccount[]> { - return listProfiles().then((profiles) => - profiles.map((profile) => { - const snapshot = getSnapshot(profile.identity.accountId); - const windows = snapshot ? projectCachedWindows(snapshot) : []; + async function accountRows(): Promise<ProviderUsageAccount[]> { + const profiles = await listProfiles(); + const snapshots = readSnapshots(); + return profiles.map((profile) => { + const snapshot = snapshots[profile.identity.accountId] ?? null; + const windows = snapshot ? projectCachedWindows(snapshot) : []; const hasCachedWindows = windows.length > 0; return { id: `codex:${profile.identity.accountId}`, providerId: "codex" as const, profileName: profile.profileName, accountLabel: profile.identity.email, planLabel: snapshot?.planLabel ?? profile.identity.plan, isActive: profile.isActive, status: hasCachedWindows ? profile.isActive ? "ok" : "cached" : "no-data", statusMessage: hasCachedWindows ? profile.isActive ? "live" : "cached" : "Use Codex once to record limits", windows, }; - }), - ); + }); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts` around lines 382 - 409, Refactor accountRows() to async/await and load the snapshots data once before iterating profiles, then resolve each profile’s cached snapshot from that in-memory data instead of calling getSnapshot() inside map. Preserve the existing account-row fields, status logic, and empty-snapshot behavior while matching the file’s established async/await style.
74-96: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueUse the async
renamein these async helpers.Both helpers are async but call
renameSync. This blocks the Electron main thread. Importrenamefromnode:fs/promisesto keep the whole path non-blocking.♻️ Proposed change
-import { - copyFile, - mkdir, - readdir, - readFile, - writeFile, -} from "node:fs/promises"; +import { + copyFile, + mkdir, + readdir, + readFile, + rename, + writeFile, +} from "node:fs/promises";await copyFile(source, tempPath); chmodSensitive(tempPath); - renameSync(tempPath, destination); + await rename(tempPath, destination); chmodSensitive(destination);await writeFile(tempPath, contents, { mode: SUPERSET_SENSITIVE_FILE_MODE, }); chmodSensitive(tempPath); - renameSync(tempPath, destination); + await rename(tempPath, destination); chmodSensitive(destination);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts` around lines 74 - 96, Replace the synchronous renameSync calls in copySensitiveFileAtomically and writeSensitiveFileAtomically with the awaited async rename from node:fs/promises, adding the required import and preserving the existing atomic operation order.
205-212: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRename
CCM_CODEX_EXECUTABLEtoSUPERSET_CODEX_EXECUTABLE, or document why theCCM_prefix is intentional.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts` around lines 205 - 212, Update findCodexExecutable to read SUPERSET_CODEX_EXECUTABLE instead of CCM_CODEX_EXECUTABLE, unless the CCM_ prefix is intentionally required; if retaining it, document that intent near the environment-variable lookup.apps/desktop/src/main/lib/provider-usage/providers/codex.ts (2)
38-42: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider narrowing the
profileStoredependency type.
profileStore?: typeof codexProfileStorerequires the full concrete store shape.collectCodexUsageonly callsactiveIdentity,accountRows, andputSnapshot. The test fake incodex.test.tsmust still stubimportActive,activate,addViaIsolatedLogin,listProfiles,getSnapshot, andactiveAuthPathjust to satisfy the type. Every future store method will force the same churn.Declare only the members this module consumes.
♻️ Proposed change
+interface CodexProfileStoreAccess { + activeIdentity: () => CodexIdentity | null; + accountRows: () => Promise<ProviderUsageAccount[]>; + putSnapshot: (snapshot: CodexUsageSnapshot) => Promise<void>; +} + interface CodexUsageDependencies { readRateLimits?: () => Promise<CodexRateLimitsReadResult>; - profileStore?: typeof codexProfileStore; + profileStore?: CodexProfileStoreAccess; now?: () => number; }Import
CodexUsageSnapshotas a type from./codex-profiles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/lib/provider-usage/providers/codex.ts` around lines 38 - 42, Update the CodexUsageDependencies.profileStore type to a minimal interface containing only activeIdentity, accountRows, and putSnapshot, using the CodexUsageSnapshot type imported from ./codex-profiles as needed. Keep collectCodexUsage behavior unchanged and allow test fakes to implement only these consumed members.
213-292: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider extracting a result builder for the repeated
ProviderUsageliterals.
collectCodexUsagereturns six near-identical object literals. All six repeatproviderId: "codex"andproviderName: "Codex", and most repeat theactive?.accountLabel ?? ...andactive?.windows ?? []expressions. The differences are limited tostatus,accountLabel,windows, anderrorMessage.A small helper would shorten the function and make the branch differences visible at a glance.
♻️ Sketch
+function codexResult( + accounts: ProviderUsageAccount[], + overrides: Partial<ProviderUsage> = {}, +): ProviderUsage { + const active = activeAccountFrom(accounts); + return { + providerId: "codex", + providerName: "Codex", + status: statusFromAccounts(accounts), + accountLabel: active?.accountLabel ?? active?.planLabel ?? null, + activeAccountId: active?.id ?? null, + accounts, + windows: active?.windows ?? [], + errorMessage: null, + ...overrides, + }; +}Each return then narrows to the fields that differ, for example:
- return { - providerId: "codex", - providerName: "Codex", - status: "unavailable", - accountLabel: active?.accountLabel ?? null, - activeAccountId: active?.id ?? null, - accounts, - windows: [], - errorMessage: "Codex usage is temporarily unavailable.", - }; + return codexResult(accounts, { + status: "unavailable", + windows: [], + errorMessage: "Codex usage is temporarily unavailable.", + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/lib/provider-usage/providers/codex.ts` around lines 213 - 292, Extract a local result-builder helper for the repeated ProviderUsage objects in collectCodexUsage, centralizing providerId and providerName and accepting the varying status, account label, windows, and errorMessage values. Replace each existing return literal with calls to this helper while preserving the current branch-specific fallbacks and values.apps/desktop/src/main/lib/provider-usage/providers/codex.test.ts (1)
150-204: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the complementary test for a successful snapshot write.
This test proves that no snapshot is cached when the active identity changes. The positive case is not covered: when the identity is unchanged,
collectCodexUsagemust callputSnapshotwith the parsed windows, the parsedplanLabel, and the injectednow()value.Without that test, a regression that stops persisting snapshots entirely would still pass the suite, because the only
putSnapshotassertion istoEqual([]).💚 Sketch
+ test("caches the live reading when the active account is stable", async () => { + const identity: CodexIdentity = { + accountId: "acct_a", + email: "first@example.com", + plan: "pro", + }; + const written: CodexUsageSnapshot[] = []; + const store: typeof codexProfileStore = { + ...emptyProfileStore(), + activeIdentity: () => identity, + putSnapshot: async (snapshot) => { + written.push(snapshot); + }, + }; + + await collectCodexUsage({ + profileStore: store, + now: () => 1_700_000_000_000, + readRateLimits: async () => ({ + status: "ok", + value: { + rateLimits: { planType: "pro", primary: { usedPercent: 20 } }, + }, + }), + }); + + expect(written).toHaveLength(1); + expect(written[0]?.accountId).toBe("acct_a"); + expect(written[0]?.capturedAt).toBe(1_700_000_000_000); + expect(written[0]?.planLabel).toBe("PRO"); + expect(written[0]?.windows).toHaveLength(1); + });Import
CodexUsageSnapshotas a type from./codex-profiles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/lib/provider-usage/providers/codex.test.ts` around lines 150 - 204, Add a complementary test near the existing active-account-change case that keeps the identity unchanged and verifies successful snapshot persistence. Configure the test store to capture the written snapshot, provide parsed rate-limit windows, plan label, and an injected now() value, then assert putSnapshot receives a CodexUsageSnapshot containing those values. Import CodexUsageSnapshot as a type from ./codex-profiles.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/lib/trpc/routers/provider-usage.ts`:
- Around line 72-78: Update refreshAfterMutation so failures from
resolved.collect({ force: true }) are caught and do not propagate after mutation
succeeds. Always return the mutation result from refreshAfterMutation, while
still invoking the forced collection so existing behavior and tests remain
intact.
In `@apps/desktop/src/main/lib/provider-usage/providers/codex-app-server.ts`:
- Around line 63-71: Update the environment-loading flow around getEnv and
defaultCodexHomeEnv to retain whether environment resolution failed, then use
that state in the Codex startup ENOENT handling to return unavailable when
executable availability is unknown instead of not-configured. Add coverage for
rejected getEnv followed by an ENOENT startup error.
In `@apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts`:
- Around line 426-453: Update the login process around the spawned child in the
Codex profile flow to consume child.stderr while it runs, and include the
collected stderr text in the non-zero-exit error from the exit handler. Preserve
timeout cleanup and rejection behavior, and ensure the timeout terminates
unresponsive processes forcefully if the CLI can ignore the default termination
signal.
- Around line 354-367: Update readSnapshots to validate parsed JSON with the
existing Zod dependency and the provider-usage schema before returning it.
Derive or reuse a schema for CodexUsageSnapshot, including validating
UsageWindow entries, and retain only entries that successfully parse; return an
empty record when the top-level value is invalid. Keep getSnapshot unchanged so
projectCachedWindows never receives malformed snapshots.
- Around line 74-83: Update copySensitiveFileAtomically and
writeSensitiveFileAtomically to clean up their temporary paths when copying,
permission changes, or renaming fails. Ensure cleanup is attempted in a catch or
finally block without masking the original error, while preserving the existing
atomic write behavior on success.
- Around line 339-347: Update the backup flow around copySensitiveFileAtomically
to prune backupsDir after each successful backup, retaining only a small fixed
number of the newest auth-*.json files and deleting older backups. Ensure
pruning is limited to matching backup files and preserves the newly created
backup.
In `@apps/desktop/src/main/lib/provider-usage/providers/codex.ts`:
- Around line 106-122: Unify Codex profile-name derivation by exporting the
normalization helper used by accountRows in
apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts:382-409,
then reuse it in currentAccountRow in
apps/desktop/src/main/lib/provider-usage/providers/codex.ts:106-122 instead of
using the raw email fallback, so both paths produce the same normalized
profileName.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/AIUsageIndicator.tsx`:
- Around line 100-112: Update the importCodex, addCodex, and switchCodex
mutations with onError handlers that store or expose each failure message, then
render the resulting error in the Codex popover. Ensure inline error text
includes the select-text cursor-text classes, while preserving the existing
success invalidation behavior.
- Line 25: Update the persisted-key registry test data to include
superset.aiUsage.metricSelection, documenting its fixed four-boolean bound,
deletion owner or path, and removal behavior. Ensure the registry entry matches
the key used by AIUsageIndicator and follows the existing persisted-key
test-data structure.
---
Nitpick comments:
In `@apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.test.ts`:
- Around line 154-191: Add a third window with resetAt: null to the snapshot in
the projectCachedWindows test, then include that window unchanged in the
expected result alongside the non-expired secondary window. This should cover
the branch that preserves windows without a reset time.
- Around line 77-91: Extend the credential test around parseCodexIdentity to
import statSync and SUPERSET_SENSITIVE_FILE_MODE, then assert the stored
auth.json permission bits (statSync(...).mode & 0o777) equal
SUPERSET_SENSITIVE_FILE_MODE, alongside the existing accountId assertion.
- Line 192: Add tests covering the store’s putSnapshot and accountRows behaviors
using the existing injected rootDir setup. Verify putSnapshot rejects writes
whose capturedAt is less than or equal to the existing snapshot and accepts
newer snapshots, then verify accountRows produces the expected
ProviderUsageAccount fields and ok/cached/no-data status mapping.
In `@apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts`:
- Around line 382-409: Refactor accountRows() to async/await and load the
snapshots data once before iterating profiles, then resolve each profile’s
cached snapshot from that in-memory data instead of calling getSnapshot() inside
map. Preserve the existing account-row fields, status logic, and empty-snapshot
behavior while matching the file’s established async/await style.
- Around line 74-96: Replace the synchronous renameSync calls in
copySensitiveFileAtomically and writeSensitiveFileAtomically with the awaited
async rename from node:fs/promises, adding the required import and preserving
the existing atomic operation order.
- Around line 205-212: Update findCodexExecutable to read
SUPERSET_CODEX_EXECUTABLE instead of CCM_CODEX_EXECUTABLE, unless the CCM_
prefix is intentionally required; if retaining it, document that intent near the
environment-variable lookup.
In `@apps/desktop/src/main/lib/provider-usage/providers/codex.test.ts`:
- Around line 150-204: Add a complementary test near the existing
active-account-change case that keeps the identity unchanged and verifies
successful snapshot persistence. Configure the test store to capture the written
snapshot, provide parsed rate-limit windows, plan label, and an injected now()
value, then assert putSnapshot receives a CodexUsageSnapshot containing those
values. Import CodexUsageSnapshot as a type from ./codex-profiles.
In `@apps/desktop/src/main/lib/provider-usage/providers/codex.ts`:
- Around line 38-42: Update the CodexUsageDependencies.profileStore type to a
minimal interface containing only activeIdentity, accountRows, and putSnapshot,
using the CodexUsageSnapshot type imported from ./codex-profiles as needed. Keep
collectCodexUsage behavior unchanged and allow test fakes to implement only
these consumed members.
- Around line 213-292: Extract a local result-builder helper for the repeated
ProviderUsage objects in collectCodexUsage, centralizing providerId and
providerName and accepting the varying status, account label, windows, and
errorMessage values. Replace each existing return literal with calls to this
helper while preserving the current branch-specific fallbacks and values.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/AIUsageIndicator.tsx`:
- Around line 77-91: In AIUsageIndicator, simplify the remaining calculation to
use getLowestSelectedRemainingPercent(providers, selection) directly, removing
the unreachable getLowestRemainingPercent fallback and its import. At the later
usage around the component’s line 229, reuse the already-derived providers value
instead of reading data?.providers again.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.test.tsx`:
- Around line 121-158: The test “renders inactive cached accounts separately
from the active account” should also verify that Claude providers do not render
the Codex-only Switch button. Add a negative assertion against “Switch” using
the existing markup result, while preserving the current account and
inactive-state assertions.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.tsx`:
- Around line 61-78: In the synthesized default account within ProviderUsageRow,
simplify the status fields to their branch-invariant values: set isActive to
true and status to "ok". Do not copy provider.accountLabel into planLabel; use
the appropriate absent/default plan value while preserving the existing
accountLabel and other account properties.
- Around line 139-224: Move AccountUsageBlock from
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.tsx:139-224
into a co-located components/AccountUsageBlock/AccountUsageBlock.tsx with an
index.ts barrel export, and update ProviderUsageRow imports. Move MetricToggle
from
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/AIUsageIndicator.tsx:267-290
into components/MetricToggle/MetricToggle.tsx with an index.ts barrel export,
and update AIUsageIndicator imports; leave each parent file with only its
primary component.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.test.ts`:
- Around line 118-178: Add a test fixture with an active account window sharing
an id with a provider window but having a different remainingPercent, then
assert the relevant summary or lowest-remaining result uses the account value.
Keep the provider window and active account association explicit so
activeWindows precedence for the matching id is exercised.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.ts`:
- Around line 76-85: Rename the function findWindowByMinutes to
findWindowBySeconds and update both call sites to use the new name, preserving
the existing windowSeconds matching behavior.
- Around line 142-151: Refactor getMenuBarSummaryParts to use a helper that
returns structured { label, value } pairs for the selected providers and
metrics. Build the displayed summary strings from the pair labels, and update
getLowestSelectedRemainingPercent to compute the minimum directly from the pair
values instead of parsing formatted strings, preserving the existing fallback
when no finite values exist.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 0b27558e-1b18-4b7f-b4e2-df7d69e60296
📒 Files selected for processing (18)
apps/desktop/src/lib/trpc/routers/provider-usage.schema.tsapps/desktop/src/lib/trpc/routers/provider-usage.test.tsapps/desktop/src/lib/trpc/routers/provider-usage.tsapps/desktop/src/main/lib/provider-usage/index.test.tsapps/desktop/src/main/lib/provider-usage/index.tsapps/desktop/src/main/lib/provider-usage/providers/claude.test.tsapps/desktop/src/main/lib/provider-usage/providers/claude.tsapps/desktop/src/main/lib/provider-usage/providers/codex-app-server.test.tsapps/desktop/src/main/lib/provider-usage/providers/codex-app-server.tsapps/desktop/src/main/lib/provider-usage/providers/codex-profiles.test.tsapps/desktop/src/main/lib/provider-usage/providers/codex-profiles.tsapps/desktop/src/main/lib/provider-usage/providers/codex.test.tsapps/desktop/src/main/lib/provider-usage/providers/codex.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/AIUsageIndicator.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.test.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- apps/desktop/src/main/lib/provider-usage/index.test.ts
- apps/desktop/src/main/lib/provider-usage/providers/codex-app-server.test.ts
- apps/desktop/src/main/lib/provider-usage/index.ts
Comment on lines
+354
to
+367
| function readSnapshots(): Record<string, CodexUsageSnapshot> { | ||
| try { | ||
| return JSON.parse(readFileSync(snapshotsPath, "utf8")) as Record< | ||
| string, | ||
| CodexUsageSnapshot | ||
| >; | ||
| } catch { | ||
| return {}; | ||
| } | ||
| } | ||
|
|
||
| function getSnapshot(accountId: string): CodexUsageSnapshot | null { | ||
| return readSnapshots()[accountId] ?? null; | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Validate the parsed snapshots file before use.
readSnapshots casts the parsed JSON straight to Record<string, CodexUsageSnapshot> with no validation. JSON.parse succeeds for any well-formed JSON, so a truncated write, a hand edit, or a snapshot written by an older schema version all pass through. projectCachedWindows then calls snapshot.windows.filter(...) at line 200. If windows is not an array, this throws a TypeError that propagates out of accountRows and fails the whole usage collection.
The file already depends on Zod through provider-usage.schema. Validate the shape here and drop entries that do not match.
🛡️ Proposed fix using a defensive guard
function readSnapshots(): Record<string, CodexUsageSnapshot> {
try {
- return JSON.parse(readFileSync(snapshotsPath, "utf8")) as Record<
- string,
- CodexUsageSnapshot
- >;
+ const parsed: unknown = JSON.parse(readFileSync(snapshotsPath, "utf8"));
+ if (!parsed || typeof parsed !== "object") return {};
+ const result: Record<string, CodexUsageSnapshot> = {};
+ for (const [accountId, value] of Object.entries(parsed)) {
+ const candidate = value as Partial<CodexUsageSnapshot>;
+ if (
+ typeof candidate?.accountId === "string" &&
+ typeof candidate.capturedAt === "number" &&
+ Array.isArray(candidate.windows)
+ ) {
+ result[accountId] = candidate as CodexUsageSnapshot;
+ }
+ }
+ return result;
} catch {
return {};
}
}A Zod schema derived from UsageWindow would be stronger, because it would also reject malformed window entries.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| function readSnapshots(): Record<string, CodexUsageSnapshot> { | |
| try { | |
| return JSON.parse(readFileSync(snapshotsPath, "utf8")) as Record< | |
| string, | |
| CodexUsageSnapshot | |
| >; | |
| } catch { | |
| return {}; | |
| } | |
| } | |
| function getSnapshot(accountId: string): CodexUsageSnapshot | null { | |
| return readSnapshots()[accountId] ?? null; | |
| } | |
| function readSnapshots(): Record<string, CodexUsageSnapshot> { | |
| try { | |
| const parsed: unknown = JSON.parse(readFileSync(snapshotsPath, "utf8")); | |
| if (!parsed || typeof parsed !== "object") return {}; | |
| const result: Record<string, CodexUsageSnapshot> = {}; | |
| for (const [accountId, value] of Object.entries(parsed)) { | |
| const candidate = value as Partial<CodexUsageSnapshot>; | |
| if ( | |
| typeof candidate?.accountId === "string" && | |
| typeof candidate.capturedAt === "number" && | |
| Array.isArray(candidate.windows) | |
| ) { | |
| result[accountId] = candidate as CodexUsageSnapshot; | |
| } | |
| } | |
| return result; | |
| } catch { | |
| return {}; | |
| } | |
| } | |
| function getSnapshot(accountId: string): CodexUsageSnapshot | null { | |
| return readSnapshots()[accountId] ?? null; | |
| } |
🧰 Tools
🪛 ast-grep (0.45.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts` around
lines 354 - 367, Update readSnapshots to validate parsed JSON with the existing
Zod dependency and the provider-usage schema before returning it. Derive or
reuse a schema for CodexUsageSnapshot, including validating UsageWindow entries,
and retain only entries that successfully parse; return an empty record when the
top-level value is invalid. Keep getSnapshot unchanged so projectCachedWindows
never receives malformed snapshots.
Comment on lines
+106
to
+122
| function currentAccountRow( | ||
| identity: CodexIdentity, | ||
| windows: UsageWindow[], | ||
| planLabel: string | null, | ||
| ): ProviderUsageAccount { | ||
| return { | ||
| id: `codex:${identity.accountId}`, | ||
| providerId: "codex", | ||
| profileName: identity.email ?? "current", | ||
| accountLabel: identity.email, | ||
| planLabel: planLabel ?? identity.plan, | ||
| isActive: true, | ||
| status: windows.length > 0 ? "ok" : "no-data", | ||
| statusMessage: windows.length > 0 ? "live" : "No reading yet", | ||
| windows, | ||
| }; | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
profileName uses two different formats for the same Codex account. Both files build a ProviderUsageAccount, but each derives profileName differently. The shared root cause is that no single function owns the profile-name format. An account that is active but not yet imported shows one label, then shows a different label after import, with no user action in between.
apps/desktop/src/main/lib/provider-usage/providers/codex.ts#L106-L122:currentAccountRowsetsprofileName: identity.email ?? "current", so a raw email address. Derive the name with the same normalization the profile store uses, or set a clearly provisional value that the renderer can distinguish.apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts#L382-L409:accountRowssetsprofileName: profile.profileName, the normalized directory slug such asperson-example-com. Export the normalization helper socodex.tscan reuse it and both paths agree.
📍 Affects 2 files
apps/desktop/src/main/lib/provider-usage/providers/codex.ts#L106-L122(this comment)apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts#L382-L409
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/lib/provider-usage/providers/codex.ts` around lines 106
- 122, Unify Codex profile-name derivation by exporting the normalization helper
used by accountRows in
apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts:382-409,
then reuse it in currentAccountRow in
apps/desktop/src/main/lib/provider-usage/providers/codex.ts:106-122 instead of
using the raw email fallback, so both paths produce the same normalized
profileName.
| type UsageMetricSelection, | ||
| } from "./usageIndicatorPolicy"; | ||
|
|
||
| const METRIC_SELECTION_STORAGE_KEY = "superset.aiUsage.metricSelection"; |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Verify the new localStorage key is registered in the persisted key registry.
set -uo pipefail
fd -i 'persisted-key-registry' apps/desktop/src
# Test: the new key must appear in the registry test data. Expect: at least one match.
rg -n 'aiUsage|metricSelection' apps/desktop/src/renderer/lib/persisted-keys/ || echo "NOT REGISTERED"
# Show how existing keys are declared and documented.
fd -i 'persisted-key' apps/desktop/src --exec rg -n -C3 'superset\.' {}Repository: superset-sh/superset
Length of output: 336
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- target file ---'
sed -n '1,180p' apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/AIUsageIndicator.tsx
printf '%s\n' '--- persisted-key candidates ---'
fd -i 'persisted|storage|registry' apps/desktop/src apps/desktop 2>/dev/null | head -200
printf '%s\n' '--- relevant references ---'
rg -n -C3 'localStorage|persisted-key|metricSelection|aiUsage' apps/desktop/src apps/desktop 2>/dev/null | head -300Repository: superset-sh/superset
Length of output: 39042
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- all persisted-key registry paths ---'
git ls-files | rg -i 'persisted[-_]?key|localstorage|storage.*registry|registry.*storage' || true
printf '%s\n' '--- all registry and allowlist references ---'
rg -n -i 'persisted[- ]key|localStorage.*allowlist|allowlist.*localStorage|storage key|deletion owner|feature.*removed' . \
-g '!**/node_modules/**' -g '!**/.git/**' | head -400 || true
printf '%s\n' '--- exact metric key usage ---'
rg -n -C5 'superset\.aiUsage\.metricSelection|METRIC_SELECTION_STORAGE_KEY' apps/desktop/srcRepository: superset-sh/superset
Length of output: 5427
Add and document the persisted-key registry entry.
AIUsageIndicator reads and writes superset.aiUsage.metricSelection. Add apps/desktop/src/renderer/lib/persisted-keys/persisted-key-registry.test-data.ts and register this key with its fixed four-boolean bound, deletion owner or path, and removal behavior.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/AIUsageIndicator.tsx`
at line 25, Update the persisted-key registry test data to include
superset.aiUsage.metricSelection, documenting its fixed four-boolean bound,
deletion owner or path, and removal behavior. Ensure the registry entry matches
the key used by AIUsageIndicator and follows the existing persisted-key
test-data structure.
Source: Path instructions
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 22 files
Architecture diagram
sequenceDiagram
participant UI as AIUsageIndicator (TopBar)
participant Policy as usageIndicatorPolicy
participant IPC as tRPC Router (providerUsage)
participant Collector as ProviderUsageCollector (Main)
participant Cache as Five-Minute Cache
participant Claude as Claude Provider
participant Codex as Codex Provider
participant ProfileStore as CodexProfileStore
participant AppServer as codex app-server (subprocess)
participant Anthropic as Anthropic API
participant Keychain as macOS Keychain / Superset Credentials
participant FS as Filesystem (~/.codex, app-owned dirs)
Note over UI,FS: Renderer (network-cold) <-> Main process boundary
rect rgb(240, 248, 255)
Note over UI,Policy: Renderer - network-cold until opened
UI->>Policy: shouldQueryProviderUsage(open)
Policy-->>UI: false (closed) / true (open)
alt Popover closed
Note over UI: No IPC call, no polling
else Popover opened
UI->>IPC: getSnapshot.query()
Note over UI: refetchInterval set to 5 min while open
end
UI->>Policy: getMenuBarSummaryParts(providers, selection)
Policy-->>UI: "A 72 · C 40" compact label
UI->>Policy: getLowestSelectedRemainingPercent()
Policy-->>UI: lowest remaining %
end
rect rgb(255, 250, 240)
Note over IPC,Collector: Main process - caching layer
IPC->>Collector: collect({ force: false })
Collector->>Cache: Check TTL (5 min)
alt Cache valid and !force
Cache-->>Collector: cached snapshot
else Cache expired or force
Collector->>Collector: inFlight deduplication check
par Claude and Codex in parallel
Collector->>Claude: collectClaudeUsage()
Collector->>Codex: collectCodexUsage()
end
Cache-->>Collector: new snapshot
end
Collector-->>IPC: ProviderUsageSnapshot (Zod-validated)
IPC-->>UI: Safe fields only (no credentials)
end
rect rgb(245, 245, 255)
Note over Claude,Keychain: Claude Provider
Claude->>Keychain: readCredentials()
alt Not configured
Keychain-->>Claude: null
Claude-->>Collector: status=not-configured
else Has valid OAuth (Keychain or Superset creds)
Claude->>Anthropic: GET /api/oauth/usage (Bearer token)
alt 200 OK with usage buckets
Anthropic-->>Claude: five_hour, seven_day, limits[]
Claude-->>Collector: status=ok, windows[]
else Non-200 or malformed
Anthropic-->>Claude: error/429
Claude-->>Collector: status=unavailable, safe errorMessage
end
end
end
rect rgb(240, 255, 240)
Note over Codex,FS: Codex Provider (local subprocess)
Codex->>ProfileStore: activeIdentity()
ProfileStore->>FS: read ~/.codex/auth.json
FS-->>ProfileStore: identity (accountId, email, plan)
Codex->>AppServer: spawn codex app-server --stdio
Note over AppServer: Env strips CODEX_HOME / CODEX_SQLITE_HOME
AppServer->>AppServer: initialize (id=1)
AppServer-->>Codex: initialize response
AppServer->>AppServer: initialized notification
AppServer->>AppServer: account/rateLimits/read (id=2)
AppServer-->>Codex: rateLimits result
alt Executable not found (ENOENT)
AppServer-->>Codex: status=not-configured
else Success
Codex->>ProfileStore: confirmedActiveIdentity()
alt Identity stable during poll
Codex->>ProfileStore: putSnapshot(accountId, capturedAt, windows)
end
ProfileStore->>ProfileStore: projectCachedWindows() - filters expired
Codex-->>Collector: status=ok, windows[], accounts[]
end
end
rect rgb(255, 245, 255)
Note over UI,ProfileStore: Account Mutations (user-triggered)
UI->>IPC: importCurrentCodex.mutation()
IPC->>ProfileStore: importActive()
ProfileStore->>FS: copy auth.json -> app-owned provider-usage dir
Note over ProfileStore: temp-file + rename, chmod sensitive
UI->>IPC: addCodexAccount.mutation()
IPC->>ProfileStore: addViaIsolatedLogin()
ProfileStore->>FS: isolated CODEX_HOME login flow
UI->>IPC: switchCodexProfile(profileName)
IPC->>IPC: Regex guard /^[a-z0-9][a-z0-9-]*$/
IPC->>ProfileStore: activate(profileName)
ProfileStore->>FS: backup active auth.json to backups dir
ProfileStore->>FS: atomic replace ~/.codex/auth.json
Note over IPC,Collector: Each mutation triggers force refresh
IPC->>Collector: collect({ force: true })
Collector-->>IPC: updated snapshot (accounts reflect change)
IPC-->>UI: mutation result + invalidate query
end
rect rgb(250, 250, 250)
Note over UI,Policy: Renderer - error/window states
UI->>Policy: formatResetLabel(resetAt)
Policy-->>UI: "3h 42m · Jul 21, 6:30 PM"
alt Provider not-configured
UI->>UI: Show "Sign in with Claude/Codex CLI"
else Provider unavailable
UI->>UI: Show generic errorMessage, no fabricated capacity
else Provider ok but account cached
UI->>UI: Show cached status with "Available after using this account"
end
end
Tip: cubic used a learning from your PR history. Let your coding agent read cubic learnings directly with the cubic MCP.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/lib/provider-usage/index.test.ts`:
- Around line 116-122: Update the test around collect() so the initial poll
remains pending after starting collect({ force: true }); assert claudeCalls is
still 1 before resolving the first poll, then call resolveClaude and retain the
existing result and call-count assertions to verify the forced poll waits.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/components/AccountUsageBlock/AccountUsageBlock.tsx`:
- Around line 46-51: Update the Codex inactive-account control in
AccountUsageBlock so it is only rendered when onSwitchProfile exists, or is
disabled when that callback is unavailable. Preserve the existing
actionsDisabled behavior and profile-switch callback invocation when the handler
is provided.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f1da826f-13f0-4a24-896f-6aebdc2f9fb6
📒 Files selected for processing (20)
apps/desktop/src/lib/trpc/routers/provider-usage.test.tsapps/desktop/src/lib/trpc/routers/provider-usage.tsapps/desktop/src/main/lib/provider-usage/index.test.tsapps/desktop/src/main/lib/provider-usage/index.tsapps/desktop/src/main/lib/provider-usage/providers/claude.tsapps/desktop/src/main/lib/provider-usage/providers/codex-app-server.test.tsapps/desktop/src/main/lib/provider-usage/providers/codex-app-server.tsapps/desktop/src/main/lib/provider-usage/providers/codex-profiles.test.tsapps/desktop/src/main/lib/provider-usage/providers/codex-profiles.tsapps/desktop/src/main/lib/provider-usage/providers/codex.test.tsapps/desktop/src/main/lib/provider-usage/providers/codex.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/AIUsageIndicator.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/MetricToggle/MetricToggle.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/MetricToggle/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.test.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/components/AccountUsageBlock/AccountUsageBlock.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/components/AccountUsageBlock/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.ts
🚧 Files skipped from review as they are similar to previous changes (6)
- apps/desktop/src/main/lib/provider-usage/index.ts
- apps/desktop/src/lib/trpc/routers/provider-usage.test.ts
- apps/desktop/src/lib/trpc/routers/provider-usage.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.ts
- apps/desktop/src/main/lib/provider-usage/providers/codex.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/AIUsageIndicator.tsx
Contributor
There was a problem hiding this comment.
All reported issues were addressed across 20 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.test.tsx (1)
291-330: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse a fixed expired timestamp.
Line 318 uses
Date.now()even though this test only needs an already-expired window. UseresetAt: 0so the test has no wall-clock dependency.Proposed fix
- resetAt: Date.now() - 1_000, + resetAt: 0,Based on learnings: “test code should avoid real wall-clock timers.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.test.tsx` around lines 291 - 330, Replace the Date.now()-based resetAt value in the “filters expired cached account windows at render time” test with the fixed expired timestamp 0, leaving the rest of the test unchanged.Source: Learnings
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.test.tsx`:
- Around line 291-330: Replace the Date.now()-based resetAt value in the
“filters expired cached account windows at render time” test with the fixed
expired timestamp 0, leaving the rest of the test unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d5815f35-061b-4950-aa8d-4fc5fceb31bb
📒 Files selected for processing (7)
apps/desktop/src/main/lib/provider-usage/index.test.tsapps/desktop/src/main/lib/provider-usage/index.tsapps/desktop/src/main/lib/provider-usage/providers/codex-profiles.test.tsapps/desktop/src/main/lib/provider-usage/providers/codex-profiles.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/AIUsageIndicator.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.test.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/components/AccountUsageBlock/AccountUsageBlock.tsx
🚧 Files skipped from review as they are similar to previous changes (2)
- apps/desktop/src/main/lib/provider-usage/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/AIUsageIndicator.tsx
Contributor
There was a problem hiding this comment.
2 issues found across 7 files (changes from recent commits).
You’re at about 90% of the monthly reviewed-line limit. You may want to disable incremental reviews to conserve quota. Reviews will continue until that limit is exceeded. If you need help avoiding interruptions, please contact contact@cubic.dev.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/desktop/src/main/lib/provider-usage/index.ts">
<violation number="1" location="apps/desktop/src/main/lib/provider-usage/index.ts:85">
P2: Concurrent forced refreshes can still launch duplicate provider polls: `inFlight` is cleared before the coalesced `forcedFollowUp` begins, so a force request in that interval skips this guard and calls `collectFresh()` independently. Checking `forcedFollowUp` before the `inFlight` branch (or otherwise serializing that transition) keeps all force requests on the same follow-up and prevents competing snapshots.
(Based on your team's feedback about preserving forced follow-up requests across overlapping scans.)</violation>
</file>
<file name="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/components/AccountUsageBlock/AccountUsageBlock.tsx">
<violation number="1" location="apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/components/AccountUsageBlock/AccountUsageBlock.tsx:28">
P2: When every returned window has expired, this row now hides the progress bar but displays `live` as its usage message (and in the account header), implying a valid live reading when none is visible. The empty-state message should distinguish a successful fetch with no active windows from an account that has a current reading.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
| } | ||
| if (inFlight) { | ||
| if (!options.force) return inFlight; | ||
| if (forcedFollowUp) return forcedFollowUp; |
Contributor
There was a problem hiding this comment.
P2: Concurrent forced refreshes can still launch duplicate provider polls: inFlight is cleared before the coalesced forcedFollowUp begins, so a force request in that interval skips this guard and calls collectFresh() independently. Checking forcedFollowUp before the inFlight branch (or otherwise serializing that transition) keeps all force requests on the same follow-up and prevents competing snapshots.
(Based on your team's feedback about preserving forced follow-up requests across overlapping scans.)
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/main/lib/provider-usage/index.ts, line 85:
<comment>Concurrent forced refreshes can still launch duplicate provider polls: `inFlight` is cleared before the coalesced `forcedFollowUp` begins, so a force request in that interval skips this guard and calls `collectFresh()` independently. Checking `forcedFollowUp` before the `inFlight` branch (or otherwise serializing that transition) keeps all force requests on the same follow-up and prevents competing snapshots.
(Based on your team's feedback about preserving forced follow-up requests across overlapping scans.) </comment>
<file context>
@@ -80,7 +82,15 @@ export function createProviderUsageCollector(
if (inFlight) {
if (!options.force) return inFlight;
- await inFlight.catch(() => undefined);
+ if (forcedFollowUp) return forcedFollowUp;
+ const currentPoll = inFlight;
+ forcedFollowUp = currentPoll
</file context>
| ? "account" | ||
| : (account.accountLabel ?? account.profileName); | ||
| const now = Date.now(); | ||
| const visibleWindows = account.windows.filter( |
Contributor
There was a problem hiding this comment.
P2: When every returned window has expired, this row now hides the progress bar but displays live as its usage message (and in the account header), implying a valid live reading when none is visible. The empty-state message should distinguish a successful fetch with no active windows from an account that has a current reading.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/components/AccountUsageBlock/AccountUsageBlock.tsx, line 28:
<comment>When every returned window has expired, this row now hides the progress bar but displays `live` as its usage message (and in the account header), implying a valid live reading when none is visible. The empty-state message should distinguish a successful fetch with no active windows from an account that has a current reading.</comment>
<file context>
@@ -24,6 +24,10 @@ export function AccountUsageBlock({
? "account"
: (account.accountLabel ?? account.profileName);
+ const now = Date.now();
+ const visibleWindows = account.windows.filter(
+ (window) => window.resetAt === null || window.resetAt > now,
+ );
</file context>
…inimal-ai-usage-meter # Conflicts: # bun.lock # packages/local-db/drizzle/meta/0043_snapshot.json # packages/local-db/drizzle/meta/_journal.json
Contributor
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts (2)
403-412: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winValidate the parsed snapshots file before use.
readSnapshotsstill casts the parsed JSON directly toRecord<string, CodexUsageSnapshot>.JSON.parseaccepts any well-formed JSON, so a truncated write or an older schema version passes through.projectCachedWindowsthen callssnapshot.windows.filter(...)at Line 211. Ifwindowsis not an array, theTypeErrorpropagates out ofaccountRowsand fails the whole usage collection.🛡️ Proposed fix
function readSnapshots(): Record<string, CodexUsageSnapshot> { try { - return JSON.parse(readFileSync(snapshotsPath, "utf8")) as Record< - string, - CodexUsageSnapshot - >; + const parsed: unknown = JSON.parse(readFileSync(snapshotsPath, "utf8")); + if (!parsed || typeof parsed !== "object") return {}; + const result: Record<string, CodexUsageSnapshot> = {}; + for (const [accountId, value] of Object.entries(parsed)) { + const candidate = value as Partial<CodexUsageSnapshot>; + if ( + typeof candidate?.accountId === "string" && + typeof candidate.capturedAt === "number" && + Array.isArray(candidate.windows) + ) { + result[accountId] = candidate as CodexUsageSnapshot; + } + } + return result; } catch { return {}; } }A Zod schema built from
UsageWindowis stronger, because it also rejects malformed window entries.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts` around lines 403 - 412, Update readSnapshots to validate the JSON result with the existing Zod schema derived from UsageWindow before returning it as Record<string, CodexUsageSnapshot>; reject invalid root values, snapshots, and window entries and return {} on validation failure. Remove the unsafe direct cast while preserving the current fallback behavior for unreadable or invalid snapshot files.
469-497: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winConsume the piped stderr, or the login can hang.
stdiosets stderr to"pipe"at Line 479, but nothing readschild.stderr. The pipe buffer is finite. Ifcodex loginwrites more than the buffer holds, the child blocks on its next write and never exits, so the flow stalls until the 10-minuteCODEX_LOGIN_TIMEOUT_MSfires. The rejection message also discards the stderr text that explains the failure. Collect stderr and include it in the error, or set stderr to"ignore".🐛 Proposed fix
stdio: ["ignore", "ignore", "pipe"], }); + let stderr = ""; + child.stderr?.setEncoding("utf8"); + child.stderr?.on("data", (chunk: string) => { + stderr = `${stderr}${chunk}`.slice(-4096); + }); const timeout = setTimeout(() => { child.kill(); reject(new Error("Codex login timed out.")); }, CODEX_LOGIN_TIMEOUT_MS); @@ } else { - reject(new Error("Codex sign-in did not complete.")); + reject( + new Error( + `Codex sign-in did not complete.${stderr ? ` ${stderr.trim()}` : ""}`, + ), + ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts` around lines 469 - 497, Update the child process handling in the Codex login flow around createCodexLoginCommand and spawn to consume child.stderr while the process runs, preventing pipe-buffer blocking; collect the stderr text and include it in the rejection error when the exit code is nonzero, while preserving the existing timeout and successful exit behavior.
🧹 Nitpick comments (1)
apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts (1)
431-452: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winRead the snapshots file once per
accountRowscall.
getSnapshotcallsreadSnapshots, which performs a synchronousreadFileSyncand a fullJSON.parse.accountRowscalls it once per profile, so the same file is read and parsed N times on the Electron main thread. Read the map once and index it inside the map callback.♻️ Proposed fix
function accountRows(): Promise<ProviderUsageAccount[]> { - return listProfiles().then((profiles) => - profiles.map((profile) => { - const snapshot = getSnapshot(profile.identity.accountId); + return listProfiles().then((profiles) => { + const snapshots = readSnapshots(); + return profiles.map((profile) => { + const snapshot = snapshots[profile.identity.accountId] ?? null; const windows = snapshot ? projectCachedWindows(snapshot) : [];🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts` around lines 431 - 452, Update accountRows to load the snapshots map once before iterating profiles, then index that map by profile.identity.accountId inside the map callback instead of calling getSnapshot for each profile. Preserve the existing fallback behavior when an account has no snapshot.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts`:
- Around line 418-429: Serialize putSnapshot operations through a single shared
in-flight promise so each invocation reads the latest snapshots after the
previous write completes. Preserve the existing timestamp guard and atomic write
behavior, and ensure the shared promise is updated and cleared correctly even
when a write fails.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/components/AccountUsageBlock/AccountUsageBlock.tsx`:
- Around line 44-48: Update the status message span in AccountUsageBlock to
include the same select-text and cursor-text classes used by the fallback
rendering, while preserving its existing truncation and muted styling.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/BehaviorSettings/BehaviorSettings.tsx`:
- Around line 246-254: Wrap the “AI usage meter” label text in HighlightText,
passing query={searchQuery}, while preserving the existing label structure and
htmlFor association. Follow the HighlightText usage used by the other settings
items in BehaviorSettings.
- Around line 256-258: Update the Switch controlled by aiUsageIndicatorEnabled
to import and use DEFAULT_SHOW_AI_USAGE_INDICATOR from shared/constants as the
nullish fallback instead of false, preserving the server-default loading state.
---
Duplicate comments:
In `@apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts`:
- Around line 403-412: Update readSnapshots to validate the JSON result with the
existing Zod schema derived from UsageWindow before returning it as
Record<string, CodexUsageSnapshot>; reject invalid root values, snapshots, and
window entries and return {} on validation failure. Remove the unsafe direct
cast while preserving the current fallback behavior for unreadable or invalid
snapshot files.
- Around line 469-497: Update the child process handling in the Codex login flow
around createCodexLoginCommand and spawn to consume child.stderr while the
process runs, preventing pipe-buffer blocking; collect the stderr text and
include it in the rejection error when the exit code is nonzero, while
preserving the existing timeout and successful exit behavior.
---
Nitpick comments:
In `@apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts`:
- Around line 431-452: Update accountRows to load the snapshots map once before
iterating profiles, then index that map by profile.identity.accountId inside the
map callback instead of calling getSnapshot for each profile. Preserve the
existing fallback behavior when an account has no snapshot.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 28a6580f-9139-4bb8-9ec6-cd81ef0190e5
📒 Files selected for processing (35)
apps/desktop/src/lib/trpc/routers/index.tsapps/desktop/src/lib/trpc/routers/provider-usage.schema.tsapps/desktop/src/lib/trpc/routers/provider-usage.test.tsapps/desktop/src/lib/trpc/routers/provider-usage.tsapps/desktop/src/lib/trpc/routers/settings/index.tsapps/desktop/src/main/lib/provider-usage/index.test.tsapps/desktop/src/main/lib/provider-usage/index.tsapps/desktop/src/main/lib/provider-usage/providers/claude.test.tsapps/desktop/src/main/lib/provider-usage/providers/claude.tsapps/desktop/src/main/lib/provider-usage/providers/codex-app-server.test.tsapps/desktop/src/main/lib/provider-usage/providers/codex-app-server.tsapps/desktop/src/main/lib/provider-usage/providers/codex-profiles.test.tsapps/desktop/src/main/lib/provider-usage/providers/codex-profiles.tsapps/desktop/src/main/lib/provider-usage/providers/codex.test.tsapps/desktop/src/main/lib/provider-usage/providers/codex.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/TopBar.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/AIUsageIndicator.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/MetricToggle/MetricToggle.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/MetricToggle/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.test.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/components/AccountUsageBlock/AccountUsageBlock.tsxapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/components/AccountUsageBlock/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/index.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.test.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.tsapps/desktop/src/renderer/routes/_authenticated/_dashboard/v2-workspace/$workspaceId/page.tsxapps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/BehaviorSettings/BehaviorSettings.tsxapps/desktop/src/renderer/routes/_authenticated/settings/utils/settings-search/settings-search.tsapps/desktop/src/shared/constants.tspackages/local-db/drizzle/0046_wise_lorna_dane.sqlpackages/local-db/drizzle/meta/0046_snapshot.jsonpackages/local-db/drizzle/meta/_journal.jsonpackages/local-db/src/schema/schema.ts
🚧 Files skipped from review as they are similar to previous changes (22)
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/components/AccountUsageBlock/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/MetricToggle/MetricToggle.tsx
- apps/desktop/src/lib/trpc/routers/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/MetricToggle/index.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.test.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.test.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/index.ts
- apps/desktop/src/main/lib/provider-usage/index.test.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.tsx
- apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.test.ts
- apps/desktop/src/main/lib/provider-usage/providers/claude.test.ts
- apps/desktop/src/lib/trpc/routers/provider-usage.test.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/TopBar.tsx
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/AIUsageIndicator.tsx
- apps/desktop/src/main/lib/provider-usage/providers/codex-app-server.test.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.ts
- apps/desktop/src/main/lib/provider-usage/providers/codex.ts
- apps/desktop/src/main/lib/provider-usage/providers/codex.test.ts
- apps/desktop/src/lib/trpc/routers/provider-usage.ts
- apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/index.ts
- apps/desktop/src/lib/trpc/routers/provider-usage.schema.ts
- apps/desktop/src/main/lib/provider-usage/index.ts
Comment on lines
+418
to
+429
| async function putSnapshot(snapshot: CodexUsageSnapshot): Promise<void> { | ||
| ensureStore(); | ||
| const snapshots = readSnapshots(); | ||
| const existing = snapshots[snapshot.accountId]; | ||
| if (existing && existing.capturedAt >= snapshot.capturedAt) return; | ||
| snapshots[snapshot.accountId] = snapshot; | ||
| await writeSensitiveFileAtomically( | ||
| snapshotsPath, | ||
| JSON.stringify(snapshots, null, 2), | ||
| join(storeRoot, `.snapshots-${randomId()}.json`), | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Serialize snapshot writes to avoid lost updates.
putSnapshot reads snapshots.json, mutates the object, and writes the full map back. Two concurrent calls for different accounts both read the same base state, so the later write drops the earlier account entry. Multi-account collection can call this concurrently. Chain the operations through a single in-flight promise.
♻️ Proposed fix
+ let snapshotWriteQueue: Promise<void> = Promise.resolve();
+
async function putSnapshot(snapshot: CodexUsageSnapshot): Promise<void> {
- ensureStore();
- const snapshots = readSnapshots();
- const existing = snapshots[snapshot.accountId];
- if (existing && existing.capturedAt >= snapshot.capturedAt) return;
- snapshots[snapshot.accountId] = snapshot;
- await writeSensitiveFileAtomically(
- snapshotsPath,
- JSON.stringify(snapshots, null, 2),
- join(storeRoot, `.snapshots-${randomId()}.json`),
- );
+ snapshotWriteQueue = snapshotWriteQueue.then(async () => {
+ ensureStore();
+ const snapshots = readSnapshots();
+ const existing = snapshots[snapshot.accountId];
+ if (existing && existing.capturedAt >= snapshot.capturedAt) return;
+ snapshots[snapshot.accountId] = snapshot;
+ await writeSensitiveFileAtomically(
+ snapshotsPath,
+ JSON.stringify(snapshots, null, 2),
+ join(storeRoot, `.snapshots-${randomId()}.json`),
+ );
+ });
+ await snapshotWriteQueue;
}🧰 Tools
🪛 ast-grep (0.45.1)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { spawn } from "node:child_process";
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.ts` around
lines 418 - 429, Serialize putSnapshot operations through a single shared
in-flight promise so each invocation reads the latest snapshots after the
previous write completes. Preserve the existing timestamp guard and atomic write
behavior, and ensure the shared promise is updated and cleared correctly even
when a write fails.
Comment on lines
+44
to
+48
| {account.statusMessage && ( | ||
| <span className="truncate text-[9px] text-muted-foreground"> | ||
| {account.statusMessage} | ||
| </span> | ||
| )} |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the status message selectable here too.
account.statusMessage carries provider failure text, for example an unavailable Codex reading. The fallback render at Line 102 includes select-text cursor-text, but this header render does not. Add the same classes so users can copy the message from both places.
♿ Proposed fix
{account.statusMessage && (
- <span className="truncate text-[9px] text-muted-foreground">
+ <span className="truncate text-[9px] text-muted-foreground select-text cursor-text">
{account.statusMessage}
</span>
)}As per coding guidelines: "Rendered error messages must include select-text cursor-text classes so users can copy them; Sonner toasts are exempt."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| {account.statusMessage && ( | |
| <span className="truncate text-[9px] text-muted-foreground"> | |
| {account.statusMessage} | |
| </span> | |
| )} | |
| {account.statusMessage && ( | |
| <span className="truncate text-[9px] text-muted-foreground select-text cursor-text"> | |
| {account.statusMessage} | |
| </span> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/components/AccountUsageBlock/AccountUsageBlock.tsx`
around lines 44 - 48, Update the status message span in AccountUsageBlock to
include the same select-text and cursor-text classes used by the fallback
rendering, while preserving its existing truncation and muted styling.
Source: Coding guidelines
Comment on lines
+246
to
+254
| <Label | ||
| htmlFor="ai-usage-indicator" | ||
| className="text-sm font-medium" | ||
| > | ||
| AI usage meter | ||
| </Label> | ||
| <p className="text-xs text-muted-foreground"> | ||
| Show remaining Claude and Codex capacity in the top bar | ||
| </p> |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Wrap the label text in HighlightText.
This setting is registered as searchable in settings-search.ts with the title "AI usage meter". Every other item in this file passes its title through HighlightText with query={searchQuery}. Without it, a search match shows no highlight for this row.
🔍 Proposed fix
<Label
htmlFor="ai-usage-indicator"
className="text-sm font-medium"
>
- AI usage meter
+ <HighlightText text="AI usage meter" query={searchQuery} />
</Label>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| <Label | |
| htmlFor="ai-usage-indicator" | |
| className="text-sm font-medium" | |
| > | |
| AI usage meter | |
| </Label> | |
| <p className="text-xs text-muted-foreground"> | |
| Show remaining Claude and Codex capacity in the top bar | |
| </p> | |
| <Label | |
| htmlFor="ai-usage-indicator" | |
| className="text-sm font-medium" | |
| > | |
| <HighlightText text="AI usage meter" query={searchQuery} /> | |
| </Label> | |
| <p className="text-xs text-muted-foreground"> | |
| Show remaining Claude and Codex capacity in the top bar | |
| </p> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@apps/desktop/src/renderer/routes/_authenticated/settings/behavior/components/BehaviorSettings/BehaviorSettings.tsx`
around lines 246 - 254, Wrap the “AI usage meter” label text in HighlightText,
passing query={searchQuery}, while preserving the existing label structure and
htmlFor association. Follow the HighlightText usage used by the other settings
items in BehaviorSettings.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What & why
Related to #5733. This expands the desktop AI usage meter from an active-account-only meter into a multi-account usage surface modeled after the latest Notch Limits AI meter work.
The top-bar indicator still stays compact and network-cold until opened, but the popover now supports:
CODEX_HOMElogin flowThe account mutations force-refresh the main-process usage cache, so the next renderer read reflects account imports/switches immediately instead of waiting for the five-minute polling cache.
Provider access and privacy
codex app-server --stdioand readsaccount/rateLimits/read. Superset stores copied Codex profile auth files under its app-owned provider-usage directory, uses temp-file + rename for sensitive writes, backs up the previous active~/.codex/auth.jsonbefore switching, and strips inheritedCODEX_HOME/CODEX_SQLITE_HOMEduring live polling so the switcher and reader use the same default Codex home policy.limits[]buckets.Review follow-up already handled
resets_atvalues.CODEX_HOMEconsistency, and saved-account visibility while Codex usage is unavailable.How I tested it
npx bun@1.3.14 test apps/desktop/src/lib/trpc/routers/provider-usage.test.ts apps/desktop/src/main/lib/provider-usage/index.test.ts apps/desktop/src/main/lib/provider-usage/providers/codex-profiles.test.ts apps/desktop/src/main/lib/provider-usage/providers/claude.test.ts apps/desktop/src/main/lib/provider-usage/providers/codex-app-server.test.ts apps/desktop/src/main/lib/provider-usage/providers/codex.test.ts apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/usageIndicatorPolicy.test.ts apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicator/components/ProviderUsageRow/ProviderUsageRow.test.tsx— 43 passingnpx tsc --noEmit -p apps/desktop/tsconfig.jsonnpx biome check apps/desktop/src/lib/trpc/routers/provider-usage.ts apps/desktop/src/lib/trpc/routers/provider-usage.schema.ts apps/desktop/src/lib/trpc/routers/provider-usage.test.ts apps/desktop/src/main/lib/provider-usage apps/desktop/src/renderer/routes/_authenticated/_dashboard/components/TopBar/components/AIUsageIndicatorgit diff --checkChecklist
type(scope): subject)Summary by CodeRabbit