diff --git a/src/frontend/src/components/core/parameterRenderComponent/components/modelInputComponent/__tests__/ModelInputComponent.test.tsx b/src/frontend/src/components/core/parameterRenderComponent/components/modelInputComponent/__tests__/ModelInputComponent.test.tsx index 220abfeca5..07f75486cc 100644 --- a/src/frontend/src/components/core/parameterRenderComponent/components/modelInputComponent/__tests__/ModelInputComponent.test.tsx +++ b/src/frontend/src/components/core/parameterRenderComponent/components/modelInputComponent/__tests__/ModelInputComponent.test.tsx @@ -171,7 +171,10 @@ const defaultProps: BaseInputProps & ModelInputComponentType = { editNode: false, }; -// Helper to render with QueryClientProvider +// Helper to render with QueryClientProvider. Returns the raw RTL handle plus +// a ``rerenderWithProvider`` wrapper so callers can rerender without losing +// the surrounding ``QueryClientProvider`` (rerender replaces the root JSX — +// dropping the wrapper would force a remount and reset component state). const renderWithQueryClient = (component: React.ReactElement) => { const queryClient = new QueryClient({ defaultOptions: { @@ -179,9 +182,13 @@ const renderWithQueryClient = (component: React.ReactElement) => { mutations: { retry: false }, }, }); - return render( - {component}, + const wrap = (node: React.ReactElement) => ( + {node} ); + const result = render(wrap(component)); + const rerenderWithProvider = (node: React.ReactElement) => + result.rerender(wrap(node)); + return { ...result, rerenderWithProvider }; }; describe("ModelInputComponent", () => { @@ -190,14 +197,15 @@ describe("ModelInputComponent", () => { }); describe("Rendering", () => { - it("should render disabled combobox when no options are provided", () => { + it("renders the Setup Provider CTA when no models are enabled", () => { renderWithQueryClient( , ); - const combobox = screen.getByRole("combobox"); - expect(combobox).toBeInTheDocument(); - expect(combobox).toBeDisabled(); + // No combobox — the trigger swaps for the Setup Provider button when + // there's nothing to pick from. + expect(screen.queryByRole("combobox")).not.toBeInTheDocument(); + expect(screen.getByText("Setup Provider")).toBeInTheDocument(); }); it("should render the model selector when options are available", () => { @@ -323,6 +331,80 @@ describe("ModelInputComponent", () => { expect(screen.getByTestId("model-provider-modal")).toBeInTheDocument(); }); }); + + it("keeps loading state until both providers and enabled-models refetches settle", async () => { + // The post-close refresh invalidates both ``useGetModelProviders`` AND + // ``useGetEnabledModels``. The component must wait for BOTH to finish + // before clearing the loading state — otherwise ``groupedOptions`` + // renders against a stale ``enabledModelsData`` cache and disabled + // models briefly leak back into the dropdown after the user closes + // the provider modal. + let providersFetching = true; + let enabledFetching = true; + + const mockedProviders = useGetModelProviders as jest.MockedFunction< + typeof useGetModelProviders + >; + const mockedEnabled = useGetEnabledModels as jest.MockedFunction< + typeof useGetEnabledModels + >; + + mockedProviders.mockImplementation( + () => + ({ + data: mockProvidersData, + isLoading: false, + isFetching: providersFetching, + }) as unknown as ReturnType, + ); + mockedEnabled.mockImplementation( + () => + ({ + data: { enabled_models: {} }, + isLoading: false, + isFetching: enabledFetching, + }) as unknown as ReturnType, + ); + + const user = userEvent.setup(); + const { rerenderWithProvider } = renderWithQueryClient( + , + ); + + // Open the dropdown then the provider manager dialog + const trigger = screen.getByRole("combobox"); + await user.click(trigger); + await waitFor(() => { + expect( + screen.getByTestId("manage-model-providers"), + ).toBeInTheDocument(); + }); + await user.click(screen.getByTestId("manage-model-providers")); + await waitFor(() => { + expect(screen.getByTestId("model-provider-modal")).toBeInTheDocument(); + }); + + // Close the modal — this sets isRefreshingAfterClose=true and the + // loading button replaces the dropdown. + await user.click(screen.getByTestId("close-provider-modal")); + await waitFor(() => { + expect(screen.getByText("Loading models")).toBeInTheDocument(); + }); + + // Providers refetch completes FIRST. With the fix, loading persists + // because the enabled-models refetch is still in flight. + providersFetching = false; + rerenderWithProvider(); + await new Promise((r) => setTimeout(r, 30)); + expect(screen.getByText("Loading models")).toBeInTheDocument(); + + // Enabled-models refetch completes. Loading state clears. + enabledFetching = false; + rerenderWithProvider(); + await waitFor(() => { + expect(screen.queryByText("Loading models")).not.toBeInTheDocument(); + }); + }); }); describe("Footer Buttons", () => { @@ -602,117 +684,88 @@ describe("ModelInputComponent", () => { expect(screen.getByRole("combobox")).not.toBeDisabled(); }); - it("keeps a saved value whose model isn't enabled locally and renders the Configure wrench", async () => { - // The backend's update_model_options_in_build_config injects the saved - // value into options tagged with `not_enabled_locally: true` whenever - // it isn't in the user's enabled list. The frontend must: - // 1. NOT auto-reset the saved value. - // 2. Keep the option visible/selectable in the dropdown. - // 3. Render the Configure wrench next to the trigger. - mockedUseGetEnabledModels.mockReturnValue({ - data: { - enabled_models: { - OpenAI: { "gpt-4": true, "gpt-3.5-turbo": true }, - }, - }, - isLoading: false, - }); - - const handleOnNewValue = jest.fn(); - const savedValue = [ - { - id: "ibm/granite-3", - name: "ibm/granite-3", - icon: "IBMWatsonx", - provider: "IBM watsonx.ai", - metadata: { not_enabled_locally: true }, - }, - ]; - // Backend-style options: the saved value is injected into options with - // the sticky flag so the client can render it. - const optionsWithSticky = [ - ...mockOptions, - { - id: "ibm/granite-3", - name: "ibm/granite-3", - icon: "IBMWatsonx", - provider: "IBM watsonx.ai", - metadata: { not_enabled_locally: true }, - }, - ]; - - renderWithQueryClient( - , - ); - - // Value must not be reset by the auto-select effect. - expect(handleOnNewValue).not.toHaveBeenCalled(); - - // Saved model remains visible in the trigger label. - await waitFor(() => { - expect(screen.getByText("ibm/granite-3")).toBeInTheDocument(); - }); - - // Configure wrench is rendered next to the trigger. - expect( - screen.getByTestId(`${defaultProps.id}-configure`), - ).toBeInTheDocument(); - }); - - it("opens the provider manager when the Configure wrench is clicked", async () => { + it("auto-selects an available model when the saved value is globally disabled", async () => { + // The user previously chose ibm/granite-3 but that provider's models + // are no longer enabled. Some other models (gpt-4) ARE enabled, so the + // component must drop the stale selection and align the stored value + // with the first available option rather than visually showing a model + // the dropdown doesn't contain. mockedUseGetEnabledModels.mockReturnValue({ data: { enabled_models: { OpenAI: { "gpt-4": true } } }, isLoading: false, }); + const handleOnNewValue = jest.fn(); const savedValue = [ { id: "ibm/granite-3", name: "ibm/granite-3", icon: "IBMWatsonx", provider: "IBM watsonx.ai", - metadata: { not_enabled_locally: true }, - }, - ]; - const optionsWithSticky = [ - ...mockOptions, - { - id: "ibm/granite-3", - name: "ibm/granite-3", - icon: "IBMWatsonx", - provider: "IBM watsonx.ai", - metadata: { not_enabled_locally: true }, + metadata: {}, }, ]; - const handleOnNewValue = jest.fn(); - const user = userEvent.setup(); renderWithQueryClient( , ); - const wrench = await screen.findByTestId(`${defaultProps.id}-configure`); - await user.click(wrench); - + // Auto-select fires because the saved name isn't in flatOptions. await waitFor(() => { - expect(screen.getByTestId("model-provider-modal")).toBeInTheDocument(); + expect(handleOnNewValue).toHaveBeenCalled(); }); - // Clicking the wrench must not mutate the saved value. - expect(handleOnNewValue).not.toHaveBeenCalled(); + const newValue = handleOnNewValue.mock.calls[0][0].value; + expect(newValue[0].name).toBe("gpt-4"); + + // The stale model name must NOT be rendered anywhere. + expect(screen.queryByText("ibm/granite-3")).not.toBeInTheDocument(); }); - it("does not render Configure when the selected model isn't flagged", () => { - // Baseline: a normal enabled model must not surface the wrench. + it("renders the Setup Provider CTA when a provider is configured but all its models are disabled", () => { + // OpenAI is configured (is_configured/is_enabled true on the provider) + // but every model is explicitly disabled in enabled_models. There's + // nothing for the user to pick from, so we must show the Setup + // Provider CTA — same UX as the never-configured case. + mockedUseGetModelProviders.mockReturnValue({ + data: [ + { + provider: "OpenAI", + is_enabled: true, + is_configured: true, + icon: "OpenAI", + models: [ + { model_name: "gpt-4", metadata: { model_type: "llm" } }, + { model_name: "gpt-3.5-turbo", metadata: { model_type: "llm" } }, + ], + }, + ], + isLoading: false, + }); + mockedUseGetEnabledModels.mockReturnValue({ + data: { + enabled_models: { + OpenAI: { "gpt-4": false, "gpt-3.5-turbo": false }, + }, + }, + isLoading: false, + }); + + renderWithQueryClient( + , + ); + + expect(screen.queryByRole("combobox")).not.toBeInTheDocument(); + expect(screen.getByText("Setup Provider")).toBeInTheDocument(); + }); + + it("never renders the Configure wrench affordance", () => { + // The sticky-default UX was removed: there's no per-trigger affordance + // for a disabled-but-saved model. The dropdown is the single source + // of truth. mockedUseGetEnabledModels.mockReturnValue({ data: { enabled_models: { OpenAI: { "gpt-4": true } } }, isLoading: false, diff --git a/src/frontend/src/components/core/parameterRenderComponent/components/modelInputComponent/index.tsx b/src/frontend/src/components/core/parameterRenderComponent/components/modelInputComponent/index.tsx index 9a3c4de061..b19be72281 100644 --- a/src/frontend/src/components/core/parameterRenderComponent/components/modelInputComponent/index.tsx +++ b/src/frontend/src/components/core/parameterRenderComponent/components/modelInputComponent/index.tsx @@ -80,10 +80,6 @@ export default function ModelInputComponent({ const { refreshAllModelInputs } = useRefreshModelInputs(); - // Ref to track if we've already processed the empty options state - // prevents infinite loop when no models are available - const hasProcessedEmptyRef = useRef(false); - const postTemplateValue = usePostTemplateValue({ parameterId: "model", nodeId: nodeId || "", @@ -113,17 +109,14 @@ export default function ModelInputComponent({ isLoading: isLoadingProviders, isFetching: isFetchingProviders, } = useGetModelProviders({}); - const { data: enabledModelsData, isLoading: isLoadingEnabledModels } = - useGetEnabledModels(); + const { + data: enabledModelsData, + isLoading: isLoadingEnabledModels, + isFetching: isFetchingEnabledModels, + } = useGetEnabledModels(); const isLoading = isLoadingProviders || isLoadingEnabledModels; - const hasEnabledProviders = useMemo(() => { - return providersData?.some( - (provider) => provider.is_enabled || provider.is_configured, - ); - }, [providersData]); - // Groups models by their provider name for sectioned display in dropdown. // Filters out models from disabled providers AND disabled models, then // augments with any enabled models from `providersData` that weren't in the @@ -137,20 +130,12 @@ export default function ModelInputComponent({ if (option.metadata?.is_disabled_provider) continue; const provider = option.provider || "Unknown"; - // Backend sticky-default: options tagged with `not_enabled_locally` are - // the user's saved selection injected into the options list by the - // unified-models build_config helper even though they aren't in the - // enabled list. They must always pass the client-side filter so the - // selection stays visible and selectable in the dropdown. - const isStickyNotEnabled = option.metadata?.not_enabled_locally === true; - - // Filter against client-side enabled models data. This is the source of - // truth for what the current user has enabled — stale `options` saved in - // an imported flow may include models from providers the current user - // hasn't enabled (e.g. WatsonX). When the provider is tracked in - // enabled_models, the model must be explicitly enabled (=== true); a - // `false` or missing entry means the model should be hidden. - if (!isStickyNotEnabled && enabledModelsData?.enabled_models) { + // Filter against client-side enabled models data. The user's + // current enabled list is the single source of truth: a model that + // isn't explicitly enabled (=== true) is hidden, even if it's the + // node's saved selection. There is no sticky-default carve-out — + // a globally-disabled model never appears in the dropdown. + if (enabledModelsData?.enabled_models) { const providerModels = enabledModelsData.enabled_models[provider]; if (providerModels && providerModels[option.name] !== true) { continue; @@ -216,33 +201,17 @@ export default function ModelInputComponent({ } } - // Zero-provider import fallback: if the user has no providers configured - // locally but an imported flow carries a saved model, inject that saved - // selection as the only dropdown entry so the trigger renders the model - // (with the Configure wrench) instead of the "Setup Provider" button. - // Without this, ``flatOptions`` would be empty and ``ModelTrigger`` would - // swap the dropdown for the setup CTA, visually losing the imported - // selection. - const hasAnyGrouped = Object.keys(grouped).length > 0; - const savedValue = value?.[0]; - if (!hasAnyGrouped && savedValue?.name) { - const providerName = savedValue.provider || "Unknown"; - grouped[providerName] = [ - { - ...(savedValue.id && { id: savedValue.id }), - name: savedValue.name, - icon: savedValue.icon || "Bot", - provider: providerName, - metadata: { - ...(savedValue.metadata ?? {}), - not_enabled_locally: true, - }, - } as ModelOption, - ]; - } - return grouped; - }, [options, enabledModelsData, providersData, modelType, value]); + }, [options, enabledModelsData, providersData, modelType]); + + // True iff at least one model of this component's type is enabled across + // any provider. Drives the Setup Provider CTA — a provider that is + // configured but has all its models disabled is treated the same as a + // provider that hasn't been configured yet. + const hasEnabledProviders = useMemo( + () => Object.values(groupedOptions).some((models) => models.length > 0), + [groupedOptions], + ); // Flattened array of all enabled options for efficient lookups by name const flatOptions = useMemo( @@ -250,9 +219,12 @@ export default function ModelInputComponent({ [groupedOptions], ); - // Derive the currently selected model from the value prop + // Derive the currently selected model from the value prop. If the saved + // value isn't in the current ``flatOptions`` (typically because the + // model was globally disabled), it's treated as if no model is selected + // — the trigger renders the first available option as the visual + // selection and the effect below realigns the stored ``value`` to match. const selectedModel = useMemo(() => { - // If we're in connection mode, show the connection option as selected if (isConnectionMode) { return { name: @@ -264,56 +236,26 @@ export default function ModelInputComponent({ } const currentName = value?.[0]?.name; - if (!currentName) { - // Logic to auto-select the first model if none is selected - // We only do this check if we have options available - if (flatOptions.length > 0 && !hasProcessedEmptyRef.current) { - // If we haven't processed empty state yet, we render the first one - return flatOptions[0]; - } - return null; - } - - const match = flatOptions.find((option) => option.name === currentName); - if (match) return match; - - // Saved name isn't in the filtered options list — typically because the - // flow was imported via drag-drop (no backend sticky-default round-trip) - // or because an outdated component was upgraded and the fresh template - // lacks the sticky-default metadata. Preserve the saved selection in the - // trigger so it doesn't visually "snap" to the user's first enabled - // model. The wrench affordance in the dropdown handles configuration. - const saved = value?.[0]; - if (saved) { - return { - ...(saved.id && { id: saved.id }), - name: saved.name, - icon: saved.icon || "Bot", - provider: saved.provider || "Unknown", - metadata: { - ...(saved.metadata ?? {}), - not_enabled_locally: true, - }, - } as SelectedModel; + if (currentName) { + const match = flatOptions.find((option) => option.name === currentName); + if (match) return match; } return flatOptions.length > 0 ? flatOptions[0] : null; }, [value, flatOptions, isConnectionMode, externalOptions]); + // Auto-select the first available option whenever the stored ``value`` + // doesn't point at a currently-available model — including the case + // where the previously-selected model was globally disabled. Without + // this, the trigger would visually show one model while the node's + // saved value pointed at a different (or removed) one. useEffect(() => { if (flatOptions.length === 0 || isConnectionMode) return; - if (hasProcessedEmptyRef.current) return; - const isEmpty = !value || value.length === 0; - // Sticky-default: if the component has a saved value, keep it as-is. The - // backend injects any selection that isn't in the user's enabled list - // back into `options` tagged with `not_enabled_locally`, so the saved - // value remains visible and runnable. Only auto-select the first option - // when there's no saved value at all. - if (!isEmpty) return; + const savedName = value?.[0]?.name; + if (savedName && flatOptions.some((o) => o.name === savedName)) return; const firstOption = flatOptions[0]; - // Construct the new value object const newValue = [ { ...(firstOption.id && { id: firstOption.id }), @@ -324,7 +266,6 @@ export default function ModelInputComponent({ }, ]; handleOnNewValue({ value: newValue }); - hasProcessedEmptyRef.current = true; }, [flatOptions, value, handleOnNewValue, isConnectionMode]); /** @@ -402,22 +343,27 @@ export default function ModelInputComponent({ setIsRefreshingAfterClose(true); }, []); - // Clear the refreshing indicator after the providers query completes a full - // refetch cycle (isFetchingProviders: false → true → false). We track whether - // we've seen the fetch start so we don't clear prematurely before the - // invalidation has even been triggered by refreshAllModelInputs. + // Clear the refreshing indicator only after BOTH the providers and the + // enabled-models queries have completed a full refetch cycle (isFetching: + // false → true → false). Watching only ``isFetchingProviders`` clears the + // loading state too early when the enabled-models refetch is slower, + // letting ``groupedOptions`` render against a stale ``enabledModelsData`` + // cache — disabled models would briefly leak back into the dropdown after + // the user closes the provider modal. We track whether we've seen the + // fetch start so we don't clear prematurely before the invalidation has + // even been triggered by refreshAllModelInputs. const hasSeenFetchStartRef = useRef(false); useEffect(() => { if (!isRefreshingAfterClose) { hasSeenFetchStartRef.current = false; return; } - if (isFetchingProviders) { + if (isFetchingProviders || isFetchingEnabledModels) { hasSeenFetchStartRef.current = true; } else if (hasSeenFetchStartRef.current) { setIsRefreshingAfterClose(false); } - }, [isRefreshingAfterClose, isFetchingProviders]); + }, [isRefreshingAfterClose, isFetchingProviders, isFetchingEnabledModels]); // Safety timeout: clear loading even if no refetch cycle is detected // (e.g. no model nodes on canvas, or the refresh was a no-op) @@ -521,49 +467,22 @@ export default function ModelInputComponent({ return
{renderLoadingButton()}
; } - // Show a small "Configure" wrench next to the trigger when the currently - // selected model was injected by the backend as a sticky default — i.e. - // the user's saved selection isn't in their current enabled_models list. - // Clicking it jumps straight to the provider manager so the user can - // enable the provider without losing their selection. - const showConfigureAffordance = - selectedModel?.metadata?.not_enabled_locally === true; - // Main render return ( <> -
-
- setOpenManageProvidersDialog(true)} - id={id} - refButton={refButton} - showEmptyState={showEmptyState} - /> -
- {showConfigureAffordance && ( - - )} -
+ setOpenManageProvidersDialog(true)} + id={id} + refButton={refButton} + showEmptyState={showEmptyState} + /> {renderPopoverContent()} diff --git a/src/frontend/src/modals/modelProviderModal/__tests__/useModelToggleQueue.test.tsx b/src/frontend/src/modals/modelProviderModal/__tests__/useModelToggleQueue.test.tsx new file mode 100644 index 0000000000..4ec26be191 --- /dev/null +++ b/src/frontend/src/modals/modelProviderModal/__tests__/useModelToggleQueue.test.tsx @@ -0,0 +1,452 @@ +import { renderHook } from "@testing-library/react"; +import { act } from "react"; +import { useGetEnabledModels } from "@/controllers/API/queries/models/use-get-enabled-models"; +import { useModelToggleQueue } from "../hooks/useModelToggleQueue"; + +// --------------------------------------------------------------------------- +// React Query — capture invocation order on the shared mock so the test can +// assert that ``cancelQueries`` is called BEFORE ``setQueryData`` whenever a +// user toggles a model. The two are wired to a single Jest mock function so +// their relative call order is preserved. +// --------------------------------------------------------------------------- +const recordedCalls: Array<{ method: string; args: unknown[] }> = []; + +const trackingQueryClient = { + cancelQueries: jest.fn((...args: unknown[]) => { + recordedCalls.push({ method: "cancelQueries", args }); + return Promise.resolve(); + }), + setQueryData: jest.fn((...args: unknown[]) => { + recordedCalls.push({ method: "setQueryData", args }); + const updater = args[1] as + | ((prev: unknown) => unknown) + | Record; + if (typeof updater === "function") { + updater({ enabled_models: { OpenAI: { "gpt-4": true } } }); + } + return undefined; + }), + getQueryData: jest.fn(() => ({ + enabled_models: { OpenAI: { "gpt-4": true } }, + })), + invalidateQueries: jest.fn((...args: unknown[]) => { + recordedCalls.push({ method: "invalidateQueries", args }); + return Promise.resolve(); + }), +}; + +jest.mock("@tanstack/react-query", () => ({ + useQueryClient: () => trackingQueryClient, +})); + +// The hook subscribes to ``useGetEnabledModels`` so the re-overlay effect can +// react when a refetch lands. The shared mock starts with the same baseline +// the trackingQueryClient holds. +jest.mock("@/controllers/API/queries/models/use-get-enabled-models", () => ({ + useGetEnabledModels: jest.fn(() => ({ + data: { enabled_models: { OpenAI: { "gpt-4": true } } }, + })), +})); + +// Mutation mock — tests can read the recorded payloads to assert that each +// flush sends ONLY the unsent slice, never re-sending an in-flight overlay, +// and can drive ``onError`` / ``onSettled`` via the captured callbacks. +const mutationCalls: Array<{ + updates: { provider: string; model_id: string; enabled: boolean }[]; +}> = []; +const mutationCallbacks: Array<{ + onError?: (error: unknown) => void; + onSettled?: () => void; +}> = []; + +jest.mock("@/controllers/API/queries/models/use-update-enabled-models", () => ({ + useUpdateEnabledModels: () => ({ + mutate: jest.fn((vars, callbacks) => { + mutationCalls.push(vars); + mutationCallbacks.push(callbacks); + }), + mutateAsync: jest.fn((vars) => { + mutationCalls.push(vars); + return Promise.resolve({ disabled_models: [] }); + }), + }), +})); + +// Debounce stub: capturing the pending function instead of running it +// synchronously lets each test pick when (or whether) the debounced flush +// fires. Tests that exercise the debounced path call ``runDebounced()``; +// tests that exercise the awaitable ``flushPendingChanges`` path don't, so +// ``unsentToggles`` is still populated when the explicit flush runs. +let pendingDebouncedFns: Array<() => void> = []; +const runDebounced = () => { + const fns = pendingDebouncedFns; + pendingDebouncedFns = []; + for (const fn of fns) fn(); +}; +jest.mock("@/hooks/use-debounce", () => ({ + useDebounce: (fn: (...args: unknown[]) => unknown) => { + const wrapped = (..._args: unknown[]) => { + pendingDebouncedFns.push(() => fn()); + }; + (wrapped as unknown as { cancel: () => void }).cancel = jest.fn(() => { + pendingDebouncedFns = []; + }); + return wrapped; + }, +})); + +const mockRefreshAllModelInputs = jest.fn(() => Promise.resolve()); +jest.mock("@/hooks/use-refresh-model-inputs", () => ({ + useRefreshModelInputs: () => ({ + refreshAllModelInputs: mockRefreshAllModelInputs, + }), +})); + +const mockSetErrorData = jest.fn(); +jest.mock("@/stores/alertStore", () => ({ + __esModule: true, + default: (selector: (state: unknown) => unknown) => + selector({ + setSuccessData: jest.fn(), + setErrorData: mockSetErrorData, + }), +})); + +describe("useModelToggleQueue", () => { + beforeEach(() => { + recordedCalls.length = 0; + mutationCalls.length = 0; + mutationCallbacks.length = 0; + pendingDebouncedFns = []; + trackingQueryClient.cancelQueries.mockClear(); + trackingQueryClient.setQueryData.mockClear(); + trackingQueryClient.getQueryData.mockClear(); + trackingQueryClient.invalidateQueries.mockClear(); + mockRefreshAllModelInputs.mockClear(); + mockSetErrorData.mockClear(); + }); + + describe("optimistic update", () => { + it("cancels in-flight useGetEnabledModels refetches before the optimistic cache update", () => { + const { result } = renderHook(() => + useModelToggleQueue({ providerName: "OpenAI" }), + ); + + act(() => { + result.current.handleModelToggle("gpt-4", false); + }); + + expect(trackingQueryClient.cancelQueries).toHaveBeenCalledWith({ + queryKey: ["useGetEnabledModels"], + }); + expect(trackingQueryClient.setQueryData).toHaveBeenCalledWith( + ["useGetEnabledModels"], + expect.any(Function), + ); + + const cancelIdx = recordedCalls.findIndex( + (call) => call.method === "cancelQueries", + ); + const setIdx = recordedCalls.findIndex( + (call) => call.method === "setQueryData", + ); + + expect(cancelIdx).toBeGreaterThanOrEqual(0); + expect(setIdx).toBeGreaterThanOrEqual(0); + expect(cancelIdx).toBeLessThan(setIdx); + }); + + it("no-ops when no provider is selected", () => { + const { result } = renderHook(() => + useModelToggleQueue({ providerName: null }), + ); + + act(() => { + result.current.handleModelToggle("gpt-4", false); + }); + + expect(trackingQueryClient.cancelQueries).not.toHaveBeenCalled(); + expect(trackingQueryClient.setQueryData).not.toHaveBeenCalled(); + }); + }); + + describe("re-overlay effect", () => { + it("re-applies the pending overlay when a refetch surfaces stale data", () => { + const mockedEnabled = useGetEnabledModels as jest.MockedFunction< + typeof useGetEnabledModels + >; + + // Initial render: gpt-4 enabled on the server. + mockedEnabled.mockReturnValue({ + data: { enabled_models: { OpenAI: { "gpt-4": true } } }, + } as unknown as ReturnType); + + const { result, rerender } = renderHook(() => + useModelToggleQueue({ providerName: "OpenAI" }), + ); + + // User toggles gpt-4 off — overlay now holds {gpt-4: false}. + act(() => { + result.current.handleModelToggle("gpt-4", false); + }); + expect(trackingQueryClient.setQueryData).toHaveBeenCalled(); + + // Drain the call log so the re-overlay can be detected specifically. + recordedCalls.length = 0; + trackingQueryClient.setQueryData.mockClear(); + + // Simulate a refetch that lands inside the debounce window: the cache + // reports the still-stale server state (gpt-4: true). + mockedEnabled.mockReturnValue({ + data: { enabled_models: { OpenAI: { "gpt-4": true } } }, + } as unknown as ReturnType); + rerender(); + + // The effect detects drift and re-applies the overlay. + expect(trackingQueryClient.setQueryData).toHaveBeenCalledWith( + ["useGetEnabledModels"], + expect.any(Function), + ); + + const updater = trackingQueryClient.setQueryData.mock.calls[0][1] as ( + old: unknown, + ) => unknown; + const result2 = updater({ + enabled_models: { OpenAI: { "gpt-4": true, "gpt-3.5-turbo": true } }, + }) as { enabled_models: { OpenAI: Record } }; + expect(result2.enabled_models.OpenAI["gpt-4"]).toBe(false); + expect(result2.enabled_models.OpenAI["gpt-3.5-turbo"]).toBe(true); + }); + + it("does not re-overlay when no toggles are pending", () => { + const mockedEnabled = useGetEnabledModels as jest.MockedFunction< + typeof useGetEnabledModels + >; + mockedEnabled.mockReturnValue({ + data: { enabled_models: { OpenAI: { "gpt-4": true } } }, + } as unknown as ReturnType); + + renderHook(() => useModelToggleQueue({ providerName: "OpenAI" })); + + // No toggle was performed — the mount effect must NOT call setQueryData. + expect(trackingQueryClient.setQueryData).not.toHaveBeenCalled(); + }); + }); + + describe("send buffer", () => { + it("does not resend in-flight toggles when a new toggle is flushed", () => { + const { result } = renderHook(() => + useModelToggleQueue({ providerName: "OpenAI" }), + ); + + // Toggle A → debounce schedules a flush; drive it to send mutation A. + act(() => { + result.current.handleModelToggle("gpt-4", false); + runDebounced(); + }); + expect(mutationCalls).toHaveLength(1); + expect(mutationCalls[0].updates).toEqual([ + { provider: "OpenAI", model_id: "gpt-4", enabled: false }, + ]); + + // Toggle B while A's mutation is still in flight (onSettled hasn't + // fired). The next flush MUST send ONLY B — re-sending A would be a + // duplicate request with non-deterministic ordering vs the original. + act(() => { + result.current.handleModelToggle("gpt-3.5-turbo", false); + runDebounced(); + }); + expect(mutationCalls).toHaveLength(2); + expect(mutationCalls[1].updates).toEqual([ + { provider: "OpenAI", model_id: "gpt-3.5-turbo", enabled: false }, + ]); + }); + + it("re-sends a model when the user re-toggles it after the previous flush fired", () => { + const { result } = renderHook(() => + useModelToggleQueue({ providerName: "OpenAI" }), + ); + + // Toggle A → false. Drive the debounce. + act(() => { + result.current.handleModelToggle("gpt-4", false); + runDebounced(); + }); + expect(mutationCalls).toHaveLength(1); + expect(mutationCalls[0].updates).toEqual([ + { provider: "OpenAI", model_id: "gpt-4", enabled: false }, + ]); + + // User re-toggles A → true before the first mutation settles. The + // re-toggle is a fresh intent and must be sent. + act(() => { + result.current.handleModelToggle("gpt-4", true); + runDebounced(); + }); + expect(mutationCalls).toHaveLength(2); + expect(mutationCalls[1].updates).toEqual([ + { provider: "OpenAI", model_id: "gpt-4", enabled: true }, + ]); + }); + }); + + describe("error path", () => { + it("rolls back to previousData when the toggle mutation fails", () => { + const { result } = renderHook(() => + useModelToggleQueue({ providerName: "OpenAI" }), + ); + + act(() => { + result.current.handleModelToggle("gpt-4", false); + runDebounced(); + }); + expect(mutationCallbacks).toHaveLength(1); + + // The first setQueryData is the optimistic update from handleModelToggle. + // The second will be the rollback we expect on error. + const setQueryDataCallsBefore = + trackingQueryClient.setQueryData.mock.calls.length; + + act(() => { + mutationCallbacks[0].onError?.(new Error("backend down")); + }); + + // Rollback restored ``previousData`` via setQueryData with the snapshot + // (NOT a function updater). + const newCalls = trackingQueryClient.setQueryData.mock.calls.slice( + setQueryDataCallsBefore, + ); + const rollbackCall = newCalls.find( + ([_, arg]) => typeof arg !== "function", + ); + expect(rollbackCall).toBeDefined(); + expect(rollbackCall?.[0]).toEqual(["useGetEnabledModels"]); + expect(rollbackCall?.[1]).toEqual({ + enabled_models: { OpenAI: { "gpt-4": true } }, + }); + + // User sees an error toast. + expect(mockSetErrorData).toHaveBeenCalledWith( + expect.objectContaining({ title: "Error updating model status" }), + ); + }); + + it("does not re-apply the overlay after a failed mutation drains it", () => { + // The drain-before-rollback ordering inside ``rollbackToggleBatch`` is + // load-bearing. Without it, the re-overlay effect (triggered by the + // setQueryData rollback) would re-apply the stale overlay onto the + // just-rolled-back cache and silently undo the rollback. + const mockedEnabled = useGetEnabledModels as jest.MockedFunction< + typeof useGetEnabledModels + >; + mockedEnabled.mockReturnValue({ + data: { enabled_models: { OpenAI: { "gpt-4": true } } }, + } as unknown as ReturnType); + + const { result, rerender } = renderHook(() => + useModelToggleQueue({ providerName: "OpenAI" }), + ); + + act(() => { + result.current.handleModelToggle("gpt-4", false); + runDebounced(); + }); + + // Mutation fails — overlay is drained, cache reverts. + act(() => { + mutationCallbacks[0].onError?.(new Error("backend down")); + }); + + // Subsequent refetch surfaces the (now-correct) server state. + trackingQueryClient.setQueryData.mockClear(); + mockedEnabled.mockReturnValue({ + data: { enabled_models: { OpenAI: { "gpt-4": true } } }, + } as unknown as ReturnType); + rerender(); + + // The re-overlay effect must NOT re-apply the failed toggle. The + // overlay has been drained, so there's nothing to re-apply. + expect(trackingQueryClient.setQueryData).not.toHaveBeenCalled(); + }); + + it("preserves a mid-flight re-toggle when the original mutation fails", () => { + // Scenario: user toggles A→false, then re-toggles A→true while the + // first mutation is in flight. The first mutation then fails. The + // user's latest intent (A=true) must survive — both in the overlay + // (so the re-overlay effect protects it) and in the unsent buffer + // (so the next flush sends it). + const { result } = renderHook(() => + useModelToggleQueue({ providerName: "OpenAI" }), + ); + + act(() => { + result.current.handleModelToggle("gpt-4", false); + runDebounced(); + }); + expect(mutationCalls).toHaveLength(1); + expect(mutationCalls[0].updates[0].enabled).toBe(false); + + // Re-toggle while first mutation is in flight. + act(() => { + result.current.handleModelToggle("gpt-4", true); + runDebounced(); + }); + expect(mutationCalls).toHaveLength(2); + expect(mutationCalls[1].updates[0].enabled).toBe(true); + + // First mutation (A=false) fails. ``clearSentOverlay`` must NOT drop + // the A entry from the overlay, because the current overlay value + // (true) doesn't match what we sent (false) — the user re-toggled. + // The second mutation's overlay entry stays protected. + mutationCalls.length = 0; + act(() => { + mutationCallbacks[0].onError?.(new Error("backend down")); + }); + + // Trigger a fresh flush by toggling another key — if the overlay had + // been cleared in error, this flush would NOT include any prior + // intent. We verify the overlay survived by re-toggling A back to + // its in-flight value: if the overlay still has A=true, this is a + // no-op intent that doesn't appear in unsent. If the overlay was + // cleared, this re-toggle would be a fresh intent. + // + // Simpler check: the second mutation (A=true) is still in flight + // and must complete cleanly. The user's last intent is preserved + // by the overlay until that second mutation settles. + expect(mutationCallbacks).toHaveLength(2); + act(() => { + mutationCallbacks[1].onSettled?.(); + }); + // No additional rollback calls happened — the second mutation's + // overlay entry was preserved through the first failure. + expect(mockSetErrorData).toHaveBeenCalledTimes(1); + }); + }); + + describe("flushPendingChanges", () => { + it("invalidates useGetEnabledModels on success without relying on the caller", async () => { + const { result } = renderHook(() => + useModelToggleQueue({ providerName: "OpenAI" }), + ); + + // Toggle but DON'T run the debounced flush — leave the entry in + // unsentToggles for the awaitable close handler to consume. + act(() => { + result.current.handleModelToggle("gpt-4", false); + }); + + await act(async () => { + await result.current.flushPendingChanges(); + }); + + // The async flush must invalidate the enabled-models cache itself — + // callers should not have to remember to invalidate downstream. + expect(trackingQueryClient.invalidateQueries).toHaveBeenCalledWith({ + queryKey: ["useGetEnabledModels"], + }); + expect(trackingQueryClient.invalidateQueries).toHaveBeenCalledWith({ + queryKey: ["useGetModelProviders"], + }); + }); + }); +}); diff --git a/src/frontend/src/modals/modelProviderModal/hooks/useModelToggleQueue.ts b/src/frontend/src/modals/modelProviderModal/hooks/useModelToggleQueue.ts new file mode 100644 index 0000000000..792f66ad4b --- /dev/null +++ b/src/frontend/src/modals/modelProviderModal/hooks/useModelToggleQueue.ts @@ -0,0 +1,292 @@ +import { useQueryClient } from "@tanstack/react-query"; +import { useCallback, useEffect, useRef } from "react"; +import { + EnabledModelsResponse, + useGetEnabledModels, +} from "@/controllers/API/queries/models/use-get-enabled-models"; +import { useUpdateEnabledModels } from "@/controllers/API/queries/models/use-update-enabled-models"; +import { useDebounce } from "@/hooks/use-debounce"; +import { useRefreshModelInputs } from "@/hooks/use-refresh-model-inputs"; +import useAlertStore from "@/stores/alertStore"; + +// Extracted from useProviderConfiguration.ts to keep the toggle-queue +// concerns (overlay buffer, unsent buffer, debounced flush, awaitable +// flush, re-overlay effect, optimistic cache management) in a single +// focused module. The parent hook now owns only variable CRUD and +// provider lifecycle — the two responsibilities no longer share a file. + +const getErrorMessage = (error: unknown): string | undefined => { + const e = error as { + response?: { data?: { detail?: string } }; + message?: string; + }; + return e?.response?.data?.detail || e?.message; +}; + +export interface UseModelToggleQueueOptions { + /** + * Provider whose models the user is toggling. ``null`` short-circuits all + * handlers — useful while the modal is still resolving the selection. + */ + providerName: string | null | undefined; +} + +export interface UseModelToggleQueueReturn { + handleModelToggle: (modelName: string, enabled: boolean) => void; + flushPendingChanges: () => Promise; +} + +interface ToggleBatch { + updates: { provider: string; model_id: string; enabled: boolean }[]; + previousData: EnabledModelsResponse | undefined; + togglesToSend: Record; +} + +/** + * Coordinated optimistic-update queue for model enable/disable toggles. + * + * Two refs back the queue, each with a single responsibility: + * + * - ``overlayToggles`` — the union of every toggle the user has made + * whose change has not yet been confirmed by the server. The + * re-overlay effect re-applies it whenever ``useGetEnabledModels`` + * emits new data, so any refetch which lands inside the debounce or + * in-flight-mutation window can't overwrite the optimistic cache + * with stale server state. Entries are drained per-key on + * ``onSettled``/``onError`` — but only when the entry still matches + * the value we sent (a user re-toggle mid-flight becomes a fresh + * intent and must survive the clear). + * + * - ``unsentToggles`` — the strict subset that has NOT been sent in a + * mutation yet (or was re-toggled since the last send). It's drained + * immediately at flush time so subsequent flushes don't resend the + * same payload — without this split, mutation A's in-flight overlay + * entries would be snapshotted into mutation B's payload, producing + * duplicate requests with non-deterministic success/failure ordering. + */ +export const useModelToggleQueue = ({ + providerName, +}: UseModelToggleQueueOptions): UseModelToggleQueueReturn => { + const queryClient = useQueryClient(); + const setErrorData = useAlertStore((state) => state.setErrorData); + const { mutate: updateEnabledModels, mutateAsync: updateEnabledModelsAsync } = + useUpdateEnabledModels({ retry: 0 }); + const { refreshAllModelInputs } = useRefreshModelInputs(); + const { data: enabledModelsData } = useGetEnabledModels(); + + const overlayToggles = useRef>({}); + const unsentToggles = useRef>({}); + const fallbackModelData = useRef( + undefined, + ); + + // After a mutation settles, remove its entries from the overlay — but + // only when the current overlay value still matches what we sent. A + // mismatch means the user re-toggled the same model mid-flight; that + // entry already sits in ``unsentToggles`` for the next flush and must + // not be dropped from the overlay until its own mutation settles. + const clearSentOverlay = useCallback((sent: Record) => { + for (const [key, value] of Object.entries(sent)) { + if (overlayToggles.current[key] === value) { + delete overlayToggles.current[key]; + } + } + if (Object.keys(overlayToggles.current).length === 0) { + fallbackModelData.current = undefined; + } + }, []); + + // Shared flush prelude: builds the mutation payload, snapshots the + // pre-toggle cache for rollback, and drains ``unsentToggles`` so a + // follow-up flush triggered by a new user toggle cannot resend what we + // just sent. Returns null when there's nothing to do so callers can + // bail symmetrically. + const buildAndConsumeToggleBatch = useCallback((): ToggleBatch | null => { + if (!providerName) return null; + + const togglesToSend = { ...unsentToggles.current }; + if (Object.keys(togglesToSend).length === 0) return null; + + const updates = Object.entries(togglesToSend).map( + ([modelName, enabled]) => ({ + provider: providerName, + model_id: modelName, + enabled, + }), + ); + + const previousData = fallbackModelData.current; + unsentToggles.current = {}; + + return { updates, previousData, togglesToSend }; + }, [providerName]); + + // Shared error-path: drain the overlay BEFORE restoring previousData so + // the re-overlay effect (triggered by setQueryData below) can't re-apply + // a stale overlay over the rollback we just performed. + const rollbackToggleBatch = useCallback( + ( + togglesToSend: Record, + previousData: EnabledModelsResponse | undefined, + error: unknown, + ) => { + clearSentOverlay(togglesToSend); + if (previousData) { + queryClient.setQueryData(["useGetEnabledModels"], previousData); + } + setErrorData({ + title: "Error updating model status", + list: [getErrorMessage(error) || "Failed to update model status"], + }); + }, + [clearSentOverlay, queryClient, setErrorData], + ); + + const flushModelToggles = useDebounce(() => { + const batch = buildAndConsumeToggleBatch(); + if (!batch) return; + const { updates, previousData, togglesToSend } = batch; + + updateEnabledModels( + { updates }, + { + onError: (error: unknown) => { + rollbackToggleBatch(togglesToSend, previousData, error); + }, + onSettled: () => { + clearSentOverlay(togglesToSend); + queryClient.invalidateQueries({ + queryKey: ["useGetEnabledModels"], + }); + queryClient.invalidateQueries({ + queryKey: ["useGetModelProviders"], + }); + refreshAllModelInputs({ silent: true }); + }, + }, + ); + }, 1000); + + const flushPendingChanges = useCallback(async () => { + // Cancel the pending debounce timer — we'll send the toggles directly + flushModelToggles.cancel(); + + const batch = buildAndConsumeToggleBatch(); + if (!batch) return; + const { updates, previousData, togglesToSend } = batch; + + try { + await updateEnabledModelsAsync({ updates }); + clearSentOverlay(togglesToSend); + // Invalidate the affected queries inline so callers don't need to + // bolt this on. The modal's onClose still triggers + // ``refreshAllModelInputs`` afterwards to repopulate per-node + // template options, but ``useGetEnabledModels`` consumers no longer + // depend on the caller for cache freshness. + queryClient.invalidateQueries({ queryKey: ["useGetEnabledModels"] }); + queryClient.invalidateQueries({ queryKey: ["useGetModelProviders"] }); + } catch (error: unknown) { + rollbackToggleBatch(togglesToSend, previousData, error); + } + }, [ + flushModelToggles, + buildAndConsumeToggleBatch, + updateEnabledModelsAsync, + clearSentOverlay, + rollbackToggleBatch, + queryClient, + ]); + + const handleModelToggle = useCallback( + (modelName: string, enabled: boolean) => { + if (!providerName) return; + + // Cancel any in-flight refetch of useGetEnabledModels so its (stale) + // result cannot overwrite the optimistic cache update below. The + // re-overlay effect handles refetches that start AFTER this point; + // ``cancelQueries`` covers the ones already in flight at click time. + void queryClient.cancelQueries({ queryKey: ["useGetEnabledModels"] }); + + if (Object.keys(overlayToggles.current).length === 0) { + fallbackModelData.current = + queryClient.getQueryData([ + "useGetEnabledModels", + ]); + } + + queryClient.setQueryData( + ["useGetEnabledModels"], + (old) => { + if (!old) return old; + return { + ...old, + enabled_models: { + ...old.enabled_models, + [providerName]: { + ...old.enabled_models[providerName], + [modelName]: enabled, + }, + }, + }; + }, + ); + + // Track in BOTH buffers: overlay for UI protection across refetches, + // unsent for the next flush's payload. + overlayToggles.current[modelName] = enabled; + unsentToggles.current[modelName] = enabled; + flushModelToggles(); + }, + [providerName, queryClient, flushModelToggles], + ); + + // Re-overlay effect — protects the pending-toggle window in its entirety, + // not just the instant of the click. Any refetch (window focus, remount, + // reconnect, or a stale-time expiry) that lands while ``overlayToggles`` + // has entries will surface the server's pre-toggle state into + // ``enabledModelsData``; this effect detects the drift and re-applies the + // pending overlay so the Switch tracks the user's intent through the + // entire debounce + in-flight window. Once ``clearSentOverlay`` drains + // the entry on settle, the next data emission is a no-op. + useEffect(() => { + if (!providerName) return; + if (!enabledModelsData) return; + const overlay = overlayToggles.current; + if (Object.keys(overlay).length === 0) return; + + const current = enabledModelsData.enabled_models[providerName] ?? {}; + // Loop guard: the ``setQueryData`` below re-emits ``enabledModelsData`` + // and re-runs this effect; ``drifted`` must return false on the second + // pass for the recursion to terminate. Don't replace this with an + // unconditional re-apply — the second invocation finds the overlay + // already applied, ``current[model] === enabled`` for every entry, and + // bails. Any future refactor that removes the drift check must + // introduce an equivalent termination condition. + const drifted = Object.entries(overlay).some( + ([model, enabled]) => current[model] !== enabled, + ); + if (!drifted) return; + + queryClient.setQueryData( + ["useGetEnabledModels"], + (old) => { + if (!old) return old; + return { + ...old, + enabled_models: { + ...old.enabled_models, + [providerName]: { + ...(old.enabled_models[providerName] ?? {}), + ...overlay, + }, + }, + }; + }, + ); + }, [enabledModelsData, providerName, queryClient]); + + return { + handleModelToggle, + flushPendingChanges, + }; +}; diff --git a/src/frontend/src/modals/modelProviderModal/hooks/useProviderConfiguration.ts b/src/frontend/src/modals/modelProviderModal/hooks/useProviderConfiguration.ts index fd7cde9dfe..98f9c59a66 100644 --- a/src/frontend/src/modals/modelProviderModal/hooks/useProviderConfiguration.ts +++ b/src/frontend/src/modals/modelProviderModal/hooks/useProviderConfiguration.ts @@ -5,10 +5,8 @@ import { ProviderVariable, VARIABLE_CATEGORY, } from "@/constants/providerConstants"; -import { EnabledModelsResponse } from "@/controllers/API/queries/models/use-get-enabled-models"; import { useGetModelProviders } from "@/controllers/API/queries/models/use-get-model-providers"; import { useGetProviderVariables } from "@/controllers/API/queries/models/use-get-provider-variables"; -import { useUpdateEnabledModels } from "@/controllers/API/queries/models/use-update-enabled-models"; import { useValidateProvider } from "@/controllers/API/queries/models/use-validate-provider"; import { useDeleteGlobalVariables, @@ -16,14 +14,25 @@ import { usePatchGlobalVariables, usePostGlobalVariables, } from "@/controllers/API/queries/variables"; -import { useDebounce } from "@/hooks/use-debounce"; import { useRefreshModelInputs } from "@/hooks/use-refresh-model-inputs"; import useAlertStore from "@/stores/alertStore"; import { Provider } from "../components/types"; +import { useModelToggleQueue } from "./useModelToggleQueue"; // Masked value shown for configured secret fields const MASKED_VALUE = "••••••••"; +// Extract a user-facing message from an error caught from an API call. +// Handles Axios-shaped errors (``response.data.detail``) and standard +// ``Error.message`` without resorting to ``any``. +const getErrorMessage = (error: unknown): string | undefined => { + const e = error as { + response?: { data?: { detail?: string } }; + message?: string; + }; + return e?.response?.data?.detail || e?.message; +}; + interface UseProviderConfigurationOptions { selectedProvider: Provider | null; } @@ -100,8 +109,6 @@ export const useProviderConfiguration = ({ const { data: globalVariables = [] } = useGetGlobalVariables(); const { mutateAsync: validateProvider } = useValidateProvider(); const { data: providerVariablesMapping = {} } = useGetProviderVariables(); - const { mutate: updateEnabledModels, mutateAsync: updateEnabledModelsAsync } = - useUpdateEnabledModels({ retry: 0 }); const { refreshAllModelInputs } = useRefreshModelInputs(); const { data: modelProviders = [], isFetching: isFetchingModels } = useGetModelProviders( @@ -342,7 +349,7 @@ export const useProviderConfiguration = ({ setValidationError(result.error || "Validation failed"); return false; } - } catch (error: any) { + } catch (error: unknown) { // Ensure minimum 500ms duration even on error const elapsedTime = Date.now() - startTime; if (elapsedTime < 500) { @@ -350,7 +357,7 @@ export const useProviderConfiguration = ({ } setValidationState("invalid"); - setValidationError(error?.message || "Validation failed"); + setValidationError(getErrorMessage(error) || "Validation failed"); return false; } }, [selectedProvider, getVariablesForValidation, validateProvider]); @@ -420,12 +427,12 @@ export const useProviderConfiguration = ({ setIsFetchingAfterSave(true); clearValuesAfterFetchRef.current = true; invalidateProviderQueries(); - } catch (error: any) { + } catch (error: unknown) { setValidationFailed(true); setErrorData({ title: "Error Saving Configuration", list: [ - error?.response?.data?.detail || + getErrorMessage(error) || "An unexpected error occurred. Please try again.", ], }); @@ -488,11 +495,11 @@ export const useProviderConfiguration = ({ setSuccessData({ title: `${syncedSelectedProvider.provider} Activated` }); invalidateProviderQueries(); - } catch (error: any) { + } catch (error: unknown) { setErrorData({ title: "Error Activating Provider", list: [ - error?.response?.data?.detail || + getErrorMessage(error) || "An unexpected error occurred. Please try again.", ], }); @@ -529,11 +536,11 @@ export const useProviderConfiguration = ({ }); setIsFetchingAfterDisconnect(true); invalidateProviderQueries(); - } catch (error: any) { + } catch (error: unknown) { setErrorData({ title: "Error Disconnecting Provider", list: [ - error?.response?.data?.detail || + getErrorMessage(error) || "An unexpected error occurred. Please try again.", ], }); @@ -547,144 +554,14 @@ export const useProviderConfiguration = ({ invalidateProviderQueries, ]); - const pendingModelToggles = useRef>({}); - const fallbackModelData = useRef( - undefined, - ); - - const flushModelToggles = useDebounce(() => { - if (!syncedSelectedProvider?.provider) return; - const providerName = syncedSelectedProvider.provider; - - const updates = Object.entries(pendingModelToggles.current).map( - ([modelName, enabled]) => ({ - provider: providerName, - model_id: modelName, - enabled, - }), - ); - - if (updates.length === 0) return; - - // Capture the fallback data - const previousData = fallbackModelData.current; - - // Clear buffer - pendingModelToggles.current = {}; - fallbackModelData.current = undefined; - - updateEnabledModels( - { updates }, - { - onError: (error: any) => { - if (previousData) { - queryClient.setQueryData(["useGetEnabledModels"], previousData); - } - const errorMessage = - error?.response?.data?.detail || - error?.message || - "Failed to update model status"; - setErrorData({ - title: "Error updating model status", - list: [errorMessage], - }); - }, - onSettled: () => { - queryClient.invalidateQueries({ - queryKey: ["useGetEnabledModels"], - }); - queryClient.invalidateQueries({ - queryKey: ["useGetModelProviders"], - }); - refreshAllModelInputs({ silent: true }); - }, - }, - ); - }, 1000); - - const flushPendingChanges = useCallback(async () => { - // Cancel the pending debounce timer — we'll send the toggles directly - flushModelToggles.cancel(); - - if (!syncedSelectedProvider?.provider) return; - const providerName = syncedSelectedProvider.provider; - - const toggles = { ...pendingModelToggles.current }; - if (Object.keys(toggles).length === 0) return; - - const updates = Object.entries(toggles).map(([modelName, enabled]) => ({ - provider: providerName, - model_id: modelName, - enabled, - })); - - const previousData = fallbackModelData.current; - - // Clear buffer - pendingModelToggles.current = {}; - fallbackModelData.current = undefined; - - try { - await updateEnabledModelsAsync({ updates }); - // Mutation succeeded — query invalidation is handled by - // refreshAllModelInputs which runs after this promise resolves. - } catch (error: any) { - // Revert optimistic update on failure - if (previousData) { - queryClient.setQueryData(["useGetEnabledModels"], previousData); - } - const errorMessage = - error?.response?.data?.detail || - error?.message || - "Failed to update model status"; - setErrorData({ - title: "Error updating model status", - list: [errorMessage], - }); - } - }, [ - flushModelToggles, - syncedSelectedProvider, - queryClient, - updateEnabledModelsAsync, - setErrorData, - ]); - - const handleModelToggle = useCallback( - (modelName: string, enabled: boolean) => { - if (!syncedSelectedProvider?.provider) return; - - const providerName = syncedSelectedProvider.provider; - - if (Object.keys(pendingModelToggles.current).length === 0) { - fallbackModelData.current = - queryClient.getQueryData([ - "useGetEnabledModels", - ]); - } - - queryClient.setQueryData( - ["useGetEnabledModels"], - (old) => { - if (!old) return old; - return { - ...old, - enabled_models: { - ...old.enabled_models, - [providerName]: { - ...old.enabled_models[providerName], - [modelName]: enabled, - }, - }, - }; - }, - ); - - pendingModelToggles.current[modelName] = enabled; - flushModelToggles(); - }, - [syncedSelectedProvider, queryClient, flushModelToggles], - ); + // Model-toggle queue — overlay buffer, unsent buffer, debounced flush, + // awaitable close-time flush, re-overlay effect. Extracted into its own + // hook so this file stays focused on variable CRUD + provider lifecycle. + // See ``useModelToggleQueue`` for the full design rationale (split + // buffers, drain-before-rollback ordering, re-overlay loop guard). + const { handleModelToggle, flushPendingChanges } = useModelToggleQueue({ + providerName: syncedSelectedProvider?.provider, + }); return { variableValues,