From eef5dfb3ede597ea916ff685c05286569b07b569 Mon Sep 17 00:00:00 2001 From: Lucio Lelii Date: Wed, 9 Sep 2026 12:23:19 +0200 Subject: [PATCH] Stop offering a default on a field the state has made required A schema-driven dialog read only the schema's own `required` list, so a field made required by `x-ui-required-when` - an MCP server's `url` once the catalog is off, its `name` - was drawn with "Use default" and could be saved empty. The inline editor already accounted for the conditional rule; this brings the dialog to the same answer, and the same rule now gates both the button and saving. Co-Authored-By: Claude Opus 5 (1M context) --- .../shared/nodes/schema-driven-fields.spec.ts | 64 +++++++++++++++++++ src/app/shared/nodes/schema-driven-fields.ts | 34 +++++++++- 2 files changed, 95 insertions(+), 3 deletions(-) diff --git a/src/app/shared/nodes/schema-driven-fields.spec.ts b/src/app/shared/nodes/schema-driven-fields.spec.ts index b843336..f04898c 100644 --- a/src/app/shared/nodes/schema-driven-fields.spec.ts +++ b/src/app/shared/nodes/schema-driven-fields.spec.ts @@ -3,8 +3,10 @@ import { isFlowDataFieldPath } from './flow-data-schema-fields'; import { parentGroupLabel } from './node-utility'; +import { validateFieldValue } from '@services/dialogs/node-settings-dialog'; import { buildOrderedSchemaDisplay, + buildSchemaObjectDialog, buildTemplatedRichContentParts, buildSchemaEditableFieldDefinitions, buildSchemaFieldViewModel, @@ -720,4 +722,66 @@ describe('buildTemplatedRichContentParts', () => { expect(buildTemplatedRichContentParts({ subject: ' ' }, 'subject', schema, splitParts)).toEqual([]); }); + + /** + * The MCP server binding, reduced to the two properties that made the dialog misbehave: a + * sourceType with a declared default, and a serverName that only the CATALOG state demands. + */ + const mcpBindingSchema = { + type: 'object', + required: [], + properties: { + sourceType: { type: 'string', enum: ['CATALOG', 'CUSTOM'], default: 'CATALOG', title: 'Source type' }, + serverName: { + type: 'string', + title: 'Server name', + // The key the producer emits for @UiRequiredWhen(equalsAny = ...) is `in`, not `equalsAny`. + 'x-ui-required-when': { field: 'sourceType', in: ['CATALOG', ''] } + } + } + }; + + const dialogHooks = { + schemaRoot: mcpBindingSchema, + loadOptions: (propertySchema: Record) => { + const values: string[] = propertySchema['enum'] ?? []; + return values.map((value) => ({ label: value, value })); + } + }; + + it('refuses to save a field that the current state requires and nobody filled', async () => { + const dialog = await buildSchemaObjectDialog(mcpBindingSchema, 'MCP server', { sourceType: 'CATALOG' }, dialogHooks); + const serverName = dialog.fields.find((field) => field.key === 'serverName')!; + + // The gap this closes: an MCP server was saved with nothing selected, because the dialog only + // ever read the schema's own `required` and this field is optional there. + expect(serverName.required).toBe(true); + expect(validateFieldValue(serverName, '')).toBe('Required'); + }); + + it('stops requiring it in the state that does not ask for it', async () => { + const dialog = await buildSchemaObjectDialog(mcpBindingSchema, 'MCP server', { sourceType: 'CUSTOM' }, dialogHooks); + const serverName = dialog.fields.find((field) => field.key === 'serverName')!; + + expect(serverName.required).toBe(false); + expect(validateFieldValue(serverName, '')).toBeNull(); + }); + + it('does not offer a default on a field the current state requires', async () => { + const catalog = await buildSchemaObjectDialog(mcpBindingSchema, 'MCP server', { sourceType: 'CATALOG' }, dialogHooks); + const custom = await buildSchemaObjectDialog(mcpBindingSchema, 'MCP server', { sourceType: 'CUSTOM' }, dialogHooks); + + // "Use default" cleared serverName straight into an invalid state, towards a default that does + // not exist. It belongs only where emptying the field is a legitimate answer. + expect(catalog.fields.find((field) => field.key === 'serverName')!.defaultsWhenEmpty).toBe(false); + expect(custom.fields.find((field) => field.key === 'serverName')!.defaultsWhenEmpty).toBe(true); + }); + + it('names the default an empty field is using, when the schema declares one', async () => { + const dialog = await buildSchemaObjectDialog(mcpBindingSchema, 'MCP server', {}, dialogHooks); + const sourceType = dialog.fields.find((field) => field.key === 'sourceType')!; + + expect(sourceType.defaultsWhenEmpty).toBe(true); + expect(sourceType.defaultValue).toBe('CATALOG'); + }); }); diff --git a/src/app/shared/nodes/schema-driven-fields.ts b/src/app/shared/nodes/schema-driven-fields.ts index 2385794..c3045ec 100644 --- a/src/app/shared/nodes/schema-driven-fields.ts +++ b/src/app/shared/nodes/schema-driven-fields.ts @@ -913,8 +913,9 @@ export async function buildSchemaObjectDialog( const fields: NodeSettingField[] = []; const initial: Record = {}; - // Only to tell an optional field from a required one. Requiredness deliberately does not gate - // saving here: an emptied required number still becomes 0, as it always has. + // The schema's own requiredness, which still does not gate saving: an emptied required number + // becomes 0 as it always has, and a blank @ConfigurableAsInput field means "from the input port". + // What does gate saving is a x-ui-required-when rule satisfied by the current draft, below. const requiredKeys = new Set(Array.isArray(objectSchema?.['required']) ? objectSchema['required'] as string[] : []); for (const { key, schema: propertySchema } of orderedSchemaPropertyEntries(objectSchema, schemaRoot)) { @@ -927,6 +928,22 @@ export async function buildSchemaObjectDialog( ); if (!visible) continue; + /* + * Required as things stand, not just required in the abstract. + * + *

Most requiredness in these configurations is conditional: an MCP binding's serverName is + * optional in the schema and mandatory the moment sourceType is CATALOG. Reading only the + * schema's own `required` offered "Use default" on it, which cleared the field straight into an + * invalid state - and towards a default that does not exist. Re-evaluated per render, so the + * button appears and disappears as the field it depends on changes; onValuesChange rebuilds + * this whole dialog from the draft, which is what makes that work. + */ + const requiredWhenRule = readUiConditionRule(propertySchema['x-ui-required-when']); + const requiredByState = requiredWhenRule + ? evaluateUiConditionRule(requiredWhenRule, value, (fieldPath) => resolveSchemaPath(objectSchema, fieldPath)) + : false; + const requiredNow = requiredKeys.has(key) || requiredByState; + if (hooks.dynamic?.isDynamic(propertySchema)) { const dynamicFields = await hooks.dynamic.buildFields(key, propertySchema, value); fields.push(...dynamicFields.fields); @@ -963,8 +980,19 @@ export async function buildSchemaObjectDialog( minLength: isNumeric ? undefined : fieldUi.minLength, maxLength: isNumeric ? undefined : fieldUi.maxLength, pattern: isNumeric ? undefined : fieldUi.pattern, + /* + * Gated on the conditional rule alone, not on the schema's own `required`. + * + *

A statically required field is knowingly left blank in one case: a + * @ConfigurableAsInput field - an LLM descriptor's model, say - stays required in the + * published schema while blank means "comes from the node input", which is why saving was + * never gated on it. A x-ui-required-when rule makes no such claim: it says this field must + * be filled in the state the dialog is in, and an MCP binding with sourceType CATALOG and no + * serverName was being saved as valid. + */ + required: requiredByState, // A checkbox is excluded because it has no empty state: false is a value, not an absence. - defaultsWhenEmpty: !requiredKeys.has(key) && fieldType !== 'checkbox', + defaultsWhenEmpty: !requiredNow && fieldType !== 'checkbox', defaultValue: propertySchema?.['default'] == null ? undefined : String(propertySchema['default']), readonly: !fieldUi.enabledWhen.every((rule) => evaluateUiConditionRule(rule, value, (fieldPath) => resolveSchemaPath(objectSchema, fieldPath))