Ma77Ball commented on code in PR #8540:
URL: https://github.com/apache/texera/pull/8540#discussion_r4031777788
##########
frontend/src/app/common/service/workflow-persist/workflow-persist.service.ts:
##########
@@ -68,29 +72,74 @@ 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 the page on completion (the view
switches): its own save
+ * completing is not enough, a save queued behind it (a rename's, a
description's) would still be
+ * aborted by the page load.
+ */
+ 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 {
Review Comment:
With `withLocalEdits` now applied to every persist response inside this
service, the per-caller name guard in `workflow-form.component.ts:1923`
(`current.name !== preserved.name ? { ...updatedWorkflow, name: current.name }
: updatedWorkflow`) is redundant: by the time that handler runs,
`updatedWorkflow.name` is already the page's current name, so both branches
produce the same result.
Why it matters: two places now enforce the same "keep the local name over
the response" rule, and the caller-side one is subtler (name-only, keyed off a
`preserved` snapshot). They will drift - a future change to `withLocalEdits`
(or a reader trusting only one of them) can reintroduce the exact bug this PR
fixes, for description in particular, which the caller-side guard never covered.
--
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]
