yangzhang75 commented on code in PR #8516:
URL: https://github.com/apache/texera/pull/8516#discussion_r3994426632
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -510,11 +545,41 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
}
/**
- * Show the workflow rather than edit it: the graph shape and its properties
are read-only
- * on this page. A later PR's authoring mode makes properties editable with
write access.
+ * Whether this page may hold the graph unlocked: a writer in edit mode, and
no run in flight. The
+ * run rule is the canvas's own (a run locks the graph until it ends), kept
here too so that entering
+ * edit mode mid-run cannot undo it. Every other state is locked, so a
reader, or a writer merely
+ * viewing, cannot change the workflow through this page.
*/
+ private mayUnlock(): boolean {
+ return this.authoring && this.canEdit && !this.isRunning;
+ }
+
+ /** Lock or unlock editing from the current state (see mayUnlock); the one
reducer for both directions. */
private applyEditability(): void {
- this.workflowActionService.disableWorkflowModification();
+ if (this.mayUnlock()) {
+ this.workflowActionService.enableWorkflowModification();
+ } else {
+ this.workflowActionService.disableWorkflowModification();
+ }
+ }
+
+ /**
+ * The lock is a root-level flag with writers that know nothing of this
page: the execute service
+ * unlocks it whenever a run ends (completed, failed, killed, reset), the
computing-unit selector
+ * unlocks it when it finds no run on the chosen unit, and any future caller
may too. Rather than
+ * chase each one, clamp at the one place they all report to: whenever the
flag turns on while this
+ * page must stay locked, turn it off again. In edit mode the canvas rule
then stands unchanged:
+ * locked while a run is in flight, unlocked by the execute service once it
ends.
+ */
+ private clampEditability(): void {
+ this.workflowActionService
+ .getWorkflowModificationEnabledStream()
+ .pipe(untilDestroyed(this))
+ .subscribe(enabled => {
+ if (enabled && !this.mayUnlock()) {
+ this.workflowActionService.disableWorkflowModification();
Review Comment:
Confirmed against updateExecutionState (lock first, state second). 3660475dc
re-applies the editability rule in the execution-state handler once the state
has settled, so edit mode gets the unlock back; the clamp stays for the other
unlockers. The three specs now emit the unlock before the state, and a deletion
check on the re-application turns two of them red.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -1301,40 +1526,56 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
* workflow when the payload has no id, so saving whatever the graph holds
would spawn
* stray "Untitled workflow" rows when the page is left before its workflow
loaded.
*/
- private save(): void {
+ private save(afterwards?: () => void): void {
// A read-only viewer can open and run the form (execution is gated on
computing-unit access,
// not workflow access) but must never persist: every such save is a
guaranteed 403 that would
// spam "Could not save" on each debounce. Their inputs are non-editable,
so nothing is lost.
+ // `afterwards` runs once the save has completed, or at once when there is
nothing to save;
+ // it does not run when the save fails, so a caller that navigates on it
stays put instead.
if (!this.canEdit) {
+ afterwards?.();
return;
}
if (!this.userService.isLogin() ||
!this.workflowPersistService.isWorkflowPersistEnabled()) {
+ afterwards?.();
return;
}
const workflow = this.workflowActionService.getWorkflow();
if (workflow.wid === undefined || workflow.wid !== this.wid) {
+ afterwards?.();
return;
}
+ if (this.destroyed) {
+ // On the way out there is no page left to queue on, and the request
must NOT be tied to this
+ // component: ngOnDestroy calls save(), and untilDestroyed would tear
the subscription down as
+ // part of the very same destroy sequence, aborting the request that was
the point of the
+ // call. The persist is a one-shot HTTP request that completes on its
own, so it needs no
+ // teardown operator; the error is reported inside persistNow.
+ // eslint-disable-next-line rxjs-angular/prefer-takeuntil
+ this.persistNow().subscribe({ error: () => undefined });
Review Comment:
Agreed. 3660475dc drops the direct destroy-time persist: the queue's drain
is no longer tied to the component, the final save joins it behind whatever is
in flight, and ngOnDestroy completes the queue so the drain ends by itself.
Each request carries the snapshot taken when the save was asked for
(ngOnDestroy clears the graph right after enqueueing, so a send-time snapshot
would have been empty). Spec: the final save waits for an in-flight autosave
and goes out after it.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -510,11 +545,41 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
}
/**
- * Show the workflow rather than edit it: the graph shape and its properties
are read-only
- * on this page. A later PR's authoring mode makes properties editable with
write access.
+ * Whether this page may hold the graph unlocked: a writer in edit mode, and
no run in flight. The
+ * run rule is the canvas's own (a run locks the graph until it ends), kept
here too so that entering
+ * edit mode mid-run cannot undo it. Every other state is locked, so a
reader, or a writer merely
+ * viewing, cannot change the workflow through this page.
*/
+ private mayUnlock(): boolean {
+ return this.authoring && this.canEdit && !this.isRunning;
+ }
+
+ /** Lock or unlock editing from the current state (see mayUnlock); the one
reducer for both directions. */
private applyEditability(): void {
- this.workflowActionService.disableWorkflowModification();
+ if (this.mayUnlock()) {
+ this.workflowActionService.enableWorkflowModification();
+ } else {
+ this.workflowActionService.disableWorkflowModification();
+ }
+ }
+
+ /**
+ * The lock is a root-level flag with writers that know nothing of this
page: the execute service
+ * unlocks it whenever a run ends (completed, failed, killed, reset), the
computing-unit selector
+ * unlocks it when it finds no run on the chosen unit, and any future caller
may too. Rather than
+ * chase each one, clamp at the one place they all report to: whenever the
flag turns on while this
+ * page must stay locked, turn it off again. In edit mode the canvas rule
then stands unchanged:
+ * locked while a run is in flight, unlocked by the execute service once it
ends.
+ */
+ private clampEditability(): void {
+ this.workflowActionService
+ .getWorkflowModificationEnabledStream()
+ .pipe(untilDestroyed(this))
+ .subscribe(enabled => {
+ if (enabled && !this.mayUnlock()) {
Review Comment:
Yes, exactly that, and the spec ordering too. 3660475dc: applyEditability()
runs in the execution-state subscription after this.executionState =
current.state, the clamp stays for the computing-unit selector and any other
unlocker, and the specs emit the unlock before the state as the service does.
Deleting the re-application turns the mid-run and the run-ended specs red.
--
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]
