fix(frontend): restore Regions and Status canvas toggles#5160
Conversation
|
This fix doesn't seem to be correct. A significant amount of region-related code was removed in #4495, so I'd expect more changes to be needed here. Please manually test the feature and verify that it works as expected before requesting another review. It's quite inefficient for reviewers to be responsible for testing the feature and then reporting issues back to the author. Otherwise, the review process can easily turn into several rounds of back-and-forth before the fix is actually validated. |
Both toggles broke after apache#4495 changed visibility from JointJS attributes to CSS classes. - Regions: drop the dead body/visibility loop; toggle the hide-region class only. - Status: re-add the hide-operator-status SCSS rule (retargeted to the renamed .texera-operator-state element), add the default class so Status starts hidden, and fix the stale selector in applyOperatorStatusPosition.
bobbai00
force-pushed
the
fix/5120-region-canvas-toggle
branch
from
4f82cd1 to
466fb7f
Compare
bobbai00
changed the title
My bad, I should make the PR as drafts when they are not ready for review. Now it is ready for a review. Please check |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #5160 +/- ##
============================================
+ Coverage 49.16% 49.40% +0.23%
+ Complexity 2385 2381 -4
============================================
Files 1051 1051
Lines 40350 40396 +46
Branches 4279 4292 +13
============================================
+ Hits 19838 19957 +119
+ Misses 19353 19260 -93
- Partials 1159 1179 +20
*This pull request uses carry forward flags. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
aglinxinyuan
left a comment
aglinxinyuan
left a comment
There was a problem hiding this comment.
There was a reason we didn't use the implementation used in this PR, even though it was cleaner. It will cause the issue described in issue #4027.
Tested locally, and the mini map still shows the region when it's disabled:
Restore the per-element body/visibility flip in toggleRegion that the previous cleanup removed; the mini-map shares the model but has no hide-region class, so it relies on this attribute (regression of apache#4027). Both the CSS class (main canvas, apache#5120) and the attribute (mini-map, apache#4027) are needed, now documented inline. Add unit tests for toggleRegion, toggleStatus, the status-label repositioning, and the default hide-region / hide-operator-status canvas classes.
Switch regions back to the pure body/visibility approach from apache#4112. apache#4495 had regions hidden via a hide-region CSS class on the main paper only, with the element defaulting to visible — so the mini-map (which shares the model but not the class) showed region hulls regardless of the toggle, and freshly-created regions reappeared on every execution update. Now the region element defaults to visibility: hidden and toggleRegion flips body/visibility on the shared model, so both the main canvas and the mini-map stay in sync (apache#5120 and apache#4027). Drops the hide-region class and SCSS rule. Tests updated accordingly.
Regions are recreated on every RegionUpdateEvent during execution, so a toggle that only flipped existing elements was lost as soon as the next update arrived — regions stayed hidden mid-run even with the toggle on. Make the Regions toggle a shared, persistent flag on JointGraphWrapper. The editor reapplies it to the shared JointJS model both when the flag changes and whenever regions are recreated, so the main canvas and the mini-map stay in sync on and off during execution (apache#5120, apache#4027). The menu toggle now just sets the flag; the editor owns applying it. Tests cover creation-while-on, recreation, and toggling existing regions.
|
on top of that, please ensure we add enough tests to at least make sure coverage is enough. We don't want this fix to cause more bugs. @bobbai00 please prioritize this PR. |
I have fixed this issue. Please check again |
Direct tests for setRegionsDisplayed/getRegionsDisplayed and the BehaviorSubject stream (default, update, replay-and-change).
aglinxinyuan
left a comment
aglinxinyuan
left a comment
There was a problem hiding this comment.
LGTM! Tested locally.
### What changes were proposed in this PR? Restores the **Regions** and **Status** toggles under View, both of which broke after apache#4495 switched these elements from JointJS `visibility` attributes to CSS classes. - **Regions**: region visibility is now a shared, persistent flag on `JointGraphWrapper` (`setRegionsDisplayed` / `getRegionsDisplayed` / stream). The editor applies it to the shared JointJS model both when the flag changes and whenever regions are recreated on a `RegionUpdateEvent`. Because the model is shared, the toggle drives both the main canvas (apache#5120) and the mini-map (apache#4027), and it survives the region elements being recreated during execution (previously regions went hidden mid-run even with the toggle on). The menu toggle just sets the flag; the editor owns applying it. The `hide-region` CSS class/SCSS rule are removed. - **Status** (`workflow-editor.component.scss`, `workflow-editor.component.ts`, `menu.component.ts`): re-added the `hide-operator-status` SCSS rule (retargeted to the renamed `.texera-operator-state` element), added the default class so Status starts hidden, and fixed the stale `.operator-status` selector in `applyOperatorStatusPosition()`. Unit tests added for the toggle delegation, status repositioning, the default `hide-operator-status` class, and region visibility across creation / recreation / toggling. Here is a demo of the style after fix: <img width="1707" height="1162" alt="2026-05-30 11 35 33" src="https://github.com/user-attachments/assets/ae5fc0d3-76f1-4bf8-914e-2644f89b46c2" /> ### Any related issues, documentation, discussions? Closes apache#5120 Regression from apache#4495. ### How was this PR tested? Manual + unit tests. ### Was this PR authored or co-authored using generative AI tooling? Generated-by: Claude Opus 4.8 (1M context) --------- Co-authored-by: Bob Bai <bobbai0509@gmail.com>
4 participants
What changes were proposed in this PR?
Restores the Regions and Status toggles under View, both of which broke after #4495 switched these elements from JointJS
visibilityattributes to CSS classes.JointGraphWrapper(setRegionsDisplayed/getRegionsDisplayed/ stream). The editor applies it to the shared JointJS model both when the flag changes and whenever regions are recreated on aRegionUpdateEvent. Because the model is shared, the toggle drives both the main canvas (Region layer renders on mini-map but not on main workflow canvas #5120) and the mini-map (Mini map still shows Region highlighting when “Regions” is turned off #4027), and it survives the region elements being recreated during execution (previously regions went hidden mid-run even with the toggle on). The menu toggle just sets the flag; the editor owns applying it. Thehide-regionCSS class/SCSS rule are removed.workflow-editor.component.scss,workflow-editor.component.ts,menu.component.ts): re-added thehide-operator-statusSCSS rule (retargeted to the renamed.texera-operator-stateelement), added the default class so Status starts hidden, and fixed the stale.operator-statusselector inapplyOperatorStatusPosition().Unit tests added for the toggle delegation, status repositioning, the default
hide-operator-statusclass, and region visibility across creation / recreation / toggling.Here is a demo of the style after fix:
Any related issues, documentation, discussions?
Closes #5120
Regression from #4495.
How was this PR tested?
Manual + unit tests.
Was this PR authored or co-authored using generative AI tooling?
Generated-by: Claude Opus 4.8 (1M context)