From fee24f174b37d152deb74e1a2917594e829eff0d Mon Sep 17 00:00:00 2001 From: Lucio Lelii Date: Thu, 3 Sep 2026 16:07:33 +0200 Subject: [PATCH] Save the edited global inputs in one request The Save bar fired a request per edited input, all at once, and they all mutated the same execution. That is how two typed values went missing: the bulk global endpoint replaced the whole set of globals, so the list input's save landed last and took its neighbours down with it. The endpoint now merges (service-side fix), but a save should not depend on request ordering to be correct. Every edited global now goes in a single PUT /executions/{id}/globals, and the node inputs follow one at a time - there is no bulk endpoint per step, so the best available is not to have them in flight together. The single-input save uses the same two requests, a global batch of exactly one, so there is one code path and one value normalisation instead of a second copy that could drift. planInputSaves and preparedInputValue are pure and live with the other viewer utils, which is what made them testable: the component has no spec harness (14 injected services), and the parts worth pinning are which endpoint gets called and what shape the value takes. The fake now writes globals to context.globalInputs and the descriptors, where the viewer actually reads them. The older single-key fakes only ever touched context.inputs, so a saved global never showed up in development at all. A failed batch reports the same error on every input in it: it failed as a batch, and guessing a culprit would be worse than saying so. 487 frontend tests green. Co-Authored-By: Claude Opus 5 (1M context) --- .../task-executions-call.base.ts | 8 ++ .../task-executions-call.fake.ts | 23 ++++ .../task-executions/task-executions-call.ts | 8 ++ .../task-executions/task-executions.ts | 7 + .../execution-viewer.utils.spec.ts | 104 +++++++++++++- .../execution-viewer.utils.ts | 46 +++++++ .../task-execution-viewer.ts | 128 ++++++++++-------- 7 files changed, 269 insertions(+), 55 deletions(-) diff --git a/src/app/services/task-executions/task-executions-call.base.ts b/src/app/services/task-executions/task-executions-call.base.ts index 58b6764..3686122 100644 --- a/src/app/services/task-executions/task-executions-call.base.ts +++ b/src/app/services/task-executions/task-executions-call.base.ts @@ -80,6 +80,14 @@ export abstract class TaskExecutionsCallServiceBase { inputName: string, values: string[] ): Observable; + /** + * Sets several global inputs in one request. The single-key endpoint had to be called once per + * input, and those calls raced each other on the same execution. + */ + abstract prepareGlobalInputs( + executionId: string, + values: Record + ): Observable; abstract prepareGlobalFileInput( executionId: string, inputName: string, diff --git a/src/app/services/task-executions/task-executions-call.fake.ts b/src/app/services/task-executions/task-executions-call.fake.ts index bd2dc31..39c8d8d 100644 --- a/src/app/services/task-executions/task-executions-call.fake.ts +++ b/src/app/services/task-executions/task-executions-call.fake.ts @@ -901,6 +901,29 @@ export class TaskExecutionsCallServiceFake extends TaskExecutionsCallServiceBase return of(execution); } + override prepareGlobalInputs( + executionId: string, + values: Record + ): Observable { + const execution = this.findExecution(executionId); + // The viewer reads globals from context.globalInputs and the descriptors, so write them there: + // the older single-key fakes only ever touched context.inputs, where nothing looks for them. + execution.context.globalInputs = { ...(execution.context.globalInputs ?? {}) }; + execution.context.globalInputDescriptors = { ...(execution.context.globalInputDescriptors ?? {}) }; + for (const [inputName, value] of Object.entries(values)) { + execution.context.globalInputs[inputName] = value; + const descriptor = execution.context.globalInputDescriptors[inputName]; + if (descriptor) { + execution.context.globalInputDescriptors[inputName] = { ...descriptor, value }; + } + } + const provided = new Set(Object.keys(values)); + execution.missingGlobalInputKeys = (execution.missingGlobalInputKeys ?? []) + .filter((key) => !provided.has(key)); + execution.context.status = execution.context.waitingSteps.length ? 'WAITING' : execution.context.status; + return of(execution); + } + override prepareGlobalFileInput( executionId: string, inputName: string, diff --git a/src/app/services/task-executions/task-executions-call.ts b/src/app/services/task-executions/task-executions-call.ts index 88b8f0e..dcf2f32 100644 --- a/src/app/services/task-executions/task-executions-call.ts +++ b/src/app/services/task-executions/task-executions-call.ts @@ -256,6 +256,14 @@ export class TaskExecutionsCallService extends TaskExecutionsCallServiceBase { }).pipe(map((raw) => this.mapExecution(raw))); } + override prepareGlobalInputs( + executionId: string, + values: Record + ): Observable { + const url = `${environment.apiUrl}/executions/${encodeURIComponent(executionId)}/globals`; + return this.http.put(url, values).pipe(map((raw) => this.mapExecution(raw))); + } + override prepareGlobalFileInput( executionId: string, inputName: string, diff --git a/src/app/services/task-executions/task-executions.ts b/src/app/services/task-executions/task-executions.ts index 1f40a9b..38e3ad2 100644 --- a/src/app/services/task-executions/task-executions.ts +++ b/src/app/services/task-executions/task-executions.ts @@ -336,6 +336,13 @@ export class TaskExecutionsService { ); } + prepareGlobalInputs(executionId: string, values: Record) { + return this.withRefreshAndErrorHandling( + this.taskExecutionsCallService.prepareGlobalInputs(executionId, values), + 'Prepare global inputs failed' + ); + } + prepareGlobalFileInput(executionId: string, inputName: string, file: File) { return this.withRefreshAndErrorHandling( this.taskExecutionsCallService.prepareGlobalFileInput(executionId, inputName, file), diff --git a/src/app/shared/task-execution-viewer/execution-viewer.utils.spec.ts b/src/app/shared/task-execution-viewer/execution-viewer.utils.spec.ts index b27600b..9febdc9 100644 --- a/src/app/shared/task-execution-viewer/execution-viewer.utils.spec.ts +++ b/src/app/shared/task-execution-viewer/execution-viewer.utils.spec.ts @@ -1,5 +1,14 @@ import { TaskExecution, TaskExecutionStep } from '@models/task-execution'; -import { buildAuthorizationGate, buildVisibleExecutionLogs, getExecutionInputValues, getExecutionOutputValues, hasStoredValue, isExecutionStartable } from './execution-viewer.utils'; +import { + buildAuthorizationGate, + buildVisibleExecutionLogs, + getExecutionInputValues, + getExecutionOutputValues, + hasStoredValue, + isExecutionStartable, + planInputSaves, + preparedInputValue +} from './execution-viewer.utils'; describe('execution viewer runtime values', () => { const documentedStep: TaskExecutionStep = { @@ -188,3 +197,96 @@ describe('hasStoredValue', () => { expect(hasStoredValue(false)).toBe(true); }); }); + +describe('planInputSaves', () => { + function input(overrides: Record = {}): any { + return { + key: 'global:role', scope: 'global', nodeId: null, inputName: 'role', + title: 'Flow', subtitle: 'role', type: 'TEXT', multiple: false, value: '', provided: false, + ...overrides + }; + } + + it('puts every edited global in one map, so one request can carry them all', () => { + // A request per global, fired in parallel, is what lost the values: the bulk endpoint replaced + // the whole set, so whichever save landed last wiped its neighbours. + const inputs = [ + input({ key: 'g:title', inputName: 'positionTitle' }), + input({ key: 'g:req', inputName: 'jobRequirements' }), + input({ key: 'g:questions', inputName: 'interviewQuestions', multiple: true, value: [] }) + ]; + + const plan = planInputSaves(inputs, { + 'g:title': 'Backend Developer', + 'g:req': 'At least 3 years', + 'g:questions': ['first', 'second'] + }); + + expect(plan.globals).toHaveLength(3); + expect(plan.globalValues).toEqual({ + positionTitle: 'Backend Developer', + jobRequirements: 'At least 3 years', + interviewQuestions: ['first', 'second'] + }); + expect(plan.nodeInputs).toEqual([]); + }); + + it('keeps node inputs separate: there is no bulk endpoint per step', () => { + const inputs = [ + input({ key: 'g:role', inputName: 'role' }), + input({ key: 'step-1:cv', scope: 'node', nodeId: 'step-1', inputName: 'cv' }) + ]; + + const plan = planInputSaves(inputs, { 'g:role': 'HR', 'step-1:cv': 'a cv' }); + + expect(Object.keys(plan.globalValues)).toEqual(['role']); + expect(plan.nodeInputs.map((one) => one.key)).toEqual(['step-1:cv']); + }); + + it('ignores anything the user has not edited', () => { + const inputs = [ + input({ key: 'g:role', inputName: 'role', value: 'stored' }), + input({ key: 'g:other', inputName: 'other', value: 'also stored' }) + ]; + + const plan = planInputSaves(inputs, { 'g:role': 'edited' }); + + // Sending an untouched value back would be a write the user did not ask for. + expect(plan.globalValues).toEqual({ role: 'edited' }); + expect(plan.globals).toHaveLength(1); + }); + + it('sends an emptied field, which is an edit like any other', () => { + const plan = planInputSaves([input({ key: 'g:role', inputName: 'role', value: 'was set' })], + { 'g:role': '' }); + + expect(plan.globalValues).toEqual({ role: '' }); + }); +}); + +describe('preparedInputValue', () => { + function listInput(): any { + return { key: 'g:q', scope: 'global', nodeId: null, inputName: 'q', title: 'Flow', + subtitle: 'q', type: 'TEXT', multiple: true, value: [], provided: false }; + } + + it('drops blank items and trims the rest of a list', () => { + expect(preparedInputValue(listInput(), [' first ', '', ' ', 'second'])) + .toEqual(['first', 'second']); + }); + + it('sends a single value as a string, not as a one-item list', () => { + const single: any = { ...listInput(), multiple: false, key: 'g:role', inputName: 'role' }; + expect(preparedInputValue(single, 'Backend Developer')).toBe('Backend Developer'); + }); + + it('takes the first item when a single-valued input somehow holds a list', () => { + const single: any = { ...listInput(), multiple: false }; + expect(preparedInputValue(single, ['first', 'second'])).toBe('first'); + }); + + it('falls back to the stored value when there is no pending edit', () => { + const single: any = { ...listInput(), multiple: false, value: 'stored' }; + expect(preparedInputValue(single, undefined)).toBe('stored'); + }); +}); diff --git a/src/app/shared/task-execution-viewer/execution-viewer.utils.ts b/src/app/shared/task-execution-viewer/execution-viewer.utils.ts index 3b94c19..fff5562 100644 --- a/src/app/shared/task-execution-viewer/execution-viewer.utils.ts +++ b/src/app/shared/task-execution-viewer/execution-viewer.utils.ts @@ -17,6 +17,7 @@ import { TaskExecutionStep, } from '@models/task-execution'; import { LlmProviderCapability } from '@models/llm-provider'; +import { EditableExecutionInput } from '@shared/task-execution-inputs-panel/task-execution-inputs-panel'; export type ExecutionOutputEntry = { key: string; @@ -547,3 +548,48 @@ export function hasStoredValue(value: unknown): boolean { } return String(value).trim().length > 0; } + +/** + * What one Save has to send: every edited global in a single map, and the node inputs one by one. + * + * Splitting it this way is the point. A request per input, fired in parallel, had them all mutating + * the same execution at once - and the bulk global endpoint replaced the whole set, so the list + * input's save arrived last and took the neighbouring typed values with it. + */ +export type PlannedInputSaves = { + globals: EditableExecutionInput[]; + globalValues: Record; + nodeInputs: EditableExecutionInput[]; +}; + +/** The value as the API wants it: a trimmed list of non-empty items, or a single string. */ +export function preparedInputValue( + input: EditableExecutionInput, + pendingValue: string | string[] | undefined +): string | string[] { + const value = pendingValue ?? normalizeEditableInputValue(input.value, input.multiple); + if (input.multiple) { + return (Array.isArray(value) ? value : [String(value)]) + .map((item) => String(item).trim()) + .filter((item) => item.length > 0); + } + return String(Array.isArray(value) ? value[0] ?? '' : value); +} + +export function planInputSaves( + editableInputs: EditableExecutionInput[], + pending: Record +): PlannedInputSaves { + const edited = editableInputs + .filter((input) => Object.prototype.hasOwnProperty.call(pending, input.key)); + const globals = edited.filter((input) => input.scope === 'global'); + const globalValues: Record = {}; + for (const input of globals) { + globalValues[input.inputName] = preparedInputValue(input, pending[input.key]); + } + return { + globals, + globalValues, + nodeInputs: edited.filter((input) => input.scope !== 'global') + }; +} diff --git a/src/app/shared/task-execution-viewer/task-execution-viewer.ts b/src/app/shared/task-execution-viewer/task-execution-viewer.ts index da55f60..1f4d75d 100644 --- a/src/app/shared/task-execution-viewer/task-execution-viewer.ts +++ b/src/app/shared/task-execution-viewer/task-execution-viewer.ts @@ -57,7 +57,7 @@ import { BiasCompareDialogService } from '@services/dialogs/bias-compare-dialog' import { BiasComparisonViewStateService } from '@services/bias/bias-comparison-view-state'; import { BiasImpactReportListComponent } from '@shared/bias-impact-report-list/bias-impact-report-list'; import { JsonViewerComponent } from '@shared/json-viewer/json-viewer'; -import { firstValueFrom, Observable, of, take, tap } from 'rxjs'; +import { catchError, concat, firstValueFrom, Observable, of, take, tap } from 'rxjs'; import { ExecutionOutputEntry, ExecutionOutputGroup, @@ -78,6 +78,8 @@ import { buildExecutionIntermediateInputGroups, buildVisibleExecutionLogs, normalizeEditableInputValue, + planInputSaves, + preparedInputValue, getExecutionInputValues, getExecutionOutputValues, getConnectedInputs, @@ -1119,11 +1121,17 @@ export class TaskExecutionViewerComponent implements OnDestroy { }); } + /** Saves one input, over the same requests the Save bar uses - a global batch of exactly one. */ submitTextInput(input: EditableExecutionInput) { if (this.inputsReadOnly()) return; const executionId = this.execution()?.id; if (!executionId) return; - this.sendPreparedTextInput(input, executionId); + const request$ = input.scope === 'global' + ? this.bulkGlobalRequest([input], + { [input.inputName]: preparedInputValue(input, this.pendingTextInputs()[input.key]) }, + executionId) + : this.singleInputRequest(input, executionId); + request$.subscribe(); } onFileInputChange(input: EditableExecutionInput, files: File[]) { @@ -1244,65 +1252,77 @@ export class TaskExecutionViewerComponent implements OnDestroy { } /** - * Saves every edited input in one go. Each still goes through the same single-input request the - * per-field button used - only the trigger is shared - so a failure is reported per input. + * Saves every edited input. + * + *

The globals go in a single bulk request, and the node inputs follow one at a time. Firing a + * request per input in parallel had them all mutating the same execution at once, and the values + * the user had just typed could come back missing. */ submitAllTextInputs() { if (this.inputsReadOnly()) return; - const pending = new Set(Object.keys(this.pendingTextInputs())); - this.editableInputs() - .filter((input) => pending.has(input.key)) - .forEach((input) => this.submitTextInput(input)); + const executionId = this.execution()?.id; + if (!executionId) return; + + const plan = planInputSaves(this.editableInputs(), this.pendingTextInputs()); + const steps: Observable[] = []; + if (plan.globals.length) { + steps.push(this.bulkGlobalRequest(plan.globals, plan.globalValues, executionId)); + } + for (const input of plan.nodeInputs) { + steps.push(this.singleInputRequest(input, executionId)); + } + if (!steps.length) return; + + // Sequential: one request at a time on one execution, which is the whole point of the change. + concat(...steps).subscribe(); } - private sendPreparedTextInput(input: EditableExecutionInput, executionId: string) { - if (this.inputsReadOnly() || this.execution()?.id !== executionId) return; + /** One request for every edited global, so they cannot overwrite one another. */ + private bulkGlobalRequest( + globals: EditableExecutionInput[], + values: Record, + executionId: string + ): Observable { + globals.forEach((input) => this.setInputSaving(input.key, true)); - const value = this.pendingTextInputs()[input.key] ?? normalizeEditableInputValue(input.value, input.multiple); - this.setInputSaving(input.key, true); - const normalizedValues = (Array.isArray(value) ? value : [String(value)]) - .map((item) => item.trim()) - .filter((item) => item.length > 0); - const request$ = input.scope === 'global' - ? ( - input.multiple - ? this.taskExecutionsService.prepareGlobalStringArrayInput( - executionId, - input.inputName, - normalizedValues - ) - : this.taskExecutionsService.prepareGlobalStringInput( - executionId, - input.inputName, - String(Array.isArray(value) ? value[0] ?? '' : value) - ) - ) - : ( - input.multiple - ? this.taskExecutionsService.prepareStringArrayInput( - executionId, - input.nodeId!, - input.inputName, - normalizedValues - ) - : this.taskExecutionsService.prepareStringInput( - executionId, - input.nodeId!, - input.inputName, - String(Array.isArray(value) ? value[0] ?? '' : value) - ) - ); - - request$.subscribe({ - next: () => { - this.pendingTextInputs.update((current) => { - const next = { ...current }; - delete next[input.key]; - return next; - }); + return this.taskExecutionsService.prepareGlobalInputs(executionId, values).pipe( + tap(() => globals.forEach((input) => { + this.clearPendingInput(input.key); this.clearInputSaving(input.key); - }, - error: () => this.setInputError(input.key, 'Failed to update input') + })), + // The batch failed as a batch, so say so on each input in it rather than guessing a culprit. + catchError(() => { + globals.forEach((input) => this.setInputError(input.key, 'Failed to update inputs')); + return of(null); + }) + ); + } + + /** A node input still goes one at a time: there is no bulk endpoint per step. */ + private singleInputRequest(input: EditableExecutionInput, executionId: string): Observable { + const value = preparedInputValue(input, this.pendingTextInputs()[input.key]); + this.setInputSaving(input.key, true); + const request$ = Array.isArray(value) + ? this.taskExecutionsService.prepareStringArrayInput(executionId, input.nodeId!, input.inputName, value) + : this.taskExecutionsService.prepareStringInput(executionId, input.nodeId!, input.inputName, value); + + return request$.pipe( + tap(() => { + this.clearPendingInput(input.key); + this.clearInputSaving(input.key); + }), + catchError(() => { + this.setInputError(input.key, 'Failed to update input'); + return of(null); + }) + ); + } + + private clearPendingInput(key: string) { + this.pendingTextInputs.update((current) => { + const next = { ...current }; + delete next[key]; + return next; }); }