diff --git a/apps/web/src/components/ChatView.tsx b/apps/web/src/components/ChatView.tsx index 42938f15..60849176 100644 --- a/apps/web/src/components/ChatView.tsx +++ b/apps/web/src/components/ChatView.tsx @@ -67,6 +67,7 @@ import { providerAgentsQueryOptions, providerComposerCapabilitiesQueryOptions, providerCommandsQueryOptions, + isInitialModelDiscoveryPending, providerModelsQueryOptions, providerPluginsQueryOptions, providerSkillsQueryOptions, @@ -1477,14 +1478,12 @@ export default function ChatView({ const hasResolvedCursorModelDiscovery = cursorDynamicModelsQuery.data?.source === "cursor.cli" && (cursorDynamicModelsQuery.data.models.length ?? 0) > 0; - // Only the very first discovery attempt (no settled result yet) should gate - // the picker. Once discovery resolves — including a fault-isolated "error" - // result when the Cursor CLI is unavailable — background refetches must not - // re-blank the shared model picker. + // Gate the skeleton on the initial discovery fetch only; once discovery has + // settled, background refetches must not re-blank the shared model picker. const cursorModelDiscoveryPending = cursorModelDiscoveryEnabled && !hasResolvedCursorModelDiscovery && - cursorDynamicModelsQuery.isLoading; + isInitialModelDiscoveryPending(cursorDynamicModelsQuery); const kiloModelDiscoveryEnabled = selectedProvider === "kilo" || lockedProvider === "kilo" || isModelPickerOpen; const hasResolvedKiloModelDiscovery = @@ -1492,7 +1491,9 @@ export default function ChatView({ kiloDynamicModelsQuery.data?.source === "kilo") && (kiloDynamicModelsQuery.data.models.length ?? 0) > 0; const kiloModelDiscoveryPending = - kiloModelDiscoveryEnabled && !hasResolvedKiloModelDiscovery && kiloDynamicModelsQuery.isLoading; + kiloModelDiscoveryEnabled && + !hasResolvedKiloModelDiscovery && + isInitialModelDiscoveryPending(kiloDynamicModelsQuery); const modelOptionsByProvider = useMemo(() => { const staticOptions: Record> = { codex: getAppModelOptions( diff --git a/apps/web/src/lib/providerDiscoveryReactQuery.test.ts b/apps/web/src/lib/providerDiscoveryReactQuery.test.ts index 83199c14..7c8cf610 100644 --- a/apps/web/src/lib/providerDiscoveryReactQuery.test.ts +++ b/apps/web/src/lib/providerDiscoveryReactQuery.test.ts @@ -1,12 +1,10 @@ -// FILE: providerDiscoveryReactQuery.test.ts -// Purpose: Verifies per-provider model discovery stays fault-isolated. -// Layer: Web data fetching tests -// Depends on: Vitest, React Query, and the native API bridge mock. - import type { NativeApi, ProviderKind, ProviderListModelsInput } from "@t3tools/contracts"; -import { QueryClient } from "@tanstack/react-query"; +import { QueryClient, QueryObserver } from "@tanstack/react-query"; import { afterEach, describe, expect, it, vi } from "vitest"; -import { providerModelsQueryOptions } from "./providerDiscoveryReactQuery"; +import { + isInitialModelDiscoveryPending, + providerModelsQueryOptions, +} from "./providerDiscoveryReactQuery"; import * as nativeApi from "../nativeApi"; function mockListModels(impl: (input: ProviderListModelsInput) => Promise) { @@ -103,3 +101,51 @@ describe("providerModelsQueryOptions", () => { expect(cursorKey).not.toEqual(codexKey); }); }); + +describe("isInitialModelDiscoveryPending", () => { + it("gates the picker during the initial fetch but not on background refetches", async () => { + let resolveListModels: (value: unknown) => void = () => {}; + mockListModels( + () => + new Promise((resolve) => { + resolveListModels = resolve; + }), + ); + const discovered = { + models: [{ slug: "auto", name: "Auto" }], + source: "cursor.cli", + cached: false, + }; + + const queryClient = new QueryClient(); + const observer = new QueryObserver( + queryClient, + providerModelsQueryOptions({ provider: "cursor" }), + ); + const unsubscribe = observer.subscribe(() => {}); + + // placeholderData keeps the query in "success" status during the first + // fetch, so isLoading stays false — only isFetching && isPlaceholderData + // marks the initial discovery as pending. + expect(observer.getCurrentResult().isLoading).toBe(false); + expect(isInitialModelDiscoveryPending(observer.getCurrentResult())).toBe(true); + + resolveListModels(discovered); + await vi.waitFor(() => { + expect(observer.getCurrentResult().isFetching).toBe(false); + }); + expect(isInitialModelDiscoveryPending(observer.getCurrentResult())).toBe(false); + + // A background refetch over already-settled data must not re-gate the picker. + const refetch = observer.refetch(); + await vi.waitFor(() => { + expect(observer.getCurrentResult().isFetching).toBe(true); + }); + expect(observer.getCurrentResult().isPlaceholderData).toBe(false); + expect(isInitialModelDiscoveryPending(observer.getCurrentResult())).toBe(false); + + resolveListModels(discovered); + await refetch; + unsubscribe(); + }); +}); diff --git a/apps/web/src/lib/providerDiscoveryReactQuery.ts b/apps/web/src/lib/providerDiscoveryReactQuery.ts index 72dbd7af..9435c2df 100644 --- a/apps/web/src/lib/providerDiscoveryReactQuery.ts +++ b/apps/web/src/lib/providerDiscoveryReactQuery.ts @@ -29,10 +29,8 @@ const EMPTY_MODELS_RESULT: ProviderListModelsResult = { cached: false, }; -// Returned when a single provider's model discovery fails (e.g. the Cursor CLI -// is not installed or not authenticated). Resolving to this instead of -// rejecting keeps the failure isolated to that provider so the shared model -// picker — and every other provider's models — stays usable. +// Resolved (not rejected) when a provider's model discovery fails, so the +// failure stays isolated to that provider and the shared picker stays usable. const MODEL_DISCOVERY_ERROR_RESULT: ProviderListModelsResult = { models: [], source: "error", @@ -176,25 +174,32 @@ export function providerModelsQueryOptions(input: { ...(input.agentDir ? { agentDir: input.agentDir } : {}), }); } catch (error) { - // Fault isolation: model discovery runs per provider, so one failing - // CLI must degrade only that provider. Rejecting here would put the - // query in an error state and blank the shared model picker; instead - // we resolve to an empty "error" result and let the UI fall back to - // that provider's static models while every other provider is - // unaffected. + // Resolve instead of rejecting so one provider's failure can't blank + // the shared picker; the UI falls back to that provider's static models. console.warn(`Model discovery failed for provider "${input.provider}".`, error); return MODEL_DISCOVERY_ERROR_RESULT; } }, enabled: input.enabled ?? true, - // Discovery failures are caught inside queryFn and surfaced as a resolved - // "error" result, so the query itself never rejects and never retries. + // queryFn already catches failures into a resolved result, so a retry would + // only re-run a query that never rejects. retry: false, staleTime: 60_000, placeholderData: (previous) => previous ?? EMPTY_MODELS_RESULT, }); } +// True only on a query's first fetch. placeholderData keeps the query in +// "success" status during that fetch (isLoading never flips true), so +// isFetching && isPlaceholderData is what separates it from later refetches. +export function isInitialModelDiscoveryPending(query: { + isLoading: boolean; + isFetching: boolean; + isPlaceholderData: boolean; +}): boolean { + return query.isLoading || (query.isFetching && query.isPlaceholderData); +} + export function providerAgentsQueryOptions(input: { provider: ProviderKind; enabled?: boolean }) { return queryOptions({ queryKey: providerDiscoveryQueryKeys.agents(input.provider),