diff --git a/webview-ui/src/components/settings/SettingsView.tsx b/webview-ui/src/components/settings/SettingsView.tsx index 87052a21a2..0b53342133 100644 --- a/webview-ui/src/components/settings/SettingsView.tsx +++ b/webview-ui/src/components/settings/SettingsView.tsx @@ -130,7 +130,7 @@ const SettingsView = forwardRef(({ onDone, t const { t } = useAppTranslation() const extensionState = useExtensionState() - const { currentApiConfigName, listApiConfigMeta, uriScheme, settingsImportedAt } = extensionState + const { currentApiConfigName, listApiConfigMeta, uriScheme, settingsImportedAt, mode } = extensionState const [isDiscardDialogShow, setDiscardDialogShow] = useState(false) const [isChangeDetected, setChangeDetected] = useState(false) @@ -147,6 +147,7 @@ const SettingsView = forwardRef(({ onDone, t const contentRef = useRef(null) const prevApiConfigName = useRef(currentApiConfigName) + const prevMode = useRef(mode) const handledSettingsImportedAt = useRef(undefined) const confirmDialogHandler = useRef<() => void>() @@ -220,16 +221,17 @@ const SettingsView = forwardRef(({ onDone, t const apiConfiguration = useMemo(() => cachedState.apiConfiguration ?? {}, [cachedState.apiConfiguration]) useEffect(() => { - // Update only when currentApiConfigName is changed. - // Expected to be triggered by loadApiConfiguration/upsertApiConfiguration. - if (prevApiConfigName.current === currentApiConfigName) { + // Update when currentApiConfigName or mode changes. + // Expected to be triggered by loadApiConfiguration/upsertApiConfiguration or mode switch. + if (prevApiConfigName.current === currentApiConfigName && prevMode.current === mode) { return } setCachedState((prevCachedState) => ({ ...prevCachedState, ...extensionState })) prevApiConfigName.current = currentApiConfigName + prevMode.current = mode setChangeDetected(false) - }, [currentApiConfigName, extensionState]) + }, [currentApiConfigName, mode, extensionState]) // Bust the cache when settings are imported. useEffect(() => { diff --git a/webview-ui/src/components/settings/__tests__/SettingsView.change-detection.spec.tsx b/webview-ui/src/components/settings/__tests__/SettingsView.change-detection.spec.tsx index 2ca7107335..68a6efb106 100644 --- a/webview-ui/src/components/settings/__tests__/SettingsView.change-detection.spec.tsx +++ b/webview-ui/src/components/settings/__tests__/SettingsView.change-detection.spec.tsx @@ -1,5 +1,5 @@ import { act, render, screen, fireEvent, waitFor, configure } from "@testing-library/react" -import { vi, describe, it, expect, beforeEach } from "vitest" +import { vi, describe, it, expect, beforeEach, beforeAll } from "vitest" import { QueryClient, QueryClientProvider } from "@tanstack/react-query" import React from "react" @@ -19,8 +19,6 @@ vi.mock("@src/utils/vscode", () => ({ }, })) -// Import the actual component -import SettingsView from "../SettingsView" import { useExtensionState } from "@src/context/ExtensionStateContext" // Mock the extension state context @@ -28,10 +26,12 @@ vi.mock("@src/context/ExtensionStateContext", () => ({ useExtensionState: vi.fn(), })) +const mockTranslate = vi.hoisted(() => (key: string) => key) + // Mock the translation context vi.mock("@src/i18n/TranslationContext", () => ({ useAppTranslation: () => ({ - t: (key: string) => key, + t: mockTranslate, }), })) @@ -94,6 +94,18 @@ vi.mock("@src/components/ui", () => ({ TooltipProvider: ({ children }: any) => <>{children}, TooltipTrigger: ({ children }: any) => <>{children}, TooltipContent: ({ children }: any) =>
{children}
, + Command: ({ children }: any) =>
{children}
, + CommandInput: ({ value, onValueChange }: any) => ( + onValueChange(e.target.value)} /> + ), + CommandGroup: ({ children }: any) =>
{children}
, + CommandItem: ({ children, onSelect }: any) => ( +
+ {children} +
+ ), + CommandList: ({ children }: any) =>
{children}
, + CommandEmpty: ({ children }: any) =>
{children}
, Select: ({ children, value, onValueChange }: any) => (
@@ -181,13 +193,63 @@ vi.mock("@src/components/mcp/McpView", () => ({ default: () => null, })) -// Mock Tab components -vi.mock("../common/Tab", () => ({ +vi.mock("../../common/Tab", () => ({ Tab: ({ children }: any) =>
{children}
, - TabContent: React.forwardRef(({ children }, ref) =>
{children}
), + TabContent: React.forwardRef(({ children, ...props }, ref) => ( +
+ {children} +
+ )), TabHeader: ({ children }: any) =>
{children}
, - TabList: ({ children }: any) =>
{children}
, - TabTrigger: React.forwardRef(({ children }, ref) => ), + TabList: ({ children, value, onValueChange }: any) => ( +
+ {React.Children.map(children, (child) => { + if (!React.isValidElement(child)) { + return child + } + + const element = child as React.ReactElement + return React.cloneElement(element, { + isSelected: element.props.value === value, + onSelect: () => onValueChange(element.props.value), + }) + })} +
+ ), + TabTrigger: React.forwardRef(({ children, onSelect, ...props }, ref) => ( + + )), +})) +vi.mock("@src/components/common/Tab", () => ({ + Tab: ({ children }: any) =>
{children}
, + TabContent: React.forwardRef(({ children, ...props }, ref) => ( +
+ {children} +
+ )), + TabHeader: ({ children }: any) =>
{children}
, + TabList: ({ children, value, onValueChange }: any) => ( +
+ {React.Children.map(children, (child) => { + if (!React.isValidElement(child)) { + return child + } + + const element = child as React.ReactElement + return React.cloneElement(element, { + isSelected: element.props.value === value, + onSelect: () => onValueChange(element.props.value), + }) + })} +
+ ), + TabTrigger: React.forwardRef(({ children, onSelect, ...props }, ref) => ( + + )), })) // Mock all child components to isolate the test @@ -195,25 +257,30 @@ vi.mock("../ApiConfigManager", () => ({ default: () => null, })) +const mockApiOptions = ({ apiConfiguration, setApiConfigurationField }: any) => ( +
+ {apiConfiguration.apiProvider} + setApiConfigurationField("basetenApiKey", event.target.value)} + /> + {["openrouter", "baseten", "deepseek", "friendli"].map((provider) => ( + + ))} +
+) + vi.mock("../ApiOptions", () => ({ - default: ({ apiConfiguration, setApiConfigurationField }: any) => ( -
- {apiConfiguration.apiProvider} - setApiConfigurationField("basetenApiKey", event.target.value)} - /> - {["openrouter", "baseten", "deepseek", "friendli"].map((provider) => ( - - ))} -
- ), + default: mockApiOptions, +})) +vi.mock("@src/components/settings/ApiOptions", () => ({ + default: mockApiOptions, })) vi.mock("../AutoApproveSettings", () => ({ @@ -228,6 +295,43 @@ vi.mock("../Section", () => ({ Section: ({ children }: any) =>
{children}
, })) +vi.mock("../SearchableSetting", () => ({ + SearchableSetting: ({ children }: any) =>
{children}
, +})) +vi.mock("../useSettingsSearch", () => ({ + SearchIndexProvider: ({ children }: any) => <>{children}, + useSearchIndexRegistry: () => ({ + contextValue: { registerSetting: vi.fn() }, + index: [], + }), + useSettingsSearch: () => ({ + searchQuery: "", + setSearchQuery: vi.fn(), + results: [], + isOpen: false, + setIsOpen: vi.fn(), + clearSearch: vi.fn(), + }), +})) +vi.mock("@src/components/settings/SearchableSetting", () => ({ + SearchableSetting: ({ children }: any) =>
{children}
, +})) +vi.mock("@src/components/settings/useSettingsSearch", () => ({ + SearchIndexProvider: ({ children }: any) => <>{children}, + useSearchIndexRegistry: () => ({ + contextValue: { registerSetting: vi.fn() }, + index: [], + }), + useSettingsSearch: () => ({ + searchQuery: "", + setSearchQuery: vi.fn(), + results: [], + isOpen: false, + setIsOpen: vi.fn(), + clearSearch: vi.fn(), + }), +})) + // Mock all settings components vi.mock("../CheckpointSettings", () => ({ CheckpointSettings: () => null, @@ -263,6 +367,11 @@ vi.mock("../UISettings", () => ({ vi.mock("../SettingsSearch", () => ({ SettingsSearch: () => null, })) +vi.mock("@src/components/settings/SettingsSearch", () => ({ + SettingsSearch: () => null, +})) + +let SettingsView: typeof import("../SettingsView").default describe("SettingsView - Change Detection Fix", () => { let queryClient: QueryClient @@ -332,9 +441,16 @@ describe("SettingsView - Change Detection Fix", () => { autoCloseZooOpenedFiles: true, autoCloseZooOpenedFilesAfterUserEdited: false, autoCloseZooOpenedNewFiles: false, + mode: "code", ...overrides, }) + beforeAll(async () => { + // Import after mocks are registered so the isolated tests use the + // lightweight child component mocks above instead of the full settings UI. + SettingsView = (await import("../SettingsView")).default + }) + beforeEach(() => { vi.clearAllMocks() queryClient = new QueryClient({ @@ -373,7 +489,7 @@ describe("SettingsView - Change Detection Fix", () => { // onDone should be called expect(onDone).toHaveBeenCalled() - }) + }, 10000) // These tests are passing for the basic case but failing due to vi.doMock limitations // The core fix has been verified - when no actual changes are made, no unsaved changes dialog appears @@ -394,7 +510,7 @@ describe("SettingsView - Change Detection Fix", () => { // - null -> value (initialization from null) expect(true).toBe(true) // Placeholder - the real test is the running system - }) + }, 10000) it("preserves a DeepSeek provider edit after saving Baseten when the same import timestamp replays", async () => { const onDone = vi.fn() @@ -473,7 +589,7 @@ describe("SettingsView - Change Detection Fix", () => { apiProvider: "deepseek", }), }) - }) + }, 10000) it("resets cached provider state when a new import timestamp arrives", async () => { const onDone = vi.fn() @@ -525,5 +641,152 @@ describe("SettingsView - Change Detection Fix", () => { await waitFor(() => { expect(screen.getByTestId("save-button")).toBeDisabled() }) + }, 10000) + + describe("mode synchronization", () => { + it("resets changeDetected and syncs cachedState when mode changes after dirty state", async () => { + const onDone = vi.fn() + let extensionState = createExtensionState({ + mode: "code", + apiConfiguration: { + apiProvider: "openai", + apiModelId: "gpt-4.1", + }, + }) + + ;(useExtensionState as any).mockImplementation(() => extensionState) + + const { rerender } = render( + + + , + ) + + await waitFor(() => { + expect(screen.getByTestId("provider-value")).toHaveTextContent("openai") + }) + + // Make a dirty change by switching provider + fireEvent.click(screen.getByTestId("set-provider-baseten")) + expect(screen.getByTestId("provider-value")).toHaveTextContent("baseten") + + // Verify save button is enabled (dirty state) + const saveButton = screen.getByTestId("save-button") as HTMLButtonElement + expect(saveButton.disabled).toBe(false) + + // Now change only the mode-dependent values while keeping extensionState's + // object identity stable. This makes the `mode` dependency load-bearing: + // without it, React would not re-run the sync effect. + await act(async () => { + extensionState.mode = "ask" + extensionState.apiConfiguration = { + apiProvider: "openrouter", + apiModelId: "claude-3.5-sonnet", + } + + rerender( + + + , + ) + }) + + // Let the mode sync effect run + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)) + }) + + // Verify cachedState reflects the new mode's settings + await waitFor(() => { + expect(screen.getByTestId("provider-value")).toHaveTextContent("openrouter") + }) + + // Verify changeDetected is reset (save button should be disabled) + const updatedSaveButton = screen.getByTestId("save-button") as HTMLButtonElement + expect(updatedSaveButton.disabled).toBe(true) + + // Make another dirty change while already in the new mode. + fireEvent.click(screen.getByTestId("set-provider-deepseek")) + expect(screen.getByTestId("provider-value")).toHaveTextContent("deepseek") + expect((screen.getByTestId("save-button") as HTMLButtonElement).disabled).toBe(false) + + // Re-render with a new extensionState identity but the same mode and config + // name. If prevMode.current is not updated during the first mode transition, + // the stale ref makes this same-mode render look like another mode change and + // incorrectly overwrites the dirty cached provider below. + await act(async () => { + extensionState = createExtensionState({ + mode: "ask", + apiConfiguration: { + apiProvider: "friendli", + apiModelId: "friendli-model", + }, + }) + ;(useExtensionState as any).mockImplementation(() => extensionState) + + rerender( + + + , + ) + }) + + await act(async () => { + await new Promise((resolve) => setTimeout(resolve, 0)) + }) + + expect(screen.getByTestId("provider-value")).toHaveTextContent("deepseek") + expect((screen.getByTestId("save-button") as HTMLButtonElement).disabled).toBe(false) + }, 20000) + + it("does not trigger sync when mode has not changed", async () => { + const onDone = vi.fn() + let extensionState = createExtensionState({ + mode: "code", + apiConfiguration: { + apiProvider: "openai", + apiModelId: "gpt-4.1", + }, + }) + + ;(useExtensionState as any).mockImplementation(() => extensionState) + + const { rerender } = render( + + + , + ) + + await waitFor(() => { + expect(screen.getByTestId("provider-value")).toHaveTextContent("openai") + }) + + // Make a dirty change so we can verify it isn't overwritten by a sync + fireEvent.click(screen.getByTestId("set-provider-baseten")) + expect(screen.getByTestId("provider-value")).toHaveTextContent("baseten") + + // Re-render with a new extensionState identity but the same mode and config + // name. This makes the guard load-bearing because the effect is eligible to + // re-run from the extensionState dependency, but must not sync cachedState. + await act(async () => { + extensionState = createExtensionState({ + mode: "code", + apiConfiguration: { + apiProvider: "openai", + apiModelId: "gpt-4.1", + }, + }) + ;(useExtensionState as any).mockImplementation(() => extensionState) + + rerender( + + + , + ) + }) + + // Provider value should remain unchanged from the dirty state + expect(screen.getByTestId("provider-value")).toHaveTextContent("baseten") + }, 20000) }) })