diff --git a/.changelog/next/fixed-issue-4157.md b/.changelog/next/fixed-issue-4157.md new file mode 100644 index 0000000000..3410fab0f0 --- /dev/null +++ b/.changelog/next/fixed-issue-4157.md @@ -0,0 +1 @@ +- Vision pickers in the universe builder (image describe, style-reference analysis, corrective reference) now offer installed vision models the client id regex doesn't recognize, instead of showing a "no vision model" blocker diff --git a/client/src/components/universe/VisionDescribeModal.test.jsx b/client/src/components/universe/VisionDescribeModal.test.jsx index 2608ffb855..84d2b69360 100644 --- a/client/src/components/universe/VisionDescribeModal.test.jsx +++ b/client/src/components/universe/VisionDescribeModal.test.jsx @@ -15,6 +15,13 @@ vi.mock('../../hooks/useProviderModels', () => ({ }), })); +// VisionProviderPicker unions the server's authoritative VLM set into its model +// filter, so it runs the capability scan on mount — stub it out (this suite is +// about the modal's actions, and the picker's own suite covers the union). +vi.mock('../../hooks/useVisionModelIds', () => ({ + default: () => ({ idsByProvider: null, loaded: true }), +})); + // The gallery picker pulls in the media/socket layer — stub it. vi.mock('../imageGen/GalleryImagePicker', () => ({ default: () => null })); vi.mock('../ProviderModelSelector', () => ({ default: () => null })); diff --git a/client/src/components/universe/VisionProviderPicker.jsx b/client/src/components/universe/VisionProviderPicker.jsx index 428a8c5d4b..fa10d2be28 100644 --- a/client/src/components/universe/VisionProviderPicker.jsx +++ b/client/src/components/universe/VisionProviderPicker.jsx @@ -3,7 +3,9 @@ * * Owns a `useProviderModels` instance scoped to enabled API providers, with * LOCAL backends (Ollama / LM Studio) restricted to vision-capable models (cloud - * providers' lists are left intact). Renders the provider+model dropdowns plus + * providers' lists are left intact) — by the client id regex UNIONED with the + * server's authoritative per-provider VLM set (`useVisionModelIds`), like every + * other vision picker. Renders the provider+model dropdowns plus * the "no vision model" / "no provider" guidance, and lifts the current * selection to the parent via `onChange` so the caller can submit the chosen * `{ providerId, model }` and gate its action on a vision model being present. @@ -15,27 +17,48 @@ * it's actually needed. */ -import { useEffect } from 'react'; +import { useCallback, useEffect } from 'react'; import ProviderModelSelector from '../ProviderModelSelector'; import useProviderModels from '../../hooks/useProviderModels'; -import { enabledApiProviderFilter, visionLocalModelFilter } from '../../utils/providers'; +import useVisionModelIds from '../../hooks/useVisionModelIds'; +import { enabledApiProviderFilter, localBackendForProvider, visionLocalModelFilter } from '../../utils/providers'; export default function VisionProviderPicker({ label = 'Vision provider', onChange }) { + // The server's authoritative per-provider VLM set, unioned into the filter: + // the client id regex only knows the multimodal families it was written + // against, so on its own it hides installed VLMs from newer ones (`gemma4`). + // Every mount of this picker is behind a modal/conditional render, so the + // capability scan is already deferred until it's needed — no `enabled` gate. + const { idsByProvider: visionIds, loaded: visionLoaded } = useVisionModelIds(); + const modelFilter = useCallback( + (id, provider) => visionLocalModelFilter(id, provider, visionIds), + [visionIds], + ); + const { providers, selectedProviderId, selectedModel, availableModels, setSelectedProviderId, setSelectedModel, loading, - } = useProviderModels({ filter: enabledApiProviderFilter, modelFilter: visionLocalModelFilter, silent: true }); + } = useProviderModels({ filter: enabledApiProviderFilter, modelFilter, silent: true }); const hasProviders = providers.length > 0; + // While the capability scan is in flight the filter is regex-only, so an empty + // selection on a LOCAL backend is "don't know yet", not "none installed" — + // asserting the blocker here would flash it and then flip. Cloud providers are + // never filtered, so their empty selection is already a final answer. + const visionPending = !visionLoaded + && !!localBackendForProvider(providers.find((p) => p.id === selectedProviderId)); // A provider is selected but exposes no vision-capable model (all of a local // backend's models were filtered out) — block the run with an explanation. - const noVisionModel = hasProviders && !selectedModel; + const noVisionModel = hasProviders && !selectedModel && !visionPending; // Lift the selection so the caller can submit it and gate on a vision model. // `onChange` should be a stable setter; deps are bounded (load + user picks). + // `loading` covers the capability scan too — the auto-picked model can still + // change when it lands, so the selection isn't final until it settles. + const resolving = loading || visionPending; useEffect(() => { - onChange?.({ providerId: selectedProviderId, model: selectedModel, hasProviders, noVisionModel, loading }); - }, [onChange, selectedProviderId, selectedModel, hasProviders, noVisionModel, loading]); + onChange?.({ providerId: selectedProviderId, model: selectedModel, hasProviders, noVisionModel, loading: resolving }); + }, [onChange, selectedProviderId, selectedModel, hasProviders, noVisionModel, resolving]); if (!hasProviders) { return ( diff --git a/client/src/components/universe/VisionProviderPicker.test.jsx b/client/src/components/universe/VisionProviderPicker.test.jsx new file mode 100644 index 0000000000..3eb9f26462 --- /dev/null +++ b/client/src/components/universe/VisionProviderPicker.test.jsx @@ -0,0 +1,104 @@ +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { render, screen, waitFor, within } from '@testing-library/react'; + +vi.mock('../../services/api', () => ({ getProviders: vi.fn() })); +vi.mock('../../services/apiLocalLlm', () => ({ getVisionModels: vi.fn(), getToolUseModels: vi.fn() })); + +import VisionProviderPicker from './VisionProviderPicker'; +import { getProviders } from '../../services/api'; +import { getVisionModels } from '../../services/apiLocalLlm'; + +// A local backend whose ONLY vision-capable model belongs to a family the +// client id regex predates — the `gemma4` gap, restaged with a placeholder id +// so it keeps testing the GAP rather than whichever families the regex has +// since learned. Regex-only, this picker renders empty. +const VLM_ID = 'muse-glimmer:30b'; +const OLLAMA = { + id: 'ollama', + name: 'Ollama', + type: 'api', + enabled: true, + endpoint: 'http://127.0.0.1:11434', + defaultModel: 'qwen3.6:35b', + models: ['qwen3.6:35b', VLM_ID, 'nomic-embed-text'], +}; + +// Cloud providers are never id-filtered — the regex is a local-name heuristic. +const CLOUD = { + id: 'cloud-api', + name: 'Cloud API', + type: 'api', + enabled: true, + defaultModel: 'omni-1', + models: ['omni-1', 'omni-1-mini'], +}; + +const modelSelect = () => screen.getByLabelText('Model'); + +beforeEach(() => { + vi.clearAllMocks(); + getProviders.mockResolvedValue({ providers: [OLLAMA] }); + getVisionModels.mockResolvedValue({ models: [] }); +}); + +describe('VisionProviderPicker', () => { + it('offers a VLM the client id regex does not recognize, once the server list lands', async () => { + getVisionModels.mockResolvedValue({ + models: [{ id: VLM_ID, backend: 'ollama', providerId: 'ollama', vision: true }], + }); + const onChange = vi.fn(); + render(); + + // The auto-pick is lifted, so the caller's "Run" gate (`!!vision.model`) opens. + await waitFor(() => expect(onChange).toHaveBeenLastCalledWith( + expect.objectContaining({ providerId: 'ollama', model: VLM_ID, noVisionModel: false }), + )); + expect(modelSelect()).toHaveValue(VLM_ID); + expect(within(modelSelect()).getByRole('option', { name: VLM_ID })).toBeInTheDocument(); + expect(screen.queryByText(/no vision-capable model installed/i)).not.toBeInTheDocument(); + }); + + it('still blocks with an explanation when the backend really has no VLM', async () => { + // The scan settles reporting nothing installed — regex-only is then the best + // answer available, and it also finds nothing. + render(); + + await waitFor(() => expect(screen.getByText(/no vision-capable model installed/i)).toBeInTheDocument()); + expect(screen.queryByLabelText('Model')).not.toBeInTheDocument(); + }); + + it('does not flash the blocker while the capability scan is still in flight', async () => { + let settle; + getVisionModels.mockReturnValue(new Promise((resolve) => { settle = resolve; })); + const onChange = vi.fn(); + render(); + + // Providers have loaded and the regex found nothing — but "don't know yet" + // must not render as "none installed". + await waitFor(() => expect(onChange).toHaveBeenCalled()); + expect(screen.queryByText(/no vision-capable model installed/i)).not.toBeInTheDocument(); + expect(onChange).toHaveBeenLastCalledWith( + expect.objectContaining({ noVisionModel: false, loading: true }), + ); + + settle({ models: [] }); + await waitFor(() => expect(screen.getByText(/no vision-capable model installed/i)).toBeInTheDocument()); + }); + + it('leaves a cloud provider unfiltered while the scan is pending', async () => { + getProviders.mockResolvedValue({ providers: [CLOUD] }); + getVisionModels.mockReturnValue(new Promise(() => {})); + render(); + + // No local backend selected → nothing to wait for; the cloud list is final. + await waitFor(() => expect(modelSelect()).toHaveValue('omni-1')); + expect(screen.queryByText(/no vision-capable model installed/i)).not.toBeInTheDocument(); + }); + + it('explains the empty case when no provider is configured at all', async () => { + getProviders.mockResolvedValue({ providers: [] }); + render(); + + await waitFor(() => expect(screen.getByText(/No API provider with a vision-capable model/i)).toBeInTheDocument()); + }); +}); diff --git a/client/src/hooks/useProviderModels.js b/client/src/hooks/useProviderModels.js index b08240a470..36638b3c70 100644 --- a/client/src/hooks/useProviderModels.js +++ b/client/src/hooks/useProviderModels.js @@ -35,6 +35,15 @@ const sourceModels = (provider, withEffort) => { * set, the auto-selected / provider-change model is the first model that * passes the filter rather than the provider's `defaultModel` (which may not * qualify). Omit for the full selectable list. + * + * A `modelFilter` whose IDENTITY changes is supported and expected: a vision + * picker starts on the client-side id regex and widens once the server's + * authoritative capability list resolves (`useVisionModelIds`). The hook then + * re-runs its initial pick — without refetching the provider list — so a + * selection the first, blinder filter couldn't make isn't frozen at `''`. + * Once the user picks (or clears) a model, that wins and the re-pick stands + * down. Memoize the predicate (`useCallback`) so it only changes when its + * inputs do. * @param {boolean} [options.withEffort] - Set when the caller also renders an * effort control (`ProviderModelSelector`'s `effort`/`onEffortChange`) and * threads the value to the server. Providers whose CLI bakes the reasoning @@ -50,6 +59,12 @@ export default function useProviderModels({ filter, allowDefault = false, silent const [selectedModel, setSelectedModel] = useState(''); const [loading, setLoading] = useState(true); const hasSetInitialRef = useRef(false); + // Latched once the model is chosen deliberately — a picker change, or a + // caller restoring a saved pin. The auto re-pick below then stands down for + // good, so a deliberate CLEAR (`''`) is not silently refilled when the filter + // later widens; `selectedModel === ''` alone can't carry that distinction, + // since it is also what a filter that matched nothing produces. + const userPickedModelRef = useRef(false); // Resolve the model to pin when a provider is (auto-)selected. With a // modelFilter, the provider's defaultModel may not qualify (e.g. a vision @@ -62,6 +77,16 @@ export default function useProviderModels({ filter, allowDefault = false, silent return models[0] || ''; }, [modelFilter, withEffort]); + // `load` must NOT depend on `pickInitialModel` (and so on `modelFilter`): a + // caller whose filter identity changes when a capability list resolves would + // otherwise re-run the whole `api.getProviders()` fetch for a change that + // needs no new data. The ref hands the async body the freshest picker without + // pulling it into the dependency list — synced in an effect (never mutated + // during render), which is soon enough: `load` only reads it after awaiting + // the fetch, and the initial value covers the mount. + const pickInitialModelRef = useRef(pickInitialModel); + useEffect(() => { pickInitialModelRef.current = pickInitialModel; }, [pickInitialModel]); + const load = useCallback(async () => { setLoading(true); const data = await api.getProviders(silent ? { silent: true } : undefined).catch((err) => { @@ -76,10 +101,10 @@ export default function useProviderModels({ filter, allowDefault = false, silent if (!allowDefault && filtered.length > 0 && !hasSetInitialRef.current) { hasSetInitialRef.current = true; setSelectedProviderId(filtered[0].id); - setSelectedModel(pickInitialModel(filtered[0])); + setSelectedModel(pickInitialModelRef.current(filtered[0])); } setLoading(false); - }, [filter, allowDefault, silent, pickInitialModel]); + }, [filter, allowDefault, silent]); useEffect(() => { load(); }, [load]); @@ -107,8 +132,35 @@ export default function useProviderModels({ filter, allowDefault = false, silent [currentProvider, modelFilter, selectedModel, withEffort] ); + // Re-run the initial pick when the `modelFilter`'s identity changes. Without + // this, `hasSetInitialRef` freezes the auto-pick at whatever the FIRST filter + // produced: a vision picker running on the client id regex alone returns `''` + // for a backend whose only VLM the regex doesn't know (`gemma4`), and the + // authoritative list landing a moment later never gets a say — leaving a + // "no vision model" blocker next to a now-populated dropdown. Scoped to + // filtered pickers (an unfiltered one pins `defaultModel`, which needs no + // revision) and to a selection that is still the hook's own. + useEffect(() => { + if (!modelFilter || allowDefault || userPickedModelRef.current) return; + if (!hasSetInitialRef.current || !currentProvider) return; + // Still valid under the current filter → nothing to revise. + if (selectedModel && availableModels.includes(selectedModel)) return; + const next = pickInitialModel(currentProvider); + if (next !== selectedModel) setSelectedModel(next); + }, [modelFilter, allowDefault, currentProvider, availableModels, selectedModel, pickInitialModel]); + + // A user pick latches: the re-pick above never overrides it, in either + // direction (a chosen model, or a deliberate clear). + const handleModelChange = useCallback((model) => { + userPickedModelRef.current = true; + setSelectedModel(model); + }, []); + const handleProviderChange = useCallback((id) => { setSelectedProviderId(id); + // The model that follows a provider change is auto-picked, not user-picked, + // so it stays eligible for the re-pick above if the filter widens later. + userPickedModelRef.current = false; if (allowDefault) { // Empty model = "use the default model" — don't pin the provider's // defaultModel, which would suppress the empty-sentinel choice. @@ -131,7 +183,7 @@ export default function useProviderModels({ filter, allowDefault = false, silent availableModels, selectedProvider, setSelectedProviderId: handleProviderChange, - setSelectedModel, + setSelectedModel: handleModelChange, loading }; } diff --git a/client/src/hooks/useProviderModels.test.js b/client/src/hooks/useProviderModels.test.js index 5e6b02ca0a..58872380f1 100644 --- a/client/src/hooks/useProviderModels.test.js +++ b/client/src/hooks/useProviderModels.test.js @@ -90,3 +90,101 @@ describe('useProviderModels — Antigravity base models', () => { ]); }); }); + +// A capability-scoped picker (vision) starts on the client-side id regex and +// widens once the server's authoritative list resolves, so its `modelFilter` +// identity changes AFTER the first load. Stand-in filters here: the contract +// under test is the identity change, not any particular capability rule. +describe('useProviderModels — a modelFilter whose identity changes', () => { + const LOCAL = { + id: 'local-backend', + name: 'Local Backend', + type: 'api', + enabled: true, + defaultModel: 'text-a', + models: ['text-a', 'vlm-b', 'vlm-c'], + }; + const OTHER = { ...LOCAL, id: 'other-backend', name: 'Other Backend', defaultModel: 'text-a' }; + + // The blind first pass: knows no vision family this backend has installed. + const matchesNothing = () => false; + const matchesB = (id) => id === 'vlm-b'; + const matchesBandC = (id) => id === 'vlm-b' || id === 'vlm-c'; + + const mountFiltered = async (modelFilter, providers = [LOCAL], extra = {}) => { + api.getProviders.mockResolvedValue({ providers }); + const hook = renderHook( + ({ filterFn }) => useProviderModels({ modelFilter: filterFn, ...extra }), + { initialProps: { filterFn: modelFilter } }, + ); + await waitFor(() => expect(hook.result.current.loading).toBe(false)); + return hook; + }; + + beforeEach(() => vi.clearAllMocks()); + + it('re-picks the initial model once the filter widens', async () => { + const { result, rerender } = await mountFiltered(matchesNothing); + // The blind first pass finds nothing — the bug this guards is that the + // hook used to freeze here forever. + expect(result.current.selectedModel).toBe(''); + expect(result.current.availableModels).toEqual([]); + + rerender({ filterFn: matchesB }); + await waitFor(() => expect(result.current.selectedModel).toBe('vlm-b')); + expect(result.current.availableModels).toEqual(['vlm-b']); + }); + + it('does not refetch the provider list for a filter identity change', async () => { + const { rerender, result } = await mountFiltered(matchesNothing); + expect(api.getProviders).toHaveBeenCalledTimes(1); + + rerender({ filterFn: matchesB }); + await waitFor(() => expect(result.current.selectedModel).toBe('vlm-b')); + expect(api.getProviders).toHaveBeenCalledTimes(1); + }); + + it('leaves a deliberate user clear alone when the filter widens', async () => { + const { result, rerender } = await mountFiltered(matchesB); + expect(result.current.selectedModel).toBe('vlm-b'); + + act(() => result.current.setSelectedModel('')); + rerender({ filterFn: matchesBandC }); + // A clear and a "the filter matched nothing" both read as `''` — only the + // user-pick latch tells them apart, and the user's wins. + await waitFor(() => expect(result.current.availableModels).toEqual(['vlm-b', 'vlm-c'])); + expect(result.current.selectedModel).toBe(''); + }); + + it('leaves a user-picked model alone when the filter changes', async () => { + const { result, rerender } = await mountFiltered(matchesBandC); + act(() => result.current.setSelectedModel('vlm-c')); + + rerender({ filterFn: matchesB }); + await waitFor(() => expect(result.current.availableModels).toEqual(['vlm-b'])); + expect(result.current.selectedModel).toBe('vlm-c'); + }); + + it('re-arms the auto-pick after a provider change', async () => { + const { result, rerender } = await mountFiltered(matchesNothing, [LOCAL, OTHER]); + // A provider change picks through the CURRENT (still blind) filter… + act(() => result.current.setSelectedProviderId('other-backend')); + expect(result.current.selectedModel).toBe(''); + + // …so the widened filter must still get its say on that provider. + rerender({ filterFn: matchesB }); + await waitFor(() => expect(result.current.selectedModel).toBe('vlm-b')); + expect(result.current.selectedProviderId).toBe('other-backend'); + }); + + it('keeps the empty-model sentinel under allowDefault', async () => { + const { result, rerender } = await mountFiltered(matchesNothing, [LOCAL], { allowDefault: true }); + expect(result.current.selectedProviderId).toBe(''); + expect(result.current.selectedModel).toBe(''); + + rerender({ filterFn: matchesB }); + await waitFor(() => expect(result.current.availableModels).toEqual([])); + // `''` is the "use the default model" choice here, never an auto-pick target. + expect(result.current.selectedModel).toBe(''); + }); +});