fix(ThinkingBudget): extract maxOutputTokensControl to resolve undefined reference in binary reasoning path#332
Conversation
…ned reference in binary reasoning path
proyectoauraorg
requested review from
JamesRobert20,
edelauna,
hannesrudolph,
navedmerchant and
taltas
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthrough
ChangesBinary Reasoning Tokens Control
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
webview-ui/src/components/settings/ThinkingBudget.tsxESLint skipped: missing config or dependency (missing-dependency). The ESLint configuration references a package that is not available in the sandbox. webview-ui/src/components/settings/__tests__/ThinkingBudget.spec.tsxESLint skipped: the ESLint configuration for this file references a package that is not available in the sandbox. 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 |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
webview-ui/src/components/settings/ThinkingBudget.tsx (1)
220-235: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winReuse the extracted
maxOutputTokensControlto eliminate duplication.The slider markup here is identical to the extracted
maxOutputTokensControldefined at lines 167-186. Reusing the extracted control ensures a single source of truth and simplifies future maintenance.♻️ Proposed refactor to reuse extracted control
- <div className="flex flex-col gap-1"> - <div className="font-medium">{t("settings:thinkingBudget.maxTokens")}</div> - <div className="flex items-center gap-1"> - <Slider - min={8192} - max={Math.max( - modelInfo.maxTokens || 8192, - customMaxOutputTokens, - DEFAULT_HYBRID_REASONING_MODEL_MAX_TOKENS, - )} - step={1024} - value={[customMaxOutputTokens]} - onValueChange={([value]) => setApiConfigurationField("modelMaxTokens", value)} - /> - <div className="w-12 text-sm text-center">{customMaxOutputTokens}</div> - </div> - </div> + {maxOutputTokensControl}🤖 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 `@webview-ui/src/components/settings/ThinkingBudget.tsx` around lines 220 - 235, The duplicated Slider block should be replaced with the already extracted maxOutputTokensControl to avoid duplication: locate the JSX where customMaxOutputTokens, modelInfo, DEFAULT_HYBRID_REASONING_MODEL_MAX_TOKENS, Slider and setApiConfigurationField are used (the current Slider block in ThinkingBudget.tsx) and swap that markup to render the pre-defined maxOutputTokensControl variable/component instead, ensuring the same props/state (customMaxOutputTokens and setApiConfigurationField) remain connected so behavior is unchanged.
🤖 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 `@webview-ui/src/components/settings/ThinkingBudget.tsx`:
- Around line 167-186: Add a data-testid to the maxOutputTokensControl slider
container (the JSX that renders Slider inside maxOutputTokensControl) so tests
can target it, ensure the Slider component props (min, max calculation using
modelInfo.maxTokens, customMaxOutputTokens,
DEFAULT_HYBRID_REASONING_MODEL_MAX_TOKENS, step, value and onValueChange) remain
unchanged, and then add Vitest React tests under webview-ui/src/**/__tests__
that render the binary reasoning branch and assert: the slider element (by the
data-testid) is present, changing its value triggers
setApiConfigurationField("modelMaxTokens", ...) with the new value, and the
computed max bound respects modelInfo.maxTokens vs customMaxOutputTokens vs
DEFAULT_HYBRID_REASONING_MODEL_MAX_TOKENS.
---
Outside diff comments:
In `@webview-ui/src/components/settings/ThinkingBudget.tsx`:
- Around line 220-235: The duplicated Slider block should be replaced with the
already extracted maxOutputTokensControl to avoid duplication: locate the JSX
where customMaxOutputTokens, modelInfo,
DEFAULT_HYBRID_REASONING_MODEL_MAX_TOKENS, Slider and setApiConfigurationField
are used (the current Slider block in ThinkingBudget.tsx) and swap that markup
to render the pre-defined maxOutputTokensControl variable/component instead,
ensuring the same props/state (customMaxOutputTokens and
setApiConfigurationField) remain connected so behavior is unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 16a179b8-3c4a-4a96-8192-5fce85e035f4
📒 Files selected for processing (1)
webview-ui/src/components/settings/ThinkingBudget.tsx
| // Max output tokens control used by both binary reasoning and budget reasoning paths. | ||
| const maxOutputTokensControl = ( | ||
| <div className="flex flex-col gap-1"> | ||
| <div className="font-medium">{t("settings:thinkingBudget.maxTokens")}</div> | ||
| <div className="flex items-center gap-1"> | ||
| <Slider | ||
| min={8192} | ||
| max={Math.max( | ||
| modelInfo.maxTokens || 8192, | ||
| customMaxOutputTokens, | ||
| DEFAULT_HYBRID_REASONING_MODEL_MAX_TOKENS, | ||
| )} | ||
| step={1024} | ||
| value={[customMaxOutputTokens]} | ||
| onValueChange={([value]) => setApiConfigurationField("modelMaxTokens", value)} | ||
| /> | ||
| <div className="w-12 text-sm text-center">{customMaxOutputTokens}</div> | ||
| </div> | ||
| </div> | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy lift
Add test coverage and data-testid for the new max tokens slider.
The extracted maxOutputTokensControl is now rendered in the binary reasoning branch (line 199), introducing new component rendering and prop wiring behavior. Consider adding a data-testid attribute to the slider container to facilitate testing, and adding Vitest test coverage under webview-ui/src/**/__tests__ to verify:
- Slider renders correctly for binary reasoning models
- Value changes call
setApiConfigurationField("modelMaxTokens", ...)correctly - Slider bounds respect model capabilities
As per coding guidelines, prefer local webview-ui tests for React/webview behavior such as component rendering, local state, hooks, form dirty-state, validation, or prop wiring.
📋 Example: Adding data-testid
const maxOutputTokensControl = (
- <div className="flex flex-col gap-1">
+ <div className="flex flex-col gap-1" data-testid="max-output-tokens">
<div className="font-medium">{t("settings:thinkingBudget.maxTokens")}</div>📝 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.
| // Max output tokens control used by both binary reasoning and budget reasoning paths. | |
| const maxOutputTokensControl = ( | |
| <div className="flex flex-col gap-1"> | |
| <div className="font-medium">{t("settings:thinkingBudget.maxTokens")}</div> | |
| <div className="flex items-center gap-1"> | |
| <Slider | |
| min={8192} | |
| max={Math.max( | |
| modelInfo.maxTokens || 8192, | |
| customMaxOutputTokens, | |
| DEFAULT_HYBRID_REASONING_MODEL_MAX_TOKENS, | |
| )} | |
| step={1024} | |
| value={[customMaxOutputTokens]} | |
| onValueChange={([value]) => setApiConfigurationField("modelMaxTokens", value)} | |
| /> | |
| <div className="w-12 text-sm text-center">{customMaxOutputTokens}</div> | |
| </div> | |
| </div> | |
| ) | |
| // Max output tokens control used by both binary reasoning and budget reasoning paths. | |
| const maxOutputTokensControl = ( | |
| <div className="flex flex-col gap-1" data-testid="max-output-tokens"> | |
| <div className="font-medium">{t("settings:thinkingBudget.maxTokens")}</div> | |
| <div className="flex items-center gap-1"> | |
| <Slider | |
| min={8192} | |
| max={Math.max( | |
| modelInfo.maxTokens || 8192, | |
| customMaxOutputTokens, | |
| DEFAULT_HYBRID_REASONING_MODEL_MAX_TOKENS, | |
| )} | |
| step={1024} | |
| value={[customMaxOutputTokens]} | |
| onValueChange={([value]) => setApiConfigurationField("modelMaxTokens", value)} | |
| /> | |
| <div className="w-12 text-sm text-center">{customMaxOutputTokens}</div> | |
| </div> | |
| </div> | |
| ) |
🤖 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 `@webview-ui/src/components/settings/ThinkingBudget.tsx` around lines 167 -
186, Add a data-testid to the maxOutputTokensControl slider container (the JSX
that renders Slider inside maxOutputTokensControl) so tests can target it,
ensure the Slider component props (min, max calculation using
modelInfo.maxTokens, customMaxOutputTokens,
DEFAULT_HYBRID_REASONING_MODEL_MAX_TOKENS, step, value and onValueChange) remain
unchanged, and then add Vitest React tests under webview-ui/src/**/__tests__
that render the binary reasoning branch and assert: the slider element (by the
data-testid) is present, changing its value triggers
setApiConfigurationField("modelMaxTokens", ...) with the new value, and the
computed max bound respects modelInfo.maxTokens vs customMaxOutputTokens vs
DEFAULT_HYBRID_REASONING_MODEL_MAX_TOKENS.
edelauna
left a comment
edelauna
left a comment
There was a problem hiding this comment.
Nice - thanks for adding this, would it be possible to write an e2e or integration test verifying the component works as intended?
Also could you include a screenshot
In the binary-reasoning path the max-output-tokens slider rendered even with reasoning off, but getModelMaxOutputTokens then ignores the value (returns the model default), so the control was interactive yet silently ignored. Guard it with enableReasoningEffort to match the budget path, and add tests covering both states. Addresses PR Zoo-Code-Org#332 review (edelauna).
|
Applied your suggestion — the max-output-tokens slider is now gated behind |
| return null | ||
| } | ||
|
|
||
| // Max output tokens control used by both binary reasoning and budget reasoning paths. |
There was a problem hiding this comment.
The comment says "used by both binary reasoning and budget reasoning paths", but the budget path (around line 222) still has its own copy of this same slider JSX. Could maxOutputTokensControl replace that block there too?
2 participants
Summary
Extracts the
maxOutputTokensControlJSX fragment as a named variable declared before both the binary reasoning and budget reasoning conditional branches inThinkingBudget.tsx.This fixes a scoping issue where the max output tokens slider was only defined inside the budget reasoning block, making it unavailable (and would crash at runtime) for models using
supportsReasoningBinary(binary reasoning toggle).Changes
maxOutputTokensControlas a standalone JSX variable at line 167, before the binary/budget conditional branches{maxOutputTokensControl}inside the binary reasoning<div>so the slider appears for both reasoning modesMotivation
This fix addresses a bug reported in PR #274 by @edelauna: models using binary reasoning mode (e.g., Zhipu GLM via
supportsReasoningBinary) could not configuremodelMaxTokensbecause the slider control was scoped only within the budget reasoning conditional path.Related
modelMaxTokensbugSummary by CodeRabbit