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-6927-5021bc6f1a96d00cb26aa9c3a82c86fee67da0a9 in repository https://gitbox.apache.org/repos/asf/texera.git
commit 7daf8d78415d5f40fc017678738ce57b137065c5 Author: Prateek Ganigi <[email protected]> AuthorDate: Wed Aug 12 18:51:52 2026 +0000 refactor(frontend): make redundant operator border repaints a no-op (#6927) ### What changes were proposed in this PR? When an operator is added, its border was painted by two paths - the operator-add restore and the validation pass, producing the same color. Harmless, but a redundant repaint. This PR adds a guarded border setter (`paintOperatorBorder`) in `JointUIService` that writes `rect.body/stroke` only when the color actually changes. Both `changeOperatorColor` and `changeOperatorState` route their border write through it, so a repaint with the color the border already has becomes a no-op, effectively "painted once" - including on the navigation-return (reload) path. **Deviation from the approach suggested on the issue:** the issue suggested dropping the `applyOperatorBorder` call from the operator-add handler and letting the validation pass set the border. I kept that call and used the guard instead, because `changeOperatorStatistics` already paints the border via `changeOperatorState` *without* checking validity. Dropping `applyOperatorBorder` would make an invalid operator with a cached "completed" status rely on the validation pass firing afterward to correct green→red, reintroducing the order-dependent border fragility that #5146 removed (and it would break in the edge case where `setDynamicSchema` skips its emit because the schema is unchanged). The guard reaches the same no-redundant-repaint goal while keeping the border validity-correct regardless of event timing. ### Any related issues, documentation, discussions? Part of #5726 ### How was this PR tested? Unit tests: - `JointUIService`: the guarded setter skips the write when the border is already the requested color, and writes when it differs. - `WorkflowEditorComponent`: added a navigation-return test for a cached **Running** operator (orange), alongside the existing completed (green), default (gray), invalid (red), and invalid-over-cached-priority cases. - Full frontend suite: 3739 passing. Manual (navigated away from a running workflow and back), border restored correctly for: - Completed operators → green - Running operators → orange - Invalid operators → red - Valid, not run → default gray ### Was this PR authored or co-authored using generative AI tooling? This PR was co-authored using Claude Code (Anthropic Claude Opus 4.7) in compliance with ASF. --- .../workflow-editor.component.spec.ts | 19 ++++++++++++++++--- .../service/joint-ui/joint-ui.service.spec.ts | 15 +++++++++++++++ .../workspace/service/joint-ui/joint-ui.service.ts | 21 +++++++++++++++++---- 3 files changed, 48 insertions(+), 7 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 d2ecfda618..2c8807a9ec 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 @@ -887,15 +887,16 @@ describe("WorkflowEditorComponent", () => { */ describe("operator border restoration after navigation", () => { let workflowStatusService: WorkflowStatusService; - const cachedCompleted = { + const cachedStatus = (operatorState: OperatorState) => ({ [mockScanPredicate.operatorID]: { - operatorState: OperatorState.Completed, + operatorState, aggregatedInputRowCount: 0, inputPortMetrics: {}, aggregatedOutputRowCount: 0, outputPortMetrics: {}, }, - }; + }); + const cachedCompleted = cachedStatus(OperatorState.Completed); const getStroke = (operatorID: string): string => component.paper.getModelById(operatorID).attr("rect.body/stroke") as string; @@ -913,6 +914,18 @@ describe("WorkflowEditorComponent", () => { expect(getStroke(mockScanPredicate.operatorID)).toBe("green"); }); + it("paints the execution-state stroke (orange) for a valid operator with a cached Running status", () => { + // Navigation-return with a mid-run operator: the border must be restored + // to the running color, not the default (see #3614). + vi.spyOn(workflowStatusService, "getCurrentStatus").mockReturnValue(cachedStatus(OperatorState.Running)); + vi.spyOn(validationWorkflowService, "validateOperator").mockReturnValue({ isValid: true }); + + workflowActionService.addOperator(mockScanPredicate, mockPoint); + fixture.detectChanges(); + + expect(getStroke(mockScanPredicate.operatorID)).toBe("orange"); + }); + it("falls back to the default valid stroke (#CFCFCF) when no cached status exists", () => { vi.spyOn(workflowStatusService, "getCurrentStatus").mockReturnValue({}); vi.spyOn(validationWorkflowService, "validateOperator").mockReturnValue({ isValid: true }); diff --git a/frontend/src/app/workspace/service/joint-ui/joint-ui.service.spec.ts b/frontend/src/app/workspace/service/joint-ui/joint-ui.service.spec.ts index d34a9eb2b3..c383e0963d 100644 --- a/frontend/src/app/workspace/service/joint-ui/joint-ui.service.spec.ts +++ b/frontend/src/app/workspace/service/joint-ui/joint-ui.service.spec.ts @@ -361,6 +361,21 @@ describe("JointUIService", () => { service.changeOperatorColor(paper, "op-1", false); expect(attrSpy).toHaveBeenCalledWith("rect.body/stroke", "red"); }); + it("skips the write when the border is already the requested color", () => { + const { paper, attrSpy } = makePaperWithModel(); + // model reports it is already neutral; the guarded setter must not rewrite it + attrSpy.mockImplementation((selector: string) => (selector === "rect.body/stroke" ? "#CFCFCF" : undefined)); + const service = new JointUIService(emptyMetadataStub as never); + service.changeOperatorColor(paper, "op-1", true); + expect(attrSpy).not.toHaveBeenCalledWith("rect.body/stroke", "#CFCFCF"); + }); + it("writes the border when the current color differs", () => { + const { paper, attrSpy } = makePaperWithModel(); + attrSpy.mockImplementation((selector: string) => (selector === "rect.body/stroke" ? "red" : undefined)); + const service = new JointUIService(emptyMetadataStub as never); + service.changeOperatorColor(paper, "op-1", true); + expect(attrSpy).toHaveBeenCalledWith("rect.body/stroke", "#CFCFCF"); + }); }); describe("changeOperatorState", () => { diff --git a/frontend/src/app/workspace/service/joint-ui/joint-ui.service.ts b/frontend/src/app/workspace/service/joint-ui/joint-ui.service.ts index d069a270eb..bb6fbff0f0 100644 --- a/frontend/src/app/workspace/service/joint-ui/joint-ui.service.ts +++ b/frontend/src/app/workspace/service/joint-ui/joint-ui.service.ts @@ -491,11 +491,24 @@ export class JointUIService { * @param isOperatorValid */ public changeOperatorColor(jointPaper: joint.dia.Paper, operatorID: string, isOperatorValid: boolean): void { - if (isOperatorValid) { - jointPaper.getModelById(operatorID).attr("rect.body/stroke", "#CFCFCF"); - } else { - jointPaper.getModelById(operatorID).attr("rect.body/stroke", "red"); + this.paintOperatorBorder(jointPaper, operatorID, isOperatorValid ? "#CFCFCF" : "red"); + } + + /** + * Sets the operator's border stroke, returning early when it is already that + * color. A same-value attr() write would not re-render (Backbone's Model.set + * no-ops via _.isEqual), but attr() still deep-clones and deep-compares the + * whole attrs tree before reaching that check. On operator add the validation + * pass and the operator-add restore both request a border color for the same + * operator, so returning early here skips that clone/compare on the second + * call. + */ + private paintOperatorBorder(jointPaper: joint.dia.Paper, operatorID: string, color: string): void { + const model = jointPaper.getModelById(operatorID); + if (model.attr("rect.body/stroke") === color) { + return; } + model.attr("rect.body/stroke", color); } public changeOperatorDisableStatus(jointPaper: joint.dia.Paper, operator: OperatorPredicate): void {
