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<string, any>

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.
This commit is contained in:
yuneng-jiang 2026-08-18 23:44:12 -07:00 • committed by GitHub
parent 14c05628b4
commit 0700b1e54e
No known key found for this signature in database
GPG key ID: B5690EEEBB952194
2 changed files with 138 additions and 16 deletions

View file

@ -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<Record<string, unknown>>): UseFormGetValues<MountedFormValues> =>
((names: readonly string[]) => names.map((name) => store[name])) as unknown as UseFormGetValues<MountedFormValues>;
const project = (store: Readonly<Record<string, unknown>>) =>
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" }],
});
});
});

View file

@ -13,9 +13,14 @@ import { Field, FieldDescription, FieldError, FieldLabel } from "@/components/sh
export type MountedFormValues = Record<string, unknown>;
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<MountedFormContextValue>({
export const MountedFormProvider = MountedFormContext.Provider;
export const useMountRegistry = (): MountRegistry => {
const counts = React.useRef<Map<string, number>>(new Map());
const counts = React.useRef<Map<string, { readonly name: MountedFieldName; readonly count: number }>>(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<string, unknown> =
target !== null && typeof target === "object" && !Array.isArray(target) ? (target as Record<string, unknown>) : {};
return { ...base, [head]: rest.length === 0 ? value : setPath(base[head], rest, value) };
};
export const projectMountedValues = (
registry: MountRegistry,
getValues: UseFormGetValues<MountedFormValues>,
): 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<MountedFormValues>(
(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<MountedFormFieldProps> = ({
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<MountedFormValues, string>["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<MountedFormFieldProps> = ({
return (
<Field data-invalid={invalid || undefined} className={className}>
{label !== undefined && <FieldLabel htmlFor={name}>{label}</FieldLabel>}
{label !== undefined && <FieldLabel htmlFor={path}>{label}</FieldLabel>}
{children(controlProps)}
{hasHelp ? (
<FieldDescription id={helpId}>{help}</FieldDescription>
@ -142,5 +168,5 @@ export const MountedFormField: React.FC<MountedFormFieldProps> = ({
);
};
return <Controller control={control} name={name} rules={rules} defaultValue={defaultValue} render={renderField} />;
return <Controller control={control} name={path} rules={rules} defaultValue={defaultValue} render={renderField} />;
};