fix(frontend): preserve operator state border on workflow page return#5146
fix(frontend): preserve operator state border on workflow page return#5146PG1204 wants to merge 7 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5146 +/- ##
============================================
+ Coverage 49.16% 49.71% +0.55%
+ Complexity 2384 2347 -37
============================================
Files 1051 1049 -2
Lines 40350 40219 -131
Branches 4279 4278 -1
============================================
+ Hits 19837 19994 +157
+ Misses 19353 19069 -284
+ Partials 1160 1156 -4
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Codecov flagged 0% patch because my tests are in workflow-editor.component.spec.ts, which is excluded from the unit-test runner in angular.json. I'm planning to move them into a new sibling file workflow-editor-status-restoration.spec.ts so the default glob picks them up and was verified locally that this brings patch coverage close to 100%. Happy to take a different approach if the team prefers, otherwise can push the new planned spec.ts file. |
|
For UI/user facing changes, please include before and after screenshots in the PR description, under |
|
Also please add tests to cover your changes. |
|
@Yicong-Huang I have added a comment above regarding the test file & code coverage. |
just saw it, sorry. sg, you can place the tests else where for now, or simply add the tests in the same file and comment out the previous tests. |
No worries. Thanks for the suggestions. |
Quick clarification on Option 2, since workflow-editor.component.spec.ts is currently in angular.json's exclude list, commenting out the previous tests by itself wouldn't make the file run in CI (so Codecov would still report 0% on the new tests). Were you implicitly suggesting I also remove the exclusion as part of this, or would you prefer that change be left out of this PR? |
|
you can
Alternatively you can use another PR to enable this workflow-editor.component.spec.ts and uncomment and fix the previous test cases first. |
Should I wait for this PR to be reviewed and merged first before raising the follow-up PR to uncomment (and fix) the test cases in workflow-editor.component.spec.ts? |
/request-review @Yicong-Huang I shall raise the follow-up PR as soon as this is merged. Thanks. |
|
I added @Xiao-zhen-Liu to be a reviewer, assuming @Yicong-Huang may not be very available. My preference is that if the test cases are not related to this functionality of this PR, it should be fixed in a separate PR. |
|
as we are low on coverage, I would encourage to add test cases to related files when fit, even it is not fully related to the changed line. Otherwise we won't know if the change in this PR would break some related parts (because we don't have tests for them). After we have reached to a good ratio of coverage, then we can make it strict that only introduce or change tests related to the changed lines. |
There was a problem hiding this comment.
Thanks for tackling #3614 — the writeup of what was going wrong is excellent, and the actual fix (the two changes in workflow-editor.component.ts) is small, focused, and fixes a real problem. Two things worth addressing, both about how the border color is decided and how the new tests check it.
How the border color gets decided
- The underlying problem is that two separate parts of the code both set the operator's border color, and they were overwriting each other. This fix makes them agree in the normal case (a valid operator with a saved "completed" status), which is correct. But they are still two separate parts setting the same color, with the same look-up-and-repaint logic duplicated in both, and one tricky case remains: an operator that is both invalid AND has a saved "completed" status. Red (invalid) is supposed to win, but whether it actually does depends on which of the two code paths runs last — and nothing guarantees that order. It works today, but it's fragile. Having a single place decide the final color (check validity first, then saved status, then fall back to the default) would make this reliable and remove the duplicated logic that now lives in both places.
The new tests
- The new tests check "was a particular paint function called?" rather than "what color did the operator actually end up?". Because the same paint function is also called from the other new code path, one of the tests would pass even if the fix were removed, and another doesn't confirm the final color actually wins. Checking the operator's final border color directly would make the tests genuinely verify the behavior. See the inline notes on the two tests.
| // 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 |
There was a problem hiding this 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.
| } | ||
| const statistics = this.workflowStatusService.getCurrentStatus()[value.operatorID]; | ||
| if (statistics) { | ||
| this.jointUIService.changeOperatorState(this.paper, value.operatorID, statistics.operatorState); |
There was a problem hiding this 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.
| workflowStatusService = TestBed.inject(WorkflowStatusService); | ||
| }); | ||
|
|
||
| it("repaints execution-state stroke for valid operators with a cached status", () => { |
There was a problem hiding this 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.
| expect(changeOperatorColorSpy).toHaveBeenCalledWith(component.paper, mockScanPredicate.operatorID, true); | ||
| }); | ||
|
|
||
| it("paints the invalid stroke (red) for invalid operators regardless of cached status", () => { |
There was a problem hiding this 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.
Xiao-zhen-Liu
left a comment
Xiao-zhen-Liu
left a comment
There was a problem hiding this comment.
Requesting changes — I think two of the points raised are worth addressing before merge rather than leaving as follow-ups:
-
Decide the operator's border color in one place. Right now two separate parts of the code set it, with the same look-up-and-repaint logic duplicated in both. Consolidating it into a single decision (check validity first, then saved status, then the default) removes the duplication and also fixes the edge case where an operator that is both invalid and has a saved "completed" status can show the wrong color depending on which code path happens to run last.
-
Make the two new tests check the operator's final border color rather than just whether a paint function was called. As written, one of them would pass even if the fix were removed, so the regression tests don't yet guard against the regression.
Details are in the inline comments. The behavior fix itself is correct and close — these are about making it robust and properly tested.
5 participants
What changes were proposed in this PR?
Fixes a visual regression where an operator's border, state text, port row counts, and worker count reset to default after navigating away from a workflow page and returning, even though the execution state is still cached in WorkflowStatusService.
Root cause: WorkspaceComponent clears the workflow on destroy and calls reloadWorkflow() on re-init, recreating every operator from the workflow JSON with default JointJS attributes. The cached execution status was never reapplied, and the validation pass that runs on operator-add called changeOperatorColor(..., true) for valid operators, overwriting rect.body/stroke and forcing the border back to gray.
Fix (two changes, both in workflow-editor.component.ts):
Subscribe to getOperatorAddStream() inside handleOperatorStatisticsUpdate. When an operator is added (drag-drop, reloadWorkflow, undo/redo, collaborative add via Yjs - all routed through a single emission point), look up the cached OperatorStatistics. If present, call changeOperatorStatistics(...) to restore the state color, port labels, worker count, and state text. New operators with no cached entry early-return and get default coloring.
Make handleOperatorValidation status-aware. Invalid operators still get a red border (priority preserved). For valid operators, the handler now checks the cached status - if one exists, it repaints via changeOperatorState(...) instead of overwriting with default gray. Valid operators with no cached status continue to get the default gray border.
Before fix:
473837274-d703b42e-8504-4735-bc9a-bcac9602c201.mov
After fix:
Screen.Recording.2026-05-22.at.4.18.14.PM.mov
Any related issues, documentation, discussions?
Fixes #3614.
How was this PR tested?
Unit tests: Three tests added to workflow-editor.component.spec.ts under a new describe("operator border restoration after navigation") block:
Valid operator + cached Completed -> changeOperatorState(..., Completed) is called.
Valid operator + empty cache -> default changeOperatorColor(..., true) is called (existing behavior preserved).
Invalid operator + cached Completed -> changeOperatorColor(..., false) is called, red wins (existing behavior preserved).
All three pass under Vitest (ng test).
Manual UI testing: Reproduced the issue's recording locally:
Open a workflow (e.g., CSV File Scan → Radar Chart) and run it; both operators turn green with port row counts.
Navigate to a different page, then back.
Before fix: operators reset to default gray borders, row counts blank. After fix: green borders, row counts, worker counts, and "Completed" state label all persist.
Edge cases manually verified: fresh workflow (default coloring), new operator dragged in after a completed run (default for new, cached for existing), re-running (resetStatus repaints Uninitialized, live updates flow normally), invalid operator with cached Completed (red border, validation priority).
Note: workflow-editor.component.spec.ts was previously excluded from the default jsdom test target in angular.json. This PR removes that exclusion (so the new tests run in CI and contribute to Codecov coverage) and comments out six pre-existing mouse-event tests that fail under jsdom (they continue to pass in the ng run gui:test-browser target, marked with TODO(#3614) in place). A follow-up PR will revive those commented-out tests.
Was this PR authored or co-authored using generative AI tooling?
Co-authored-by: Claude Code (Anthropic Claude Opus 4.7)