The GitHub Actions job "Comment commands" on texera.git/main has succeeded. Run started by GitHub user sshiv012 (triggered by sshiv012).
Head commit for run: 54fba318c15b38d128e7d28969bd18c72f8fd206 / yangzhang75 <[email protected]> perf(workflow): switch between a workflow's two views without reloading the page (#8581) ### What changes were proposed in this PR? The operator canvas and the Form View are two views of one open workflow, but moving between them was a full browser page load. Everything that made the workflow live was thrown away on the way out -- the Yjs shared document and the co-editing room it holds, the computing unit connection, the execution state -- and rebuilt on the other side: an Angular bootstrap blocked on `/api/config`, a re-parse of the whole bundle, a re-fetch of the operator metadata a root singleton had already cached, a workflow fetch, a new room, a reconnect, and then a wait for the backend to report a run the page it just left already knew about. #### Before https://github.com/user-attachments/assets/3bc1498a-64a2-40fc-8860-4a873491e963 <!-- Drop the recording here: switching canvas -> Form View -> canvas on main. Each switch is a full page load, with the spinner and the reconnect. --> #### After https://github.com/user-attachments/assets/f0f26b87-4628-49fa-9f9e-f3b18b395e5f <!-- Drop the recording here: the same two switches on this branch. --> Both directions now route, and the session is handed over rather than rebuilt. There are two questions to answer, and each is asked in exactly one place: * **`isLeavingWorkspace`** (in `app-routing.constant.ts`) -- is the navigation now in flight leaving this workflow, or moving between the two views it is open under? Both views ask it as they are destroyed and both must answer it the same way, so the rule lives next to the URLs it is about. No navigation in flight means the browser is unloading, which is leaving. * **`WorkflowActionService.hasWorkflowOpen`** -- is the shared document already in this workflow's room? Both views ask it before loading and, when it is, skip the fetch, the reset, the new shared model and the graph rebuild, taking the name and access from what is already open. The canvas additionally lifts the lock the Form View put on the graph, since editing is what a canvas is for, and centres its new paper. `MenuComponent.openFormViewPage` and `WorkflowFormComponent.openCanvasPage` navigate instead of assigning `window.location.href`. The save-then-hand-over ordering around them is unchanged. Leaving the workspace altogether is unchanged: the same teardown, including on unload, where there is no navigation to ask about. Routing between these two views was tried during the Form View work and reverted, for three symptoms the code comments recorded. **The ghost coeditor of yourself is removed here rather than avoided:** the ghost was the second shared document the arriving view built for a workflow the page was already in the room for, so not rebuilding it is what removes it. The other two, undraggable operators and broken runs, are why this is a draft; see below. **One behaviour changes.** A hand-over skips the broken-workflow warning `loadWorkflowWithId` raises, because nothing is loaded. The warning belongs to reading a workflow from the server, and the graph the canvas receives is the one it or the Form View already validated. Previously every switch was a page load, so it was re-raised on each one. **One commit here fixes a defect older than this PR, because this PR is what makes it bite.** `WorkflowEditorComponent` found the element it builds its paper into with `document.getElementById("workflow-editor")`, and that id comes from its own template, so the lookup returns whichever instance is first in the document rather than this one's. Harmless while the two views never coexisted; routing overlaps them for a tick, and the arriving canvas built its paper into the departing Form View's container, coming back blank -- nothing to pan, nothing to click, while the graph itself was untouched. It resolves both elements from its own host now. Carried here rather than left to merge first, so that whichever order these land in, `main` is never the broken one. Closes #8606. `MiniMapComponent` turned out to be exposed to the same tick -- both views render one beside their editor -- so its three lookups (`#mini-map`, `#mini-map-navigator`, and `#workflow-editor` for the navigator's sizing) are carried here too: the first two resolve from its own host, and the third is the main paper's own `el`. What is left in #8606 is `ReportGenerationService`, which snapshots the canvas and is not on this path. **Carried here after all: no JointJS paper was ever disposed.** `WorkflowEditorComponent` and `MiniMapComponent` each create one bound to the root-provided joint graph, which outlives them, and neither `ngOnDestroy` removed it, so every remount left another paper listening to that graph from a detached DOM node. It was filed as [#8582](https://github.com/apache/texera/issues/8582) and left out of this PR while remounting was rare; routing makes a remount the normal switch, which is this PR's doing, so both are disposed here. Whether the undraggable operators recorded in [#8580](https://github.com/apache/texera/issues/8580) follow from the leak is **not settled**: measuring one operator by model id across six round-trips gave a single failure that did not recur, so the symptom is neither confirmed nor ruled out. #8582 stays open for it, and for the Hub preview, which remounts an editor on a path this PR does not touch. #### What a view that arrives on a live session has to be told The review found the same defect several times over: a view created by the switch subscribes to a stream that carries no current value, initialises a field from its default, and waits for an event that does not follow a hand-over. Rather than keep finding them one at a time, every subscription in the components both views create -- the two views themselves, the menu, the computing-unit picker, the editor, the mini-map, the result panel, the property panel: 151 in all -- was checked against one question: *if this component mounts after the state it shows already exists, does it read that state?* And every `ngOnDestroy` against another: *does it undo everything its mount created?* | State a late-mounting view needs | Where it lives | Late mount reads it? | Done here | |---|---|---|---| | Workflow name, id, write access, last-saved time | `workflowMetaDataChanged()` (plain Subject) | No: every consumer initialised from its default | `republishWorkflowMetadata()` on arrival | | Execution state, run/stop button, edit-mode lock | `getExecutionStateStream()` (plain Subject, carries *transitions*) | No | Views read `getExecutionState()`; the canvas asks the service to `reapplyExecutionLock()`; the state is never re-announced as a transition | | Run clock | `ExecutionDurationUpdateEvent` (twice per run) | No: per-view timers hung off the event | Anchored and ticked in `ExecuteWorkflowService`, on a replaying stream both views read | | Failure banner | set only inside the state-stream subscriber | No | `showFailureIfAny(retained)` on arrival, after the fields exist | | Export button | `resetFlags()` on menu destroy | Read `false` for results still there | Recomputed when a menu mounts | | Zoom ratio | wrapper (root), new paper starts at 1 | No: first "zoom in" after a switch could shrink the canvas | Paper adopts the wrapper's ratio on mount | | Result panel contents | re-rendered only on events | No: empty beside an operator still shown selected | Rendered once on init | | Computing unit, its status, the unit list, warehouse, validation, websocket status, modification lock, version preview, heat-map, regions toggle, compilation | `BehaviorSubject` / `ReplaySubject` | Yes, by construction | Nothing needed | | Operator status badges, statistics, region hulls | attributes and cells on the *shared* JointJS model | Yes: a new paper renders the model | Nothing needed | | Results shown in the Form View | read through `WorkflowResultService` getters | Yes | Nothing needed | | Co-editor list | array on the presence service | Yes | Nothing needed | | Teardown a remount now exercises | Done here | |---|---| | Editor and mini-map papers never disposed (#8582) | `paper.remove()` on destroy; the mini-map's off `beforeunload`, which a bfcache restore survives | | Editor keydown listener added with one `.bind()` and removed with another | one bound reference | | Mini-map's three handlers on the main paper, never removed; the paper stream replays to the departing view | removed when a new paper arrives and on destroy | | The rendering context's static reference to the attached paper | detached on destroy, unless a newer paper has already been attached | | Export flags reset by *replacing* the subject, orphaning subscribers | `.next(false)` | | Document-wide DOM lookups in a component both views mount | Done here | |---|---| | Editor `#workflow-editor` / `#workflow-editor-wrapper` (#8606) | own host | | Mini-map `#mini-map`, `#mini-map-navigator`, and the main canvas's element for the navigator | own host; the main paper's own `el` | | Property panel `#right-container` (placement restore) | own host | | Result panel `#result-container` | canvas-only component, one instance; left as is | | Computing-unit picker (rename input, PVE log) | reached only from a user action, not on mount; left as is | Left open, and why: `CodeEditorService.vc` is a root singleton pointing at the canvas's `ViewContainerRef`, so on the Form View it points at a destroyed component; the form's preview panel can reach a Python UDF's code-editor button in edit mode, and pressing it was already broken before this PR (a fresh page had no `vc` at all) -- a fifth piece of "old state attached", not made worse here; it is item 1 of [#8439](https://github.com/apache/texera/issues/8439), the Form View's missing editor host, noted there. A region hull stops following an operator that is dragged until the next region event, because the per-editor map from hull to operators is rebuilt only from that event and the cells do not record it; the regions toggle itself is right on arrival. `ReportGenerationService` and `dashboard.component.ts` also look `#workflow-editor` up in the document, on paths this PR does not touch (#8606). ### Any related issues, documentation, discussions? Closes [#8580](https://github.com/apache/texera/issues/8580). Follow-up to the Form View feature (parent issue [#8011](https://github.com/apache/texera/issues/8011)); the switch itself landed in [#8456](https://github.com/apache/texera/pull/8456). ### How was this PR tested? Unit tests (vitest), by theme. **Hand-over and teardown:** each view keeps the session when its sibling takes over and releases it for any other destination, including another workflow's view and a workflow created in this session, whose document is in no room; each direction routes rather than reloading; a navigation that does not go through lowers the hand-over flag. **Arrival on a live session:** each view re-announces the metadata for the subscribers it has just mounted; the Form View arrives knowing a run is in flight, at the clock the run is at, and showing a failure the run already had; the canvas asks the execute service to reapply its lock rather than unlocking; the computing-unit picker leaves a run in flight locked even when the backend reports no executions; the menu recomputes the export flags; the result panel renders once on init; the heat-map view is reset on leaving and kept on a hand-over. **The run clock**, on the service against real timers: anchored, ticking, replayed to a late subscriber, stopped at the run's end, reset on both reset paths. **Containers and teardown:** the editor, mini-map and property panel build into their own host rather than the document's first match; both papers are disposed on destroy and not on `beforeunload`; the editor's keydown listener and the mini-map's paper handlers are the references that were registered; the rendering context forgets a paper on destroy unless a newer one has attached. **`hasWorkflowOpen` / `getOpenWorkflowId`:** true only for the room the document is in, false for a workflow that has been left, and never for a falsy id. Every new guard was deletion-checked -- removed or inverted, run, restored -- and each turns exactly the intended named tests red. Two tests needed care to discriminate at all: jsdom's `getElementById` returns elements in the order their ids were registered, not tree order, so a decoy container has to be in the document *before* the component is created; and the change-detection pass that runs `ngOnInit` also applies the template's width binding, so a restored placement is probed through `left`. Full frontend suite: 229 files, 6241 passed, 1 skipped (pre-existing), 0 failed. `ng build --configuration=production` (AOT) clean. `eslint` and `prettier --check` on every changed file: clean. **Verified in a browser**, on a local instance running this branch against a 14-operator pipeline that takes about three minutes to run: - The switch routes. A marker set on `window` survives both directions and the document-load count stays at 1; on `main` the marker is gone and the count goes up by one per switch. - The blank canvas after opening the Form View's preview (#8606): reproduced first -- two `#workflow-editor` in the document at the moment the canvas mounts, the lookup answering the first, the canvas's own container left with no SVG -- then fixed, the same steps ending with the canvas's container holding the graph. - A run switched away from and back to, both directions, recorded before and after (the two recordings above). - Dragging an operator after six round-trips: one isolated failure that did not recur, so the undraggable-operator symptom recorded in #8580 is neither confirmed nor ruled out; #8582 keeps it. **Not yet verified in a browser:** a second editor in the room during a switch (the ghost co-editor this removes by construction); the heat-map overlay across a switch, since #8552 landed after the browser pass; and a navigation refused mid-switch, which nobody has managed to reach on purpose. ### Was this PR authored or co-authored using generative AI tooling? Yes. Generated-by: Claude Code (Claude Opus 5, Anthropic). Co-authored with Claude; the author reviewed the change line by line before submission. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01FVvP3ttj22f9LB4p9u2anY --------- Co-authored-by: Claude Opus 5 (1M context) <[email protected]> Report URL: https://github.com/apache/texera/actions/runs/35806685679 With regards, GitHub Actions via GitBox
