Xiao-zhen-Liu commented on code in PR #5146:
URL: https://github.com/apache/texera/pull/5146#discussion_r3320350416
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts:
##########
@@ -358,6 +358,28 @@ export class WorkflowEditorComponent implements OnInit,
AfterViewInit, OnDestroy
});
}
});
+
+ // When operators are (re)added to the graph — e.g. after navigating back
to
+ // the workflow page, where WorkspaceComponent calls reloadWorkflow and
+ // operators are recreated from the workflow JSON — restore their visual
+ // state from the cached status so completed runs don't appear to reset.
+ this.workflowActionService
Review Comment:
This new block looks up the saved status and repaints the operator's border
color. The validation code further down (the other change in this PR) now does
the same look-up-and-repaint. So the same logic lives in two places, which is
easy to break later: someone updates one copy and forgets the other.
Stepping back: the original bug is that two separate parts of the code both
set the border color and end up overwriting each other. This fix makes them
agree in the normal case, but they're still two separate things setting the
same color. It would be cleaner to have one function decide the final border
color (taking into account both whether the operator is valid and what its run
status is) and call that from a single place.
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts:
##########
@@ -949,15 +971,31 @@ export class WorkflowEditorComponent implements OnInit,
AfterViewInit, OnDestroy
}
/**
- * if the operator is valid , the border of the box will be default
+ * Applies the validation result to the operator's border.
+ * - Invalid operators are drawn with a red border (validation takes
priority).
+ * - Valid operators with a cached execution status keep their
execution-state
+ * border so e.g. a Completed (green) run isn't repainted gray when the
user
+ * navigates back to the workflow and reloadWorkflow re-adds the operators,
+ * which triggers a validation pass that would otherwise overwrite the
+ * execution-state stroke set by handleOperatorStatisticsUpdate.
+ * - Valid operators with no cached status get the default valid border.
*/
private handleOperatorValidation(): void {
this.validationWorkflowService
.getOperatorValidationStream()
.pipe(untilDestroyed(this))
- .subscribe(value =>
- this.jointUIService.changeOperatorColor(this.paper, value.operatorID,
value.validation.isValid)
- );
+ .subscribe(value => {
+ if (!value.validation.isValid) {
+ this.jointUIService.changeOperatorColor(this.paper,
value.operatorID, false);
+ return;
+ }
+ const statistics =
this.workflowStatusService.getCurrentStatus()[value.operatorID];
+ if (statistics) {
+ this.jointUIService.changeOperatorState(this.paper,
value.operatorID, statistics.operatorState);
Review Comment:
Edge case worth thinking about: an operator that is both invalid AND has a
saved "completed" status. Red (invalid) is supposed to win over green here. But
whether it actually does depends on which of the two color-setting code paths
happens to run last, and nothing in the code guarantees that order: they are
triggered through two different chains of events. It happens to work today, but
if that order ever changed, an invalid operator could wrongly show up green
instead of red.
Having one place decide the final color (validation first, then saved
status, then default) would remove this dependence on timing.
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts:
##########
@@ -962,5 +975,75 @@ describe("WorkflowEditorComponent", () => {
fixture.detectChanges();
expect(redoSpy).toHaveBeenCalledTimes(4);
});
+
+ /**
+ * Regression coverage for the bug where the operator border resets to the
+ * default (gray) when the user navigates away from and back to a workflow
+ * that has already finished executing. The visual state is driven by two
+ * separate streams that both touch rect.body/stroke: the execution status
+ * stream (sets state-derived color) and the validation stream (sets red on
+ * invalid, gray on valid). When operators are re-added by reloadWorkflow,
+ * the validation pass fires after the status repaint and used to overwrite
+ * it. handleOperatorValidation now consults the cached status before
+ * deciding which color to apply.
+ */
+ describe("operator border restoration after navigation", () => {
+ let workflowStatusService: WorkflowStatusService;
+ const cachedCompleted = {
+ [mockScanPredicate.operatorID]: {
+ operatorState: OperatorState.Completed,
+ aggregatedInputRowCount: 0,
+ inputPortMetrics: {},
+ aggregatedOutputRowCount: 0,
+ outputPortMetrics: {},
+ },
+ };
+
+ beforeEach(() => {
+ workflowStatusService = TestBed.inject(WorkflowStatusService);
+ });
+
+ it("repaints execution-state stroke for valid operators with a cached
status", () => {
Review Comment:
This test is meant to verify the validation fix, but it only checks "was the
green-painting function called?". That same function also gets called by the
other new block (the operator-add code above), so this test would still pass
even if the validation fix were removed entirely. In other words, it does not
actually test the change it is paired with.
Checking the operator's actual final border color would test the real
behavior.
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts:
##########
@@ -962,5 +975,75 @@ describe("WorkflowEditorComponent", () => {
fixture.detectChanges();
expect(redoSpy).toHaveBeenCalledTimes(4);
});
+
+ /**
+ * Regression coverage for the bug where the operator border resets to the
+ * default (gray) when the user navigates away from and back to a workflow
+ * that has already finished executing. The visual state is driven by two
+ * separate streams that both touch rect.body/stroke: the execution status
+ * stream (sets state-derived color) and the validation stream (sets red on
+ * invalid, gray on valid). When operators are re-added by reloadWorkflow,
+ * the validation pass fires after the status repaint and used to overwrite
+ * it. handleOperatorValidation now consults the cached status before
+ * deciding which color to apply.
+ */
+ describe("operator border restoration after navigation", () => {
+ let workflowStatusService: WorkflowStatusService;
+ const cachedCompleted = {
+ [mockScanPredicate.operatorID]: {
+ operatorState: OperatorState.Completed,
+ aggregatedInputRowCount: 0,
+ inputPortMetrics: {},
+ aggregatedOutputRowCount: 0,
+ outputPortMetrics: {},
+ },
+ };
+
+ beforeEach(() => {
+ workflowStatusService = TestBed.inject(WorkflowStatusService);
+ });
+
+ it("repaints execution-state stroke for valid operators with a cached
status", () => {
+ vi.spyOn(workflowStatusService,
"getCurrentStatus").mockReturnValue(cachedCompleted);
+ vi.spyOn(validationWorkflowService,
"validateOperator").mockReturnValue({ isValid: true });
+ const changeOperatorStateSpy = vi.spyOn(jointUIService,
"changeOperatorState");
+
+ workflowActionService.addOperator(mockScanPredicate, mockPoint);
+ fixture.detectChanges();
+
+ expect(changeOperatorStateSpy).toHaveBeenCalledWith(
+ component.paper,
+ mockScanPredicate.operatorID,
+ OperatorState.Completed
+ );
+ });
+
+ it("falls back to the default valid stroke when no cached status
exists", () => {
+ vi.spyOn(workflowStatusService,
"getCurrentStatus").mockReturnValue({});
+ vi.spyOn(validationWorkflowService,
"validateOperator").mockReturnValue({ isValid: true });
+ const changeOperatorColorSpy = vi.spyOn(jointUIService,
"changeOperatorColor");
+
+ workflowActionService.addOperator(mockScanPredicate, mockPoint);
+ fixture.detectChanges();
+
+ expect(changeOperatorColorSpy).toHaveBeenCalledWith(component.paper,
mockScanPredicate.operatorID, true);
+ });
+
+ it("paints the invalid stroke (red) for invalid operators regardless of
cached status", () => {
Review Comment:
This checks "was the red-painting function called?", but calling it is not
enough on its own: red also has to be the color that actually ends up on the
operator (see my note on the validation handler about the ordering). The test
does not confirm the operator ends up red, so it would not catch the timing
problem described there.
Checking the final border color would make this test reliable.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
