Skip to content

Conversation

@cannuri
Copy link
Contributor

@cannuri cannuri commented May 8, 2025

Related GitHub Issue

Closes: #3009 and #3010

Description

This PR addresses the issue where the "Max Tokens" configuration slider was not visible for certain models that have a configurable maxTokens property but are not classified as "thinking" models.

The solution involved refactoring the webview settings UI to decouple the "Max Output Tokens" slider from the "Max Thinking Tokens" slider.

Key implementation details:

  • Created a new component, webview-ui/src/components/settings/MaxOutputTokensControl.tsx, to handle the display and logic for the "Max Output Tokens" slider. This component's visibility is now solely based on the presence and value of modelInfo.maxTokens.
  • Refactored the existing webview-ui/src/components/settings/ThinkingBudget.tsx component to only manage and display the "Max Thinking Tokens" slider. Its visibility remains tied to modelInfo.thinking === true and modelInfo.maxTokens.
  • Updated the webview-ui/src/components/settings/ApiOptions.tsx component to conditionally render both MaxOutputTokensControl and the refactored ThinkingBudget based on their respective visibility logic.
  • Adjusted the styling of the MaxOutputTokensControl component to match the layout of the ThinkingBudget slider, displaying the numerical value to the right.
  • Ensured the correct, existing localization key (providers.customModel.maxTokens.label) is used for the "Max Output Tokens" slider label.

The development followed a granular, iterative Test-Driven Development (TDD) approach, with small cycles of writing/modifying tests and then implementing the corresponding code, as detailed in the todo.md file.

Test Procedure

The changes were developed and verified using a granular TDD workflow:

  1. For each component (MaxOutputTokensControl, ThinkingBudget, ApiOptions), tests were written/modified first
  2. These tests were run to confirm they failed as expected (Red phase).
  3. Corresponding code was implemented/refactored (Tasks C1.C*, C2.C*, C3.C*) to make the tests pass (Green phase).
  4. This cycle was repeated for each granular task within the component's development.
  5. Upon completion of each component's development cycle, all project unit tests (excluding e2e) were run to check for regressions.
  6. All unit tests are currently passing.

Manual UI verification is required to confirm the visual behavior:

  • Test with a non-thinking model that has maxTokens (e.g., gemini-2.5-pro-exp-03-25 with GCP Vertex AI): "Max Output Tokens" slider should be visible, "Max Thinking Tokens" should not.
  • Test with a thinking model (e.g., claude-3-7-sonnet-20250219:thinking with Anthropic): Both "Max Output Tokens" and "Max Thinking Tokens" sliders should be visible.
  • Test with a model that has no maxTokens or maxTokens: 0: Neither slider related to these should be visible.
  • Verify default values and slider behavior in these scenarios.

Type of Change

  • 🐛 Bug Fix: Non-breaking change that fixes an issue.
  • New Feature: Non-breaking change that adds functionality.
  • 💥 Breaking Change: Fix or feature that would cause existing functionality to not work as expected.
  • ♻️ Refactor: Code change that neither fixes a bug nor adds a feature.
  • 💅 Style: Changes that do not affect the meaning of the code (white-space, formatting, etc.).
  • 📚 Documentation: Updates to documentation files.
  • ⚙️ Build/CI: Changes to the build process or CI configuration.
  • 🧹 Chore: Other changes that don't modify src or test files.

Pre-Submission Checklist

  • Issue Linked: This PR is linked to an approved GitHub Issue (see "Related GitHub Issue" above).
  • Scope: My changes are focused on the linked issue (one major feature/fix per PR).
  • Self-Review: I have performed a thorough self-review of my code.
  • Code Quality:
    • My code adheres to the project's style guidelines. (Addressed during development)
    • There are no new linting errors or warnings (npm run lint). (Addressed during development/testing)
    • All debug code (e.g., console.log) has been removed. (Addressed during development/testing)
  • Testing:
    • New and/or updated tests have been added to cover my changes. (Completed as per todo.md)
    • All tests pass locally (npm test). (Confirmed during development cycles)
    • The application builds successfully with my changes. (Confirmed after resolving build issues)
  • Branch Hygiene: My branch is up-to-date (rebased) with the main branch.
  • Documentation Impact: I have considered if my changes require documentation updates (see "Documentation Updates" section below).
  • Changeset: A changeset has been created using npm run changeset if this PR includes user-facing changes or dependency updates.
  • Contribution Guidelines: I have read and agree to the Contributor Guidelines.

Screenshots / Videos

Before:
Screenshot 2025-05-08 at 22 04 42

After:
Screenshot 2025-05-08 at 22 15 33

Documentation Updates

[ ] No documentation updates are required.
[ ] Yes, documentation updates are required. (Assess if user-facing documentation describing settings needs updating).
[x] I am not fully sure, but I guess no.


Important

Decouples Max Output Tokens and Max Thinking Tokens sliders in the UI, adding a new component for Max Output Tokens and refactoring existing logic, with comprehensive tests.

  • UI Components:
    • Added MaxOutputTokensControl component in MaxOutputTokensControl.tsx to manage "Max Output Tokens" slider visibility based on modelInfo.maxTokens.
    • Refactored ThinkingBudget.tsx to only manage "Max Thinking Tokens" slider, visible when modelInfo.thinking is true.
    • Updated ApiOptions.tsx to conditionally render MaxOutputTokensControl and ThinkingBudget based on model properties.
  • Testing:
    • Added MaxOutputTokensControl.test.tsx to test rendering and behavior of the new component.
    • Updated ApiOptions.test.tsx and ThinkingBudget.test.tsx to reflect changes in component logic and rendering.
  • Misc:
    • Mocked Slider component in __mocks__/Slider.tsx for testing purposes.

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

@changeset-bot
Copy link

changeset-bot bot commented May 8, 2025

⚠️ No Changeset found

Latest commit: 9be98c06b971d4a0481de520d994ad3c18e83ef1

Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset.

This PR includes no changesets

When changesets are added to this PR, you'll see the packages that this PR includes changesets for and the associated semver types

Click here to learn what changesets are, and how to add one.

Click here if you're a maintainer who wants to add a changeset to this PR

@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. bug Something isn't working labels May 8, 2025
@hannesrudolph hannesrudolph moved this from New to PR [Pre Approval Review] in Roo Code Roadmap May 10, 2025
bgilbert6 pushed a commit to bgilbert6/Roo-Code that referenced this pull request May 14, 2025
* fix excessive markdown format character escaping

* add changeset

* made it a little more robust

---------

Co-authored-by: Wesley Smith <[email protected]>
Co-authored-by: Cline Evaluation <[email protected]>
@hannesrudolph hannesrudolph moved this from New to PR [Pre Approval Review] in Roo Code Roadmap May 20, 2025
@hannesrudolph hannesrudolph moved this from PR [Needs Review] to TEMP in Roo Code Roadmap May 26, 2025
@daniel-lxs daniel-lxs moved this from TEMP to PR [Needs Review] in Roo Code Roadmap May 27, 2025
@daniel-lxs
Copy link
Member

Hey @cannuri,

Thank you for your contribution! Sorry that we took so long to review this PR.

I see that the issue still exists, non-thinking models with configurable maxTokens (like gemini-2.5-pro-exp-03-25) still don't show the max output tokens slider.

However, due to recent changes to how thinking models are handled in the codebase, this PR needs some updates:

  1. Model properties have changed: The PR currently uses modelInfo.thinking which no longer exists. The current implementation uses:

    • supportsReasoningBudget determines if a model supports reasoning/thinking features
    • requiredReasoningBudget determines if reasoning is mandatory for the model
    • supportsReasoningEffort determines if the model supports reasoning effort levels
  2. The fix needed:

    • MaxOutputTokensControl should show whenever modelInfo.maxTokens > 0, regardless of reasoning support
    • ThinkingBudget should only handle thinking-related controls based on supportsReasoningBudget

Would you be able to update the PR to work with the current model properties? I'd be happy to help if you need any clarification on the changes needed.

Thanks again for your patience and for working on this fix!

@daniel-lxs daniel-lxs moved this from PR [Needs Prelim Review] to PR [Draft / In Progress] in Roo Code Roadmap May 31, 2025
@cannuri
Copy link
Contributor Author

cannuri commented Jun 2, 2025

Hey @cannuri,

Thank you for your contribution! Sorry that we took so long to review this PR.

I see that the issue still exists, non-thinking models with configurable maxTokens (like gemini-2.5-pro-exp-03-25) still don't show the max output tokens slider.

However, due to recent changes to how thinking models are handled in the codebase, this PR needs some updates:

1. Model properties have changed: The PR currently uses `modelInfo.thinking` which no longer exists. The current implementation uses:
   
   * `supportsReasoningBudget`  determines if a model supports reasoning/thinking features
   * `requiredReasoningBudget`  determines if reasoning is mandatory for the model
   * `supportsReasoningEffort`  determines if the model supports reasoning effort levels

2. The fix needed:
   
   * `MaxOutputTokensControl` should show whenever `modelInfo.maxTokens > 0`, regardless of reasoning support
   * `ThinkingBudget` should only handle thinking-related controls based on `supportsReasoningBudget`

Would you be able to update the PR to work with the current model properties? I'd be happy to help if you need any clarification on the changes needed.

Thanks again for your patience and for working on this fix!

I am on it.

@cannuri cannuri force-pushed the fix_max_tokens_slider branch from f370101 to 1d3c2d2 Compare June 2, 2025 22:56
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Jun 2, 2025
- Replace modelInfo.thinking with supportsReasoningBudget properties
- Update all tests to use new model property structure
- Lower MIN_OUTPUT_TOKENS to 2048 for better usability
- Add standalone MaxOutputTokensControl component
@cannuri cannuri force-pushed the fix_max_tokens_slider branch from 1d3c2d2 to 7b4f93c Compare June 2, 2025 23:00
@daniel-lxs daniel-lxs marked this pull request as draft June 3, 2025 23:34
@hannesrudolph
Copy link
Collaborator

Stale. fixed already.

@github-project-automation github-project-automation bot moved this from PR [Draft / In Progress] to Done in Roo Code Roadmap Jul 7, 2025
@github-project-automation github-project-automation bot moved this from PR [Pre Approval Review] to Done in Roo Code Roadmap Jul 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working PR - Draft / In Progress size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

Max Tokens config does not appear for gemini-2.5-pro; it should

3 participants