mirror of
https://github.com/danny-avila/LibreChat.git
synced 2026-08-27 12:13:30 +00:00
🎛️ fix: Withhold the Seeded Model Catalogue Until Models Resolve (#15035)
`useGetModelsQuery` seeds from a static fallback config, so `modelsQuery.data` describes a hardcoded model list both before the mounted fetch resolves and after it fails outright. The agent builder read that seed as authoritative and offered models the active server configuration never exposed. Blank the catalogue until the mounted fetch actually succeeds, surface the failure in the model panel instead of silently falling back to the seed, and refuse to create an agent against a provider/model pair the resolved catalogue does not offer. Also wires the builder's orphaned `htmlFor` labels to the controls they name.
This commit is contained in:
parent
018775de07
commit
bf6144c9e1
7 changed files with 251 additions and 17 deletions
|
|
@ -227,6 +227,7 @@ export type AgentModelPanelProps = {
|
|||
agent_id?: string;
|
||||
providers: Option[];
|
||||
models: Record<string, string[] | undefined>;
|
||||
modelsError: boolean;
|
||||
modelsReady: boolean;
|
||||
setActivePanel: React.Dispatch<React.SetStateAction<Panel>>;
|
||||
};
|
||||
|
|
|
|||
|
|
@ -77,6 +77,7 @@ const AgentCategorySelector: React.FC<{ className?: string }> = ({ className })
|
|||
|
||||
return (
|
||||
<ControlCombobox
|
||||
selectId="category-selector"
|
||||
selectedValue={field.value}
|
||||
displayValue={displayValue}
|
||||
searchPlaceholder={searchPlaceholder}
|
||||
|
|
|
|||
|
|
@ -101,6 +101,7 @@ export default function AgentConfig() {
|
|||
{localize('com_ui_model')} <span className="text-red-500">*</span>
|
||||
</Label>
|
||||
<button
|
||||
id="provider"
|
||||
type="button"
|
||||
onClick={() => setActivePanel(Panel.model)}
|
||||
title={model || undefined}
|
||||
|
|
|
|||
|
|
@ -6,7 +6,7 @@ import { render, waitFor, fireEvent, act } from '@testing-library/react';
|
|||
import { QueryClient, QueryClientProvider } from '@tanstack/react-query';
|
||||
import type { UseFormReturn } from 'react-hook-form';
|
||||
import type { Agent } from 'librechat-data-provider';
|
||||
import type { AgentForm } from '~/common';
|
||||
import type { AgentForm, AgentModelPanelProps } from '~/common';
|
||||
|
||||
// Mock toast context - define this after all mocks
|
||||
let mockShowToast: jest.Mock;
|
||||
|
|
@ -16,6 +16,11 @@ let mockModelsQuery: {
|
|||
isSuccess: boolean;
|
||||
isFetching?: boolean;
|
||||
} = { data: {}, isFetchedAfterMount: true, isSuccess: true };
|
||||
let mockModelPanelProps: Pick<
|
||||
AgentModelPanelProps,
|
||||
'models' | 'modelsError' | 'modelsReady'
|
||||
> | null = null;
|
||||
let mockFormDefaults: Partial<AgentForm> = {};
|
||||
let mockAgentPanelContext = {
|
||||
activePanel: 'builder',
|
||||
agentsConfig: { allowedProviders: [] as string[] },
|
||||
|
|
@ -50,6 +55,7 @@ jest.mock('librechat-data-provider', () => {
|
|||
return {
|
||||
...actualModule,
|
||||
dataService: {
|
||||
createAgent: jest.fn(),
|
||||
updateAgent: jest.fn(),
|
||||
},
|
||||
Tools: actualModule.Tools || {
|
||||
|
|
@ -163,7 +169,10 @@ jest.mock('./AgentSelect', () => ({
|
|||
|
||||
jest.mock('./ModelPanel', () => ({
|
||||
__esModule: true,
|
||||
default: () => <div>{`Model Panel`}</div>,
|
||||
default: (props: Pick<AgentModelPanelProps, 'models' | 'modelsError' | 'modelsReady'>) => {
|
||||
mockModelPanelProps = props;
|
||||
return <div>{`Model Panel`}</div>;
|
||||
},
|
||||
}));
|
||||
|
||||
// Mock AgentFooter to provide a save button
|
||||
|
|
@ -196,6 +205,7 @@ jest.mock('react-hook-form', () => {
|
|||
execute_code: false,
|
||||
file_search: false,
|
||||
web_search: false,
|
||||
...mockFormDefaults,
|
||||
},
|
||||
});
|
||||
|
||||
|
|
@ -308,6 +318,8 @@ describe('AgentPanel - Update Agent Toast Messages', () => {
|
|||
mockShowToast = jest.fn();
|
||||
mockFormSubmitHandler = null;
|
||||
capturedFormMethods = null;
|
||||
mockModelPanelProps = null;
|
||||
mockFormDefaults = {};
|
||||
mockModelsQuery = {
|
||||
data: { openai: ['gpt-4'] },
|
||||
isFetchedAfterMount: true,
|
||||
|
|
@ -391,6 +403,45 @@ describe('AgentPanel - Update Agent Toast Messages', () => {
|
|||
expect(localStorage.getItem('lastAgentModel')).toBeNull();
|
||||
});
|
||||
|
||||
it('withholds the seeded catalogue until the mounted fetch resolves', async () => {
|
||||
const { mockUseGetAgentByIdQuery } = setupMocks();
|
||||
mockAgentQuery(mockUseGetAgentByIdQuery, {});
|
||||
mockAgentPanelContext = { ...mockAgentPanelContext, activePanel: 'model' };
|
||||
mockModelsQuery = {
|
||||
data: { openai: ['seeded-fallback-model'] },
|
||||
isFetchedAfterMount: false,
|
||||
isSuccess: true,
|
||||
isFetching: true,
|
||||
};
|
||||
|
||||
const Wrapper = createWrapper();
|
||||
render(<AgentPanel />, { wrapper: Wrapper });
|
||||
|
||||
await waitFor(() => expect(mockModelPanelProps).not.toBeNull());
|
||||
expect(mockModelPanelProps?.models).toEqual({});
|
||||
expect(mockModelPanelProps?.modelsReady).toBe(false);
|
||||
expect(mockModelPanelProps?.modelsError).toBe(false);
|
||||
});
|
||||
|
||||
it('withholds the seeded catalogue when the models request fails', async () => {
|
||||
const { mockUseGetAgentByIdQuery } = setupMocks();
|
||||
mockAgentQuery(mockUseGetAgentByIdQuery, {});
|
||||
mockAgentPanelContext = { ...mockAgentPanelContext, activePanel: 'model' };
|
||||
mockModelsQuery = {
|
||||
data: { openai: ['seeded-fallback-model'] },
|
||||
isFetchedAfterMount: true,
|
||||
isSuccess: false,
|
||||
isFetching: false,
|
||||
};
|
||||
|
||||
const Wrapper = createWrapper();
|
||||
render(<AgentPanel />, { wrapper: Wrapper });
|
||||
|
||||
await waitFor(() => expect(mockModelPanelProps).not.toBeNull());
|
||||
expect(mockModelPanelProps?.models).toEqual({});
|
||||
expect(mockModelPanelProps?.modelsError).toBe(true);
|
||||
});
|
||||
|
||||
it("preserves an existing agent's configured model", async () => {
|
||||
const { mockUseGetAgentByIdQuery } = setupMocks();
|
||||
mockAgentQuery(mockUseGetAgentByIdQuery, {});
|
||||
|
|
@ -605,6 +656,88 @@ describe('AgentPanel - Update Agent Toast Messages', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('agent creation', () => {
|
||||
/** Creation runs off the form's own `id`, and a provider/model the user picked keeps the
|
||||
* saved-defaults reconciliation from rewriting the pair before it reaches submission. */
|
||||
const renderAndSubmitNewAgent = async () => {
|
||||
const Wrapper = createWrapper();
|
||||
const { container } = render(<AgentPanel />, { wrapper: Wrapper });
|
||||
|
||||
await act(async () => {
|
||||
capturedFormMethods?.setValue('provider', 'openai', { shouldDirty: true });
|
||||
capturedFormMethods?.setValue('model', 'gpt-4', { shouldDirty: true });
|
||||
});
|
||||
|
||||
fireEvent.submit(container.querySelector('form')!);
|
||||
if (mockFormSubmitHandler) {
|
||||
await act(async () => {
|
||||
mockFormSubmitHandler!();
|
||||
});
|
||||
}
|
||||
};
|
||||
|
||||
beforeEach(() => {
|
||||
mockFormDefaults = { id: '' };
|
||||
mockAgentPanelContext = { ...mockAgentPanelContext, agent_id: undefined };
|
||||
});
|
||||
|
||||
it('refuses to create an agent while the catalogue is unavailable', async () => {
|
||||
const { mockUseGetAgentByIdQuery } = setupMocks();
|
||||
mockAgentQuery(mockUseGetAgentByIdQuery, {});
|
||||
mockModelsQuery = {
|
||||
data: { openai: ['gpt-4'] },
|
||||
isFetchedAfterMount: true,
|
||||
isSuccess: false,
|
||||
isFetching: false,
|
||||
};
|
||||
|
||||
await renderAndSubmitNewAgent();
|
||||
|
||||
expect(mockShowToast).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ message: 'com_error_models_not_loaded' }),
|
||||
);
|
||||
expect(dataService.createAgent).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('refuses to create an agent with a model the catalogue no longer offers', async () => {
|
||||
const { mockUseGetAgentByIdQuery } = setupMocks();
|
||||
mockAgentQuery(mockUseGetAgentByIdQuery, {});
|
||||
mockModelsQuery = {
|
||||
data: { openai: ['gpt-4o'] },
|
||||
isFetchedAfterMount: true,
|
||||
isSuccess: true,
|
||||
isFetching: false,
|
||||
};
|
||||
|
||||
await renderAndSubmitNewAgent();
|
||||
|
||||
expect(mockShowToast).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ message: 'com_error_model_not_found' }),
|
||||
);
|
||||
expect(dataService.createAgent).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it('creates the agent when the catalogue offers the selected model', async () => {
|
||||
const { mockUseGetAgentByIdQuery } = setupMocks();
|
||||
mockAgentQuery(mockUseGetAgentByIdQuery, {});
|
||||
(dataService.createAgent as jest.Mock).mockResolvedValue(createMockAgent());
|
||||
mockModelsQuery = {
|
||||
data: { openai: ['gpt-4'] },
|
||||
isFetchedAfterMount: true,
|
||||
isSuccess: true,
|
||||
isFetching: false,
|
||||
};
|
||||
|
||||
await renderAndSubmitNewAgent();
|
||||
|
||||
await waitFor(() =>
|
||||
expect(dataService.createAgent).toHaveBeenCalledWith(
|
||||
expect.objectContaining({ model: 'gpt-4', provider: 'openai' }),
|
||||
),
|
||||
);
|
||||
});
|
||||
});
|
||||
|
||||
it('should show error toast on update failure', async () => {
|
||||
const { mockUseGetAgentByIdQuery, mockUpdateAgent } = setupMocks();
|
||||
|
||||
|
|
|
|||
|
|
@ -319,8 +319,15 @@ export default function AgentPanel() {
|
|||
|
||||
const agentQuery = canEdit && expandedAgentQuery.data ? expandedAgentQuery : basicAgentQuery;
|
||||
|
||||
const models = useMemo(() => modelsQuery.data ?? {}, [modelsQuery.data]);
|
||||
const modelsReady = modelsQuery.isFetchedAfterMount && !modelsQuery.isFetching;
|
||||
const modelsError = modelsQuery.isFetchedAfterMount && !modelsQuery.isSuccess;
|
||||
/** The models query is seeded with a static fallback config, so its entries only describe the
|
||||
* active server once the fetch issued on mount has resolved. Until then there is nothing
|
||||
* authoritative to offer, and an outright failure must not fall back to the seed either. */
|
||||
const models = useMemo(
|
||||
() => (modelsQuery.isFetchedAfterMount && !modelsError ? (modelsQuery.data ?? {}) : {}),
|
||||
[modelsError, modelsQuery.isFetchedAfterMount, modelsQuery.data],
|
||||
);
|
||||
const methods = useForm<AgentForm>({
|
||||
defaultValues: getDefaultAgentFormValues(defaultStatefulCodeEnvironment),
|
||||
mode: 'onChange',
|
||||
|
|
@ -600,6 +607,18 @@ export default function AgentPanel() {
|
|||
status: 'error',
|
||||
});
|
||||
}
|
||||
if (!modelsReady || modelsError) {
|
||||
return showToast({
|
||||
message: localize('com_error_models_not_loaded'),
|
||||
status: 'error',
|
||||
});
|
||||
}
|
||||
if (!(models[provider] ?? []).includes(model)) {
|
||||
return showToast({
|
||||
message: localize('com_error_model_not_found'),
|
||||
status: 'error',
|
||||
});
|
||||
}
|
||||
if (!data.name) {
|
||||
return showToast({
|
||||
message: localize('com_agents_missing_name'),
|
||||
|
|
@ -609,7 +628,18 @@ export default function AgentPanel() {
|
|||
|
||||
create.mutate({ ...basePayload, model, tools, provider });
|
||||
},
|
||||
[agent_id, create, dirtyFields, handleAvatarUpload, update, showToast, localize],
|
||||
[
|
||||
agent_id,
|
||||
create,
|
||||
dirtyFields,
|
||||
handleAvatarUpload,
|
||||
models,
|
||||
modelsError,
|
||||
modelsReady,
|
||||
update,
|
||||
showToast,
|
||||
localize,
|
||||
],
|
||||
);
|
||||
|
||||
const handleSelectAgent = useCallback(() => {
|
||||
|
|
@ -693,6 +723,7 @@ export default function AgentPanel() {
|
|||
<ModelPanel
|
||||
models={models}
|
||||
providers={providers}
|
||||
modelsError={modelsError}
|
||||
modelsReady={modelsReady}
|
||||
setActivePanel={setActivePanel}
|
||||
/>
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@ import type { AgentForm } from '~/common';
|
|||
import ModelPanel from './ModelPanel';
|
||||
|
||||
jest.mock('@librechat/client', () => ({
|
||||
Alert: ({ children }: { children: React.ReactNode }) => <div role="alert">{children}</div>,
|
||||
Button: ({ children, onClick, type }: React.ButtonHTMLAttributes<HTMLButtonElement>) => (
|
||||
<button type={type} onClick={onClick}>
|
||||
{children}
|
||||
|
|
@ -17,17 +18,25 @@ jest.mock('@librechat/client', () => ({
|
|||
ariaLabel,
|
||||
disabled,
|
||||
items,
|
||||
selectId,
|
||||
selectedValue,
|
||||
selectPlaceholder,
|
||||
setValue,
|
||||
}: {
|
||||
ariaLabel: string;
|
||||
disabled?: boolean;
|
||||
items: Array<{ label: string; value: string }>;
|
||||
selectId?: string;
|
||||
selectedValue: string;
|
||||
selectPlaceholder?: string;
|
||||
setValue: (value: string) => void;
|
||||
}) => (
|
||||
<div>
|
||||
<button id={selectId} type="button" disabled={disabled} aria-label={ariaLabel}>
|
||||
{selectedValue || selectPlaceholder}
|
||||
</button>
|
||||
<span data-testid={`${ariaLabel}-selected`}>{selectedValue}</span>
|
||||
<span data-testid={`${ariaLabel}-placeholder`}>{selectPlaceholder}</span>
|
||||
{items.map((item) => (
|
||||
<button
|
||||
key={item.value}
|
||||
|
|
@ -67,12 +76,14 @@ function TestForm({
|
|||
defaultModel = '',
|
||||
defaultProvider = '',
|
||||
models,
|
||||
modelsError = false,
|
||||
modelsReady,
|
||||
providers = [{ label: 'Custom', value: 'custom' }],
|
||||
}: {
|
||||
defaultModel?: string;
|
||||
defaultProvider?: string;
|
||||
models: Record<string, string[]>;
|
||||
modelsError?: boolean;
|
||||
modelsReady: boolean;
|
||||
providers?: Array<{ label: string; value: string }>;
|
||||
}) {
|
||||
|
|
@ -89,6 +100,7 @@ function TestForm({
|
|||
<ModelPanel
|
||||
providers={providers}
|
||||
models={models}
|
||||
modelsError={modelsError}
|
||||
modelsReady={modelsReady}
|
||||
setActivePanel={jest.fn()}
|
||||
/>
|
||||
|
|
@ -164,4 +176,37 @@ describe('ModelPanel', () => {
|
|||
expect(localStorage.getItem('lastAgentProvider')).toBe('custom');
|
||||
expect(localStorage.getItem('lastAgentModel')).toBe('second-model');
|
||||
});
|
||||
|
||||
it('announces the pending catalogue instead of inviting a selection', () => {
|
||||
const { getByTestId } = render(
|
||||
<TestForm defaultProvider="custom" models={{}} modelsReady={false} />,
|
||||
);
|
||||
|
||||
expect(getByTestId('com_ui_model-placeholder')).toHaveTextContent('com_ui_loading');
|
||||
});
|
||||
|
||||
it('offers no models and explains the failure when the catalogue cannot be loaded', () => {
|
||||
const { getByRole, getByTestId, queryByTestId } = render(
|
||||
<TestForm defaultProvider="custom" models={{}} modelsError={true} modelsReady={true} />,
|
||||
);
|
||||
|
||||
expect(getByRole('alert')).toHaveTextContent('com_error_models_not_loaded');
|
||||
expect(queryByTestId('com_ui_model-placeholder')).toHaveTextContent('com_ui_select_model');
|
||||
expect(getByTestId('com_ui_provider-custom')).toBeDisabled();
|
||||
});
|
||||
|
||||
it('labels the provider and model controls', () => {
|
||||
const { container } = render(
|
||||
<TestForm
|
||||
defaultProvider="custom"
|
||||
models={{ custom: ['custom-model'] }}
|
||||
modelsReady={true}
|
||||
/>,
|
||||
);
|
||||
|
||||
expect(container.querySelector('label[for="provider"]')).not.toBeNull();
|
||||
expect(container.querySelector('label[for="model"]')).not.toBeNull();
|
||||
expect(container.querySelector('#provider')).not.toBeNull();
|
||||
expect(container.querySelector('#model')).not.toBeNull();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,7 +1,7 @@
|
|||
import React, { useMemo } from 'react';
|
||||
import keyBy from 'lodash/keyBy';
|
||||
import { ChevronLeft, RotateCcw } from 'lucide-react';
|
||||
import { Button, ControlCombobox } from '@librechat/client';
|
||||
import { Alert, Button, ControlCombobox } from '@librechat/client';
|
||||
import { useFormContext, useWatch, Controller } from 'react-hook-form';
|
||||
import {
|
||||
alternateName,
|
||||
|
|
@ -21,12 +21,26 @@ import { useLocalize } from '~/hooks';
|
|||
import { Panel } from '~/common';
|
||||
import { cn } from '~/utils';
|
||||
|
||||
function getModelPlaceholderKey(modelsPending: boolean, provider: string) {
|
||||
if (modelsPending) {
|
||||
return 'com_ui_loading';
|
||||
}
|
||||
if (provider) {
|
||||
return 'com_ui_select_model';
|
||||
}
|
||||
return 'com_ui_select_provider_first';
|
||||
}
|
||||
|
||||
export default function ModelPanel({
|
||||
providers,
|
||||
modelsError,
|
||||
setActivePanel,
|
||||
models: modelsData,
|
||||
modelsReady,
|
||||
}: Pick<AgentModelPanelProps, 'models' | 'modelsReady' | 'providers' | 'setActivePanel'>) {
|
||||
}: Pick<
|
||||
AgentModelPanelProps,
|
||||
'models' | 'modelsError' | 'modelsReady' | 'providers' | 'setActivePanel'
|
||||
>) {
|
||||
const localize = useLocalize();
|
||||
const { announcePolite } = useLiveAnnouncer();
|
||||
|
||||
|
|
@ -47,6 +61,8 @@ export default function ModelPanel({
|
|||
() => (provider ? (modelsData[provider] ?? []) : []),
|
||||
[modelsData, provider],
|
||||
);
|
||||
const modelsPending = !modelsReady && !modelsError;
|
||||
const selectionDisabled = !modelsReady || modelsError;
|
||||
|
||||
const { data: endpointsConfig = {} } = useGetEndpointsQuery();
|
||||
|
||||
|
|
@ -105,10 +121,13 @@ export default function ModelPanel({
|
|||
</header>
|
||||
<div>
|
||||
{/* Endpoint aka Provider for Agents */}
|
||||
<div className="mb-3">
|
||||
<div className="mb-3" aria-busy={modelsPending}>
|
||||
<label
|
||||
id="provider-label"
|
||||
className="mb-1 block text-[11px] font-medium uppercase tracking-wide text-text-secondary"
|
||||
className={cn(
|
||||
'mb-1 block text-[11px] font-medium uppercase tracking-wide text-text-secondary',
|
||||
modelsPending && 'opacity-60',
|
||||
)}
|
||||
htmlFor="provider"
|
||||
>
|
||||
{localize('com_ui_provider')} <span className="text-red-500">*</span>
|
||||
|
|
@ -130,6 +149,7 @@ export default function ModelPanel({
|
|||
return (
|
||||
<>
|
||||
<ControlCombobox
|
||||
selectId="provider"
|
||||
selectedValue={value}
|
||||
displayValue={alternateName[display] ?? display}
|
||||
selectPlaceholder={localize('com_ui_select_provider')}
|
||||
|
|
@ -154,7 +174,7 @@ export default function ModelPanel({
|
|||
}))}
|
||||
className={cn(error ? 'border-2 border-red-500' : '')}
|
||||
ariaLabel={localize('com_ui_provider')}
|
||||
disabled={!modelsReady}
|
||||
disabled={selectionDisabled}
|
||||
isCollapsed={false}
|
||||
showCarat={true}
|
||||
/>
|
||||
|
|
@ -169,12 +189,12 @@ export default function ModelPanel({
|
|||
/>
|
||||
</div>
|
||||
{/* Model */}
|
||||
<div className="mb-3">
|
||||
<div className="mb-3" aria-busy={modelsPending}>
|
||||
<label
|
||||
id="model-label"
|
||||
className={cn(
|
||||
'mb-1 block text-[11px] font-medium uppercase tracking-wide text-text-secondary',
|
||||
!provider && 'opacity-60',
|
||||
(!provider || modelsPending) && 'opacity-60',
|
||||
)}
|
||||
htmlFor="model"
|
||||
>
|
||||
|
|
@ -188,12 +208,9 @@ export default function ModelPanel({
|
|||
return (
|
||||
<>
|
||||
<ControlCombobox
|
||||
selectId="model"
|
||||
selectedValue={field.value || ''}
|
||||
selectPlaceholder={
|
||||
provider
|
||||
? localize('com_ui_select_model')
|
||||
: localize('com_ui_select_provider_first')
|
||||
}
|
||||
selectPlaceholder={localize(getModelPlaceholderKey(modelsPending, provider))}
|
||||
searchPlaceholder={localize('com_ui_select_model')}
|
||||
setValue={(value) => {
|
||||
field.onChange(value);
|
||||
|
|
@ -208,7 +225,7 @@ export default function ModelPanel({
|
|||
label: model,
|
||||
value: model,
|
||||
}))}
|
||||
disabled={!provider || !modelsReady}
|
||||
disabled={!provider || selectionDisabled}
|
||||
className={cn('disabled:opacity-50', error ? 'border-2 border-red-500' : '')}
|
||||
ariaLabel={localize('com_ui_model')}
|
||||
isCollapsed={false}
|
||||
|
|
@ -223,6 +240,11 @@ export default function ModelPanel({
|
|||
);
|
||||
}}
|
||||
/>
|
||||
{modelsError && (
|
||||
<Alert variant="error" className="mt-1">
|
||||
{localize('com_error_models_not_loaded')}
|
||||
</Alert>
|
||||
)}
|
||||
</div>
|
||||
</div>
|
||||
{/* Model Parameters */}
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue