Extract Task usage tracking#28
Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughTask delegates token/tool usage tracking to a new UsageTracker that computes metrics, debounces emissions, exposes token/tool state, is instantiated/disposed by Task; tests updated and new UsageTracker unit tests added. ChangesUsage Tracking Extraction
Sequence Diagram(s)sequenceDiagram
participant Client
participant Task
participant UsageTracker
participant EventBus
Client->>Task: call recordToolUsage / getTokenUsage / combineMessages
Task->>UsageTracker: delegate call
UsageTracker->>EventBus: emit TaskTokenUsageUpdated / TaskToolFailed
EventBus-->>Client: notifications/handlers
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
![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 (1)
src/core/task/__tests__/Task.throttle.test.ts (1)
299-326: ⚡ Quick winAssert the public Task contract instead of
usageTrackerinternals.These cases now cast through
anyand inspect private snapshot fields, so they won’t catch regressions in the delegatedTask.tokenUsage/ final-emission behavior even though preserving the Task surface is the goal of this PR. Prefer assertingtask.tokenUsageand emitted payloads here, or move snapshot-specific checks into dedicatedUsageTrackerunit tests.Also applies to: 461-495
🤖 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 `@src/core/task/__tests__/Task.throttle.test.ts` around lines 299 - 326, The test is inspecting private internals (usageTracker.tokenUsageSnapshot) via casts; change assertions to use the public Task API instead: call task.tokenUsage and assert its values change (and/or assert the emitted payloads from addToClineMessages) rather than touching usageTracker.tokenUsageSnapshot; if you need snapshot-specific behavior keep those assertions in a dedicated UsageTracker unit test that directly exercises UsageTracker methods (or expose a test-only accessor there) and remove any (task as any).usageTracker assertions from this Task.throttle.test.ts file.
🤖 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 `@src/core/task/UsageTracker.ts`:
- Around line 105-113: The tokenUsage getter in UsageTracker currently returns
the cached tokenUsageSnapshot forever once set, causing stale values; change the
getter to always return this.getTokenUsage() (i.e., recompute on reads) while
leaving tokenUsageSnapshot and tokenUsageSnapshotAt in place for emitted-state
comparisons and for whichever emit/save logic relies on them; keep
getLastMessageTs(), getTokenUsage(), and any emit/update code unchanged so only
the read path of tokenUsage is modified to return fresh data.
---
Nitpick comments:
In `@src/core/task/__tests__/Task.throttle.test.ts`:
- Around line 299-326: The test is inspecting private internals
(usageTracker.tokenUsageSnapshot) via casts; change assertions to use the public
Task API instead: call task.tokenUsage and assert its values change (and/or
assert the emitted payloads from addToClineMessages) rather than touching
usageTracker.tokenUsageSnapshot; if you need snapshot-specific behavior keep
those assertions in a dedicated UsageTracker unit test that directly exercises
UsageTracker methods (or expose a test-only accessor there) and remove any (task
as any).usageTracker assertions from this Task.throttle.test.ts file.
🪄 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 Plus
Run ID: d244e906-88f4-44aa-8554-61cadf064b40
📥 Commits
Reviewing files that changed from the base of the PR and between 81afb6c and 60ded846820391282bbc03fd1e40c3eb49225be3.
📒 Files selected for processing (3)
src/core/task/Task.tssrc/core/task/UsageTracker.tssrc/core/task/__tests__/Task.throttle.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/core/task/Task.ts`:
- Around line 531-535: Add proper disposal for the UsageTracker debounce:
implement a cancel() or dispose() method on the UsageTracker class that calls
this.debouncedEmitTokenUsage.cancel() (and any other cleanup), and then call
that new method from Task.dispose() (alongside existing cleanup like
fileContextTracker.dispose(), e.g., right after that call) so pending lodash
debounce timers are cancelled when a Task is disposed; update any references to
UsageTracker (created in the Task constructor where this.usageTracker is
assigned) and ensure Task.dispose() invokes usageTracker.cancel()/dispose().
🪄 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 Plus
Run ID: 50612190-45c1-4b03-a572-d9e2cb4d25aa
📥 Commits
Reviewing files that changed from the base of the PR and between 60ded846820391282bbc03fd1e40c3eb49225be3 and d0705354a7ed696af15d929a409a9870cdeea91b.
📒 Files selected for processing (3)
src/core/task/Task.tssrc/core/task/UsageTracker.tssrc/core/task/__tests__/UsageTracker.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/task/UsageTracker.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/core/task/UsageTracker.ts`:
- Around line 68-79: UsageTracker can be reactivated after dispose because
dispose() only cancels pending debounce work; change dispose() to mark the
tracker inert (e.g., set a this.disposed flag) and then make
emitTokenUsageUpdate and emitFinalTokenUsageUpdate check that flag and return
immediately when disposed, and ensure dispose still calls
debouncedEmitTokenUsage.cancel() and debouncedEmitTokenUsage.flush() (or
replaces debouncedEmitTokenUsage with a no-op object) so no new timers or
flushes can be scheduled after disposal; update references in the class to use
the disposed flag and guard all public emission methods (emitTokenUsageUpdate,
emitFinalTokenUsageUpdate) accordingly.
🪄 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 Plus
Run ID: 6261fe1c-d3d2-4a01-8871-616e88ece917
📥 Commits
Reviewing files that changed from the base of the PR and between d0705354a7ed696af15d929a409a9870cdeea91b and 76bf595c2d13926a3bcb2679fa97fb8fa6687043.
📒 Files selected for processing (4)
src/core/task/Task.tssrc/core/task/UsageTracker.tssrc/core/task/__tests__/Task.dispose.test.tssrc/core/task/__tests__/UsageTracker.spec.ts
|
Addressed the dispose follow-up in
Validation: |
edelauna
force-pushed
the
issue-18-usage-tracker
branch
from
86ddd51 to
9133ae7
Compare
edelauna
requested review from
JamesRobert20,
edelauna,
navedmerchant and
taltas
as code owners
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
|
Converting to draft - lets resolve conflicts before reopening. |
2 participants
Related GitHub Issue
Refs #18
Description
Extract token and tool usage tracking from
Task.tsintosrc/core/task/UsageTracker.ts.The new tracker owns:
TaskTaskkeeps the existing public methods/properties (getTokenUsage,recordToolUsage,recordToolError,emitFinalTokenUsageUpdate,toolUsage,tokenUsage) as thin delegates so existing call sites keep the same surface.Test Procedure
pnpm --dir src check-typespnpm --dir src exec vitest run core/task/__tests__/Task.throttle.test.tsNotes
This is scoped to the usage-tracking extraction described in #18.
Summary by CodeRabbit
Refactor
Tests