Copilot commented on code in PR #8581:
URL: https://github.com/apache/texera/pull/8581#discussion_r4057130829
##########
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:
`republishExecutionState()` emits a synthetic `Recovering -> Recovering`
transition when a handoff occurs during recovery. The newly mounted
`WorkflowEditorComponent` subscribes before this call and handles every event
whose previous state is `Recovering` as a transition out; because `Recovering`
is not one of its accepted destinations, it raises `unknown state transition
from recovering state` (`workflow-editor.component.ts:403-413`). Switching back
to the canvas during recovery therefore produces an unhandled RxJS error.
Reapply the lock separately and expose the retained state without fabricating a
transition (for example, initialize consumers from `getExecutionState()` or add
a dedicated replay/current-state mechanism).
##########
frontend/src/app/workspace/component/workflow-editor/mini-map/mini-map.component.ts:
##########
@@ -90,6 +97,10 @@ export class MiniMapComponent implements AfterViewInit,
OnDestroy {
@HostListener("window:beforeunload")
ngOnDestroy(): void {
+ // Bound to the root-provided joint graph, which outlives this component:
an undisposed paper
+ // goes on listening to that graph from a detached node, and once the
switch between a
+ // workflow's two views routes, one is left behind on every switch (issue
#8582).
+ this.ownPaper?.remove();
localStorage.setItem("mini-map", JSON.stringify(this.hidden));
}
Review Comment:
This method is also the `window:beforeunload` handler, so `paper.remove()`
now disposes the live mini-map during unload. If the browser puts the document
in the back/forward cache, Angular does not recreate this component on restore
and the mini-map remains disposed. Keep the beforeunload handler
persistence-only and dispose the paper only from Angular's actual
`ngOnDestroy`; add a restore-oriented regression assertion that unload itself
does not call `remove()`.
--
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]
