-
Notifications
You must be signed in to change notification settings - Fork 2.3k
feat: Add token statistics display to API requests (fixes #7740) #7741
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reviewing my own code is like debugging in production - technically possible but morally questionable.
input: inputDisplay, | ||
output: outputDisplay, | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing unit tests for this new utility file. Consider adding tests to verify the formatting logic, especially edge cases like:
- Zero values
- Undefined values
- Large numbers (millions)
- Decimal precision for different ranges
This would help ensure the formatting behaves correctly across all scenarios.
fontSize: "12px", | ||
color: "var(--vscode-descriptionForeground)", | ||
}}> | ||
↑ {tokenStats.input} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the arrow direction intentional? Typically, input flows down (↓) into the system and output flows up (↑) from it. The current implementation shows ↑ for input and ↓ for output, which might be counterintuitive for users.
<div | ||
className="flex items-center gap-1 flex-wrap" | ||
style={{ opacity: hasTokenData ? 1 : 0 }}> | ||
<span |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding ARIA labels or semantic HTML attributes here to help screen reader users understand what these arrows and numbers represent. For example:
return "0" | ||
} | ||
|
||
if (count < 1000) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider extracting this magic number to a named constant for better maintainability:
// Add cache reads in parentheses if they exist | ||
if (cacheReads && cacheReads > 0) { | ||
const cacheDisplay = formatTokenCount(cacheReads) | ||
inputDisplay = `${inputDisplay} (${cacheDisplay} cache)` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The "k" suffix and "cache" label are hardcoded. Consider using the i18n system for these strings to maintain consistency with the rest of the application's internationalization approach.
tokensIn?: number, | ||
tokensOut?: number, | ||
cacheReads?: number, | ||
): { input: string; output: string } { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider defining a more specific return type interface for better type safety:
Hi @mikhailsal! I see the issue with the price breaking into two lines and overflowing the gray box in narrow windows. I'll fix this right away to ensure the token statistics display properly at all window sizes. Working on it now! |
✅ @mikhailsal The overflow issue has been fixed! Changes made:
The token statistics and price will now stay on a single line even in narrow windows, preventing the overflow issue you reported. The changes have been pushed to the PR and CI checks are currently running. All tests are passing locally. The fix ensures the UI remains clean and readable at all window sizes. |
Hi @mikhailsal! I see the issue with the "API Request" text getting cut off in narrow windows. Ill implement a solution to hide the "API Request" text when theres insufficient space, keeping only the essential token statistics visible. Working on this now! |
✅ @mikhailsal The "API Request" text truncation issue has been fixed! Changes implemented:
Technical details:
The UI will now gracefully handle narrow windows by hiding the "API Request" label while keeping the essential information (cost and token stats) visible. The changes have been pushed and CI checks are currently running. |
@roomote-agent I think it would be a good idea to add this to a tooltip that appears when the price is hovered |
Hi @daniel-lxs! I see your suggestion to add the token statistics to a tooltip that appears when hovering over the price. That sounds like a great UX improvement! Let me implement this change for you. |
- Wrapped cost badge with StandardTooltip component - Token statistics (input/output/cache) now shown in tooltip - Improved UX by reducing visual clutter while keeping info accessible - Added cursor:default to badge when hovering As suggested by @daniel-lxs in PR #7741
Hi @daniel-lxs! I've successfully implemented your suggestion. Changes Made:
Technical Details:
The UI is now cleaner with the token statistics accessible on hover, reducing visual clutter while keeping the information readily available. The CI checks are currently running to validate the changes. Thanks for the great suggestion! 🎉 |
content={ | ||
<div className="flex flex-col gap-1"> | ||
<div className="flex items-center gap-2"> | ||
<span>↑ Input:</span> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider internationalizing the token statistic labels (currently the hardcoded strings "Input:" and "Output:") using the translation function (t). This will help keep UI text consistent and localizable.
This comment was generated because it violated a code review rule: irule_C0ez7Rji6ANcGkkX.
@daniel-lxs For some requests, there is no price at all, so this information about tokens will not be visible in any way. Could we display a tooltip when hovering over the text "API Request"? |
@roomote-agent Please:
|
Hi @mikhailsal! I see your request. I will:
Working on these changes now! |
…uest without price - Added i18n translations for "Input" and "Output" in chat.json - Updated ChatRow.tsx to use translated strings in token stats tooltip - Added tooltip to "API Request" text when there are token stats but no price - Ensures token information is always accessible via tooltip hover
1666dd5
to
ef3fd5b
Compare
🚀 Preview deployed! Your changes have been deployed to Vercel: Preview URL: https://roo-code-website-kuxg7q5oz-roo-code.vercel.app This preview will be updated automatically when you push new commits to this PR. |
Generated with ❤️ by ellipsis.dev |
Hi @mikhailsal! I've successfully implemented both requested fixes: ✅ Changes Made
📋 Implementation Details
✅ CI StatusThe The changes are ready for review! |
This PR had to be abandoned due to an AI error (context loss), I've created new one and fixed/implemented everything. |
This PR adds detailed token statistics display to API requests in the UI, showing input tokens, output tokens, and cache reads alongside the existing cost display.
Changes
formatTokens.ts
utility file with helper functions to format token countsChatRow.tsx
to display token statistics with up/down arrows next to cost badgeScreenshots
The token statistics now appear next to the cost display:
Testing
Fixes #7740
Important
This PR adds token statistics display to the UI, updates telemetry settings, and removes unused code components.
formatTokens.ts
for formatting token counts.ChatRow.tsx
to display token statistics with input/output tokens and cache reads.TelemetryClient
inTelemetryClient.ts
to enable telemetry only when explicitly set to "enabled".CloudView.tsx
by removing manual URL entry for authentication.DismissibleUpsell
component and related tests.This description was created by
for ef3fd5b. You can customize this summary. It will automatically update as commits are pushed.