From c6060540f946b0c20ad350b566ea3f7862d35ec8 Mon Sep 17 00:00:00 2001 From: Lucio Lelii Date: Fri, 4 Sep 2026 12:39:55 +0200 Subject: [PATCH] Render an optional group as one button that opens a modal Five empty chips for parameters nobody sets on most nodes took more room than the prompt. The group now shows as a single control saying how many of its settings are set, and opens the whole object in one dialog. The write-back reuses the object round trip rather than the array one, so an optional numeric cleared in the modal removes the key instead of persisting 0 - otherwise the provider default would be unreachable, which is the bug fixed yesterday for the inline editor. Clearing everything drops the object entirely, so a saved flow never carries an empty husk that implies a choice was made. A temperature of 0 counts towards the badge: it is the repeatable setting, not an absence, and a collapsed control must never hide a value. Co-Authored-By: Claude Opus 5 (1M context) --- .../nodes/generic-node/generic-node.css | 44 +++++ .../nodes/generic-node/generic-node.html | 32 +++ .../nodes/generic-node/generic-node.spec.ts | 147 ++++++++++++++ .../shared/nodes/generic-node/generic-node.ts | 182 ++++++++++++++++-- .../shared/nodes/schema-driven-fields.spec.ts | 32 +++ 5 files changed, 417 insertions(+), 20 deletions(-) diff --git a/src/app/shared/nodes/generic-node/generic-node.css b/src/app/shared/nodes/generic-node/generic-node.css index 7f0f647..8f3ff24 100644 --- a/src/app/shared/nodes/generic-node/generic-node.css +++ b/src/app/shared/nodes/generic-node/generic-node.css @@ -938,3 +938,47 @@ white-space: normal; line-height: 1.15; } + +/* An optional group is one control, not a box of fields: it reads as a chip but behaves as a + button, and says how many of its settings are set so a collapsed group never hides a choice. */ +.llm-optional-group { + display: flex; + align-items: center; + gap: 8px; + width: 100%; + text-align: left; + cursor: pointer; + font: inherit; + color: inherit; +} + +.llm-optional-group:hover:not(:disabled) { + border-color: #94a3b8; + background: #f8fafc; +} + +.llm-optional-group:disabled { + cursor: default; +} + +.llm-optional-group .llm-param-key { + flex: 1 1 auto; + min-width: 0; +} + +.llm-optional-group-count { + flex: 0 0 auto; + padding: 1px 7px; + border-radius: 999px; + background: #e0e7ff; + color: #3730a3; + font-size: 10px; + font-weight: 600; + letter-spacing: 0.02em; +} + +.llm-optional-group-icon { + flex: 0 0 auto; + color: #64748b; + font-size: 12px; +} diff --git a/src/app/shared/nodes/generic-node/generic-node.html b/src/app/shared/nodes/generic-node/generic-node.html index cdfd4dc..ca64885 100644 --- a/src/app/shared/nodes/generic-node/generic-node.html +++ b/src/app/shared/nodes/generic-node/generic-node.html @@ -348,6 +348,22 @@ } + @if (item.optionalGroupField; as optionalGroup) { + + } @if (item.arrayField; as arrayField) {
@@ -478,6 +494,22 @@
} + @if (item.optionalGroupField; as optionalGroup) { + + } @if (item.arrayField; as arrayField) {
diff --git a/src/app/shared/nodes/generic-node/generic-node.spec.ts b/src/app/shared/nodes/generic-node/generic-node.spec.ts index d821b0a..692d2bb 100644 --- a/src/app/shared/nodes/generic-node/generic-node.spec.ts +++ b/src/app/shared/nodes/generic-node/generic-node.spec.ts @@ -193,6 +193,56 @@ describe('GenericNodeComponent', () => { expect(component.ensureBlockConfiguration()['skills'][0].weight).toBe(0); }); + it('leaves out an emptied optional number instead of writing 0', async () => { + // The behaviour is derived from the schema, not from who is calling: `weight` is required and + // keeps its 0, while an optional sibling is omitted so the default still applies. That is + // what lets one parser serve both an array item and an optional group. + const component = fixture.componentInstance as any; + component.arrayFieldDefinitions = [{ + path: 'skills', + label: 'Skills', + itemSchema: { + type: 'object', + required: ['name', 'weight'], + properties: { + name: { type: 'string' }, + weight: { type: 'integer' }, + temperature: { type: 'number' } + } + }, + uniqueBy: null, + ui: { structural: false, visibleWhen: [], enabledWhen: [] } + }]; + const open = TestBed.inject(NodeSettingsDialogService).open as ReturnType; + open.mockResolvedValue({ name: 'x', weight: '', temperature: '' }); + + await component.addArrayItem('skills'); + + const written = component.ensureBlockConfiguration()['skills'][0]; + expect(written.weight).toBe(0); + expect('temperature' in written).toBe(false); + }); + + it('keeps a typed zero in an optional number, because it is a value', async () => { + const component = fixture.componentInstance as any; + component.arrayFieldDefinitions = [{ + path: 'skills', + label: 'Skills', + itemSchema: { + type: 'object', + properties: { temperature: { type: 'number' } } + }, + uniqueBy: null, + ui: { structural: false, visibleWhen: [], enabledWhen: [] } + }]; + const open = TestBed.inject(NodeSettingsDialogService).open as ReturnType; + open.mockResolvedValue({ temperature: '0' }); + + await component.addArrayItem('skills'); + + expect(component.ensureBlockConfiguration()['skills'][0].temperature).toBe(0); + }); + it('edits an existing item in place rather than appending', async () => { const component = withArrayField(); const config = component.ensureBlockConfiguration(); @@ -442,4 +492,101 @@ describe('GenericNodeComponent', () => { biasAnnotations: [{ id: 'bias-1', category: 'DYNAMIC', issue: 'keep me' }] })); }); + + describe('an optional group', () => { + /** + * Five empty chips for parameters nobody sets on most nodes became one control. The behaviour + * that matters is what the modal writes back: the group must be able to return to "nothing set" + * so the provider default applies again, and a temperature of 0 must survive as a real value. + */ + const groupSchema = { + type: 'object', + properties: { + temperature: { type: 'number', 'x-ui-label': 'Temperature' }, + topK: { type: 'integer', 'x-ui-label': 'Top K' } + } + }; + + function withOptionalGroup(current?: Record) { + const component = fixture.componentInstance as any; + component.optionalGroupFieldDefinitions = [{ + path: 'llmDescriptor.parameters', + label: 'Model parameters', + objectSchema: groupSchema, + ui: { structural: false, visibleWhen: [], enabledWhen: [] } + }]; + const config = component.ensureBlockConfiguration(); + config['llmDescriptor'] = { provider: 'p', model: 'm', ...(current ? { parameters: current } : {}) }; + return component; + } + + it('opens one dialog for the whole group, prefilled with what is set', async () => { + const component = withOptionalGroup({ temperature: 0.7 }); + const open = TestBed.inject(NodeSettingsDialogService).open as ReturnType; + open.mockResolvedValue(null); + + await component.openOptionalGroupEditor('llmDescriptor.parameters'); + + const dialog = open.mock.calls.at(-1)?.[0]; + expect(dialog.title).toBe('Model parameters'); + expect(dialog.fields.map((field: any) => field.key)).toEqual(['temperature', 'topK']); + expect(dialog.initial).toEqual({ temperature: '0.7', topK: '' }); + }); + + it('writes only what was filled in', async () => { + const component = withOptionalGroup(); + const open = TestBed.inject(NodeSettingsDialogService).open as ReturnType; + open.mockResolvedValue({ temperature: '0.7', topK: '' }); + + await component.openOptionalGroupEditor('llmDescriptor.parameters'); + + expect(component.ensureBlockConfiguration()['llmDescriptor'].parameters).toEqual({ temperature: 0.7 }); + }); + + it('keeps a temperature of 0, which is the repeatable setting and not an absence', async () => { + const component = withOptionalGroup(); + const open = TestBed.inject(NodeSettingsDialogService).open as ReturnType; + open.mockResolvedValue({ temperature: '0', topK: '' }); + + await component.openOptionalGroupEditor('llmDescriptor.parameters'); + + expect(component.ensureBlockConfiguration()['llmDescriptor'].parameters).toEqual({ temperature: 0 }); + }); + + it('removes the group entirely when everything is cleared', async () => { + // Otherwise the saved flow keeps an empty object, which reads as "parameters were chosen". + const component = withOptionalGroup({ temperature: 0.7 }); + const open = TestBed.inject(NodeSettingsDialogService).open as ReturnType; + open.mockResolvedValue({ temperature: '', topK: '' }); + + await component.openOptionalGroupEditor('llmDescriptor.parameters'); + + expect('parameters' in component.ensureBlockConfiguration()['llmDescriptor']).toBe(false); + }); + + it('leaves the group untouched when the dialog is cancelled', async () => { + const component = withOptionalGroup({ temperature: 0.7 }); + const open = TestBed.inject(NodeSettingsDialogService).open as ReturnType; + open.mockResolvedValue(null); + + await component.openOptionalGroupEditor('llmDescriptor.parameters'); + + expect(component.ensureBlockConfiguration()['llmDescriptor'].parameters).toEqual({ temperature: 0.7 }); + }); + + it('counts a temperature of 0 as set, so the button never says the group is empty', () => { + const component = withOptionalGroup({ temperature: 0, topK: null }); + + const views = component.optionalGroupViews(); + + expect(views).toHaveLength(1); + expect(views[0]).toMatchObject({ path: 'llmDescriptor.parameters', label: 'Model parameters', setCount: 1 }); + }); + + it('reports nothing set when the group is absent', () => { + const component = withOptionalGroup(); + + expect(component.optionalGroupViews()[0].setCount).toBe(0); + }); + }); }); diff --git a/src/app/shared/nodes/generic-node/generic-node.ts b/src/app/shared/nodes/generic-node/generic-node.ts index d90c31d..9217f10 100644 --- a/src/app/shared/nodes/generic-node/generic-node.ts +++ b/src/app/shared/nodes/generic-node/generic-node.ts @@ -63,6 +63,19 @@ type EditableFieldDefinition = SchemaEditableFieldDefinition; type EditableFieldView = SchemaParameterFieldView; +/** A nested object of optional settings, rendered as one control that opens a dialog. */ +type OptionalGroupFieldDefinition = { + path: string; + label: string; + objectSchema: Record | null; + ui: { + structural: boolean; + visibleWhen: UiConditionRule[]; + enabledWhen: UiConditionRule[]; + group: string | null; + }; +}; + type ArrayFieldDefinition = { path: string; label: string; @@ -89,7 +102,16 @@ type ArrayFieldView = { type RichContentView = SchemaRichContentFieldView; -type ParameterDisplayItem = SchemaDisplayItem; +/** What the card shows for an optional group: its name, and how many of its settings are set. */ +type OptionalGroupView = { + path: string; + label: string; + setCount: number; + enabled: boolean; +}; + +type ParameterDisplayItem = + SchemaDisplayItem; type EditableFieldGroupView = SchemaDisplayGroup; @@ -145,6 +167,7 @@ export class GenericNodeComponent implements OnDestroy { parameterDisplayItems: ParameterDisplayItem[] = []; parameterDisplaySections: ParameterDisplaySection[] = []; arrayFields: ArrayFieldView[] = []; + optionalGroupFields: OptionalGroupView[] = []; name = 'noName'; localEditorOpen = false; @@ -256,6 +279,7 @@ export class GenericNodeComponent implements OnDestroy { this.parameterDisplayItems = []; this.parameterDisplaySections = []; this.arrayFields = []; + this.optionalGroupFields = []; const config = this.ensureBlockConfiguration(); @@ -761,6 +785,7 @@ export class GenericNodeComponent implements OnDestroy { this.schemaRequirements = extractSchemaRequirements(this.blockSchema); this.editableFieldDefinitions = this.buildEditableFieldDefinitions(this.blockSchema); this.arrayFieldDefinitions = this.buildArrayFieldDefinitions(this.blockSchema); + this.optionalGroupFieldDefinitions = this.buildOptionalGroupFieldDefinitions(this.blockSchema); this.pruneInactiveConfiguration(this.ensureBlockConfiguration()); await this.refreshConditionalRequirements(); @@ -832,6 +857,8 @@ export class GenericNodeComponent implements OnDestroy { }); } + optionalGroupFieldDefinitions: OptionalGroupFieldDefinition[] = []; + private buildArrayFieldDefinitions(schema: Record | null): ArrayFieldDefinition[] { return collectSchemaLeafFields(schema, ({ key, path, schema: childResolved, ui }) => { if (childResolved?.['type'] !== 'array') return null; @@ -856,6 +883,56 @@ export class GenericNodeComponent implements OnDestroy { }); } + /** + * A group that holds values must not look like an empty one: starting a run with settings you + * cannot see is exactly what a collapsed control risks. + */ + private optionalGroupViews(): OptionalGroupView[] { + const config = this.blockConfiguration ?? {}; + return this.optionalGroupFieldDefinitions + .filter((definition) => this.isPathVisible(definition.path)) + .map((definition) => { + const value = this.getByPath(config, definition.path); + const values = value && typeof value === 'object' && !Array.isArray(value) + ? Object.values(value as Record) + : []; + return { + path: definition.path, + label: definition.label, + // A temperature of 0 counts: it is the repeatable setting, not an empty box. + setCount: values.filter((entry) => entry !== undefined && entry !== null && entry !== '').length, + enabled: this.isPathEnabled(definition.path) + }; + }); + } + + private buildOptionalGroupFieldDefinitions(schema: Record | null): OptionalGroupFieldDefinition[] { + return collectSchemaLeafFields(schema, ({ path, schema: childResolved, ui }) => { + if (childResolved?.['x-ui-optional-group'] !== true) return null; + + // The label comes from the annotation when given, so a group can be named for what it is + // rather than for the field that happens to hold it. + const label = typeof childResolved?.['x-ui-optional-group-label'] === 'string' + && String(childResolved['x-ui-optional-group-label']).trim().length + ? String(childResolved['x-ui-optional-group-label']).trim() + : schemaFieldLabel(path, childResolved); + + return { + path, + label, + objectSchema: childResolved, + ui: { + structural: ui.structural, + visibleWhen: ui.visibleWhen, + enabledWhen: ui.enabledWhen, + group: ui.group + } + }; + }, { + includeOptionalGroups: true + }); + } + private isStructuralField(path: string): boolean { return getSchemaPathUiMeta(this.blockSchema, path).structural; } @@ -1104,6 +1181,8 @@ export class GenericNodeComponent implements OnDestroy { items: this.toArrayFieldItems(definition, this.getByPath(config, definition.path)) })); + this.optionalGroupFields = this.optionalGroupViews(); + if (this.editableFieldDefinitions.length) { const groupedFields = buildSchemaFieldViewModel({ definitions: this.editableFieldDefinitions, @@ -1132,6 +1211,7 @@ export class GenericNodeComponent implements OnDestroy { fields: allFields, richContentFields: allRichContentFields, arrayFields: this.arrayFields, + optionalGroupFields: this.optionalGroupFields, resolveGroupLabel: (path) => getSchemaPathUiMeta(this.blockSchema, path).group ?? parentGroupLabel(path) }); this.parameterDisplayItems = ordered.rootItems; @@ -1217,10 +1297,53 @@ export class GenericNodeComponent implements OnDestroy { if (key === 'type' || key === 'name' || key.startsWith('__')) return null; return { path }; }, { - includeArrays: true + includeArrays: true, + includeOptionalGroups: true }); } + /** + * Opens the whole group in one dialog. It reuses the round trip the array editor uses - the same + * builder, the same parser, the same post-save tail - so an optional numeric cleared here is + * removed rather than written as 0, exactly as it is when edited inline. + */ + async openOptionalGroupEditor(path: string, event?: Event) { + event?.preventDefault(); + event?.stopPropagation(); + if (this.isReadonly) return; + + const definition = this.optionalGroupFieldDefinitions.find((field) => field.path === path); + if (!definition || !this.isPathVisible(path) || !this.isPathEnabled(path)) return; + + const config = this.ensureBlockConfiguration(); + const current = this.getByPath(config, path); + const currentValue = current && typeof current === 'object' && !Array.isArray(current) + ? { ...(current as Record) } + : {}; + + const dialog = await this.buildObjectDialog(definition.objectSchema, definition.label, currentValue); + if (!dialog) return; + + const result = await this.settingsDialog.open(dialog); + if (!result) return; + + const next = this.parseObjectDialogResult(definition.objectSchema, result, currentValue); + if (Object.keys(next).length) { + setSchemaValueByPath(config, path, next); + } else { + // Nothing set: the key goes away entirely, so the object never appears in a saved flow as an + // empty husk that implies settings were chosen. + deleteSchemaValueByPath(config, path); + } + + if (this.isStructuralField(path)) this.markBlockForServerRecreate(); + this.pruneInactiveConfiguration(config); + this.refreshParameterFields(); + this.refreshValidationState(); + this.markFlowDirty(); + this.maybeCreateBlockOnServer(); + } + async addArrayItem(path: string, event?: Event) { event?.preventDefault(); event?.stopPropagation(); @@ -1265,13 +1388,16 @@ export class GenericNodeComponent implements OnDestroy { const current = this.getByPath(config, path); const items = Array.isArray(current) ? [...current] : []; const currentItem = index == null ? this.createEmptyArrayItem(definition.itemSchema) : this.cloneFlowData(items[index] ?? {}); - const dialog = await this.buildArrayItemDialog(definition, currentItem, index); + const dialog = await this.buildObjectDialog( + definition.itemSchema, + `${index == null ? 'Add' : 'Edit'} ${definition.label} item`, + currentItem); if (!dialog) return; const result = await this.settingsDialog.open(dialog); if (!result) return; - const nextItem = this.parseArrayItemDialogResult(definition, result, currentItem); + const nextItem = this.parseObjectDialogResult(definition.itemSchema, result, currentItem); const duplicateError = this.validateUniqueArrayItem(definition, items, nextItem, index); if (duplicateError) { window.alert(duplicateError); @@ -1295,18 +1421,18 @@ export class GenericNodeComponent implements OnDestroy { this.maybeCreateBlockOnServer(); } - private async buildArrayItemDialog( - definition: ArrayFieldDefinition, - item: Record, - index: number | null + /** Builds a dialog from an object's schema. Serves an array element and an optional group alike. */ + private async buildObjectDialog( + itemSchema: Record | null, + title: string, + item: Record ): Promise { - const itemSchema = definition.itemSchema; const properties = itemSchema?.['properties'] as Record | undefined; const schemaRoot = this.blockSchema ?? itemSchema ?? {}; if (!properties) { return { - title: `${index == null ? 'Add' : 'Edit'} ${definition.label} item`, + title, fields: [ { key: '__raw', @@ -1374,12 +1500,14 @@ export class GenericNodeComponent implements OnDestroy { } return { - title: `${index == null ? 'Add' : 'Edit'} ${definition.label} item`, + title, fields, initial, + // Re-derives itself from the draft, which is how conditional visibility and dependent + // retrievers re-resolve while the dialog is open. onValuesChange: async (draft) => { - const draftItem = this.parseArrayItemDialogResult(definition, draft, item); - const nextDialog = await this.buildArrayItemDialog(definition, draftItem, index); + const draftItem = this.parseObjectDialogResult(itemSchema, draft, item); + const nextDialog = await this.buildObjectDialog(itemSchema, title, draftItem); if (!nextDialog) return null; return { fields: nextDialog.fields, @@ -1389,14 +1517,20 @@ export class GenericNodeComponent implements OnDestroy { }; } - private parseArrayItemDialogResult( - definition: ArrayFieldDefinition, + /** + * Reads a dialog result back into an object, using the object's own schema. + * + * Takes a schema rather than an ArrayFieldDefinition because the same round trip serves an array + * element and an optional group; array-ness only ever showed up in the parameter type. + */ + private parseObjectDialogResult( + itemSchema: Record | null, result: NodeSettingsValues, previousItem: Record ) { - const itemSchema = definition.itemSchema; const properties = itemSchema?.['properties'] as Record | undefined; const schemaRoot = this.blockSchema ?? itemSchema ?? {}; + const requiredKeys = new Set(Array.isArray(itemSchema?.['required']) ? itemSchema['required'] as string[] : []); if (!properties) { try { return JSON.parse(String(result['__raw'] ?? '{}')) as Record; @@ -1441,10 +1575,18 @@ export class GenericNodeComponent implements OnDestroy { } if (propertySchema?.['type'] === 'number' || propertySchema?.['type'] === 'integer') { - const numeric = Number(rawValue ?? 0); - nextItem[key] = Number.isFinite(numeric) - ? (propertySchema['type'] === 'integer' ? Math.trunc(numeric) : numeric) - : 0; + const text = String(rawValue ?? '').trim(); + const numeric = Number(text); + if (!text.length || !Number.isFinite(numeric)) { + // Cleared. For an optional field that means "unset", and the key is left out entirely - + // writing 0 would make the provider default unreachable on the very setting, like a + // temperature, where 0 is itself a meaningful value. A required field keeps the old + // behaviour: there is no "absent" for something the schema insists on. + if (!requiredKeys.has(key)) continue; + nextItem[key] = 0; + continue; + } + nextItem[key] = propertySchema['type'] === 'integer' ? Math.trunc(numeric) : numeric; continue; } diff --git a/src/app/shared/nodes/schema-driven-fields.spec.ts b/src/app/shared/nodes/schema-driven-fields.spec.ts index 4d17562..be8b0b7 100644 --- a/src/app/shared/nodes/schema-driven-fields.spec.ts +++ b/src/app/shared/nodes/schema-driven-fields.spec.ts @@ -382,6 +382,38 @@ describe('schema-driven-fields', () => { ]); }); + it('places an optional group in the ordering, in its own slot', () => { + // The group is one item like an array is, so it must reach the template through a slot of its + // own rather than being mistaken for a field and swept into a grouped fieldset. + const result = buildOrderedSchemaDisplay({ + definitions: [{ path: 'name' }, { path: 'llmDescriptor.parameters' }], + fields: [ + { + path: 'name', + label: 'Name', + value: 'Summarise', + wide: false, + expandable: false, + enabled: true, + type: 'string' as const, + booleanValue: false + } + ], + richContentFields: [], + arrayFields: [], + optionalGroupFields: [ + { path: 'llmDescriptor.parameters', label: 'Model parameters', setCount: 2, enabled: true } + ], + resolveGroupLabel: (path) => path.startsWith('llmDescriptor.') ? 'llm' : null + }); + + expect(result.rootItems.map((item) => item.path)).toEqual(['name', 'llmDescriptor.parameters']); + expect(result.groups).toHaveLength(0); + const group = result.sections.find((section) => section.item?.path === 'llmDescriptor.parameters'); + expect(group?.item?.optionalGroupField).toMatchObject({ label: 'Model parameters', setCount: 2 }); + expect(group?.item?.field).toBeNull(); + }); + it('updates and deletes nested schema values by path', () => { const config: Record = {};