-
Notifications
You must be signed in to change notification settings - Fork 2.5k
fix: prevent dirty state on initial mount in ImageGenerationSettings #7495
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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) => { | ||
| 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) => { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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"> | ||
|
|
@@ -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)} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Consider using proper TypeScript types instead of |
||
| placeholder={t("settings:experimental.IMAGE_GENERATION.openRouterApiKeyPlaceholder")} | ||
| className="w-full" | ||
| type="password" | ||
|
|
@@ -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}> | ||
|
|
||
| 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 | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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() | ||
| }) | ||
| }) | ||
| }) | ||
There was a problem hiding this comment.
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?