fix: stop const-less anyOf branches from crashing the form, and never select a non-string enum
Closes #2007. Two things, both surfaced while answering "does #1928's PR also fix #2007?". **#2007, and why it belongs in this PR.** `z.array(z.union([z.object(…), z.object(…)]))` gives an `items.anyOf` whose branches carry no top-level `const`, so every MultiSelect option was `String(item.const ?? "")` — the empty string, twice. Mantine *throws* on duplicate option values, greying out the whole tool panel. That is a pre-existing bug, but this branch makes it reachable from a new direction: a *nullable* array of object unions previously had no top-level `type` and fell to the JSON fallback, and now collapses to `type: "array"` with `items.anyOf` — straight into the crash. Shipping the collapse without this fix would widen an existing crash, so the two go together. Both shapes are covered by tests. `toConstOptions` now returns `null` unless every branch yields a distinct, non-empty value, and the field falls through to the JSON editor — which is the honest widget for a union of object shapes anyway. Applied to the `string`/`oneOf` path as well, which had the identical `?? ""` and the same duplicate-throw waiting on it. **Copilot round 2.** Round 1's fix kept "an explicit `type` is authoritative", so `{ type: "number", enum: [1, 2] }` still collapses with its enum hoisted. The TUI's dispatcher tests `property.enum` *before* the type switch, and `toSelectOptions` stringifies, so that reached a select that submits `"1"` for `1`. The select path is now gated on `isStringEnum` (exported from core for this), and a typed non-string enum falls through to its typed field: the enum constraint is lost, the value's type is not, which is the safer of the two losses. Pre-existing for a plain numeric enum; the collapse just made the nullable form reachable too. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
e4e16bb3e6
commit
80eb321b14
5 changed files with 205 additions and 18 deletions
|
|
@ -311,6 +311,39 @@ describe("schemaToForm", () => {
|
||||||
]);
|
]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// ink-form hands a select's stringified value straight back, so a numeric
|
||||||
|
// enum routed to a select would submit "1" for 1. The typed field loses the
|
||||||
|
// enum constraint but keeps the value's type — the safer loss.
|
||||||
|
it("routes a nullable numeric enum to its typed field, not a select", () => {
|
||||||
|
const form = schemaToForm(
|
||||||
|
{
|
||||||
|
properties: {
|
||||||
|
level: {
|
||||||
|
anyOf: [{ type: "integer", enum: [1, 2] }, { type: "null" }],
|
||||||
|
},
|
||||||
|
ratio: {
|
||||||
|
anyOf: [{ type: "number", enum: [0.5, 1.5] }, { type: "null" }],
|
||||||
|
},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
"numericEnum",
|
||||||
|
);
|
||||||
|
expect(
|
||||||
|
form.sections[0]!.fields.map((f) => ({ name: f.name, type: f.type })),
|
||||||
|
).toEqual([
|
||||||
|
{ name: "level", type: "integer" },
|
||||||
|
{ name: "ratio", type: "float" },
|
||||||
|
]);
|
||||||
|
});
|
||||||
|
|
||||||
|
it("routes a plain numeric enum to its typed field too", () => {
|
||||||
|
const form = schemaToForm(
|
||||||
|
{ properties: { level: { type: "integer", enum: [1, 2] } } },
|
||||||
|
"plainNumericEnum",
|
||||||
|
);
|
||||||
|
expect(form.sections[0]!.fields[0]).toMatchObject({ type: "integer" });
|
||||||
|
});
|
||||||
|
|
||||||
it("leaves a union of two real types as a plain string field", () => {
|
it("leaves a union of two real types as a plain string field", () => {
|
||||||
const form = schemaToForm(
|
const form = schemaToForm(
|
||||||
{
|
{
|
||||||
|
|
|
||||||
|
|
@ -3,7 +3,10 @@
|
||||||
*/
|
*/
|
||||||
|
|
||||||
import type { FormStructure, FormSection, FormField } from "ink-form";
|
import type { FormStructure, FormSection, FormField } from "ink-form";
|
||||||
import { normalizeNullableUnion } from "@inspector/core/json/nullableUnion.js";
|
import {
|
||||||
|
isStringEnum,
|
||||||
|
normalizeNullableUnion,
|
||||||
|
} from "@inspector/core/json/nullableUnion.js";
|
||||||
|
|
||||||
/** Minimal JSON Schema property shape used when building tool parameter forms */
|
/** Minimal JSON Schema property shape used when building tool parameter forms */
|
||||||
interface JsonSchemaProperty {
|
interface JsonSchemaProperty {
|
||||||
|
|
@ -105,12 +108,17 @@ export function schemaToForm(
|
||||||
...baseField,
|
...baseField,
|
||||||
options: toSelectOptions(property.items.enum, property.items.enumNames),
|
options: toSelectOptions(property.items.enum, property.items.enumNames),
|
||||||
} as FormField;
|
} as FormField;
|
||||||
} else if (property.enum) {
|
} else if (isStringEnum(property.enum)) {
|
||||||
// Single select
|
// Single select. Gated on the members being strings because
|
||||||
|
// `toSelectOptions` stringifies them and ink-form hands the string
|
||||||
|
// straight back: a numeric `enum: [1, 2]` would submit `"1"` and violate
|
||||||
|
// the schema. A typed non-string enum falls through to its typed field
|
||||||
|
// below, which loses the enum constraint but keeps the value's type —
|
||||||
|
// the safer of the two losses.
|
||||||
field = {
|
field = {
|
||||||
type: "select",
|
type: "select",
|
||||||
...baseField,
|
...baseField,
|
||||||
options: toSelectOptions(property.enum, property.enumNames),
|
options: toSelectOptions(property.enum ?? [], property.enumNames),
|
||||||
} as FormField;
|
} as FormField;
|
||||||
} else {
|
} else {
|
||||||
// Map JSON Schema types to ink-form types
|
// Map JSON Schema types to ink-form types
|
||||||
|
|
|
||||||
|
|
@ -971,6 +971,108 @@ describe("SchemaForm nullable unions", () => {
|
||||||
expect(onChange).toHaveBeenCalledWith({ profile: { nick: "z" } });
|
expect(onChange).toHaveBeenCalledWith({ profile: { nick: "z" } });
|
||||||
});
|
});
|
||||||
|
|
||||||
|
// #2007: `z.array(z.union([z.object(…), z.object(…)]))` gives an `items.anyOf`
|
||||||
|
// whose branches carry no top-level `const`, so every MultiSelect option was
|
||||||
|
// the empty string — and Mantine *throws* on duplicate option values, greying
|
||||||
|
// out the whole tool panel. The nullable form below is reachable only because
|
||||||
|
// this PR now collapses it into `type: "array"`, so it has to be safe too.
|
||||||
|
it("falls back to the JSON input for an array of object-union items", () => {
|
||||||
|
const objectBranch = (name: string): InspectorFormSchema => ({
|
||||||
|
type: "object",
|
||||||
|
properties: { type: { type: "string", const: name } },
|
||||||
|
required: ["type"],
|
||||||
|
});
|
||||||
|
const schema: InspectorFormSchema = {
|
||||||
|
type: "object",
|
||||||
|
properties: {
|
||||||
|
items: {
|
||||||
|
title: "Items",
|
||||||
|
type: "array",
|
||||||
|
items: { anyOf: [objectBranch("A"), objectBranch("B")] },
|
||||||
|
},
|
||||||
|
},
|
||||||
|
};
|
||||||
|
renderWithMantine(
|
||||||
|
<SchemaForm schema={schema} values={{}} onChange={vi.fn()} />,
|
||||||
|
);
|
||||||
|
expect(screen.getByLabelText(/Items/).tagName).toBe("TEXTAREA");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("falls back to the JSON input for a nullable array of object-union items", () => {
|
||||||
|
const objectBranch = (name: string): InspectorFormSchema => ({
|
||||||
|
type: "object",
|
||||||
|
properties: { type: { type: "string", const: name } },
|
||||||
|
required: ["type"],
|
||||||
|
});
|
||||||
|
const schema: InspectorFormSchema = {
|
||||||
|
type: "object",
|
||||||
|
properties: {
|
||||||
|
items: {
|
||||||
|
title: "Items",
|
||||||
|
anyOf: [
|
||||||
|
{ type: "array", items: { anyOf: [objectBranch("A")] } },
|
||||||
|
{ type: "null" },
|
||||||
|
],
|
||||||
|
},
|
||||||
|
},
|
||||||
|
};
|
||||||
|
renderWithMantine(
|
||||||
|
<SchemaForm schema={schema} values={{}} onChange={vi.fn()} />,
|
||||||
|
);
|
||||||
|
expect(screen.getByLabelText(/Items/).tagName).toBe("TEXTAREA");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("falls back to the JSON input when two anyOf branches share a const", () => {
|
||||||
|
const schema: InspectorFormSchema = {
|
||||||
|
type: "object",
|
||||||
|
properties: {
|
||||||
|
items: {
|
||||||
|
title: "Items",
|
||||||
|
type: "array",
|
||||||
|
items: { anyOf: [{ const: "dup" }, { const: "dup" }] },
|
||||||
|
},
|
||||||
|
},
|
||||||
|
};
|
||||||
|
renderWithMantine(
|
||||||
|
<SchemaForm schema={schema} values={{}} onChange={vi.fn()} />,
|
||||||
|
);
|
||||||
|
expect(screen.getByLabelText(/Items/).tagName).toBe("TEXTAREA");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("falls back to a text input for a string oneOf with no consts", () => {
|
||||||
|
const schema: InspectorFormSchema = {
|
||||||
|
type: "object",
|
||||||
|
properties: {
|
||||||
|
mode: {
|
||||||
|
title: "Mode",
|
||||||
|
type: "string",
|
||||||
|
oneOf: [{ type: "string" }, { type: "string" }],
|
||||||
|
},
|
||||||
|
},
|
||||||
|
};
|
||||||
|
renderWithMantine(
|
||||||
|
<SchemaForm schema={schema} values={{}} onChange={vi.fn()} />,
|
||||||
|
);
|
||||||
|
expect(screen.getByLabelText(/Mode/).tagName).toBe("INPUT");
|
||||||
|
});
|
||||||
|
|
||||||
|
it("still renders a MultiSelect when every anyOf branch has a distinct const", () => {
|
||||||
|
const schema: InspectorFormSchema = {
|
||||||
|
type: "object",
|
||||||
|
properties: {
|
||||||
|
items: {
|
||||||
|
title: "Items",
|
||||||
|
type: "array",
|
||||||
|
items: { anyOf: [{ const: "a", title: "Alpha" }, { const: "b" }] },
|
||||||
|
},
|
||||||
|
},
|
||||||
|
};
|
||||||
|
renderWithMantine(
|
||||||
|
<SchemaForm schema={schema} values={{}} onChange={vi.fn()} />,
|
||||||
|
);
|
||||||
|
expect(screen.getByRole("textbox", { name: "Items" })).toBeInTheDocument();
|
||||||
|
});
|
||||||
|
|
||||||
it("still falls back to the JSON input for a union of two real types", () => {
|
it("still falls back to the JSON input for a union of two real types", () => {
|
||||||
const schema: InspectorFormSchema = {
|
const schema: InspectorFormSchema = {
|
||||||
type: "object",
|
type: "object",
|
||||||
|
|
|
||||||
|
|
@ -11,7 +11,10 @@ import {
|
||||||
import { useState } from "react";
|
import { useState } from "react";
|
||||||
import { ClearButton } from "../../elements/ClearButton/ClearButton";
|
import { ClearButton } from "../../elements/ClearButton/ClearButton";
|
||||||
import { useValueChange } from "../../../hooks/useValueChange";
|
import { useValueChange } from "../../../hooks/useValueChange";
|
||||||
import type { InspectorFormSchema } from "../../../utils/jsonUtils";
|
import type {
|
||||||
|
InspectorFormSchema,
|
||||||
|
JsonSchemaConst,
|
||||||
|
} from "../../../utils/jsonUtils";
|
||||||
import { normalizeNullableUnion } from "@inspector/core/json/nullableUnion.js";
|
import { normalizeNullableUnion } from "@inspector/core/json/nullableUnion.js";
|
||||||
|
|
||||||
const FieldLabel = Text.withProps({
|
const FieldLabel = Text.withProps({
|
||||||
|
|
@ -52,6 +55,44 @@ function toEnumData(
|
||||||
return values;
|
return values;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Build `Select`/`MultiSelect` option data from a list of `oneOf`/`anyOf`
|
||||||
|
* branches that are expected to be constants.
|
||||||
|
*
|
||||||
|
* Returns `null` when the branches are not usable as options, which sends the
|
||||||
|
* field to the JSON fallback instead. Three ways that happens, all of which
|
||||||
|
* Mantine reacts to badly:
|
||||||
|
*
|
||||||
|
* - **No `const`.** An `anyOf` of *object* schemas — what
|
||||||
|
* `z.array(z.union([z.object(…), z.object(…)]))` compiles to — has no
|
||||||
|
* top-level `const` on any branch, so every option would be the empty
|
||||||
|
* string. Mantine **throws** on duplicate option values, which greys out the
|
||||||
|
* whole tool panel rather than degrading (#2007).
|
||||||
|
* - **Duplicate values.** Two branches sharing a `const` throw the same way.
|
||||||
|
* - **An empty option value**, which Mantine cannot render as selectable.
|
||||||
|
*
|
||||||
|
* A union of object shapes has no faithful dropdown anyway, so the JSON editor
|
||||||
|
* is the honest widget for it rather than a lossy or crashing one.
|
||||||
|
*/
|
||||||
|
function toConstOptions(
|
||||||
|
branches: (InspectorFormSchema | JsonSchemaConst)[],
|
||||||
|
): { value: string; label: string }[] | null {
|
||||||
|
const options: { value: string; label: string }[] = [];
|
||||||
|
const seen = new Set<string>();
|
||||||
|
for (const branch of branches) {
|
||||||
|
if (branch.const === undefined || branch.const === null) {
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
const value = String(branch.const);
|
||||||
|
if (value === "" || seen.has(value)) {
|
||||||
|
return null;
|
||||||
|
}
|
||||||
|
seen.add(value);
|
||||||
|
options.push({ value, label: branch.title ?? value });
|
||||||
|
}
|
||||||
|
return options.length > 0 ? options : null;
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Whether an enum `Select` should offer a clear affordance. Only a schema that
|
* Whether an enum `Select` should offer a clear affordance. Only a schema that
|
||||||
* admits `null` does: clearing emits `null`, and an enum without a null branch
|
* admits `null` does: clearing emits `null`, and an enum without a null branch
|
||||||
|
|
@ -248,11 +289,11 @@ export function SchemaForm({
|
||||||
}
|
}
|
||||||
|
|
||||||
// string with oneOf
|
// string with oneOf
|
||||||
if (fieldSchema.type === "string" && fieldSchema.oneOf) {
|
const oneOfData = fieldSchema.oneOf
|
||||||
const data = fieldSchema.oneOf.map((item) => ({
|
? toConstOptions(fieldSchema.oneOf)
|
||||||
value: String(item.const ?? ""),
|
: null;
|
||||||
label: item.title ?? String(item.const ?? ""),
|
if (fieldSchema.type === "string" && oneOfData) {
|
||||||
}));
|
const data = oneOfData;
|
||||||
return (
|
return (
|
||||||
<Select
|
<Select
|
||||||
key={fieldName}
|
key={fieldName}
|
||||||
|
|
@ -352,11 +393,11 @@ export function SchemaForm({
|
||||||
}
|
}
|
||||||
|
|
||||||
// array with items having anyOf
|
// array with items having anyOf
|
||||||
if (fieldSchema.type === "array" && fieldSchema.items?.anyOf) {
|
const itemsAnyOfData = fieldSchema.items?.anyOf
|
||||||
const data = fieldSchema.items.anyOf.map((item) => ({
|
? toConstOptions(fieldSchema.items.anyOf)
|
||||||
value: String(item.const ?? ""),
|
: null;
|
||||||
label: item.title ?? String(item.const ?? ""),
|
if (fieldSchema.type === "array" && itemsAnyOfData) {
|
||||||
}));
|
const data = itemsAnyOfData;
|
||||||
return (
|
return (
|
||||||
<MultiSelect
|
<MultiSelect
|
||||||
key={fieldName}
|
key={fieldName}
|
||||||
|
|
|
||||||
|
|
@ -53,8 +53,7 @@ function toBranch(value: unknown): Record<string, unknown> | null {
|
||||||
}
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Whether a branch's `enum` is one a *typeless* branch can be read as a string
|
* Whether an `enum` is one that can be rendered as a string-valued dropdown.
|
||||||
* enum.
|
|
||||||
*
|
*
|
||||||
* JSON Schema's `enum` is untyped — `[1, 2]`, `[true, false]`, and `[null]` are
|
* JSON Schema's `enum` is untyped — `[1, 2]`, `[true, false]`, and `[null]` are
|
||||||
* all legal — so a bare `{ enum: [...] }` does **not** imply strings. Guessing
|
* all legal — so a bare `{ enum: [...] }` does **not** imply strings. Guessing
|
||||||
|
|
@ -63,8 +62,12 @@ function toBranch(value: unknown): Record<string, unknown> | null {
|
||||||
* would `String(...)` them and submit `"1"` where the server expects `1`. So
|
* would `String(...)` them and submit `"1"` where the server expects `1`. So
|
||||||
* only an all-string enum earns the inference; anything else stays on the
|
* only an all-string enum earns the inference; anything else stays on the
|
||||||
* fallback path, which renders the value honestly as JSON.
|
* fallback path, which renders the value honestly as JSON.
|
||||||
|
*
|
||||||
|
* Exported because the same question decides whether a *dispatcher* may route
|
||||||
|
* an enum to a select at all. The TUI's does, and its options are stringified,
|
||||||
|
* so an unguarded numeric enum there submits `"1"` where the server wants `1`.
|
||||||
*/
|
*/
|
||||||
function isStringEnum(value: unknown): boolean {
|
export function isStringEnum(value: unknown): boolean {
|
||||||
return (
|
return (
|
||||||
Array.isArray(value) &&
|
Array.isArray(value) &&
|
||||||
value.length > 0 &&
|
value.length > 0 &&
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue