mirror of
https://github.com/BerriAI/litellm.git
synced 2026-08-28 05:25:59 +00:00
fix(ui): stop the Add Model mapping table from looping the page (#37741)
Entering a custom model name on the Add Model form crashed the whole page to "This page couldn't load" (React error #185, maximum update depth exceeded), taking the provider credential fields down with it, so the model could never be created. ConditionalPublicModelName kept a `tableKey` counter and bumped it from an effect on every run to force the mappings table to remount. That was harmless under antd, whose useWatch handed back the stored array. React Hook Form's useWatch returns a fresh array each render, so the effect's dependency changed every render, the effect bumped state again, and the render loop never settled. The table is driven by its `data` prop, so the remount counter buys nothing: drop it, key the effects off the selection contents rather than the array identity, and write model_mappings only when they actually change. The two `react-hooks/set-state-in-effect` suppressions on this file, which were recording exactly this bug, go with it.
This commit is contained in:
parent
52403d7a8d
commit
cb89c7aa8f
3 changed files with 58 additions and 12 deletions
|
|
@ -1468,9 +1468,6 @@
|
|||
},
|
||||
"local/no-complex-jsx-arrow": {
|
||||
"count": 1
|
||||
},
|
||||
"react-hooks/set-state-in-effect": {
|
||||
"count": 2
|
||||
}
|
||||
},
|
||||
"src/components/add_model/handle_add_auto_router_submit.tsx": {
|
||||
|
|
|
|||
|
|
@ -1,8 +1,28 @@
|
|||
import { render, screen } from "@testing-library/react";
|
||||
import React, { useEffect, useRef } from "react";
|
||||
import { useFormContext, useWatch } from "react-hook-form";
|
||||
import { describe, expect, it } from "vitest";
|
||||
import { MountedFormHost } from "../../../tests/mounted-form-host";
|
||||
import type { MountedFormValues } from "../common_components/MountedFormField";
|
||||
import ConditionalPublicModelName from "./conditional_public_model_name";
|
||||
|
||||
const WRITE_BUDGET = 20;
|
||||
|
||||
const LoopGuard: React.FC = () => {
|
||||
const form = useFormContext<MountedFormValues>();
|
||||
const mappings = useWatch({ control: form.control, name: "model_mappings" });
|
||||
const writes = useRef(0);
|
||||
|
||||
useEffect(() => {
|
||||
writes.current += 1;
|
||||
if (writes.current > WRITE_BUDGET) {
|
||||
throw new Error(`model_mappings changed ${WRITE_BUDGET}+ times: the mapping effects are looping`);
|
||||
}
|
||||
}, [mappings]);
|
||||
|
||||
return null;
|
||||
};
|
||||
|
||||
describe("ConditionalPublicModelName", () => {
|
||||
it("should render", () => {
|
||||
render(
|
||||
|
|
@ -25,4 +45,28 @@ describe("ConditionalPublicModelName", () => {
|
|||
expect(screen.getByText("Public Model Name")).toBeInTheDocument();
|
||||
expect(screen.getByText("LiteLLM Model Name")).toBeInTheDocument();
|
||||
});
|
||||
|
||||
it("settles after rewriting the custom placeholder mapping to the entered model name", () => {
|
||||
render(
|
||||
<MountedFormHost
|
||||
defaultValues={{
|
||||
model: ["custom"],
|
||||
custom_model_name: "my-custom-model",
|
||||
model_mappings: [
|
||||
{
|
||||
public_name: "custom",
|
||||
litellm_model: "custom",
|
||||
},
|
||||
],
|
||||
}}
|
||||
>
|
||||
<ConditionalPublicModelName />
|
||||
<LoopGuard />
|
||||
</MountedFormHost>,
|
||||
);
|
||||
|
||||
expect(screen.getByDisplayValue("my-custom-model")).toBeInTheDocument();
|
||||
expect(screen.getByText("my-custom-model")).toBeInTheDocument();
|
||||
expect(screen.queryByDisplayValue("custom")).not.toBeInTheDocument();
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -1,4 +1,4 @@
|
|||
import React, { useEffect, useState } from "react";
|
||||
import React, { useEffect, useMemo } from "react";
|
||||
import type { ColumnDef } from "@tanstack/react-table";
|
||||
import { useFormContext, useWatch } from "react-hook-form";
|
||||
import { DataTable } from "@/components/shared/DataTable";
|
||||
|
|
@ -13,6 +13,13 @@ interface ModelMapping {
|
|||
litellm_model: string;
|
||||
}
|
||||
|
||||
const sameMappings = (left: readonly ModelMapping[], right: readonly ModelMapping[]): boolean =>
|
||||
left.length === right.length &&
|
||||
left.every(
|
||||
(mapping, index) =>
|
||||
mapping.public_name === right[index].public_name && mapping.litellm_model === right[index].litellm_model,
|
||||
);
|
||||
|
||||
const modelMappingsRule = {
|
||||
validator: async (_: unknown, value: unknown) => {
|
||||
if (!value || (value as ModelMapping[]).length === 0) {
|
||||
|
|
@ -29,15 +36,14 @@ const modelMappingsRule = {
|
|||
|
||||
const ConditionalPublicModelName: React.FC = () => {
|
||||
const form = useFormContext<MountedFormValues>();
|
||||
const [tableKey, setTableKey] = useState(0); // Add a key to force table re-render
|
||||
|
||||
// Watch the 'model' field for changes and ensure it's always an array
|
||||
const modelValue = useWatch({ control: form.control, name: "model" }) || [];
|
||||
const selectedModels = Array.isArray(modelValue) ? modelValue : [modelValue];
|
||||
const selectionKey = JSON.stringify(Array.isArray(modelValue) ? modelValue : [modelValue]);
|
||||
const selectedModels = useMemo(() => JSON.parse(selectionKey) as string[], [selectionKey]);
|
||||
const customModelName = useWatch({ control: form.control, name: "custom_model_name" }) as string | undefined;
|
||||
const showPublicModelName = !selectedModels.includes("all-wildcard");
|
||||
const selectedProvider = useWatch({ control: form.control, name: "custom_llm_provider" });
|
||||
// Force table to re-render when custom model name changes
|
||||
|
||||
useEffect(() => {
|
||||
if (customModelName && selectedModels.includes("custom")) {
|
||||
const currentMappings = (form.getValues("model_mappings") as ModelMapping[]) || [];
|
||||
|
|
@ -56,8 +62,9 @@ const ConditionalPublicModelName: React.FC = () => {
|
|||
}
|
||||
return mapping;
|
||||
});
|
||||
form.setValue("model_mappings", updatedMappings);
|
||||
setTableKey((prev) => prev + 1); // Force table re-render
|
||||
if (!sameMappings(currentMappings, updatedMappings)) {
|
||||
form.setValue("model_mappings", updatedMappings);
|
||||
}
|
||||
}
|
||||
}, [customModelName, selectedModels, selectedProvider, form]);
|
||||
|
||||
|
|
@ -109,7 +116,6 @@ const ConditionalPublicModelName: React.FC = () => {
|
|||
});
|
||||
|
||||
form.setValue("model_mappings", mappings);
|
||||
setTableKey((prev) => prev + 1); // Force table re-render
|
||||
}
|
||||
}
|
||||
}, [selectedModels, customModelName, selectedProvider, form]);
|
||||
|
|
@ -210,7 +216,6 @@ const ConditionalPublicModelName: React.FC = () => {
|
|||
>
|
||||
{(control) => (
|
||||
<DataTable
|
||||
key={tableKey} // Add key to force re-render
|
||||
data={(control.value as ModelMapping[] | undefined) ?? []}
|
||||
columns={columns}
|
||||
getRowId={(row) => row.litellm_model}
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue