From 55a41150afe9443c777d2653298b28c8d51a0ee5 Mon Sep 17 00:00:00 2001
From: cannuri <91494156+cannuri@users.noreply.github.com>
Date: Thu, 8 May 2025 21:55:57 +0200
Subject: [PATCH 1/4] Decouple Max Tokens from Max Thinking Tokens
---
.../src/components/__mocks__/Slider.tsx | 14 +
.../src/components/settings/ApiOptions.tsx | 12 +
.../settings/MaxOutputTokensControl.test.tsx | 280 +++++++++++
.../settings/MaxOutputTokensControl.tsx | 46 ++
.../components/settings/ThinkingBudget.tsx | 41 +-
.../settings/__tests__/ApiOptions.test.tsx | 458 +++++++-----------
.../__tests__/ThinkingBudget.test.tsx | 104 +++-
7 files changed, 623 insertions(+), 332 deletions(-)
create mode 100644 webview-ui/src/components/__mocks__/Slider.tsx
create mode 100644 webview-ui/src/components/settings/MaxOutputTokensControl.test.tsx
create mode 100644 webview-ui/src/components/settings/MaxOutputTokensControl.tsx
diff --git a/webview-ui/src/components/__mocks__/Slider.tsx b/webview-ui/src/components/__mocks__/Slider.tsx
new file mode 100644
index 0000000000..bf4b5fc3e7
--- /dev/null
+++ b/webview-ui/src/components/__mocks__/Slider.tsx
@@ -0,0 +1,14 @@
+import React from "react"
+
+// This is a manual mock for the Slider component.
+// It will be automatically used by Jest when jest.mock('../Slider') is called.
+
+// Create a Jest mock function for the component itself
+const MockSlider = jest.fn((props: any) => {
+ // You can add basic rendering if needed for other tests,
+ // or just keep it simple for prop checking.
+ // Include data-testid for potential queries if the component rendered something.
+ return
+})
+
+export const Slider = MockSlider
diff --git a/webview-ui/src/components/settings/ApiOptions.tsx b/webview-ui/src/components/settings/ApiOptions.tsx
index 905f34a860..1a301e1693 100644
--- a/webview-ui/src/components/settings/ApiOptions.tsx
+++ b/webview-ui/src/components/settings/ApiOptions.tsx
@@ -54,6 +54,7 @@ import { TemperatureControl } from "./TemperatureControl"
import { RateLimitSecondsControl } from "./RateLimitSecondsControl"
import { BedrockCustomArn } from "./providers/BedrockCustomArn"
import { buildDocLink } from "@src/utils/docLinks"
+import { MaxOutputTokensControl } from "./MaxOutputTokensControl"
export interface ApiOptionsProps {
uriScheme: string | undefined
@@ -462,9 +463,20 @@ const ApiOptions = ({
isDescriptionExpanded={isDescriptionExpanded}
setIsDescriptionExpanded={setIsDescriptionExpanded}
/>
+
)}
+ {selectedModelInfo &&
+ typeof selectedModelInfo.maxTokens === "number" &&
+ selectedModelInfo.maxTokens > 0 && (
+
+ )}
+
({
+ observe: jest.fn(),
+ unobserve: jest.fn(),
+ disconnect: jest.fn(),
+}))
+// Static import of Slider removed again to avoid TS error for non-existent module
+
+// Mock i18n
+jest.mock("react-i18next", () => ({
+ useTranslation: () => ({
+ t: (key: string) => key,
+ }),
+}))
+
+// Use jest.doMock with { virtual: true } to mock a non-existent module.
+// This mock factory will be used when '../Slider' is imported.
+jest.mock("../ui/slider", () => ({
+ __esModule: true,
+ Slider: jest.fn((props) => ),
+}))
+
+// Import the mocked Slider *after* jest.mock
+// We need to refer to it for assertions and clearing.
+// eslint-disable-next-line @typescript-eslint/no-var-requires
+const MockedSlider = require("../ui/slider").Slider as jest.Mock
+
+describe("MaxOutputTokensControl", () => {
+ const mockSetApiConfigurationField = jest.fn()
+ const mockApiConfiguration: ApiConfiguration = {
+ apiProvider: "openai",
+ openAiModelId: "test-model",
+ modelTemperature: 0.5,
+ modelMaxTokens: undefined,
+ modelMaxThinkingTokens: undefined,
+ }
+
+ beforeEach(() => {
+ mockSetApiConfigurationField.mockClear()
+ MockedSlider.mockClear()
+ })
+
+ it("should render when modelInfo.maxTokens is a positive number", () => {
+ const modelInfo: Partial = {
+ maxTokens: 16000,
+ // Add other potentially accessed properties if needed by the component, even if undefined
+ }
+
+ render(
+ ,
+ )
+
+ expect(screen.getByTestId("max-output-tokens-control")).toBeInTheDocument()
+ })
+
+ it("should display the current value and label", () => {
+ const modelInfo: Partial = {
+ maxTokens: 16000,
+ }
+ const apiConfigurationWithOverride: ApiConfiguration = {
+ ...mockApiConfiguration,
+ modelMaxTokens: 10000, // Override the default maxTokens
+ }
+
+ render(
+ ,
+ )
+
+ // Calculate the expected displayed value
+ const expectedValue = apiConfigurationWithOverride.modelMaxTokens ?? modelInfo.maxTokens!
+
+ // Assert that the current value is displayed
+ expect(screen.getByText(expectedValue.toString())).toBeInTheDocument()
+
+ // Assert that the label is displayed (using the mocked translation key)
+ expect(screen.getByText("providers.customModel.maxTokens.label")).toBeInTheDocument()
+ })
+
+ // Make the test async to use dynamic import
+ it("should pass min={8192} to the Slider component when rendered", async () => {
+ // Dynamically import Slider; it will be the mocked version due to jest.doMock
+ // Note: For this to work, the test function must be async and you await the import.
+ // However, for prop checking on a mock function, we can directly use the
+ // mockSliderImplementation defined in the jest.doMock scope.
+ // No need for dynamic import if we directly use the mock function instance.
+
+ const modelInfo: Partial = {
+ maxTokens: 16000, // A positive number to ensure rendering
+ }
+
+ render(
+ ,
+ )
+
+ // First, ensure the main control is rendered
+ expect(screen.getByTestId("max-output-tokens-control")).toBeInTheDocument()
+
+ // Assert that the mockSliderImplementation (which is what Slider becomes)
+ // was called with the correct 'min' prop.
+ // This test is expected to fail because Slider is not yet used or not with this prop.
+ expect(MockedSlider).toHaveBeenCalledWith(
+ expect.objectContaining({
+ min: 8192,
+ }),
+ expect.anything(), // Context for React components
+ )
+ })
+
+ it("should pass max={modelInfo.maxTokens!} to the Slider component when rendered", () => {
+ const modelInfo: Partial = {
+ maxTokens: 32000, // A positive number to ensure rendering
+ }
+
+ render(
+ ,
+ )
+
+ // First, ensure the main control is rendered
+ expect(screen.getByTestId("max-output-tokens-control")).toBeInTheDocument()
+
+ // Assert that the MockedSlider was called with the correct 'max' prop.
+ expect(MockedSlider).toHaveBeenCalledWith(
+ expect.objectContaining({
+ max: modelInfo.maxTokens,
+ }),
+ expect.anything(), // Context for React components
+ )
+ })
+
+ it("should pass step={1024} to the Slider component when rendered", () => {
+ const modelInfo: Partial = {
+ maxTokens: 16000, // A positive number to ensure rendering
+ }
+
+ render(
+ ,
+ )
+
+ // First, ensure the main control is rendered
+ expect(screen.getByTestId("max-output-tokens-control")).toBeInTheDocument()
+
+ // Assert that the MockedSlider was called with the correct 'step' prop.
+ expect(MockedSlider).toHaveBeenCalledWith(
+ expect.objectContaining({
+ step: 1024,
+ }),
+ expect.anything(), // Context for React components
+ )
+ })
+
+ describe("should pass the correct initial value to the Slider component", () => {
+ it("when apiConfiguration.modelMaxTokens is defined", () => {
+ const modelInfo: Partial = {
+ maxTokens: 32000, // This should be ignored
+ }
+ const apiConfigurationWithOverride: ApiConfiguration = {
+ ...mockApiConfiguration,
+ modelMaxTokens: 10000,
+ }
+
+ render(
+ ,
+ )
+
+ expect(MockedSlider).toHaveBeenCalledWith(
+ expect.objectContaining({
+ value: [apiConfigurationWithOverride.modelMaxTokens],
+ }),
+ expect.anything(),
+ )
+ })
+
+ it("when apiConfiguration.modelMaxTokens is undefined", () => {
+ const modelInfo: Partial = {
+ maxTokens: 16000, // This should be used
+ }
+ const apiConfigurationWithoutOverride: ApiConfiguration = {
+ ...mockApiConfiguration,
+ modelMaxTokens: undefined,
+ }
+
+ render(
+ ,
+ )
+
+ expect(MockedSlider).toHaveBeenCalledWith(
+ expect.objectContaining({
+ value: [modelInfo.maxTokens!],
+ }),
+ expect.anything(),
+ )
+ })
+
+ it('should call setApiConfigurationField with "modelMaxTokens" and the new value when the slider value changes', () => {
+ const modelInfo: Partial = {
+ maxTokens: 32000, // A positive number to ensure rendering
+ }
+
+ render(
+ ,
+ )
+
+ // Simulate a value change by calling the onValueChange prop of the mocked Slider
+ // We need to access the props that the mocked component was called with.
+ // MockedSlider.mock.calls[0][0] gives us the props of the first render call.
+ const sliderProps = MockedSlider.mock.calls[0][0]
+ const newValue = [20000]
+ sliderProps.onValueChange(newValue)
+
+ // Assert that setApiConfigurationField was called with the correct arguments
+ expect(mockSetApiConfigurationField).toHaveBeenCalledWith("modelMaxTokens", newValue[0])
+ })
+ })
+
+ describe("should not render when modelInfo.maxTokens is not a positive number", () => {
+ const testCases = [
+ { name: "undefined", value: undefined },
+ { name: "null", value: null },
+ { name: "0", value: 0 },
+ ]
+
+ it.each(testCases)("when maxTokens is $name ($value)", ({ value }) => {
+ const modelInfo: Partial = {
+ maxTokens: value as any, // Use 'as any' to allow null/undefined for testing
+ }
+
+ render(
+ ,
+ )
+
+ // Check that the component renders null or an empty fragment
+ // by querying for the test ID and expecting it not to be there.
+ expect(screen.queryByTestId("max-output-tokens-control")).not.toBeInTheDocument()
+ // Alternatively, if the component renders null, its container might be empty or contain minimal structure.
+ // For a component that should render absolutely nothing, its direct container might be empty.
+ // However, queryByTestId is more robust for checking absence.
+ })
+ })
+})
diff --git a/webview-ui/src/components/settings/MaxOutputTokensControl.tsx b/webview-ui/src/components/settings/MaxOutputTokensControl.tsx
new file mode 100644
index 0000000000..6c3c991de8
--- /dev/null
+++ b/webview-ui/src/components/settings/MaxOutputTokensControl.tsx
@@ -0,0 +1,46 @@
+import React from "react"
+import { ApiConfiguration, ModelInfo } from "@roo/shared/api"
+import { Slider } from "../ui/slider"
+import { useTranslation } from "react-i18next"
+
+interface MaxOutputTokensControlProps {
+ apiConfiguration: ApiConfiguration
+ setApiConfigurationField: (field: K, value: ApiConfiguration[K]) => void
+ modelInfo?: ModelInfo
+}
+
+const MIN_OUTPUT_TOKENS = 8192
+const STEP_OUTPUT_TOKENS = 1024
+
+export const MaxOutputTokensControl: React.FC = ({
+ apiConfiguration,
+ setApiConfigurationField,
+ modelInfo,
+}) => {
+ const { t } = useTranslation()
+ const shouldRender = modelInfo && typeof modelInfo.maxTokens === "number" && modelInfo.maxTokens > 0
+
+ if (!shouldRender) {
+ return null
+ }
+
+ const currentMaxOutputTokens = apiConfiguration.modelMaxTokens ?? modelInfo.maxTokens!
+
+ // Basic structure for now, will be expanded in later tasks.
+ return (
+