fix: address Copilot review round 35 — reseed on an in-place schema change, label every primitive const
- The seeding effect is keyed by the fixed values themselves as well as the entity and branch. A tool refreshed in place keeps its `resetKey` and its branch while its schema changes underneath, so a newly pinned field was rendered read-only and never seeded — leaving a required one permanently unsubmittable. The schema object's identity says nothing (callers rebuild it every render), which is why the key is derived from what has to be seeded. - branchLabel stringifies every primitive constant. `hasDiscriminator` accepts distinct booleans and nulls, so a `true`/`false` union was discriminated perfectly well and still labelled "Option 1"/"Option 2". Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
ba91db97e2
commit
139563f720
4 changed files with 76 additions and 9 deletions
|
|
@ -2316,6 +2316,45 @@ describe("SchemaForm multiline strings (#2042)", () => {
|
|||
expect(onChange).toHaveBeenCalledWith({ count: undefined });
|
||||
});
|
||||
|
||||
it("seeds a newly pinned field when the schema changes in place", async () => {
|
||||
// A tool refreshed in place keeps its `resetKey` and its branch while the
|
||||
// schema changes underneath — the new read-only field must still be
|
||||
// seeded, or a required one leaves submit disabled with no way to fix it.
|
||||
const onChange = vi.fn();
|
||||
const before: InspectorFormSchema = {
|
||||
type: "object",
|
||||
properties: { a: { type: "string", title: "A" } },
|
||||
};
|
||||
const { rerender } = renderWithMantine(
|
||||
<SchemaForm
|
||||
schema={before}
|
||||
values={{}}
|
||||
onChange={onChange}
|
||||
resetKey="same-tool"
|
||||
/>,
|
||||
);
|
||||
await Promise.resolve();
|
||||
onChange.mockClear();
|
||||
|
||||
rerender(
|
||||
<SchemaForm
|
||||
schema={{
|
||||
type: "object",
|
||||
properties: {
|
||||
a: { type: "string", title: "A" },
|
||||
version: { type: "string", const: "2" },
|
||||
},
|
||||
}}
|
||||
values={{}}
|
||||
onChange={onChange}
|
||||
resetKey="same-tool"
|
||||
/>,
|
||||
);
|
||||
await waitFor(() =>
|
||||
expect(onChange).toHaveBeenCalledWith({ version: "2" }),
|
||||
);
|
||||
});
|
||||
|
||||
it("renders no picker for a single-branch union but still shows its fields", () => {
|
||||
const schema: InspectorFormSchema = {
|
||||
type: "object",
|
||||
|
|
|
|||
|
|
@ -719,6 +719,11 @@ export function SchemaForm({
|
|||
// calling it during our render would update another component mid-render.
|
||||
// Only names absent from `values` are added, so a caller that has already
|
||||
// seeded them sees no call at all, and re-running cannot loop.
|
||||
// The fixed values themselves, as a stable key: callers rebuild the schema
|
||||
// object every render, so its identity says nothing, while this changes
|
||||
// exactly when what has to be seeded changes.
|
||||
const seedKey = serializeJson(collectSchemaDefaults(effectiveSchema));
|
||||
|
||||
const latestSeed = useRef({ schema: effectiveSchema, values, onChange });
|
||||
// Written in an effect, not during render — the same shape the validity
|
||||
// reporter below uses, and what `react-hooks/refs` requires.
|
||||
|
|
@ -737,9 +742,11 @@ export function SchemaForm({
|
|||
if (missing.length > 0) {
|
||||
report({ ...held, ...Object.fromEntries(missing) });
|
||||
}
|
||||
// Keyed by the entity and branch being edited: a different one has
|
||||
// different fixed values, and the same one needs this only once.
|
||||
}, [draftKey]);
|
||||
// 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
|
||||
// while its schema changes underneath, and a newly pinned field would
|
||||
// otherwise be rendered read-only and never seeded.
|
||||
}, [draftKey, seedKey]);
|
||||
|
||||
// Read through a ref so the callback's identity is not a dependency. It has
|
||||
// to be one or the other, and a *stable* dependency is what this needs: a
|
||||
|
|
|
|||
|
|
@ -583,6 +583,20 @@ describe("resolveRootUnion", () => {
|
|||
expect(branches[0].label).toBe("email");
|
||||
});
|
||||
|
||||
it("labels boolean and null constants by their values", () => {
|
||||
// `hasDiscriminator` accepts these, so the label must too — otherwise a
|
||||
// perfectly discriminated union reads as "Option 1"/"Option 2".
|
||||
const { branches } = resolveRootUnion({
|
||||
type: "object",
|
||||
required: ["v"],
|
||||
oneOf: [
|
||||
{ type: "object", properties: { v: { const: true }, a: {} } },
|
||||
{ type: "object", properties: { v: { const: null }, b: {} } },
|
||||
],
|
||||
});
|
||||
expect(branches.map((branch) => branch.label)).toEqual(["true", "null"]);
|
||||
});
|
||||
|
||||
it("labels a numeric const by its value", () => {
|
||||
const { branches } = resolveRootUnion({
|
||||
type: "object",
|
||||
|
|
|
|||
|
|
@ -586,20 +586,27 @@ function branchLabel(
|
|||
return branch.title;
|
||||
}
|
||||
const properties = propertiesOf(branch);
|
||||
const constOf = (name: string): string | null => {
|
||||
// Every primitive constant is a usable label, not just the string and number
|
||||
// cases: `hasDiscriminator` accepts distinct booleans and nulls, so a
|
||||
// `true`/`false` union would otherwise be labelled "Option 1"/"Option 2"
|
||||
// while being discriminated perfectly well. An object or array constant is
|
||||
// not a label — `undefined` marks "nothing usable here", which lets a `null`
|
||||
// constant be a value rather than the sentinel.
|
||||
const constOf = (name: string): string | undefined => {
|
||||
const property = toBranch(properties[name]) as { const?: unknown } | null;
|
||||
const value = property?.const;
|
||||
return typeof value === "string" || typeof value === "number"
|
||||
if (property === null || !("const" in property)) return undefined;
|
||||
const value = property.const;
|
||||
return value === null || typeof value !== "object"
|
||||
? String(value)
|
||||
: null;
|
||||
: undefined;
|
||||
};
|
||||
if (discriminatorProperty !== undefined) {
|
||||
const value = constOf(discriminatorProperty);
|
||||
if (value !== null) return value;
|
||||
if (value !== undefined) return value;
|
||||
}
|
||||
const constants = Object.keys(properties)
|
||||
.map((name) => constOf(name))
|
||||
.filter((value): value is string => value !== null);
|
||||
.filter((value): value is string => value !== undefined);
|
||||
if (constants.length === 1) return constants[0];
|
||||
return `Option ${index + 1}`;
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue