yangzhang75 commented on code in PR #8581:
URL: https://github.com/apache/texera/pull/8581#discussion_r4056199844
##########
frontend/src/app/workspace/component/workflow-editor/workflow-editor.component.ts:
##########
@@ -205,8 +205,14 @@ export class WorkflowEditorComponent implements OnInit,
AfterViewInit, OnDestroy
}
ngAfterViewInit() {
- this.editor = document.getElementById("workflow-editor")!;
- this.editorWrapper = document.getElementById("workflow-editor-wrapper")!;
+ // This component's own elements, not whichever the document happens to
hold first. Two of
+ // these editors are briefly in the page at once when the two views of a
workflow hand over:
+ // the arriving one initialises while the departing one is still being
removed. Searching the
+ // document returned the departing view's container, so the paper was
built into a div about
+ // to disappear and the arriving canvas stayed blank, with nothing to pan
and nothing to click.
+ const host = this.elementRef.nativeElement as HTMLElement;
+ this.editor = host.querySelector("#workflow-editor")!;
+ this.editorWrapper = host.querySelector("#workflow-editor-wrapper")!;
Review Comment:
Fixed, both of them. `WorkflowEditorComponent` and `MiniMapComponent` each
call `paper.remove()` on destroy now, so a mount no longer leaves one listening
to the root-provided graph from a detached node. Tests: each fixture's paper is
disposed when the fixture is destroyed; both deletion-checked.
One correction to what I can claim for it: I went looking for the
user-visible symptom and could not establish one. A first attempt measured "the
first .joint-element", which is not the same element once one has been dragged
or selected, and produced a meaningless alternation I briefly took for
evidence. Measuring one operator by model id across six round-trips gave one
isolated failure that did not recur, so the undraggable-operator symptom
recorded in #8580 is still unconfirmed either way. The fix stands on the leak
itself, which is real. 8582 stays open for that symptom. b88e821
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -566,7 +566,24 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
// without loading anything, so a request that then fails cannot strand
the visitor on
// an error instead of the page they would have gotten.
if (!this.config.env.formViewEnabled) {
- void this.router.navigate([USER_WORKSPACE, String(wid)], { replaceUrl:
true });
+ void this.router.navigateByUrl(workspaceCanvasUrl(wid), { replaceUrl:
true });
+ return;
+ }
+ // Arriving from the operator canvas of this same workflow, which kept its
session for us.
+ // The graph is already here and already in its co-editing room, so there
is nothing to fetch
+ // and nothing to rebuild: take the name and access from what is open and
settle in.
+ if (this.workflowActionService.hasWorkflowOpen(wid)) {
+ const metadata = this.workflowActionService.getWorkflowMetadata();
+ this.workflowName = metadata.name;
+ this.storedPositions = {
...(this.workflowActionService.getWorkflow().content?.operatorPositions ?? {})
};
+ this.canEdit = !metadata.readonly;
+ this.settleIntoForm();
Review Comment:
Fixed, and thank you -- this is the half of a root cause I had fixed for the
workflow metadata and missed for the execution state. `settleIntoForm` reads
`getExecutionState()` before applying the lock rule, so a page handed a session
mid-run arrives knowing it, and `mayUnlock` (which reads `isRunning`) sees the
run. Test: with a run in flight, a page that attaches to the open workflow
comes up with `executionState` Running; deletion-checked. b88e821
##########
frontend/src/app/workspace/component/workspace.component.ts:
##########
@@ -316,6 +333,22 @@ export class WorkspaceComponent implements AfterViewInit,
OnInit, OnDestroy {
registerLoadOperatorMetadata() {
const wid = this.route.snapshot.params.id;
+ // The Form View handed this workflow over still open: the graph, its
co-editing room and the
+ // computing unit connection are the ones it was using. Nothing to fetch,
nothing to rebuild.
+ // Only the lock it put on the graph is undone, since editing is what the
canvas is for, and
+ // the view is centred because this canvas's paper is a new one, at its
default offset.
+ if (this.resumedSession) {
+ this.workflowActionService.enableWorkflowModification();
Review Comment:
Fixed. The resume path calls a new
`ExecuteWorkflowService.republishExecutionState()` instead of
`enableWorkflowModification()`: it reapplies the state-to-lock rule the service
already owns, so a workflow still running stays locked, and it re-emits the
state for the subscribers the new page has just mounted. Restating the rule in
the component would have been a second copy of it. Tests: the resume path asks
for the reapply and never unlocks outright; on the service, Running locks and
Uninitialized unlocks, and the current state is re-announced. All
deletion-checked. b88e821
##########
frontend/src/app/workspace/component/menu/menu.component.ts:
##########
@@ -756,15 +756,16 @@ export class MenuComponent implements OnInit, OnDestroy {
}
/**
- * The full-page handover to the Form View, apart from the save so the order
is testable.
- * Excluded from coverage as a whole: jsdom cannot navigate, so the specs
stub this method and
- * assert when it is called rather than what it does.
+ * The hand-over to the Form View, apart from the save so the order is
testable.
+ *
+ * A route, not a page load: the two views are views of one open workflow,
and reloading threw
+ * away everything that made the workflow live -- the shared document, the
computing unit
+ * connection, the execution state -- only to rebuild it on the other side.
The canvas keeps
+ * the session on its way out (see its ngOnDestroy) and the Form View
attaches to it.
*/
- /* v8 ignore start */
private openFormViewPage(wid: number): void {
- window.location.href = `${USER_WORKSPACE}/${wid}/form`;
+ void this.router.navigateByUrl(workspaceFormUrl(wid));
Review Comment:
Fixed. One bound reference is stored and used for both `addEventListener`
and `removeEventListener`. Test: after the fixture is destroyed, a real
`keydown` on the document no longer reaches the handler; deletion-checked by
restoring the second `.bind(this)`.
While here: `MiniMapComponent` found both its own container and the main
canvas's by document-wide id lookup, which has the same problem, since both
views mount a mini-map. It uses its own host and the main paper's `el` now.
b88e821
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
