mirror of
https://github.com/BerriAI/litellm.git
synced 2026-10-09 03:18:44 +00:00
fix(ui): harden the Add Provider wizard against three failure modes
A council reviewed the four findings the review bots raised on the wizard and ruled these three worth fixing before human review, since all of them are in code this PR introduces. Creation no longer treats a failed deployment lookup as an empty list. That lookup is what makes a retry skip rows already created, so swallowing its error turned a partial-failure retry into duplicate deployments. It now stops and says why, with the rows still on screen. Whether the wizard creates or updates a credential is derived from the name it actually saved rather than a flag that was set once and never cleared. Renaming after a save used to take the update path against a name the server had never seen, which dead-ended the flow with no way forward but a reload. Create is disabled while any model name is blank, instead of submitting an empty model_name and producing a deployment that cannot be addressed. Each fix carries a regression test that fails when the fix is reverted.
This commit is contained in:
parent
576ada8a70
commit
bacf9c9ad0
3 changed files with 99 additions and 7 deletions
|
|
@ -333,4 +333,80 @@ describe("AddProviderPanel", () => {
|
|||
);
|
||||
expect(await screen.findByText("claude-3-opus")).toBeInTheDocument();
|
||||
});
|
||||
it("refuses to create when the deployment lookup fails, rather than duplicating saved rows", async () => {
|
||||
discoverProviderModelsCall.mockResolvedValue({ models: ["claude-3-opus"] });
|
||||
listAllModelsCall.mockRejectedValue(new Error("proxy unreachable"));
|
||||
const { user } = await setup();
|
||||
|
||||
await chooseProvider(user, "Anthropic");
|
||||
await user.type(screen.getByLabelText("Credential name"), "anthropic-prod");
|
||||
await user.click(screen.getByRole("button", { name: /Next/ }));
|
||||
await user.type(await screen.findByLabelText("API Key"), "sk-ant-test");
|
||||
await user.click(screen.getByRole("button", { name: "Save credential" }));
|
||||
|
||||
await screen.findByText("claude-3-opus");
|
||||
await user.click(screen.getByRole("button", { name: "Create 1 model" }));
|
||||
|
||||
expect(await screen.findByText(/could duplicate ones already saved/i)).toBeInTheDocument();
|
||||
expect(createProviderModelCall).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("creates rather than PATCHes after the credential name changes following a save", async () => {
|
||||
discoverProviderModelsCall.mockResolvedValue({ models: ["claude-3-opus"] });
|
||||
const { user } = await setup();
|
||||
|
||||
await chooseProvider(user, "Anthropic");
|
||||
await user.type(screen.getByLabelText("Credential name"), "anthropic-first");
|
||||
await user.click(screen.getByRole("button", { name: /Next/ }));
|
||||
|
||||
await chooseSelectOption(
|
||||
user,
|
||||
await screen.findByRole("combobox", { name: "Authentication method" }),
|
||||
"Workload Identity Federation (LiteLLM-signed)",
|
||||
);
|
||||
fireEvent.change(await screen.findByLabelText("Organization ID"), { target: { value: "org-1" } });
|
||||
fireEvent.change(screen.getByLabelText("Issuer URL"), { target: { value: "https://proxy.example.com" } });
|
||||
fireEvent.change(screen.getByLabelText("Issuer Subject"), { target: { value: "litellm-proxy" } });
|
||||
fireEvent.change(screen.getByLabelText("Signing Key Reference"), { target: { value: "os.environ/SIGNING_KEY" } });
|
||||
await user.click(screen.getByRole("button", { name: "Save credential" }));
|
||||
|
||||
// The JWKS step is the one place the wizard pauses after a save, so it is the route back to
|
||||
// the name field. Renaming there must create the new credential, never PATCH the old name.
|
||||
await screen.findByText("Register this JWKS with Anthropic");
|
||||
await user.click(screen.getByRole("button", { name: /Back/ }));
|
||||
await user.click(await screen.findByRole("button", { name: /Back/ }));
|
||||
|
||||
const nameInput = await screen.findByLabelText("Credential name");
|
||||
await user.clear(nameInput);
|
||||
await user.type(nameInput, "anthropic-second");
|
||||
await user.click(screen.getByRole("button", { name: /Next/ }));
|
||||
|
||||
credentialCreateCall.mockClear();
|
||||
credentialUpdateCall.mockClear();
|
||||
await user.click(await screen.findByRole("button", { name: /Save credential/ }));
|
||||
|
||||
await waitFor(() =>
|
||||
expect(credentialCreateCall).toHaveBeenCalledWith(
|
||||
"test-access-token",
|
||||
expect.objectContaining({ credential_name: "anthropic-second" }),
|
||||
),
|
||||
);
|
||||
expect(credentialUpdateCall).not.toHaveBeenCalled();
|
||||
});
|
||||
|
||||
it("blocks creation while any model name is blank", async () => {
|
||||
discoverProviderModelsCall.mockResolvedValue({ models: ["claude-3-opus"] });
|
||||
const { user } = await setup();
|
||||
|
||||
await chooseProvider(user, "Anthropic");
|
||||
await user.type(screen.getByLabelText("Credential name"), "anthropic-prod");
|
||||
await user.click(screen.getByRole("button", { name: /Next/ }));
|
||||
await user.type(await screen.findByLabelText("API Key"), "sk-ant-test");
|
||||
await user.click(screen.getByRole("button", { name: "Save credential" }));
|
||||
|
||||
const nameCell = await screen.findByDisplayValue("claude-3-opus");
|
||||
await user.clear(nameCell);
|
||||
|
||||
expect(screen.getByRole("button", { name: /Create 1 model/ })).toBeDisabled();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -90,12 +90,13 @@ export default function AddProviderPanel() {
|
|||
const [step, setStep] = React.useState<WizardStep>("provider");
|
||||
const [selectedProvider, setSelectedProvider] = React.useState<Providers | null>(null);
|
||||
const [credentialName, setCredentialName] = React.useState("");
|
||||
const [credentialSaved, setCredentialSaved] = React.useState(false);
|
||||
const [savedCredentialName, setSavedCredentialName] = React.useState<string | null>(null);
|
||||
const [savedValues, setSavedValues] = React.useState<Record<string, unknown>>({});
|
||||
const [federationRuleId, setFederationRuleId] = React.useState("");
|
||||
const [jwks, setJwks] = React.useState<AnthropicJwks | null>(null);
|
||||
const [jwksError, setJwksError] = React.useState<string | null>(null);
|
||||
const [discoveryError, setDiscoveryError] = React.useState<string | null>(null);
|
||||
const [createError, setCreateError] = React.useState<string | null>(null);
|
||||
const [isDiscovering, setIsDiscovering] = React.useState(false);
|
||||
const [rows, setRows] = React.useState<DiscoveredModelRow[]>([]);
|
||||
const [isCreating, setIsCreating] = React.useState(false);
|
||||
|
|
@ -124,8 +125,11 @@ export default function AddProviderPanel() {
|
|||
);
|
||||
const litellmProvider = selectedProviderInfo?.litellm_provider ?? "";
|
||||
|
||||
const credentialSaved = savedCredentialName !== null && savedCredentialName === credentialName;
|
||||
|
||||
const nameCollision =
|
||||
credentialName.length > 0 &&
|
||||
credentialName !== savedCredentialName &&
|
||||
(credentialsResponse?.credentials ?? []).some((c) => c.credential_name === credentialName);
|
||||
|
||||
const goTo = (next: WizardStep) => setStep(next);
|
||||
|
|
@ -160,7 +164,7 @@ export default function AddProviderPanel() {
|
|||
await credentialUpdateCall(accessToken, credentialName, updatePayload);
|
||||
}
|
||||
setSavedValues(values);
|
||||
setCredentialSaved(true);
|
||||
setSavedCredentialName(credentialName);
|
||||
setFederationRuleId(
|
||||
typeof values.anthropic_federation_rule_id === "string" ? values.anthropic_federation_rule_id : "",
|
||||
);
|
||||
|
|
@ -231,13 +235,19 @@ export default function AddProviderPanel() {
|
|||
setIsCreating(true);
|
||||
setCreationResults([]);
|
||||
setAliasCollisions([]);
|
||||
setCreateError(null);
|
||||
goTo("creating");
|
||||
|
||||
let existing: DeploymentInfoRow[] = [];
|
||||
let existing: DeploymentInfoRow[];
|
||||
try {
|
||||
existing = (await listAllModelsCall(accessToken)).data;
|
||||
} catch {
|
||||
existing = [];
|
||||
} catch (error) {
|
||||
setIsCreating(false);
|
||||
setCreateError(
|
||||
`Could not read the existing deployments, so creating now could duplicate ones already saved. ${extractProxyErrorMessage(error)}`,
|
||||
);
|
||||
goTo("review");
|
||||
return;
|
||||
}
|
||||
const pending = rowsPendingCreation(rows, litellmProvider, credentialName, existing);
|
||||
const pendingIds = new Set(pending.map((r) => r.id));
|
||||
|
|
@ -346,6 +356,7 @@ export default function AddProviderPanel() {
|
|||
<ReviewModelsStep
|
||||
rows={rows}
|
||||
setRows={setRows}
|
||||
createError={createError}
|
||||
onBack={() => {
|
||||
goTo("discover");
|
||||
void runDiscovery();
|
||||
|
|
|
|||
|
|
@ -13,13 +13,16 @@ import { buildManualRow, type DiscoveredModelRow } from "./wizardLogic";
|
|||
interface ReviewModelsStepProps {
|
||||
rows: DiscoveredModelRow[];
|
||||
setRows: React.Dispatch<React.SetStateAction<DiscoveredModelRow[]>>;
|
||||
createError: string | null;
|
||||
onBack: () => void;
|
||||
onCreateModels: () => void;
|
||||
}
|
||||
|
||||
const ReviewModelsStep: React.FC<ReviewModelsStepProps> = ({ rows, setRows, onBack, onCreateModels }) => {
|
||||
const ReviewModelsStep: React.FC<ReviewModelsStepProps> = ({ rows, setRows, createError, onBack, onCreateModels }) => {
|
||||
const [manualId, setManualId] = React.useState("");
|
||||
|
||||
const hasBlankName = rows.some((row) => row.modelName.trim() === "");
|
||||
|
||||
const updateRow = (id: string, patch: Partial<DiscoveredModelRow>) =>
|
||||
setRows((current) => current.map((row) => (row.id === id ? { ...row, ...patch } : row)));
|
||||
|
||||
|
|
@ -113,11 +116,13 @@ const ReviewModelsStep: React.FC<ReviewModelsStepProps> = ({ rows, setRows, onBa
|
|||
</Button>
|
||||
</div>
|
||||
|
||||
{createError && <p className="text-sm text-destructive">{createError}</p>}
|
||||
|
||||
<div className="flex justify-between">
|
||||
<Button type="button" variant="outline" onClick={onBack}>
|
||||
<ArrowLeft className="mr-1 size-4" /> Back
|
||||
</Button>
|
||||
<Button disabled={rows.length === 0} onClick={onCreateModels}>
|
||||
<Button disabled={rows.length === 0 || hasBlankName} onClick={onCreateModels}>
|
||||
Create {rows.length} model{rows.length === 1 ? "" : "s"}
|
||||
</Button>
|
||||
</div>
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue