fix: address Copilot review round 15 — unevaluable const siblings, $ref fields, per-branch draft keys
- rootUnion: a merged declaration pairing a `const` with an assertion this
module cannot evaluate (`minimum`, `pattern`, …) is declined. Proving the
conjunction safe is the requirement, not disproving it — `minimum: 10`
beside `const: 1` is as unsatisfiable as a type mismatch.
- declaresAnyFields counts a `$ref`: its shape is unknown rather than empty,
so `anyOf: [{ $ref }, { $ref }]` no longer reports an App tool as
input-free and auto-invokes it with `{}`.
- SchemaForm keys its draft-holding fields by the active branch as well, so a
half-typed `-` or an unparsed JSON draft cannot survive a switch into a
same-named field of the incoming branch — with both parent values
`undefined`, nothing else tells the field the entity changed.
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
5b6d68a9b6
commit
09914772ea
5 changed files with 90 additions and 2 deletions
|
|
@ -2182,6 +2182,39 @@ describe("SchemaForm multiline strings (#2042)", () => {
|
||||||
expect(onChange).toHaveBeenCalledWith({ count: 3 });
|
expect(onChange).toHaveBeenCalledWith({ count: 3 });
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("clears an in-progress draft when the branch changes", async () => {
|
||||||
|
const user = userEvent.setup();
|
||||||
|
const schema: InspectorFormSchema = {
|
||||||
|
type: "object",
|
||||||
|
anyOf: [
|
||||||
|
{
|
||||||
|
type: "object",
|
||||||
|
title: "A",
|
||||||
|
properties: { value: { type: "number", title: "Value" } },
|
||||||
|
},
|
||||||
|
{
|
||||||
|
type: "object",
|
||||||
|
title: "B",
|
||||||
|
properties: { value: { type: "number", title: "Value" } },
|
||||||
|
},
|
||||||
|
],
|
||||||
|
};
|
||||||
|
renderWithMantine(
|
||||||
|
<SchemaForm schema={schema} values={{}} onChange={vi.fn()} />,
|
||||||
|
);
|
||||||
|
const before = screen.getByLabelText(/Value/) as HTMLInputElement;
|
||||||
|
// A lone `-` parses to nothing, so the parent value stays `undefined` on
|
||||||
|
// both sides of the switch — the field's own key is what has to change.
|
||||||
|
await user.type(before, "-");
|
||||||
|
expect(before.value).toBe("-");
|
||||||
|
|
||||||
|
await user.click(screen.getByRole("textbox", { name: /Variant/ }));
|
||||||
|
await user.click(screen.getByRole("option", { name: "B" }));
|
||||||
|
expect((screen.getByLabelText(/Value/) as HTMLInputElement).value).toBe(
|
||||||
|
"",
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
it("renders no picker for a single-branch union but still shows its fields", () => {
|
it("renders no picker for a single-branch union but still shows its fields", () => {
|
||||||
const schema: InspectorFormSchema = {
|
const schema: InspectorFormSchema = {
|
||||||
type: "object",
|
type: "object",
|
||||||
|
|
|
||||||
|
|
@ -606,6 +606,16 @@ export function SchemaForm({
|
||||||
const properties = effectiveSchema.properties ?? {};
|
const properties = effectiveSchema.properties ?? {};
|
||||||
const requiredFields = effectiveSchema.required ?? [];
|
const requiredFields = effectiveSchema.required ?? [];
|
||||||
|
|
||||||
|
// The key the draft-holding fields are remounted by. Switching branches is a
|
||||||
|
// reset for them too: two alternatives may declare the same name with the
|
||||||
|
// same widget, and a half-typed `-` or an unparsed JSON draft would otherwise
|
||||||
|
// survive into a field the switch was supposed to clear — with both parent
|
||||||
|
// values `undefined`, nothing else tells them the entity changed.
|
||||||
|
const draftKey =
|
||||||
|
activeBranch === null
|
||||||
|
? resetKey
|
||||||
|
: `${resetKey ?? ""}#${branches.indexOf(activeBranch)}`;
|
||||||
|
|
||||||
// The names of fields currently holding unsendable text. Held here rather
|
// The names of fields currently holding unsendable text. Held here rather
|
||||||
// than in each field because only the form sees them all, and only the form
|
// than in each field because only the form sees them all, and only the form
|
||||||
// knows when the last one cleared.
|
// knows when the last one cleared.
|
||||||
|
|
@ -867,7 +877,7 @@ export function SchemaForm({
|
||||||
<SchemaNumberInput
|
<SchemaNumberInput
|
||||||
// The only field holding local state, so the only one that has to be
|
// The only field holding local state, so the only one that has to be
|
||||||
// remounted when `resetKey` says the form moved to another entity.
|
// remounted when `resetKey` says the form moved to another entity.
|
||||||
key={resetKey === undefined ? fieldName : `${resetKey}:${fieldName}`}
|
key={draftKey === undefined ? fieldName : `${draftKey}:${fieldName}`}
|
||||||
fieldName={fieldName}
|
fieldName={fieldName}
|
||||||
label={label}
|
label={label}
|
||||||
description={description}
|
description={description}
|
||||||
|
|
@ -980,7 +990,7 @@ export function SchemaForm({
|
||||||
<SchemaJsonField
|
<SchemaJsonField
|
||||||
// Holds local draft state, so — like the number field — it has to be
|
// Holds local draft state, so — like the number field — it has to be
|
||||||
// remounted when `resetKey` says the form moved to another entity.
|
// remounted when `resetKey` says the form moved to another entity.
|
||||||
key={resetKey === undefined ? fieldName : `${resetKey}:${fieldName}`}
|
key={draftKey === undefined ? fieldName : `${draftKey}:${fieldName}`}
|
||||||
fieldName={fieldName}
|
fieldName={fieldName}
|
||||||
label={label}
|
label={label}
|
||||||
description={description}
|
description={description}
|
||||||
|
|
|
||||||
|
|
@ -402,6 +402,20 @@ describe("resolveRootUnion", () => {
|
||||||
expect(branches).toEqual([]);
|
expect(branches).toEqual([]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("declines a const paired with an assertion it cannot evaluate", () => {
|
||||||
|
// `minimum: 10` beside `const: 1` is as unsatisfiable as a type mismatch,
|
||||||
|
// and proving the conjunction safe is the requirement here.
|
||||||
|
const { branches } = resolveRootUnion({
|
||||||
|
type: "object",
|
||||||
|
properties: { x: { type: "number", minimum: 10 } },
|
||||||
|
anyOf: [
|
||||||
|
{ type: "object", properties: { x: { const: 1 } } },
|
||||||
|
{ type: "object", properties: { other: { type: "string" } } },
|
||||||
|
] as unknown[],
|
||||||
|
});
|
||||||
|
expect(branches).toEqual([]);
|
||||||
|
});
|
||||||
|
|
||||||
it("accepts a const its root type and enum admit", () => {
|
it("accepts a const its root type and enum admit", () => {
|
||||||
const { branches } = resolveRootUnion({
|
const { branches } = resolveRootUnion({
|
||||||
type: "object",
|
type: "object",
|
||||||
|
|
|
||||||
|
|
@ -134,6 +134,17 @@ describe("hasInputFields with root composition (#2123)", () => {
|
||||||
).toBe(true);
|
).toBe(true);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("counts a $ref member, whose shape is unknown rather than empty", () => {
|
||||||
|
expect(
|
||||||
|
hasInputFields(
|
||||||
|
tool({
|
||||||
|
type: "object",
|
||||||
|
anyOf: [{ $ref: "#/$defs/Email" }, { $ref: "#/$defs/Sms" }],
|
||||||
|
}),
|
||||||
|
),
|
||||||
|
).toBe(true);
|
||||||
|
});
|
||||||
|
|
||||||
it("still reports no fields for a bare object schema", () => {
|
it("still reports no fields for a bare object schema", () => {
|
||||||
expect(hasInputFields(tool({ type: "object" }))).toBe(false);
|
expect(hasInputFields(tool({ type: "object" }))).toBe(false);
|
||||||
});
|
});
|
||||||
|
|
|
||||||
|
|
@ -258,10 +258,26 @@ function jsonTypeOf(value: unknown): string {
|
||||||
* it leaves nothing satisfiable. An `integer` const satisfies a `number` type,
|
* it leaves nothing satisfiable. An `integer` const satisfies a `number` type,
|
||||||
* which is the one direction JSON Schema widens.
|
* which is the one direction JSON Schema widens.
|
||||||
*/
|
*/
|
||||||
|
const CONST_CHECKABLE = new Set(["const", "type", "enum"]);
|
||||||
|
|
||||||
function constSatisfiesSiblings(schema: Record<string, unknown>): boolean {
|
function constSatisfiesSiblings(schema: Record<string, unknown>): boolean {
|
||||||
if (!("const" in schema)) return true;
|
if (!("const" in schema)) return true;
|
||||||
const value = schema.const;
|
const value = schema.const;
|
||||||
const { type, enum: allowed } = schema;
|
const { type, enum: allowed } = schema;
|
||||||
|
// A `const` can be contradicted by any assertion, and only `type` and `enum`
|
||||||
|
// are evaluated here — `minimum: 10` beside `const: 1` is as unsatisfiable as
|
||||||
|
// a type mismatch. Rather than partially evaluate JSON Schema, a merged
|
||||||
|
// declaration pairing a `const` with an assertion this cannot check is
|
||||||
|
// declined: proving the conjunction safe is the requirement, not disproving
|
||||||
|
// it. Annotations are exempt, since they assert nothing.
|
||||||
|
if (
|
||||||
|
Object.keys(schema).some(
|
||||||
|
(keyword) =>
|
||||||
|
!CONST_CHECKABLE.has(keyword) && !ANNOTATION_KEYWORDS.has(keyword),
|
||||||
|
)
|
||||||
|
) {
|
||||||
|
return false;
|
||||||
|
}
|
||||||
const actual = jsonTypeOf(value);
|
const actual = jsonTypeOf(value);
|
||||||
const admits = (name: unknown) =>
|
const admits = (name: unknown) =>
|
||||||
name === actual || (name === "number" && actual === "integer");
|
name === actual || (name === "number" && actual === "integer");
|
||||||
|
|
@ -420,6 +436,10 @@ export function declaresAnyFields(
|
||||||
): boolean {
|
): boolean {
|
||||||
if (schema === undefined) return false;
|
if (schema === undefined) return false;
|
||||||
if (Object.keys(propertiesOf(schema) ?? {}).length > 0) return true;
|
if (Object.keys(propertiesOf(schema) ?? {}).length > 0) return true;
|
||||||
|
// A `$ref`'s shape is unknown rather than empty, so it counts. Reporting "no
|
||||||
|
// fields" for `anyOf: [{ $ref: … }, { $ref: … }]` would auto-invoke an App
|
||||||
|
// tool with `{}` on the strength of something never read.
|
||||||
|
if (schema.$ref !== undefined) return true;
|
||||||
const members = [
|
const members = [
|
||||||
...(schema.allOf ?? []),
|
...(schema.allOf ?? []),
|
||||||
...(schema.anyOf ?? []),
|
...(schema.anyOf ?? []),
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue