refactor(frontend): avoid redundant validateOperator call in applyOperatorBorder - #5702
Merged
Xiao-zhen-Liu merged 13 commits intoJun 25, 2026
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.54% 54.40% -0.15%
+ Complexity 3164 2857 -307
============================================
Files 1128 1108 -20
Lines 45753 42773 -2980
Branches 5008 4605 -403
============================================
- Hits 24958 23269 -1689
+ Misses 19362 18149 -1213
+ Partials 1433 1355 -78
*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:
|
Contributor
Author
|
/request-review @Xiao-zhen-Liu Follow-up for the split-out applyOperatorBorder change: #5626. Includes the test for the "validation passed in" path. |
6 tasks
Contributor
|
@Xiao-zhen-Liu do you want to check on this one? |
6 tasks
Contributor
Author
Hi team, any update on this PR? |
Contributor
|
@Xiao-zhen-Liu Please check this PR. |
Contributor
Automated Reviewer SuggestionsBased on the
|
Xiao-zhen-Liu
approved these changes
Jun 22, 2026
Xiao-zhen-Liu
left a comment
Xiao-zhen-Liu
left a comment
Contributor
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).
PG1204
added a commit
to PG1204/texera
that referenced
this pull request
Jul 14, 2026
…torBorder (apache#6075) ### What changes were proposed in this PR? Follow-up to apache#5702. `WorkflowEditorComponent.applyOperatorBorder` decides an operator's border color and has two callers: - `handleOperatorValidation`: the validation-stream subscriber, which already had the `Validation` and passed it in. - The operator-add stream subscriber: which passed no `Validation` and relied on an optional-parameter fallback that recomputed it inside the helper. This PR unifies the two paths so both callers obtain the `Validation` themselves and pass it in: - The operator-add subscriber now computes it via `validationWorkflowService.validateOperator(...)` and passes it, mirroring the validation-stream caller. - `applyOperatorBorder`'s `validation` parameter becomes **required**, and the `?? validateOperator(...)` fallback is removed. The color decision no longer silently depends on a recompute hidden inside the helper. This is a non-functional cleanup and no behavioral change. In particular, the operator-add subscriber still paints the border immediately (and restores cached run statistics), preserving the navigation-reset behavior from apache#3614. Note on scope: the issue's summary mentions "painted exactly once." Truly collapsing to a single paint conflicts with the apache#3614 requirement that borders render immediately on reload without waiting for validation events, and would need fragile "just-added" state tracking in the validation handler. This PR implements the safe, behavior-preserving unification (consistent validation acquisition, fallback removed) rather than suppressing either paint. ### Any related issues, documentation, discussions? Part of apache#5726. Related: apache#5702 (added the optional parameter; this issue arose from its review), apache#5146 (introduced `applyOperatorBorder` and the apache#3614 navigation fix). ### How was this PR tested? Updated `workflow-editor.component.spec.ts`: - Renamed the "uses the Validation passed in instead of recomputing it" test to "relies solely on the passed-in Validation (never recomputes inside the helper)": the old name implied a recompute path that no longer exists. - Added "supplies a computed Validation from the operator-add path", asserting the operator-add subscriber calls `applyOperatorBorder` with a `Validation` object. - Existing border-outcome tests (green / gray / red / invalid-over-cached priority) remain and continue to pass. Verified locally: - `tsc --noEmit`: clean - `prettier --check` / `eslint`: clean - `ng test` (jsdom): 30/30 pass in the editor spec - `ng run gui:test-browser`: 13/13 pass ### Was this PR authored or co-authored using generative AI tooling? Co-authored with Claude Opus 4.8 in compliance with ASF
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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)