fix(viewer): correct Token Savings calculation to match CLI#721
fix(viewer): correct Token Savings calculation to match CLI#721strugglerrr wants to merge 1 commit into
Conversation
The Viewer was calculating Token Savings incorrectly: - Before: estInjected = sessions.length * tokenBudget (default 2000) - After: estInjected = min(totalObs, 50) * 38 This matches the calculation in cli.mjs and produces accurate savings estimates. The old formula vastly overestimated injected tokens, causing savings to show 0% even when significant savings existed. Fixes the discrepancy between 'agentmemory status' (correct) and Viewer dashboard (always showed 0%).
|
@strugglerrr is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
📝 WalkthroughWalkthroughThe PR updates the dashboard "Token Savings" estimation logic to compute injected tokens based on a capped observation count multiplied by a fixed rate, replacing the previous session-count-based estimate. This change affects the displayed savings percentage and cost string in the Token Savings stat card. ChangesToken Savings Estimation Update
Estimated code review effort🎯 1 (Trivial) | ⏱️ ~3 minutes Poem
🚥 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/viewer/index.html (1)
1357-1358: ⚡ Quick winExtract magic numbers to named constants for clarity.
The constants
80,50, and38in the token savings formula are not self-documenting. Extracting them to named constants would make the calculation more understandable and easier to maintain.♻️ Proposed refactor
+ var TOKENS_PER_OBS_FULL = 80; + var TOKENS_PER_OBS_INJECTED = 38; + var MAX_OBS_FOR_INJECTION = 50; var totalObs = d.sessions.reduce(function(a, s) { return a + (s.observationCount || 0); }, 0); - var tokenBudget = parseInt(new URLSearchParams(window.location.search).get('tokenBudget') || '2000', 10) || 2000; - var estFull = totalObs * 80; - var estInjected = Math.min(totalObs, 50) * 38; + var estFull = totalObs * TOKENS_PER_OBS_FULL; + var estInjected = Math.min(totalObs, MAX_OBS_FOR_INJECTION) * TOKENS_PER_OBS_INJECTED;🤖 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/viewer/index.html` around lines 1357 - 1358, Extract the magic numbers in the token savings calculation into named constants: replace the literal 80 with a descriptive constant like TOKENS_PER_OBS_FULL, 50 with MAX_INJECTED_OBS (or similar), and 38 with TOKENS_PER_OBS_INJECTED; declare these constants near the top of the script or above the block where estFull and estInjected are computed, then update the formulas to use these constants (estFull = totalObs * TOKENS_PER_OBS_FULL and estInjected = Math.min(totalObs, MAX_INJECTED_OBS) * TOKENS_PER_OBS_INJECTED) so the intent of estFull, estInjected, and totalObs is clear and maintainable.
🤖 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/viewer/index.html`:
- Line 1356: Remove the dead local variable tokenBudget: the parseInt(...)
assignment to tokenBudget is no longer used after the change that hardcodes 38
in the subsequent calculation, so delete the var tokenBudget = ... statement
(remove any reference to tokenBudget) to avoid confusing dead code; ensure no
other code in index.html references tokenBudget before committing.
---
Nitpick comments:
In `@src/viewer/index.html`:
- Around line 1357-1358: Extract the magic numbers in the token savings
calculation into named constants: replace the literal 80 with a descriptive
constant like TOKENS_PER_OBS_FULL, 50 with MAX_INJECTED_OBS (or similar), and 38
with TOKENS_PER_OBS_INJECTED; declare these constants near the top of the script
or above the block where estFull and estInjected are computed, then update the
formulas to use these constants (estFull = totalObs * TOKENS_PER_OBS_FULL and
estInjected = Math.min(totalObs, MAX_INJECTED_OBS) * TOKENS_PER_OBS_INJECTED) so
the intent of estFull, estInjected, and totalObs is clear and maintainable.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
| @@ -1355,7 +1355,7 @@ <h1>agentmemory</h1> | |||
| var totalObs = d.sessions.reduce(function(a, s) { return a + (s.observationCount || 0); }, 0); | |||
| var tokenBudget = parseInt(new URLSearchParams(window.location.search).get('tokenBudget') || '2000', 10) || 2000; | |||
There was a problem hiding this comment.
Remove unused tokenBudget variable.
The tokenBudget variable is no longer used after the formula change on line 1358. The new calculation uses the constant 38 instead of the query parameter value, making this line dead code that can confuse future maintainers.
🧹 Proposed fix
var totalObs = d.sessions.reduce(function(a, s) { return a + (s.observationCount || 0); }, 0);
- var tokenBudget = parseInt(new URLSearchParams(window.location.search).get('tokenBudget') || '2000', 10) || 2000;
var estFull = totalObs * 80;📝 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.
| var tokenBudget = parseInt(new URLSearchParams(window.location.search).get('tokenBudget') || '2000', 10) || 2000; | |
| var totalObs = d.sessions.reduce(function(a, s) { return a + (s.observationCount || 0); }, 0); | |
| var estFull = totalObs * 80; |
🤖 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/viewer/index.html` at line 1356, Remove the dead local variable
tokenBudget: the parseInt(...) assignment to tokenBudget is no longer used after
the change that hardcodes 38 in the subsequent calculation, so delete the var
tokenBudget = ... statement (remove any reference to tokenBudget) to avoid
confusing dead code; ensure no other code in index.html references tokenBudget
before committing.
1 participant
Problem
The Viewer dashboard shows Token Savings: 0% even when
agentmemory statusreports significant savings (e.g., 53%).Root Cause
The calculation formulas are inconsistent:
min(totalObs, 50) * 38sessions.length * tokenBudgetThe Viewer uses
sessions.length * 2000which vastly overestimates injected tokens, causing savings to be negative (clamped to 0%).Fix
Changed Viewer calculation to match CLI:
Verification
After fix:
totalObs = 46estFull = 46 * 80 = 3,680estInjected = min(46, 50) * 38 = 1,748savings = (1 - 1748/3680) * 100 = 52.5%Now matches
agentmemory statusoutput.Summary by CodeRabbit