fix: address Copilot review round 3 — typed consts, null admission, preset coverage
Three findings (one inline, two in the suppressed block), all valid.
1. `toConstOptions` still stringified every non-null `const`, so
`items: { anyOf: [{ const: 1 }, { const: 2 }] }` submitted `["1"]` where
the schema says `[1]`. Same wrong-type-on-the-wire problem that keeps a
numeric `enum` off the select path, so it gets the same answer: only a
string `const` is selectable, anything else falls to the JSON editor
where the value keeps its type.
2. `hasMissingRequiredFields` derived null admission from the *renderer's*
collapse, which is narrower on purpose — it only flattens a two-member
union. So `anyOf: [string, number, null]` renders through the JSON
fallback (where `null` is typeable) yet was still marked missing,
disabling submit on a value the schema accepts. Null admission is a
validity question, not a rendering one, so it is now its own predicate:
`admitsNull` in core, recognizing `nullable`, `type: "null"`,
`type: [..., "null"]`, and a null branch anywhere in an `anyOf`/`oneOf`
of any size.
3. Nothing exercised the `record_shipment` preset — the web and TUI tests
build the `anyOf` shape by hand, which verifies the renderers but
*assumes* the premise the whole fix rests on: that Zod's `.nullish()`
emits `anyOf: [<branch>, { type: "null" }]` with the enum on the branch.
A Zod output change or a registry typo would leave every unit test green
while the documented reproduction server quietly stopped reproducing.
Added an integration test that boots the preset over HTTP, asserts the
wire shape, and runs it back through the collapse — following the
`duplicate-tool-names.test.ts` precedent, which exists for the same
reason (#1957).
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
80eb321b14
commit
34ba8256a4
2 changed files with 271 additions and 10 deletions
|
|
@ -1022,6 +1022,26 @@ describe("SchemaForm nullable unions", () => {
|
|||
expect(screen.getByLabelText(/Items/).tagName).toBe("TEXTAREA");
|
||||
});
|
||||
|
||||
// Mantine's select is string-valued, so a numeric const would submit "1" for
|
||||
// 1 — the same wrong-type-on-the-wire problem that keeps a numeric enum off
|
||||
// the select path.
|
||||
it("falls back to the JSON input for non-string anyOf consts", () => {
|
||||
const schema: InspectorFormSchema = {
|
||||
type: "object",
|
||||
properties: {
|
||||
items: {
|
||||
title: "Items",
|
||||
type: "array",
|
||||
items: { anyOf: [{ const: 1 }, { const: 2 }] },
|
||||
},
|
||||
},
|
||||
};
|
||||
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",
|
||||
|
|
|
|||
|
|
@ -60,10 +60,14 @@ function toEnumData(
|
|||
* 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:
|
||||
* field to the JSON fallback instead. Four ways that happens:
|
||||
*
|
||||
* - **No `const`.** An `anyOf` of *object* schemas — what
|
||||
* - **A non-string `const`.** Mantine's select is string-valued, so
|
||||
* `anyOf: [{ const: 1 }, { const: 2 }]` would submit `["1"]` where the
|
||||
* schema says `[1]` — the same wrong-type-on-the-wire problem that keeps a
|
||||
* numeric `enum` off the select path. An inspector must not misreport what
|
||||
* it sends, so this stays on the JSON editor, where the value keeps its type.
|
||||
* - **No `const` at all.** 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
|
||||
|
|
@ -80,10 +84,10 @@ function toConstOptions(
|
|||
const options: { value: string; label: string }[] = [];
|
||||
const seen = new Set<string>();
|
||||
for (const branch of branches) {
|
||||
if (branch.const === undefined || branch.const === null) {
|
||||
if (typeof branch.const !== "string") {
|
||||
return null;
|
||||
}
|
||||
const value = String(branch.const);
|
||||
const value = branch.const;
|
||||
if (value === "" || seen.has(value)) {
|
||||
return null;
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue