Skip to content

refactor(frontend): avoid redundant validateOperator call in applyOperatorBorder#5702

Queued
PG1204 wants to merge 13 commits into
apache:mainfrom
PG1204:refactor/apply-operator-border-reuse-validation
Queued

refactor(frontend): avoid redundant validateOperator call in applyOperatorBorder#5702
PG1204 wants to merge 13 commits into
apache:mainfrom
PG1204:refactor/apply-operator-border-reuse-validation

Conversation

@PG1204

@PG1204 PG1204 commented Jun 14, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

WorkflowEditorComponent.applyOperatorBorder(operatorID)` always recomputes validation as its first step:

const validation = this.validationWorkflowService.validateOperator(operatorID);

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:

  • Adds an optional validation?: Validation parameter to applyOperatorBorder. The helper uses it when provided, and falls back to validateOperator(operatorID) otherwise.
  • Updates handleOperatorValidation to pass value.validation from the stream into the helper.
  • Leaves the operator-add stream subscriber unchanged and hence it doesn't have a Validation in hand at that point, so it correctly falls through to the lazy-fetch path.
  • Functionally identical (the stream emits the same Validation that would have been recomputed), purely avoids the duplicate call.

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.ts inside the existing operator border restoration after navigation describe block: it("uses the Validation passed in instead of recomputing it", ...). The test clears the validateOperator spy after the operator-add validation chain settles, then calls applyOperatorBorder directly with a Validation argument and asserts validateOperator is not called.

Verified locally:

  • tsc --noEmit: clean
  • eslint: clean
  • ng test (jsdom): 26/26 pass in the editor spec (was 25, new test adds one)
  • ng run gui:test-browser: 13/13 pass

Was this PR authored or co-authored using generative AI tooling?

This PR was co-authored using Claude Code (Anthropic Claude Opus 4.7)

@github-actions github-actions Bot added refactor Refactor the code frontend Changes related to the frontend GUI labels Jun 14, 2026
@codecov-commenter

codecov-commenter commented Jun 14, 2026 •
edited
Loading

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 54.40%. Comparing base (d4eb9f7) to head (3f9ffb4).

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
Flag Coverage Δ *Carryforward flag
access-control-service 69.69% <ø> (-0.97%) ⬇️ Carriedforward from 6433e71
agent-service 34.36% <ø> (ø) Carriedforward from 6433e71
amber 56.17% <ø> (-0.69%) ⬇️ Carriedforward from 6433e71
computing-unit-managing-service 0.28% <ø> (-0.97%) ⬇️ Carriedforward from 6433e71
config-service 54.83% <ø> (+2.41%) ⬆️ Carriedforward from 6433e71
file-service 59.31% <ø> (+1.45%) ⬆️ Carriedforward from 6433e71
frontend 48.39% <100.00%> (+0.47%) ⬆️
notebook-migration-service ?
pyamber 90.20% <ø> (-0.99%) ⬇️ Carriedforward from 6433e71
python 90.76% <ø> (ø) Carriedforward from 6433e71
workflow-compiling-service 57.57% <ø> (+7.57%) ⬆️ Carriedforward from 6433e71

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@PG1204

PG1204 commented Jun 14, 2026 •
edited
Loading

Copy link
Copy Markdown
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.

@github-actions github-actions Bot requested a review from Xiao-zhen-Liu June 14, 2026 00:17
@PG1204 PG1204 mentioned this pull request Jun 15, 2026
Open
6 tasks
@Yicong-Huang

Yicong-Huang commented Jun 15, 2026

Copy link
Copy Markdown
Contributor

@Xiao-zhen-Liu do you want to check on this one?

@PG1204 PG1204 mentioned this pull request Jun 17, 2026
Open
6 tasks
@PG1204

PG1204 commented Jun 21, 2026

Copy link
Copy Markdown
Contributor Author

@Xiao-zhen-Liu do you want to check on this one?

Hi team, any update on this PR?

@chenlica

chenlica commented Jun 21, 2026

Copy link
Copy Markdown
Contributor

@Xiao-zhen-Liu Please check this PR.

@github-actions

github-actions Bot commented Jun 22, 2026

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

Based on the git blame history of the changed files, we recommend the following reviewers:

  • No candidates found from git blame history.

Xiao-zhen-Liu
Xiao-zhen-Liu approved these changes Jun 22, 2026
View reviewed changes

@Xiao-zhen-Liu Xiao-zhen-Liu left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Comment thread frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts Outdated
Comment thread frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts
@Xiao-zhen-Liu Xiao-zhen-Liu added this pull request to the merge queue Jun 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Yicong-Huang Yicong-Huang Yicong-Huang left review comments
@Xiao-zhen-Liu Xiao-zhen-Liu Xiao-zhen-Liu approved these changes

Assignees

@PG1204 PG1204

Labels

frontend Changes related to the frontend GUI refactor Refactor the code

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Reuse validation result from the validation stream in applyOperatorBorder

5 participants

@PG1204 @codecov-commenter @Yicong-Huang @chenlica @Xiao-zhen-Liu