Copilot commented on code in PR #8581:
URL: https://github.com/apache/texera/pull/8581#discussion_r4065367805
##########
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:
A routed handoff recreates this component without reconnecting the
websocket, so a run already in progress loses its clock.
`ExecutionDurationUpdateEvent` is a plain `Subject` event and the backend emits
it only when the execution start/end timestamp changes; the newly mounted Form
View therefore starts at `0` and receives no event/timer until the run ends.
Cache/replay the current duration for remounted views before replacing the
full-page navigation.
##########
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:
Restoring only the state enum misses the existing failure-handling side
effect. If the retained state is `Failed`, no new transition is emitted, so the
subscriber at lines 468–475 never populates `runError` and the Form View
silently hides the failure banner. Apply the current state through shared
initialization logic (after the form is built for required-field detection) and
cover a failed-state handoff.
This issue also appears on line 1843 of the same file.
##########
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:
This lock can still be undone after the method returns. The newly mounted
computing-unit selector immediately receives the retained selected unit and
starts an async execution lookup limited to `Running`/`Initializing`; for a
retained `Paused`, `Pausing`, `Recovering`, or `Resuming` state, its empty
response calls `enableWorkflowModification()`. Because no state transition
follows the handoff, the canvas then remains editable during the active
execution. Reconcile that selector with the retained execution state (or
include every locking state) and test a paused/recovering handoff.
--
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]
