Skip to content

fix(ThinkingBudget): extract maxOutputTokensControl to resolve undefined reference in binary reasoning path #332

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.

Sign up for GitHub

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

Open
proyectoauraorg wants to merge 2 commits into
base: main
Choose a base branch
from
+64 −0
Open

fix(ThinkingBudget): extract maxOutputTokensControl to resolve undefined reference in binary reasoning path #332

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0801635
fix(ThinkingBudget): extract maxOutputTokensControl to resolve undefi…
proyectoauraorg May 26, 2026
21ece1c
fix(ThinkingBudget): gate max-output slider behind reasoning toggle
proyectoauraorg May 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 25 additions & 0 deletions webview-ui/src/components/settings/ThinkingBudget.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,6 +164,27 @@ export const ThinkingBudget = ({ apiConfiguration, setApiConfigurationField, mod
return null
}

// Max output tokens control used by both binary reasoning and budget reasoning paths.
Copy link
Copy Markdown
Contributor

@edelauna edelauna May 27, 2026

Choose a reason for hiding this comment

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

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?

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>
)
Comment on lines +167 to +186
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai Bot May 26, 2026

Choose a reason for hiding this comment

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

🛠️ 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.

Suggested change
// 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.


// Models with supportsReasoningBinary (binary reasoning) show a simple on/off toggle
if (isReasoningSupported) {
return (
Expand All @@ -175,6 +196,10 @@ export const ThinkingBudget = ({ apiConfiguration, setApiConfigurationField, mod
}>
{t("settings:providers.useReasoning")}
</Checkbox>
{/* Only show the max-output-tokens slider when reasoning is enabled: with it off,
getModelMaxOutputTokens returns the model default regardless of the slider, so an
interactive-but-ignored control would mislead. Matches the budget path's guard. */}
{enableReasoningEffort && maxOutputTokensControl}
</div>
)
}
Expand Down
39 changes: 39 additions & 0 deletions webview-ui/src/components/settings/__tests__/ThinkingBudget.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -111,6 +111,45 @@ describe("ThinkingBudget", () => {
expect(screen.queryByTestId("reasoning-effort")).not.toBeInTheDocument()
})

const binaryReasoningModel: ModelInfo = {
...mockModelInfo,
supportsReasoningBinary: true,
supportsReasoningBudget: false,
supportsReasoningEffort: false,
}

it("should hide the max-output-tokens slider in binary reasoning when reasoning is off", () => {
// With reasoning disabled, getModelMaxOutputTokens ignores the slider value, so an
// interactive control would be misleading — it must not render (PR #332 review).
render(
<ThinkingBudget
{...defaultProps}
apiConfiguration={{ enableReasoningEffort: false }}
modelInfo={binaryReasoningModel}
/>,
)

expect(screen.getByText("settings:providers.useReasoning")).toBeInTheDocument()
expect(screen.queryByTestId("slider")).not.toBeInTheDocument()
})

it("should show and wire the max-output-tokens slider in binary reasoning when reasoning is on", () => {
const setApiConfigurationField = vi.fn()
render(
<ThinkingBudget
{...defaultProps}
apiConfiguration={{ enableReasoningEffort: true }}
setApiConfigurationField={setApiConfigurationField}
modelInfo={binaryReasoningModel}
/>,
)

const slider = screen.getByTestId("slider")
expect(slider).toBeInTheDocument()
fireEvent.change(slider, { target: { value: "12288" } })
expect(setApiConfigurationField).toHaveBeenCalledWith("modelMaxTokens", 12288)
})

it("should render sliders when model supports thinking", () => {
render(<ThinkingBudget {...defaultProps} />)

Expand Down
Loading