yangzhang75 commented on code in PR #8581:
URL: https://github.com/apache/texera/pull/8581#discussion_r4065198364
##########
frontend/src/app/workspace/service/execute-workflow/execute-workflow.service.ts:
##########
@@ -194,6 +194,22 @@ export class ExecuteWorkflowService {
return this.currentState;
}
+ /**
+ * Announce the execution state already in hand, unchanged, and reapply the
graph lock it implies,
+ * for a view that arrived on a workflow whose run was already in flight.
+ *
+ * `getExecutionStateStream()` is a plain Subject, so it carries no current
value: a view that
+ * attaches to a handed-over session subscribes after the last state change
and hears nothing
+ * until the next one. Two things were then wrong at once. The arriving page
showed **Run** for a
+ * workflow that was running, because its own `executionState` sat at its
initial value. And the
+ * lock is only reapplied when the state changes (see
`updateExecutionState`), so a canvas that
+ * unlocked the graph on arrival left a running workflow editable until the
run happened to end.
+ */
+ public republishExecutionState(): void {
+ this.updateWorkflowActionLock(this.currentState);
+ this.executionStateStream.next({ previous: this.currentState, current:
this.currentState });
Review Comment:
Right, and the announcement was the wrong idea rather than something to
patch around. The stream carries transitions, not a current value, so repeating
the current state as `previous -> current` of the same state is a transition
that never happened -- `Recovering -> Recovering` reaching the editor's handler
is the sharpest consequence, but the result panel would also have read one as a
run just finishing.
It turns out nothing needed the announcement. `MenuComponent` already reads
`getExecutionState()` when it is constructed, the Form View reads it before
settling in, and the remaining subscribers (result panel, console frame, result
export) want transitions. So the method only reapplies the lock now, and is
named `reapplyExecutionLock` for it. Tests: Running locks and Uninitialized
unlocks; and, with the service left in `Recovering`, a subscriber attached
beforehand receives nothing. Both deletion-checked.
Your review overview also caught the second one, which I had introduced in
the same commit: the mini-map's `ngOnDestroy` carries a `beforeunload` host
binding, so putting `paper.remove()` there disposed a paper the document needs
if it comes back from the back/forward cache -- the same shape as #8599. Split
as that fix split the workspace's: `beforeunload` only remembers whether the
mini-map was hidden. b954174
##########
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")!;
Review Comment:
You are right, and the description was out of date rather than the code --
sorry for the misdirection. All three of the mini-map's lookups were carried in
b88e821, one commit before your comment: `#mini-map` and `#mini-map-navigator`
resolve from its own host, and the navigator's sizing uses the main paper's own
`el` rather than looking `#workflow-editor` up in the document. The sentence in
the PR body that sent them to #8606 is corrected now; what is actually left
there is `ReportGenerationService`, which snapshots the canvas and is not on
this path.
Its paper was undisposed too, which matters for the same reason -- both
views mount one, so routing leaves one behind per switch -- so that is fixed
here as well, and the `beforeunload` binding on its `ngOnDestroy` needed
splitting first so leaving the page does not dispose a paper a bfcache restore
still needs. b954174
--
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]
