fix: address Copilot review round 37 — stale constants, inherited answers, prefixed labels
- The seeding effect re-applies constants as well as filling absences: an in-place schema change can move a `const` the user cannot edit, and the read-only field then displayed the new value while `values` still held the old one — which is what would be submitted. Ordinary defaults are still only ever added, so an edited field keeps what the user put there. - hasMissingIn checks own-property presence: a required argument legally named `constructor` resolved to the inherited one and read as supplied, enabling a submit the schema rejects. - The TUI keeps the declared name as a branch field's fallback title. The prefix is an internal field NAME, and buildFields labels from its map key — so the showcase displayed `__b0__address` where the schema says `address`. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
f78b541ce1
commit
85b030cbde
6 changed files with 120 additions and 5 deletions
|
|
@ -465,6 +465,35 @@ describe("schemaToForm", () => {
|
||||||
]);
|
]);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("labels a branch field by the name the schema declared", () => {
|
||||||
|
// The prefix is an internal field name; `buildFields` falls back to its
|
||||||
|
// map key for a label, so the user would otherwise see `__b0__address`.
|
||||||
|
const form = schemaToForm(UNION, "union_tool");
|
||||||
|
expect(form.sections[1]!.fields[1]).toMatchObject({
|
||||||
|
name: "__b0__address",
|
||||||
|
label: "address",
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
|
it("keeps a declared title ahead of the fallback", () => {
|
||||||
|
const form = schemaToForm(
|
||||||
|
{
|
||||||
|
type: "object",
|
||||||
|
anyOf: [
|
||||||
|
{
|
||||||
|
type: "object",
|
||||||
|
properties: { a: { type: "string", title: "Street address" } },
|
||||||
|
},
|
||||||
|
{ type: "object", properties: { b: { type: "string" } } },
|
||||||
|
],
|
||||||
|
},
|
||||||
|
"titled",
|
||||||
|
);
|
||||||
|
expect(form.sections[1]!.fields[0]).toMatchObject({
|
||||||
|
label: "Street address",
|
||||||
|
});
|
||||||
|
});
|
||||||
|
|
||||||
it("offers a variant select listing the alternatives", () => {
|
it("offers a variant select listing the alternatives", () => {
|
||||||
const form = schemaToForm(UNION, "union_tool");
|
const form = schemaToForm(UNION, "union_tool");
|
||||||
expect(form.sections[0]!.fields[0]).toMatchObject({
|
expect(form.sections[0]!.fields[0]).toMatchObject({
|
||||||
|
|
|
||||||
|
|
@ -143,6 +143,19 @@ function branchFields(
|
||||||
return [...new Set([...own, ...sharedFieldNames(base, branches)])];
|
return [...new Set([...own, ...sharedFieldNames(base, branches)])];
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* A property declaration carrying the name it was declared under as its
|
||||||
|
* fallback `title`, so a renamed field still displays the schema's own name.
|
||||||
|
*/
|
||||||
|
function labelled(property: unknown, name: string): unknown {
|
||||||
|
if (typeof property !== "object" || property === null) {
|
||||||
|
// JSON Schema's `true` constrains nothing, so a title-only object says the
|
||||||
|
// same thing and carries the label.
|
||||||
|
return { title: name };
|
||||||
|
}
|
||||||
|
return { title: name, ...property };
|
||||||
|
}
|
||||||
|
|
||||||
/** The `const` a property schema pins its value to, if any. */
|
/** The `const` a property schema pins its value to, if any. */
|
||||||
function constOf(schema: unknown): unknown {
|
function constOf(schema: unknown): unknown {
|
||||||
if (typeof schema !== "object" || schema === null) return undefined;
|
if (typeof schema !== "object" || schema === null) return undefined;
|
||||||
|
|
@ -217,7 +230,11 @@ export function schemaToForm(
|
||||||
const properties = Object.fromEntries(
|
const properties = Object.fromEntries(
|
||||||
branchFields(base, branches, index).map((name) => [
|
branchFields(base, branches, index).map((name) => [
|
||||||
branchFieldName(prefix, index, name),
|
branchFieldName(prefix, index, name),
|
||||||
branch.schema.properties?.[name],
|
// The prefix is an internal field NAME, never a label: `buildFields`
|
||||||
|
// falls back to its map key when a declaration carries no `title`, so
|
||||||
|
// without this the form would show `__b0__address` where the schema
|
||||||
|
// says `address`.
|
||||||
|
labelled(branch.schema.properties?.[name], name),
|
||||||
]),
|
]),
|
||||||
);
|
);
|
||||||
sections.push({
|
sections.push({
|
||||||
|
|
|
||||||
|
|
@ -2391,6 +2391,43 @@ describe("SchemaForm multiline strings (#2042)", () => {
|
||||||
expect(screen.getByRole("textbox", { name: /Address/ })).toBeTruthy();
|
expect(screen.getByRole("textbox", { name: /Address/ })).toBeTruthy();
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("corrects a stale const when the schema changes in place", async () => {
|
||||||
|
// The user cannot edit a read-only field, so a `const` that moves under
|
||||||
|
// an unchanged `resetKey` would display the new value while the old one
|
||||||
|
// sat in `values`, waiting to be submitted.
|
||||||
|
const onChange = vi.fn();
|
||||||
|
const pinned = (value: string): InspectorFormSchema => ({
|
||||||
|
type: "object",
|
||||||
|
properties: {
|
||||||
|
kind: { type: "string", const: value },
|
||||||
|
note: { type: "string", title: "Note" },
|
||||||
|
},
|
||||||
|
});
|
||||||
|
const { rerender } = renderWithMantine(
|
||||||
|
<SchemaForm
|
||||||
|
schema={pinned("email")}
|
||||||
|
values={{ kind: "email", note: "kept" }}
|
||||||
|
onChange={onChange}
|
||||||
|
resetKey="same-tool"
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
await Promise.resolve();
|
||||||
|
onChange.mockClear();
|
||||||
|
|
||||||
|
rerender(
|
||||||
|
<SchemaForm
|
||||||
|
schema={pinned("sms")}
|
||||||
|
values={{ kind: "email", note: "kept" }}
|
||||||
|
onChange={onChange}
|
||||||
|
resetKey="same-tool"
|
||||||
|
/>,
|
||||||
|
);
|
||||||
|
// The constant is corrected; what the user typed is left alone.
|
||||||
|
await waitFor(() =>
|
||||||
|
expect(onChange).toHaveBeenCalledWith({ kind: "sms", note: "kept" }),
|
||||||
|
);
|
||||||
|
});
|
||||||
|
|
||||||
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",
|
||||||
|
|
|
||||||
|
|
@ -36,7 +36,10 @@ import {
|
||||||
resolveRootUnion,
|
resolveRootUnion,
|
||||||
selectBranchIndex,
|
selectBranchIndex,
|
||||||
} from "@inspector/core/json/rootUnion.js";
|
} from "@inspector/core/json/rootUnion.js";
|
||||||
import { collectSchemaDefaults } from "../../../utils/jsonUtils";
|
import {
|
||||||
|
applySchemaConstants,
|
||||||
|
collectSchemaDefaults,
|
||||||
|
} from "../../../utils/jsonUtils";
|
||||||
|
|
||||||
const FieldLabel = Text.withProps({
|
const FieldLabel = Text.withProps({
|
||||||
fw: 500,
|
fw: 500,
|
||||||
|
|
@ -751,9 +754,20 @@ export function SchemaForm({
|
||||||
const missing = Object.entries(collectSchemaDefaults(current)).filter(
|
const missing = Object.entries(collectSchemaDefaults(current)).filter(
|
||||||
([name]) => !Object.hasOwn(held, name),
|
([name]) => !Object.hasOwn(held, name),
|
||||||
);
|
);
|
||||||
if (missing.length > 0) {
|
// Constants are re-applied as well as seeded: an in-place schema change can
|
||||||
report({ ...held, ...Object.fromEntries(missing) });
|
// move a `const` the user cannot edit — the read-only field then displays
|
||||||
}
|
// the new value while `values` still holds the old one, which is what would
|
||||||
|
// be submitted. Ordinary defaults are only ever *added*, so an edited field
|
||||||
|
// keeps what the user put there.
|
||||||
|
const next = applySchemaConstants(
|
||||||
|
current,
|
||||||
|
missing.length > 0 ? { ...held, ...Object.fromEntries(missing) } : held,
|
||||||
|
);
|
||||||
|
const changed = Object.keys(next).some(
|
||||||
|
(name) =>
|
||||||
|
!Object.hasOwn(held, name) || !Object.is(next[name], held[name]),
|
||||||
|
);
|
||||||
|
if (changed) report(next);
|
||||||
// Keyed by the entity and branch being edited, and by the fixed values
|
// Keyed by the entity and branch being edited, and by the fixed values
|
||||||
// themselves: a tool refreshed in place keeps its `resetKey` and its branch
|
// themselves: a tool refreshed in place keeps its `resetKey` and its branch
|
||||||
// while its schema changes underneath, and a newly pinned field would
|
// while its schema changes underneath, and a newly pinned field would
|
||||||
|
|
|
||||||
|
|
@ -601,6 +601,20 @@ describe("root composition (#2123)", () => {
|
||||||
).toBe(true);
|
).toBe(true);
|
||||||
});
|
});
|
||||||
|
|
||||||
|
it("does not read an inherited property as a supplied answer", () => {
|
||||||
|
// `constructor` is a legal argument name; reading it off the prototype
|
||||||
|
// would enable a submit the schema rejects.
|
||||||
|
const schema: InspectorFormSchema = {
|
||||||
|
type: "object",
|
||||||
|
// Built through `fromEntries`: in an object literal, `constructor` is
|
||||||
|
// TypeScript's own inherited member rather than a plain key.
|
||||||
|
properties: Object.fromEntries([["constructor", { type: "string" }]]),
|
||||||
|
required: ["constructor"],
|
||||||
|
};
|
||||||
|
expect(hasMissingRequiredFields(schema, {})).toBe(true);
|
||||||
|
expect(hasMissingRequiredFields(schema, { constructor: "x" })).toBe(false);
|
||||||
|
});
|
||||||
|
|
||||||
it("blocks submission while no branch is satisfied", () => {
|
it("blocks submission while no branch is satisfied", () => {
|
||||||
expect(hasMissingRequiredFields(UNION, {})).toBe(true);
|
expect(hasMissingRequiredFields(UNION, {})).toBe(true);
|
||||||
expect(hasMissingRequiredFields(UNION, { kind: "email" })).toBe(true);
|
expect(hasMissingRequiredFields(UNION, { kind: "email" })).toBe(true);
|
||||||
|
|
|
||||||
|
|
@ -352,6 +352,10 @@ function hasMissingIn(
|
||||||
const required = Array.isArray(schema.required) ? schema.required : [];
|
const required = Array.isArray(schema.required) ? schema.required : [];
|
||||||
const properties = schema.properties ?? {};
|
const properties = schema.properties ?? {};
|
||||||
return required.some((field) => {
|
return required.some((field) => {
|
||||||
|
// `hasOwn` first: an argument legally named `constructor` would otherwise
|
||||||
|
// resolve to the inherited one and read as supplied, enabling a submit the
|
||||||
|
// schema rejects.
|
||||||
|
if (!Object.hasOwn(values, field)) return true;
|
||||||
const value = values[field];
|
const value = values[field];
|
||||||
if (value === null) {
|
if (value === null) {
|
||||||
const fieldSchema = properties[field];
|
const fieldSchema = properties[field];
|
||||||
|
|
|
||||||
Loading…
Reference in a new issue