modelcontextprotocol/inspector
Watch
1
0
Fork
You've already forked inspector
 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions 2

fix: align enumNames when stripping null, and make the JSON fallback typeable

Browse source 
Copilot round 5: one inline comment, two suppressed. All three valid, and two
were defects from round 4's own fix.

1. Stripping the `null` sentinel from an `enum` left the parallel `enumNames`
   untouched. Both renderers discard labels outright when the two lengths
   disagree, so `{ enum: ["a", null], enumNames: ["Alpha", "None"] }` lost the
   `Alpha` label entirely — a worse outcome than the sentinel it was fixing.
   `enumNames` is now filtered by the same indices, and only when it was
   positionally parallel to begin with (a mismatched list is already ignored
   by both renderers, so re-indexing it would invent an alignment the server
   never declared).

2. `enum: [null]` became `enum: undefined`, which widened a schema permitting
   *only* `null` into a plain string field accepting arbitrary text — the form
   would then invite values the schema forbids. Trading a cosmetic problem for
   a correctness one, which was the wrong call. The collapse now declines
   entirely for that case and the field renders through the JSON editor.

3. That JSON editor was still carrying #1928's original mechanism: it stored
   unparseable text back as the *value*, which the next render
   re-`JSON.stringify`d, so typing `[` gave `"["` then `"\"[\""`. Routing
   nullable fields to real widgets removed the common way of landing there but
   never fixed the editor — and #2007's fix deliberately sends object unions to
   it, so a fallback nobody can type into is not a fallback. `SchemaJsonField`
   now holds the draft text locally (the split `SchemaNumberInput` already
   uses) and reports parsed JSON, or `undefined` while the text is mid-edit —
   an inspector must not report a value it cannot send. Keyed by `resetKey`
   like the number field, since it now holds state.

Also replaces an `as unknown as string[]` in the round-4 test, which violated
the repo's TypeScript rules and papered over `InspectorFormSchema.enum` being
unable to model a valid `["a", null]`. The fixture is wire data, so it now goes
through `toFormSchema` — the same wire→form narrowing boundary every production
call site uses.

Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
cliffhall 2026-08-15 18:54:58 -04:00
commit 32d6f80bc1
4 changed files with 279 additions and 47 deletions
Download patch file Download diff file Expand all files Collapse all files

78
clients/web/src/components/groups/SchemaForm/SchemaForm.test.tsx
View file

@ -2,6 +2,7 @@ import { useState } from "react";
import { describe, it, expect, vi } from "vitest";
import userEvent from "@testing-library/user-event";
import type { InspectorFormSchema } from "../../../utils/jsonUtils";
import { toFormSchema } from "../../../utils/jsonUtils";
import {
fireEvent,
renderWithMantine,
@ -737,7 +738,13 @@ describe("SchemaForm", () => {
expect(lastCall.config).toEqual([1, 2]);
});
it("falls back to passing raw string to onChange when JSON is invalid in JsonInput", async () => {
// The JSON field used to store unparseable text back as the *value*, which
// the next render re-stringified — so each keystroke added a layer of
// escaping (`[` → `"["` → `"\"[\""`). That compounding escape is #1928's
// original symptom, and it lived here rather than in the dispatch. It matters
// more since #2007, whose fix deliberately routes object unions to this
// editor: a fallback nobody can type into is not a fallback.
it("reports no value, not raw text, while the JSON is mid-edit", async () => {
const user = userEvent.setup();
const onChange = vi.fn();
const schema: InspectorFormSchema = {
@ -751,9 +758,59 @@ describe("SchemaForm", () => {
);
const jsonInput = screen.getByLabelText(/Config/) as HTMLTextAreaElement;
await user.type(jsonInput, "x");
expect(onChange).toHaveBeenCalled();
const lastCall = onChange.mock.calls[onChange.mock.calls.length - 1][0];
expect(typeof lastCall.config).toBe("string");
expect(onChange).toHaveBeenLastCalledWith({ config: undefined });
});
it("lets an array literal be typed one character at a time", async () => {
const user = userEvent.setup();
const schema: InspectorFormSchema = {
type: "object",
properties: {
config: { type: "array", title: "Config" },
},
};
// Drive the real controlled loop: each onChange feeds straight back in as
// `values`, which is what turned the old handler's raw-text write into a
// compounding re-escape.
function Harness() {
const [values, setValues] = useState<Record<string, unknown>>({});
return (
<SchemaForm schema={schema} values={values} onChange={setValues} />
);
}
renderWithMantine(<Harness />);
const jsonInput = screen.getByLabelText(/Config/) as HTMLTextAreaElement;
// `[` is a userEvent keyboard descriptor, so it is escaped as `[[`.
await user.type(jsonInput, '[[1,"a"]');
// The box shows exactly what was typed — no injected quotes or backslashes.
expect(jsonInput.value).toBe('[1,"a"]');
});
it("keeps an in-progress draft visible instead of rewriting it", async () => {
const user = userEvent.setup();
const schema: InspectorFormSchema = {
type: "object",
properties: {
config: { type: "array", title: "Config" },
},
};
function Harness() {
const [values, setValues] = useState<Record<string, unknown>>({});
return (
<SchemaForm schema={schema} values={values} onChange={setValues} />
);
}
renderWithMantine(<Harness />);
const jsonInput = screen.getByLabelText(/Config/) as HTMLTextAreaElement;
await user.type(jsonInput, "[[1,");
// Unparseable so far, and it must survive the re-render untouched.
expect(jsonInput.value).toBe("[1,");
await user.type(jsonInput, "2]");
expect(jsonInput.value).toBe("[1,2]");
});
it("uses default values when value is undefined", () => {
@ -891,18 +948,23 @@ describe("SchemaForm nullable unions", () => {
it("renders a Select for a type: [string, null] enum, without a null option", async () => {
const user = userEvent.setup();
const onChange = vi.fn();
const schema: InspectorFormSchema = {
// Built through `toFormSchema`, the same narrowing boundary every
// production call site uses, rather than cast into `InspectorFormSchema`.
// A `null` member is valid JSON Schema but outside that type's `string[]`
// `enum`, and this fixture *is* wire data — so the honest way to introduce
// it is the wire→form narrow, not a double cast that erases the mismatch.
const schema = toFormSchema({
type: "object",
properties: {
direction: {
title: "Direction",
type: ["string", "null"],
enum: ["envio", "recebimento", null] as unknown as string[],
enum: ["envio", "recebimento", null],
},
},
};
});
renderWithMantine(
<SchemaForm schema={schema} values={{}} onChange={onChange} />,
<SchemaForm schema={schema!} values={{}} onChange={onChange} />,
);
await user.click(screen.getByRole("textbox", { name: "Direction" }));
const options = await screen.findAllByRole("option", { hidden: true });

98
clients/web/src/components/groups/SchemaForm/SchemaForm.tsx
View file

@ -146,6 +146,88 @@ function toNumericValue(raw: string | number): number | undefined {
return Number.isSafeInteger(Math.trunc(parsed)) ? parsed : undefined;
}
/** Parse editor text, reporting `undefined` for anything that is not JSON yet. */
function parseJsonDraft(text: string): unknown {
if (text.trim() === "") {
return undefined;
}
try {
return JSON.parse(text);
} catch {
return undefined;
}
}
/** Structural equality for the draft/value re-sync, via canonical JSON. */
function isSameJson(a: unknown, b: unknown): boolean {
if (a === undefined || b === undefined) {
return a === b;
}
return serializeJson(a) === serializeJson(b);
}
interface SchemaJsonFieldProps {
label: string;
description?: string;
withAsterisk: boolean;
disabled: boolean;
value: unknown;
onChange: (value: unknown) => void;
}
/**
* A `JsonInput` that keeps the text the user is typing, not just the value it
* currently parses to.
*
* This field used to drive the box straight off the parent's value and, when
* the text failed `JSON.parse`, store the **raw text** back as the value — which
* the next render re-`JSON.stringify`d, adding a layer of escaping per
* keystroke. Typing `[` gave `"["`, then `"\"[\""`, and the field was unusable
* within a few characters. That compounding escape is the original #1928
* symptom, and it lived in this fallback rather than in the dispatch.
*
* Routing nullable fields to real widgets removed the common way of *landing*
* here, but it did not fix the editor — and #2007's fix deliberately sends a
* union of object shapes to it, so the editor itself has to be typeable. Hence
* the same split `SchemaNumberInput` uses: the raw text is the source of truth
* for what is *displayed*, while the parent only ever sees parsed JSON.
*
* While the text is mid-edit and does not parse, the parent is told
* `undefined` rather than handed the text. An inspector must not report a value
* it cannot send, and `undefined` is how "no value supplied" is spelled
* everywhere else in this form.
*/
function SchemaJsonField({
value,
onChange,
...inputProps
}: SchemaJsonFieldProps) {
const [draft, setDraft] = useState(() =>
value === undefined ? "" : serializeJson(value),
);
// Re-sync only when the parent's value genuinely diverges from what the draft
// parses to, which leaves an external reset (a cleared form, a loaded
// example) working while an in-progress `[` — whose parse is `undefined`,
// matching the `undefined` we just emitted — is left alone.
useValueChange(value, (next) => {
if (!isSameJson(parseJsonDraft(draft), next)) {
setDraft(next === undefined ? "" : serializeJson(next));
}
});
return (
<SchemaJsonInput
{...inputProps}
value={draft}
onChange={(text) => {
setDraft(text);
onChange(parseJsonDraft(text));
}}
/>
);
}
interface SchemaNumberInputProps {
label: string;
description?: string;
@ -440,20 +522,16 @@ export function SchemaForm({
// fallback: JsonInput for complex schemas
return (
<SchemaJsonInput
key={fieldName}
<SchemaJsonField
// Holds local draft state, so — like the number field — it has to be
// remounted when `resetKey` says the form moved to another entity.
key={resetKey === undefined ? fieldName : `${resetKey}:${fieldName}`}
label={label}
description={description}
withAsterisk={isRequired}
disabled={disabled}
value={rawValue !== undefined ? serializeJson(rawValue) : ""}
onChange={(val) => {
try {
handleFieldChange(fieldName, JSON.parse(val));
} catch {
handleFieldChange(fieldName, val);
}
}}
value={rawValue}
onChange={(val) => handleFieldChange(fieldName, val)}
/>
);
}

48
clients/web/src/test/core/nullableUnion.test.ts
View file

@ -277,14 +277,52 @@ describe("normalizeNullableUnion", () => {
});
});
it("drops an enum that held nothing but null", () => {
// No value a dropdown could offer, so the field is better served by its
// plain widget than by an empty select.
// Declining is the point: emitting `enum: undefined` would turn a schema that
// permits *only* `null` into a plain string field accepting arbitrary text,
// inviting values the schema forbids. Left uncollapsed it renders through the
// JSON editor, which represents it honestly.
it("declines to collapse an enum that held nothing but null", () => {
const schema = { type: ["string", "null"], enum: [null] };
expect(normalizeNullableUnion(schema)).toBe(schema);
});
it("declines the same way on the anyOf encoding", () => {
const schema = {
anyOf: [{ type: "string", enum: [null] }, { type: "null" as const }],
};
expect(normalizeNullableUnion(schema)).toBe(schema);
});
// The parallel `enumNames` is filtered by the same indices. Both renderers
// discard labels outright on a length mismatch, so stripping one without the
// other would silently lose every label, not just the dropped one's.
it("keeps enumNames aligned when stripping the null member", () => {
expect(
normalizeNullableUnion({ type: ["string", "null"], enum: [null] }),
normalizeNullableUnion({
type: ["string", "null"],
enum: ["a", null, "b"],
enumNames: ["Alpha", "None", "Beta"],
}),
).toEqual({
type: "string",
enum: undefined,
enum: ["a", "b"],
enumNames: ["Alpha", "Beta"],
anyOf: undefined,
nullable: true,
});
});
it("leaves a mismatched enumNames alone rather than inventing an alignment", () => {
expect(
normalizeNullableUnion({
type: ["string", "null"],
enum: ["a", null],
enumNames: ["Alpha"],
}),
).toEqual({
type: "string",
enum: ["a"],
enumNames: ["Alpha"],
anyOf: undefined,
nullable: true,
});

100
core/json/nullableUnion.ts
View file

@ -102,27 +102,64 @@ export type NormalizedNullableUnion<T extends NullableUnionSchema> = Omit<
};
/**
* Drop a `null` sentinel from an `enum`, since the collapse has already moved
* that fact onto `nullable`.
* What {@link stripNullEnumMembers} concluded about an `enum`.
*
* `"only-null"` is a distinct outcome rather than "an empty list" because it
* has to **stop the collapse**, not just empty a field. See below.
*/
type EnumStrip =
| { kind: "unchanged" }
| { kind: "filtered"; members: unknown[]; names?: unknown }
| { kind: "only-null" };
/**
* Drop `null` members from an `enum`, keeping the parallel `enumNames` aligned.
*
* The `type: [T, "null"]` encoding keeps its keywords at the top level, so a
* nullable enum written that way carries the null *inside* the list:
* `{ type: ["string", "null"], enum: ["envio", "recebimento", null] }`. Leaving
* it there breaks both renderers in different ways — the web dispatcher would
* hand `null` to Mantine as option data, and the TUI's all-strings check would
* reject the whole enum and fall back to a plain text field. Neither is the
* dropdown the schema is asking for.
* `{ type: ["string", "null"], enum: ["envio", "recebimento", null] }`. The
* collapse has already moved that fact onto `nullable`, so leaving the sentinel
* in the list breaks both renderers in different ways — the web dispatcher
* would hand `null` to Mantine as option data, and the TUI's all-strings check
* would reject the whole enum and fall back to a plain text field.
*
* Returns `undefined` when nothing but `null` was in the list: an enum of only
* `null` permits no value a dropdown could offer, so the field is better served
* by its plain widget than by an empty select.
* **`enumNames` is filtered by the same indices, not left alone.** It is a
* positional parallel array, and both renderers discard labels outright when
* the two lengths disagree — so stripping one without the other silently loses
* every label rather than just the dropped one's.
*
* **An enum of nothing but `null` reports `"only-null"` so the caller declines
* to collapse at all.** Emitting `enum: undefined` instead would turn a schema
* permitting *only* `null` into a plain string field that accepts arbitrary
* text — trading a cosmetic problem for a correctness one, since the form would
* then invite values the schema forbids. Left uncollapsed it renders through
* the JSON editor, which represents it honestly.
*/
function stripNullFromEnum(value: unknown): unknown[] | undefined {
if (!Array.isArray(value)) {
return undefined;
function stripNullEnumMembers(members: unknown, names: unknown): EnumStrip {
if (!Array.isArray(members)) {
return { kind: "unchanged" };
}
const members = value.filter((member) => member !== null);
return members.length > 0 ? members : undefined;
const kept = members
.map((member, index) => ({ member, index }))
.filter((entry) => entry.member !== null);
if (kept.length === members.length) {
return { kind: "unchanged" };
}
if (kept.length === 0) {
return { kind: "only-null" };
}
// Only realign names that were positionally parallel to begin with; a
// mismatched list is already ignored by both renderers, so re-indexing it
// would invent an alignment the server never declared.
const aligned =
Array.isArray(names) && names.length === members.length
? kept.map((entry) => names[entry.index])
: names;
return {
kind: "filtered",
members: kept.map((entry) => entry.member),
names: aligned,
};
}
/**
@ -133,23 +170,32 @@ function stripNullFromEnum(value: unknown): unknown[] | undefined {
* deferred while `T` is open — so TS cannot verify the two line up even though
* every field is set right here. Isolated in this one function so the exported
* signature carries the contract and nothing else has to assert it.
*
* Returns `null` when the merged schema turns out to permit only `null`, which
* is not collapsible; see {@link stripNullEnumMembers}.
*/
function collapsed<T extends NullableUnionSchema>(
schema: T,
branch: Record<string, unknown> | undefined,
type: RenderableType,
): NormalizedNullableUnion<T> {
): NormalizedNullableUnion<T> | null {
const merged = { ...schema, ...branch };
// Read after the merge so the branch's list wins when it has one.
const strip = stripNullEnumMembers(
merged.enum,
(merged as { enumNames?: unknown }).enumNames,
);
if (strip.kind === "only-null") {
return null;
}
return {
...merged,
type,
anyOf: undefined,
nullable: true,
// `enum` is read after the spread so the branch's list wins when it has
// one, and the null sentinel is stripped either way.
...(merged.enum === undefined
? {}
: { enum: stripNullFromEnum(merged.enum) }),
...(strip.kind === "filtered"
? { enum: strip.members, enumNames: strip.names }
: {}),
} as NormalizedNullableUnion<T>;
}
@ -233,7 +279,12 @@ export function normalizeNullableUnion<T extends NullableUnionSchema>(
const type =
branch.type ?? (isStringEnum(branch.enum) ? "string" : undefined);
if (isRenderableType(type)) {
return collapsed(schema, branch, type);
// `null` when the merged enum permits only `null`, which is not
// collapsible — fall through and return the schema by identity.
const result = collapsed(schema, branch, type);
if (result !== null) {
return result;
}
}
}
}
@ -245,7 +296,10 @@ export function normalizeNullableUnion<T extends NullableUnionSchema>(
) {
const type = schema.type.find((member) => member !== "null");
if (isRenderableType(type)) {
return collapsed(schema, undefined, type);
const result = collapsed(schema, undefined, type);
if (result !== null) {
return result;
}
}
}