feat(dashboard): add local usage statistics dashboard with session detail - #1225
feat(dashboard): add local usage statistics dashboard with session detail#1225myk1yt wants to merge 32 commits into
Conversation
qwen3-coder-plus and qwen3-coder-flash reported zero prices, which made usage-stat cost recalculation return 0 for those models. Required by the cost recalculation tests in the stats engine.
Data-minimized usage event V1 schema (no prompts, responses, or credentials), stats query/snapshot contracts, and dashboard summary/detail/stream types shared between extension host and webview.
- UsageEventStore: append-only segmented NDJSON with lock/queue, idempotency keys, 5MiB rotation, 100MiB cap, corrupt-line quarantine - UsageStatsDatabase/Migration/Projection: transactional node:sqlite rollup (lazy-loaded, degrades gracefully when unavailable) - UsageStatsStreamCoordinator: epoch-guarded subscription streaming - UsageAggregator/costRecalculation/statsQueryRange: provider-aware cost and timezone-aware range math - DashboardTaskCatalog/Projection: History-first task catalog fed by TaskHistoryStore.onDidChange (new small event surface) - UsageCapture: endpoint-domain extraction for custom base URLs
OpenAI, OpenAI Codex, Anthropic Vertex, Mistral, Moonshot, and Kenari handlers now attach totalCost (via shared/cost) to their usage chunks so recorded usage events carry provider-calculated cost.
Initializes UsageStatsService and DashboardTaskCatalog alongside TaskHistoryStore, forwards cross-window change notifications to the webview, and disposes both with the provider. Initialization failure is non-fatal: the service becomes unavailable and handlers degrade gracefully.
Single instrumentation point: the captureUsageData boundary (completed and failed/cancelled paths) records exactly one event per API attempt, keyed by taskId:apiReqIndex:retryAttempt so tool-use turns do not dedupe away. Recording is fire-and-forget and silently skipped when the stats service is unavailable.
- usageStatsMessageHandler: query/clear/export/nonce handlers, session and History-first task detail, and subscribe/pause/resume/ resync stream handlers with typed error codes - webviewMessageHandler delegates stats message types through a single dispatcher guard (+7 lines) - ExtensionMessage/WebviewMessage contracts for the stats and dashboard request/response payloads
- DashboardView with summary cards, time-range filter, breakdowns, History-first task list, and session/task detail - Daily-activity heatmap (UsageHeatmap), animated counters, and stream reducer with epoch-guarded incremental updates - dashboardButtonClicked command wired through package.json menus, registerCommands, and App tab routing; ErrorBoundary gains onRetry
stats.json and dashboard.json for all 18 locales plus the shared error-boundary retry string in common.json.
UsageHeatmap was the only component under components/stats/ and is used exclusively by DashboardView. Moving it keeps all dashboard UI under a single components/dashboard/ directory.
The dashboardButtonClicked contribution referenced %command.dashboard.title% but the key was missing from the default package.nls.json, which failed vsce package. The 18 locale files already carry their translations.
- Rollup fast path now serves cacheRatio queries: stats_rollup gains unreported_cache_input_tokens (schema v6, rebuilt on migrate) so per-row cacheRatio estimation is exactly equivalent to the per-event path, including mixed-reporting buckets; server-reported cacheRead is always kept verbatim - Ranged event reader (readEventsInRange) replaces full-table scans in the event-scan fallback and in export/getFilteredEvents - Task usage aggregation for bounded ranges runs as GROUP BY SQL with json_extract instead of per-row deserialization; only rows missing cost are recalculated - Memoize Intl.DateTimeFormat per timezone in aggregator/projection
Expanded root tasks now display an aggregate strip above the subtask list: subtree-summed input/output tokens and cost plus every distinct mode and model used across the subtree. Aggregates come from a single grouped SQL pass over usage_events (no schema change); stream deltas flow through the same projection path. Also drops the obsolete 'rebuild' action strings from all locales.
- Export button now downloads JSON scoped to the currently selected time range (Today/7d/30d/custom/All); the backend already supported JSON, and exports now read events via the ranged DB path - Rebuild Stats button and its IPC chain are removed (rollup rebuilds remain an internal migration/coordinator concern) - Clear Statistics opens its warning dialog immediately on click; the clear nonce is requested in parallel and only the confirm action waits for it (host-side nonce validation unchanged)
v6 committed its version marker before rebuilding rollups, so a failed rebuild left the meta at v6 with pre-v6 rollup values and every later activation skipped the migration entirely — breaking cacheRatio estimation (unreported_cache_input_tokens stuck at 0). v6 now rebuilds before committing its marker so a failure is retried, and v7 performs an idempotent self-heal rebuild for databases already stranded.
Places the estimation input between Daily Activity and Breakdown so range/breakdown controls read top-down in one flow.
Events without server-reported cacheRead now receive a ratio-proportional cache discount on cost: cost(ratio) = storedCost - ratio x discountBase, where discountBase = input x max(0, inputPrice - cacheReadsPrice) from the existing pricing tables. Server-reported cacheRead keeps the verbatim path in both tokens and cost. The per-event discount base is persisted on usage_events, stats_rollup, and task_usage_metadata (schema v8, self-healing rebuild) so Breakdown, summary cards, Tasks (rows, parent aggregate strip), and session/task detail all respond to the slider without rescanning events.
📝 WalkthroughWalkthroughAdded a local usage statistics system. It records API usage, stores events, aggregates statistics, streams dashboard updates, and renders a localized dashboard with task and session details. It also adds exports, clearing, cost recalculation, visual tests, and snapshot automation. ChangesUsage contracts and persistence
Extension and webview integration
Validation and tooling
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Task
participant UsageRecorder
participant UsageEventStore
participant UsageStatsService
participant UsageStatsStreamCoordinator
participant DashboardView
Task->>UsageRecorder: finalize terminal usage event
UsageRecorder->>UsageEventStore: append idempotent event
UsageEventStore-->>UsageStatsService: persist usage data
UsageStatsService->>UsageStatsStreamCoordinator: notify appended event
UsageStatsStreamCoordinator-->>DashboardView: send snapshot or delta
DashboardView->>UsageStatsStreamCoordinator: replace or pause subscription
sequenceDiagram
participant DashboardView
participant usageStatsMessageHandler
participant UsageStatsService
participant DashboardTaskProjection
DashboardView->>usageStatsMessageHandler: request dashboard snapshot or task page
usageStatsMessageHandler->>UsageStatsService: resolve service and stream state
usageStatsMessageHandler->>DashboardTaskProjection: compute task page or detail
DashboardTaskProjection-->>usageStatsMessageHandler: return dashboard payload
usageStatsMessageHandler-->>DashboardView: send correlated response
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
src/services/stats/UsageStatsStreamCoordinator.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. webview-ui/playwright-ct.config.tsESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. webview-ui/playwright/ExtensionStateContext.mock.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox.
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 |
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![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
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (29)
src/core/task/Task.ts-3189-3228 (1)
3189-3228: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRecord completed attempts when usage counters are zero or unavailable.
finalizeUsageEvent()is nested incaptureUsageData(). That function only runs when at least one token counter is greater than zero. A completed request with no usage chunk therefore produces no event. The dashboard then undercounts completed calls and omits that activity.Finalize the completed event outside the positive-token guard. Keep token fields unset when their values are zero.
🤖 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/Task.ts` around lines 3189 - 3228, Move the terminal finalize block around usageRecorder.finalizeUsageEvent outside captureUsageData’s positive-token guard so completed attempts are recorded even when all counters are zero or unavailable. Preserve the existing requestKey, status, and UsageRecordingContext construction, while leaving zero-valued token fields unset as required.src/core/webview/usageStatsMessageHandler.ts-1139-1143 (1)
1139-1143: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winReplace the untyped
_streamSinkproperty with a typed field onClineProvider. The stream sink is stored as an undeclared ad-hoc property on the provider and reached from two modules through four double assertions. The two modules even annotate it with different types (ProviderStreamSinkhere,StatsStreamSinkinClineProvider.ts), so the compiler cannot detect a mismatch. Declareprivate statsStreamSink?: StatsStreamSinkonClineProviderwith a getter and setter, then delete every cast. The coding guidelines require avoiding untyped escapes and reserving double assertions for a last resort with an explanatory comment; these casts have neither.
src/core/webview/usageStatsMessageHandler.ts#L1139-L1143: this is the only writer. Replace the read and the write withprovider.getStatsStreamSink()andprovider.setStatsStreamSink(sink).src/core/webview/usageStatsMessageHandler.ts#L1008-L1013: replace the cast inresolveTaskRangeMswithprovider.getStatsStreamSink().src/core/webview/usageStatsMessageHandler.ts#L1021-L1026: replace the cast inresolveTaskCacheRatiowithprovider.getStatsStreamSink().src/core/webview/ClineProvider.ts#L737-L744: read the new private field directly inclearWebviewResourcesand set it toundefinedafter unsubscribing, removing both assertions.🤖 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/webview/usageStatsMessageHandler.ts` around lines 1139 - 1143, Replace the ad-hoc _streamSink casts with a typed private statsStreamSink?: StatsStreamSink field and getStatsStreamSink/setStatsStreamSink accessors on ClineProvider. In src/core/webview/usageStatsMessageHandler.ts at lines 1139-1143, use the accessors for reading and storing the sink; at lines 1008-1013 and 1021-1026, use getStatsStreamSink(). In src/core/webview/ClineProvider.ts at lines 737-744, read the private field directly in clearWebviewResources and reset it to undefined after unsubscribing, removing all double assertions.Source: Coding guidelines
src/core/webview/usageStatsMessageHandler.ts-625-668 (1)
625-668: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winSession titles are derived with sequential disk reads.
Line 649 awaits
deriveSessionTitleonce per session group inside a sequentialforloop. Each call reads and JSON-parses the wholeui_messages.jsonfile for that task.For a user with hundreds of sessions this produces hundreds of serialized file reads on every
getDashboardSessionsrequest, and the dashboard issues that request on each time-range change. Resolve the titles concurrently.⚡ Proposed fix to parallelize title resolution
- const summaries: SessionSummary[] = [] - - for (const [taskId, taskEvents] of groups) { + const summaries: SessionSummary[] = await Promise.all( + Array.from(groups, async ([taskId, taskEvents]) => { // Sort events within a task by occurredAt ascending so the first // event is the earliest (representative model/provider/mode) and // the last event gives the most recent activity timestamp. const sorted = [...taskEvents].sort( (a, b) => new Date(a.occurredAt).getTime() - new Date(b.occurredAt).getTime(), ) const first = sorted[0] const last = sorted[sorted.length - 1] // Aggregate totals across all events in the task. // Feature 1: Use getEffectiveCost to compute missing costs on-the-fly. let totalTokens = 0 let totalCost = 0 for (const ev of sorted) { totalTokens += ev.usage.totalTokens?.value ?? 0 totalCost += applyCacheDiscount( getEffectiveCost(ev, customPricing), computeCacheDiscountBase(ev, customPricing), cacheRatio, ) } const title = await deriveSessionTitle(taskId, globalStoragePath) - summaries.push({ + return { taskId, title, timestamp: new Date(last.occurredAt).getTime(), model: first.model, provider: first.provider, mode: first.mode, models: [...new Set(sorted.map((e) => e.model))], modes: [...new Set(sorted.map((e) => e.mode))], totalTokens, totalCost, callCount: sorted.length, - }) - } + } + }), + )If the session count can be large, bound the concurrency instead of using an unbounded
Promise.all.🤖 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/webview/usageStatsMessageHandler.ts` around lines 625 - 668, Update the session summary construction around deriveSessionTitle so title resolution runs concurrently rather than awaiting each title inside the sequential group loop. Preserve the existing aggregation and summary fields, and use bounded concurrency if the available implementation supports a concurrency limiter; otherwise collect per-group work and await the title results together.src/core/webview/usageStatsMessageHandler.ts-570-578 (1)
570-578: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftResolve the complete parent chain for session grouping.
buildParentMaponly includes tasks with usage events. IfCpoints toM, butMhas no events,resolveRootTaskId(C)returnsMinstead of rootR. The session list andhandleGetDashboardSessionDetailthen omitCfromR's session. UseDashboardTaskCatalogor task history for parent resolution in both paths, and add a regression test for this case.🤖 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/webview/usageStatsMessageHandler.ts` around lines 570 - 578, Update buildParentMap and the resolveRootTaskId/session-grouping flow to resolve parent relationships from DashboardTaskCatalog or task history, not only usage events, so chains traverse eventless intermediate tasks to the true root. Ensure both session listing and handleGetDashboardSessionDetail use the complete parent map, and add a regression test covering C → M (no events) → R.webview-ui/src/components/dashboard/DashboardView.tsx-240-282 (1)
240-282: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winSuperseded task-detail requests leave the row stuck in the loading state.
fetchTaskDetailaddstaskIdtotaskDetailLoadingand overwriteslatestTaskDetailRequestIdRef. The response handler removes the entry only for the request that matcheslatestTaskDetailRequestIdRef. If the user expands task A and then expands task B before A responds, the A response is dropped at Line 325 and A stays intaskDetailLoadingforever.handleToggleTaskthen skips a refetch of A becausetaskDetailLoading.has(taskId)is true, so re-expanding A rendersTaskDetailLoadingpermanently.Track the request id per task instead of a single latest ref, and clear the loading entry for the task the response belongs to.
🐛 Proposed fix
- const latestTaskDetailRequestIdRef = useRef<string>("") - const latestTaskDetailIdRef = useRef<string | undefined>(undefined) + // requestId -> taskId for every in-flight detail request. + const taskDetailRequestsRef = useRef<Map<string, string>>(new Map())const fetchTaskDetail = useCallback((taskId: string) => { const requestId = `dashboard-task-detail-${Date.now()}-${Math.random().toString(36).slice(2, 8)}` - latestTaskDetailRequestIdRef.current = requestId - latestTaskDetailIdRef.current = taskId + taskDetailRequestsRef.current.set(requestId, taskId)if (message.type === "dashboardTaskDetailResponse") { - if (message.requestId !== latestTaskDetailRequestIdRef.current) return - - const taskId = latestTaskDetailIdRef.current - if (!taskId) return + const requestId = message.requestId + if (!requestId) return + const taskId = taskDetailRequestsRef.current.get(requestId) + if (!taskId) return + taskDetailRequestsRef.current.delete(requestId)Also applies to: 320-348
🤖 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 `@webview-ui/src/components/dashboard/DashboardView.tsx` around lines 240 - 282, Update fetchTaskDetail and the response handler to track the active request ID per task rather than using the single latestTaskDetailRequestIdRef. When handling any response, clear taskDetailLoading for that response’s taskId, then ignore stale results by comparing its request ID with that task’s active request ID; preserve the existing behavior for applying only the current task detail response.webview-ui/src/components/dashboard/dashboardStreamReducer.ts-325-350 (1)
325-350: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftUse a dense delivery sequence before adding gap detection.
usage_events.seqis not contiguous:INSERT OR IGNOREcan consume anAUTOINCREMENTvalue. The host can also skip deltas for hidden sinks while advancinglastSequence. Add an explicit per-subscription delivery cursor or gap marker, then setpendingResyncfrom that signal. Do not comparesequencewithstate.sequence + 1directly.🤖 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 `@webview-ui/src/components/dashboard/dashboardStreamReducer.ts` around lines 325 - 350, Replace direct gap detection based on usage_events.sequence with an explicit per-subscription dense delivery cursor or gap marker in the DELTA flow of the dashboard stream reducer. Track the delivery sequence independently from state.sequence, advance it only for delivered deltas, and set pendingResync from the explicit gap signal; retain existing stale, generation, and duplicate handling.src/core/webview/__tests__/usageStatsMessageRouting.spec.ts-456-493 (1)
456-493: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winThis test does not verify production disposal wiring.
The test builds a local object literal with its own
dispose()implementation at lines 467-486, calls it, and then asserts that the literal did what the literal was written to do. No symbol fromUsageStatsServiceorClineProvideris imported or exercised. The test passes even ifUsageStatsService.dispose()is deleted, so it cannot detect the regression the file docblock claims it covers at line 11.Assert against the real service instead. Construct a
UsageStatsServicewith injected coordinator and database doubles, call itsdispose(), and check that both doubles were disposed.🔧 Proposed direction
- // Simulate the service's dispose chain - const service: { - coordinator: typeof coordinator | null - database: typeof mockDb - watcher: { dispose(): void } | null - changeListeners: Array<() => void> - dispose(): void - } = { - coordinator, - database: mockDb, - watcher: null, - changeListeners: [], - dispose() { - this.coordinator?.dispose() - this.coordinator = null - this.watcher?.dispose() - this.watcher = null - this.changeListeners.length = 0 - this.database.close() - }, - } - - service.dispose() + const service = new UsageStatsService(/* ...test deps... */) + // Install the doubles on the real instance, then exercise the real dispose(). + ;(service as unknown as Record<string, unknown>)["coordinator"] = coordinator + ;(service as unknown as Record<string, unknown>)["database"] = mockDb + + service.dispose()If
UsageStatsServicecannot be constructed cheaply in this suite, move the assertion intosrc/services/stats/__tests__/UsageStatsService.spec.ts, which already owns that layer. Do you want me to draft the replacement test?🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/webview/__tests__/usageStatsMessageRouting.spec.ts` around lines 456 - 493, Replace the locally implemented service object in the “coordinator disposal” test with an actual UsageStatsService instance, injecting the existing coordinator and database doubles, then call its dispose() and assert both coordinator.dispose and database.close are invoked once. Import and exercise the production UsageStatsService; if construction is impractical in this suite, move the test to its existing service-level spec.src/core/webview/__tests__/usageStatsMessageRouting.spec.ts-1-1 (1)
1-1: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winBoth usage-stats spec files suppress lint rules instead of satisfying them. The shared root cause is that the stream-handler doubles have no typed shape, so each call site reaches for
as any, and the deliberate fire-and-forget handler calls are never marked. The repository guidelines require fixing these rules rather than disabling them.
src/core/webview/__tests__/usageStatsMessageRouting.spec.ts#L1-L1: remove the@typescript-eslint/no-explicit-anydisable. Replace the 12as anycasts at lines 292, 318, 333, 347, 362, 376, 391, 409, 440, 500, 529, and 561 with a shared typed service double.src/core/webview/__tests__/usageStatsMessageHandler.spec.ts#L1-L1: remove both disables. Replace theas anycasts with the same typed double, and prefix the unawaited handler calls at lines 1336, 1360, 1384, 1466, and 1558 withvoid.Define the double once in a shared helper so both files import it. That also removes the need for
as anyon the_streamSinkassignments at lines 1759 and 1837 of the handler spec; use bracket notation on a typed cast instead.As per coding guidelines: "Fix lint violations in new TypeScript code instead of suppressing them." and "Avoid
as any; use typed APIs, bracket notation for private members where necessary, or precise test doubles andunknowntype guards."🤖 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/webview/__tests__/usageStatsMessageRouting.spec.ts` at line 1, Remove the lint suppressions in src/core/webview/__tests__/usageStatsMessageRouting.spec.ts:1-1 and src/core/webview/__tests__/usageStatsMessageHandler.spec.ts:1-1. Define one shared typed stream-handler test double, import and use it at all listed as-any call sites in both specs, and replace the handler spec’s _streamSink casts at lines 1759 and 1837 with bracket notation on a precise typed cast. Prefix the unawaited handler calls in usageStatsMessageHandler.spec.ts at lines 1336, 1360, 1384, 1466, and 1558 with void.Source: Coding guidelines
src/services/stats/__tests__/UsageRecorder.spec.ts-32-46 (1)
32-46: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winAdd coverage for idempotency and store-failure isolation.
UsageRecorder.finalizeUsageEventguarantees two behaviors that this suite does not exercise:
- A repeated call with the same
requestKeyandstatusmust not append a second event. The PR objective states recording is single-point and idempotent, so this is the load-bearing guarantee.- A rejecting
sink.appendmust not reject the caller. The objective states storage failures are handled gracefully.A third uncovered path:
notifyChangedmust fire only whenappendresolvestrue.Both fakes are already available through the injected sink, so the tests are cheap.
🧪 Proposed additional tests
it("returns true after a request has been finalized", async () => { const sink = { append: vi.fn().mockResolvedValue(true) } const recorder = new UsageRecorder(sink) expect(recorder._hasFinalized("task-001:0:1", "completed")).toBe(false) await recorder.finalizeUsageEvent("task-001:0:1", "completed", makeContext()) expect(recorder._hasFinalized("task-001:0:1", "completed")).toBe(true) expect(sink.append).toHaveBeenCalledTimes(1) }) + }) + + describe("finalizeUsageEvent", () => { + it("appends only once for a repeated requestKey and status", async () => { + const sink = { append: vi.fn().mockResolvedValue(true) } + const notifyChanged = vi.fn() + const recorder = new UsageRecorder(sink, notifyChanged) + + await recorder.finalizeUsageEvent("task-001:0:1", "completed", makeContext()) + await recorder.finalizeUsageEvent("task-001:0:1", "completed", makeContext()) + + expect(sink.append).toHaveBeenCalledTimes(1) + expect(notifyChanged).toHaveBeenCalledTimes(1) + }) + + it("appends separately for a different status on the same requestKey", async () => { + const sink = { append: vi.fn().mockResolvedValue(true) } + const recorder = new UsageRecorder(sink) + + await recorder.finalizeUsageEvent("task-001:0:1", "completed", makeContext()) + await recorder.finalizeUsageEvent("task-001:0:1", "failed", makeContext()) + + expect(sink.append).toHaveBeenCalledTimes(2) + }) + + it("does not reject the caller when the sink throws", async () => { + const sink = { append: vi.fn().mockRejectedValue(new Error("store down")) } + const notifyChanged = vi.fn() + const recorder = new UsageRecorder(sink, notifyChanged) + + await expect( + recorder.finalizeUsageEvent("task-001:0:1", "completed", makeContext()), + ).resolves.toBeUndefined() + expect(notifyChanged).not.toHaveBeenCalled() + }) + + it("does not notify when the sink deduplicates the event", async () => { + const sink = { append: vi.fn().mockResolvedValue(false) } + const notifyChanged = vi.fn() + const recorder = new UsageRecorder(sink, notifyChanged) + + await recorder.finalizeUsageEvent("task-001:0:1", "completed", makeContext()) + + expect(notifyChanged).not.toHaveBeenCalled() + }) })🤖 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/services/stats/__tests__/UsageRecorder.spec.ts` around lines 32 - 46, Extend the UsageRecorder tests around finalizeUsageEvent to verify idempotency by calling it twice with the same request key and status and asserting sink.append runs once. Add coverage for a rejecting sink.append to confirm finalizeUsageEvent resolves without propagating the failure, and verify notifyChanged is called only when append resolves true, using the injected sink’s existing fakes.src/services/stats/__tests__/UsageStatsService.spec.ts-75-88 (1)
75-88: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winDispose the shared service in
afterEach.
beforeEachinitializesservice, which opens a SQLite handle and registers a file watcher.afterEachnever callsservice.dispose(). Every test in this suite leaks one watcher and one open database. The open handle can also makefs.rmfail on Windows, and the surroundingcatchhides that failure, so temp directories accumulate.♻️ Proposed fix
afterEach(async () => { + service.dispose() // Clean up temp directory (test isolation) try { await fs.rm(tempDir, { recursive: true, force: true }) } catch { // ignore cleanup errors } })🤖 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/services/stats/__tests__/UsageStatsService.spec.ts` around lines 75 - 88, Update the afterEach cleanup for UsageStatsService tests to call service.dispose() before removing tempDir, ensuring the SQLite handle and file watcher are released while preserving the existing forced directory cleanup.webview-ui/src/components/dashboard/__tests__/DashboardSummary.visual.tsx-57-82 (1)
57-82: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winThe visual test fails in CI.
The pipeline reports a failure for this test in
Webview Visual Regression. The likely cause is a missing or stale committed baseline fordashboard-summary-dark.png.Generate the baseline in the container and commit it:
pnpm --filter `@roo-code/vscode-webview` test:visual:docker:updateDo not commit host-rendered baselines. As per coding guidelines: "Run visual comparisons and create or update committed baselines using pnpm test:visual:docker and pnpm test:visual:docker:update; do not commit host-rendered baselines."
🤖 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 `@webview-ui/src/components/dashboard/__tests__/DashboardSummary.visual.tsx` around lines 57 - 82, Update the committed visual baseline for the DashboardSummary test by running pnpm --filter `@roo-code/vscode-webview` test:visual:docker:update in the container. Commit the generated dashboard-summary-dark.png baseline, and do not use or commit host-rendered screenshots.Sources: Coding guidelines, Pipeline failures
webview-ui/src/components/dashboard/__tests__/DashboardView.visual.tsx-51-51 (1)
51-51: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winMake the visual test clock deterministic.
Date.now()feeds the coverage timestamps, whichDashboardViewrenders withtoLocaleString().TaskListalso callsDate.now()for relative timestamps. Freeze the browser clock before mounting and derive all fixture timestamps from the same fixed epoch.🤖 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 `@webview-ui/src/components/dashboard/__tests__/DashboardView.visual.tsx` at line 51, Update the visual test setup around DashboardView mounting to freeze the browser clock at a fixed epoch before rendering. Replace the standalone Date.now() fixture value with that shared epoch, and derive all coverage and TaskList timestamp fixtures from it so toLocaleString() and relative timestamps remain deterministic.Source: Coding guidelines
src/services/stats/costRecalculation.ts-227-247 (1)
227-247: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winDocument the
as ModelInfocasts.Both fallback branches build an object with only four price fields and cast it to
ModelInfo.ModelInfodeclares additional required fields. The cast is safe only becausecalculateApiCostAnthropicandcalculateApiCostOpenAIread price fields alone. That contract is not stated anywhere near the cast.Add a short comment at each cast that records this assumption, or type the return as
Pick<ModelInfo, "inputPrice" | "outputPrice" | "cacheWritesPrice" | "cacheReadsPrice">and widen at the call sites.As per coding guidelines: "If an unavoidable cast is required, document why in a nearby comment."
🤖 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/services/stats/costRecalculation.ts` around lines 227 - 247, Document both as ModelInfo casts in the modelPricing and customPricing fallback branches, stating that calculateApiCostAnthropic and calculateApiCostOpenAI access only the four price fields provided by these objects. Keep the existing fallback behavior unchanged.Source: Coding guidelines
src/services/stats/costRecalculation.ts-213-222 (1)
213-222: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winCache the sorted registry keys instead of sorting on every lookup.
lookupModelInfocallsObject.keys(registry)and sorts the result on each invocation. The sort is O(n log n) over every model id in the provider registry. This function runs once per usage event throughcomputeEventCost,getEffectiveCost, andcomputeCacheDiscountBase, andproviderReportsCacheat lines 358-365 repeats the same sort. During a rollup rebuild over full history, or a 100-event drain batch, the repeated sort dominates the lookup cost.Precompute the sorted key list per provider once at module load.
⚡ Proposed fix
+const SORTED_REGISTRY_IDS: Record<string, string[]> = Object.fromEntries( + Object.entries(PROVIDER_MODEL_REGISTRIES).map(([provider, registry]) => [ + provider, + Object.keys(registry).sort((a, b) => b.length - a.length), + ]), +) + +/** Resolves a model id against a provider registry: exact match, then longest substring match. */ +function resolveRegistryModel(provider: string, model: string): ModelInfo | undefined { + const registry = PROVIDER_MODEL_REGISTRIES[provider] + if (!registry) return undefined + if (model in registry) return registry[model] + const lowerModel = model.toLowerCase() + for (const knownId of SORTED_REGISTRY_IDS[provider] ?? []) { + if (lowerModel.includes(knownId.toLowerCase())) return registry[knownId] + } + return undefined +}Then use
resolveRegistryModelin bothlookupModelInfo(lines 209-222) andproviderReportsCache(lines 350-366). This also removes the duplicated matching logic between the two functions.🤖 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/services/stats/costRecalculation.ts` around lines 213 - 222, Precompute each provider registry’s keys sorted by descending length at module load, exposing or reusing a shared resolveRegistryModel helper. Update lookupModelInfo and providerReportsCache to call resolveRegistryModel instead of rebuilding and sorting Object.keys(registry) or duplicating substring matching, while preserving case-insensitive longest-ID matching and existing fallback behavior.src/services/stats/statsQueryRange.ts-35-38 (1)
35-38: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winWebview-supplied query values reach the host without runtime validation.
packages/types/src/usage-stats.tsline 217 states that Zod validation is required for every webview-originated query. TheWebviewMessagefields that carry those values are declared as plain TypeScript types, so the compiler enforces nothing at the message boundary and the host consumes the raw values. Parse each webview-originated query payload through its Zod schema inusageStatsMessageHandlerbefore use.
src/services/stats/statsQueryRange.ts#L35-L38: reject or drop an unparseablequery.from/query.toinstead of producing aNaNbound, which makesisStatsQueryRangeBoundedreporttruewhileisWithinStatsQueryRangeadmits every timestamp.packages/types/src/vscode-extension-host.ts#L851-L858: clampdashboardSessionLimitanddashboardTaskLimitto the documented 1–100 range, or parse the page request through the existingDashboardSessionPageRequestschema, which already encodes those bounds.🤖 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/services/stats/statsQueryRange.ts` around lines 35 - 38, In usageStatsMessageHandler, validate every webview-originated query with its existing Zod schema before use. At src/services/stats/statsQueryRange.ts lines 35-38, reject or omit unparseable query.from and query.to values so no NaN bounds are produced; at packages/types/src/vscode-extension-host.ts lines 851-858, parse the page request through DashboardSessionPageRequest or clamp dashboardSessionLimit and dashboardTaskLimit to 1–100.src/services/stats/costRecalculation.ts-313-318 (1)
313-318: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
getEffectiveCostnever recomputes a stored cost of 0.
getEffectiveCostreturns the stored value whenevercostUsdis defined, including{ value: 0 }.computeEventCostat line 274 uses a stricter guard and only short-circuits whencostUsd.value > 0, which shows the intent to recompute zero-valued costs.Because
getEffectiveCostis documented at line 307 as the primary entry point, the> 0branch incomputeEventCostis unreachable through it. An event recorded with an explicitcostUsd: 0by a provider that did not price the call keeps a cost of 0 forever, which is the exact failure the module header at lines 5-7 describes.Align the two guards.
🐛 Proposed fix
export function getEffectiveCost(event: UsageEventV1, customPricing?: CustomModelPricingMap): number { - if (event.usage.costUsd !== undefined) { + if (event.usage.costUsd !== undefined && event.usage.costUsd.value > 0) { return event.usage.costUsd.value } return computeEventCost(event, customPricing) }If a stored 0 must be treated as authoritative, remove the
> 0condition fromcomputeEventCostinstead and update its doc comment. Confirm which semantic the aggregation tests expect before choosing.🤖 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/services/stats/costRecalculation.ts` around lines 313 - 318, Align the cost guards used by getEffectiveCost and computeEventCost so stored zero-valued costs follow the module’s intended recomputation behavior. Update getEffectiveCost to avoid short-circuiting on costUsd.value === 0, or instead make computeEventCost treat zero as authoritative and revise its documentation; use the aggregation tests to confirm the expected semantic.src/services/stats/UsageStatsStreamCoordinator.ts-271-284 (1)
271-284: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winClamp the caller-supplied
lastSequenceinresume.
resumeacceptslastSequencefrom the webview and assigns it directly tostate.lastSequenceat line 281 when the gap is small. The gap check at line 275 only rejects values that are too far behind.If
lastSequenceexceeds the database's current last sequence,gapis negative, the check passes, andstate.lastSequencemoves ahead of the store. The drain filter at line 452 (e.sequence > sub.lastSequence) then matches nothing, and the subscriber receives no further deltas until an unrelated snapshot path fires.🐛 Proposed fix
const gap = currentLastSeq - lastSequence - if (gap > MAX_BATCH_EVENTS) { + if (gap > MAX_BATCH_EVENTS || gap < 0) { // Gap too large, or the cursor is ahead of the store — send full snapshot state.lastSequence = currentLastSeq this.sendSnapshot(state) } else {🤖 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/services/stats/UsageStatsStreamCoordinator.ts` around lines 271 - 284, Clamp the caller-supplied lastSequence to currentLastSeq in the resume flow before assigning state.lastSequence or evaluating the gap, so values ahead of the store cannot advance the subscription cursor. Preserve the existing full-snapshot behavior for oversized gaps and drain scheduling for valid small gaps.src/services/stats/UsageStatsStreamCoordinator.ts-566-607 (1)
566-607: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winExtract the duplicated snapshot-assembly block.
Lines 566-607 in
sendSnapshotand lines 690-731 inscheduleAsyncRebuildcontain the same logic, character for character: the task/session branch selection, thecomputeTaskPagecall and its seven arguments, thestate.visibleTaskIdsassignment, and the returned snapshot object.Both copies must stay in sync.
state.visibleTaskIdsfeeds the delta filter at line 487. If one copy changes and the other does not, the two paths produce different visible task sets and the delta stream diverges from the snapshot.♻️ Proposed extraction
+ /** + * Assembles the snapshot payload for a subscriber and records the visible + * task ids used by the delta filter. Shared by the immediate and the + * post-rebuild snapshot paths so both stay in sync. + */ + private assembleSnapshotFor( + state: SubscriptionState, + stats: StatsSnapshot, + heatmap: HeatmapSnapshot, + generation: number, + sequence: number, + customPricing: CustomModelPricingMap | undefined, + ): DashboardStatsSnapshot | DashboardTaskStatsSnapshot { + const base = { requestId: state.subscription.requestId, generation, sequence, stats, heatmap } + + if (!this.taskCatalog || !this.database) { + const sessions = computeSessionPage( + this.database!, + state.subscription.requestId, + undefined, + state.subscription.sessionPageSize, + ) + return { ...base, sessions, cursor: sessions.cursor } + } + + const tasks = computeTaskPage( + this.taskCatalog, + this.database, + state.subscription.requestId, + undefined, + state.subscription.sessionPageSize, + resolveStatsQueryRangeMs(state.subscription.range), + state.subscription.range.cacheRatio, + customPricing, + ) + state.visibleTaskIds = new Set([...tasks.tasks, ...(tasks.childTasks ?? [])].map((task) => task.taskId)) + return { ...base, tasks, cursor: tasks.cursor } + }Then call
this.assembleSnapshotFor(state, stats, heatmap, generation, sequence, customPricing)from both sites. ImportStatsSnapshotandHeatmapSnapshotas types.🤖 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/services/stats/UsageStatsStreamCoordinator.ts` around lines 566 - 607, Extract the duplicated task/session snapshot construction from sendSnapshot and scheduleAsyncRebuild into a shared assembleSnapshotFor method on UsageStatsStreamCoordinator, accepting state, stats, heatmap, generation, sequence, and customPricing. Preserve the computeTaskPage arguments, visibleTaskIds assignment, and returned snapshot fields exactly, then call the helper from both paths and import StatsSnapshot and HeatmapSnapshot as types.src/services/stats/UsageStatsStreamCoordinator.ts-658-670 (1)
658-670: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftProcess rollup rebuilding in bounded asynchronous chunks.
setImmediateonly defers the call;rebuildRollupsFromEvents()synchronously scans all events and updates multiple derived tables in one transaction. Its runtime grows with event history and can freeze the extension host during dashboard startup. Process batches across event-loop turns or use a worker.🤖 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/services/stats/UsageStatsStreamCoordinator.ts` around lines 658 - 670, Update scheduleAsyncRebuild and rebuildRollupsFromEvents so rollup rebuilding processes bounded event batches across multiple event-loop turns instead of one synchronous full-history transaction. Preserve rebuildInFlight, disposed/database checks, completion state, and error handling while ensuring each batch yields before the next.src/services/stats/UsageStatsStreamCoordinator.ts-794-806 (1)
794-806: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve recovery when the dashboard is hidden.
DashboardViewdoes not passvisible, so the hook defaults totrue. The host sendsdidBecomeVisibleonly when the view reappears. Hidden deltas are skipped, butdrainstill advancessub.lastSequence; the hook then sends no resume message and remains stale.Setting
snapshotSent = falsealone does not recover the subscriber becausedrainexcludes it andresumeonly schedules another excluded drain for small gaps. Wire actual visibility into the hook, pass the acknowledged sequence inresumeDashboardStats, and send a snapshot when visibility causes delta loss.🤖 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/services/stats/UsageStatsStreamCoordinator.ts` around lines 794 - 806, Update the DashboardView subscription flow and postMessage/drain recovery logic so actual visibility is propagated instead of defaulting to true. Ensure resumeDashboardStats carries the acknowledged sequence, and when visibility resumes after skipped deltas, send a snapshot that restores the subscriber rather than relying on another excluded drain.src/services/stats/UsageStatsService.ts-388-394 (1)
388-394: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winExport silently omits events when the SQLite migration is incomplete.
readEventsForQueryprefers the database whenever_isInitialized()is true.doInitializecatches migration failures at Lines 215-217 and only logs them. After a partial migration the database holds a subset of the events, but exports,getFilteredEvents, and the session grouping still read from it. The NDJSON segments, which remain the complete record, are never consulted.Gate the database path on migration completion, for example by checking
UsageStatsMigration.isComplete(), and fall back tothis.store.readAll()otherwise.🤖 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/services/stats/UsageStatsService.ts` around lines 388 - 394, Update readEventsForQuery to use the database only when UsageStatsMigration.isComplete() confirms migration completion in addition to _isInitialized(); otherwise fall back to this.store.readAll() so incomplete migrations never omit events.src/services/stats/UsageStatsService.ts-150-166 (1)
150-166: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winThe pricing refresh timer runs every 10 seconds for the entire extension lifetime.
buildCustomPricingMapFromAllProfiles(providerSettingsManager)enumerates every provider profile. The interval invokes it every 10 seconds from service construction untildispose(), whether or not the dashboard is open and whether or not any profile changed. Provider profiles hold API keys, so each pass also touches secret storage.Two lower-cost options exist:
- Refresh on demand: cache the map with a timestamp and rebuild it inside
customPricingProvideronly when the cache is older than a threshold.- Refresh on change: subscribe to a provider-settings change event, if
ProviderSettingsManagerLikeexposes one.The current callback also does not guard against overlap. If one refresh exceeds 10 seconds, a second starts before the first finishes.
🤖 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/services/stats/UsageStatsService.ts` around lines 150 - 166, Replace the lifetime setInterval refresh in the providerSettingsManager branch of UsageStatsService with on-demand, timestamp-based cache refresh inside customPricingProvider, using an appropriate freshness threshold and ensuring concurrent calls share or skip an in-flight refresh. Preserve the existing cached pricing on refresh errors and the customPricingProvider fallback path for externally supplied providers; remove the periodic pricingRefreshTimer setup and update disposal accordingly.src/services/stats/UsageStatsMigration.ts-101-105 (1)
101-105: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftThe migration reads every segment twice, synchronously, during activation.
buildParentMap()reads and parses all segment files, then the loop at Line 125 reads and parses the same files again. Both passes usefs.readFileSyncandJSON.parseon the extension host thread.UsageStatsService.doInitialize()callsmigrate()inline, so activation blocks for the full duration.
UsageEventStorecaps total segment size at 100 MiB, so the worst case is roughly 200 MiB of synchronous reads plus two full JSON parse passes.The comment at Line 312 states that the parent map is built "in a streaming fashion to avoid loading all events into memory", but
readFileSyncloads each whole segment.Two options reduce the cost:
- Build the parent map during the single migration pass and resolve
rootTaskIdin a second, database-only step.- Move
migrate()off the activation path, for example behind an idle callback or a first dashboard open.Also applies to: 314-331
🤖 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/services/stats/UsageStatsMigration.ts` around lines 101 - 105, The migration currently performs two synchronous full reads and JSON parses of every segment during activation. Update UsageStatsMigration.migrate and buildParentMap to avoid the duplicate segment pass, preferably collecting parent mappings during the existing migration read and resolving rootTaskId in a database-only follow-up step; ensure migration is no longer blocking activation if the existing initialization flow permits deferring it.src/services/stats/UsageStatsProjection.ts-594-604 (1)
594-604: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDay-axis buckets will not sum to the totals when custom pricing is active.
Lines 552-582 recompute
cacheDiscountBasefromusage_eventsand override the totals cost. Themodel/provider/modeaxes get the same treatment at Lines 618-633. Thedayaxis does not: it keeps the stored value, which the comment states is 0 for custom models.A dashboard that renders a per-day chart next to the totals card therefore shows two different cost figures for the same query. The gap widens with the number of custom-priced events.
Consider deriving the day-axis discount base with a per-day, per-(provider, model) query, or excluding the discount from the totals as well so both figures use the same basis until the query exists.
🤖 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/services/stats/UsageStatsProjection.ts` around lines 594 - 604, Align the day-axis bucket costs with the totals by applying the same custom-pricing cache discount-base recomputation used in the totals and model/provider/mode axes. Update the day-axis flow around dailyRowToBucket and queryDailyRollupsDetailed to derive the value per day and provider/model, or consistently exclude this discount from all corresponding totals until that query is available; do not leave day buckets using the stored zero value for custom models.src/services/stats/UsageAggregator.ts-185-220 (1)
185-220: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
resolveTimeRangeruns per event and builds uncachedIntl.DateTimeFormatobjects.
computeEventContribution()callsresolveTimeRange(query)at Line 527 for every event, andUsageStatsProjection.applyEventToProjection()calls it per event as well. Each preset call chain constructs fourIntl.DateTimeFormatinstances: two intoTimezoneDate, one instartOfDayInTimezone, and one ingetTimezoneOffsetMinutes. This file already memoizes formatters at Line 226 for bucket computation, so the same treatment is missing here.Two changes fix the hot path:
- Route
startOfDayInTimezoneandgetTimezoneOffsetMinutesthrough the memoized formatter cache.- Resolve the range once per query and pass it into
computeEventContribution, instead of resolving it inside the per-event function.A second, separate problem exists in the preset branches.
startOfDayInTimezonereturns the UTC instant of midnight inquery.timezone, but Lines 194, 199, 201, 206, and 208 then usesetDate/getDate, which operate in the host's local timezone. When the host local timezone crosses a DST boundary inside the computed span, the range shifts by one hour. Use UTC arithmetic (setUTCDate/getUTCDate) or add explicit day offsets in milliseconds.🤖 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/services/stats/UsageAggregator.ts` around lines 185 - 220, The resolveTimeRange hot path needs memoized timezone formatters and consistent UTC date arithmetic. Route startOfDayInTimezone and getTimezoneOffsetMinutes through the existing formatter cache, resolve the range once per query before per-event processing, and pass it into computeEventContribution and UsageStatsProjection.applyEventToProjection rather than resolving per event. In resolveTimeRange, replace host-local setDate/getDate operations in the today, 7d, and 30d preset branches with UTC-based arithmetic so DST in the host timezone cannot shift the range.src/services/stats/DashboardTaskProjection.ts-193-193 (1)
193-193: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
getTotalTokensdisagrees with the aggregator's token definition.
getTotalTokensprefers the storedevent.usage.totalTokens.valueand only falls back toinput + output.UsageAggregator.computeEventDeltatakes the opposite approach: Lines 462-465 ofsrc/services/stats/UsageAggregator.tsstate thattotalTokensis recomputed frominput + outputspecifically "to repair historical events that may have been persisted with the old double-counted sum".For any event written by the earlier recorder, the task detail total at Line 193 shows the double-counted value while the dashboard summary shows the repaired value. The two cards then disagree for the same task.
Align this helper with the aggregator.
🐛 Proposed fix
function getTotalTokens(event: UsageEventV1): number { - return ( - event.usage.totalTokens?.value ?? (event.usage.inputTokens?.value ?? 0) + (event.usage.outputTokens?.value ?? 0) - ) + // Match UsageAggregator.computeEventDelta: totalTokens is always + // input + output. The stored value may carry the old double-counted sum. + return (event.usage.inputTokens?.value ?? 0) + (event.usage.outputTokens?.value ?? 0) }Also applies to: 356-360
🤖 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/services/stats/DashboardTaskProjection.ts` at line 193, Update getTotalTokens, used by the totalTokens reduce in DashboardTaskProjection and the related path at lines 356-360, to always compute totals from inputTokens plus outputTokens rather than preferring event.usage.totalTokens.value. Match UsageAggregator.computeEventDelta so historical double-counted totals are repaired consistently.src/services/stats/UsageStatsService.ts-232-236 (1)
232-236: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
ensureInitialized()does not initialize.The method awaits
initPromiseonly when that promise already exists. If a caller invokesensureInitialized()beforeinitialize()runs, the method resolves immediately and the caller proceeds against a service whose database, store, and coordinator are not ready. The name states the opposite guarantee.
activateDashboardstartsinitialize()without awaiting it (seesrc/activate/activateDashboard.ts), so this window is reachable during activation.🐛 Proposed fix
async ensureInitialized(): Promise<void> { - if (this.initPromise) { - await this.initPromise - } + await this.initialize() }🤖 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/services/stats/UsageStatsService.ts` around lines 232 - 236, Update ensureInitialized() in UsageStatsService so it starts initialization when initPromise is absent, then awaits the resulting promise in all cases. Reuse the existing initialize() flow and initPromise state so callers always proceed only after the database, store, and coordinator are ready.src/services/stats/UsageEventStore.ts-247-258 (1)
247-258: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winRefresh the cached segment stat after an incremental push.
The incremental update pushes the event into
cachedEventsbut leavescachedActiveSegmentSizeandcachedActiveSegmentMtimeMsat their pre-append values. The nextreadAll()compares those stale values against the new on-disk size and mtime, so the warm-hit check at Lines 295-301 always fails. The result is a fullscanAllSegments()on every read that follows an append, which removes the benefit of the incremental push.Stat the active segment after the write and store the new values.
♻️ Proposed fix
const manifest = await this.loadOrCreateManifest() if (this.cachedSegmentCount !== manifest.currentSegment) { this.invalidateCache() } else { this.cachedEvents.push(event) + const activeStat = await fs + .stat(this.getSegmentPath(manifest.currentSegment)) + .catch(() => null) + if (activeStat) { + this.cachedActiveSegmentSize = activeStat.size + this.cachedActiveSegmentMtimeMs = activeStat.mtimeMs + } else { + this.invalidateCache() + } }🤖 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/services/stats/UsageEventStore.ts` around lines 247 - 258, In the incremental cache-update branch of UsageEventStore, refresh cachedActiveSegmentSize and cachedActiveSegmentMtimeMs after pushing the event into cachedEvents by statting the active segment written by the append and storing its current size and modification time. Preserve the existing manifest mismatch invalidation path and only update these cached stats when the cache remains valid.src/services/stats/UsageEventStore.ts-549-564 (1)
549-564: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRe-read the manifest and re-check the idempotency key inside the lock.
Two problems exist on this path:
- The doc comment at Line 217 states that duplicates are checked "within the lock", but the check at Line 550 runs before
acquireManifestLock(). A second VS Code window can append the sameidempotencyKeyconcurrently.loadOrCreateManifest()returnsthis.manifestfrom memory once it is cached (Line 636). It never re-reads the file. After acquiring the cross-process lock, this window therefore uses a stalegenerationandcurrentSegment. If another window ranclear()(which bumps the generation and moves segments) or rotated a segment, this window appends into the wrong segment and keeps a stale idempotency set.Read the manifest from disk after the lock is acquired, then evaluate the dedupe set against the current generation.
🐛 Suggested direction
- // Idempotency check - if (this.idempotencyKeys.has(event.idempotencyKey)) { - return false - } - let releaseLock: () => Promise<void> = async () => {} try { releaseLock = await this.acquireManifestLock() } catch (err) { throw new StatsStoreError("STATS_STORE/append/002", "Failed to acquire manifest lock for append", err) } try { - const manifest = await this.loadOrCreateManifest() + // Discard the in-memory copy so the on-disk manifest wins after + // another window mutated generation/currentSegment. + this.manifest = null + const manifest = await this.loadOrCreateManifest() + if (this.idempotencyKeys.has(event.idempotencyKey)) { + return false + }🤖 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/services/stats/UsageEventStore.ts` around lines 549 - 564, Move the idempotency check in the append flow to after acquireManifestLock(), then reload the manifest from disk while holding the lock instead of using the cached result from loadOrCreateManifest(). Refresh the in-memory manifest and generation-specific idempotency state before checking event.idempotencyKey and selecting the segment via getSegmentPath, preserving the duplicate return behavior.
…ests Three dashboard visual tests (DashboardSummary, DashboardView, UsageHeatmap) failed under Playwright CT. Root causes fixed: - ExtensionStateContextProvider pulled @roo-code/types/Zod into the browser bundle; added a Zod-free ExtensionStateContext mock and exact-match aliases in playwright-ct.config.ts. - Playwright CT instantiated @/i18n/TranslationContext twice when translation helpers lived in the test file, so components read a different context and rendered empty labels; moved provider wiring into dedicated component-only fixtures. - StandardTooltip requires a TooltipProvider ancestor; added it to all three fixtures. - Corrected summary-card label assertions to match en/dashboard.json casing. Also includes UsageStatsStreamCoordinator snapshot-fallback fix (delta double-send / cursor pollution guard). Verified: playwright-ct --update-snapshots 11/11 pass; eslint clean.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
webview-ui/src/components/dashboard/__tests__/UsageHeatmap.visual.fixture.tsx (1)
39-46: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winInclude zero-intensity days in the visual fixture.
Line 45 always returns at least
500. The fixture does not render the level-0 cells that its comment claims to cover. Add deterministic zero values so the screenshot can detect inactive-cell color and style regressions.Proposed fix
const values: number[] = Array.from({ length: 60 }, (_, i) => { + if (i % 13 === 0) { + return 0 + } const wave = Math.sin(i / 4) * 0.5 + 0.5 const weekend = i % 7 === 0 || i % 7 === 6 ? 0.2 : 1 return Math.round(500 + 10_000 * wave * weekend) })As per coding guidelines: “Add a visual snapshot when a change visibly affects … empty states.”
🤖 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 `@webview-ui/src/components/dashboard/__tests__/UsageHeatmap.visual.fixture.tsx` around lines 39 - 46, Update the values generator in the UsageHeatmap visual fixture so some deterministic entries return 0, ensuring level-0 inactive days are rendered alongside the existing intensity levels. Preserve the 60-day oldest-first sequence and keep the generated values suitable for exercising levels 1–5.Source: Coding guidelines
webview-ui/src/components/dashboard/__tests__/DashboardSummary.visual.fixture.tsx (1)
36-39: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument or replace both double assertions.
Both fixtures use
null as unknown as ...fori18nwithout a nearby explanation. This bypasses TypeScript type checking. Use a typed test double if practical. If the assertion must remain, add a nearby comment that states why it is safe.
webview-ui/src/components/dashboard/__tests__/DashboardSummary.visual.fixture.tsx#L36-L39: document why the fixture can supply a nulli18nvalue.webview-ui/src/components/dashboard/__tests__/UsageHeatmap.visual.fixture.tsx#L34-L37: apply the same documentation or typed test-double pattern.As per coding guidelines: “Use double assertions only as a last resort and explain them with a comment.”
🤖 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 `@webview-ui/src/components/dashboard/__tests__/DashboardSummary.visual.fixture.tsx` around lines 36 - 39, Replace the null double assertions for i18n with a typed test double where practical; otherwise add a nearby comment explaining why null is safe. Apply this consistently in translationContextValue in webview-ui/src/components/dashboard/__tests__/DashboardSummary.visual.fixture.tsx (lines 36-39) and webview-ui/src/components/dashboard/__tests__/UsageHeatmap.visual.fixture.tsx (lines 34-37).Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@webview-ui/src/components/dashboard/__tests__/DashboardSummary.visual.fixture.tsx`:
- Around line 36-39: Replace the null double assertions for i18n with a typed
test double where practical; otherwise add a nearby comment explaining why null
is safe. Apply this consistently in translationContextValue in
webview-ui/src/components/dashboard/__tests__/DashboardSummary.visual.fixture.tsx
(lines 36-39) and
webview-ui/src/components/dashboard/__tests__/UsageHeatmap.visual.fixture.tsx
(lines 34-37).
In
`@webview-ui/src/components/dashboard/__tests__/UsageHeatmap.visual.fixture.tsx`:
- Around line 39-46: Update the values generator in the UsageHeatmap visual
fixture so some deterministic entries return 0, ensuring level-0 inactive days
are rendered alongside the existing intensity levels. Preserve the 60-day
oldest-first sequence and keep the generated values suitable for exercising
levels 1–5.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b323954-c054-4ef3-b399-a3486bc7275d
⛔ Files ignored due to path filters (3)
webview-ui/src/components/dashboard/__tests__/__screenshots__/dashboard-summary-dark.pngis excluded by!**/*.pngwebview-ui/src/components/dashboard/__tests__/__screenshots__/dashboard-view-dark.pngis excluded by!**/*.pngwebview-ui/src/components/dashboard/__tests__/__screenshots__/usage-heatmap-dark.pngis excluded by!**/*.png
📒 Files selected for processing (8)
src/services/stats/UsageStatsStreamCoordinator.tswebview-ui/playwright-ct.config.tswebview-ui/playwright/ExtensionStateContext.mock.tsxwebview-ui/src/components/dashboard/__tests__/DashboardSummary.visual.fixture.tsxwebview-ui/src/components/dashboard/__tests__/DashboardSummary.visual.tsxwebview-ui/src/components/dashboard/__tests__/DashboardView.visual.fixture.tsxwebview-ui/src/components/dashboard/__tests__/UsageHeatmap.visual.fixture.tsxwebview-ui/src/components/dashboard/__tests__/UsageHeatmap.visual.tsx
🚧 Files skipped from review as they are similar to previous changes (3)
- webview-ui/src/components/dashboard/tests/DashboardView.visual.fixture.tsx
- webview-ui/src/components/dashboard/tests/DashboardSummary.visual.tsx
- src/services/stats/UsageStatsStreamCoordinator.ts
github-actions
Bot
added
the
awaiting-review
1 participant
Closes #947
Summary
Adds a local usage statistics dashboard accessible from the sidebar. Tracks LLM API call token usage and costs per session, with daily heatmap, session list/detail views, and export.
Features
Dashboard (Sidebar)
Usage Recording
Task.tsterminal finalize — no streaming duplicatesglobalStorage/usage-stats/— no cloud sync, no telemetryCost Calculation
costUsdare recalculated at query timeInternationalization
Architecture
Testing
pnpm check-types— 11/11 packages passpnpm lint— 0 warningsvi.hoisted()pattern for Vitest 4 isolationNotes for Reviewers
ClineProvider.tschanges are minimal (dead code removal + dispose safety)Summary by CodeRabbit