fix: correct usage analytics and plan detection#34
Open
arafa-dev wants to merge 8 commits into
Open
Conversation
📝 WalkthroughWalkthroughThis PR refactors the usage tracking and quota system. It introduces plan tier detection from local config, splits token display into billable and cache categories, switches event identity to message-based deduplication with upserts, and upgrades the database schema to consolidate events into sessions. ChangesUsage Quota and Event Tracking v3
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
internal/cli/usage_internal_test.go (1)
54-67: ⚡ Quick winConsider table-driven test structure for pure functions.
The test correctly validates
billableTokensandcacheTokens, but a table-driven approach would be more idiomatic and make it easier to add additional test cases (e.g., zero tokens, boundary values). As per coding guidelines, pure functions should use table-driven tests.♻️ Suggested table-driven refactor
func TestBillableAndCacheSplit(t *testing.T) { - u := contracts.Usage{ - InputTokens: 1_000_000, - OutputTokens: 4_000_000, - CacheReadTokens: 600_000_000, - CacheCreateTokens: 50_000_000, - } - if got := billableTokens(u); got != 5_000_000 { - t.Fatalf("billableTokens = %d, want 5_000_000", got) - } - if got := cacheTokens(u); got != 650_000_000 { - t.Fatalf("cacheTokens = %d, want 650_000_000", got) - } + tests := []struct { + name string + usage contracts.Usage + wantBillable int + wantCache int + }{ + { + name: "standard_split", + usage: contracts.Usage{ + InputTokens: 1_000_000, + OutputTokens: 4_000_000, + CacheReadTokens: 600_000_000, + CacheCreateTokens: 50_000_000, + }, + wantBillable: 5_000_000, + wantCache: 650_000_000, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + if got := billableTokens(tt.usage); got != tt.wantBillable { + t.Errorf("billableTokens() = %d, want %d", got, tt.wantBillable) + } + if got := cacheTokens(tt.usage); got != tt.wantCache { + t.Errorf("cacheTokens() = %d, want %d", got, tt.wantCache) + } + }) + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/cli/usage_internal_test.go` around lines 54 - 67, Refactor TestBillableAndCacheSplit into a table-driven test: create a slice of test cases each with a name, input contracts.Usage, expected billable and expected cache ints, then loop over cases and run t.Run(case.name, func(t *testing.T){ if got:=billableTokens(case.u); got!=case.wantBillable { t.Fatalf(...)} if got:=cacheTokens(case.u); got!=case.wantCache { t.Fatalf(...)} }), include at least the existing case plus extras (zero tokens and a boundary case) to exercise billableTokens and cacheTokens more idiomatically.internal/quotawire/adapter.go (1)
43-51: Detected-tier override logic is correct; note the per-request disk read.
Detectis invoked for every profile on everyQuotacall, so eachGET /api/usage/--quotainvocation re-reads.claude.jsonfrom disk per profile. With few profiles this is negligible, but if the dashboard polls frequently you may want to cache detection results (e.g., per profile with a short TTL or invalidation on config mtime) to keep the quota path cheap.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/quotawire/adapter.go` around lines 43 - 51, The current loop calls plandetect.Detect(profiles[i].ConfigDir) for every profile on every Quota call (i.e., each GET /api/usage or --quota), causing a disk read per profile; cache detection results keyed by profile (e.g., profile ID or ConfigDir) with a short TTL or invalidate on config file mtime to avoid re-reading .claude.json on every invocation, and update the loop that sets profiles[i].Limits.PlanTier to use the cached value (fall back to plandetect.Detect only when cache miss or stale).internal/plandetect/detect_test.go (1)
1-1: ⚡ Quick winUse the
_testpackage suffix.This new test only exercises the exported
Detect, so it can live in an external test package to enforce testing through the public API.♻️ Switch to external test package
-package plandetect +package plandetect_test + +import "github.com/arafa-dev/ccx/internal/plandetect"Call sites become
plandetect.Detect(dir).As per coding guidelines: "use
_testpackage suffix".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/plandetect/detect_test.go` at line 1, Change the test package from plandetect to an external test package by renaming the package declaration to plandetect_test and update all call sites to reference the exported API (e.g., call plandetect.Detect(dir) instead of Detect(dir)) so the test exercises the public Detect function through the package boundary.internal/storage/events.go (1)
38-46: ⚡ Quick winNo merge change needed: duplicates only vary
output_tokens, notinput_tokens/cache_*
internal/scanner/parse_test.goshows duplicate JSONL lines sharing the samemessage.idhave identicalinput_tokens,cache_read_input_tokens, andcache_creation_input_tokens, whileoutput_tokensgrows; therefore the currentON CONFLICTbehavior (keepingMAX(output_tokens)but overwriting the other fields) is order-independent.internal/scanner/parse.gocomment “same … usage” is slightly misleading given the test case; clarify it to say onlyoutput_tokensvaries across duplicate lines.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@internal/storage/events.go` around lines 38 - 46, The ON CONFLICT clause in internal/storage/events.go is already correct (it preserves MAX(output_tokens) while overwriting other fields), but the comment in internal/scanner/parse.go is misleading; update the parse.go comment to state that duplicate JSONL lines with the same message.id have identical input_tokens, cache_read_input_tokens, and cache_creation_input_tokens while only output_tokens grows across duplicates, and optionally add a short inline comment next to the ON CONFLICT SQL (in events.go) noting we only need to take MAX for output_tokens while other token counts are identical so order-independence is preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@internal/plandetect/detect.go`:
- Around line 21-35: Change Detect to accept ctx context.Context as its first
parameter and add "context" to imports; inside Detect, check ctx.Err() and
return early if canceled before performing os.ReadFile, then proceed to read and
unmarshal as before and return mapOrgType result. Update the caller in
internal/quotawire/adapter.go to pass the request context into Detect and adjust
tests in detect_test.go to provide a context.Context argument to Detect. Ensure
the function name Detect and the mapping call to mapOrgType remain unchanged.
---
Nitpick comments:
In `@internal/cli/usage_internal_test.go`:
- Around line 54-67: Refactor TestBillableAndCacheSplit into a table-driven
test: create a slice of test cases each with a name, input contracts.Usage,
expected billable and expected cache ints, then loop over cases and run
t.Run(case.name, func(t *testing.T){ if got:=billableTokens(case.u);
got!=case.wantBillable { t.Fatalf(...)} if got:=cacheTokens(case.u);
got!=case.wantCache { t.Fatalf(...)} }), include at least the existing case plus
extras (zero tokens and a boundary case) to exercise billableTokens and
cacheTokens more idiomatically.
In `@internal/plandetect/detect_test.go`:
- Line 1: Change the test package from plandetect to an external test package by
renaming the package declaration to plandetect_test and update all call sites to
reference the exported API (e.g., call plandetect.Detect(dir) instead of
Detect(dir)) so the test exercises the public Detect function through the
package boundary.
In `@internal/quotawire/adapter.go`:
- Around line 43-51: The current loop calls
plandetect.Detect(profiles[i].ConfigDir) for every profile on every Quota call
(i.e., each GET /api/usage or --quota), causing a disk read per profile; cache
detection results keyed by profile (e.g., profile ID or ConfigDir) with a short
TTL or invalidate on config file mtime to avoid re-reading .claude.json on every
invocation, and update the loop that sets profiles[i].Limits.PlanTier to use the
cached value (fall back to plandetect.Detect only when cache miss or stale).
In `@internal/storage/events.go`:
- Around line 38-46: The ON CONFLICT clause in internal/storage/events.go is
already correct (it preserves MAX(output_tokens) while overwriting other
fields), but the comment in internal/scanner/parse.go is misleading; update the
parse.go comment to state that duplicate JSONL lines with the same message.id
have identical input_tokens, cache_read_input_tokens, and
cache_creation_input_tokens while only output_tokens grows across duplicates,
and optionally add a short inline comment next to the ON CONFLICT SQL (in
events.go) noting we only need to take MAX for output_tokens while other token
counts are identical so order-independence is preserved.
🪄 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: 43fcf20d-4e07-4525-8344-ad1884f14e28
📒 Files selected for processing (15)
internal/cli/usage.gointernal/cli/usage_internal_test.gointernal/plandetect/detect.gointernal/plandetect/detect_test.gointernal/plandetect/doc.gointernal/quotawire/adapter.gointernal/quotawire/adapter_test.gointernal/run/process_launcher.gointernal/scanner/parse.gointernal/scanner/parse_test.gointernal/storage/events.gointernal/storage/events_test.gointernal/storage/migrate.gointernal/storage/migrate_test.gointernal/storage/testhelpers_test.go
Comment on lines
+21
to
+35
| func Detect(configDir string) (tier string, ok bool) { | ||
| if configDir == "" { | ||
| return "", false | ||
| } | ||
| // #nosec G304 -- configDir is a local Claude profile path selected by the user. | ||
| b, err := os.ReadFile(filepath.Join(configDir, ".claude.json")) | ||
| if err != nil { | ||
| return "", false | ||
| } | ||
| var cfg claudeConfig | ||
| if err := json.Unmarshal(b, &cfg); err != nil { | ||
| return "", false | ||
| } | ||
| return mapOrgType(cfg.OAuthAccount.OrganizationType, cfg.OAuthAccount.OrganizationRateLimit) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Detect performs file I/O but does not accept a context.Context.
Detect reads .claude.json via os.ReadFile, so per the project convention it should take ctx context.Context as its first parameter (and can short-circuit on ctx.Err() before the read). This also lets the quotawire caller propagate its request context.
♻️ Add ctx to the signature and propagate from the caller
-func Detect(configDir string) (tier string, ok bool) {
+func Detect(ctx context.Context, configDir string) (tier string, ok bool) {
if configDir == "" {
return "", false
}
+ if err := ctx.Err(); err != nil {
+ return "", false
+ }
// `#nosec` G304 -- configDir is a local Claude profile path selected by the user.
b, err := os.ReadFile(filepath.Join(configDir, ".claude.json"))Add "context" to imports. Downstream caller in internal/quotawire/adapter.go (Line 48) becomes:
- if tier, ok := plandetect.Detect(profiles[i].ConfigDir); ok {
+ if tier, ok := plandetect.Detect(ctx, profiles[i].ConfigDir); ok {and the tests in detect_test.go need the ctx argument as well.
As per coding guidelines: "Every I/O or blocking public function must take ctx context.Context as its first parameter".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| func Detect(configDir string) (tier string, ok bool) { | |
| if configDir == "" { | |
| return "", false | |
| } | |
| // #nosec G304 -- configDir is a local Claude profile path selected by the user. | |
| b, err := os.ReadFile(filepath.Join(configDir, ".claude.json")) | |
| if err != nil { | |
| return "", false | |
| } | |
| var cfg claudeConfig | |
| if err := json.Unmarshal(b, &cfg); err != nil { | |
| return "", false | |
| } | |
| return mapOrgType(cfg.OAuthAccount.OrganizationType, cfg.OAuthAccount.OrganizationRateLimit) | |
| } | |
| func Detect(ctx context.Context, configDir string) (tier string, ok bool) { | |
| if configDir == "" { | |
| return "", false | |
| } | |
| if err := ctx.Err(); err != nil { | |
| return "", false | |
| } | |
| // `#nosec` G304 -- configDir is a local Claude profile path selected by the user. | |
| b, err := os.ReadFile(filepath.Join(configDir, ".claude.json")) | |
| if err != nil { | |
| return "", false | |
| } | |
| var cfg claudeConfig | |
| if err := json.Unmarshal(b, &cfg); err != nil { | |
| return "", false | |
| } | |
| return mapOrgType(cfg.OAuthAccount.OrganizationType, cfg.OAuthAccount.OrganizationRateLimit) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@internal/plandetect/detect.go` around lines 21 - 35, Change Detect to accept
ctx context.Context as its first parameter and add "context" to imports; inside
Detect, check ctx.Err() and return early if canceled before performing
os.ReadFile, then proceed to read and unmarshal as before and return mapOrgType
result. Update the caller in internal/quotawire/adapter.go to pass the request
context into Detect and adjust tests in detect_test.go to provide a
context.Context argument to Detect. Ensure the function name Detect and the
mapping call to mapOrgType remain unchanged.
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
message.id, merge duplicate event rows with maxoutput_tokens, and add schema v3 reset/re-scan support that preserves shared-history session ownership.ccx usage --quotaheadline token count.Verification
make cimake build./dist/ccx usage --since 30d./dist/ccx usage --quotaGET /api/usageandGET /api/healthNotes
claude_pro -> proand otherwise falls back to manual config.Summary by CodeRabbit
New Features
Bug Fixes