From 0700b1e54e89ead317cd2476ecabbf495aa89b1c Mon Sep 17 00:00:00 2001 From: yuneng-jiang Date: Tue, 18 Aug 2026 23:44:12 -0700 Subject: [PATCH] fix(ui): rebuild nested and list paths in the mounted-field projection (#37450) * refactor(ui): extract the MCP server edit save payload into a pure builder `handleSave` built the update payload inline across 276 lines, spreading `...restValues` straight off a mounted-only `onFinish`. That makes the payload a function of which fields happen to be mounted, and it leaves no seam to test the shape without rendering the whole edit form. Move the payload construction into `editServerPayload.ts` as `buildEditServerPayload(values, ui)`, a pure function over the submitted values plus the nine pieces of component state the handler reads. Failures become values rather than early returns with a toast: the six error branches are a tagged union that `editPayloadErrorMessage` maps back to the exact strings shown today, via an exhaustive switch. `handleSave` keeps the network call, the OAuth token persistence and its own try/catch. This is a move, not a rewrite. To prove that, `editServerPayload.differential.test.ts` holds a baseline machine-extracted from the pre-refactor function body by line range, with the failure branches converted by exact string replacement. The generator refuses to emit unless the slice is still present verbatim in the source, every conversion matches exactly once, no toast call survives, and a deliberately corrupted probe still trips that check. 59 scenarios run both implementations and compare the payload object, its key order, and its serialised bytes, so a re-ordering that leaves values untouched is caught too. The duplicate local `AUTH_TYPES_REQUIRING_CREDENTIALS` is dropped in favour of the identical exported list, and `reduceStaticHeaders` is shared with the create side. Both were verified equal before reuse. Behaviour is unchanged. The 466 pre-existing tests in the directory pass unedited. * refactor(ui): type the MCP edit payload builder instead of Record The extraction created a new public signature, so it should carry a real contract. buildEditServerPayload now takes EditServerFormValues and returns EditServerPayload, both declaring every field the builder actually reads and writes, with an unknown-valued index signature for the keys the form passes straight through. handleSave is annotated too, so antd's untyped onFinish value is narrowed once at the boundary rather than travelling as any. Fields that arrive from the store with their own runtime validation (static_headers, env_vars, credentials) stay unknown rather than being given a narrower declared type the form does not actually guarantee. Values are not run through a parser: the payload's serialised key order is part of the contract this module exists to hold, and rebuilding the object would reorder it. The credentials assignment moves from two post-hoc mutations to a single resolved entry, which keeps the payload readonly end to end and lands the key in the same position in all four branches. Behaviour is unchanged. The 59 differential scenarios still match the frozen pre-extraction body on object, Object.keys order and JSON.stringify bytes, and the mcp-servers suite is 525/525 across all 30 files. Three tsc probes confirm the new types have teeth: a wrong payload assignment, a misspelled field read and an invalid value each fail the type check. * feat(ui): add mounted-field projections for the MCP server form graph antd's onFinish reports exactly the fields mounted at submit time, and both MCP server payload builders spread that object straight through. react-hook-form with shouldUnregister false hands back the whole store instead, so a port needs the mount set written out explicitly before any JSX moves. This adds mountedEditFieldNames / mountedCreateFieldNames as pure functions over the form values, plus the projections that apply them, covering all 22 gates across the graph's 89 named bindings. No JSX changes, nothing imports them yet. Three behaviours are probed against antd 5.29.3 rather than assumed, and the tests pin all three: - a mounted-but-unset field is EMITTED as a key holding undefined, at the root and inside credentials, so the projection emits rather than omits - a Form.List row is NOT projected down to its mounted sub-fields, so env_vars rows pass through whole; filtering them would drop per-user values - the two roots disagree on more than StdioConfiguration: edit gates url on a deny-list while create uses an allow-list, and create additionally gates the whole auth section on a non-empty transport * refactor(ui): port the create key form off antd Form onto react-hook-form antd hands onFinish exactly the fields mounted at submit time, so a collapsed section contributes nothing to the request while the values typed into it survive for re-expansion. react-hook-form reaches only one of those two behaviours per shouldUnregister setting, so the store is kept intact and projected down to the mounted set through an explicit mount registry. MountedFormField carries the rest of the Form.Item contract the payload depends on: defaults taken from each field's own declaration rather than a blanket empty value, and help text that replaces the rule message instead of sitting beside it. The 60-case submit differential runs unedited against the port, joined by cases for the writers outside the submit path, mounted-set validation, Enter to submit, and switch coercion. * test(ui): pin the mounted-field sets by membership, not array order Two credential assertions compared the returned array with toStrictEqual against a literal in source order, so they failed when two names were swapped inside the source array even though the projected payload was unchanged. Key order is not observable in the payload, so those two assertions rejected a refactor that changes nothing a caller can see. Route both through the same sorted() helper the other fifteen set assertions already use. Membership keeps its teeth: deleting any one of the eighty emitted field names still fails the suite, while reordering two of them now passes. * docs(ui): state the mounted projection's static-name limit at its export The registry counts by name and the projection emits flat keys, so a Form.List row and its per-row sub-fields, whose names are generated at runtime, are never in the mounted set and go missing from the payload. That is silent and it is correct for every static field around it, so the contract belongs where the next consumer reads it. * refactor(ui): cut the mounted field's explanatory comments to the contract limit The mechanism the projection uses and the reason a helped field hides its rule message are both derivable from the code, so they belong in the pull request rather than in two places. What survives is the one thing no reader can derive: that a runtime-generated name is silently absent from the payload. * refactor(ui): drop the doc comment from MountedFormField The static-name constraint it described moves to the PR description, where it is not a second place to keep in sync with the code. * fix(ui): rebuild nested and list paths in the mounted-field projection projectMountedValues emitted one flat key per registered name, so a field registered under a dotted path produced a literal "credentials.client_id" key instead of a nested credentials object, and a field-array row produced "env_vars.0.name" instead of a row. Both are silent. buildEditServerPayload destructures credentials and passes it to buildCredentials, which returns undefined for a non-object, so every credential drops out of the payload while the code compiles and the existing suites pass. reduceStaticHeaders loses its rows the same way. Split each registered name on "." and rebuild the value, treating a numeric segment as an array index so field-array rows come back as arrays rather than objects keyed by digits. Nested credentials and both field-array sites are the same defect and take the same fix. * fix(ui): make mounted-field nesting opt-in via array names The first cut split every registered name on ".", which diverges from antd. antd's getNamePath is toArray, so a string name is a one-element path and "a.b" is stored as a literal flat key; only an array name nests. check_openapi_schema registers names taken from a live /openapi.json at runtime, so the field set is an input rather than source. A spec property containing a dot would have silently nested under the unconditional split and changed that payload with nothing failing. Accept string | readonly string[]. An array nests, with a numeric segment as an array index, which is what the credential paths and the field-array rows already use. A string stays one literal key. The registry keys by the joined path but projects from the original shape, so the two cannot drift. --- .../MountedFormField.test.ts | 96 +++++++++++++++++++ .../common_components/MountedFormField.tsx | 58 +++++++---- 2 files changed, 138 insertions(+), 16 deletions(-) create mode 100644 ui/litellm-dashboard/src/components/common_components/MountedFormField.test.ts diff --git a/ui/litellm-dashboard/src/components/common_components/MountedFormField.test.ts b/ui/litellm-dashboard/src/components/common_components/MountedFormField.test.ts new file mode 100644 index 00000000000..b619ec5748a --- /dev/null +++ b/ui/litellm-dashboard/src/components/common_components/MountedFormField.test.ts @@ -0,0 +1,96 @@ +import { describe, expect, it } from "vitest"; +import type { UseFormGetValues } from "react-hook-form"; + +import { + projectMountedValues, + type MountedFieldName, + type MountedFormValues, + type MountRegistry, +} from "./MountedFormField"; + +const registryOf = (names: readonly MountedFieldName[]): MountRegistry => ({ + register: () => () => undefined, + mountedNames: () => names, +}); + +const getValuesOf = (store: Readonly>): UseFormGetValues => + ((names: readonly string[]) => names.map((name) => store[name])) as unknown as UseFormGetValues; + +const project = (store: Readonly>) => + projectMountedValues(registryOf(Object.keys(store)), getValuesOf(store)); + +const projectPaths = (entries: readonly (readonly [MountedFieldName, unknown])[]) => { + const store = Object.fromEntries( + entries.map(([name, value]) => [Array.isArray(name) ? name.join(".") : (name as string), value]), + ); + return projectMountedValues(registryOf(entries.map(([name]) => name)), getValuesOf(store)); +}; + +describe("projectMountedValues", () => { + it("keeps a flat name flat", () => { + expect(project({ server_name: "s1", transport: "http" })).toStrictEqual({ server_name: "s1", transport: "http" }); + }); + + it("nests an ARRAY name into a credentials object", () => { + expect( + projectPaths([ + [["credentials", "aws_region_name"], "us-east-1"], + [["credentials", "aws_access_key_id"], "AKIA"], + ]), + ).toStrictEqual({ credentials: { aws_region_name: "us-east-1", aws_access_key_id: "AKIA" } }); + }); + + it("keeps a literal dotted STRING name flat, matching antd getNamePath toArray", () => { + expect(projectPaths([["a.b", 1]])).toStrictEqual({ "a.b": 1 }); + expect(projectPaths([["schema.property.with.dots", "v"]])).toStrictEqual({ "schema.property.with.dots": "v" }); + }); + + it("rebuilds Form.List rows as an array, not an object keyed by digits", () => { + const projected = projectPaths([ + [["env_vars", "0", "name"], "API_KEY"], + [["env_vars", "0", "description"], "the key"], + [["env_vars", "1", "name"], "REGION"], + ]); + expect(projected).toStrictEqual({ + env_vars: [{ name: "API_KEY", description: "the key" }, { name: "REGION" }], + }); + expect(Array.isArray(projected.env_vars)).toBe(true); + }); + + it("rebuilds static_headers rows, the second Form.List site", () => { + expect( + projectPaths([ + [["static_headers", "0", "key"], "X-Tenant"], + [["static_headers", "0", "value"], "acme"], + ]), + ).toStrictEqual({ + static_headers: [{ key: "X-Tenant", value: "acme" }], + }); + }); + + it("emits a mounted-but-unset field as a key holding undefined, matching antd onFinish", () => { + const projected = project({ alias: undefined }); + expect(Object.keys(projected)).toStrictEqual(["alias"]); + expect(projected.alias).toBeUndefined(); + }); + + it("leaves a sparse row index as a hole rather than shifting later rows down", () => { + const projected = projectPaths([[["env_vars", "2", "name"], "THIRD"]]) as { env_vars: readonly unknown[] }; + expect(projected.env_vars).toHaveLength(3); + expect(projected.env_vars[2]).toStrictEqual({ name: "THIRD" }); + }); + + it("mixes flat, nested and list names in one projection", () => { + expect( + projectPaths([ + ["transport", "http"], + [["credentials", "client_id"], "cid"], + [["env_vars", "0", "name"], "K"], + ]), + ).toStrictEqual({ + transport: "http", + credentials: { client_id: "cid" }, + env_vars: [{ name: "K" }], + }); + }); +}); diff --git a/ui/litellm-dashboard/src/components/common_components/MountedFormField.tsx b/ui/litellm-dashboard/src/components/common_components/MountedFormField.tsx index 8a92873495c..0ae58e102e7 100644 --- a/ui/litellm-dashboard/src/components/common_components/MountedFormField.tsx +++ b/ui/litellm-dashboard/src/components/common_components/MountedFormField.tsx @@ -13,9 +13,14 @@ import { Field, FieldDescription, FieldError, FieldLabel } from "@/components/sh export type MountedFormValues = Record; +const fieldKey = (name: string | readonly string[]): string => + Array.isArray(name) ? name.join(".") : (name as string); + +export type MountedFieldName = string | readonly string[]; + export interface MountRegistry { - readonly register: (name: string) => () => void; - readonly mountedNames: () => readonly string[]; + readonly register: (name: MountedFieldName) => () => void; + readonly mountedNames: () => readonly MountedFieldName[]; } export interface MountedFormContextValue { @@ -40,33 +45,53 @@ const MountedFormContext = React.createContext({ export const MountedFormProvider = MountedFormContext.Provider; export const useMountRegistry = (): MountRegistry => { - const counts = React.useRef>(new Map()); + const counts = React.useRef>(new Map()); return React.useMemo( () => ({ - register: (name: string) => { - counts.current.set(name, (counts.current.get(name) ?? 0) + 1); + register: (name: MountedFieldName) => { + const key = fieldKey(name); + counts.current.set(key, { name, count: (counts.current.get(key)?.count ?? 0) + 1 }); return () => { - const remaining = (counts.current.get(name) ?? 0) - 1; + const remaining = (counts.current.get(key)?.count ?? 0) - 1; if (remaining > 0) { - counts.current.set(name, remaining); + counts.current.set(key, { name, count: remaining }); } else { - counts.current.delete(name); + counts.current.delete(key); } }; }, - mountedNames: () => Array.from(counts.current.keys()), + mountedNames: () => Array.from(counts.current.values(), (entry) => entry.name), }), [], ); }; +const withIndex = (base: readonly unknown[], index: number, next: unknown): readonly unknown[] => + Array.from({ length: Math.max(base.length, index + 1) }, (_, i) => (i === index ? next : base[i])); + +const setPath = (target: unknown, segments: readonly string[], value: unknown): unknown => { + const [head, ...rest] = segments; + if (/^\d+$/.test(head)) { + const base: readonly unknown[] = Array.isArray(target) ? target : []; + const index = Number(head); + return withIndex(base, index, rest.length === 0 ? value : setPath(base[index], rest, value)); + } + const base: Record = + target !== null && typeof target === "object" && !Array.isArray(target) ? (target as Record) : {}; + return { ...base, [head]: rest.length === 0 ? value : setPath(base[head], rest, value) }; +}; + export const projectMountedValues = ( registry: MountRegistry, getValues: UseFormGetValues, ): MountedFormValues => { const names = [...registry.mountedNames()]; - const values = getValues(names); - return Object.fromEntries(names.map((name, index) => [name, values[index]])); + const values = getValues(names.map(fieldKey)); + return names.reduce( + (projected, name, index) => + setPath(projected, Array.isArray(name) ? name : [name as string], values[index]) as MountedFormValues, + {}, + ); }; export type MountedFieldControlProps = { @@ -81,7 +106,7 @@ export type MountedFieldControlProps = { }; export interface MountedFormFieldProps { - readonly name: string; + readonly name: MountedFieldName; readonly label?: React.ReactNode; readonly help?: React.ReactNode; readonly required?: boolean; @@ -107,15 +132,16 @@ export const MountedFormField: React.FC = ({ children, }) => { const { control, registry } = React.useContext(MountedFormContext); + const path = fieldKey(name); React.useEffect(() => registry.register(name), [registry, name]); - const helpId = `${name}_help`; + const helpId = `${path}_help`; const hasHelp = help !== undefined && help !== null; const renderField: ControllerProps["render"] = ({ field, fieldState }) => { const invalid = fieldState.error !== undefined; const controlProps: MountedFieldControlProps = { - id: name, + id: path, name: field.name, value: field.value, onChange: field.onChange, @@ -131,7 +157,7 @@ export const MountedFormField: React.FC = ({ return ( - {label !== undefined && {label}} + {label !== undefined && {label}} {children(controlProps)} {hasHelp ? ( {help} @@ -142,5 +168,5 @@ export const MountedFormField: React.FC = ({ ); }; - return ; + return ; };