refactor(frontend): avoid redundant validateOperator call in applyOperatorBorder#5702
refactor(frontend): avoid redundant validateOperator call in applyOperatorBorder#5702PG1204 wants to merge 2 commits into
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5702 +/- ##
============================================
- Coverage 52.95% 52.71% -0.25%
+ Complexity 2627 2501 -126
============================================
Files 1090 1090
Lines 42210 42053 -157
Branches 4534 4520 -14
============================================
- Hits 22353 22167 -186
- Misses 18546 18569 +23
- Partials 1311 1317 +6
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
/request-review @Xiao-zhen-Liu Follow-up for the split-out applyOperatorBorder change: #5626. Includes the test for the "validation passed in" path. |
| const resolved = validation ?? this.validationWorkflowService.validateOperator(operatorID); | ||
| if (!resolved.isValid) { | ||
| this.jointUIService.changeOperatorColor(this.paper, operatorID, false); | ||
| return; | ||
| } |
There was a problem hiding this comment.
if all validations are done already, we could even remove this if fallback?
There was a problem hiding this comment.
Good question, but I think keeping the fallback would be good to have. It's there for the operator-add case, which fires before validation has been computed (separate event streams), so there's genuinely no validation result to pass in at that point. That path was added for the navigation-reset fix (#3614) and needs to color the border right away. The optimization just removes the duplicate computation on the validation-handler path, where we already have the result. Happy to revisit if you're seeing a case where validation always runs first.
There was a problem hiding this comment.
ok. can we combine the logic of operator-add case? I think logically it should also have a validation result right away. maybe a follow up?
There was a problem hiding this comment.
i agree with this, this would be a good follow up. The two paths currently run off separate event streams (operator-add restore vs. the validation stream), so unifying them so a new operator is validated/painted exactly once is worth doing, should be done carefully though, especially to preserve the #3614 navigation-reset behavior. I can file a follow-up issue to track it so we can keep this PR scoped.
Thanks!
What changes were proposed in this PR?
WorkflowEditorComponent.applyOperatorBorder(operatorID)` always recomputes validation as its first step:
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:
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.tsinside the existingoperator border restoration after navigationdescribe block:it("uses the Validation passed in instead of recomputing it", ...). The test clears thevalidateOperatorspy after the operator-add validation chain settles, then callsapplyOperatorBorderdirectly with aValidationargument and assertsvalidateOperatoris not called.Verified locally:
tsc --noEmit: cleaneslint: cleanng test(jsdom): 26/26 pass in the editor spec (was 25, new test adds one)ng run gui:test-browser: 13/13 passWas this PR authored or co-authored using generative AI tooling?
This PR was co-authored using Claude Code (Anthropic Claude Opus 4.7)