fix(#80): unify cost computation — single source of truth#83
fix(#80): unify cost computation — single source of truth#83Psypeal wants to merge 1 commit intomatt1398:mainfrom
Conversation
sessionAnalyzer now reads pre-computed costUsd from calculateMetrics() instead of re-computing costs independently. Parent cost uses detail.metrics.costUsd, subagent cost uses proc.metrics.costUsd. This ensures the cost analysis panel and chat header always agree. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Summary of ChangesHello @Psypeal, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request refactors the cost computation logic within the session analyzer to establish a single source of truth for both parent and subagent costs. By directly utilizing pre-computed metric values, it eliminates inconsistencies and ensures that cost analysis panels and chat headers display identical, accurate figures. Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request effectively unifies the cost computation by establishing a single source of truth for parent and subagent costs, reading them from pre-computed metrics. This is a great improvement for consistency and maintainability, eliminating potential discrepancies. The changes in sessionAnalyzer.ts are clean and the removal of redundant calculations is well-executed. The accompanying tests are thorough, with new test cases that specifically validate the unified cost logic. I have one minor suggestion to improve the robustness of the tests when dealing with floating-point numbers.
| expect(report.costAnalysis.parentCostUsd).toBe(0.50); | ||
| expect(report.costAnalysis.subagentCostUsd).toBe(0.30); | ||
| expect(report.costAnalysis.totalSessionCostUsd).toBe(0.80); |
There was a problem hiding this comment.
While these assertions with toBe are correct for the given test data, it's a good practice to use toBeCloseTo for floating-point number comparisons in tests. This helps prevent brittle tests that might fail due to minor floating-point inaccuracies with different test values. This advice applies to other floating-point cost assertions in this file as well.
| expect(report.costAnalysis.parentCostUsd).toBe(0.50); | |
| expect(report.costAnalysis.subagentCostUsd).toBe(0.30); | |
| expect(report.costAnalysis.totalSessionCostUsd).toBe(0.80); | |
| expect(report.costAnalysis.parentCostUsd).toBeCloseTo(0.50); | |
| expect(report.costAnalysis.subagentCostUsd).toBeCloseTo(0.30); | |
| expect(report.costAnalysis.totalSessionCostUsd).toBeCloseTo(0.80); |
📝 WalkthroughWalkthroughRefactored cost computation in session analyzer to use Changes
Possibly related issues
Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 2✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/renderer/utils/sessionAnalyzer.ts (1)
1137-1142:⚠️ Potential issue | 🟡 Minor
costByModelstill uses a separate computation path fromparentCostUsd.
costByModelis built frommodelStats, which accumulates costs viacalculateMessageCost()in the single-pass loop (Line 403). MeanwhileparentCostUsdreads fromdetail.metrics.costUsd. These two paths can still diverge, so the sum ofcostByModelvalues may not reconcile withtotalSessionCostUsddisplayed in the header — a narrower variant of issue#80scoped to the model breakdown table.Consider documenting this known divergence (e.g. a code comment stating "
costByModelis an approximate per-model attribution and may not sum tototalSessionCostUsd") so future contributors don't treat it as a bug.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/renderer/utils/sessionAnalyzer.ts` around lines 1137 - 1142, The per-model breakdown costByModel is computed from modelStats (populated via calculateMessageCost in the single-pass loop) while parentCostUsd reads detail.metrics.costUsd, so these paths can diverge and not sum to totalSessionCostUsd; add a concise code comment near costByModel (and optionally near parentCostUsd or totalSessionCostUsd) stating that costByModel is an approximate per-model attribution and may not equal totalSessionCostUsd to prevent future confusion, referencing modelStats, calculateMessageCost, and parentCostUsd in the comment for clarity.
🧹 Nitpick comments (1)
test/renderer/utils/sessionAnalyzer.test.ts (1)
1510-1589: New suite covers the four key single-source scenarios well.The four tests correctly exercise: non-zero parent cost, undefined fallback, per-process subagent cost aggregation, and the combined total. One coverage gap worth considering: none of the new tests exercises a session with both real message usage data (feeding
calculateMessageCostintocostByModel) and adetail.metrics.costUsdthat differs from the per-message sum. Such a test would explicitly document the known divergence betweencostByModelandparentCostUsd, protecting against future changes that inadvertently try to unify them again.🧪 Suggested additional test
it('costByModel total may differ from parentCostUsd (documented divergence)', () => { const messages: ParsedMessage[] = [ createMockMessage({ type: 'assistant', model: 'claude-sonnet-4-20250514', // These tokens will feed calculateMessageCost() into costByModel usage: { input_tokens: 100000, output_tokens: 20000, cache_read_input_tokens: 0, cache_creation_input_tokens: 0, }, isSidechain: false, }), ]; // Set a deliberately different pre-computed cost const report = analyzeSession( createMockDetail({ messages, metrics: createMockMetrics({ costUsd: 0.01 }), }) ); // parentCostUsd and totalSessionCostUsd always come from detail.metrics expect(report.costAnalysis.parentCostUsd).toBe(0.01); expect(report.costAnalysis.totalSessionCostUsd).toBe(0.01); // costByModel is computed independently via calculateMessageCost; total may differ const modelTotal = Object.values(report.costAnalysis.costByModel).reduce( (sum, c) => sum + c, 0 ); // Intentional: these two are separate computation paths expect(modelTotal).not.toBeCloseTo(report.costAnalysis.totalSessionCostUsd, 2); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@test/renderer/utils/sessionAnalyzer.test.ts` around lines 1510 - 1589, Add a test that verifies costByModel (computed via calculateMessageCost from messages) can differ from the precomputed parent cost in detail.metrics.costUsd: createMockMessage with large usage values so calculateMessageCost produces a non-trivial cost, pass it into analyzeSession via createMockDetail along with createMockMetrics({ costUsd: 0.01 }) and assert that report.costAnalysis.parentCostUsd and report.costAnalysis.totalSessionCostUsd equal the provided detail.metrics.costUsd while the sum of Object.values(report.costAnalysis.costByModel) is not close to that parent/total value; reference analyzeSession, createMockDetail, createMockMessage, createMockMetrics, costByModel, and costAnalysis.parentCostUsd/totalSessionCostUsd to locate where to add the test.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@src/renderer/utils/sessionAnalyzer.ts`:
- Around line 1137-1142: The per-model breakdown costByModel is computed from
modelStats (populated via calculateMessageCost in the single-pass loop) while
parentCostUsd reads detail.metrics.costUsd, so these paths can diverge and not
sum to totalSessionCostUsd; add a concise code comment near costByModel (and
optionally near parentCostUsd or totalSessionCostUsd) stating that costByModel
is an approximate per-model attribution and may not equal totalSessionCostUsd to
prevent future confusion, referencing modelStats, calculateMessageCost, and
parentCostUsd in the comment for clarity.
---
Nitpick comments:
In `@test/renderer/utils/sessionAnalyzer.test.ts`:
- Around line 1510-1589: Add a test that verifies costByModel (computed via
calculateMessageCost from messages) can differ from the precomputed parent cost
in detail.metrics.costUsd: createMockMessage with large usage values so
calculateMessageCost produces a non-trivial cost, pass it into analyzeSession
via createMockDetail along with createMockMetrics({ costUsd: 0.01 }) and assert
that report.costAnalysis.parentCostUsd and
report.costAnalysis.totalSessionCostUsd equal the provided
detail.metrics.costUsd while the sum of
Object.values(report.costAnalysis.costByModel) is not close to that parent/total
value; reference analyzeSession, createMockDetail, createMockMessage,
createMockMetrics, costByModel, and
costAnalysis.parentCostUsd/totalSessionCostUsd to locate where to add the test.
|
Closing — the dual computation path this fixes no longer exists. PR #87 reverted sessionAnalyzer.ts entirely, making calculateMetrics() the sole cost source. |
2 participants
Summary
detail.metrics.costUsd(pre-computed bycalculateMetrics()) instead of re-accumulating per-message in the analysis loopproc.metrics.costUsdinstead of re-computing viacalculateMessageCost()proc.metrics.costUsdis not populated upstreamstats.costUsdaccumulation kept intact for the cost-by-model breakdown tableThis ensures the cost analysis panel and chat header always show identical values.
Test plan
pnpm typecheck— no type errorspnpm test— all 831 tests pass (4 new unified cost tests added)pnpm lint:fix— no lint errorsCloses #80
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests