Repository navigation
fix(workspace, workflow-form): show the step the shared selection already holds - #9408
Merged
Merged
Conversation
…eady holds A workflow's two views share one selection: it lives in the root-provided JointGraphWrapper, which the switch between the canvas and the Form View hands over rather than rebuilds. Nothing is lost in the switch, but all three things that show the selection followed only the highlight streams and never read it when they mounted, so the view arriving at a selection waited for an event that had already happened: the property panel showed nothing and the step was drawn as not selected. Clicking that step did not help, since the wrapper treats an already-highlighted element as a no-op; it took clicking a different step to bring the panel back. Each now reads the selection it mounts into, then follows the changes: - PropertyEditorComponent (the canvas's panel and every host of it): the stream body becomes showHighlighted(), called before subscribing and only when something is highlighted, since the no-frame branch is for a selection being dropped (it clears the editing marker and resizes the docked panel) and a fresh mount must not run it. Called after the toolbar's choosing toggle has replayed, so the frame is built once with that state rather than rebuilt by the replay in the first change detection pass. The syncTexeraGraph guard moves from the stream's filter into the method, so both paths apply it once. - WorkflowEditorComponent: the paper is new on every switch and the ring was applied only from the stream, so it now rings what the wrapper already holds before following the streams. - WorkflowFormComponent: one call to its own syncSelectionFromHighlight() in ngOnInit, after it subscribes. With nothing selected nothing is read, so the canvas alone behaves as before. The wrapper's already-highlighted no-op stays: it keeps a shift-selection stable, and the arriving view now already shows the step. Co-Authored-By: Claude Fable 5.1 <[email protected]>
Contributor
Backport auto-label reportThis
|
Contributor
Automated Reviewer SuggestionsBased on the
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9408 +/- ##
============================================
- Coverage 91.81% 91.81% -0.01%
Complexity 5019 5019
============================================
Files 1277 1278 +1
Lines 56369 56514 +145
Branches 7364 7417 +53
============================================
+ Hits 51755 51886 +131
- Misses 2759 2764 +5
- Partials 1855 1864 +9
*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:
|
yangzhang75
marked this pull request as ready for review
November 11, 2026 07:54
mengw15
removed
release/v1.2
Contributor
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Copilot review overview
2 open issues
What changed in this PR
Fixes Form View/canvas switch behavior so UI elements immediately reflect an already-existing shared selection (instead of waiting for a highlight event that won’t re-fire).
Changes:
- Read and apply current selection on mount for Form View, Workflow Editor highlighting (“ring”), and Property Editor frame selection.
- Refactor Property Editor highlight handling to reuse a single
showHighlighted()path and add tests covering mount-into-existing-selection behavior. - Update Form View template comment to reflect the current rationale for
[hidden]vs*ngIf.
| File | Description |
|---|---|
| frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts | Pulls current selection once on init so Form View opens correctly when arriving with an existing selection. |
| frontend/src/app/workspace/component/workflow-form/workflow-form.component.spec.ts | Adds coverage for arriving with 0/1/many selected steps already in the shared wrapper. |
| frontend/src/app/workspace/component/workflow-form/workflow-form.component.html | Updates rationale comment for keeping the panel mounted while hidden. |
| frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts | Applies highlight to currently-selected elements immediately on paper creation, then subscribes to streams. |
| frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.spec.ts | Adds a regression test ensuring “ring existing selection on wiring up” runs once. |
| frontend/src/app/workspace/component/property-editor/property-editor.component.ts | Refactors highlight handling to read selection on mount and centralize the decision logic in showHighlighted(). |
| frontend/src/app/workspace/component/property-editor/property-editor.component.spec.ts | Adds tests for mounting into existing selection + choosing-toggle replay order. |
Review effort: Lite
Copilot stopped reviewing on behalf of
mengw15 due to an error
November 11, 2026 22:31
The comment read "only when there is one", which a reader could take as one operator; the guard is "only when something is selected", and the reason is that a fresh mount has no frame to clear while the no-frame branch has side effects (editing marker, docked-panel resize). Says so, and what a link- or comment-box-only selection comes to. Co-Authored-By: Claude Fable 5.1 <[email protected]>
mengw15
enabled auto-merge
November 11, 2026 22:46
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Nov 11, 2026
github-merge-queue
Bot
removed this pull request from the merge queue due to failed status checks
Nov 12, 2026
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
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?
Closes #9400.
A workflow's two views share one selection: it lives in the root-provided
JointGraphWrapper, which the switch between the canvas and the Form View hands over rather than rebuilds. Nothing is lost in the switch, but everything that shows the selection followed only the highlight streams and never read it when it mounted. The event that made the selection had already happened, so the view arriving at it waited for an event that never came: the panel showed nothing, and the step was drawn as not selected. Clicking that step did not help, since the wrapper treats an already-highlighted element as a no-op and emits nothing; it took clicking a different step to bring the panel back.Each of the three now reads the selection it mounts into, then follows the changes as before:
PropertyEditorComponent(the canvas's panel and every host of it, so not specific to the Form View, to any operator type, or to operators as opposed to ports): the body of the highlight subscription becomesshowHighlighted(), the same decision as before (one operator, one port, or nothing) with thesyncTexeraGraphguard that the stream applied as afilternow inside it, so it is applied once on both paths.registerHighlightEventsHandler()calls it before subscribing, and only when something is highlighted: the no-frame branch is for a selection being dropped (it clears the editing marker on the shared awareness channel and resizes the docked panel to its empty content), which a fresh mount must not do. It is also called after the toolbar's choosing toggle has replayed its value, so the frame it builds carrieschoosingfrom the start instead of being torn down and rebuilt by the replay inside the page's first change-detection pass. NotstartWithon the stream: its initial emission is unconditional, which is exactly the empty-branch-at-mount this avoids.WorkflowEditorComponent: the ring. The paper is new on every switch, and the highlighter was applied only from the stream, so the arriving view would have shown its panel for a step drawn as not selected. The handler now rings the operators and comment boxes the wrapper already holds, then follows the streams; thefindViewByModelguard the unhighlight branch already had is shared with it.WorkflowFormComponent: one call to thesyncSelectionFromHighlight()it already has, inngOnInitafter it subscribes, so the form's step panel opens on the selection it arrives with. The template note that justified[hidden]over*ngIfby the panel having "no initial pull" is reworded to the reason that still holds.No change to who owns the selection, to what a click does, or to the canvas's own behaviour when it is the only view open: with nothing selected, nothing is read, and both panels mount empty exactly as before. The wrapper's no-op for an already-highlighted element is deliberately untouched (it is what keeps a shift-selection stable); it simply stops mattering, because the arriving view already shows that step.
Video
Before
701575292-964c65df-4a17-4d9e-86ce-61fde00e383d.mov
After
Screen.Recording.2026-11-10.at.11.52.58.PM.mov
Any related issues, documentation, discussions?
Closes #9400. Part of the Form View (#8011).
Related to #8846, which touches
workflow-form.component.tselsewhere and does not read the selection on mount: itsframeWritesguard comparesselectedOperatorId, which onlysyncSelectionFromHighlight()sets, so on arriving in the Form View with a step selected that guard sawundefinedand the form wrote the declared parameters even while the live panel showed that step. With this changeselectedOperatorIdis set on arrival and the guard holds. The two do not overlap in code; whichever lands second rebases.How was this PR tested?
property-editor.component.spec.ts: a panel mounting into an existing selection shows that operator; mounting into no selection leaves the panel alone and mounting into two shows no frame (the two-operator selection is made explicit withsetMultiSelectMode(true)and asserted before the check); and the frame a panel mounts into is built with the toolbar's choosing state, with no rebuild, which pins the order of the two subscriptions.workflow-editor.component.spec.ts: wiring the highlight handler into a selection the wrapper already holds rings that operator once (a spy installed after the stream has done its work).workflow-form.component.spec.ts: the form's step panel opens on the step the canvas had selected, and stays closed for none or several — seeded with a stale id, so a read that never happened could not pass as "undefined".main(cfca8e1e5) and this branch served side by side on the same backend and the same workflow, driven by a Playwright probe that reads what the panel shows at each step:Was this PR authored or co-authored using generative AI tooling?
Yes. Generated-by: Claude Code (Claude Fable 5.1, Anthropic). Co-authored with Claude, reviewed line by line by the author before submission.
🤖 Generated with Claude Code