From 08e0a7bf54c29c388e738bd92bc3c1a58abfeb7d Mon Sep 17 00:00:00 2001 From: JunghwanNA <70629228+shaun0927@users.noreply.github.com> Date: Fri, 17 Apr 2026 00:28:11 +0900 Subject: [PATCH 1/4] Prevent stale model fetches from polluting a switched provider list Dynamic provider-model fetching allows users to switch providers while a request is still in flight. The Add Model form captured the selected provider in the callback closure, so a late response from the previous provider could still replace `fetchedModelsList` after the UI had moved on. Track the latest provider in a ref and ignore outdated async results. Add a focused regression test that reproduces the race by resolving an OpenAI fetch after switching the UI to Anthropic. Constraint: Keep the fix limited to the validated GUI race and regression coverage Rejected: Cancel in-flight requests globally | more invasive than needed for the current flow Confidence: high Scope-risk: narrow Reversibility: clean Directive: If the UI later supports multiple concurrent fetches per provider, switch from provider tracking to request-token invalidation Tested: cd gui && npm test -- --run src/forms/AddModelForm.dynamicFetchRace.test.tsx Not-tested: Full gui test suite; lint; typecheck; manual VS Code / JetBrains verification --- .../AddModelForm.dynamicFetchRace.test.tsx | 110 ++++++++++++++++++ gui/src/forms/AddModelForm.tsx | 13 ++- 2 files changed, 119 insertions(+), 4 deletions(-) create mode 100644 gui/src/forms/AddModelForm.dynamicFetchRace.test.tsx diff --git a/gui/src/forms/AddModelForm.dynamicFetchRace.test.tsx b/gui/src/forms/AddModelForm.dynamicFetchRace.test.tsx new file mode 100644 index 00000000000..3648b41893a --- /dev/null +++ b/gui/src/forms/AddModelForm.dynamicFetchRace.test.tsx @@ -0,0 +1,110 @@ +import { act, screen, waitFor } from "@testing-library/react"; +import { describe, expect, it, vi, beforeEach } from "vitest"; +import { AddModelForm } from "./AddModelForm"; +import { renderWithProviders } from "../util/test/render"; + +const fetchProviderModelsMock = vi.hoisted(() => vi.fn()); +const initializeDynamicModelsMock = vi.hoisted(() => vi.fn(async () => {})); + +vi.mock("../pages/AddNewModel/configs/fetchProviderModels", () => ({ + fetchProviderModels: fetchProviderModelsMock, + initializeDynamicModels: initializeDynamicModelsMock, +})); + +vi.mock("../components/modelSelection/ModelSelectionListbox", () => ({ + default: ({ + selectedProvider, + setSelectedProvider, + topOptions = [], + otherOptions = [], + searchPlaceholder, + }: any) => ( +
+
+ {selectedProvider.title} +
+
+ {topOptions.map((option: any) => ( + + ))} +
+
+ {otherOptions.map((option: any) => ( +
+ {option.title} +
+ ))} +
+
+ ), +})); + +function deferred() { + let resolve!: (value: T) => void; + const promise = new Promise((res) => { + resolve = res; + }); + return { promise, resolve }; +} + +function makeFetchedModel(title: string) { + return { + title, + description: title, + params: { + title, + model: title.toLowerCase().replace(/\s+/g, "-"), + }, + isOpenSource: false, + providerOptions: ["openai"], + }; +} + +describe("AddModelForm dynamic fetch race", () => { + beforeEach(() => { + fetchProviderModelsMock.mockReset(); + initializeDynamicModelsMock.mockClear(); + }); + + it("does not leak stale models from the previous provider if the earlier fetch resolves late", async () => { + const pendingOpenAiFetch = deferred(); + + fetchProviderModelsMock.mockImplementation((_messenger: unknown, provider: string) => { + if (provider === "openai") { + return pendingOpenAiFetch.promise; + } + return Promise.resolve([]); + }); + + const { user } = await renderWithProviders(); + + await user.type( + screen.getByPlaceholderText(/Enter your OpenAI API key/i), + "sk-test-key", + ); + await user.click(screen.getByTitle(/fetch available models/i)); + + await user.click(screen.getByRole("button", { name: "Anthropic" })); + expect(screen.getByTestId("provider-current")).toHaveTextContent("Anthropic"); + + await act(async () => { + pendingOpenAiFetch.resolve([makeFetchedModel("OpenAI Dynamic Model")]); + await pendingOpenAiFetch.promise; + }); + + await waitFor(() => { + expect(screen.getByTestId("provider-current")).toHaveTextContent( + "Anthropic", + ); + expect(screen.getByTestId("model-listbox")).not.toHaveTextContent( + "OpenAI Dynamic Model", + ); + }); + }); +}); diff --git a/gui/src/forms/AddModelForm.tsx b/gui/src/forms/AddModelForm.tsx index 7041d2c0814..d3d8ffbad3a 100644 --- a/gui/src/forms/AddModelForm.tsx +++ b/gui/src/forms/AddModelForm.tsx @@ -2,7 +2,7 @@ import { ArrowPathIcon, ArrowTopRightOnSquareIcon, } from "@heroicons/react/24/outline"; -import { useCallback, useContext, useEffect, useState } from "react"; +import { useCallback, useContext, useEffect, useRef, useState } from "react"; import { FormProvider, useForm } from "react-hook-form"; import { Button, Input, StyledActionButton } from "../components"; import Alert from "../components/gui/Alert"; @@ -49,11 +49,16 @@ export function AddModelForm({ [], ); const [isFetchingModels, setIsFetchingModels] = useState(false); + const selectedProviderRef = useRef(selectedProvider.provider); useEffect(() => { void initializeDynamicModels(ideMessenger); }, []); + useEffect(() => { + selectedProviderRef.current = selectedProvider.provider; + }, [selectedProvider]); + useEffect(() => { setFetchedModelsList([]); }, [selectedProvider]); @@ -72,9 +77,9 @@ export function AddModelForm({ apiKey, apiBase, ); - setFetchedModelsList((prev) => - selectedProvider.provider === providerAtFetchTime ? models : prev, - ); + if (selectedProviderRef.current === providerAtFetchTime) { + setFetchedModelsList(models); + } } catch (error) { console.error("Failed to fetch models:", error); } finally { From fbd092c34aadf7b611fd426f69bfe6a3a38f1a91 Mon Sep 17 00:00:00 2001 From: JunghwanNA <70629228+shaun0927@users.noreply.github.com> Date: Fri, 17 Apr 2026 01:02:15 +0900 Subject: [PATCH 2/4] Close the last provider-switch race window in dynamic model fetching The first fix guarded late async results, but the provider ref still moved in a passive effect. Under a fast provider switch, an older in-flight request could resolve before that effect ran and still see the stale provider value. Update the ref synchronously at provider-selection time and format the regression test so Prettier passes in CI. Constraint: Keep the follow-up limited to the bot-confirmed race window and formatting failure Rejected: Expand this PR to address provider-specific listModels config gaps | unrelated to the validated GUI race and would broaden scope Confidence: high Scope-risk: narrow Reversibility: clean Directive: If provider selection can change from additional code paths later, centralize provider-ref synchronization instead of relying on one callback Tested: cd gui && npm test -- --run src/forms/AddModelForm.dynamicFetchRace.test.tsx Tested: ./node_modules/.bin/prettier --check gui/src/forms/AddModelForm.tsx gui/src/forms/AddModelForm.dynamicFetchRace.test.tsx --ignore-path .gitignore --ignore-path .prettierignore Not-tested: Full GUI test suite; upstream CI rerun after push; JetBrains/e2e workflows --- .../AddModelForm.dynamicFetchRace.test.tsx | 26 ++++++++++++------- gui/src/forms/AddModelForm.tsx | 5 +--- 2 files changed, 18 insertions(+), 13 deletions(-) diff --git a/gui/src/forms/AddModelForm.dynamicFetchRace.test.tsx b/gui/src/forms/AddModelForm.dynamicFetchRace.test.tsx index 3648b41893a..f0080ec2970 100644 --- a/gui/src/forms/AddModelForm.dynamicFetchRace.test.tsx +++ b/gui/src/forms/AddModelForm.dynamicFetchRace.test.tsx @@ -20,7 +20,9 @@ vi.mock("../components/modelSelection/ModelSelectionListbox", () => ({ searchPlaceholder, }: any) => (
-
+
{selectedProvider.title}
@@ -75,14 +77,18 @@ describe("AddModelForm dynamic fetch race", () => { it("does not leak stale models from the previous provider if the earlier fetch resolves late", async () => { const pendingOpenAiFetch = deferred(); - fetchProviderModelsMock.mockImplementation((_messenger: unknown, provider: string) => { - if (provider === "openai") { - return pendingOpenAiFetch.promise; - } - return Promise.resolve([]); - }); + fetchProviderModelsMock.mockImplementation( + (_messenger: unknown, provider: string) => { + if (provider === "openai") { + return pendingOpenAiFetch.promise; + } + return Promise.resolve([]); + }, + ); - const { user } = await renderWithProviders(); + const { user } = await renderWithProviders( + , + ); await user.type( screen.getByPlaceholderText(/Enter your OpenAI API key/i), @@ -91,7 +97,9 @@ describe("AddModelForm dynamic fetch race", () => { await user.click(screen.getByTitle(/fetch available models/i)); await user.click(screen.getByRole("button", { name: "Anthropic" })); - expect(screen.getByTestId("provider-current")).toHaveTextContent("Anthropic"); + expect(screen.getByTestId("provider-current")).toHaveTextContent( + "Anthropic", + ); await act(async () => { pendingOpenAiFetch.resolve([makeFetchedModel("OpenAI Dynamic Model")]); diff --git a/gui/src/forms/AddModelForm.tsx b/gui/src/forms/AddModelForm.tsx index d3d8ffbad3a..bba746e9454 100644 --- a/gui/src/forms/AddModelForm.tsx +++ b/gui/src/forms/AddModelForm.tsx @@ -55,10 +55,6 @@ export function AddModelForm({ void initializeDynamicModels(ideMessenger); }, []); - useEffect(() => { - selectedProviderRef.current = selectedProvider.provider; - }, [selectedProvider]); - useEffect(() => { setFetchedModelsList([]); }, [selectedProvider]); @@ -208,6 +204,7 @@ export function AddModelForm({ (provider) => provider.title === val.title, ); if (match) { + selectedProviderRef.current = match.provider; setSelectedProvider(match); } }} From 1078373f251106f8c13a71d872d7855a25e15f7e Mon Sep 17 00:00:00 2001 From: JunghwanNA <70629228+shaun0927@users.noreply.github.com> Date: Fri, 17 Apr 2026 13:31:05 +0900 Subject: [PATCH 3/4] Prevent provider switches from reusing the previous API key during model fetches Codex surfaced a real follow-up risk in the Add Model flow: after switching between two providers that both require API keys, the form could retain the previous provider's secret long enough for a fetch click to send it to the new provider. This clears the shared API key field on provider changes and adds a focused regression test that proves a keyed-to-keyed switch leaves no stale key behind for the next fetch. Constraint: Keep the follow-up limited to the credible cross-provider credential leak in the existing GUI flow Rejected: Expand this PR to broader listModels capability filtering or provider-specific fetch config forwarding | those are separate fetch-path concerns and would widen the validated race-fix scope Confidence: high Scope-risk: narrow Reversibility: clean Directive: If the form later stores provider-specific secrets simultaneously, replace this blanket reset with per-provider key scoping rather than reusing one shared apiKey field Tested: cd gui && npm test -- --run src/forms/AddModelForm.dynamicFetchRace.test.tsx Tested: ./node_modules/.bin/prettier --check gui/src/forms/AddModelForm.tsx gui/src/forms/AddModelForm.dynamicFetchRace.test.tsx --ignore-path .gitignore --ignore-path .prettierignore Not-tested: Full GUI/e2e test matrix; provider-specific dynamic fetches against live APIs --- .../AddModelForm.dynamicFetchRace.test.tsx | 26 +++++++++++++++++++ gui/src/forms/AddModelForm.tsx | 4 +-- 2 files changed, 27 insertions(+), 3 deletions(-) diff --git a/gui/src/forms/AddModelForm.dynamicFetchRace.test.tsx b/gui/src/forms/AddModelForm.dynamicFetchRace.test.tsx index f0080ec2970..44acb1d9051 100644 --- a/gui/src/forms/AddModelForm.dynamicFetchRace.test.tsx +++ b/gui/src/forms/AddModelForm.dynamicFetchRace.test.tsx @@ -115,4 +115,30 @@ describe("AddModelForm dynamic fetch race", () => { ); }); }); + + it("clears the previous provider API key before a newly selected keyed provider can fetch models", async () => { + fetchProviderModelsMock.mockResolvedValue([]); + + const { user } = await renderWithProviders( + , + ); + + const apiKeyInput = screen.getByPlaceholderText( + /Enter your OpenAI API key/i, + ); + await user.type(apiKeyInput, "sk-openai-secret"); + expect(apiKeyInput).toHaveValue("sk-openai-secret"); + + await user.click(screen.getByRole("button", { name: "Anthropic" })); + + const anthropicApiKeyInput = screen.getByPlaceholderText( + /Enter your Anthropic API key/i, + ); + expect(anthropicApiKeyInput).toHaveValue(""); + + const fetchButton = screen.getByTitle(/fetch available models/i); + await user.click(fetchButton); + + expect(fetchProviderModelsMock).not.toHaveBeenCalled(); + }); }); diff --git a/gui/src/forms/AddModelForm.tsx b/gui/src/forms/AddModelForm.tsx index bba746e9454..a4383ff69fb 100644 --- a/gui/src/forms/AddModelForm.tsx +++ b/gui/src/forms/AddModelForm.tsx @@ -129,9 +129,7 @@ export function AddModelForm({ useEffect(() => { setSelectedModel(selectedProvider.packages[0]); - if (!selectedProvider.tags?.includes(ModelProviderTags.RequiresApiKey)) { - formMethods.setValue("apiKey", ""); - } + formMethods.setValue("apiKey", ""); }, [selectedProvider]); const requiresSkPrefix = From 9deaa89bc950e0faaaca18444bcea8601c99314e Mon Sep 17 00:00:00 2001 From: JunghwanNA <70629228+shaun0927@users.noreply.github.com> Date: Fri, 17 Apr 2026 13:52:46 +0900 Subject: [PATCH 4/4] Let provider switches start a fresh model fetch immediately cubic identified a real follow-up usability bug in the Add Model race fix: switching providers cleared stale results but left the previous fetch lock active, so the newly selected provider could stay blocked until the old request finished. If that earlier request hung, the new provider could be stuck indefinitely even though stale results were already ignored. This invalidates in-flight fetch generations on provider change, clears the fetch lock for the new provider, and makes `finally` unlock only the currently active fetch. The regression test now proves a pending OpenAI request no longer blocks an immediate Anthropic fetch after switching. Constraint: Keep the follow-up limited to the provider-switch fetch lock identified by cubic Rejected: Leave `isFetchingModels` tied to the old request | new provider remains blocked until stale fetch resolves Rejected: Broaden this PR to unrelated model-list filtering/fetchModels core concerns | widens scope beyond the validated GUI race path Confidence: high Scope-risk: narrow Reversibility: clean Directive: Any future async fetch guard here should invalidate both stale results and stale loading state together Tested: cd gui && npm test -- --run src/forms/AddModelForm.dynamicFetchRace.test.tsx Tested: cd gui && npx prettier --check src/forms/AddModelForm.tsx src/forms/AddModelForm.dynamicFetchRace.test.tsx --ignore-path ../.gitignore --ignore-path ../.prettierignore Not-tested: Full GUI test suite; full PR CI rerun; manual VS Code / JetBrains validation --- .../AddModelForm.dynamicFetchRace.test.tsx | 63 +++++++++++++++++++ gui/src/forms/AddModelForm.tsx | 9 ++- 2 files changed, 71 insertions(+), 1 deletion(-) diff --git a/gui/src/forms/AddModelForm.dynamicFetchRace.test.tsx b/gui/src/forms/AddModelForm.dynamicFetchRace.test.tsx index 44acb1d9051..2dc7bc70d8f 100644 --- a/gui/src/forms/AddModelForm.dynamicFetchRace.test.tsx +++ b/gui/src/forms/AddModelForm.dynamicFetchRace.test.tsx @@ -141,4 +141,67 @@ describe("AddModelForm dynamic fetch race", () => { expect(fetchProviderModelsMock).not.toHaveBeenCalled(); }); + + it("releases the previous provider fetch lock so the newly selected provider can fetch immediately", async () => { + const pendingOpenAiFetch = deferred(); + + fetchProviderModelsMock.mockImplementation( + (_messenger: unknown, provider: string) => { + if (provider === "openai") { + return pendingOpenAiFetch.promise; + } + if (provider === "anthropic") { + return Promise.resolve([makeFetchedModel("Anthropic Dynamic Model")]); + } + return Promise.resolve([]); + }, + ); + + const { user } = await renderWithProviders( + , + ); + + await user.type( + screen.getByPlaceholderText(/Enter your OpenAI API key/i), + "sk-openai-secret", + ); + await user.click(screen.getByTitle(/fetch available models/i)); + + await user.click(screen.getByRole("button", { name: "Anthropic" })); + + const anthropicApiKeyInput = screen.getByPlaceholderText( + /Enter your Anthropic API key/i, + ); + await user.type(anthropicApiKeyInput, "sk-anthropic-secret"); + + const fetchButton = screen.getByTitle(/fetch available models/i); + expect(fetchButton).not.toBeDisabled(); + await user.click(fetchButton); + + await waitFor(() => { + expect(fetchProviderModelsMock).toHaveBeenCalledWith( + expect.anything(), + "anthropic", + "sk-anthropic-secret", + undefined, + ); + expect(screen.getByTestId("model-listbox")).toHaveTextContent( + "Anthropic Dynamic Model", + ); + }); + + await act(async () => { + pendingOpenAiFetch.resolve([makeFetchedModel("OpenAI Dynamic Model")]); + await pendingOpenAiFetch.promise; + }); + + await waitFor(() => { + expect(screen.getByTestId("model-listbox")).toHaveTextContent( + "Anthropic Dynamic Model", + ); + expect(screen.getByTestId("model-listbox")).not.toHaveTextContent( + "OpenAI Dynamic Model", + ); + }); + }); }); diff --git a/gui/src/forms/AddModelForm.tsx b/gui/src/forms/AddModelForm.tsx index a4383ff69fb..6007cb43a6d 100644 --- a/gui/src/forms/AddModelForm.tsx +++ b/gui/src/forms/AddModelForm.tsx @@ -50,6 +50,7 @@ export function AddModelForm({ ); const [isFetchingModels, setIsFetchingModels] = useState(false); const selectedProviderRef = useRef(selectedProvider.provider); + const fetchGenerationRef = useRef(0); useEffect(() => { void initializeDynamicModels(ideMessenger); @@ -57,6 +58,8 @@ export function AddModelForm({ useEffect(() => { setFetchedModelsList([]); + fetchGenerationRef.current += 1; + setIsFetchingModels(false); }, [selectedProvider]); const handleFetchModels = useCallback(async () => { @@ -65,6 +68,8 @@ export function AddModelForm({ if (!apiKey) return; const providerAtFetchTime = selectedProvider.provider; + const fetchGeneration = fetchGenerationRef.current + 1; + fetchGenerationRef.current = fetchGeneration; setIsFetchingModels(true); try { const models = await fetchProviderModels( @@ -79,7 +84,9 @@ export function AddModelForm({ } catch (error) { console.error("Failed to fetch models:", error); } finally { - setIsFetchingModels(false); + if (fetchGenerationRef.current === fetchGeneration) { + setIsFetchingModels(false); + } } }, [ideMessenger, selectedProvider, formMethods]);