Skip to content

Conversation

@mrubens
Copy link
Collaborator

@mrubens mrubens commented Aug 13, 2025

Reverts #6122

After using this for a bit, I agree with the previous comment that it's better UX to interrupt the request than to get the cost perfect.


Important

Reverts token usage reporting improvements to prioritize user experience by interrupting requests instead of perfecting cost reporting.

  • Behavior:
  • Code Changes:
    • Deletes DEFAULT_USAGE_COLLECTION_TIMEOUT_MS from global-settings.ts.
    • Removes drainStreamInBackgroundToFindAllUsage function and related logic in Task.ts.
    • Adjusts recursivelyMakeClineRequests in Task.ts to handle request interruptions without background usage collection.

This description was created by Ellipsis for 015b876. You can customize this summary. It will automatically update as commits are pushed.

@mrubens mrubens requested review from cte and jr as code owners August 13, 2025 05:26
@dosubot dosubot bot added the size:L This PR changes 100-499 lines, ignoring generated files. label Aug 13, 2025
Copy link
Contributor

@roomote roomote bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for this revert! I've reviewed the changes and this is a clean revert that properly prioritizes user experience over perfect cost reporting. The decision to favor immediate interruption makes sense given user feedback. I have a few suggestions for consideration below.

// PREV: We need to let the request finish for openrouter to
// get generation details.
// UPDATE: It's better UX to interrupt the request at the
// cost of the API cost not being retrieved.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider adding a comment here explaining the trade-off decision. Something like:

Suggested change
// cost of the API cost not being retrieved.
// PREV: We need to let the request finish for openrouter to
// get generation details.
// UPDATE: It's better UX to interrupt the request at the
// cost of the API cost not being retrieved.
// This prioritizes user experience (immediate interruption) over
// perfect cost tracking based on user feedback.

This would help future developers understand why this approach was chosen.

// anyways, so it remains solely for legacy purposes to keep track
// of prices in tasks from history (it's worth removing a few months
// from now).
const updateApiReqMsg = (cancelReason?: ClineApiReqCancelReason, streamingFailedMessage?: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of the lastApiReqIndex < 0 check could potentially cause issues if this function is called with an invalid index. Consider keeping this defensive check even though the background processing is removed:

Suggested change
const updateApiReqMsg = (cancelReason?: ClineApiReqCancelReason, streamingFailedMessage?: string) => {
const updateApiReqMsg = (cancelReason?: ClineApiReqCancelReason, streamingFailedMessage?: string) => {
if (lastApiReqIndex < 0 || !this.clineMessages[lastApiReqIndex]) {
return
}
const existingData = JSON.parse(this.clineMessages[lastApiReqIndex].text || "{}")

break
}

// PREV: We need to let the request finish for openrouter to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The comment here specifically mentions OpenRouter, but this behavior affects all providers that return usage data at the end of streams. Consider making it more generic:

Suggested change
// PREV: We need to let the request finish for openrouter to
// PREV: We need to let the request finish for providers to
// get complete usage/generation details.
// UPDATE: It's better UX to interrupt the request at the
// cost of the API cost not being retrieved.

@mrubens mrubens merged commit 6f81b77 into main Aug 13, 2025
14 of 15 checks passed
@mrubens mrubens deleted the revert-6122-christiaan/usage branch August 13, 2025 05:33
@github-project-automation github-project-automation bot moved this from Triage to Done in Roo Code Roadmap Aug 13, 2025
@github-project-automation github-project-automation bot moved this from New to Done in Roo Code Roadmap Aug 13, 2025
@hannesrudolph hannesrudolph added the Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. label Aug 13, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Issue/PR - Triage New issue. Needs quick review to confirm validity and assign labels. size:L This PR changes 100-499 lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

3 participants