yangzhang75 commented on code in PR #8540:
URL: https://github.com/apache/texera/pull/8540#discussion_r4111836450
##########
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:
Agreed, waiting on the queue made the switch depend on requests it did not
make. Bounded in aff3b835a: the wait for the drain is `timeout`ed at
`HANDOVER_DRAIN_TIMEOUT_MS` (10 s); past it the hand-over leaves as it did
before the wait existed, the request going on in the service with nobody to
answer to, and an edit landed meanwhile is not saved again (that save would
queue behind the request that never answers). Spec: a drain that never emits,
the clock advanced to the bound, the hand-over leaves and `persistWorkflow` was
called once. What the bound does not cover, since it predates this PR: the
queue is serial (#8456), so a request that never answers *ahead* of the
switch's own save stalls that save the same way; a timeout on the persist
request itself would be the general fix, and is not in this PR.
##########
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:
Right, the default id is two states. Told apart in 4ac6dbe7f by the id the
save was sent with: `withLocalEdits(response, sentWid)` merges the local edits
only when the save answers the open workflow (`current.wid === sentWid`) or
created the one the page still holds under the default id (both ids default). A
page cleared meanwhile holds the default id but its save went out with the real
one, so its response is left alone. Spec: `wid: 9` sent, metadata back to the
default by the time it answers, the response keeps its own name and description
(red on the old check).
--
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]
