mengw15 commented on code in PR #8540:
URL: https://github.com/apache/texera/pull/8540#discussion_r4108353813
##########
frontend/src/app/workspace/component/menu/menu.component.ts:
##########
@@ -766,14 +771,26 @@ export class MenuComponent implements OnInit, OnDestroy {
this.notificationService.error("Could not save. Your latest changes
are not stored yet.");
},
complete: () => {
- if (this.editedSinceSwitchSnapshot) {
- // An edit landed while the save was out; store it here rather
than leave it to an
- // autosave that would fire under the other view.
- this.saveThenOpenFormView(target);
- return;
- }
- this.isSaving = false;
- this.openFormViewPage(target);
+ // A save queued behind ours (a rename's, a description's: those
save through the menu
+ // itself, not the autosave) answers to this component: its error is
shown here, its
+ // response fed back here, and neither reaches a component the route
has destroyed. Leave
+ // once it has answered; a failure of its own is reported by its
caller and does not hold
+ // the hand-over.
+ this.workflowPersistService
+ .whenSavesDrained()
Review Comment:
The switch's liveness now depends on a request it did not make. It used to
leave when its own save completed; it now waits for the whole queue, so a save
queued behind it that never answers — not one that fails, `catchError` plus
`finalize` covers that — leaves `pendingSaves` above zero for good. `isSaving`
and `handingOverToFormView` both stay set, so the spinner does not stop and the
button does not respond again; the ways out are a reload or leaving by some
other route.
Nothing bounds it: `app.module.ts` registers only the blob-error and
unauthorized interceptors, and nothing on this path uses `timeout`. Narrow to
trigger (a rename or description save queued behind the switch's, and that
request hanging rather than erroring), so not blocking — but worth being
deliberate about, since waiting on its own save could not hang on anyone else's.
##########
frontend/src/app/common/service/workflow-persist/workflow-persist.service.ts:
##########
@@ -68,29 +72,75 @@ export class WorkflowPersistService {
* before it has landed too. Each request snapshots its payload when asked
for; it is sent when its
* turn comes, and its outcome is relayed to that caller alone. A failed
save fails its own caller
* and does not hold up the next.
+ *
+ * A response is relayed with the page's current name and description in
place of its own (see
+ * withLocalEdits): those are the two fields a user edits, and a response
answers the save it was
+ * sent for, which may be older than an edit made since. Callers feed the
response back as the
+ * workflow's metadata; without this, a rename made while a save was out
came back undone.
*/
private readonly persistQueue = new Subject<{ send: Observable<Workflow>;
result: Subject<Workflow> }>();
+ /** Saves asked for and not yet answered (or failed); see whenSavesDrained.
*/
+ private pendingSaves = 0;
+ private readonly savesDrained = new Subject<void>();
+
constructor(
private http: HttpClient,
- private notificationService: NotificationService
+ private notificationService: NotificationService,
+ // Looked up lazily, at response time: the persist service is also used by
the dashboard, where
+ // no workflow is open and constructing the (graph-owning) action service
would be a side effect.
+ private injector: Injector
) {
this.persistQueue
.pipe(
concatMap(({ send, result }) =>
send.pipe(
+ map(updated => this.withLocalEdits(updated)),
tap({
next: updated => result.next(updated),
error: (err: unknown) => result.error(err),
complete: () => result.complete(),
}),
- catchError(() => EMPTY)
+ catchError(() => EMPTY),
+ finalize(() => this.saveDone())
)
)
)
.subscribe();
}
+ /**
+ * Emits once every save asked for so far has been answered or has failed;
at once when none is
+ * pending. For a caller that leaves its view on completion (the Form View
switch): its own save
+ * completing is not enough. A save queued behind it (a rename's, a
description's) is still sent
+ * -- this service outlives the view -- but it answers to the component that
asked for it, and a
+ * component the route has destroyed shows no error and feeds back no
response.
+ */
+ public whenSavesDrained(): Observable<void> {
+ return this.pendingSaves === 0 ? of(undefined) :
this.savesDrained.pipe(take(1));
+ }
+
+ private saveDone(): void {
+ this.pendingSaves -= 1;
+ if (this.pendingSaves === 0) {
+ this.savesDrained.next();
+ }
+ }
+
+ /**
+ * The response with the page's current name and description: a response
carries the values the
+ * save was sent with, and an edit made since would be undone by feeding
them back. Left alone when
+ * another workflow is open by now (nothing local belongs to this response);
a workflow just created
+ * still carries the default id locally, and its rename made meanwhile is
kept too.
+ */
+ private withLocalEdits(response: Workflow): Workflow {
+ const current =
this.injector.get(WorkflowActionService).getWorkflowMetadata();
+ if (current.wid !== response.wid && current.wid !== DEFAULT_WORKFLOW.wid) {
Review Comment:
`current.wid !== DEFAULT_WORKFLOW.wid` matches two states, not one. The
intended one is a workflow the save has just created. The other is a page that
has been cleared: `clearWorkflow` sets the metadata to `DEFAULT_WORKFLOW`, so a
save still in flight when the workspace is left answers with `current.wid ===
0` and is relayed carrying `name: "Untitled Workflow"` and `description:
undefined`.
Nothing applies that today, because every caller is scoped to a component
already destroyed by then (the form's late drain has its own `destroyed`
guard), so it is latent rather than broken. Worth saying because that scoping
is what makes the branch safe, and a caller added here later would have no way
to know it has to be component-scoped.
--
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]
