Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
32 changes: 25 additions & 7 deletions webview-ui/src/components/settings/ImageGenerationSettings.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -36,14 +36,32 @@ export const ImageGenerationSettings = ({
imageGenerationSettings.selectedModel || IMAGE_GENERATION_MODELS[0].value,
)

// Update parent state when local state changes
// Update local state when apiConfiguration changes (e.g., when switching profiles)
useEffect(() => {
setOpenRouterApiKey(imageGenerationSettings.openRouterApiKey || "")
setSelectedModel(imageGenerationSettings.selectedModel || IMAGE_GENERATION_MODELS[0].value)
}, [imageGenerationSettings.openRouterApiKey, imageGenerationSettings.selectedModel])

// Helper function to update settings
const updateSettings = (newApiKey: string, newModel: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it necessary to pass both values when only one changes? Could we optimize this to only update the changed field, perhaps by having the updateSettings function read the current state internally?

const newSettings = {
openRouterApiKey,
selectedModel,
openRouterApiKey: newApiKey,
selectedModel: newModel,
}
setApiConfigurationField("openRouterImageGenerationSettings", newSettings)
}, [openRouterApiKey, selectedModel, setApiConfigurationField])
setApiConfigurationField("openRouterImageGenerationSettings", newSettings, true)
}

// Handle API key changes
const handleApiKeyChange = (value: string) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you considered debouncing the API key input? Since this triggers a state update on every keystroke, rapid typing could cause many unnecessary updates. A debounce of 300-500ms could improve performance without affecting user experience.

setOpenRouterApiKey(value)
updateSettings(value, selectedModel)
}

// Handle model selection changes
const handleModelChange = (value: string) => {
setSelectedModel(value)
updateSettings(openRouterApiKey, value)
}

return (
<div className="space-y-4">
Expand All @@ -67,7 +85,7 @@ export const ImageGenerationSettings = ({
</label>
<VSCodeTextField
value={openRouterApiKey}
onInput={(e: any) => setOpenRouterApiKey(e.target.value)}
onInput={(e: any) => handleApiKeyChange(e.target.value)}
Copy link
Contributor

Choose a reason for hiding this comment

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

Consider using proper TypeScript types instead of any for better type safety. For example: (e: Event) => handleApiKeyChange((e.target as HTMLInputElement).value)

placeholder={t("settings:experimental.IMAGE_GENERATION.openRouterApiKeyPlaceholder")}
className="w-full"
type="password"
Expand All @@ -91,7 +109,7 @@ export const ImageGenerationSettings = ({
</label>
<VSCodeDropdown
value={selectedModel}
onChange={(e: any) => setSelectedModel(e.target.value)}
onChange={(e: any) => handleModelChange(e.target.value)}
className="w-full">
{IMAGE_GENERATION_MODELS.map((model) => (
<VSCodeOption key={model.value} value={model.value}>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,94 @@
import { render, fireEvent } from "@testing-library/react"
import { vi } from "vitest"
import { ImageGenerationSettings } from "../ImageGenerationSettings"
import type { ProviderSettings } from "@roo-code/types"

// Mock the translation context
vi.mock("@/i18n/TranslationContext", () => ({
useAppTranslation: () => ({
t: (key: string) => key,
}),
}))

describe("ImageGenerationSettings", () => {
const mockSetApiConfigurationField = vi.fn()
const mockOnChange = vi.fn()

const defaultProps = {
enabled: false,
onChange: mockOnChange,
apiConfiguration: {} as ProviderSettings,
setApiConfigurationField: mockSetApiConfigurationField,
}

beforeEach(() => {
vi.clearAllMocks()
})

describe("Initial Mount Behavior", () => {
it("should not call setApiConfigurationField on initial mount with empty configuration", () => {
render(<ImageGenerationSettings {...defaultProps} />)

// Should NOT call setApiConfigurationField on initial mount to prevent dirty state
expect(mockSetApiConfigurationField).not.toHaveBeenCalled()
})

it("should not call setApiConfigurationField on initial mount with existing configuration", () => {
const apiConfiguration = {
openRouterImageGenerationSettings: {
openRouterApiKey: "existing-key",
selectedModel: "google/gemini-2.5-flash-image-preview:free",
},
} as ProviderSettings

render(<ImageGenerationSettings {...defaultProps} apiConfiguration={apiConfiguration} />)

// Should NOT call setApiConfigurationField on initial mount to prevent dirty state
expect(mockSetApiConfigurationField).not.toHaveBeenCalled()
})
})

describe("User Interaction Behavior", () => {
it("should call setApiConfigurationField when user changes API key", async () => {
const { getByPlaceholderText } = render(<ImageGenerationSettings {...defaultProps} enabled={true} />)

const apiKeyInput = getByPlaceholderText(
"settings:experimental.IMAGE_GENERATION.openRouterApiKeyPlaceholder",
)

// Simulate user typing
fireEvent.input(apiKeyInput, { target: { value: "new-api-key" } })

// Should call setApiConfigurationField with isUserAction=true
expect(mockSetApiConfigurationField).toHaveBeenCalledWith(
"openRouterImageGenerationSettings",
{
openRouterApiKey: "new-api-key",
selectedModel: "google/gemini-2.5-flash-image-preview",
},
true, // This should be true for user actions
)
})

// Note: Testing VSCode dropdown components is complex due to their custom nature
Copy link
Contributor

Choose a reason for hiding this comment

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

While I understand testing VSCode dropdown components is complex, would it be possible to add at least a basic test for the model selection handler? This would ensure complete coverage of all user interactions. You could mock the VSCodeDropdown component if needed.

// The key functionality (not marking as dirty on initial mount) is already tested above
})

describe("Conditional Rendering", () => {
it("should render input fields when enabled is true", () => {
const { getByPlaceholderText } = render(<ImageGenerationSettings {...defaultProps} enabled={true} />)

expect(
getByPlaceholderText("settings:experimental.IMAGE_GENERATION.openRouterApiKeyPlaceholder"),
).toBeInTheDocument()
})

it("should not render input fields when enabled is false", () => {
const { queryByPlaceholderText } = render(<ImageGenerationSettings {...defaultProps} enabled={false} />)

expect(
queryByPlaceholderText("settings:experimental.IMAGE_GENERATION.openRouterApiKeyPlaceholder"),
).not.toBeInTheDocument()
})
})
})
Loading