Copilot commented on code in PR #8516:
URL: https://github.com/apache/texera/pull/8516#discussion_r3994499960
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -316,7 +346,11 @@ export class WorkflowFormComponent implements OnInit,
OnDestroy {
.getViewResultOperatorsChangedStream()
.pipe(untilDestroyed(this))
.subscribe(() => {
+ // An eye toggled on the canvas changes both what shows (a
newly-viewed step) and what the
+ // author can pick, so rebuild the picker here too -- otherwise a
just-eyed step would not
+ // appear as an option (and an un-eyed one would linger) until the
next full re-read.
this.refreshShownResults();
+ this.rebuildResultChoices();
Review Comment:
This updates result availability only when an eye changes, but the new
filter also depends on operator existence, enabled state, and terminal
topology. Those changes emit separate add/delete/link/disabled streams, so a
co-editor can disable a currently shown step (or add a downstream link) and
this page keeps the stale result card and picker pill until an unrelated
result/config event, contradicting the rule that disabled/unavailable steps are
neither offered nor shown. Subscribe to all graph events that can change
availability and refresh both derived collections.
##########
frontend/src/app/workspace/component/workflow-form/workflow-form.component.ts:
##########
@@ -1301,40 +1545,47 @@ 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;
}
+ // Snapshot now, not when the request's turn comes: on the way out
ngOnDestroy clears the graph
+ // right after asking for this save, and the queue may only reach it after
that.
const preserved: Workflow = {
...workflow,
content: { ...workflow.content, operatorPositions:
this.positionsToSave(workflow.content) },
};
- // On the way out the subscription 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.
- const persist = this.workflowPersistService.persistWorkflow(preserved);
- // The `destroyed` branch deliberately omits untilDestroyed (see above);
the persist call
- // is a one-shot HTTP request that completes on its own, so it needs no
teardown operator.
- // eslint-disable-next-line rxjs-angular/prefer-takeuntil
- (this.destroyed ? persist : persist.pipe(untilDestroyed(this))).subscribe({
- // Feed the saved workflow back, exactly as the operator canvas does:
this advances
- // lastModifiedTime (and the normalised name), and the metadata
subscription then repaints
- // the title bar -- so "Saved at ..." moves past the moment the page
opened.
- next: updatedWorkflow =>
this.workflowActionService.setWorkflowMetadata(updatedWorkflow),
- // A save that fails silently is the worst thing this page can do: the
author walks
- // away believing the form they just built is stored.
- error: () => this.notificationService.error("Could not save. Your latest
changes are not stored yet."),
- });
+ this.persistQueue.next({ workflow: preserved, afterwards });
+ }
+
+ /** One persist of the given snapshot, reporting its outcome; the queue
orders them. */
+ private persistNow(preserved: Workflow): Observable<Workflow> {
+ return this.workflowPersistService.persistWorkflow(preserved).pipe(
+ tap({
+ // Feed the saved workflow back, exactly as the operator canvas does:
this advances
+ // lastModifiedTime (and the normalised name), and the metadata
subscription then repaints
+ // the title bar -- so "Saved at ..." moves past the moment the page
opened.
+ next: updatedWorkflow =>
this.workflowActionService.setWorkflowMetadata(updatedWorkflow),
Review Comment:
Applying every queued response unconditionally can overwrite newer local
metadata. For example, while save A is in flight, rename to B and enqueue save
B; when A completes, this resets the shared metadata (and, after the 100 ms
metadata subscription, the title) to A until B returns, and another autosave in
that window can snapshot A as the newest value. The drain can also run this
after teardown and repopulate the singleton after `clearWorkflow()`. Guard the
response against the request snapshot/current workflow, and skip metadata
feedback after destruction, while still merging server-owned save metadata such
as the timestamp.
--
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]
