Skip to content
Open
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
12 changes: 7 additions & 5 deletions webview-ui/src/components/settings/SettingsView.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ 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)
Expand All @@ -147,6 +147,7 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ onDone, t
const contentRef = useRef<HTMLDivElement | null>(null)

const prevApiConfigName = useRef(currentApiConfigName)
const prevMode = useRef(mode)
const handledSettingsImportedAt = useRef<number | undefined>(undefined)
const confirmDialogHandler = useRef<() => void>()

Expand Down Expand Up @@ -220,16 +221,17 @@ const SettingsView = forwardRef<SettingsViewRef, SettingsViewProps>(({ 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(() => {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,18 @@ vi.mock("@src/components/ui", () => ({
TooltipProvider: ({ children }: any) => <>{children}</>,
TooltipTrigger: ({ children }: any) => <>{children}</>,
TooltipContent: ({ children }: any) => <div>{children}</div>,
Command: ({ children }: any) => <div data-testid="command">{children}</div>,
CommandInput: ({ value, onValueChange }: any) => (
<input data-testid="command-input" value={value} onChange={(e) => onValueChange(e.target.value)} />
),
CommandGroup: ({ children }: any) => <div data-testid="command-group">{children}</div>,
CommandItem: ({ children, onSelect }: any) => (
<div data-testid="command-item" onClick={onSelect}>
{children}
</div>
),
CommandList: ({ children }: any) => <div data-testid="command-list">{children}</div>,
CommandEmpty: ({ children }: any) => <div data-testid="command-empty">{children}</div>,
Select: ({ children, value, onValueChange }: any) => (
<div data-testid="select" data-value={value}>
<button onClick={() => onValueChange && onValueChange("test-change")}>{value}</button>
Expand Down Expand Up @@ -332,6 +344,7 @@ describe("SettingsView - Change Detection Fix", () => {
autoCloseZooOpenedFiles: true,
autoCloseZooOpenedFilesAfterUserEdited: false,
autoCloseZooOpenedNewFiles: false,
mode: "code",
...overrides,
})

Expand Down Expand Up @@ -373,7 +386,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
Expand All @@ -394,7 +407,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()
Expand Down Expand Up @@ -473,7 +486,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()
Expand Down Expand Up @@ -525,5 +538,109 @@ 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(
<QueryClientProvider client={queryClient}>
<SettingsView onDone={onDone} />
</QueryClientProvider>,
)

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 the mode - this should trigger the mode sync effect
await act(async () => {
extensionState = createExtensionState({
mode: "ask",
apiConfiguration: {
apiProvider: "openrouter",
apiModelId: "claude-3.5-sonnet",
},
})
;(useExtensionState as any).mockImplementation(() => extensionState)

rerender(
<QueryClientProvider client={queryClient}>
<SettingsView onDone={onDone} />
</QueryClientProvider>,
)
})

// 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)
}, 20000)
Comment on lines +574 to +605

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Two mutations of the fix that this test seems to miss:

  • If mode were dropped from the dependency array, the effect would still fire here because extensionState is reassigned to a new object at line 576 — so the mode dep entry (part of this PR) is effectively untested.
  • If the prevMode.current = mode assignment were removed, the single code → ask transition would still produce correct state once; the stale ref is never re-exercised without a second transition.

Would it be worth adding a rerender where only extensionState identity changes (mode + apiConfigName unchanged) to pin the dep array, and a second same-mode rerender to expose a stale prevMode?


it("does not trigger sync when mode has not changed", async () => {
const onDone = vi.fn()
const extensionState = createExtensionState({
mode: "code",
apiConfiguration: {
apiProvider: "openai",
apiModelId: "gpt-4.1",
},
})

;(useExtensionState as any).mockImplementation(() => extensionState)

const { rerender } = render(
<QueryClientProvider client={queryClient}>
<SettingsView onDone={onDone} />
</QueryClientProvider>,
)

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 same mode - should not trigger sync
await act(async () => {
rerender(
<QueryClientProvider client={queryClient}>
<SettingsView onDone={onDone} />
</QueryClientProvider>,
)
})

// Provider value should remain unchanged from the dirty state
expect(screen.getByTestId("provider-value")).toHaveTextContent("baseten")
}, 20000)
Comment thread
coderabbitai[bot] marked this conversation as resolved.
Comment on lines +607 to +644

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

It looks like this test would pass even if the whole mode fix were reverted. Since extensionState is a const (line 609) and the rerender at line 635 reuses the same object reference, React sees no dependency change — the sync effect never re-runs under either the old or new code, so provider-value stays baseten purely from the dirty click above.

Would reassigning extensionState to a new object (same values, new identity) before the rerender make this guard load-bearing? As written, I think this catches a regression of nothing.

})
})
Loading