fix: address Copilot review round 10 — structural const match, deep-link constants
- selectBranchIndex compares a discriminator structurally: a `const` may be an object or array, and deep-link arguments arrive as freshly parsed instances that could never be reference-equal to the schema's own. - New applySchemaConstants, applied by InspectorView AFTER the deep link's `appArgs` are spread over the seeded defaults. The form renders a pinned field read-only, so a link disagreeing with one would otherwise auto-open an App with a hidden value contradicting the shape on screen. Signed-off-by: cliffhall <cliff@futurescale.com>
This commit is contained in:
parent
246ee1c262
commit
775fec0f6a
5 changed files with 91 additions and 8 deletions
|
|
@ -103,7 +103,11 @@ import {
|
|||
correlatedFetchStatusById,
|
||||
revealableMessageIds,
|
||||
} from "../../../utils/correlateTransportErrors";
|
||||
import { collectSchemaDefaults, toFormSchema } from "../../../utils/jsonUtils";
|
||||
import {
|
||||
applySchemaConstants,
|
||||
collectSchemaDefaults,
|
||||
toFormSchema,
|
||||
} from "../../../utils/jsonUtils";
|
||||
import { MONITOR_COLUMN_ANIM_MS } from "./monitorColumnAnimation";
|
||||
|
||||
const SORT_DEFAULT: SortDirection = "newest-first";
|
||||
|
|
@ -1040,13 +1044,15 @@ export function InspectorView({
|
|||
// than the first, and defaults seeded from the wrong branch would sit in
|
||||
// the submitted arguments where the form — showing the branch the args
|
||||
// identify — never displays them (#2123).
|
||||
const formValues = {
|
||||
...collectSchemaDefaults(
|
||||
toFormSchema(target.inputSchema) ?? {},
|
||||
deepLink.appArgs ?? {},
|
||||
),
|
||||
// …and the schema's constants are re-applied *after* the overlay: a field
|
||||
// the form renders read-only cannot be corrected by the user, so a deep
|
||||
// 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,
|
||||
};
|
||||
});
|
||||
// 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
|
||||
|
|
|
|||
|
|
@ -695,6 +695,25 @@ describe("resolveRootUnion", () => {
|
|||
expect(selectBranchIndex(undiscriminated, {})).toBeNull();
|
||||
});
|
||||
|
||||
it("matches an object-valued discriminator structurally", () => {
|
||||
// Deep-link arguments are freshly parsed instances, never `===` the
|
||||
// schema's own constant.
|
||||
const objectPinned = resolveRootUnion({
|
||||
type: "object",
|
||||
anyOf: [
|
||||
{
|
||||
type: "object",
|
||||
properties: { tag: { const: { a: 1 } }, x: { type: "string" } },
|
||||
},
|
||||
{
|
||||
type: "object",
|
||||
properties: { tag: { const: { a: 2 } }, y: { type: "string" } },
|
||||
},
|
||||
],
|
||||
}).branches;
|
||||
expect(selectBranchIndex(objectPinned, { tag: { a: 2 } })).toBe(1);
|
||||
});
|
||||
|
||||
it("reports none when the values identify nothing", () => {
|
||||
expect(selectBranchIndex(branches, {})).toBeNull();
|
||||
expect(selectBranchIndex(branches, { kind: "other" })).toBeNull();
|
||||
|
|
|
|||
|
|
@ -6,6 +6,7 @@ import {
|
|||
getValueAtPath,
|
||||
collectSchemaDefaults,
|
||||
hasMissingRequiredFields,
|
||||
applySchemaConstants,
|
||||
} from "./jsonUtils";
|
||||
import type { InspectorFormSchema } from "./jsonUtils";
|
||||
|
||||
|
|
@ -432,6 +433,28 @@ describe("root composition (#2123)", () => {
|
|||
expect(hasMissingRequiredFields(schema, values)).toBe(false);
|
||||
});
|
||||
|
||||
it("re-applies a branch's constants over conflicting supplied values", () => {
|
||||
// A read-only field cannot be corrected by the user, so a deep link
|
||||
// disagreeing with a `const` must not survive into the submitted arguments.
|
||||
expect(applySchemaConstants(UNION, { kind: "sms", note: "hi" })).toEqual({
|
||||
kind: "sms",
|
||||
note: "hi",
|
||||
});
|
||||
expect(applySchemaConstants(UNION, { kind: "nonsense" })).toEqual({
|
||||
kind: "email",
|
||||
});
|
||||
});
|
||||
|
||||
it("leaves values alone when nothing is pinned", () => {
|
||||
const values = { a: 1 };
|
||||
expect(
|
||||
applySchemaConstants(
|
||||
{ type: "object", properties: { a: { type: "number" } } },
|
||||
values,
|
||||
),
|
||||
).toBe(values);
|
||||
});
|
||||
|
||||
it("blocks submission while no branch is satisfied", () => {
|
||||
expect(hasMissingRequiredFields(UNION, {})).toBe(true);
|
||||
expect(hasMissingRequiredFields(UNION, { kind: "email" })).toBe(true);
|
||||
|
|
|
|||
|
|
@ -174,6 +174,36 @@ export function collectSchemaDefaults(
|
|||
return result;
|
||||
}
|
||||
|
||||
/**
|
||||
* Overwrite every `const`-pinned field with the value its schema fixes.
|
||||
*
|
||||
* The form renders such a field read-only, so a value disagreeing with it can
|
||||
* only have come from outside the form — an App deep link's `appArgs`, which
|
||||
* are spread over the seeded defaults and would otherwise leave the arguments
|
||||
* claiming a shape the user is being shown the opposite of (#2123). Apply this
|
||||
* *after* any such overlay.
|
||||
*
|
||||
* The branch is chosen the same way {@link collectSchemaDefaults} chooses it,
|
||||
* from the values themselves, so the constants applied belong to the shape the
|
||||
* form will display.
|
||||
*/
|
||||
export function applySchemaConstants(
|
||||
schema: InspectorFormSchema,
|
||||
values: Record<string, unknown>,
|
||||
): Record<string, unknown> {
|
||||
const { base, branches } = resolveRootUnion(schema);
|
||||
const selected = selectBranchIndex(branches, values) ?? 0;
|
||||
const properties = (branches[selected]?.schema ?? base).properties ?? {};
|
||||
const pinned = Object.entries(properties).filter(
|
||||
([, fieldSchema]) => fieldSchema.const !== undefined,
|
||||
);
|
||||
if (pinned.length === 0) return values;
|
||||
return Object.fromEntries([
|
||||
...Object.entries(values),
|
||||
...pinned.map(([name, fieldSchema]) => [name, fieldSchema.const]),
|
||||
]);
|
||||
}
|
||||
|
||||
/**
|
||||
* Whether any of the schema's required top-level fields is missing a value in
|
||||
* `values` (absent, null, or empty string). Used to gate a form's submit
|
||||
|
|
|
|||
|
|
@ -605,7 +605,12 @@ export function selectBranchIndex<T extends RootUnionSchema>(
|
|||
const supplied = pinned.filter(([name]) => values[name] !== undefined);
|
||||
if (
|
||||
supplied.length > 0 &&
|
||||
supplied.every(([name, constValue]) => values[name] === constValue)
|
||||
// Structural, not reference: a `const` may be an object or an array, and
|
||||
// deep-link arguments arrive as freshly parsed instances that could never
|
||||
// be `===` the schema's own.
|
||||
supplied.every(([name, constValue]) =>
|
||||
sameValue(values[name], constValue),
|
||||
)
|
||||
) {
|
||||
matches.push(index);
|
||||
}
|
||||
|
|
|
|||
Loading…
Reference in a new issue