Add AI provider usage monitoring for Claude and Codex subscriptions#2827
Open
tranquillum wants to merge 5 commits intomanaflow-ai:mainfrom
Open
Add AI provider usage monitoring for Claude and Codex subscriptions#2827tranquillum wants to merge 5 commits intomanaflow-ai:mainfrom
tranquillum wants to merge 5 commits intomanaflow-ai:mainfrom
Conversation
|
@tranquillum is attempting to deploy a commit to the Manaflow Team on Vercel. A member of the Team first needs to authorize it. |
|
To use Codex here, create a Codex account and connect to github. |
|
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 an AI provider usage-monitoring subsystem: provider contracts and registry; Keychain-backed per-account store; shared HTTP/date utilities; Claude and Codex fetchers; a visibility-aware polling controller; sidebar footer/popover, settings and editor UIs; color/threshold settings; tests and docs. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Editor as ProviderAccountEditorSheet
participant Store as ProviderAccountStore
participant Keychain
participant Controller as ProviderAccountsController
User->>Editor: Save account (name + credentials)
Editor->>Store: add(providerId, displayName, secret)
Store->>Keychain: SecItemAdd / SecItemUpdate (JSON payload)
Keychain-->>Store: Success / OSStatus error
Store->>Store: Persist accounts index (UserDefaults)
Store-->>Editor: Return success / error
Editor->>Controller: refreshNow()
Controller-->>Editor: Refresh scheduled/completed
sequenceDiagram
participant Timer
participant Controller as ProviderAccountsController
participant Store as ProviderAccountStore
participant Fetcher
participant HTTP as ProviderHTTP
participant UI as ProviderAccountsFooterPanel
Timer->>Controller: Tick()
Controller->>Store: Load accounts
Store-->>Controller: [ProviderAccount]
loop per account
Controller->>Store: secret(for: account.id)
Store-->>Controller: ProviderSecret
Controller->>Fetcher: fetchUsage(secret)
Fetcher->>HTTP: GET /usage (provider-specific) with headers
HTTP-->>Fetcher: JSON response or error
Fetcher-->>Controller: ProviderUsageWindows / fetch error
Controller->>Controller: Update snapshots & fetchErrors
end
alt status cadence
Controller->>Fetcher: fetchStatus(host, componentFilter)
Fetcher->>HTTP: GET /api/v2/incidents.json
HTTP-->>Fetcher: JSON response
Fetcher-->>Controller: [ProviderIncident] / error
end
Controller->>UI: Publish snapshots & incidents
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
Contributor
Greptile SummaryAdds a sidebar footer panel tracking Claude and Codex subscription usage (Session/Week windows) per account, with secure Keychain credential storage, a 60-second background polling loop, Statuspage.io incident integration, and per-provider settings. The implementation is careful about off-main Keychain I/O, actor-reentrancy on account mutations, header sanitization, and URL allowlisting, and all three previously flagged issues (two Confidence Score: 5/5Safe to merge; all remaining findings are P2 style suggestions that do not affect correctness or security. No P0/P1 issues found. The keychain rollback, actor-reentrancy handling, header injection defenses, and isRefreshing lifecycle are all correctly implemented. The one flagged issue (duplicate UserDefaults key string in ProviderSection) is a minor maintenance hazard with no present runtime impact. Previous review concerns are resolved. Sources/Sidebar/ProviderAccountsFooterPanel.swift — minor collapsed-key string duplication worth cleaning up before future refactors. Important Files Changed
Sequence DiagramsequenceDiagram
participant App as AppDelegate
participant PAC as ProviderAccountsController
participant PAS as ProviderAccountStore
participant KC as Keychain
participant API as Provider API
participant SP as Statuspage.io
App->>PAC: start()
PAC->>PAC: schedule DispatchSourceTimer (60s)
loop Every 60s (or on hidden to visible)
PAC->>PAS: accounts (read, MainActor)
par For each account (TaskGroup)
PAC->>KC: secret(for:) via detached task
KC-->>PAC: ProviderSecret
PAC->>API: fetchUsage(secret)
API-->>PAC: ProviderUsageWindows
end
PAC->>PAC: update snapshots / fetchErrors (MainActor)
alt Every 5th tick or force
par For each providerId (TaskGroup)
PAC->>SP: fetch incidents
SP-->>PAC: [ProviderIncident]
end
PAC->>PAC: update incidents / statusLoaded (MainActor)
end
end
App->>PAC: stop()
PAC->>PAC: cancel timer + currentTask
Reviews (19): Last reviewed commit: "Add unit tests for provider validators, ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Comment on lines
+358
to
+359
| secret: ProviderSecret(fields: ["accessToken": "eyJhbGciOiJ.eyJzdWIi.sig"]) | ||
| ) |
Contributor
There was a problem hiding this comment.
loadSecret is not accessible from tests — compile error
The test calls store.loadSecret(for: account.id), but no method with that name and single-UUID signature exists on ProviderAccountStore. The private method is loadSecret(for accountId: UUID, service: String) (two arguments, private), which @testable import cannot expose. The intended public API is secret(for id: UUID). This call — and the identical one in testIndexPersistsAcrossInstances — would produce a compile error that prevents the entire test target from building.
Suggested change
| secret: ProviderSecret(fields: ["accessToken": "eyJhbGciOiJ.eyJzdWIi.sig"]) | |
| ) | |
| let loaded = try store.secret(for: account.id) |
| XCTAssertEqual(reloaded.accounts.first?.id, createdId) | ||
| XCTAssertEqual(try reloaded.loadSecret(for: createdId).fields["accessToken"], "eyJhbGciOiJ.eyJzdWIi.sig") | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
loadSecret call in second test — compile error
Same issue as in testAddLoadUpdateRemoveRoundTrip: reloaded.loadSecret(for: createdId) does not match any accessible method and would fail to compile. The correct call is reloaded.secret(for: createdId).
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 19
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/ProviderTests.swift`:
- Around line 330-346: The test uses the private helper loadSecret(for:) which
is not part of ProviderAccountStore’s public API; replace calls to
loadSecret(for: account.id) with the public method secret(for:) on
ProviderAccountStore (e.g., use store.secret(for: account.id)) and update
assertions to compare the returned ProviderSecret.fields or unwrap the optional
appropriately, and do the same for the other occurrences (including after update
and after remove) so the tests compile against the public API.
- Around line 15-23: The tests testValidJWTAccessTokenAccepted and
testAccessTokenWithWhitespaceTrimmed use literal JWT-like strings that trigger
the repo secret scanner; instead, construct the token at runtime from
non-sensitive fragments (e.g. build header, payload, signature as separate safe
substrings and concatenate them) or replace with an obviously fake placeholder
that still satisfies CodexValidators.isValidAccessToken (for example ensure the
header begins with "eyJ" by using "eyJ" + "header_tail" and combine with
".payload.signature"); update all similar fixtures in this file (including tests
around lines noted) to use the runtime-assembly pattern so no JWT-shaped
literals are checked into the repo.
In `@docs/providers.md`:
- Around line 13-23: Add a language tag to the fenced code block that starts
with "Sources/Providers/" to satisfy MD040; change the opening ``` to ```text
(or another appropriate language) so the block is explicitly marked as plain
text, ensuring the code fence in the docs/providers.md file is updated
accordingly.
In `@docs/usage-monitoring-setup.md`:
- Around line 198-199: The docs state default thresholds as 60 and 85 but the
actual defaults in ProviderUsageColorSettings.swift are initialized to 85 and
95; update the documentation to match the code (or vice versa if code should be
changed). Locate the default threshold properties (e.g., the Low→Mid and
Mid→High default initializers or constants in the ProviderUsageColorSettings
class/struct around the initializer on lines where LowThreshold and MidThreshold
are set) and make them consistent: either change the doc text to `85` and `95`
or change the initializers in ProviderUsageColorSettings (LowThreshold /
MidThreshold) back to `60` and `85`, ensuring both the README/docs and the code
use the same default numbers.
- Around line 217-219: The documentation's Security note claiming cmux calls
/api/v2/incidents/unresolved.json is incorrect because the implementation in
StatuspageIOFetcher (Sources/Providers/StatuspageIOFetcher.swift, function/class
StatuspageIOFetcher) actually requests /api/v2/incidents.json and filters
unresolved incidents client-side; update the docs to reflect this behavior by
replacing the outdated endpoint with /api/v2/incidents.json and note that
unresolved incidents are filtered client-side by StatuspageIOFetcher, or
alternatively change the implementation to call
/api/v2/incidents/unresolved.json if you prefer server-side filtering—ensure the
doc string references StatuspageIOFetcher and the chosen behavior consistently.
In `@Sources/AppDelegate.swift`:
- Around line 2647-2648: The startup is registering provider monitoring
unconditionally; guard it so tests don't start background work: only call
ProviderAccountStore.shared and ProviderAccountsController.shared.start() when
not running under tests by wrapping both of those calls with a check like if
!isRunningUnderXCTest (or an explicit test opt-in env var) so
ProviderAccountsController.start() is skipped during default XCTest launches.
In `@Sources/cmuxApp.swift`:
- Around line 6180-6191: The catch path after calling
ProviderAccountStore.remove(id:) fails to call
ProviderAccountsController.shared.refreshNow(), so when remove(id:) mutates the
in-memory accounts then throws the UI state isn't refreshed; update the Button
action closure (the do-catch around ProviderAccountStore.shared.remove(id:)) to
ensure ProviderAccountsController.shared.refreshNow() is called regardless of
success — either move refreshNow() out of the do-catch to run after it or
explicitly call refreshNow() inside the catch before setting
providerAccountToRemove = nil so snapshots/incidents for the removed account are
refreshed.
In `@Sources/Providers/ClaudeProvider.swift`:
- Line 17: The current validation closures for sessionKey and orgId accept
whitespace-only strings; update the validators to trim whitespace/newlines and
reject empty results (use trimmingCharacters(in:
.whitespacesAndNewlines).isEmpty) and also assign/store the trimmed string
wherever sessionKey and orgId are captured or persisted (so downstream callers
receive trimmed values). Apply this change to the validate closures around the
current occurrence containing validate: { !$0.isEmpty } and the other occurrence
referenced at lines 48–52 (same sessionKey/orgId handling).
- Line 14: Replace the bare placeholder string literals in ClaudeProvider.swift
with localized strings using String(localized:_, defaultValue:_): find the two
occurrences of placeholder: "sk-ant-sid01-…" (the TextField/initializer
placeholders in ClaudeProvider) and change each to something like
String(localized: "claude.placeholder.apiKey", defaultValue: "sk-ant-sid01-…");
register matching localization keys ("claude.placeholder.apiKey", etc.) in your
Localizable.strings (or strings file) so the UI uses localized values instead of
hard-coded literals.
In `@Sources/Providers/ClaudeUsageFetcher.swift`:
- Around line 60-75: Clamp the parsed utilization values to the 0...100 range
before constructing ProviderUsageWindow so out-of-range values cannot flow into
the UI; specifically, after computing fiveHourUtil and sevenDayUtil in
ClaudeUsageFetcher (the variables named fiveHourUtil and sevenDayUtil), replace
their raw int values with clamped versions (e.g. let fiveHourUtilClamped =
max(0, min(100, fiveHourUtil)) and similarly for sevenDayUtil) and use those
clamped values when creating ProviderUsageWindow and returning
ProviderUsageWindows.
In `@Sources/Providers/CodexProvider.swift`:
- Line 35: Replace the bare placeholder string literals used in the account
editor (the placeholder: "eyJhbGciOi…" occurrences) with localized strings; for
each placeholder parameter in the CodexProvider view/editor use
NSLocalizedString (or your app's localization helper) with distinct keys like
"CodexProvider.apiKeyPlaceholder" and "CodexProvider.otherPlaceholder" and
appropriate comment text, update Localizable.strings entries for those keys, and
ensure both placeholder occurrences (the two placeholder parameters) are
switched to use those localization keys.
In `@Sources/Providers/CodexUsageFetcher.swift`:
- Around line 71-83: In parseWindow(_:), stop coercing malformed timing fields
to 0: check dict for the presence of "limit_window_seconds" and
"reset_after_seconds" and if present require Self.doubleValue(...) to return a
value, otherwise throw CodexUsageFetchError.decoding; if a key is absent treat
it as unknown (use nil or default behavior) and only use the parsed doubles to
compute windowSeconds and resetsAt (and still compute resetsAt as
Date(timeIntervalSinceNow: resetsInSeconds) when a valid resetsInSeconds
exists). Update parseWindow, keeping references to ProviderUsageWindow,
Self.doubleValue, and CodexUsageFetchError.decoding.
In `@Sources/Providers/ProviderHTTP.swift`:
- Around line 52-56: The catch block wrapping the await session.data(for:
request) currently converts all errors into ProviderHTTPError.network; update it
to preserve task cancellation by checking if the caught error is a
CancellationError and rethrowing it directly, otherwise wrap the error in
ProviderHTTPError.network. Locate the do/catch around session.data(for: request)
in ProviderHTTP and replace the single catch-throw with a conditional that tests
for CancellationError (or uses Task.isCancelled) before throwing
ProviderHTTPError.network.
- Around line 26-29: The shared URLSession created using
URLSessionConfiguration.ephemeral currently leaves cookie/cache handling
enabled, which can leak in-memory cookies between requests; update the
configuration before returning the session by disabling cookie processing and
caching: set httpShouldSetCookies = false, httpCookieStorage = nil (or
httpCookieAcceptPolicy = .never), set urlCache = nil and requestCachePolicy =
.reloadIgnoringLocalCacheData on the URLSessionConfiguration (while keeping
timeoutIntervalForRequest and timeoutIntervalForResource as-is) so the returned
URLSession(configuration: config) has no automatic cookie or cache handling.
In `@Sources/Sidebar/ProviderAccountsFooterPanel.swift`:
- Around line 347-375: The hard-coded countdown and tooltip strings in metaText
(and the related tooltipText, formatResetTooltip, formatResetVerbose,
relativeCountdown helpers) must be replaced with localized messages instead of
composing English fragments; update these functions to call String(localized:
"...") with appropriate keys (e.g. "reset.countdown.hours",
"reset.countdown.minutes", "reset.countdown.days",
"reset.countdown.lessThanOneMinute", "reset.countdown.inPrefix", etc.) and move
any pluralization into ICU-style .one/.other variants in Localizable.xcstrings
so hours/minutes/days and the "<1m" and "in …" patterns are expressed via
localized templates; ensure you pass numeric values into the localized format
(or use a plural-aware formatter) rather than building "h/m/d" pieces manually
so translations and plural forms render correctly in all locales.
In `@Sources/Sidebar/ProviderAccountsPopover.swift`:
- Around line 299-308: incident.impact and incident.status are raw API tokens
and must be converted to localized, user-facing labels before rendering; create
mapping helpers (e.g., impactLabel(for:) and statusLabel(for:)) or an Incident
extension that maps the raw values to LocalizedStringKey/NSLocalizedString keys,
update the HStack to call those helpers (replace Text(incident.impact) and
Text(incident.status) with Text(impactLabel(for: incident.impact)) and
Text(statusLabel(for: incident.status))), and ensure all mapped strings are
added to localization files.
- Around line 264-267: The popover currently uses the same fallback text for any
missing reset timestamp which causes the Week row to incorrectly show “Session
not started”; update ProviderAccountsPopover so the Session row only shows
"Session not started" when the session-specific timestamp (e.g., sessionReset or
sessionResetDate) is missing, and the Week row uses a different fallback such as
"Weekly reset unknown" (or nothing) when the week-specific timestamp (e.g.,
weeklyReset or weekResetDate) is missing; implement this by branching on the
specific timestamp fields in the view that renders the two rows and using
separate localized strings for each fallback.
- Around line 221-223: Replace the concatenation of separately localized prefix
and dynamic content with a single localized, interpolated string: instead of
Text("\(String(localized: "providers.accounts.popover.fetchedAt", defaultValue:
"Updated")) \(formattedTime(snapshot.fetchedAt))") use a single
String(localized: ...) that includes the formattedTime(snapshot.fetchedAt)
interpolation in the defaultValue (and do the same for the "Resets" line
referenced at lines 259-260). Update the localization keys (e.g.
"providers.accounts.popover.fetchedAt" and the resets key) to contain the full
sentence with a placeholder for the interpolated value so word order is correct
for all locales.
In `@Sources/Sidebar/ProviderAccountsSettingsSection.swift`:
- Around line 74-79: Create a single localized string for the whole summary
(e.g., key providers.accounts.footer.summary) instead of concatenating
fragments; add a new localization with a sensible default like "Sess %d · Week
%d" (or full English "Sess %d · Week %d") and replace the current Text that
builds the string by concatenation with one formatted/localized call that
injects snapshot.session.utilization and snapshot.week.utilization (use
String(format: String(localized: "providers.accounts.footer.summary",
defaultValue: "..."), snapshot.session.utilization, snapshot.week.utilization)
or the SwiftUI-localized-interpolation equivalent) in
ProviderAccountsSettingsSection where the Text currently uses
snapshot.session.utilization and snapshot.week.utilization.
🪄 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: 8e8a623e-fe8e-46c9-9fc9-d389f4a088d1
⛔ Files ignored due to path filters (10)
docs/assets/usage-monitoring-add-profile.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-editor-claude.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-editor-codex.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-overview.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-claude.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-codex.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-settings-empty.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-settings-filled.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-sidebar-collapsed.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-sidebar.pngis excluded by!**/*.png
📒 Files selected for processing (26)
GhosttyTabs.xcodeproj/project.pbxprojREADME.mdResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/Providers/ClaudeProvider.swiftSources/Providers/ClaudeUsageFetcher.swiftSources/Providers/CodexProvider.swiftSources/Providers/CodexUsageFetcher.swiftSources/Providers/ProviderAccount.swiftSources/Providers/ProviderAccountStore.swiftSources/Providers/ProviderAccountsController.swiftSources/Providers/ProviderHTTP.swiftSources/Providers/ProviderISO8601DateParser.swiftSources/Providers/ProviderRegistry.swiftSources/Providers/ProviderUsageColorSettings.swiftSources/Providers/StatuspageIOFetcher.swiftSources/Providers/UsageProvider.swiftSources/Sidebar/ProviderAccountEditorSheet.swiftSources/Sidebar/ProviderAccountsFooterPanel.swiftSources/Sidebar/ProviderAccountsPopover.swiftSources/Sidebar/ProviderAccountsSettingsSection.swiftSources/cmuxApp.swiftcmuxTests/ProviderTests.swiftdocs/providers.mddocs/usage-monitoring-setup.md
tranquillum
force-pushed
the
feature/agents-usage-monitoring-sidebar-panel
branch
from
300a72b to
c0de990
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (2)
Sources/Sidebar/ProviderAccountsFooterPanel.swift (1)
347-537:⚠️ Potential issue | 🟠 MajorLocalize countdown/reset copy instead of composing English fragments.
metaText,tooltipText,formatResetTooltip,formatResetVerbose, andrelativeCountdownstill hard-code fragments liked/h/m,in,<1m, and—. This will not translate correctly and bypasses locale-specific plural forms.Suggested direction
- return "\(days)d" + return String( + localized: "providers.accounts.countdown.days.short", + defaultValue: "\(days)d" + ) - return "\(absolute) (\(relative))" + return String( + localized: "providers.accounts.reset.verbose", + defaultValue: "\(absolute) (\(relative))" + )Also introduce plural-aware keys (e.g.,
.one/.other) inResources/Localizable.xcstringsfor day/hour/minute units and reset phrasing.As per coding guidelines, “All user-facing strings must be localized… Never use bare string literals in SwiftUI Text(), Button(), alert titles, or other UI components.”
Based on learnings, pluralized strings should use ICU-style.oneand.otherkeys.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Sidebar/ProviderAccountsFooterPanel.swift` around lines 347 - 537, The code currently composes English fragments directly (e.g., "d/h/m", "in", "<1m", "—") inside metaText, tooltipText, formatResetTooltip, formatResetVerbose, and relativeCountdown; replace these hard-coded fragments with localized, plural-aware strings from Localizable.xcstrings (ICU/.one/.other style) and use Swift's localization APIs (String(localized:...) or NSLocalizedString with plural variants) to format days/hours/minutes and the "in" / "resets" / "not started" connectors; add new keys such as providers.accounts.countdown.days, .hours, .minutes, providers.accounts.countdown.lessThanOneMinute, providers.accounts.usage.placeholder (for "—"), and providers.accounts.usage.resets.{one,other} and update metaText, tooltipText, formatResetTooltip, formatResetVerbose, and relativeCountdown to fetch and format those localized keys rather than concatenating English fragments.Sources/Sidebar/ProviderAccountsSettingsSection.swift (1)
74-74:⚠️ Potential issue | 🟠 MajorUse a key-based localized summary string (not inline localized text).
Line 74 should use a stable localization key with
defaultValueso translators can manage the full sentence reliably.🌐 Proposed fix
- Text(String(localized: "Session \(snapshot.session.utilization)% · Week \(snapshot.week.utilization)%")) + Text( + String( + localized: "providers.accounts.settings.summary", + defaultValue: "Session \(snapshot.session.utilization)% · Week \(snapshot.week.utilization)%" + ) + )Also add
providers.accounts.settings.summarytoResources/Localizable.xcstringswith English and Japanese translations.As per coding guidelines, "All user-facing strings must be localized. Use
String(localized: "key.name", defaultValue: "English text")for every string shown in the UI..."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Sidebar/ProviderAccountsSettingsSection.swift` at line 74, Replace the inline localized Text call in ProviderAccountsSettingsSection (the Text(String(localized: "Session \(snapshot.session.utilization)% · Week \(snapshot.week.utilization)%"))) with a key-based localized string using String(localized: "providers.accounts.settings.summary", defaultValue: "Session %d% · Week %d%") (or appropriate placeholder syntax for integers) and pass snapshot.session.utilization and snapshot.week.utilization as the format arguments; then add the key providers.accounts.settings.summary to Resources/Localizable.xcstrings with English and Japanese translations for the full sentence so translators can manage it.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Providers/ClaudeUsageFetcher.swift`:
- Around line 35-43: Check for an empty orgId before attempting percent-encoding
and URL creation: if secret.fields["orgId"] (the orgId variable used in
ClaudeUsageFetcher) is empty or only whitespace, throw
ClaudeUsageFetchError.invalidOrgId immediately; then proceed to create
encodedOrgId and build the URL as currently done (the guard that produces
encodedOrgId and url should assume orgId is non-empty). Ensure the check uses
the same orgId variable and preserves the existing error type
(ClaudeUsageFetchError.invalidOrgId) so requests like /organizations//usage
cannot be constructed.
- Around line 52-58: The catch-all currently maps any non-HTTP/network
ProviderHTTPError (e.g., ProviderHTTPError.badResponse) to
ClaudeUsageFetchError.decoding; change the catch sequence in
Sources/Providers/ClaudeUsageFetcher.swift to explicitly handle
ProviderHTTPError.badResponse (map it to a new or existing ClaudeUsageFetchError
case such as .badResponse or .transport) before the generic catch, and narrow
the final catch to only map decoding failures (e.g., catch let err as
DecodingError { throw ClaudeUsageFetchError.decoding(err) }) while converting
any remaining unknown errors to a distinct ClaudeUsageFetchError (e.g., .unknown
or .other) so transport/protocol failures are not misclassified; update any
ClaudeUsageFetchError enum accordingly if needed.
In `@Sources/Providers/CodexUsageFetcher.swift`:
- Around line 54-65: The catch-all currently maps all thrown errors from
ProviderHTTP.getJSONObject to CodexUsageFetchError.decoding, which incorrectly
converts CancellationError into decoding errors; update the error handling in
the do/catch around ProviderHTTP.getJSONObject to rethrow CancellationError
immediately (check for Swift's CancellationError or Task.isCancelled) before
mapping other ProviderHTTPError cases to CodexUsageFetchError (.http, .network,
.decoding). Apply the same change in the analogous StatuspageIOFetcher catch
blocks so CancellationError is propagated unchanged instead of being wrapped.
In `@Sources/Providers/StatuspageIOFetcher.swift`:
- Around line 26-34: The catch-all in the StatuspageIOFetcher async fetch block
is converting CancellationError into StatuspageIOFetchError.decoding; update the
error handling around ProviderHTTP.getJSONObject(url:session:) to preserve task
cancellation by explicitly catching CancellationError and rethrowing it (or
using `throw error`) before the generic `catch { ... }` so only non-cancellation
errors map to StatuspageIOFetchError.decoding; reference
ProviderHTTP.getJSONObject, CancellationError, and
StatuspageIOFetchError.decoding when making the change.
---
Duplicate comments:
In `@Sources/Sidebar/ProviderAccountsFooterPanel.swift`:
- Around line 347-537: The code currently composes English fragments directly
(e.g., "d/h/m", "in", "<1m", "—") inside metaText, tooltipText,
formatResetTooltip, formatResetVerbose, and relativeCountdown; replace these
hard-coded fragments with localized, plural-aware strings from
Localizable.xcstrings (ICU/.one/.other style) and use Swift's localization APIs
(String(localized:...) or NSLocalizedString with plural variants) to format
days/hours/minutes and the "in" / "resets" / "not started" connectors; add new
keys such as providers.accounts.countdown.days, .hours, .minutes,
providers.accounts.countdown.lessThanOneMinute,
providers.accounts.usage.placeholder (for "—"), and
providers.accounts.usage.resets.{one,other} and update metaText, tooltipText,
formatResetTooltip, formatResetVerbose, and relativeCountdown to fetch and
format those localized keys rather than concatenating English fragments.
In `@Sources/Sidebar/ProviderAccountsSettingsSection.swift`:
- Line 74: Replace the inline localized Text call in
ProviderAccountsSettingsSection (the Text(String(localized: "Session
\(snapshot.session.utilization)% · Week \(snapshot.week.utilization)%"))) with a
key-based localized string using String(localized:
"providers.accounts.settings.summary", defaultValue: "Session %d% · Week %d%")
(or appropriate placeholder syntax for integers) and pass
snapshot.session.utilization and snapshot.week.utilization as the format
arguments; then add the key providers.accounts.settings.summary to
Resources/Localizable.xcstrings with English and Japanese translations for the
full sentence so translators can manage it.
🪄 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: 557151b6-f4be-4ffc-a06a-73205d0d2f69
⛔ Files ignored due to path filters (10)
docs/assets/usage-monitoring-add-profile.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-editor-claude.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-editor-codex.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-overview.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-claude.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-codex.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-settings-empty.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-settings-filled.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-sidebar-collapsed.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-sidebar.pngis excluded by!**/*.png
📒 Files selected for processing (26)
GhosttyTabs.xcodeproj/project.pbxprojREADME.mdResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/Providers/ClaudeProvider.swiftSources/Providers/ClaudeUsageFetcher.swiftSources/Providers/CodexProvider.swiftSources/Providers/CodexUsageFetcher.swiftSources/Providers/ProviderAccount.swiftSources/Providers/ProviderAccountStore.swiftSources/Providers/ProviderAccountsController.swiftSources/Providers/ProviderHTTP.swiftSources/Providers/ProviderISO8601DateParser.swiftSources/Providers/ProviderRegistry.swiftSources/Providers/ProviderUsageColorSettings.swiftSources/Providers/StatuspageIOFetcher.swiftSources/Providers/UsageProvider.swiftSources/Sidebar/ProviderAccountEditorSheet.swiftSources/Sidebar/ProviderAccountsFooterPanel.swiftSources/Sidebar/ProviderAccountsPopover.swiftSources/Sidebar/ProviderAccountsSettingsSection.swiftSources/cmuxApp.swiftcmuxTests/ProviderTests.swiftdocs/providers.mddocs/usage-monitoring-setup.md
✅ Files skipped from review due to trivial changes (8)
- README.md
- Sources/Providers/ProviderRegistry.swift
- Sources/Providers/ProviderISO8601DateParser.swift
- docs/providers.md
- docs/usage-monitoring-setup.md
- cmuxTests/ProviderTests.swift
- Sources/Providers/UsageProvider.swift
- Sources/Providers/ProviderAccountsController.swift
🚧 Files skipped from review as they are similar to previous changes (8)
- Sources/Providers/ClaudeProvider.swift
- Sources/Providers/CodexProvider.swift
- Sources/Providers/ProviderHTTP.swift
- GhosttyTabs.xcodeproj/project.pbxproj
- Sources/Providers/ProviderAccount.swift
- Sources/Sidebar/ProviderAccountEditorSheet.swift
- Sources/Sidebar/ProviderAccountsPopover.swift
- Sources/Providers/ProviderUsageColorSettings.swift
tranquillum
force-pushed
the
feature/agents-usage-monitoring-sidebar-panel
branch
from
c0de990 to
e8789f4
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
Sources/Providers/ClaudeProvider.swift (1)
17-17:⚠️ Potential issue | 🟠 MajorNormalize and tighten credential whitespace handling.
Validation currently allows leading/trailing whitespace in otherwise non-empty values (for both
sessionKeyandorgId). If raw values are persisted/sent, this can pass validation but fail provider auth. Please trim at save/use boundaries and reject whitespace-containingorgIdvalues.🔧 Suggested adjustment in this file
CredentialField( id: "sessionKey", label: String(localized: "claude.accounts.editor.sessionKey", defaultValue: "Session key"), placeholder: String(localized: "claude.accounts.editor.sessionKey.placeholder", defaultValue: "sk-ant-sid01-…"), isSecret: true, helpText: String(localized: "claude.accounts.editor.sessionKey.help", defaultValue: "From claude.ai cookies"), - validate: { !$0.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty } + validate: { + let trimmed = $0.trimmingCharacters(in: .whitespacesAndNewlines) + return !trimmed.isEmpty && trimmed.rangeOfCharacter(from: .whitespacesAndNewlines) == nil + } ), @@ static func isValidOrgId(_ orgId: String) -> Bool { let trimmed = orgId.trimmingCharacters(in: .whitespacesAndNewlines) return !trimmed.isEmpty && !trimmed.contains("..") + && trimmed.rangeOfCharacter(from: .whitespacesAndNewlines) == nil && trimmed.rangeOfCharacter(from: segmentReserved) == nil } }Also applies to: 48-53
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Providers/ClaudeProvider.swift` at line 17, The current validate closure allows values with leading/trailing whitespace; update the validation for sessionKey and orgId (the validate: { ... } closures) to trim input when validating and to reject orgId values that contain any internal whitespace (e.g., ensure trimmed != "" and orgId contains no whitespace characters). Additionally, ensure credentials are trimmed at save/use boundaries by trimming sessionKey and orgId where they are persisted or sent (e.g., in the code paths that store or use these values) so raw values never include surrounding whitespace.Sources/Sidebar/ProviderAccountsFooterPanel.swift (1)
347-374:⚠️ Potential issue | 🟠 MajorLocalize/reset-countdown copy is still assembled from English fragments.
metaText,tooltipText,formatResetTooltip(_:),formatResetVerbose(_:), andrelativeCountdown(hours:)still construct user-facing text with hard-coded tokens/order (d/h/m,in,<1m, em dash). This breaks localization and plural handling in non-English locales.As per coding guidelines, "All user-facing strings must be localized... Never use bare string literals in SwiftUI
Text(),Button(), alert titles, or other UI components," and based on learnings, pluralized strings should use ICU-style.one/.otherkeys.Also applies to: 429-437, 470-537
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Sidebar/ProviderAccountsFooterPanel.swift` around lines 347 - 374, metaText, tooltipText, formatResetTooltip(_:), formatResetVerbose(_:), and relativeCountdown(hours:) build user-facing strings by concatenating English fragments (e.g., "d/h/m", "in", "<1m", em dash), which prevents localization and proper pluralization; update these functions to use localized format keys (Localizable.strings and Localizable.stringsdict with ICU plural rules) instead of assembling tokens at runtime: create localized templates for day/hour/minute variants (e.g., "{count, plural, one {# day} other {# days}}", "{hours, plural {...}}", and combined templates for "in X" and "<1m" fallback), replace hard-coded symbols like em dash with a localized empty-state string, and call NSLocalizedString/Locale-aware formatting from metaText, tooltipText, formatResetTooltip(_:), formatResetVerbose(_:), and relativeCountdown(hours:) to produce fully localized, plural-aware labels.
🧹 Nitpick comments (6)
Sources/Providers/ProviderAccountStore.swift (4)
126-132: Silent data loss on index decode failure.If the persisted JSON is corrupted,
loadIndex()silently returns an empty array. Users would lose all configured accounts with no indication of what happened.Consider logging in DEBUG to aid troubleshooting:
🛡️ Suggested: Log decode failures in DEBUG
private func loadIndex() -> [ProviderAccount] { - guard let data = userDefaults.data(forKey: indexKey), - let decoded = try? JSONDecoder().decode([ProviderAccount].self, from: data) else { + guard let data = userDefaults.data(forKey: indexKey) else { return [] } - return decoded + do { + return try JSONDecoder().decode([ProviderAccount].self, from: data) + } catch { + `#if` DEBUG + dlog("ProviderAccountStore.loadIndex: decode failed – \(error.localizedDescription)") + `#endif` + return [] + } }As per coding guidelines:
dlogcalls must be wrapped in#if DEBUG/#endif.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Providers/ProviderAccountStore.swift` around lines 126 - 132, loadIndex currently swallows JSON decode failures and returns an empty array; update loadIndex to catch and log decoding errors (include the decoding error and the faulty data/context) instead of silently returning [] so failures are visible during debugging, using the userDefaults.data(forKey: indexKey) and JSONDecoder().decode([ProviderAccount].self, from:) code paths; emit the diagnostic via dlog and ensure the dlog call is wrapped in `#if` DEBUG / `#endif` as per guidelines.
177-186: SecItemDelete result ignored before retry.On
errSecDuplicateItem, the delete result (line 179) is discarded. If deletion fails for a reason other than "not found," the subsequent add will also fail — that error will propagate, but the root cause (failed delete) is lost.Minor improvement for debuggability:
♻️ Optional: Check delete status before retry
if status == errSecDuplicateItem { let deleteQuery = Self.matchQuery(service: service, accountId: accountId) - SecItemDelete(deleteQuery as CFDictionary) + let deleteStatus = SecItemDelete(deleteQuery as CFDictionary) + guard deleteStatus == errSecSuccess || deleteStatus == errSecItemNotFound else { + throw ProviderAccountStoreError.keychain(deleteStatus) + } let retryStatus = SecItemAdd(query as CFDictionary, nil)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Providers/ProviderAccountStore.swift` around lines 177 - 186, When handling errSecDuplicateItem in the add flow, check the return value of SecItemDelete(Self.matchQuery(service: service, accountId: accountId) as CFDictionary) and react to failures before retrying SecItemAdd; if deleteStatus != errSecSuccess and deleteStatus != errSecItemNotFound then throw ProviderAccountStoreError.keychain(deleteStatus) so the root cause (failed delete) is preserved, otherwise proceed to call SecItemAdd and handle its retryStatus as before.
91-114: Acknowledged edge case: stale index entry could resurrect on next launch.As the comment notes (lines 101–106), if
saveIndexfails after keychain deletion, the next launch would restore an account with no secret. Callers ofsecret(for:)would then receive a keychain error.Consider adding cleanup logic in
reload()to prune accounts whose secrets are missing:♻️ Optional: Prune orphaned accounts on reload
func reload() { - accounts = loadIndex() + let loaded = loadIndex() + // Prune accounts whose keychain secrets are missing (e.g., from a prior + // partial-remove failure or manual keychain cleanup). + let valid = loaded.filter { account in + let service = keychainServiceResolver(account.providerId) + return (try? loadSecret(for: account.id, service: service)) != nil + } + if valid.count != loaded.count { + try? saveIndex(valid) + } + accounts = valid }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Providers/ProviderAccountStore.swift` around lines 91 - 114, Add pruning of orphaned accounts in reload() so accounts whose keychain secret cannot be found are removed: after loading the persisted index in reload(), iterate the loaded accounts and for each account call secret(for:) (or use deleteSecret/checkSecret helper) to detect missing secrets, build a filtered list excluding accounts whose secret lookup fails with a not-found/keychain-missing error, set accounts = filteredList and call saveIndex(filteredList) if any were removed, and ensure errors other than "not found" are propagated or logged; reference remove(id:), deleteSecret(for:), saveIndex(_:) and secret(for:) to locate related logic.
15-20: Public mutable resolver could be unexpectedly modified.
keychainServiceResolveris avarthat any caller can replace. While this is useful for testing, production code could accidentally or maliciously swap the resolver, potentially misdirecting secrets to wrong keychain services.Consider making it private with an internal/testing-only setter, or marking it with
@_spi(Testing):♻️ Suggested: Restrict mutability to tests
- var keychainServiceResolver: (String) -> String = { providerId in + private(set) var keychainServiceResolver: (String) -> String = { providerId in ProviderRegistry.provider(id: providerId)?.keychainService ?? "com.cmuxterm.app.\(providerId)-accounts" } + + `#if` DEBUG + /// Test-only hook to override keychain service resolution. + func setKeychainServiceResolverForTesting(_ resolver: `@escaping` (String) -> String) { + keychainServiceResolver = resolver + } + `#endif`🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Providers/ProviderAccountStore.swift` around lines 15 - 20, The public mutable keychainServiceResolver allows external code to replace the resolver; change it so callers cannot mutate it by making it non-publicly settable (e.g., declare keychainServiceResolver as public private(set) or internal) and add a single test-only setter function (e.g., setKeychainServiceResolver(_:)) marked for testing visibility (use `@_spi`(Testing) or a test-only access level) so tests can override it; update references to ProviderRegistry.provider(...) inside keychainServiceResolver as-is and ensure only the new test setter exposes mutation.Sources/Providers/ProviderUsageColorSettings.swift (1)
173-182: Silent black fallback on color-space conversion failure.If
NSColor(self).usingColorSpace(.sRGB)returnsnil, this silently returns black(0, 0, 0). During interpolation, this could produce unexpected dark tints if a user-configured hex is somehow invalid or uses an incompatible color space.Consider logging in DEBUG or returning a fallback that matches the default colors:
♻️ Optional: Add debug logging for failed conversion
var rgbComponents: (red: Double, green: Double, blue: Double) { guard let nsColor = NSColor(self).usingColorSpace(.sRGB) else { + `#if` DEBUG + dlog("Color.rgbComponents: failed to convert to sRGB, returning black fallback") + `#endif` return (red: 0, green: 0, blue: 0) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Providers/ProviderUsageColorSettings.swift` around lines 173 - 182, The rgbComponents getter should not silently return black when NSColor(self).usingColorSpace(.sRGB) is nil; update the nil branch in rgbComponents (the NSColor(self).usingColorSpace(.sRGB) check) to (1) emit a debug log (e.g., using os_log or a debug-only logger) describing the failed color-space conversion and the original Color/hex, and (2) return a safer fallback RGB tuple that matches your default UI color (instead of (0,0,0)) — for example use the default color constants used elsewhere in this module or derive components from a known-safe color (the project’s default) so interpolation doesn’t produce an unexpected black tint.Sources/cmuxApp.swift (1)
6516-6523: Consider moving the shared Settings primitives into their own file.Now that these types are reused outside
cmuxApp.swift, keeping them here makes this already-large file harder to navigate and broadens the app entrypoint’s responsibilities. A small dedicatedSettingsLayout-style file would keep the reuse without further growing this file.Also applies to: 6523-6555, 6586-6636, 6700-6739
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 6516 - 6523, The Settings layout view primitives (e.g., SettingsSectionHeader, the various Settings* view types, and SettingsConfigurationReview) are now reused outside cmuxApp.swift and should be moved into a dedicated file to reduce the app entrypoint size and improve navigation; create a new file (e.g., SettingsLayout.swift) and relocate the struct/type declarations and any private helper types they need, update their access levels if necessary (make them internal/public rather than file-private), and update imports/usages in other files to reference the moved types so existing references compile without changing behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Sidebar/ProviderAccountsFooterPanel.swift`:
- Around line 182-185: The statusText getter currently returns a bare "…"
literal when !isStatusLoaded; change this to use a localized key instead (e.g.
NSLocalizedString("sidebar.status.loading", comment: "Loading status
placeholder") or return a LocalizedStringKey) and update any callers (or the
Text view) to consume the localized value (e.g., Text(LocalizedStringKey) or
Text(NSLocalizedString(...))). Locate the statusText property and replace the
raw "…" with the chosen localized key, and add the corresponding entry to the
Localizable.strings file (key "sidebar.status.loading") so the UI no longer
contains a bare string literal.
---
Duplicate comments:
In `@Sources/Providers/ClaudeProvider.swift`:
- Line 17: The current validate closure allows values with leading/trailing
whitespace; update the validation for sessionKey and orgId (the validate: { ...
} closures) to trim input when validating and to reject orgId values that
contain any internal whitespace (e.g., ensure trimmed != "" and orgId contains
no whitespace characters). Additionally, ensure credentials are trimmed at
save/use boundaries by trimming sessionKey and orgId where they are persisted or
sent (e.g., in the code paths that store or use these values) so raw values
never include surrounding whitespace.
In `@Sources/Sidebar/ProviderAccountsFooterPanel.swift`:
- Around line 347-374: metaText, tooltipText, formatResetTooltip(_:),
formatResetVerbose(_:), and relativeCountdown(hours:) build user-facing strings
by concatenating English fragments (e.g., "d/h/m", "in", "<1m", em dash), which
prevents localization and proper pluralization; update these functions to use
localized format keys (Localizable.strings and Localizable.stringsdict with ICU
plural rules) instead of assembling tokens at runtime: create localized
templates for day/hour/minute variants (e.g., "{count, plural, one {# day} other
{# days}}", "{hours, plural {...}}", and combined templates for "in X" and "<1m"
fallback), replace hard-coded symbols like em dash with a localized empty-state
string, and call NSLocalizedString/Locale-aware formatting from metaText,
tooltipText, formatResetTooltip(_:), formatResetVerbose(_:), and
relativeCountdown(hours:) to produce fully localized, plural-aware labels.
---
Nitpick comments:
In `@Sources/cmuxApp.swift`:
- Around line 6516-6523: The Settings layout view primitives (e.g.,
SettingsSectionHeader, the various Settings* view types, and
SettingsConfigurationReview) are now reused outside cmuxApp.swift and should be
moved into a dedicated file to reduce the app entrypoint size and improve
navigation; create a new file (e.g., SettingsLayout.swift) and relocate the
struct/type declarations and any private helper types they need, update their
access levels if necessary (make them internal/public rather than file-private),
and update imports/usages in other files to reference the moved types so
existing references compile without changing behavior.
In `@Sources/Providers/ProviderAccountStore.swift`:
- Around line 126-132: loadIndex currently swallows JSON decode failures and
returns an empty array; update loadIndex to catch and log decoding errors
(include the decoding error and the faulty data/context) instead of silently
returning [] so failures are visible during debugging, using the
userDefaults.data(forKey: indexKey) and
JSONDecoder().decode([ProviderAccount].self, from:) code paths; emit the
diagnostic via dlog and ensure the dlog call is wrapped in `#if` DEBUG / `#endif` as
per guidelines.
- Around line 177-186: When handling errSecDuplicateItem in the add flow, check
the return value of SecItemDelete(Self.matchQuery(service: service, accountId:
accountId) as CFDictionary) and react to failures before retrying SecItemAdd; if
deleteStatus != errSecSuccess and deleteStatus != errSecItemNotFound then throw
ProviderAccountStoreError.keychain(deleteStatus) so the root cause (failed
delete) is preserved, otherwise proceed to call SecItemAdd and handle its
retryStatus as before.
- Around line 91-114: Add pruning of orphaned accounts in reload() so accounts
whose keychain secret cannot be found are removed: after loading the persisted
index in reload(), iterate the loaded accounts and for each account call
secret(for:) (or use deleteSecret/checkSecret helper) to detect missing secrets,
build a filtered list excluding accounts whose secret lookup fails with a
not-found/keychain-missing error, set accounts = filteredList and call
saveIndex(filteredList) if any were removed, and ensure errors other than "not
found" are propagated or logged; reference remove(id:), deleteSecret(for:),
saveIndex(_:) and secret(for:) to locate related logic.
- Around line 15-20: The public mutable keychainServiceResolver allows external
code to replace the resolver; change it so callers cannot mutate it by making it
non-publicly settable (e.g., declare keychainServiceResolver as public
private(set) or internal) and add a single test-only setter function (e.g.,
setKeychainServiceResolver(_:)) marked for testing visibility (use
`@_spi`(Testing) or a test-only access level) so tests can override it; update
references to ProviderRegistry.provider(...) inside keychainServiceResolver
as-is and ensure only the new test setter exposes mutation.
In `@Sources/Providers/ProviderUsageColorSettings.swift`:
- Around line 173-182: The rgbComponents getter should not silently return black
when NSColor(self).usingColorSpace(.sRGB) is nil; update the nil branch in
rgbComponents (the NSColor(self).usingColorSpace(.sRGB) check) to (1) emit a
debug log (e.g., using os_log or a debug-only logger) describing the failed
color-space conversion and the original Color/hex, and (2) return a safer
fallback RGB tuple that matches your default UI color (instead of (0,0,0)) — for
example use the default color constants used elsewhere in this module or derive
components from a known-safe color (the project’s default) so interpolation
doesn’t produce an unexpected black tint.
🪄 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: 0ddbc727-1951-4250-92b1-c9745c2b9866
⛔ Files ignored due to path filters (10)
docs/assets/usage-monitoring-add-profile.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-editor-claude.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-editor-codex.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-overview.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-claude.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-codex.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-settings-empty.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-settings-filled.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-sidebar-collapsed.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-sidebar.pngis excluded by!**/*.png
📒 Files selected for processing (26)
GhosttyTabs.xcodeproj/project.pbxprojREADME.mdResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/Providers/ClaudeProvider.swiftSources/Providers/ClaudeUsageFetcher.swiftSources/Providers/CodexProvider.swiftSources/Providers/CodexUsageFetcher.swiftSources/Providers/ProviderAccount.swiftSources/Providers/ProviderAccountStore.swiftSources/Providers/ProviderAccountsController.swiftSources/Providers/ProviderHTTP.swiftSources/Providers/ProviderISO8601DateParser.swiftSources/Providers/ProviderRegistry.swiftSources/Providers/ProviderUsageColorSettings.swiftSources/Providers/StatuspageIOFetcher.swiftSources/Providers/UsageProvider.swiftSources/Sidebar/ProviderAccountEditorSheet.swiftSources/Sidebar/ProviderAccountsFooterPanel.swiftSources/Sidebar/ProviderAccountsPopover.swiftSources/Sidebar/ProviderAccountsSettingsSection.swiftSources/cmuxApp.swiftcmuxTests/ProviderTests.swiftdocs/providers.mddocs/usage-monitoring-setup.md
✅ Files skipped from review due to trivial changes (9)
- README.md
- Sources/Providers/ProviderISO8601DateParser.swift
- Sources/Providers/ProviderRegistry.swift
- Sources/Providers/ProviderAccount.swift
- docs/usage-monitoring-setup.md
- Sources/Sidebar/ProviderAccountsSettingsSection.swift
- docs/providers.md
- Sources/Sidebar/ProviderAccountsPopover.swift
- cmuxTests/ProviderTests.swift
🚧 Files skipped from review as they are similar to previous changes (8)
- Sources/Providers/CodexProvider.swift
- GhosttyTabs.xcodeproj/project.pbxproj
- Sources/Providers/ProviderHTTP.swift
- Sources/Sidebar/ProviderAccountEditorSheet.swift
- Sources/AppDelegate.swift
- Sources/Providers/StatuspageIOFetcher.swift
- Sources/Providers/CodexUsageFetcher.swift
- Sources/Providers/ProviderAccountsController.swift
tranquillum
force-pushed
the
feature/agents-usage-monitoring-sidebar-panel
branch
from
e8789f4 to
7aed444
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
Sources/Providers/CodexUsageFetcher.swift (1)
47-49: Consider using a more semantically appropriate error for URL construction failure.The hardcoded URL
"https://chatgpt.com/backend-api/wham/usage"failing to parse is not a credentials issue—it's a programmer error. Throwing.invalidCredentialshere could mislead debugging since the error message suggests the access token is missing.This is a minor concern since the URL is hardcoded and will always parse successfully, making this branch effectively unreachable.
♻️ Potential improvement (optional)
guard let url = URL(string: "https://chatgpt.com/backend-api/wham/usage") else { - throw CodexUsageFetchError.invalidCredentials + throw CodexUsageFetchError.decoding // or a new .internalError case }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Providers/CodexUsageFetcher.swift` around lines 47 - 49, The guard that constructs URL("https://chatgpt.com/backend-api/wham/usage") in CodexUsageFetcher.swift should not throw CodexUsageFetchError.invalidCredentials on a parse failure because that misattributes a programmer/URL bug to credentials; add a more appropriate error case (e.g., CodexUsageFetchError.invalidURL or .internalError) to the CodexUsageFetchError enum and replace the throw in the URL guard with that new case so the error accurately reflects a URL construction failure in the method that builds the usage request.Sources/cmuxApp.swift (1)
6435-6437: Make the reset scope explicit for AI usage monitoring.
resetAllSettings()now resets provider thresholds, but it still preserves saved provider accounts / Keychain credentials. That makes “Reset All Settings” ambiguous for the new feature. Either clear provider accounts here too, or explicitly label/document that credentials are intentionally preserved.Based on learnings,
SettingsView.resetAllSettings()must reset newly added AppStorage toggles to defaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/cmuxApp.swift` around lines 6435 - 6437, The reset behavior is ambiguous: update the reset block around WorkspaceTabColorSettings.reset(), ProviderUsageColorSettings.shared.resetToDefaults(), and reloadWorkspaceTabColorSettings() to either also clear stored provider accounts/Keychain entries (call the method that removes saved provider accounts or Keychain credentials) or explicitly document/rename the action to indicate credentials are preserved; additionally update SettingsView.resetAllSettings() to explicitly reset the new AppStorage-backed toggles to their default values so newly added AI usage monitoring switches return to defaults when resetting settings.cmuxTests/ProviderTests.swift (1)
131-141: Force unwrap of optional TimeZone.While
TimeZone(identifier: "UTC")is extremely unlikely to fail, using force unwrap in tests can produce confusing crash reports. Consider using a guard orXCTUnwrap.🛡️ Suggested fix
func testParsedDateHasCorrectComponents() { let date = ProviderISO8601DateParser.parse("2026-04-10T14:30:00Z") XCTAssertNotNil(date) let calendar = Calendar(identifier: .gregorian) - var components = calendar.dateComponents(in: TimeZone(identifier: "UTC")!, from: date!) + let utc = try XCTUnwrap(TimeZone(identifier: "UTC")) + let parsedDate = try XCTUnwrap(date) + let components = calendar.dateComponents(in: utc, from: parsedDate) XCTAssertEqual(components.year, 2026)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmuxTests/ProviderTests.swift` around lines 131 - 141, The test testParsedDateHasCorrectComponents force-unwraps TimeZone(identifier: "UTC"), which can crash the test; update the test to safely unwrap the timezone (using guard let tz = TimeZone(identifier: "UTC") else { XCTFail("..."); return } or let tz = try XCTUnwrap(TimeZone(identifier: "UTC"))) and then use tz when calling calendar.dateComponents; keep the rest of the assertions unchanged and reference ProviderISO8601DateParser.parse for locating the test.Sources/Providers/ProviderUsageColorSettings.swift (1)
173-182: Consider handling nil color space conversion gracefully.
NSColor(self).usingColorSpace(.sRGB)can return nil for colors that can't be converted (e.g., pattern colors). While unlikely for usage bar colors, the fallback to black(0, 0, 0)might produce unexpected results. Consider logging or using a more visible fallback.💡 Optional: Add debug logging for conversion failures
var rgbComponents: (red: Double, green: Double, blue: Double) { guard let nsColor = NSColor(self).usingColorSpace(.sRGB) else { + `#if` DEBUG + dlog("ProviderUsageColorSettings: Failed to convert color to sRGB") + `#endif` return (red: 0, green: 0, blue: 0) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Providers/ProviderUsageColorSettings.swift` around lines 173 - 182, The rgbComponents getter currently returns black when NSColor(self).usingColorSpace(.sRGB) fails; update rgbComponents to handle conversion failures more gracefully by: first attempting alternative conversions (e.g., NSColor(self).usingColorSpace(.deviceRGB) or using self.cgColor?.components), then falling back to a more visible default (e.g., mid-gray or magenta) rather than silent black, and add a debug log or assertion (use your logging facility or assertionFailure) to record when NSColor(self).usingColorSpace(.sRGB) returns nil so conversion issues are visible; keep references to the rgbComponents property and the NSColor(self).usingColorSpace(.sRGB) call to locate and modify the code.Sources/Sidebar/ProviderAccountsPopover.swift (1)
275-284: Consider locale-aware time formatting.The
DateFormatteruses a fixeden_US_POSIXlocale, which is correct for machine-readable timestamps but not ideal for user-facing display. Since this shows in the "Updated HH:mm:ss" popover text, consider using the user's locale for display formatting.🌐 Suggested fix
private static let timeFormatterHMS: DateFormatter = { let formatter = DateFormatter() - formatter.dateFormat = "HH:mm:ss" - formatter.locale = Locale(identifier: "en_US_POSIX") + formatter.timeStyle = .medium return formatter }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Sidebar/ProviderAccountsPopover.swift` around lines 275 - 284, The time formatter uses a fixed en_US_POSIX locale (timeFormatterHMS) which is machine-oriented; update it to be user-locale-aware by replacing the custom dateFormat with a localized time style (e.g., formatter.timeStyle = .medium) and set formatter.locale = Locale.current (or omit setting locale) so formattedTime(_:) uses a user-friendly localized string; update the static timeFormatterHMS initializer and keep formattedTime(_:) calling Self.timeFormatterHMS.string(from:).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Sidebar/ProviderAccountEditorSheet.swift`:
- Around line 131-135: The catch block in ProviderAccountEditorSheet currently
overrides every failure with a generic errorMessage, which hides actionable
ProviderAccountStoreError details; update the catch to inspect the thrown error
(e.g., if let storeError = error as? ProviderAccountStoreError) and set
errorMessage from the storeError’s localizedDescription or a mapped localized
message for specific cases, otherwise fall back to the existing generic
localized "providers.accounts.error.loadSecret" message so Keychain/not-found
guidance is preserved.
---
Nitpick comments:
In `@cmuxTests/ProviderTests.swift`:
- Around line 131-141: The test testParsedDateHasCorrectComponents force-unwraps
TimeZone(identifier: "UTC"), which can crash the test; update the test to safely
unwrap the timezone (using guard let tz = TimeZone(identifier: "UTC") else {
XCTFail("..."); return } or let tz = try XCTUnwrap(TimeZone(identifier: "UTC")))
and then use tz when calling calendar.dateComponents; keep the rest of the
assertions unchanged and reference ProviderISO8601DateParser.parse for locating
the test.
In `@Sources/cmuxApp.swift`:
- Around line 6435-6437: The reset behavior is ambiguous: update the reset block
around WorkspaceTabColorSettings.reset(),
ProviderUsageColorSettings.shared.resetToDefaults(), and
reloadWorkspaceTabColorSettings() to either also clear stored provider
accounts/Keychain entries (call the method that removes saved provider accounts
or Keychain credentials) or explicitly document/rename the action to indicate
credentials are preserved; additionally update SettingsView.resetAllSettings()
to explicitly reset the new AppStorage-backed toggles to their default values so
newly added AI usage monitoring switches return to defaults when resetting
settings.
In `@Sources/Providers/CodexUsageFetcher.swift`:
- Around line 47-49: The guard that constructs
URL("https://chatgpt.com/backend-api/wham/usage") in CodexUsageFetcher.swift
should not throw CodexUsageFetchError.invalidCredentials on a parse failure
because that misattributes a programmer/URL bug to credentials; add a more
appropriate error case (e.g., CodexUsageFetchError.invalidURL or .internalError)
to the CodexUsageFetchError enum and replace the throw in the URL guard with
that new case so the error accurately reflects a URL construction failure in the
method that builds the usage request.
In `@Sources/Providers/ProviderUsageColorSettings.swift`:
- Around line 173-182: The rgbComponents getter currently returns black when
NSColor(self).usingColorSpace(.sRGB) fails; update rgbComponents to handle
conversion failures more gracefully by: first attempting alternative conversions
(e.g., NSColor(self).usingColorSpace(.deviceRGB) or using
self.cgColor?.components), then falling back to a more visible default (e.g.,
mid-gray or magenta) rather than silent black, and add a debug log or assertion
(use your logging facility or assertionFailure) to record when
NSColor(self).usingColorSpace(.sRGB) returns nil so conversion issues are
visible; keep references to the rgbComponents property and the
NSColor(self).usingColorSpace(.sRGB) call to locate and modify the code.
In `@Sources/Sidebar/ProviderAccountsPopover.swift`:
- Around line 275-284: The time formatter uses a fixed en_US_POSIX locale
(timeFormatterHMS) which is machine-oriented; update it to be user-locale-aware
by replacing the custom dateFormat with a localized time style (e.g.,
formatter.timeStyle = .medium) and set formatter.locale = Locale.current (or
omit setting locale) so formattedTime(_:) uses a user-friendly localized string;
update the static timeFormatterHMS initializer and keep formattedTime(_:)
calling Self.timeFormatterHMS.string(from:).
🪄 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: 4ca2f3b2-4b91-4fa3-b28a-9cb838c022b1
⛔ Files ignored due to path filters (10)
docs/assets/usage-monitoring-add-profile.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-editor-claude.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-editor-codex.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-overview.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-claude.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-codex.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-settings-empty.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-settings-filled.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-sidebar-collapsed.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-sidebar.pngis excluded by!**/*.png
📒 Files selected for processing (26)
GhosttyTabs.xcodeproj/project.pbxprojREADME.mdResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/Providers/ClaudeProvider.swiftSources/Providers/ClaudeUsageFetcher.swiftSources/Providers/CodexProvider.swiftSources/Providers/CodexUsageFetcher.swiftSources/Providers/ProviderAccount.swiftSources/Providers/ProviderAccountStore.swiftSources/Providers/ProviderAccountsController.swiftSources/Providers/ProviderHTTP.swiftSources/Providers/ProviderISO8601DateParser.swiftSources/Providers/ProviderRegistry.swiftSources/Providers/ProviderUsageColorSettings.swiftSources/Providers/StatuspageIOFetcher.swiftSources/Providers/UsageProvider.swiftSources/Sidebar/ProviderAccountEditorSheet.swiftSources/Sidebar/ProviderAccountsFooterPanel.swiftSources/Sidebar/ProviderAccountsPopover.swiftSources/Sidebar/ProviderAccountsSettingsSection.swiftSources/cmuxApp.swiftcmuxTests/ProviderTests.swiftdocs/providers.mddocs/usage-monitoring-setup.md
✅ Files skipped from review due to trivial changes (8)
- README.md
- Sources/Providers/ProviderISO8601DateParser.swift
- Sources/Providers/ProviderHTTP.swift
- Sources/Providers/ProviderRegistry.swift
- Sources/Providers/ProviderAccount.swift
- docs/usage-monitoring-setup.md
- docs/providers.md
- Sources/Providers/UsageProvider.swift
🚧 Files skipped from review as they are similar to previous changes (6)
- Sources/Providers/CodexProvider.swift
- Sources/Providers/ClaudeProvider.swift
- Sources/Providers/ClaudeUsageFetcher.swift
- GhosttyTabs.xcodeproj/project.pbxproj
- Sources/Sidebar/ProviderAccountsSettingsSection.swift
- Sources/Sidebar/ProviderAccountsFooterPanel.swift
tranquillum
force-pushed
the
feature/agents-usage-monitoring-sidebar-panel
branch
from
2fb8e80 to
bd98722
Compare
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (3)
Sources/Providers/ClaudeProvider.swift (2)
11-18:⚠️ Potential issue | 🟠 MajorNormalize
sessionKey, not just validate it.This validator trims only to decide validity; it does not normalize the stored value. A pasted cookie with a trailing newline still passes here, then goes out verbatim in
ClaudeUsageFetcherand turns into a hard-to-diagnose 401. Please trim before persistence, or trim again before building theCookieheader.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Providers/ClaudeProvider.swift` around lines 11 - 18, The sessionKey validator only trims for validation but doesn't normalize the stored value, so trailing whitespace/newlines can cause 401s; update the code that persists or uses the session key (e.g., the CredentialField with id "sessionKey" and/or the code in ClaudeUsageFetcher that builds the Cookie header) to trim() the value (removing leading/trailing whitespace/newlines) before saving or before constructing the Cookie header so the sent cookie is normalized.
48-52:⚠️ Potential issue | 🟡 MinorReject embedded whitespace in
orgId.
trimmingCharacters(in:)only removes edge whitespace, so values likeorg idoruuid\nsuffixstill pass validation and get percent-encoded into a different path. This should fail in the editor instead of becoming a remote 404.Proposed fix
static func isValidOrgId(_ orgId: String) -> Bool { let trimmed = orgId.trimmingCharacters(in: .whitespacesAndNewlines) return !trimmed.isEmpty && !trimmed.contains("..") + && trimmed.rangeOfCharacter(from: .whitespacesAndNewlines) == nil && trimmed.rangeOfCharacter(from: segmentReserved) == nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Providers/ClaudeProvider.swift` around lines 48 - 52, The current validation trims edge whitespace but still allows embedded whitespace (e.g., "org id"), so update isValidOrgId(_ orgId: String) to reject any internal whitespace: after creating trimmed, add a check that trimmed.rangeOfCharacter(from: .whitespacesAndNewlines) == nil (or equivalent) in the boolean chain along with the existing segmentReserved check and non-empty check; reference the function isValidOrgId, the trimmed variable and segmentReserved to locate where to add this additional validation.Sources/Sidebar/ProviderAccountsFooterPanel.swift (1)
347-374:⚠️ Potential issue | 🟠 MajorThe reset/countdown copy is still assembled from English fragments.
metaText,tooltipText,formatResetTooltip,formatResetVerbose, andrelativeCountdownstill emit rawd/h/m,in …, and<1mtext. That keeps both the footer and popover reset strings out ofLocalizable.xcstrings, and plural/order rules can’t be translated cleanly.As per coding guidelines, "All user-facing strings must be localized. Use
String(localized: "key.name", defaultValue: "English text")for every string shown in the UI (labels, buttons, menus, dialogs, tooltips, error messages)," and based on learnings, pluralized strings in this repo should use ICU-style.one/.otherkeys.Also applies to: 428-537
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Sidebar/ProviderAccountsFooterPanel.swift` around lines 347 - 374, The countdown strings (metaText, tooltipText, formatResetTooltip, formatResetVerbose, relativeCountdown) are currently built from hard-coded English fragments like "d/h/m", "in …", and "<1m"; replace these with localized ICU-style strings using String(localized: "key", defaultValue: ...) and plural forms (.one/.other) so all user-facing text is pulled from Localizable.xcstrings; create keys for days/hours/minutes (e.g. "reset.days", "reset.hours", "reset.minutes") and for whole phrases ("reset.in", "reset.less-than-minute") and use those localized templates to format the output in each function (use pluralization for numeric units and avoid concatenating raw "d/h/m" fragments), ensuring metaText returns a localized compact form and tooltipText/formatResetTooltip/formatResetVerbose/relativeCountdown return fully localized sentences.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Providers/CodexUsageFetcher.swift`:
- Around line 92-95: Clamp the computed usedPercent to the 0...100 range before
converting/rounding and building ProviderUsageWindow: compute a clamped value
(e.g., clamp = min(max(usedPercent, 0), 100)) and then use Int(clamp.rounded())
for the utilization passed into ProviderUsageWindow; update the return in the
function that constructs ProviderUsageWindow in CodexUsageFetcher.swift to use
that clamped value instead of the raw usedPercent.
In `@Sources/Providers/ProviderAccountsController.swift`:
- Around line 87-99: scheduleTick/currentTask currently blocks future ticks when
a provider fetch hangs because tick(generation:force:) awaits network calls with
no timeout; wrap those provider fetch awaits in a controller-level timeout so a
slow/hung request fails fast (treated like any other fetch failure) and allows
the task to complete and currentTask/isRefreshing to be cleared. Concretely, add
a small helper (e.g., withTimeout or use a racing Task that throws after N
seconds) and apply it around the provider network calls invoked from
tick(generation:force:) (the same sites referenced in the review where provider
fetches occur), ensure any timeout throws are handled the same as other fetch
errors, and that currentTask = nil and isRefreshing are reset when the
timed-out/failed task finishes.
In `@Sources/Providers/StatuspageIOFetcher.swift`:
- Around line 52-55: The current check uses componentFilter and then always
computes componentNames and returns nil when disjoint, which filters out
page-wide incidents with an empty "components" array; update the logic in the
block that references componentFilter, components, componentNames and dict so
the intersection check runs only when the payload actually includes component
attachments (i.e., only when components is non-empty) — e.g., if
components.isEmpty, skip the disjoint guard and allow the incident through;
otherwise compute componentNames and apply the existing guard
!componentNames.isDisjoint(with: componentFilter).
In `@Sources/Sidebar/ProviderAccountsPopover.swift`:
- Around line 223-225: Replace the hard-coded English timestamp label and POSIX
formatter usage in ProviderAccountsPopover.swift (the Text(...) that uses
formattedTime(snapshot.fetchedAt) and the similar block at 275-284) with a
locale-aware time formatter and an explicit localized string key; specifically,
format snapshot.fetchedAt with a locale-aware Date.FormatStyle or
DateFormatter.localizedString(for: , dateStyle: .none, timeStyle: .short) (so it
respects user locale/12h vs 24h), and embed that formatted time into
String(localized: "provider.updated_at", defaultValue: "Updated {0}") (or
equivalent localized key) instead of the English literal so the UI text is fully
localized. Ensure you update both occurrences (the one at ~223 and the block at
275-284) to use the same localized key and locale-aware formatting function.
---
Duplicate comments:
In `@Sources/Providers/ClaudeProvider.swift`:
- Around line 11-18: The sessionKey validator only trims for validation but
doesn't normalize the stored value, so trailing whitespace/newlines can cause
401s; update the code that persists or uses the session key (e.g., the
CredentialField with id "sessionKey" and/or the code in ClaudeUsageFetcher that
builds the Cookie header) to trim() the value (removing leading/trailing
whitespace/newlines) before saving or before constructing the Cookie header so
the sent cookie is normalized.
- Around line 48-52: The current validation trims edge whitespace but still
allows embedded whitespace (e.g., "org id"), so update isValidOrgId(_ orgId:
String) to reject any internal whitespace: after creating trimmed, add a check
that trimmed.rangeOfCharacter(from: .whitespacesAndNewlines) == nil (or
equivalent) in the boolean chain along with the existing segmentReserved check
and non-empty check; reference the function isValidOrgId, the trimmed variable
and segmentReserved to locate where to add this additional validation.
In `@Sources/Sidebar/ProviderAccountsFooterPanel.swift`:
- Around line 347-374: The countdown strings (metaText, tooltipText,
formatResetTooltip, formatResetVerbose, relativeCountdown) are currently built
from hard-coded English fragments like "d/h/m", "in …", and "<1m"; replace these
with localized ICU-style strings using String(localized: "key", defaultValue:
...) and plural forms (.one/.other) so all user-facing text is pulled from
Localizable.xcstrings; create keys for days/hours/minutes (e.g. "reset.days",
"reset.hours", "reset.minutes") and for whole phrases ("reset.in",
"reset.less-than-minute") and use those localized templates to format the output
in each function (use pluralization for numeric units and avoid concatenating
raw "d/h/m" fragments), ensuring metaText returns a localized compact form and
tooltipText/formatResetTooltip/formatResetVerbose/relativeCountdown return fully
localized sentences.
🪄 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: c44dfd99-80d7-4f03-9529-842c780517e4
⛔ Files ignored due to path filters (10)
docs/assets/usage-monitoring-add-profile.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-editor-claude.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-editor-codex.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-overview.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-claude.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-codex.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-settings-empty.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-settings-filled.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-sidebar-collapsed.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-sidebar.pngis excluded by!**/*.png
📒 Files selected for processing (26)
GhosttyTabs.xcodeproj/project.pbxprojREADME.mdResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/Providers/ClaudeProvider.swiftSources/Providers/ClaudeUsageFetcher.swiftSources/Providers/CodexProvider.swiftSources/Providers/CodexUsageFetcher.swiftSources/Providers/ProviderAccount.swiftSources/Providers/ProviderAccountStore.swiftSources/Providers/ProviderAccountsController.swiftSources/Providers/ProviderHTTP.swiftSources/Providers/ProviderISO8601DateParser.swiftSources/Providers/ProviderRegistry.swiftSources/Providers/ProviderUsageColorSettings.swiftSources/Providers/StatuspageIOFetcher.swiftSources/Providers/UsageProvider.swiftSources/Sidebar/ProviderAccountEditorSheet.swiftSources/Sidebar/ProviderAccountsFooterPanel.swiftSources/Sidebar/ProviderAccountsPopover.swiftSources/Sidebar/ProviderAccountsSettingsSection.swiftSources/cmuxApp.swiftcmuxTests/ProviderTests.swiftdocs/providers.mddocs/usage-monitoring-setup.md
✅ Files skipped from review due to trivial changes (7)
- README.md
- Sources/Providers/ProviderISO8601DateParser.swift
- Sources/Providers/ProviderRegistry.swift
- docs/providers.md
- GhosttyTabs.xcodeproj/project.pbxproj
- docs/usage-monitoring-setup.md
- Sources/Providers/UsageProvider.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- Sources/Providers/ProviderAccount.swift
- Sources/Providers/ProviderHTTP.swift
- Sources/Sidebar/ProviderAccountsSettingsSection.swift
- Sources/Providers/ProviderUsageColorSettings.swift
tranquillum
force-pushed
the
feature/agents-usage-monitoring-sidebar-panel
branch
3 times, most recently
from
a203aa1 to
30eabb8
Compare
Author
|
@codex review |
@tranquillum I have started the AI code review. It will take a few minutes to complete. |
|
To use Codex here, create a Codex account and connect to github. |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 52014 🏁 Script executed: Length of output: 65424
🔴 High1. Localized string keys containing runtime values (
|
| # | Severity | File | Issue |
|---|---|---|---|
| 1 | 🔴 High | ProviderAccountsPopover.swift |
Runtime-interpolated String(localized:) keys defeat localization |
| 2 | 🔴 High | ProviderAccountEditorSheet.swift, ProviderAccountStore.swift |
Synchronous Keychain on main thread |
| 3 | 🟡 Medium | ClaudeUsageFetcher.swift |
Cookie value not sanitized for semicolons |
| 4 | 🟡 Medium | ProviderAccountStore.swift |
Stale index entry on remove() failure resurrects secret-less account |
| 5 | 🟡 Medium | ProviderAccountsController.swift |
Timer tick silently dropped during long in-flight fetch |
| 6 | 🔵 Low | ProviderTests.swift |
ProviderUsageColorSettings tests pollute UserDefaults.standard |
| 7 | 🔵 Low | ProviderAccountsFooterPanel.swift |
emptyPlaceholder unreachable in normal flow |
| 8 | 🔵 Low | ProviderTests.swift |
Missing tests for ProviderSecret redaction and header sanitization |
| 9 | 🔵 Nit | CodexUsageFetcher.swift |
CFBoolean check via === works but CFGetTypeID is more idiomatic |
![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: 30eabb82ec
ℹ️ 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".
| isSession: false | ||
| ) | ||
|
|
||
| Text(String(localized: "Updated \(formattedTime(snapshot.fetchedAt))")) |
There was a problem hiding this comment.
Using String(localized: "Updated \(formattedTime(snapshot.fetchedAt))") builds a runtime lookup key that does not exist in the string catalog, so the popover falls back to English in non-English locales and translators cannot control word order around the time. This should use a stable localization key (with a placeholder) or compose the existing localized label with the formatted timestamp.
Useful? React with 👍 / 👎.
| private func popoverResetText(window: ProviderUsageWindow, isSession: Bool) -> some View { | ||
| Group { | ||
| if let resetsAt = window.resetsAt { | ||
| Text(String(localized: "Resets \(formatResetVerbose(resetsAt))")) |
There was a problem hiding this comment.
The reset line uses String(localized: "Resets \(formatResetVerbose(resetsAt))"), which similarly creates a dynamic key that is not translatable. In localized builds this string will remain English and prevents locale-specific grammar for the reset time phrase. Use a fixed localization key with formatting or join an existing localized "Resets" label with the formatted value.
Useful? React with 👍 / 👎.
| Text(account.displayName) | ||
| .font(.system(size: 13, weight: .medium)) | ||
| if let snapshot = controller.snapshots[account.id] { | ||
| Text(String(localized: "Session \(snapshot.session.utilization)% · Week \(snapshot.week.utilization)%")) |
There was a problem hiding this comment.
The settings summary String(localized: "Session \(snapshot.session.utilization)% · Week \(snapshot.week.utilization)%") is also a dynamic lookup key, so it will not pick up translations and will display English text for users running other languages. This should be a stable localized format string (or assembled from localized tokens) so translators can localize both labels and punctuation/order.
Useful? React with 👍 / 👎.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
5 issues found across 37 files
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="README.md">
<violation number="1" location="README.md:88">
P2: Markdown links were added inside an HTML table cell; use HTML anchors to ensure links render/click reliably in the README.</violation>
</file>
<file name="Sources/Providers/ProviderAccountStore.swift">
<violation number="1" location="Sources/Providers/ProviderAccountStore.swift:128">
P2: Index decode errors are silently treated as empty state, causing account disappearance and potential orphaned keychain credentials without diagnostics.</violation>
</file>
<file name="cmuxTests/ProviderTests.swift">
<violation number="1" location="cmuxTests/ProviderTests.swift:174">
P2: Clamp tests are ineffective because they only assert non-nil on a non-optional return type instead of asserting equality with boundary colors.</violation>
</file>
<file name="Sources/Providers/CodexUsageFetcher.swift">
<violation number="1" location="Sources/Providers/CodexUsageFetcher.swift:87">
P2: `reset_after_seconds` rejects JSON `null` (`NSNull`) and throws decoding, causing avoidable fetch failure for an optional field.</violation>
</file>
<file name="Sources/Providers/ClaudeUsageFetcher.swift">
<violation number="1" location="Sources/Providers/ClaudeUsageFetcher.swift:80">
P2: Weekly quota parsing silently defaults missing/invalid `seven_day` utilization to 0%, which can mask response/schema problems and display misleading usage.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| let fiveHourResetsAt = ProviderISO8601DateParser.parse(fiveHour["resets_at"] as? String) | ||
|
|
||
| let sevenDay = json["seven_day"] as? [String: Any] | ||
| let sevenDayUtil = min(max((sevenDay?["utilization"] as? NSNumber)?.intValue ?? 0, 0), 100) |
There was a problem hiding this comment.
P2: Weekly quota parsing silently defaults missing/invalid seven_day utilization to 0%, which can mask response/schema problems and display misleading usage.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Providers/ClaudeUsageFetcher.swift, line 80:
<comment>Weekly quota parsing silently defaults missing/invalid `seven_day` utilization to 0%, which can mask response/schema problems and display misleading usage.</comment>
<file context>
@@ -0,0 +1,89 @@
+ let fiveHourResetsAt = ProviderISO8601DateParser.parse(fiveHour["resets_at"] as? String)
+
+ let sevenDay = json["seven_day"] as? [String: Any]
+ let sevenDayUtil = min(max((sevenDay?["utilization"] as? NSNumber)?.intValue ?? 0, 0), 100)
+ let sevenDayResetsAt = ProviderISO8601DateParser.parse(sevenDay?["resets_at"] as? String)
+
</file context>
tranquillum
force-pushed
the
feature/agents-usage-monitoring-sidebar-panel
branch
3 times, most recently
from
3c7187d to
e959d35
Compare
Author
|
@codex review |
@tranquillum I have started the AI code review. It will take a few minutes to complete. |
|
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
Sources/Providers/ProviderAccountStore.swift (1)
140-152: Consider validating keychain presence when loading the index.
loadIndex()trusts that every decoded account has a corresponding keychain secret. If an app crash or write failure leaves an orphaned index entry (account in UserDefaults but secret deleted from keychain), that entry reappears on next launch and fails only whensecret(for:)is called. Pruning such orphans at load time would surface the inconsistency earlier and prevent confusion.♻️ Suggested defensive validation
private func loadIndex() -> [ProviderAccount] { guard let data = userDefaults.data(forKey: indexKey) else { return [] } do { - return try JSONDecoder().decode([ProviderAccount].self, from: data) + let decoded = try JSONDecoder().decode([ProviderAccount].self, from: data) + // Prune accounts whose keychain secret is missing (crash recovery, etc.) + let valid = decoded.filter { account in + let service = account.keychainService ?? keychainServiceResolver(account.providerId) + var query: [CFString: Any] = [ + kSecClass: kSecClassGenericPassword, + kSecAttrService: service, + kSecAttrAccount: account.id.uuidString, + kSecAttrSynchronizable: kCFBooleanFalse as Any, + ] + query[kSecReturnData] = false + return SecItemCopyMatching(query as CFDictionary, nil) == errSecSuccess + } + if valid.count != decoded.count { + // Persist pruned list so orphans don't reappear + try? saveIndex(valid) + } + return valid } catch { NSLog("ProviderAccountStore: failed to decode account index (\(data.count) bytes): \(error)") return [] } }Note: This synchronous keychain check runs once at init (not on every reload) so the brief main-thread hit is acceptable, but you could also move the validation into an async
Taskif startup latency is a concern.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Providers/ProviderAccountStore.swift` around lines 140 - 152, The loadIndex() method in ProviderAccountStore currently returns decoded ProviderAccount entries from UserDefaults without verifying a corresponding keychain secret, which can leave orphaned accounts; update loadIndex() to, after decoding, iterate the accounts and call the keychain lookup used by secret(for:) (or a lower-level Keychain API) for each account identifier, drop any accounts whose secret lookup fails, log occurrences with NSLog including the account id and error, and persist the pruned index back to userDefaults under indexKey so subsequent loads won't return orphaned entries; ensure this validation runs synchronously at init (or optionally wrap in Task if you later choose async) and keep the function signature loadIndex() unchanged.Sources/Providers/ProviderUsageColorSettings.swift (1)
44-60: Clamp the absolute threshold bounds in the property observers too.
setThresholds(low:high:)preserves the intended1...99contract, but these two@Publishedproperties are still publicly writable and theirdidSetblocks only fix ordering. A future direct assignment can persist0,100, or other out-of-range values and skewcolor(for:).Consider making
lowMidThreshold/midHighThresholdprivate(set)and routing all writes throughsetThresholds(low:high:), or clamp to1...98/2...99insidedidSetbefore saving toUserDefaults.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Providers/ProviderUsageColorSettings.swift` around lines 44 - 60, The two publicly writable `@Published` properties lowMidThreshold and midHighThreshold can still be set out of the intended 1...99 range; update them so writes are clamped and/or routed through setThresholds(low:high:). Either make the properties private(set) and require external code to call setThresholds(low:high:) (which already enforces 1...99), or add clamping logic inside each didSet (for lowMidThreshold clamp to 1...98 and for midHighThreshold clamp to 2...99, then ensure ordering) before calling defaults.set(...). Keep references to defaults and Self.keyLowMidThreshold / Self.keyMidHighThreshold and ensure color(for:) sees only valid threshold values.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/Providers/ProviderAccountsController.swift`:
- Around line 220-221: In ProviderAccountsController where you currently have
the invalid pattern `case .failure(is CancellationError):`, bind the associated
error value and narrow its type instead of using `is` in the pattern; for
example, match `case .failure(let error)` and then either use a type cast
pattern or a `where` clause to check that `error` is a CancellationError, then
call `continue`; update the switch arm that references the `.failure` enum case
and the CancellationError type accordingly.
In `@Sources/Sidebar/ProviderAccountsFooterPanel.swift`:
- Around line 347-354: The fallback "—" in the metaText computed property is not
localized; update metaText (used by Text(metaText)) to return a localized string
via String(localized: "sidebar.providerAccounts.resetPlaceholder", defaultValue:
"—") (or change the view to render a non-text placeholder view when
window.resetsAt is nil/expired) so all user-facing strings follow the
String(localized:..., defaultValue:...) pattern; modify the metaText getter and
any callers (e.g., Text(metaText)) accordingly and add the new localization key.
---
Nitpick comments:
In `@Sources/Providers/ProviderAccountStore.swift`:
- Around line 140-152: The loadIndex() method in ProviderAccountStore currently
returns decoded ProviderAccount entries from UserDefaults without verifying a
corresponding keychain secret, which can leave orphaned accounts; update
loadIndex() to, after decoding, iterate the accounts and call the keychain
lookup used by secret(for:) (or a lower-level Keychain API) for each account
identifier, drop any accounts whose secret lookup fails, log occurrences with
NSLog including the account id and error, and persist the pruned index back to
userDefaults under indexKey so subsequent loads won't return orphaned entries;
ensure this validation runs synchronously at init (or optionally wrap in Task if
you later choose async) and keep the function signature loadIndex() unchanged.
In `@Sources/Providers/ProviderUsageColorSettings.swift`:
- Around line 44-60: The two publicly writable `@Published` properties
lowMidThreshold and midHighThreshold can still be set out of the intended 1...99
range; update them so writes are clamped and/or routed through
setThresholds(low:high:). Either make the properties private(set) and require
external code to call setThresholds(low:high:) (which already enforces 1...99),
or add clamping logic inside each didSet (for lowMidThreshold clamp to 1...98
and for midHighThreshold clamp to 2...99, then ensure ordering) before calling
defaults.set(...). Keep references to defaults and Self.keyLowMidThreshold /
Self.keyMidHighThreshold and ensure color(for:) sees only valid threshold
values.
🪄 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: 7d8af3e6-e580-4ae3-a5ad-cee1fe39d616
⛔ Files ignored due to path filters (11)
docs/assets/usage-monitoring-add-profile.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-editor-claude.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-editor-codex.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-overview.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-claude.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-codex.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-degraded.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-settings-empty.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-settings-filled.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-sidebar-collapsed.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-sidebar.pngis excluded by!**/*.png
📒 Files selected for processing (26)
GhosttyTabs.xcodeproj/project.pbxprojREADME.mdResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/Providers/ClaudeProvider.swiftSources/Providers/ClaudeUsageFetcher.swiftSources/Providers/CodexProvider.swiftSources/Providers/CodexUsageFetcher.swiftSources/Providers/ProviderAccount.swiftSources/Providers/ProviderAccountStore.swiftSources/Providers/ProviderAccountsController.swiftSources/Providers/ProviderHTTP.swiftSources/Providers/ProviderISO8601DateParser.swiftSources/Providers/ProviderRegistry.swiftSources/Providers/ProviderUsageColorSettings.swiftSources/Providers/StatuspageIOFetcher.swiftSources/Providers/UsageProvider.swiftSources/Sidebar/ProviderAccountEditorSheet.swiftSources/Sidebar/ProviderAccountsFooterPanel.swiftSources/Sidebar/ProviderAccountsPopover.swiftSources/Sidebar/ProviderAccountsSettingsSection.swiftSources/cmuxApp.swiftcmuxTests/ProviderTests.swiftdocs/providers.mddocs/usage-monitoring-setup.md
✅ Files skipped from review due to trivial changes (8)
- Sources/Providers/ProviderISO8601DateParser.swift
- README.md
- GhosttyTabs.xcodeproj/project.pbxproj
- docs/usage-monitoring-setup.md
- Sources/Providers/ProviderAccount.swift
- Sources/Sidebar/ProviderAccountsPopover.swift
- docs/providers.md
- Sources/Providers/UsageProvider.swift
🚧 Files skipped from review as they are similar to previous changes (2)
- Sources/Providers/ProviderRegistry.swift
- Sources/Providers/CodexProvider.swift
There was a problem hiding this comment.
1 issue found across 37 files
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="Sources/Providers/ProviderAccountsController.swift">
<violation number="1" location="Sources/Providers/ProviderAccountsController.swift:156">
P2: `tick` reconciles usage/status using a stale `accounts` snapshot across `await`, which can retain removed-account/provider state until a later refresh.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
tranquillum
force-pushed
the
feature/agents-usage-monitoring-sidebar-panel
branch
from
2975726 to
ddaef05
Compare
Author
|
@codex review |
@tranquillum I have started the AI code review. It will take a few minutes to complete. |
|
To use Codex here, create a Codex account and connect to github. |
|
The PR description images show:
🔴 High:
🟡 Medium: 🔵 Low/Nit: 🧠 Learnings used✅ Actions performedReview triggered.
|
|
Codex Review: Didn't find any major issues. Breezy! ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
1 issue found across 37 files
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="Sources/Sidebar/ProviderAccountsPopover.swift">
<violation number="1" location="Sources/Sidebar/ProviderAccountsPopover.swift:279">
P2: Session popover incorrectly labels stale past-reset snapshots as "Session not started," conflating stale data with true unstarted sessions.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
There was a problem hiding this comment.
🧹 Nitpick comments (3)
docs/usage-monitoring-setup.md (1)
103-107: Clarify field key vs UI label for Claude org identifier.At Line 106, consider documenting this as “
orgId(shown as Organization ID in UI)” to keep terminology consistent with troubleshooting text (Line 150) and reduce ambiguity for users cross-referencing logs/config keys.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@docs/usage-monitoring-setup.md` around lines 103 - 107, Update the documentation to disambiguate the Claude organization identifier by changing the Credentials bullet to list "orgId (shown as **Organization ID** in UI)" instead of just "Organization ID", keep the existing `sessionKey` entry and the Keychain service `com.cmuxterm.app.claude-accounts`, and ensure the term `orgId` matches the troubleshooting section wording so readers can cross-reference logs/config keys with the UI label.Sources/Providers/ClaudeUsageFetcher.swift (1)
122-128: Consider usingCFGetTypeIDfor CFBoolean detection.The pointer identity check (
===) works correctly on Apple platforms butCFGetTypeIDis more explicit and portable:if CFGetTypeID(number) == CFBooleanGetTypeID() { return nil }This is a minor stylistic preference — the current implementation is functionally correct.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Providers/ClaudeUsageFetcher.swift` around lines 122 - 128, Replace the pointer-identity CFBoolean check in intUtilization(_:) with an explicit CoreFoundation type check: detect CFBoolean by comparing CFGetTypeID(number) to CFBooleanGetTypeID() and return nil when they match; keep the guard casting to NSNumber and the final range clamp using min(max(number.intValue, 0), 100) unchanged so only the boolean-detection line is modified.Sources/Sidebar/ProviderAccountsSettingsSection.swift (1)
354-366: Preview strip rendering could be optimized.Rendering 101 individual
Rectangleviews works but may cause unnecessary view updates. Consider using aCanvasorPathfor better performance if users report lag when adjusting thresholds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Sidebar/ProviderAccountsSettingsSection.swift` around lines 354 - 366, The preview strip currently creates 101 individual Rectangle views inside the VStack/ForEach loop (see ForEach and Rectangle and colorSettings.color(for:)), which can cause excessive view updates; replace that with a single drawing-backed view such as Canvas or a custom Shape/Path that samples colorSettings.color(for:) across the 0...100 range and paints the strip in one draw pass (or build a single horizontal gradient if colors are smoothly varying) so updates only invalidate the Canvas rather than 101 child views; keep the same frame/clipShape and caption layout (VStack/Text) and ensure the Canvas uses the same color sampling function colorSettings.color(for:) to preserve appearance.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@docs/usage-monitoring-setup.md`:
- Around line 103-107: Update the documentation to disambiguate the Claude
organization identifier by changing the Credentials bullet to list "orgId (shown
as **Organization ID** in UI)" instead of just "Organization ID", keep the
existing `sessionKey` entry and the Keychain service
`com.cmuxterm.app.claude-accounts`, and ensure the term `orgId` matches the
troubleshooting section wording so readers can cross-reference logs/config keys
with the UI label.
In `@Sources/Providers/ClaudeUsageFetcher.swift`:
- Around line 122-128: Replace the pointer-identity CFBoolean check in
intUtilization(_:) with an explicit CoreFoundation type check: detect CFBoolean
by comparing CFGetTypeID(number) to CFBooleanGetTypeID() and return nil when
they match; keep the guard casting to NSNumber and the final range clamp using
min(max(number.intValue, 0), 100) unchanged so only the boolean-detection line
is modified.
In `@Sources/Sidebar/ProviderAccountsSettingsSection.swift`:
- Around line 354-366: The preview strip currently creates 101 individual
Rectangle views inside the VStack/ForEach loop (see ForEach and Rectangle and
colorSettings.color(for:)), which can cause excessive view updates; replace that
with a single drawing-backed view such as Canvas or a custom Shape/Path that
samples colorSettings.color(for:) across the 0...100 range and paints the strip
in one draw pass (or build a single horizontal gradient if colors are smoothly
varying) so updates only invalidate the Canvas rather than 101 child views; keep
the same frame/clipShape and caption layout (VStack/Text) and ensure the Canvas
uses the same color sampling function colorSettings.color(for:) to preserve
appearance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2a23a028-9d65-468b-abcf-6dab7d0af792
⛔ Files ignored due to path filters (11)
docs/assets/usage-monitoring-add-profile.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-editor-claude.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-editor-codex.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-overview.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-claude.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-codex.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-degraded.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-settings-empty.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-settings-filled.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-sidebar-collapsed.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-sidebar.pngis excluded by!**/*.png
📒 Files selected for processing (26)
GhosttyTabs.xcodeproj/project.pbxprojREADME.mdResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/Providers/ClaudeProvider.swiftSources/Providers/ClaudeUsageFetcher.swiftSources/Providers/CodexProvider.swiftSources/Providers/CodexUsageFetcher.swiftSources/Providers/ProviderAccount.swiftSources/Providers/ProviderAccountStore.swiftSources/Providers/ProviderAccountsController.swiftSources/Providers/ProviderHTTP.swiftSources/Providers/ProviderISO8601DateParser.swiftSources/Providers/ProviderRegistry.swiftSources/Providers/ProviderUsageColorSettings.swiftSources/Providers/StatuspageIOFetcher.swiftSources/Providers/UsageProvider.swiftSources/Sidebar/ProviderAccountEditorSheet.swiftSources/Sidebar/ProviderAccountsFooterPanel.swiftSources/Sidebar/ProviderAccountsPopover.swiftSources/Sidebar/ProviderAccountsSettingsSection.swiftSources/cmuxApp.swiftcmuxTests/ProviderTests.swiftdocs/providers.mddocs/usage-monitoring-setup.md
✅ Files skipped from review due to trivial changes (9)
- README.md
- Sources/Providers/ProviderISO8601DateParser.swift
- docs/providers.md
- Sources/Providers/StatuspageIOFetcher.swift
- Sources/Providers/ProviderAccount.swift
- Sources/Providers/UsageProvider.swift
- Sources/Sidebar/ProviderAccountsFooterPanel.swift
- cmuxTests/ProviderTests.swift
- Sources/ContentView.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- Sources/Providers/ProviderRegistry.swift
- GhosttyTabs.xcodeproj/project.pbxproj
- Sources/Providers/ProviderHTTP.swift
- Sources/Providers/ProviderUsageColorSettings.swift
tranquillum
force-pushed
the
feature/agents-usage-monitoring-sidebar-panel
branch
from
ddaef05 to
edb8f75
Compare
Author
|
@codex review |
@tranquillum I have started the AI code review. It will take a few minutes to complete. |
|
The PR description images show the same well-polished UI as in prior passes — the expanded sidebar footer (CLAUDE: Personal Sess 12%/Week 10%, Work Sess 100%/Week 14%; CODEX: Personal Sess 27%/Week 8%), the per-provider popovers with reset countdowns and status indicators, the Settings → AI Usage Monitoring card with account rows and color-threshold configuration, the Claude/Codex account editor sheets, and the collapsed
🔴 High (open):
🟡 Medium (open): 🔵 Low/Nit: The two highest-priority items to address before merge remain #1 (localization keys) and #2 (Keychain I/O on the main thread). 🧠 Learnings used✅ Actions performedReview triggered.
|
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (1)
Sources/Sidebar/ProviderAccountsFooterPanel.swift (1)
446-463:⚠️ Potential issue | 🟠 MajorLocalize the full tooltip/reset phrases, not just the fragments.
These branches still compose sentence order in Swift (
"\(label) \(percentText) · ...","Today \(time)"). That keeps English punctuation/word order baked into the UI and prevents translators from reordering the dynamic pieces cleanly.♻️ Suggested fix
private var tooltipText: String { let percentText = String( localized: "providers.accounts.usage.percent", defaultValue: "\(percent)%" ) - var text = "\(label) \(percentText)" + let summary = String( + localized: "providers.accounts.usage.summary", + defaultValue: "\(label) \(percentText)" + ) // Only present the reset phrase for an upcoming reset. A resetsAt in // the past means the server-side window rolled over but we haven't // fetched a refresh yet; pairing it with "resets <time>" would imply // a future event that has already passed. if let resetsAt = window.resetsAt, resetsAt > Date() { let resetTime = formatResetTooltip(resetsAt) - let resetsPrefix = String(localized: "providers.accounts.usage.resets", defaultValue: "resets") - text += " · \(resetsPrefix) \(resetTime)" + return String( + localized: "providers.accounts.usage.summaryWithReset", + defaultValue: "\(summary) · resets \(resetTime)" + ) } else if percent == 0 { - text += " · \(String(localized: "providers.accounts.usage.notStarted", defaultValue: "not started"))" + return String( + localized: "providers.accounts.usage.summaryNotStarted", + defaultValue: "\(summary) · not started" + ) } - return text + return summary } @@ let absolute: String if calendar.isDateInToday(resetsAt) { - absolute = "\(String(localized: "providers.accounts.popover.today", defaultValue: "Today")) \(time)" + absolute = String( + localized: "providers.accounts.popover.todayAtTime", + defaultValue: "Today \(time)" + ) } else if calendar.isDateInTomorrow(resetsAt) { - absolute = "\(String(localized: "providers.accounts.popover.tomorrow", defaultValue: "Tomorrow")) \(time)" + absolute = String( + localized: "providers.accounts.popover.tomorrowAtTime", + defaultValue: "Tomorrow \(time)" + ) } else { absolute = resetAbsoluteFormatter.string(from: resetsAt) }As per coding guidelines, "All user-facing strings must be localized. Use
String(localized: "key.name", defaultValue: "English text")for every string shown in the UI."Also applies to: 539-545
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Sidebar/ProviderAccountsFooterPanel.swift` around lines 446 - 463, The tooltip currently concatenates localized fragments (percentText, resetsPrefix) with Swift string interpolation in tooltipText, preventing translators from reordering phrases; change this to use fully localized format strings instead: replace the composed lines in tooltipText with calls to String(localized: ...) that accept positional placeholders (e.g. a key like "providers.accounts.usage.tooltip" with placeholders for label, percent, and optional reset time, and another key for the "not started" variant), pass label, percentText (or resetTime from formatResetTooltip) into those localized format strings, and remove inline concatenation; ensure formatResetTooltip still returns a localized time string but does not bake in surrounding words so the final localization keys control full sentence order.
🧹 Nitpick comments (4)
Sources/Providers/ProviderUsageColorSettings.swift (1)
188-197: Silent fallback to black on color space conversion failure.If
NSColor(self).usingColorSpace(.sRGB)returnsnil(e.g., for pattern or catalog colors),rgbComponentssilently returns(0, 0, 0). This could produce unexpected black interpolation results. Consider logging a warning in DEBUG builds or documenting this behavior.Proposed fix
var rgbComponents: (red: Double, green: Double, blue: Double) { guard let nsColor = NSColor(self).usingColorSpace(.sRGB) else { + `#if` DEBUG + dlog("Color.rgbComponents: failed to convert to sRGB, falling back to black") + `#endif` return (red: 0, green: 0, blue: 0) } return (🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Providers/ProviderUsageColorSettings.swift` around lines 188 - 197, The rgbComponents getter silently returns black when NSColor(self).usingColorSpace(.sRGB) is nil; change it to log a warning in debug builds and preserve current behavior in release: wrap the nil branch in a `#if` DEBUG block and emit a clear warning (e.g., NSLog or debugPrint) that includes the original color description and notes the fallback, then return the current (0,0,0) fallback; update the comment/docstring for rgbComponents to document that non-convertible colors fall back to black so callers are aware of the behavior.Sources/Providers/StatuspageIOFetcher.swift (1)
20-23: Consider validating thehostparameter.The
hostparameter is interpolated directly into a URL string. WhileURL(string:)will returnnilfor malformed URLs, a malicious or misconfigured host value could potentially redirect requests. Ifhostcomes from user input or external configuration, consider validating it against an allowlist of known Statuspage hosts (e.g.,status.claude.com,status.openai.com).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Providers/StatuspageIOFetcher.swift` around lines 20 - 23, StatuspageIOFetcher.fetch currently interpolates the host directly into a URL which can be manipulated; before building the URL, validate the host parameter against an allowlist of known Statuspage hosts (e.g., a Set like allowedHosts = ["status.openai.com", "status.claude.com"]), ensure the host contains no scheme/path/credentials (use URLComponents.host or strict regex to enforce hostname-only), and throw a clear error (add or reuse a StatuspageIOFetchError case such as .invalidHost) when validation fails instead of proceeding to URL(string:). This protects the code from malicious or misconfigured host values while keeping URL construction and the existing error handling in fetch intact.Sources/Providers/ProviderAccountStore.swift (2)
288-303:writeAdddelete-then-add on duplicate may race with concurrent operations.If
errSecDuplicateItemoccurs, the code deletes the existing item and retries the add (lines 294-299). However, between the delete and retry, a concurrentloadorupdatecould fail unexpectedly. Since this is already inside a detached task and the store methods don't serialize concurrent calls to the same account ID, this race window exists. Consider usingSecItemUpdateas the fallback instead of delete-then-add, or documenting this as an acceptable race for the duplicate-item edge case.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Sources/Providers/ProviderAccountStore.swift` around lines 288 - 303, The writeAdd function currently does a delete-then-add on errSecDuplicateItem which can race with concurrent operations; instead, on receiving errSecDuplicateItem from SecItemAdd, call SecItemUpdate using the same matchQuery(service:accountId:) as the query and addAttributes(payload:) as the attributes-to-update, and if SecItemUpdate returns an error other than errSecSuccess then throw ProviderAccountStoreError.keychain with that status (fall back to throwing only if update fails); keep the original SecItemAdd path otherwise and retain use of SecItemAdd, matchQuery, addAttributes, SecItemUpdate, and ProviderAccountStoreError.keychain to locate the change.
237-243:probePresence()is synchronous and blocks MainActor during app initialization.
probePresence()performs a synchronousSecItemCopyMatching()call duringloadIndex()(line 160), which runs on the MainActor ininit(). The codebase design explicitly delegates keychain operations to detached tasks (see comment lines 42–45), butprobePresence()is kept synchronous to distinguisherrSecItemNotFoundfrom transient failures during orphan account pruning. While this tradeoff is intentional and documented (lines 233–236), it can cause startup hitches on slow keychain responses. Either wrap the pruning logic in an async detached task or add explicit documentation of the MainActor startup impact and why it's necessary.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Sources/cmuxApp.swift`:
- Around line 6471-6499: The alert currently displays the raw
providerRemoveError (set in the catch of Task in the removal confirmation) which
bypasses localization; instead map the caught error from
ProviderAccountStore.shared.remove(id:) to a stable localization key (e.g.,
switch on error cases or use an NSError domain/code mapping) and set
providerRemoveError to a localized String(localized:
"providers.accounts.remove.error.<case>", defaultValue: "...") for UI
presentation, while keeping the original error only in the NSLog call; update
the alert message binding to use that localized providerRemoveError and ensure
every user-facing string uses String(localized:..., defaultValue:...).
In `@Sources/Providers/ClaudeProvider.swift`:
- Around line 62-87: The validator and fetcher disagree on how many leading
"sessionKey=" prefixes to remove, allowing inputs like
"sessionKey=sessionKey=abc" to pass validation but fail auth; fix by making
canonicalization deterministic: update strippedSessionKey(_:) so it removes any
repeated leading "sessionKey=" prefixes (e.g., loop dropping the prefix while
trimmed.hasPrefix("sessionKey=")) and return that canonical body, then have
isValidSessionKey(_:) call strippedSessionKey(_:) (as it already does) and
ensure ClaudeUsageFetcher.fetch also uses strippedSessionKey(_:) when
constructing the Cookie header so both validation (isValidSessionKey) and fetch
use the same canonical form.
In `@Sources/Providers/ClaudeUsageFetcher.swift`:
- Around line 28-29: The network error case in ClaudeUsageFetcher currently
interpolates underlying.localizedDescription into the user-facing
String(localized: "claude.usage.error.network", ...), which mixes OS text into
the UI; change the returned text to a fully localized summary only (e.g. use
String(localized: "claude.usage.error.network", defaultValue: "Network error")),
and if you still need the underlying error for debugging or telemetry, log or
send underlying.localizedDescription separately from the UI path (keep this in
the same case .network(underlying) branch and ensure the log/telemetry call is
not shown to users).
In `@Sources/Providers/CodexProvider.swift`:
- Around line 9-16: The isValidAccessToken function currently trims only edges
but allows interior control/whitespace characters; update isValidAccessToken to
reject any token that contains control characters or any whitespace/newline
anywhere in the trimmed string (e.g., check that trimmed.rangeOfCharacter(from:
.whitespacesAndNewlines.union(.controlCharacters)) is nil) before proceeding
with the existing JWT-segmentation logic (the
trimmed.split/segments.count/segments.allSatisfy checks) so tokens with embedded
spaces/newlines are considered invalid.
In `@Sources/Providers/CodexUsageFetcher.swift`:
- Around line 31-32: The .network case in CodexUsageFetcher currently
interpolates underlying.localizedDescription into a user-facing
String(localized: "codex.usage.error.network", defaultValue: "Network error:
\(underlying.localizedDescription)"), which exposes raw OS text; change it to
return a fully localized summary only (e.g., String(localized:
"codex.usage.error.network", defaultValue: "Network error")) and emit the raw
underlying.localizedDescription to a debug/error logger (e.g., logger.error or
os_log) so the UI stays localized while the detailed message is preserved in
logs; update the .network case to use only the localized summary and add a
separate logging call that records underlying.localizedDescription.
In `@Sources/Providers/ProviderAccountStore.swift`:
- Around line 108-121: In remove(id: UUID) (which calls
ProviderAccountKeychain.delete(for:service:) and then saveIndex(current)), catch
errors thrown by saveIndex(current) so you don't re-throw after the keychain
secret was already deleted: update the in-memory accounts = current regardless,
wrap the saveIndex call in a do/catch, and in the catch log the failure (using
your project's logging facility or a fallback) instead of throwing
ProviderAccountStoreError so the credential-free index entry won't surface as a
broken UI item; keep throwing for the initial notFound and keychain delete
failures only.
In `@Sources/Sidebar/ProviderAccountsFooterPanel.swift`:
- Around line 435-441: The hard-coded .frame(width: 46, alignment: .trailing) on
the Text(metaText) (the countdown column) truncates translated countdown
strings; remove the fixed width and let the text size adapt by replacing
.frame(width: 46, alignment: .trailing) with a flexible alternative such as
.frame(minWidth: 46, alignment: .trailing) or no explicit width at all (or use
.fixedSize() / .layoutPriority(1) if you need to prevent truncation) so
localized strings can expand beyond the English-only 46pt constraint.
---
Duplicate comments:
In `@Sources/Sidebar/ProviderAccountsFooterPanel.swift`:
- Around line 446-463: The tooltip currently concatenates localized fragments
(percentText, resetsPrefix) with Swift string interpolation in tooltipText,
preventing translators from reordering phrases; change this to use fully
localized format strings instead: replace the composed lines in tooltipText with
calls to String(localized: ...) that accept positional placeholders (e.g. a key
like "providers.accounts.usage.tooltip" with placeholders for label, percent,
and optional reset time, and another key for the "not started" variant), pass
label, percentText (or resetTime from formatResetTooltip) into those localized
format strings, and remove inline concatenation; ensure formatResetTooltip still
returns a localized time string but does not bake in surrounding words so the
final localization keys control full sentence order.
---
Nitpick comments:
In `@Sources/Providers/ProviderAccountStore.swift`:
- Around line 288-303: The writeAdd function currently does a delete-then-add on
errSecDuplicateItem which can race with concurrent operations; instead, on
receiving errSecDuplicateItem from SecItemAdd, call SecItemUpdate using the same
matchQuery(service:accountId:) as the query and addAttributes(payload:) as the
attributes-to-update, and if SecItemUpdate returns an error other than
errSecSuccess then throw ProviderAccountStoreError.keychain with that status
(fall back to throwing only if update fails); keep the original SecItemAdd path
otherwise and retain use of SecItemAdd, matchQuery, addAttributes,
SecItemUpdate, and ProviderAccountStoreError.keychain to locate the change.
In `@Sources/Providers/ProviderUsageColorSettings.swift`:
- Around line 188-197: The rgbComponents getter silently returns black when
NSColor(self).usingColorSpace(.sRGB) is nil; change it to log a warning in debug
builds and preserve current behavior in release: wrap the nil branch in a `#if`
DEBUG block and emit a clear warning (e.g., NSLog or debugPrint) that includes
the original color description and notes the fallback, then return the current
(0,0,0) fallback; update the comment/docstring for rgbComponents to document
that non-convertible colors fall back to black so callers are aware of the
behavior.
In `@Sources/Providers/StatuspageIOFetcher.swift`:
- Around line 20-23: StatuspageIOFetcher.fetch currently interpolates the host
directly into a URL which can be manipulated; before building the URL, validate
the host parameter against an allowlist of known Statuspage hosts (e.g., a Set
like allowedHosts = ["status.openai.com", "status.claude.com"]), ensure the host
contains no scheme/path/credentials (use URLComponents.host or strict regex to
enforce hostname-only), and throw a clear error (add or reuse a
StatuspageIOFetchError case such as .invalidHost) when validation fails instead
of proceeding to URL(string:). This protects the code from malicious or
misconfigured host values while keeping URL construction and the existing error
handling in fetch intact.
🪄 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: e2c222f5-4023-4340-864b-65de68389034
⛔ Files ignored due to path filters (11)
docs/assets/usage-monitoring-add-profile.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-editor-claude.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-editor-codex.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-overview.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-claude.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-codex.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-degraded.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-settings-empty.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-settings-filled.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-sidebar-collapsed.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-sidebar.pngis excluded by!**/*.png
📒 Files selected for processing (26)
GhosttyTabs.xcodeproj/project.pbxprojREADME.mdResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/Providers/ClaudeProvider.swiftSources/Providers/ClaudeUsageFetcher.swiftSources/Providers/CodexProvider.swiftSources/Providers/CodexUsageFetcher.swiftSources/Providers/ProviderAccount.swiftSources/Providers/ProviderAccountStore.swiftSources/Providers/ProviderAccountsController.swiftSources/Providers/ProviderHTTP.swiftSources/Providers/ProviderISO8601DateParser.swiftSources/Providers/ProviderRegistry.swiftSources/Providers/ProviderUsageColorSettings.swiftSources/Providers/StatuspageIOFetcher.swiftSources/Providers/UsageProvider.swiftSources/Sidebar/ProviderAccountEditorSheet.swiftSources/Sidebar/ProviderAccountsFooterPanel.swiftSources/Sidebar/ProviderAccountsPopover.swiftSources/Sidebar/ProviderAccountsSettingsSection.swiftSources/cmuxApp.swiftcmuxTests/ProviderTests.swiftdocs/providers.mddocs/usage-monitoring-setup.md
✅ Files skipped from review due to trivial changes (7)
- README.md
- GhosttyTabs.xcodeproj/project.pbxproj
- Sources/Providers/ProviderISO8601DateParser.swift
- docs/usage-monitoring-setup.md
- docs/providers.md
- Sources/Providers/ProviderAccount.swift
- Sources/Providers/UsageProvider.swift
🚧 Files skipped from review as they are similar to previous changes (4)
- Sources/Providers/ProviderRegistry.swift
- Sources/Providers/ProviderHTTP.swift
- Sources/ContentView.swift
- cmuxTests/ProviderTests.swift
Comment on lines
+62
to
+87
| static func isValidSessionKey(_ sessionKey: String) -> Bool { | ||
| let body = strippedSessionKey(sessionKey) | ||
| guard !body.isEmpty else { return false } | ||
| var disallowed = CharacterSet.controlCharacters | ||
| // `;` and `,` are the separators that would split the `Cookie` | ||
| // header and let a paste smuggle extra directives. `=` is left in | ||
| // because cookie values (e.g. base64-padded tokens) legitimately | ||
| // contain it. | ||
| disallowed.insert(charactersIn: ";,\n\r") | ||
| // Embedded whitespace in a cookie value is not a valid sessionKey and | ||
| // the server would reject it outright; catch it here so the editor | ||
| // shows the right guidance instead of surfacing a fetch failure. | ||
| disallowed.formUnion(.whitespacesAndNewlines) | ||
| return body.rangeOfCharacter(from: disallowed) == nil | ||
| } | ||
|
|
||
| /// Trims whitespace and removes a single leading `sessionKey=` prefix. | ||
| /// Shared with the fetch path so the editor and the outgoing `Cookie` | ||
| /// header always evaluate the same canonical string. | ||
| static func strippedSessionKey(_ sessionKey: String) -> String { | ||
| let trimmed = sessionKey.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| let prefix = "sessionKey=" | ||
| if trimmed.hasPrefix(prefix) { | ||
| return String(trimmed.dropFirst(prefix.count)) | ||
| } | ||
| return trimmed |
There was a problem hiding this comment.
Make validation and fetch-time canonicalization agree.
isValidSessionKey(_:) strips a leading sessionKey= again, but ClaudeUsageFetcher.fetch only strips once before sending the Cookie header. That means sessionKey=sessionKey=abc validates here and is later sent as sessionKey=sessionKey=abc, which will fail auth. Reject repeated prefixes or canonicalize to one stable body before both validation and fetch.
Possible fix
static func isValidSessionKey(_ sessionKey: String) -> Bool {
let body = strippedSessionKey(sessionKey)
guard !body.isEmpty else { return false }
+ guard !body.hasPrefix("sessionKey=") else { return false }
var disallowed = CharacterSet.controlCharacters
disallowed.insert(charactersIn: ";,\n\r")
disallowed.formUnion(.whitespacesAndNewlines)
return body.rangeOfCharacter(from: disallowed) == nil
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Providers/ClaudeProvider.swift` around lines 62 - 87, The validator
and fetcher disagree on how many leading "sessionKey=" prefixes to remove,
allowing inputs like "sessionKey=sessionKey=abc" to pass validation but fail
auth; fix by making canonicalization deterministic: update
strippedSessionKey(_:) so it removes any repeated leading "sessionKey=" prefixes
(e.g., loop dropping the prefix while trimmed.hasPrefix("sessionKey=")) and
return that canonical body, then have isValidSessionKey(_:) call
strippedSessionKey(_:) (as it already does) and ensure ClaudeUsageFetcher.fetch
also uses strippedSessionKey(_:) when constructing the Cookie header so both
validation (isValidSessionKey) and fetch use the same canonical form.
There was a problem hiding this comment.
1 issue found across 37 files
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="Sources/Providers/ProviderAccountStore.swift">
<violation number="1" location="Sources/Providers/ProviderAccountStore.swift:160">
P2: Synchronous keychain presence checks run on `@MainActor` during index load, which can block UI during startup/reload.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| // keychain keep the entry so a later launch still sees it. | ||
| let pruned = decoded.filter { account in | ||
| let service = serviceName(for: account) | ||
| let status = ProviderAccountKeychain.probePresence(for: account.id, service: service) |
There was a problem hiding this comment.
P2: Synchronous keychain presence checks run on @MainActor during index load, which can block UI during startup/reload.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Providers/ProviderAccountStore.swift, line 160:
<comment>Synchronous keychain presence checks run on `@MainActor` during index load, which can block UI during startup/reload.</comment>
<file context>
@@ -0,0 +1,339 @@
+ // keychain keep the entry so a later launch still sees it.
+ let pruned = decoded.filter { account in
+ let service = serviceName(for: account)
+ let status = ProviderAccountKeychain.probePresence(for: account.id, service: service)
+ if status == errSecItemNotFound {
+ NSLog("ProviderAccountStore: dropping orphan account \(account.id) (keychain \(service) missing secret)")
</file context>
tranquillum
force-pushed
the
feature/agents-usage-monitoring-sidebar-panel
branch
from
edb8f75 to
6a6d9c0
Compare
Author
|
@codex review |
@tranquillum I have started the AI code review. It will take a few minutes to complete. |
|
To use Codex here, create a Codex account and connect to github. |
|
The PR description images show:
🔴 High (open):
🟡 Medium (open):
🔵 Low / Nit (open):
The two highest-priority items remain #1 (localization keys) and #2 (Keychain I/O on the main thread). 🧠 Learnings used✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
2 issues found across 37 files
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="Sources/Providers/ClaudeUsageFetcher.swift">
<violation number="1" location="Sources/Providers/ClaudeUsageFetcher.swift:128">
P2: Utilization parsing truncates fractional numeric values instead of rejecting them, which can silently underreport usage and mask API schema changes.</violation>
</file>
<file name="cmuxTests/ProviderTests.swift">
<violation number="1" location="cmuxTests/ProviderTests.swift:466">
P2: Round-trip remove test does not actually verify keychain deletion; it only checks an index-gated `.notFound` path.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| if CFGetTypeID(number as CFTypeRef) == CFBooleanGetTypeID() { | ||
| return nil | ||
| } | ||
| return min(max(number.intValue, 0), 100) |
There was a problem hiding this comment.
P2: Utilization parsing truncates fractional numeric values instead of rejecting them, which can silently underreport usage and mask API schema changes.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At Sources/Providers/ClaudeUsageFetcher.swift, line 128:
<comment>Utilization parsing truncates fractional numeric values instead of rejecting them, which can silently underreport usage and mask API schema changes.</comment>
<file context>
@@ -0,0 +1,131 @@
+ if CFGetTypeID(number as CFTypeRef) == CFBooleanGetTypeID() {
+ return nil
+ }
+ return min(max(number.intValue, 0), 100)
+ }
+}
</file context>
| try await store.remove(id: account.id) | ||
| XCTAssertTrue(store.accounts.isEmpty) | ||
| do { | ||
| _ = try await store.secret(for: account.id) |
There was a problem hiding this comment.
P2: Round-trip remove test does not actually verify keychain deletion; it only checks an index-gated .notFound path.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At cmuxTests/ProviderTests.swift, line 466:
<comment>Round-trip remove test does not actually verify keychain deletion; it only checks an index-gated `.notFound` path.</comment>
<file context>
@@ -0,0 +1,500 @@
+ try await store.remove(id: account.id)
+ XCTAssertTrue(store.accounts.isEmpty)
+ do {
+ _ = try await store.secret(for: account.id)
+ XCTFail("Expected .notFound after remove")
+ } catch ProviderAccountStoreError.notFound {
</file context>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a6d9c05a9
ℹ️ 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".
| extension Providers { | ||
| static let claude: UsageProvider = UsageProvider( | ||
| id: "claude", | ||
| displayName: "Claude", |
There was a problem hiding this comment.
The /workspace/cmux/AGENTS.md policy says all user-facing strings must use String(localized:defaultValue:), but this hardcoded provider label is surfaced directly in the sidebar/status/settings UI and bypasses Localizable.xcstrings. The same issue is repeated for Codex in Sources/Providers/CodexProvider.swift (displayName: "Codex"), so localized builds cannot manage these labels through the translation pipeline.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmuxTests/ProviderTests.swift`:
- Around line 486-490: The ProviderAccountStore currently starts
pruneOrphanAccountsIfNeeded() during init and the test sets
keychainServiceResolver only after construction, causing a race; modify
ProviderAccountStore.init(userDefaults:indexKey:) to accept an initial
keychainServiceResolver parameter (or a flag to disable pruning) and assign that
resolver to the instance before any pruning runs, ensuring
keychainServiceResolver is injected at construction time (reference
ProviderAccountStore.init(userDefaults:indexKey:), keychainServiceResolver, and
pruneOrphanAccountsIfNeeded()) so tests never start with the wrong service
mapping.
In `@Sources/Providers/ClaudeUsageFetcher.swift`:
- Around line 123-128: The intUtilization(_:) helper currently truncates
fractional NSNumber values via number.intValue; update it to reject
non-integer/fractional numeric payloads instead of silently truncating: in
intUtilization(_ value: Any?) keep the NSNumber and CFBoolean checks
(CFGetTypeID(...) == CFBooleanGetTypeID()), then verify the numeric value is
integral (e.g. compare number.doubleValue to its rounded/integral form or use
truncatingRemainder(dividingBy: 1) == 0); if it's fractional return nil so
decoding fails, otherwise convert to an Int and clamp with min(max(..., 0), 100)
as before.
In `@Sources/Providers/CodexProvider.swift`:
- Around line 9-19: The isValidAccessToken function currently only checks prefix
and dot segments, allowing characters like ';' in JWT parts; restrict each of
the three dot-separated segments (variable segments) to only base64url
characters (A–Z, a–z, 0–9, '-' and '_') and optionally '=' padding if you want
to accept padded tokens. Modify isValidAccessToken (use trimmed, segments) to
build an allowed CharacterSet or regex for base64url (plus '=' if desired) and
return false if any segment contains a scalar outside that set; keep the
existing prefix check and the three non-empty segments requirement.
In `@Sources/Providers/ProviderAccountStore.swift`:
- Around line 355-363: In readLoad(for:service:) map the SecItemCopyMatching
status so that errSecItemNotFound is converted to
ProviderAccountStoreError.notFound instead of a generic keychain error; update
the guard/throw logic in the ProviderAccountStore.readLoad method to check for
status == errSecSuccess, but if status == errSecItemNotFound throw
ProviderAccountStoreError.notFound, otherwise throw
ProviderAccountStoreError.keychain(status) so callers of secret(for:) get the
missing-account flow.
In `@Sources/Sidebar/ProviderAccountsPopover.swift`:
- Around line 335-353: localizedStatus contains unreachable cases ("resolved"
and "postmortem") because StatuspageIOFetcher filters those out—remove those
case branches from the localizedStatus switch in ProviderAccountsPopover to
avoid dead code; localizedImpact is missing the "maintenance" impact—add a case
for "maintenance" that returns String(localized:
"providers.incident.impact.maintenance", defaultValue: "Maintenance"); finally
replace the current default returns in both localizedImpact and localizedStatus
with a localized fallback String(localized: "providers.incident.unknown",
defaultValue: "Unknown") to handle unexpected API values defensibly; refer to
the localizedImpact and localizedStatus properties and StatuspageIOFetcher
behavior when making these edits.
🪄 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: dbeb4899-7329-4243-b7a5-625f1eb0a529
⛔ Files ignored due to path filters (11)
docs/assets/usage-monitoring-add-profile.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-editor-claude.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-editor-codex.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-overview.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-claude.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-codex.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-popover-degraded.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-settings-empty.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-settings-filled.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-sidebar-collapsed.pngis excluded by!**/*.pngdocs/assets/usage-monitoring-sidebar.pngis excluded by!**/*.png
📒 Files selected for processing (26)
GhosttyTabs.xcodeproj/project.pbxprojREADME.mdResources/Localizable.xcstringsSources/AppDelegate.swiftSources/ContentView.swiftSources/Providers/ClaudeProvider.swiftSources/Providers/ClaudeUsageFetcher.swiftSources/Providers/CodexProvider.swiftSources/Providers/CodexUsageFetcher.swiftSources/Providers/ProviderAccount.swiftSources/Providers/ProviderAccountStore.swiftSources/Providers/ProviderAccountsController.swiftSources/Providers/ProviderHTTP.swiftSources/Providers/ProviderISO8601DateParser.swiftSources/Providers/ProviderRegistry.swiftSources/Providers/ProviderUsageColorSettings.swiftSources/Providers/StatuspageIOFetcher.swiftSources/Providers/UsageProvider.swiftSources/Sidebar/ProviderAccountEditorSheet.swiftSources/Sidebar/ProviderAccountsFooterPanel.swiftSources/Sidebar/ProviderAccountsPopover.swiftSources/Sidebar/ProviderAccountsSettingsSection.swiftSources/cmuxApp.swiftcmuxTests/ProviderTests.swiftdocs/providers.mddocs/usage-monitoring-setup.md
✅ Files skipped from review due to trivial changes (8)
- README.md
- Sources/Providers/ProviderISO8601DateParser.swift
- Sources/Providers/ProviderRegistry.swift
- docs/providers.md
- docs/usage-monitoring-setup.md
- Sources/Providers/ProviderHTTP.swift
- Sources/Providers/UsageProvider.swift
- Sources/Providers/ProviderUsageColorSettings.swift
🚧 Files skipped from review as they are similar to previous changes (6)
- GhosttyTabs.xcodeproj/project.pbxproj
- Sources/Providers/ClaudeProvider.swift
- Sources/cmuxApp.swift
- Sources/ContentView.swift
- Sources/Providers/ProviderAccountsController.swift
- Sources/Sidebar/ProviderAccountsFooterPanel.swift
Comment on lines
+486
to
+490
| let reloaded = ProviderAccountStore( | ||
| userDefaults: userDefaults, | ||
| indexKey: "cmux.tests.providers.accounts.index" | ||
| ) | ||
| reloaded.keychainServiceResolver = { [testService] _ in testService! } |
There was a problem hiding this comment.
Inject the test Keychain service before the store starts pruning.
ProviderAccountStore.init(userDefaults:indexKey:) kicks off pruneOrphanAccountsIfNeeded() immediately. Here reloaded is constructed with the default resolver and only then switched to testService, so the background prune can race first, look in the wrong Keychain service, and delete the row you just created as an orphan. That makes this persistence test flaky. Please make the resolver injectable at initialization time (or otherwise suppress pruning for test stores) so the instance never boots with the wrong service mapping.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@cmuxTests/ProviderTests.swift` around lines 486 - 490, The
ProviderAccountStore currently starts pruneOrphanAccountsIfNeeded() during init
and the test sets keychainServiceResolver only after construction, causing a
race; modify ProviderAccountStore.init(userDefaults:indexKey:) to accept an
initial keychainServiceResolver parameter (or a flag to disable pruning) and
assign that resolver to the instance before any pruning runs, ensuring
keychainServiceResolver is injected at construction time (reference
ProviderAccountStore.init(userDefaults:indexKey:), keychainServiceResolver, and
pruneOrphanAccountsIfNeeded()) so tests never start with the wrong service
mapping.
Comment on lines
+123
to
+128
| private static func intUtilization(_ value: Any?) -> Int? { | ||
| guard let number = value as? NSNumber else { return nil } | ||
| if CFGetTypeID(number as CFTypeRef) == CFBooleanGetTypeID() { | ||
| return nil | ||
| } | ||
| return min(max(number.intValue, 0), 100) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Check file size and content
wc -l Sources/Providers/ClaudeUsageFetcher.swiftRepository: manaflow-ai/cmux
Length of output: 106
🏁 Script executed:
# Read the full file to understand context
cat -n Sources/Providers/ClaudeUsageFetcher.swiftRepository: manaflow-ai/cmux
Length of output: 6979
🏁 Script executed:
# Search for usages of intUtilization to understand how it's called
rg "intUtilization" -B 3 -A 3Repository: manaflow-ai/cmux
Length of output: 1936
🏁 Script executed:
# Check for error handling patterns in the file
rg "return nil|throw|invalid_params" Sources/Providers/ClaudeUsageFetcher.swift -B 2 -A 1Repository: manaflow-ai/cmux
Length of output: 2324
Reject fractional utilization payloads instead of truncating them.
The docstring declares this helper accepts an "integer utilization value," yet number.intValue silently truncates fractional values like 12.9 to 12. This masks API schema drift instead of failing fast with .decoding as intended.
Proposed fix
private static func intUtilization(_ value: Any?) -> Int? {
guard let number = value as? NSNumber else { return nil }
if CFGetTypeID(number as CFTypeRef) == CFBooleanGetTypeID() {
return nil
}
- return min(max(number.intValue, 0), 100)
+ let raw = number.doubleValue
+ guard raw.isFinite, raw.rounded(.towardZero) == raw else { return nil }
+ return min(max(Int(raw), 0), 100)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Providers/ClaudeUsageFetcher.swift` around lines 123 - 128, The
intUtilization(_:) helper currently truncates fractional NSNumber values via
number.intValue; update it to reject non-integer/fractional numeric payloads
instead of silently truncating: in intUtilization(_ value: Any?) keep the
NSNumber and CFBoolean checks (CFGetTypeID(...) == CFBooleanGetTypeID()), then
verify the numeric value is integral (e.g. compare number.doubleValue to its
rounded/integral form or use truncatingRemainder(dividingBy: 1) == 0); if it's
fractional return nil so decoding fails, otherwise convert to an Int and clamp
with min(max(..., 0), 100) as before.
Comment on lines
+9
to
+19
| static func isValidAccessToken(_ token: String) -> Bool { | ||
| let trimmed = token.trimmingCharacters(in: .whitespacesAndNewlines) | ||
| guard trimmed.hasPrefix("eyJ") else { return false } | ||
| let illegal = CharacterSet.whitespacesAndNewlines.union(.controlCharacters) | ||
| if trimmed.unicodeScalars.contains(where: { illegal.contains($0) }) { | ||
| return false | ||
| } | ||
| // Keep empty subsequences so consecutive or leading/trailing dots | ||
| // surface as a bad token ("eyJ..abc.def" must not pass as 3 segments). | ||
| let segments = trimmed.split(separator: ".", omittingEmptySubsequences: false) | ||
| return segments.count == 3 && segments.allSatisfy { !$0.isEmpty } |
There was a problem hiding this comment.
Restrict access-token segments to JWT/base64url characters.
Right now anything with the eyJ prefix and three non-empty dot-separated segments passes, including tokens like eyJ;bad.payload.sig. That pushes obviously invalid credentials through to the fetch path and turns input mistakes into opaque 401s.
💡 Proposed fix
static func isValidAccessToken(_ token: String) -> Bool {
let trimmed = token.trimmingCharacters(in: .whitespacesAndNewlines)
guard trimmed.hasPrefix("eyJ") else { return false }
let illegal = CharacterSet.whitespacesAndNewlines.union(.controlCharacters)
if trimmed.unicodeScalars.contains(where: { illegal.contains($0) }) {
return false
}
+ let allowedJWTCharacters = CharacterSet.alphanumerics.union(CharacterSet(charactersIn: "-_"))
// Keep empty subsequences so consecutive or leading/trailing dots
// surface as a bad token ("eyJ..abc.def" must not pass as 3 segments).
let segments = trimmed.split(separator: ".", omittingEmptySubsequences: false)
- return segments.count == 3 && segments.allSatisfy { !$0.isEmpty }
+ return segments.count == 3 && segments.allSatisfy { segment in
+ !segment.isEmpty && segment.unicodeScalars.allSatisfy { allowedJWTCharacters.contains($0) }
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Providers/CodexProvider.swift` around lines 9 - 19, The
isValidAccessToken function currently only checks prefix and dot segments,
allowing characters like ';' in JWT parts; restrict each of the three
dot-separated segments (variable segments) to only base64url characters (A–Z,
a–z, 0–9, '-' and '_') and optionally '=' padding if you want to accept padded
tokens. Modify isValidAccessToken (use trimmed, segments) to build an allowed
CharacterSet or regex for base64url (plus '=' if desired) and return false if
any segment contains a scalar outside that set; keep the existing prefix check
and the three non-empty segments requirement.
Comment on lines
+355
to
+363
| private static func readLoad(for accountId: UUID, service: String) throws -> ProviderSecret { | ||
| var query = matchQuery(service: service, accountId: accountId) | ||
| query[kSecReturnData] = true | ||
| query[kSecMatchLimit] = kSecMatchLimitOne | ||
| var result: CFTypeRef? | ||
| let status = SecItemCopyMatching(query as CFDictionary, &result) | ||
| guard status == errSecSuccess, let data = result as? Data else { | ||
| throw ProviderAccountStoreError.keychain(status) | ||
| } |
There was a problem hiding this comment.
Treat missing Keychain items as .notFound.
When a stale index row reaches secret(for:) before orphan pruning finishes, this path currently turns errSecItemNotFound into a generic Keychain error. That surfaces a misleading permissions-style message instead of the normal missing-account flow.
💡 Proposed fix
private static func readLoad(for accountId: UUID, service: String) throws -> ProviderSecret {
var query = matchQuery(service: service, accountId: accountId)
query[kSecReturnData] = true
query[kSecMatchLimit] = kSecMatchLimitOne
var result: CFTypeRef?
let status = SecItemCopyMatching(query as CFDictionary, &result)
- guard status == errSecSuccess, let data = result as? Data else {
+ if status == errSecItemNotFound {
+ throw ProviderAccountStoreError.notFound
+ }
+ guard status == errSecSuccess, let data = result as? Data else {
throw ProviderAccountStoreError.keychain(status)
}
return try decodeSecret(data)
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Providers/ProviderAccountStore.swift` around lines 355 - 363, In
readLoad(for:service:) map the SecItemCopyMatching status so that
errSecItemNotFound is converted to ProviderAccountStoreError.notFound instead of
a generic keychain error; update the guard/throw logic in the
ProviderAccountStore.readLoad method to check for status == errSecSuccess, but
if status == errSecItemNotFound throw ProviderAccountStoreError.notFound,
otherwise throw ProviderAccountStoreError.keychain(status) so callers of
secret(for:) get the missing-account flow.
Comment on lines
+335
to
+353
| private var localizedImpact: String { | ||
| switch incident.impact { | ||
| case "critical": return String(localized: "providers.incident.impact.critical", defaultValue: "Critical") | ||
| case "major": return String(localized: "providers.incident.impact.major", defaultValue: "Major") | ||
| case "minor": return String(localized: "providers.incident.impact.minor", defaultValue: "Minor") | ||
| case "none": return String(localized: "providers.incident.impact.none", defaultValue: "None") | ||
| default: return incident.impact.capitalized | ||
| } | ||
| } | ||
|
|
||
| private var localizedStatus: String { | ||
| switch incident.status { | ||
| case "investigating": return String(localized: "providers.incident.status.investigating", defaultValue: "Investigating") | ||
| case "identified": return String(localized: "providers.incident.status.identified", defaultValue: "Identified") | ||
| case "monitoring": return String(localized: "providers.incident.status.monitoring", defaultValue: "Monitoring") | ||
| case "resolved": return String(localized: "providers.incident.status.resolved", defaultValue: "Resolved") | ||
| case "postmortem": return String(localized: "providers.incident.status.postmortem", defaultValue: "Postmortem") | ||
| default: return incident.status.capitalized | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
What are the documented possible values for Statuspage incident impactandstatus fields?
💡 Result:
The documented possible values for Statuspage incident fields are: Impact: none, minor, major, critical, maintenance Status (for realtime incidents): investigating, identified, monitoring, resolved Status (for scheduled incidents): scheduled, in_progress, verifying, completed Status (for incident updates): investigating, identified, monitoring, resolved, postmortem, scheduled
Citations:
- 1: https://doers.statuspage.io/api/v1/incidents/
- 2: https://support.atlassian.com/statuspage/docs/top-level-status-and-incident-impact-calculations/
🏁 Script executed:
# Find and examine the full context of ProviderAccountsPopover.swift
find . -name "ProviderAccountsPopover.swift" -type fRepository: manaflow-ai/cmux
Length of output: 107
🏁 Script executed:
# Once we locate it, check the broader context around lines 335-353
# and identify where 'incident' data comes from
head -400 Sources/Sidebar/ProviderAccountsPopover.swift | tail -100Repository: manaflow-ai/cmux
Length of output: 4270
🏁 Script executed:
# Search for incident model definition
rg "struct.*[Ii]ncident|class.*[Ii]ncident" --type swift -A 5Repository: manaflow-ai/cmux
Length of output: 874
🏁 Script executed:
# Check what keys exist in Localizable.xcstrings for incident impact/status
rg "providers\.incident\.(impact|status)" Resources/Localizable.xcstringsRepository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Check if Localizable.xcstrings exists and search differently
find . -name "Localizable.xcstrings" -type fRepository: manaflow-ai/cmux
Length of output: 93
🏁 Script executed:
# Look for any incident-related keys in the xcstrings file
cat Resources/Localizable.xcstrings | rg -i "incident" -A 3 -B 1Repository: manaflow-ai/cmux
Length of output: 190
🏁 Script executed:
# Also check the ProviderIncident definition more thoroughly
cat Sources/Providers/UsageProvider.swift | grep -A 20 "struct ProviderIncident"Repository: manaflow-ai/cmux
Length of output: 487
🏁 Script executed:
# Look for where incidents are parsed/decoded from the API
rg "ProviderIncident\|incident\." Sources/ -A 2 -B 2 | head -80Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Search for where ProviderIncident is created/decoded
rg "ProviderIncident\(" Sources/ -B 3 -A 3Repository: manaflow-ai/cmux
Length of output: 648
🏁 Script executed:
# Look for API parsing, Statuspage API calls
rg "statuspage|incident" Sources/ -i --type swift | head -40Repository: manaflow-ai/cmux
Length of output: 4553
🏁 Script executed:
# Get the full structure of UsageProvider to understand incident sources
wc -l Sources/Providers/UsageProvider.swiftRepository: manaflow-ai/cmux
Length of output: 100
🏁 Script executed:
# Check around the incident parsing in UsageProvider
sed -n '1,100p' Sources/Providers/UsageProvider.swiftRepository: manaflow-ai/cmux
Length of output: 1931
🏁 Script executed:
# Examine StatuspageIOFetcher to understand what incident data is fetched
cat Sources/Providers/StatuspageIOFetcher.swiftRepository: manaflow-ai/cmux
Length of output: 3269
🏁 Script executed:
# Check what providers are configured and which ones use Statuspage
rg "fetchStatus.*StatuspageIOFetcher\|StatuspageIOFetcher\.fetch" Sources/ -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Check if there are localization keys for "maintenance" impact or other missing status values
cat Resources/Localizable.xcstrings | grep -E "(maintenance|scheduled|in_progress|verifying|completed)" -B 2 -A 4Repository: manaflow-ai/cmux
Length of output: 42
🏁 Script executed:
# Search for where the incident statuses and impacts are used/filtered
rg "resolved|postmortem|investigating|identified|monitoring" Sources/ --type swift | grep -i status | head -20Repository: manaflow-ai/cmux
Length of output: 1499
🏁 Script executed:
# Check if there are actual test cases or examples of incident data
rg "ProviderIncident\(" Sources/ -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 557
🏁 Script executed:
# Verify what the actual Statuspage.io API response looks like by checking for any mock data
find . -name "*.json" -o -name "*.swift" | xargs rg "investigating|identified" 2>/dev/null | head -20Repository: manaflow-ai/cmux
Length of output: 1544
🏁 Script executed:
# Check the full incident switch statement context to understand impact color mapping
sed -n '345,365p' Sources/Sidebar/ProviderAccountsPopover.swiftRepository: manaflow-ai/cmux
Length of output: 1065
🏁 Script executed:
# Check if there are any recent incidents from Claude or OpenAI to see what values actually occur
rg "impact|status" Sources/Providers/ --type swift | grep -E "claude|openai" -i -A 3 -B 3Repository: manaflow-ai/cmux
Length of output: 2145
🏁 Script executed:
# Verify impact/status values from the Statuspage API spec for the integrated hosts
# Check if there's any documentation or comments about expected values
rg "critical|major|minor|investigating|identified|monitoring" Sources/ --type swift -B 3 -A 1 | grep -E "(comment|//|status|impact)" | head -30Repository: manaflow-ai/cmux
Length of output: 3402
🏁 Script executed:
# Double-check: are "resolved" and "postmortem" actually used somewhere after filtering?
rg "resolved|postmortem" Sources/Sidebar/ProviderAccountsPopover.swift -B 2 -A 2Repository: manaflow-ai/cmux
Length of output: 595
Remove dead code and add missing impact localization.
The "resolved" and "postmortem" cases in localizedStatus are unreachable—StatuspageIOFetcher explicitly filters out incidents with these statuses before they reach the UI. Remove those cases.
Additionally, the documented Statuspage API includes "maintenance" as a possible impact value, but it is not localized in the localizedImpact switch. Add:
case "maintenance": return String(localized: "providers.incident.impact.maintenance", defaultValue: "Maintenance")For the default cases: consider replacing with a localized fallback like String(localized: "providers.incident.unknown", defaultValue: "Unknown") to remain defensible if the API introduces new enum values.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Sources/Sidebar/ProviderAccountsPopover.swift` around lines 335 - 353,
localizedStatus contains unreachable cases ("resolved" and "postmortem") because
StatuspageIOFetcher filters those out—remove those case branches from the
localizedStatus switch in ProviderAccountsPopover to avoid dead code;
localizedImpact is missing the "maintenance" impact—add a case for "maintenance"
that returns String(localized: "providers.incident.impact.maintenance",
defaultValue: "Maintenance"); finally replace the current default returns in
both localizedImpact and localizedStatus with a localized fallback
String(localized: "providers.incident.unknown", defaultValue: "Unknown") to
handle unexpected API values defensibly; refer to the localizedImpact and
localizedStatus properties and StatuspageIOFetcher behavior when making these
edits.
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
1 participant
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.
Summary
What changed? Adds an optional AI Usage Monitoring panel in the sidebar footer that tracks subscription usage (Session + Week windows) for your AI coding agents, with support for multiple accounts per provider (e.g. Personal + Work).
Ships two providers out of the box:
claude.ai5-hour and weekly rate limits.Introduces a generic
UsageProvider/ProviderRegistryabstraction so new providers can be added without touching UI code (seedocs/providers.md).Integrates provider status pages (
status.claude.com,status.openai.com) so incidents show up next to the usage bar.All credentials are stored only in macOS Keychain under a per-provider service name — never written to disk in plaintext, never logged, only sent to the provider's own API.
New settings section Settings → AI Usage Monitoring for adding/editing/removing accounts.
Full setup docs at
docs/usage-monitoring-setup.md, linked from README. The guide includes step-by-step instructions on how to obtain every credential needed for each provider (Claude session key + organization ID, Codex access token + account ID)Why? cmux positions itself as a terminal "for managing multiple AI coding agents". Running several agents against Claude and Codex subscriptions makes it very easy to hit the 5-hour or weekly limit mid-task without warning. Surfacing real-time usage and pace directly in the sidebar — where the agents live — lets users plan work against the quota instead of being surprised by a rate-limit error.
Personal note. I've been running this in my own locally-built cmux for about a week across my daily work with Claude and Codex, and it's genuinely changed how I pace agent runs — I stopped getting surprised by 5-hour limits mid-task. I'd be happy if it turns out useful for anyone else too. Open to feedback on scope, API choices, or anything that would make it easier to merge.
Screenshots
Sidebar footer with usage bars:
Per-row popover with reset times and provider status (Claude / Codex):
Settings → AI Usage Monitoring with multiple accounts:
Add-profile sheet (Claude / Codex):
Collapsed sidebar (only status dots visible):
Testing
cmuxTests/ProviderTests.swiftcover the provider registry, credential validators, usage color threshold settings, and ISO8601 parsing.Today HH:mm/Tomorrow HH:mm/MMM d, HH:mmformats.Demo Video
Not applicable — this change has no dynamic behavior to record. The screenshots above and the
docs/usage-monitoring-setup.mdguide cover every state (empty, filled, collapsed, popover, editor sheet).Review Trigger (Copy/Paste as PR comment)
Checklist
Summary by cubic
Adds a sidebar footer panel to track Claude and Codex subscription usage per account, with Session (5h) and Week bars, reset times, incident badges, and per-provider popovers. Credentials are stored in macOS Keychain; background polling starts on launch and stops on quit.
New Features
UsageProvider+ProviderRegistrywith background polling, short timeouts, Statuspage.io integration, and ephemeral HTTP sessions; disabled during tests.Docs & Tests
docs/providers.md,docs/usage-monitoring-setup.md; README section with screenshot and links.Written for commit 6a6d9c0. Summary will update on new commits.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Tests