Skip to content

Commit 43ca4b2

Browse files
authored
refactor(frontend): avoid redundant validateOperator call in applyOperatorBorder (#5702)
### What changes were proposed in this PR? WorkflowEditorComponent.applyOperatorBorder(operatorID)` always recomputes validation as its first step: ```typescript const validation = this.validationWorkflowService.validateOperator(operatorID); ``` This is redundant when the helper is called from the validation-stream subscriber in handleOperatorValidation, the stream's emitted event already carries the Validation result that was just computed by updateValidationState. This PR: - Adds an optional validation?: Validation parameter to applyOperatorBorder. The helper uses it when provided, and falls back to validateOperator(operatorID) otherwise. - Updates handleOperatorValidation to pass value.validation from the stream into the helper. - Leaves the operator-add stream subscriber unchanged and hence it doesn't have a Validation in hand at that point, so it correctly falls through to the lazy-fetch path. - Functionally identical (the stream emits the same Validation that would have been recomputed), purely avoids the duplicate call. Originally flagged as an optional nit during the PR #5146 review. Attempted in PR #5626 but split out as per the reviewer's request so that PR could stay scoped to test restructuring; this PR completes the follow-up. ### Any related issues, documentation, discussions? Closes #5683 Related: PR #5146 (where the nit was raised), PR #5626 (where this was attempted and split out). ### How was this PR tested? Added one focused unit test in `workflow-editor.component.spec.ts` inside the existing `operator border restoration after navigation` describe block: `it("uses the Validation passed in instead of recomputing it", ...)`. The test clears the `validateOperator` spy after the operator-add validation chain settles, then calls `applyOperatorBorder` directly with a `Validation` argument and asserts `validateOperator` is not called. Verified locally: - `tsc --noEmit`: clean - `eslint`: clean - `ng test` (jsdom): 26/26 pass in the editor spec (was 25, new test adds one) - `ng run gui:test-browser`: 13/13 pass ### Was this PR authored or co-authored using generative AI tooling? This PR was co-authored using Claude Code (Anthropic Claude Opus 4.7)
1 parent d4eb9f7 commit 43ca4b2

2 files changed

Lines changed: 37 additions & 5 deletions

File tree

frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -910,6 +910,34 @@ describe("WorkflowEditorComponent", () => {
910910

911911
expect(getStroke(mockScanPredicate.operatorID)).toBe("red");
912912
});
913+
914+
it("uses the Validation passed in instead of recomputing it", () => {
915+
// Let the validation chain settle from the operator-add so the spy
916+
// below is created after those calls and starts with a clean slate.
917+
workflowActionService.addOperator(mockScanPredicate, mockPoint);
918+
fixture.detectChanges();
919+
920+
const validateSpy = vi.spyOn(validationWorkflowService, "validateOperator");
921+
922+
// Call the helper directly with a Validation argument, mirroring what
923+
// the validation-stream subscriber does at runtime
924+
// (handleOperatorValidation passes value.validation through).
925+
(component as any).applyOperatorBorder(mockScanPredicate.operatorID, { isValid: true });
926+
927+
expect(validateSpy).not.toHaveBeenCalled();
928+
});
929+
930+
it("honors the passed-in Validation result (paints red when it is invalid)", () => {
931+
// Proves the passed-in value actually drives the border, not just that
932+
// the recompute is skipped: an invalid result must paint red.
933+
vi.spyOn(workflowStatusService, "getCurrentStatus").mockReturnValue({});
934+
workflowActionService.addOperator(mockScanPredicate, mockPoint);
935+
fixture.detectChanges();
936+
937+
(component as any).applyOperatorBorder(mockScanPredicate.operatorID, { isValid: false, messages: {} });
938+
939+
expect(getStroke(mockScanPredicate.operatorID)).toBe("red");
940+
});
913941
});
914942
});
915943
});

frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts

Lines changed: 9 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -25,7 +25,7 @@ import { DragDropService } from "../../service/drag-drop/drag-drop.service";
2525
import { DynamicSchemaService } from "../../service/dynamic-schema/dynamic-schema.service";
2626
import { ExecuteWorkflowService } from "../../service/execute-workflow/execute-workflow.service";
2727
import { fromJointPaperEvent, JointUIService, linkPathStrokeColor } from "../../service/joint-ui/joint-ui.service";
28-
import { ValidationWorkflowService } from "../../service/validation/validation-workflow.service";
28+
import { Validation, ValidationWorkflowService } from "../../service/validation/validation-workflow.service";
2929
import { WorkflowActionService } from "../../service/workflow-graph/model/workflow-action.service";
3030
import { WorkflowStatusService } from "../../service/workflow-status/workflow-status.service";
3131
import { ExecutionState, OperatorState } from "../../types/execute-workflow.interface";
@@ -397,10 +397,14 @@ export class WorkflowEditorComponent implements OnInit, AfterViewInit, OnDestroy
397397
* Centralizing this here avoids the race where the validation pass
398398
* overwrites a state-derived stroke (or vice versa) for an operator that
399399
* is both invalid and has a cached execution status.
400+
*
401+
* Callers that already have a Validation result (the validation stream
402+
* subscriber) may pass it in to avoid recomputing it; callers without one
403+
* (the operator-add stream subscriber) let the helper fetch it lazily.
400404
*/
401-
private applyOperatorBorder(operatorID: string): void {
402-
const validation = this.validationWorkflowService.validateOperator(operatorID);
403-
if (!validation.isValid) {
405+
private applyOperatorBorder(operatorID: string, validation?: Validation): void {
406+
const resolved = validation ?? this.validationWorkflowService.validateOperator(operatorID);
407+
if (!resolved.isValid) {
404408
this.jointUIService.changeOperatorColor(this.paper, operatorID, false);
405409
return;
406410
}
@@ -1025,7 +1029,7 @@ export class WorkflowEditorComponent implements OnInit, AfterViewInit, OnDestroy
10251029
this.validationWorkflowService
10261030
.getOperatorValidationStream()
10271031
.pipe(untilDestroyed(this))
1028-
.subscribe(value => this.applyOperatorBorder(value.operatorID));
1032+
.subscribe(value => this.applyOperatorBorder(value.operatorID, value.validation));
10291033
}
10301034

10311035
/**

0 commit comments

Comments
 (0)