From 5e2d6addc41e9ae19c2bc63d4916f08bf86e3d06 Mon Sep 17 00:00:00 2001 From: yuneng-jiang Date: Tue, 18 Aug 2026 11:42:12 -0700 Subject: [PATCH] refactor(ui): migrate user, logging and policy forms to react-hook-form and shadcn (#37303) * fix(ui): stop the policy modal cancelling its own save The Create Policy and Update Policy buttons are Tremor buttons rendered inside an antd Form. Tremor does not set a type, so both default to type="submit" and a click ran two things at once: handleSubmit's own form.validateFields(), and rc-field-form's onSubmit, which calls formInstance.submit() and validates a second time. rc-field-form keeps only the newest validation promise, so the first one resolved as outOfDate and rejected with an empty errorFields list. handleSubmit read that as a failure, so it never called createPolicy or updatePolicy and instead reported "Failed to save policy". Creating and editing a simple policy from the UI could not succeed. Marking both footer buttons type="button" leaves the submit path solely with handleSubmit. The new test file pins the request bodies for create and update, and fails without this change. * refactor(ui): migrate user, logging and policy forms to react-hook-form and shadcn Moves four antd Form units onto react-hook-form plus the shadcn kit and semantic color tokens, keeping every submit payload byte-identical. - Settings/AdminSettings/LoggingSettings - CreateUserButton - users/_components/user_edit_view - policies/_components/add_policy_form * chore(ui): drop the eslint suppressions the migrated forms no longer need * fix(ui): keep users with null optional fields editable The proxy returns null rather than omitting user_alias, user_role, budget_duration and metadata, and the new zod shape only allowed undefined, so opening any such user and saving failed validation. --- ui/litellm-dashboard/eslint-suppressions.json | 10 +- .../_components/add_policy_form.test.tsx | 207 ++++++ .../policies/_components/add_policy_form.tsx | 645 +++++++++--------- .../users/_components/user_edit_view.test.tsx | 175 +++++ .../users/_components/user_edit_view.tsx | 463 +++++++------ .../src/components/CreateUserButton.test.tsx | 218 ++++++ .../src/components/CreateUserButton.tsx | 519 ++++++++------ .../LoggingSettings/LoggingSettings.test.tsx | 80 +++ .../LoggingSettings/LoggingSettings.tsx | 231 +++++-- 9 files changed, 1727 insertions(+), 821 deletions(-) create mode 100644 ui/litellm-dashboard/src/app/(dashboard)/policies/_components/add_policy_form.test.tsx diff --git a/ui/litellm-dashboard/eslint-suppressions.json b/ui/litellm-dashboard/eslint-suppressions.json index e83c2d678ed..abc83120e16 100644 --- a/ui/litellm-dashboard/eslint-suppressions.json +++ b/ui/litellm-dashboard/eslint-suppressions.json @@ -1107,10 +1107,7 @@ "count": 1 }, "no-restricted-imports": { - "count": 2 - }, - "prefer-const": { - "count": 2 + "count": 1 }, "react-hooks/immutability": { "count": 2 @@ -1516,9 +1513,6 @@ "local/filename-pascal-case": { "count": 1 }, - "no-restricted-imports": { - "count": 2 - }, "react-hooks/set-state-in-effect": { "count": 1 } @@ -1678,7 +1672,7 @@ }, "src/components/CreateUserButton.tsx": { "no-restricted-imports": { - "count": 2 + "count": 1 }, "react-hooks/set-state-in-effect": { "count": 1 diff --git a/ui/litellm-dashboard/src/app/(dashboard)/policies/_components/add_policy_form.test.tsx b/ui/litellm-dashboard/src/app/(dashboard)/policies/_components/add_policy_form.test.tsx new file mode 100644 index 00000000000..abf409a0cba --- /dev/null +++ b/ui/litellm-dashboard/src/app/(dashboard)/policies/_components/add_policy_form.test.tsx @@ -0,0 +1,207 @@ +import { cleanup, screen, waitFor } from "@testing-library/react"; +import userEvent from "@testing-library/user-event"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import { renderWithProviders } from "../../../../../tests/test-utils"; +import type { Policy } from "@/components/policies/types"; +import AddPolicyForm from "./add_policy_form"; + +vi.mock("@/components/networking", () => ({ + getResolvedGuardrails: vi.fn().mockResolvedValue({ resolved_guardrails: [] }), + modelAvailableCall: vi.fn().mockResolvedValue({ data: [{ id: "gpt-4" }] }), +})); + +vi.mock("@/components/molecules/notifications_manager", () => ({ + default: { success: vi.fn(), fromBackend: vi.fn() }, +})); + +vi.mock("@/app/(dashboard)/hooks/useAuthorized", () => ({ + default: vi.fn().mockReturnValue({ userId: "u1", userRole: "Admin" }), +})); + +const EXISTING_POLICY: Policy = { + policy_id: "pol-1", + policy_name: "existing-policy", + inherit: "parent-policy", + description: "an existing policy", + guardrails_add: ["guard-a"], + guardrails_remove: ["guard-b"], + condition: { model: "gpt-4" }, +}; + +const PARENT_POLICY: Policy = { + policy_id: "pol-parent", + policy_name: "parent-policy", + inherit: null, + description: null, + guardrails_add: ["guard-c"], + guardrails_remove: [], + condition: null, +}; + +describe("AddPolicyForm", () => { + const createPolicy = vi.fn().mockResolvedValue({}); + const updatePolicy = vi.fn().mockResolvedValue({}); + + const defaultProps = { + visible: true, + onClose: vi.fn(), + onSuccess: vi.fn(), + onOpenFlowBuilder: vi.fn(), + accessToken: "test-token", + existingPolicies: [PARENT_POLICY, EXISTING_POLICY], + availableGuardrails: [ + { guardrail_id: "g-a", guardrail_name: "guard-a" }, + { guardrail_id: "g-b", guardrail_name: "guard-b" }, + { guardrail_id: "g-c", guardrail_name: "guard-c" }, + ] as never, + createPolicy, + updatePolicy, + }; + + beforeEach(() => { + vi.clearAllMocks(); + }); + + afterEach(() => { + cleanup(); + }); + + const enterSimpleForm = async (user: ReturnType) => { + await user.click(await screen.findByRole("button", { name: "Create Policy" })); + }; + + it("should send exactly six keys with empty-to-undefined and empty-to-array defaults on create", async () => { + const user = userEvent.setup(); + renderWithProviders(); + await enterSimpleForm(user); + + await user.type(await screen.findByLabelText("Policy Name"), "brand-new-policy"); + await user.click(screen.getByRole("button", { name: "Create Policy" })); + + await waitFor(() => { + expect(createPolicy).toHaveBeenCalled(); + }); + const payload = createPolicy.mock.calls[0][1]; + expect(Object.keys(payload).sort()).toEqual([ + "condition", + "description", + "guardrails_add", + "guardrails_remove", + "inherit", + "policy_name", + ]); + expect(payload).toStrictEqual({ + policy_name: "brand-new-policy", + description: undefined, + inherit: undefined, + guardrails_add: [], + guardrails_remove: [], + condition: undefined, + }); + }); + + it("should collapse a blank description to undefined rather than an empty string", async () => { + const user = userEvent.setup(); + renderWithProviders(); + await enterSimpleForm(user); + + const description = await screen.findByLabelText("Description"); + await user.type(description, "x"); + await user.clear(description); + await user.type(await screen.findByLabelText("Policy Name"), "blank-description"); + await user.click(screen.getByRole("button", { name: "Create Policy" })); + + await waitFor(() => { + expect(createPolicy).toHaveBeenCalled(); + }); + expect(createPolicy.mock.calls[0][1].description).toBeUndefined(); + }); + + it("should send the seeded policy through updatePolicy with condition wrapped in a model object", async () => { + const user = userEvent.setup(); + renderWithProviders(); + + await user.click(await screen.findByRole("button", { name: "Update Policy" })); + + await waitFor(() => { + expect(updatePolicy).toHaveBeenCalled(); + }); + expect(updatePolicy.mock.calls[0][0]).toBe("test-token"); + expect(updatePolicy.mock.calls[0][1]).toBe("pol-1"); + expect(updatePolicy.mock.calls[0][2]).toStrictEqual({ + policy_name: "existing-policy", + description: "an existing policy", + inherit: "parent-policy", + guardrails_add: ["guard-a"], + guardrails_remove: ["guard-b"], + condition: { model: "gpt-4" }, + }); + expect(createPolicy).not.toHaveBeenCalled(); + }); + + it("should keep the policy name field disabled while editing", async () => { + renderWithProviders(); + + expect(await screen.findByLabelText("Policy Name")).toBeDisabled(); + }); + + it("should block submission and call neither api when the policy name is missing", async () => { + const user = userEvent.setup(); + renderWithProviders(); + await enterSimpleForm(user); + + await user.click(await screen.findByRole("button", { name: "Create Policy" })); + + expect(await screen.findByText("Please enter a policy name")).toBeInTheDocument(); + expect(createPolicy).not.toHaveBeenCalled(); + expect(updatePolicy).not.toHaveBeenCalled(); + }); + + it("should block submission when the policy name has characters outside the allowed set", async () => { + const user = userEvent.setup(); + renderWithProviders(); + await enterSimpleForm(user); + + await user.type(await screen.findByLabelText("Policy Name"), "not a valid name!"); + await user.click(screen.getByRole("button", { name: "Create Policy" })); + + expect( + await screen.findByText("Policy name can only contain letters, numbers, hyphens, and underscores"), + ).toBeInTheDocument(); + expect(createPolicy).not.toHaveBeenCalled(); + }); + + it("should swap the model condition label and clear the value when the condition type changes", async () => { + const user = userEvent.setup(); + renderWithProviders(); + + expect(await screen.findByLabelText("Model (Optional)")).toBeInTheDocument(); + + await user.click(screen.getByRole("radio", { name: "Custom Regex Pattern" })); + + const regexField = await screen.findByLabelText("Regex Pattern (Optional)"); + expect(regexField).toHaveValue(""); + expect(screen.queryByLabelText("Model (Optional)")).not.toBeInTheDocument(); + + await user.click(screen.getByRole("button", { name: "Update Policy" })); + + await waitFor(() => { + expect(updatePolicy).toHaveBeenCalled(); + }); + expect(updatePolicy.mock.calls[0][2].condition).toBeUndefined(); + }); + + it("should open the flow builder instead of the simple form when that mode is confirmed", async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + const onOpenFlowBuilder = vi.fn(); + renderWithProviders(); + + await user.click(await screen.findByText("Flow Builder")); + await user.click(screen.getByRole("button", { name: "Continue to Builder" })); + + expect(onOpenFlowBuilder).toHaveBeenCalledTimes(1); + expect(onClose).toHaveBeenCalledTimes(1); + expect(createPolicy).not.toHaveBeenCalled(); + }); +}); diff --git a/ui/litellm-dashboard/src/app/(dashboard)/policies/_components/add_policy_form.tsx b/ui/litellm-dashboard/src/app/(dashboard)/policies/_components/add_policy_form.tsx index 9424dfdad75..a08ae1d9f98 100644 --- a/ui/litellm-dashboard/src/app/(dashboard)/policies/_components/add_policy_form.tsx +++ b/ui/litellm-dashboard/src/app/(dashboard)/policies/_components/add_policy_form.tsx @@ -1,13 +1,25 @@ import React, { useState, useEffect } from "react"; -import { Form, Select, Modal, Divider, Typography, Tag, Alert, Radio } from "antd"; -import { Button, TextInput, Textarea } from "@tremor/react"; +import { Modal, Alert, Tag } from "antd"; +import { z } from "zod/v4"; import { Policy, PolicyCreateRequest, PolicyUpdateRequest } from "@/components/policies/types"; import { Guardrail } from "@/components/guardrails/types"; import { getResolvedGuardrails, modelAvailableCall } from "@/components/networking"; import { toast } from "@/lib/toast"; import useAuthorized from "@/app/(dashboard)/hooks/useAuthorized"; - -const { Text } = Typography; +import { MultiSelect } from "@/components/shared/MultiSelect"; +import { SearchSelect } from "@/components/shared/SearchSelect"; +import { FieldGroup } from "@/components/shared/form/field"; +import { FormField } from "@/components/shared/form/FormField"; +import { Badge } from "@/components/ui/badge"; +import { Button } from "@/components/ui/button"; +import { Input } from "@/components/ui/input"; +import { RadioGroup, RadioGroupItem } from "@/components/ui/radio-group"; +import { Separator } from "@/components/ui/separator"; +import { Textarea } from "@/components/ui/textarea"; +import { Tooltip, TooltipContent, TooltipProvider, TooltipTrigger } from "@/components/ui/tooltip"; +import { UiLoadingSpinner } from "@/components/ui/ui-loading-spinner"; +import { useZodForm } from "@/lib/forms/useZodForm"; +import { CircleHelp } from "lucide-react"; interface AddPolicyFormProps { visible: boolean; @@ -22,48 +34,123 @@ interface AddPolicyFormProps { updatePolicy: (accessToken: string, policyId: string, policyData: any) => Promise; } -// ───────────────────────────────────────────────────────────────────────────── -// Mode Picker (Step 1) - shown first when creating a new policy -// ───────────────────────────────────────────────────────────────────────────── +type ModelConditionType = "model" | "regex"; -interface ModePicker { +const policyShape = { + policy_name: z + .string() + .min(1, "Please enter a policy name") + .regex(/^[a-zA-Z0-9_-]+$/, "Policy name can only contain letters, numbers, hyphens, and underscores"), + description: z.string(), + inherit: z.string(), + guardrails_add: z.array(z.string()), + guardrails_remove: z.array(z.string()), + model_condition: z.string(), +}; + +const policySchema = z.object(policyShape); + +type PolicyFormValues = z.infer; + +const EMPTY_VALUES: PolicyFormValues = { + policy_name: "", + description: "", + inherit: "", + guardrails_add: [], + guardrails_remove: [], + model_condition: "", +}; + +const toFormValues = (policy: Policy): PolicyFormValues => ({ + policy_name: policy.policy_name, + description: policy.description ?? "", + inherit: policy.inherit ?? "", + guardrails_add: policy.guardrails_add || [], + guardrails_remove: policy.guardrails_remove || [], + model_condition: policy.condition?.model ?? "", +}); + +const buildPolicyRequest = (values: PolicyFormValues): PolicyCreateRequest | PolicyUpdateRequest => ({ + policy_name: values.policy_name, + description: values.description || undefined, + inherit: values.inherit || undefined, + guardrails_add: values.guardrails_add, + guardrails_remove: values.guardrails_remove, + condition: values.model_condition ? { model: values.model_condition } : undefined, +}); + +const parentGuardrails = (policy: Policy, existingPolicies: Policy[]): string[] => { + const inherited = policy.inherit + ? (() => { + const grandparent = existingPolicies.find((candidate) => candidate.policy_name === policy.inherit); + return grandparent ? parentGuardrails(grandparent, existingPolicies) : []; + })() + : []; + const resolved = new Set([...inherited, ...(policy.guardrails_add ?? [])]); + (policy.guardrails_remove ?? []).forEach((guardrail) => resolved.delete(guardrail)); + return Array.from(resolved); +}; + +const resolveGuardrails = (values: PolicyFormValues, existingPolicies: Policy[]): string[] => { + const parentPolicy = values.inherit + ? existingPolicies.find((policy) => policy.policy_name === values.inherit) + : undefined; + const resolved = new Set([ + ...(parentPolicy ? parentGuardrails(parentPolicy, existingPolicies) : []), + ...values.guardrails_add, + ]); + values.guardrails_remove.forEach((guardrail) => resolved.delete(guardrail)); + return Array.from(resolved).sort(); +}; + +const labelWithHint = (label: string, hint: string): React.ReactNode => ( + <> + {label} + + } /> + {hint} + + +); + +const SectionHeading: React.FC<{ label: string }> = ({ label }) => ( +
+ {label} + +
+); + +interface ModePickerProps { selected: "simple" | "flow_builder"; onSelect: (mode: "simple" | "flow_builder") => void; } -const ModePicker: React.FC = ({ selected, onSelect }) => ( -
- {/* Simple Mode Card */} -
onSelect("simple")} - style={{ - flex: 1, - padding: "24px 20px", - border: `2px solid ${selected === "simple" ? "#4f46e5" : "#e5e7eb"}`, - borderRadius: 12, - cursor: "pointer", - backgroundColor: selected === "simple" ? "#eef2ff" : "#fff", - transition: "all 0.15s ease", - }} - > -
+const modeCardClass = (isSelected: boolean) => + [ + "relative flex-1 cursor-pointer rounded-xl border-2 px-5 py-6 transition-all", + isSelected + ? "border-indigo-600 bg-indigo-50 dark:border-indigo-400 dark:bg-indigo-950" + : "border-border bg-background", + ].join(" "); + +const modeIconClass = (isSelected: boolean) => + [ + "mb-4 flex size-10 items-center justify-center rounded-[10px]", + isSelected + ? "bg-indigo-100 text-indigo-600 dark:bg-indigo-900 dark:text-indigo-300" + : "bg-muted text-muted-foreground", + ].join(" "); + +const ModePicker: React.FC = ({ selected, onSelect }) => ( +
+
onSelect("simple")} className={modeCardClass(selected === "simple")}> +
= ({ selected, onSelect }) => (
- - Simple Mode - - - Pick guardrails from a list. All run in parallel. - + Simple Mode + Pick guardrails from a list. All run in parallel.
- {/* Flow Builder Card */} -
onSelect("flow_builder")} - style={{ - flex: 1, - padding: "24px 20px", - border: `2px solid ${selected === "flow_builder" ? "#4f46e5" : "#e5e7eb"}`, - borderRadius: 12, - cursor: "pointer", - backgroundColor: selected === "flow_builder" ? "#eef2ff" : "#fff", - transition: "all 0.15s ease", - position: "relative", - }} - > - +
onSelect("flow_builder")} className={modeCardClass(selected === "flow_builder")}> + NEW - -
+ +
= ({ selected, onSelect }) => (
- - Flow Builder - - - Define steps, conditions, and error responses. - + Flow Builder + Define steps, conditions, and error responses.
); -// ───────────────────────────────────────────────────────────────────────────── -// Main Component -// ───────────────────────────────────────────────────────────────────────────── - const AddPolicyForm: React.FC = ({ visible, onClose, @@ -158,10 +199,10 @@ const AddPolicyForm: React.FC = ({ createPolicy, updatePolicy, }) => { - const [form] = Form.useForm(); + const form = useZodForm(policySchema, { defaultValues: EMPTY_VALUES }); const [isSubmitting, setIsSubmitting] = useState(false); const [resolvedGuardrails, setResolvedGuardrails] = useState([]); - const [modelConditionType, setModelConditionType] = useState<"model" | "regex">("model"); + const [modelConditionType, setModelConditionType] = useState("model"); const [availableModels, setAvailableModels] = useState([]); const [step, setStep] = useState<"pick_mode" | "simple_form">("pick_mode"); const [selectedMode, setSelectedMode] = useState<"simple" | "flow_builder">("simple"); @@ -176,14 +217,7 @@ const AddPolicyForm: React.FC = ({ const isRegex = modelCondition && /[.*+?^${}()|[\]\\]/.test(modelCondition); setModelConditionType(isRegex ? "regex" : "model"); - form.setFieldsValue({ - policy_name: editingPolicy.policy_name, - description: editingPolicy.description, - inherit: editingPolicy.inherit, - guardrails_add: editingPolicy.guardrails_add || [], - guardrails_remove: editingPolicy.guardrails_remove || [], - model_condition: modelCondition, - }); + form.reset(toFormValues(editingPolicy)); if (editingPolicy.policy_id && accessToken) { loadResolvedGuardrails(editingPolicy.policy_id); @@ -198,7 +232,7 @@ const AddPolicyForm: React.FC = ({ // If editing a simple policy, skip mode picker setStep("simple_form"); } else if (visible) { - form.resetFields(); + form.reset(EMPTY_VALUES); setResolvedGuardrails([]); setModelConditionType("model"); setSelectedMode("simple"); @@ -237,56 +271,12 @@ const AddPolicyForm: React.FC = ({ } }; - const computeResolvedGuardrails = (): string[] => { - const values = form.getFieldsValue(true); - const inheritFrom = values.inherit; - const guardrailsAdd = values.guardrails_add || []; - const guardrailsRemove = values.guardrails_remove || []; - - let resolved = new Set(); - - if (inheritFrom) { - const parentPolicy = existingPolicies.find((p) => p.policy_name === inheritFrom); - if (parentPolicy) { - const parentResolved = resolveParentGuardrails(parentPolicy); - parentResolved.forEach((g) => resolved.add(g)); - } - } - - guardrailsAdd.forEach((g: string) => resolved.add(g)); - guardrailsRemove.forEach((g: string) => resolved.delete(g)); - - return Array.from(resolved).sort(); - }; - - const resolveParentGuardrails = (policy: Policy): string[] => { - let resolved = new Set(); - - if (policy.inherit) { - const grandparent = existingPolicies.find((p) => p.policy_name === policy.inherit); - if (grandparent) { - resolveParentGuardrails(grandparent).forEach((g) => resolved.add(g)); - } - } - if (policy.guardrails_add) { - policy.guardrails_add.forEach((g) => resolved.add(g)); - } - if (policy.guardrails_remove) { - policy.guardrails_remove.forEach((g) => resolved.delete(g)); - } - return Array.from(resolved); - }; - - const handleFormChange = () => { - setResolvedGuardrails(computeResolvedGuardrails()); - }; - - const resetForm = () => { - form.resetFields(); + const refreshResolvedGuardrails = (changed: Partial) => { + setResolvedGuardrails(resolveGuardrails({ ...form.getValues(), ...changed }, existingPolicies)); }; const handleClose = () => { - resetForm(); + form.reset(EMPTY_VALUES); setStep("pick_mode"); setSelectedMode("simple"); onClose(); @@ -301,24 +291,15 @@ const AddPolicyForm: React.FC = ({ } }; - const handleSubmit = async () => { + const handleSubmit = async (values: PolicyFormValues) => { try { setIsSubmitting(true); - await form.validateFields(); - const values = form.getFieldsValue(true); if (!accessToken) { throw new Error("No access token available"); } - const data: PolicyCreateRequest | PolicyUpdateRequest = { - policy_name: values.policy_name, - description: values.description || undefined, - inherit: values.inherit || undefined, - guardrails_add: values.guardrails_add || [], - guardrails_remove: values.guardrails_remove || [], - condition: values.model_condition ? { model: values.model_condition } : undefined, - }; + const data = buildPolicyRequest(values); if (isEditing && editingPolicy) { await updatePolicy(accessToken, editingPolicy.policy_id, data as PolicyUpdateRequest); @@ -328,7 +309,7 @@ const AddPolicyForm: React.FC = ({ toast.success("Policy created successfully"); } - resetForm(); + form.reset(EMPTY_VALUES); onSuccess(); onClose(); } catch (error) { @@ -361,26 +342,15 @@ const AddPolicyForm: React.FC = ({ )} -
- -
@@ -397,165 +367,192 @@ const AddPolicyForm: React.FC = ({ footer={null} width={700} > -
- - - + + event.preventDefault()} noValidate> + + + {({ ref, ...control }) => ( + + )} + - -