Skip to content

refactor(frontend): make redundant operator border repaints a no-op#6927

Open
PG1204 wants to merge 4 commits into
apache:mainfrom
PG1204:refactor/dedup-operator-add-border-paint
Open

refactor(frontend): make redundant operator border repaints a no-op#6927
PG1204 wants to merge 4 commits into
apache:mainfrom
PG1204:refactor/dedup-operator-add-border-paint

Conversation

@PG1204

@PG1204 PG1204 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

What changes were proposed in this PR?

When an operator is added, its border was painted by two paths - the operator-add restore and the validation pass, producing the same color. Harmless, but a redundant repaint.

This PR adds a guarded border setter (paintOperatorBorder) in JointUIService that writes rect.body/stroke only when the color actually changes. Both changeOperatorColor and changeOperatorState route their border write through it, so a repaint with the color the border already has becomes a no-op, effectively "painted once" - including on the navigation-return (reload) path.

Deviation from the approach suggested on the issue: the issue suggested dropping the applyOperatorBorder call from the operator-add handler and letting the validation pass set the border. I kept that call and used the guard instead, because changeOperatorStatistics already paints the border via changeOperatorState without checking validity. Dropping applyOperatorBorder would make an invalid operator with a cached "completed" status rely on the validation pass firing afterward to correct green→red, reintroducing the order-dependent border fragility that #5146 removed (and it would break in the edge case where setDynamicSchema skips its emit because the schema is unchanged). The guard reaches the same no-redundant-repaint goal while keeping the border validity-correct regardless of event timing.

Any related issues, documentation, discussions?

Closes #5726

How was this PR tested?

Unit tests:

  • JointUIService: the guarded setter skips the write when the border is already the requested color, and writes when it differs.
  • WorkflowEditorComponent: added a navigation-return test for a cached Running operator (orange), alongside the existing completed (green), default (gray), invalid (red), and invalid-over-cached-priority cases.
  • Full frontend suite: 3739 passing.

Manual (navigated away from a running workflow and back), border restored correctly for:

  • Completed operators → green
  • Running operators → orange
  • Invalid operators → red
  • Valid, not run → default gray

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

This PR was co-authored using Claude Code (Anthropic Claude Opus 4.7) in compliance with ASF.

@github-actions github-actions Bot added refactor Refactor the code frontend Changes related to the frontend GUI labels Jul 27, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Automated Reviewer Suggestions

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

  • Contributors with relevant context: @mengw15, @Ma77Ball, @Yicong-Huang
    You can notify them by mentioning @mengw15, @Ma77Ball, @Yicong-Huang in a comment.

@PG1204

PG1204 commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

/request-review @Xiao-zhen-Liu

@github-actions
github-actions Bot requested a review from Xiao-zhen-Liu July 27, 2026 05:43
@codecov-commenter

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 78.95%. Comparing base (49f9e2c) to head (5cb206a).

Additional details and impacted files
@@            Coverage Diff            @@
##               main    #6927   +/-   ##
=========================================
  Coverage     78.95%   78.95%           
  Complexity     3795     3795           
=========================================
  Files          1161     1161           
  Lines         46084    46087    +3     
  Branches       5110     5111    +1     
=========================================
+ Hits          36384    36387    +3     
  Misses         8077     8077           
  Partials       1623     1623           
Flag Coverage Δ *Carryforward flag
access-control-service 70.00% <ø> (ø) Carriedforward from 49f9e2c
agent-service 76.76% <ø> (ø) Carriedforward from 49f9e2c
amber 72.20% <ø> (ø) Carriedforward from 49f9e2c
computing-unit-managing-service 20.49% <ø> (ø) Carriedforward from 49f9e2c
config-service 66.66% <ø> (ø) Carriedforward from 49f9e2c
file-service 67.21% <ø> (ø) Carriedforward from 49f9e2c
frontend 82.91% <100.00%> (+<0.01%) ⬆️
notebook-migration-service 78.94% <ø> (ø) Carriedforward from 49f9e2c
pyamber 95.38% <ø> (ø) Carriedforward from 49f9e2c
workflow-compiling-service 55.14% <ø> (ø) Carriedforward from 49f9e2c

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

frontend Changes related to the frontend GUI refactor Refactor the code

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unify operator border-coloring so a newly-added operator is validated and painted once

2 participants