yangzhang75 commented on code in PR #8581:
URL: https://github.com/apache/texera/pull/8581#discussion_r4065569224
##########
frontend/src/app/workspace/component/workspace.component.ts:
##########
@@ -316,6 +333,28 @@ 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.
+ // The lock the Form View put on the graph is lifted, 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) {
+ // Not an unconditional unlock: a run may still be in flight. The
execute service owns the
+ // state-to-lock rule and reapplies it only when the state changes, so
ask it to apply that
+ // rule again rather than restating it here. Unlocking outright left a
workflow that was
+ // still running editable until its run happened to end. Nothing is
announced: the menu
+ // reads the current state when it is constructed, and the rest of this
page's subscribers
+ // want transitions, not a repeat of one that already happened.
+ this.executeWorkflowService.reapplyExecutionLock();
Review Comment:
Fixed, and the timing you describe is exactly it: the answer lands after the
page set the lock on arrival, and the query covers only Running and
Initializing, so a handed-over Paused, Pausing, Recovering or Resuming run
answered "none" and the selector unlocked. Rather than widening the query --
which would put a second copy of the lock policy here -- the negative branch
defers to `reapplyExecutionLock`, so "no execution someone else started" no
longer means "nothing is running here".
Worth noting this only started firing because of another fix in this PR: the
resumed canvas now re-announces the workflow metadata, which is what makes the
selector learn the wid and run this lookup at all. Test: with a run in flight
and the backend reporting no executions, the graph stays locked;
deletion-checked. The two existing tests did not catch it because with nothing
running the two spellings behave alike. 338e95c
##########
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. `ExecuteWorkflowService` holds the last duration the backend
reported, and both views read it when they mount. The menu had the same hole on
a resumed canvas -- it also subscribes to the event directly and starts at 0 --
so it reads it in its constructor beside the state it already read there. Test:
a page that attaches to a workflow already 42s into a run comes up at 42s;
deletion-checked. 338e95c
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -609,6 +615,29 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
});
}
+ /** What the page does once the workflow is in front of it, whichever way it
got there. */
+ private settleIntoForm(): void {
+ // The run this page arrived on top of. The state stream is a plain
Subject and carries no
+ // current value, so a page handed a session mid-run hears nothing about
it until the run
+ // changes state: it showed Run for a workflow that was running, and the
lock rule below --
+ // which reads `isRunning` -- would have let edit mode unlock a graph
mid-run. On the loading
+ // path there is nothing in flight and this reads the same Uninitialized
it started at.
+ this.executionState =
this.executeWorkflowService.getExecutionState().state;
Review Comment:
Fixed. The banner logic is `showFailureIfAny` now, called from the
subscriber as before and once on arrival -- after `readConfig`, since it asks
the fields whether a required one was left empty, which is the ordering you
pointed at. Test: a page that attaches to a retained Failed run comes up with
the message; deletion-checked.
On the second reference: line 1843 is `openCanvasPage`, which is the
hand-over out of this page rather than a second place the state is restored, so
I have not changed anything there. If you meant a different line, say which and
I will look again. 338e95c
--
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]
