This is an automated email from the ASF dual-hosted git repository. github-merge-queue[bot] pushed a commit to branch gh-readonly-queue/main/pr-5702-d4eb9f7891a3289bf034b10ff59c9034449b5c75 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 43ca4b2c70849af57656bc9a777d0e53f617bcc4 Author: Prateek Ganigi <[email protected]> AuthorDate: Wed Jun 24 22:55:06 2026 -0700 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) --- .../workflow-editor.component.spec.ts | 28 ++++++++++++++++++++++ .../workflow-editor/workflow-editor.component.ts | 14 +++++++---- 2 files changed, 37 insertions(+), 5 deletions(-) diff --git a/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts b/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts index c87ea058e7..81a84081a5 100644 --- a/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts +++ b/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts @@ -910,6 +910,34 @@ describe("WorkflowEditorComponent", () => { expect(getStroke(mockScanPredicate.operatorID)).toBe("red"); }); + + it("uses the Validation passed in instead of recomputing it", () => { + // Let the validation chain settle from the operator-add so the spy + // below is created after those calls and starts with a clean slate. + workflowActionService.addOperator(mockScanPredicate, mockPoint); + fixture.detectChanges(); + + const validateSpy = vi.spyOn(validationWorkflowService, "validateOperator"); + + // Call the helper directly with a Validation argument, mirroring what + // the validation-stream subscriber does at runtime + // (handleOperatorValidation passes value.validation through). + (component as any).applyOperatorBorder(mockScanPredicate.operatorID, { isValid: true }); + + expect(validateSpy).not.toHaveBeenCalled(); + }); + + it("honors the passed-in Validation result (paints red when it is invalid)", () => { + // Proves the passed-in value actually drives the border, not just that + // the recompute is skipped: an invalid result must paint red. + vi.spyOn(workflowStatusService, "getCurrentStatus").mockReturnValue({}); + workflowActionService.addOperator(mockScanPredicate, mockPoint); + fixture.detectChanges(); + + (component as any).applyOperatorBorder(mockScanPredicate.operatorID, { isValid: false, messages: {} }); + + expect(getStroke(mockScanPredicate.operatorID)).toBe("red"); + }); }); }); }); diff --git a/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts b/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts index 5411ea995a..1a9eb89e05 100644 --- a/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts +++ b/frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts @@ -25,7 +25,7 @@ import { DragDropService } from "../../service/drag-drop/drag-drop.service"; import { DynamicSchemaService } from "../../service/dynamic-schema/dynamic-schema.service"; import { ExecuteWorkflowService } from "../../service/execute-workflow/execute-workflow.service"; import { fromJointPaperEvent, JointUIService, linkPathStrokeColor } from "../../service/joint-ui/joint-ui.service"; -import { ValidationWorkflowService } from "../../service/validation/validation-workflow.service"; +import { Validation, ValidationWorkflowService } from "../../service/validation/validation-workflow.service"; import { WorkflowActionService } from "../../service/workflow-graph/model/workflow-action.service"; import { WorkflowStatusService } from "../../service/workflow-status/workflow-status.service"; import { ExecutionState, OperatorState } from "../../types/execute-workflow.interface"; @@ -397,10 +397,14 @@ export class WorkflowEditorComponent implements OnInit, AfterViewInit, OnDestroy * Centralizing this here avoids the race where the validation pass * overwrites a state-derived stroke (or vice versa) for an operator that * is both invalid and has a cached execution status. + * + * Callers that already have a Validation result (the validation stream + * subscriber) may pass it in to avoid recomputing it; callers without one + * (the operator-add stream subscriber) let the helper fetch it lazily. */ - private applyOperatorBorder(operatorID: string): void { - const validation = this.validationWorkflowService.validateOperator(operatorID); - if (!validation.isValid) { + private applyOperatorBorder(operatorID: string, validation?: Validation): void { + const resolved = validation ?? this.validationWorkflowService.validateOperator(operatorID); + if (!resolved.isValid) { this.jointUIService.changeOperatorColor(this.paper, operatorID, false); return; } @@ -1025,7 +1029,7 @@ export class WorkflowEditorComponent implements OnInit, AfterViewInit, OnDestroy this.validationWorkflowService .getOperatorValidationStream() .pipe(untilDestroyed(this)) - .subscribe(value => this.applyOperatorBorder(value.operatorID)); + .subscribe(value => this.applyOperatorBorder(value.operatorID, value.validation)); } /**
