fix: address Copilot review round 22 — merge deep-link args with defaults per level
The App deep link seeded defaults and spread its `appArgs` over them, which
replaces a nested object wholesale: `{ config: { kind: "sms" } }` discarded
the nested SMS branch's own defaults, which the form then displayed while the
submitted arguments omitted them. New seedSchemaValues merges the two per
level, picking each nested branch from the nested values, and the supplied
value still wins wherever the two meet.
Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
92e485ab82
commit
0be450a911
3 changed files with 122 additions and 5 deletions
|
|
@ -105,7 +105,7 @@ import {
|
|||
} from "../../../utils/correlateTransportErrors";
|
||||
import {
|
||||
applySchemaConstants,
|
||||
collectSchemaDefaults,
|
||||
seedSchemaValues,
|
||||
toFormSchema,
|
||||
} from "../../../utils/jsonUtils";
|
||||
import { MONITOR_COLUMN_ANIM_MS } from "./monitorColumnAnimation";
|
||||
|
|
@ -1049,10 +1049,13 @@ export function InspectorView({
|
|||
// link disagreeing with one would otherwise auto-open with a hidden value
|
||||
// contradicting the shape on screen.
|
||||
const appFormSchema = toFormSchema(target.inputSchema) ?? {};
|
||||
const formValues = applySchemaConstants(appFormSchema, {
|
||||
...collectSchemaDefaults(appFormSchema, deepLink.appArgs ?? {}),
|
||||
...deepLink.appArgs,
|
||||
});
|
||||
const formValues = applySchemaConstants(
|
||||
appFormSchema,
|
||||
// Merged per level, not with one shallow spread: a nested object in the
|
||||
// args would otherwise replace the whole seeded object, discarding the
|
||||
// nested defaults the form goes on displaying.
|
||||
seedSchemaValues(appFormSchema, deepLink.appArgs ?? {}),
|
||||
);
|
||||
// Seed the selection directly rather than routing through
|
||||
// AppsScreen.handleSelect. This deliberately bypasses handleSelect's
|
||||
// no-input-app auto-launch: a deep link must never invoke a tool against
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ import {
|
|||
collectSchemaDefaults,
|
||||
hasMissingRequiredFields,
|
||||
applySchemaConstants,
|
||||
seedSchemaValues,
|
||||
} from "./jsonUtils";
|
||||
import type { InspectorFormSchema } from "./jsonUtils";
|
||||
|
||||
|
|
@ -488,6 +489,65 @@ describe("root composition (#2123)", () => {
|
|||
).toEqual({ config: { kind: "email" } });
|
||||
});
|
||||
|
||||
describe("seedSchemaValues", () => {
|
||||
const NESTED: InspectorFormSchema = {
|
||||
type: "object",
|
||||
properties: {
|
||||
config: {
|
||||
type: "object",
|
||||
anyOf: [
|
||||
{
|
||||
type: "object",
|
||||
properties: {
|
||||
kind: { type: "string", const: "email" },
|
||||
retries: { type: "number", default: 1 },
|
||||
},
|
||||
},
|
||||
{
|
||||
type: "object",
|
||||
properties: {
|
||||
kind: { type: "string", const: "sms" },
|
||||
retries: { type: "number", default: 3 },
|
||||
},
|
||||
},
|
||||
],
|
||||
},
|
||||
},
|
||||
};
|
||||
|
||||
it("keeps a nested branch's defaults beside the supplied values", () => {
|
||||
// A shallow spread would replace the whole `config` object, so the SMS
|
||||
// branch's `retries` would be displayed by the form and never submitted.
|
||||
expect(seedSchemaValues(NESTED, { config: { kind: "sms" } })).toEqual({
|
||||
config: { kind: "sms", retries: 3 },
|
||||
});
|
||||
});
|
||||
|
||||
it("lets the supplied value win where the two meet", () => {
|
||||
expect(
|
||||
seedSchemaValues(NESTED, { config: { kind: "sms", retries: 9 } }),
|
||||
).toEqual({ config: { kind: "sms", retries: 9 } });
|
||||
});
|
||||
|
||||
it("does not treat an array as a nested object", () => {
|
||||
const withArray: InspectorFormSchema = {
|
||||
type: "object",
|
||||
properties: { items: { type: "array" } },
|
||||
};
|
||||
expect(seedSchemaValues(withArray, { items: [1, 2] })).toEqual({
|
||||
items: [1, 2],
|
||||
});
|
||||
});
|
||||
|
||||
it("keeps a supplied argument named __proto__", () => {
|
||||
const seeded = seedSchemaValues(
|
||||
{ type: "object", properties: {} },
|
||||
Object.fromEntries([["__proto__", "kept"]]),
|
||||
);
|
||||
expect(Object.hasOwn(seeded, "__proto__")).toBe(true);
|
||||
});
|
||||
});
|
||||
|
||||
it("re-applies a nested object's constants", () => {
|
||||
// The overlay replaces the whole nested object rather than merging into it,
|
||||
// so a link naming `{ config: { kind: "sms" } }` would otherwise slip past
|
||||
|
|
|
|||
|
|
@ -194,6 +194,60 @@ export function collectSchemaDefaults(
|
|||
return result;
|
||||
}
|
||||
|
||||
/**
|
||||
* Seed a schema's defaults underneath values a caller already holds, merging
|
||||
* the two **per level** rather than with one shallow spread.
|
||||
*
|
||||
* The App deep link is the caller: it holds `appArgs` and needs the defaults
|
||||
* the form would otherwise display. A shallow `{ ...defaults, ...args }`
|
||||
* replaces a nested object wholesale, so `{ config: { kind: "sms" } }` discards
|
||||
* the nested SMS branch's own defaults — which the form then shows while the
|
||||
* submitted arguments omit them (#2123). Recursing also lets each nested level
|
||||
* pick its branch from the nested values, rather than from the top-level ones.
|
||||
*
|
||||
* The supplied value always wins where the two meet; only what it does not
|
||||
* mention is seeded.
|
||||
*/
|
||||
export function seedSchemaValues(
|
||||
schema: InspectorFormSchema,
|
||||
suppliedValues: Record<string, unknown>,
|
||||
): Record<string, unknown> {
|
||||
const { base, branches } = resolveRootUnion(schema);
|
||||
const selected = selectBranchIndex(branches, suppliedValues) ?? 0;
|
||||
const effective = branches[selected]?.schema ?? base;
|
||||
const properties = effective.properties ?? {};
|
||||
|
||||
const merged: Record<string, unknown> = {
|
||||
...collectSchemaDefaults(effective, suppliedValues),
|
||||
};
|
||||
for (const [name, value] of Object.entries(suppliedValues)) {
|
||||
const fieldSchema = normalizeNullableUnion(properties[name] ?? {});
|
||||
const mergedValue =
|
||||
isPlainObject(value) &&
|
||||
fieldSchema.type === "object" &&
|
||||
declaresAnyFields(fieldSchema)
|
||||
? seedSchemaValues(
|
||||
fieldSchema,
|
||||
// The nested defaults are re-derived by the recursion, so only the
|
||||
// supplied half is passed down.
|
||||
value,
|
||||
)
|
||||
: value;
|
||||
Object.defineProperty(merged, name, {
|
||||
value: mergedValue,
|
||||
writable: true,
|
||||
enumerable: true,
|
||||
configurable: true,
|
||||
});
|
||||
}
|
||||
return merged;
|
||||
}
|
||||
|
||||
/** Whether a value is a plain object a nested schema could describe. */
|
||||
function isPlainObject(value: unknown): value is Record<string, unknown> {
|
||||
return typeof value === "object" && value !== null && !Array.isArray(value);
|
||||
}
|
||||
|
||||
/**
|
||||
* Overwrite every `const`-pinned field with the value its schema fixes.
|
||||
*
|
||||
|
|
|
|||
Loading…
Reference in a new issue