refactor(frontend): avoid redundant validateOperator call in applyOperatorBorder#5702
refactor(frontend): avoid redundant validateOperator call in applyOperatorBorder#5702PG1204 wants to merge 10 commits into
Conversation
github-actions
Bot
added
refactor
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #5702 +/- ##
============================================
- Coverage 54.37% 53.74% -0.63%
+ Complexity 2859 2722 -137
============================================
Files 1107 1103 -4
Lines 42767 42627 -140
Branches 4599 4591 -8
============================================
- Hits 23254 22910 -344
- Misses 18158 18383 +225
+ Partials 1355 1334 -21
*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. |
|
@Xiao-zhen-Liu do you want to check on this one? |
Hi team, any update on this PR? |
|
@Xiao-zhen-Liu Please check this PR. |
Automated Reviewer SuggestionsBased on the
|
Xiao-zhen-Liu
left a comment
Xiao-zhen-Liu
left a comment
There was a problem hiding this comment.
LGTM — clean split-out of the change from #5626, and good call keeping the lazy fallback (the operator-add path has no Validation in hand, so it can't be removed).
| workflowActionService.addOperator(mockScanPredicate, mockPoint); | ||
| fixture.detectChanges(); | ||
|
|
||
| const validateSpy = vi.spyOn(validationWorkflowService, "validateOperator").mockClear(); |
There was a problem hiding this comment.
Minor: the spy is created after the addOperator calls above, so there's nothing to clear yet — .mockClear() is redundant here.
| // 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 }); |
There was a problem hiding this comment.
Optional: this proves the recompute is skipped, but not that the passed-in value is used. Passing { isValid: false } and asserting the operator goes red would cover that too.
5 participants
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)