scriptoriumadmin/litellm
1
0
Fork 0
mirror of https://github.com/BerriAI/litellm.git synced 2026-09-13 23:11:40 +00:00
Code Issues Projects Releases Packages Wiki Activity Actions 101

refactor(ui): presets are prefill-only, not an ongoing contract

Browse source 
A preset was being treated as a persistent identity that had to be re-verified against its own bundled model list at submit time (verifyPresetStillAvailable), separately from the tiers a caller actually built. That's wrong: handlePresetChange only ever prefills complexityRouterConfig once, and everything after that is edited exactly like Custom. Tier edits after applying a preset left selectedPreset unchanged, so the submit-time check was verifying the preset's original models, not the config actually being submitted; it could block a valid customized config or silently accept a manually-added model it never checked.

Deleted verifyPresetStillAvailable and the whole submit-time preset-recheck (with it, the async loading gap that made every round of the token-consistency findings possible in the first place - there is no longer an async step between the button click and form.validateFields). The tier selects already only ever offer models from modelInfo, so submitBlockedReason (tiers + keyword rules) is a complete, accurate check regardless of whether the config came from a preset or from scratch.

Also dropped the requirement to explicitly choose a template before submitting: a caller who fills in tiers manually without ever touching the Template dropdown is functionally identical to one who clicked "Custom Configuration" first, so gating on that distinction was friction with no safety benefit. Removed the decorative required asterisk and the inline "select a template" error along with it.

Fixed a latent test-order bug surfaced by this cleanup: several tests override getMissingTiersError with mockReturnValue(null), which vi.clearAllMocks() does not undo (it clears call history, not the implementation), so the override was leaking into whichever test ran next. beforeEach now restores the real implementation explicitly.

Net -198 LOC across the two files.
This commit is contained in:
Tin Chi Lo 2026-08-03 19:54:30 -07:00
parent 91513f38bc
commit 45e54e0003
2 changed files with 19 additions and 217 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

187
ui/litellm-dashboard/src/components/add_model/add_auto_router_tab.test.tsx
View file

@ -1,5 +1,5 @@
import { renderWithProviders, screen, waitFor, testQueryClient, within } from "../../../tests/test-utils";
import { act, fireEvent } from "@testing-library/react";
import { fireEvent } from "@testing-library/react";
import userEvent from "@testing-library/user-event";
import { vi } from "vitest";
import AddAutoRouterTab from "./add_auto_router_tab";
@ -55,10 +55,16 @@ vi.mock("../molecules/notifications_manager", () => ({
default: { fromBackend: vi.fn() },
}));
// Kept real by default so the "mandatory field" test still sees genuine tier validation; one
// test overrides it to reach the submit path without driving four tier selects.
// Kept real by default so the "mandatory field" test still sees genuine tier validation; several
// tests override it with mockReturnValue(null) to reach the submit path without driving four tier
// selects. vi.clearAllMocks() only clears call history, not that override, so it leaks into
// whichever test runs next unless beforeEach restores the real implementation explicitly.
const mocks = vi.hoisted(() => ({
realGetMissingTiersError: undefined as ((tiers: unknown) => string | null) | undefined,
}));
vi.mock("./build_complexity_router_config", async (importOriginal) => {
const actual = await importOriginal<typeof import("./build_complexity_router_config")>();
mocks.realGetMissingTiersError = actual.getMissingTiersError as (tiers: unknown) => string | null;
return { ...actual, getMissingTiersError: vi.fn(actual.getMissingTiersError) };
});
@ -115,6 +121,9 @@ const Harness = () => <AddAutoRouterTab handleOk={vi.fn()} accessToken="token" u
describe("AddAutoRouterTab", () => {
beforeEach(() => {
vi.clearAllMocks();
if (mocks.realGetMissingTiersError) {
vi.mocked(getMissingTiersError).mockImplementation(mocks.realGetMissingTiersError);
}
// testQueryClient is a shared singleton with staleTime: Infinity, so cached model lists would
// otherwise bleed across tests (a later test reusing accessToken="token" would read an earlier
// test's data instead of its own mock).
@ -209,128 +218,6 @@ describe("AddAutoRouterTab", () => {
expect(mockFetchAvailableModels).toHaveBeenCalledTimes(2);
});
// react-query keeps the last successful list when a later refetch fails, so a background
// refetch error must not treat an already-verified, still-cached preset as unverifiable in the
// UI: the dropdown stays selectable and the caller never sees a stale error just from a passive
// hiccup. Submit still forces its own fresh check (separately tested below); here that fresh
// check succeeds, representing the hiccup having been transient.
it("keeps a selected preset submit-reachable when a background refetch fails but cached models remain valid", async () => {
const user = userEvent.setup();
mockFetchAvailableModels
.mockResolvedValueOnce(ALL_FAMILY_MODELS)
.mockRejectedValueOnce(new Error("boom"))
.mockResolvedValueOnce(ALL_FAMILY_MODELS);
renderWithProviders(<Harness />);
openTemplateDropdown();
await waitFor(() => expect(isOptionDisabled(optionByLabel("Anthropic Family")!)).toBe(false));
fireEvent.click(optionByLabel("Anthropic Family")!);
await act(async () => {
await testQueryClient.refetchQueries({ queryKey: ["availableModels", "autoRouter", "token"] });
});
// The passive refetch failure must not have re-disabled the already-selected preset's option.
openTemplateDropdown();
expect(isOptionDisabled(optionByLabel("Anthropic Family")!)).toBe(false);
openTemplateDropdown();
await user.type(screen.getByPlaceholderText(/smart_router/i), "resilient-router");
await user.click(screen.getByRole("button", { name: /add auto router/i }));
await waitFor(() => expect(mockHandleAddAutoRouterSubmit).toHaveBeenCalled());
expect(NotificationManager.fromBackend).not.toHaveBeenCalledWith(
"This template's models are no longer available. Please reselect a template or switch to Custom.",
);
});
// The backend does not re-check a router's referenced model names against the caller's access,
// so submit is the only place that can catch a genuinely stale preset: force a fresh fetch right
// before creating the router rather than trusting the cache, and block if that fresh check can't
// confirm availability (a real outage, or the caller's access having actually narrowed).
it("blocks submit when a fresh re-check at submit time cannot confirm the preset's models", async () => {
const user = userEvent.setup();
mockFetchAvailableModels.mockResolvedValueOnce(ALL_FAMILY_MODELS).mockRejectedValue(new Error("boom"));
renderWithProviders(<Harness />);
openTemplateDropdown();
await waitFor(() => expect(isOptionDisabled(optionByLabel("Anthropic Family")!)).toBe(false));
fireEvent.click(optionByLabel("Anthropic Family")!);
await user.type(screen.getByPlaceholderText(/smart_router/i), "unconfirmed-router");
await user.click(screen.getByRole("button", { name: /add auto router/i }));
await waitFor(() =>
expect(NotificationManager.fromBackend).toHaveBeenCalledWith(
"This template's models are no longer available. Please reselect a template or switch to Custom.",
),
);
expect(mockHandleAddAutoRouterSubmit).not.toHaveBeenCalled();
});
// The fresh re-check is a real network round trip, not instant, so the submit button must show
// its own loading state (matching the Test Connection button's existing convention) rather than
// silently doing nothing until it settles. This also proves the button can't be clicked again
// mid-check.
it("shows a loading state on the submit button while the submit-time re-check is in flight", async () => {
const user = userEvent.setup();
let resolveRecheck: (models: ModelGroup[]) => void = () => undefined;
mockFetchAvailableModels
.mockResolvedValueOnce(ALL_FAMILY_MODELS)
.mockReturnValueOnce(new Promise<ModelGroup[]>((resolve) => (resolveRecheck = resolve)));
renderWithProviders(<Harness />);
openTemplateDropdown();
await waitFor(() => expect(isOptionDisabled(optionByLabel("Anthropic Family")!)).toBe(false));
fireEvent.click(optionByLabel("Anthropic Family")!);
await user.type(screen.getByPlaceholderText(/smart_router/i), "loading-state-router");
const submitButton = screen.getByRole("button", { name: /add auto router/i });
await user.click(submitButton);
await waitFor(() => expect(submitButton).toHaveClass("ant-btn-loading"));
await user.click(submitButton);
expect(mockFetchAvailableModels).toHaveBeenCalledTimes(2);
resolveRecheck(ALL_FAMILY_MODELS);
await waitFor(() => expect(submitButton).not.toHaveClass("ant-btn-loading"));
expect(mockHandleAddAutoRouterSubmit).toHaveBeenCalledTimes(1);
});
// A single in-flight submission must stay internally consistent even if accessToken rotates
// mid-flight: verifyPresetStillAvailable and the create call both receive the token captured
// once when submit started, so they can never end up verifying one caller's models and creating
// under another's identity. Reading a "latest" ref independently at each point can't fully close
// that gap (there's always a residual window between any two reads); one snapshot, used
// throughout, closes it completely.
it("keeps verification and creation on the same token even if it rotates mid-submission", async () => {
const user = userEvent.setup();
let resolveInitialRecheck: (models: ModelGroup[]) => void = () => undefined;
mockFetchAvailableModels
.mockResolvedValueOnce(ALL_FAMILY_MODELS)
.mockReturnValueOnce(new Promise<ModelGroup[]>((resolve) => (resolveInitialRecheck = resolve)));
const { rerender } = renderWithProviders(
<AddAutoRouterTab handleOk={vi.fn()} accessToken="stale-token" userRole="Admin" />,
);
openTemplateDropdown();
await waitFor(() => expect(isOptionDisabled(optionByLabel("Anthropic Family")!)).toBe(false));
fireEvent.click(optionByLabel("Anthropic Family")!);
await user.type(screen.getByPlaceholderText(/smart_router/i), "rotated-token-router");
await user.click(screen.getByRole("button", { name: /add auto router/i }));
await waitFor(() => expect(mockFetchAvailableModels).toHaveBeenCalledTimes(2));
expect(mockFetchAvailableModels.mock.calls[1][0]).toBe("stale-token");
// The token rotates while the re-check above is still in flight.
rerender(<AddAutoRouterTab handleOk={vi.fn()} accessToken="fresh-token" userRole="Admin" />);
resolveInitialRecheck(ALL_FAMILY_MODELS);
await waitFor(() => expect(mockHandleAddAutoRouterSubmit).toHaveBeenCalled());
expect(mockHandleAddAutoRouterSubmit.mock.calls.at(-1)?.[1]).toBe("stale-token");
});
// The headline behavior: selecting a preset must pre-fill the tier config so the created
// router carries the preset's models. Real tier validation runs here (getMissingTiersError is
// not stubbed), so if selection stopped pre-filling, the empty tiers would either block the
@ -393,26 +280,7 @@ describe("AddAutoRouterTab", () => {
await user.type(screen.getByPlaceholderText(/smart_router/i), "raced-router");
await user.click(screen.getByRole("button", { name: /add auto router/i }));
// No preset was applied and none was picked, so the template check blocks the submit.
expect(mockHandleAddAutoRouterSubmit).not.toHaveBeenCalled();
});
// The Template field carries a required marker but, until now, nothing actually validated it:
// submit fell through to the unrelated missing-tiers error instead. This pins a Template-specific
// block so a future regression (e.g. dropping this check) surfaces as a wrong error message, not
// silence.
it("blocks the submit and shows an inline error when no template is chosen", async () => {
const user = userEvent.setup();
mockFetchAvailableModels.mockResolvedValue(ALL_FAMILY_MODELS);
renderWithProviders(<Harness />);
await user.type(screen.getByPlaceholderText(/smart_router/i), "no-template-router");
await user.click(screen.getByRole("button", { name: /add auto router/i }));
expect(await screen.findByText("Please select a template")).toBeInTheDocument();
expect(NotificationManager.fromBackend).toHaveBeenCalledWith(
"Please select a template, or choose Custom Configuration",
);
// No preset was applied, so tiers are still empty and submitBlockedReason disables the button.
expect(mockHandleAddAutoRouterSubmit).not.toHaveBeenCalled();
});
@ -437,35 +305,6 @@ describe("AddAutoRouterTab", () => {
expect(isOptionDisabled(optionByLabel("OpenAI Family")!)).toBe(true);
});
// handlePresetChange only ever applies an available preset, but that guarantee can go stale by
// submit time: select under a caller with the full family, then switch to a caller missing one
// of its models. Nothing clears the selection (that would erase in-progress Custom edits too),
// so submit itself must re-verify against the current caller's list before creating the router.
it("blocks submit when the selected preset's models are no longer available for the current caller", async () => {
const user = userEvent.setup();
mockFetchAvailableModels
.mockResolvedValueOnce(ALL_FAMILY_MODELS)
.mockResolvedValueOnce(ALL_FAMILY_MODELS.filter((m) => m.model_group !== "o3"));
const { rerender } = renderWithProviders(
<AddAutoRouterTab handleOk={vi.fn()} accessToken="caller-a" userRole="Admin" />,
);
openTemplateDropdown();
await waitFor(() => expect(isOptionDisabled(optionByLabel("OpenAI Family")!)).toBe(false));
fireEvent.click(optionByLabel("OpenAI Family")!);
rerender(<AddAutoRouterTab handleOk={vi.fn()} accessToken="caller-b" userRole="Admin" />);
await waitFor(() => expect(mockFetchAvailableModels).toHaveBeenCalledTimes(2));
await user.type(screen.getByPlaceholderText(/smart_router/i), "stale-preset-router");
await user.click(screen.getByRole("button", { name: /add auto router/i }));
expect(NotificationManager.fromBackend).toHaveBeenCalledWith(
"This template's models are no longer available. Please reselect a template or switch to Custom.",
);
expect(mockHandleAddAutoRouterSubmit).not.toHaveBeenCalled();
});
// Prefill must preserve a preset's deliberately-falsy fields (a 0 match threshold, an empty
// escalation list). Using `||` instead of `??` would swap the 0 for the create-form default and
// re-enable escalation the preset meant to turn off, so this asserts the exact submitted values.

49
ui/litellm-dashboard/src/components/add_model/add_auto_router_tab.tsx
View file

@ -7,7 +7,7 @@ import { all_admin_roles } from "@/utils/roles";
import { type ModelWriteScope } from "@/utils/modelPermissions";
import TeamDropdown from "../common_components/team_dropdown";
import { handleAddAutoRouterSubmit } from "./handle_add_auto_router_submit";
import { fetchAvailableModels, ModelGroup } from "@/components/llm_calls/fetch_models";
import { fetchAvailableModels } from "@/components/llm_calls/fetch_models";
import ComplexityRouterConfig, {
ComplexityRouterConfigValue,
DEFAULT_ADAPTIVE_WEIGHTS,
@ -193,48 +193,16 @@ const AddAutoRouterTab: React.FC<AddAutoRouterTabProps> = ({
setEscalationKeywords(config.escalation_keywords ?? DEFAULT_ESCALATION_KEYWORDS);
};
// The dropdown and handlePresetChange trust the cached model list, so a caller's access can
// narrow without ever being reflected here (nothing invalidates a preset already applied, and a
// background refetch failure keeps trusting the stale cache by design - see modelsUnverifiable
// above). The backend does not re-check a router's referenced model names against the caller's
// access either, so this is the only place that can catch it: force a fresh fetch right before
// creating the router, rather than trusting whatever's cached. Takes the token as a parameter,
// captured once by the caller, so this check and the create call it gates can never end up
// disagreeing about which caller they represent - reading accessToken independently at each
// point only chases a moving target and can never fully close that gap.
const verifyPresetStillAvailable = async (presetKey: string, token: string): Promise<boolean> => {
const preset = getPresetByKey(presetKey);
if (!preset) return false;
let freshModels: ModelGroup[];
try {
freshModels = await fetchAvailableModels(token);
} catch {
return false;
}
const freshSet = new Set(freshModels.map((m) => m.model_group));
return getMissingModelsInPreset(preset, freshSet).length === 0;
};
// Why the submit is unavailable, or null when it is available. The button reads this to disable
// itself and to say what is missing, so the two can never give different answers.
const submitBlockedReason =
getMissingTiersError(complexityRouterConfig.tiers) ?? getKeywordTierRulesError(keywordTierRules);
// A preset only ever prefills complexityRouterConfig at selection time (handlePresetChange);
// after that it's edited exactly like Custom, so there is nothing preset-specific left to verify
// at submit. The tier selects already only ever offer models from modelInfo, so submitBlockedReason
// covering the actual config is the whole check, regardless of how it got there.
const submitRecommendedRouter = async (name: string) => {
if (!selectedPreset) {
setShowValidationErrors(true);
NotificationManager.fromBackend("Please select a template, or choose Custom Configuration");
return;
}
if (selectedPreset !== "custom" && !(await verifyPresetStillAvailable(selectedPreset, accessToken))) {
setShowValidationErrors(true);
NotificationManager.fromBackend(
"This template's models are no longer available. Please reselect a template or switch to Custom.",
);
return;
}
const {
tiers,
classifier_type: classifierType,
@ -373,9 +341,7 @@ const AddAutoRouterTab: React.FC<AddAutoRouterTabProps> = ({
labelAlign="left"
>
<div className="mb-6">
<label className="block text-sm font-medium text-gray-900 mb-2">
Template <span className="text-red-500">*</span>
</label>
<label className="block text-sm font-medium text-gray-900 mb-2">Template</label>
<AntdSelect
value={selectedPreset}
onChange={handlePresetChange}
@ -413,9 +379,6 @@ const AddAutoRouterTab: React.FC<AddAutoRouterTabProps> = ({
);
})}
</AntdSelect>
{showValidationErrors && !selectedPreset && (
<div className="text-xs mt-1 text-red-500">Please select a template</div>
)}
{modelsUnverifiable && (
<div className="text-xs mt-1 text-red-500">
Could not load available models.{" "}