yangzhang75 commented on code in PR #8600:
URL: https://github.com/apache/texera/pull/8600#discussion_r4051387412
##########
frontend/src/app/workspace/component/workspace.component.ts:
##########
@@ -173,13 +173,27 @@ export class WorkspaceComponent implements AfterViewInit,
OnInit, OnDestroy {
this.codeEditorService.vc = this.codeEditorViewRef;
}
+ /**
+ * The browser is leaving this document: save the workflow, and change
nothing else.
+ *
+ * Tearing the session down here was the cause of a page that came back
dead. A full-page
+ * navigation away (the Form View switch is one) fires this, and the browser
may then keep the
+ * document in its back/forward cache rather than discarding it. Coming back
restores the
+ * JavaScript state exactly as it was left, so whatever this method had
already destroyed stayed
+ * destroyed: an empty graph on a canvas that answered no clicks, and a
workflow id reset to the
+ * default, which the share dialog then asked the backend about and got an
error for. Nothing
+ * re-runs on a restore, because the component was never re-created.
+ *
+ * There is nothing to tear down on the way out anyway. A document that is
really discarded takes
+ * its websockets and its graph with it, and a document that comes back
needs them.
+ */
@HostListener("window:beforeunload")
- ngOnDestroy() {
- if (this.userService.isLogin() &&
this.workflowPersistService.isWorkflowPersistEnabled()) {
- const workflow = this.workflowActionService.getWorkflow();
-
this.workflowPersistService.persistWorkflow(workflow).pipe(untilDestroyed(this)).subscribe();
- }
+ onBeforeUnload(): void {
Review Comment:
Gated. The Share button is disabled until the workflow id arrives
(`[disabled]="!workflowId"`) and `onClickOpenShareAccess` refuses the same
window, so the dialog can no longer be opened on wid 0. Checked your reading of
it first and it holds: `resetAsNewWorkflow()` at `workspace.component.ts:163`,
no gate on the button, and `<texera-menu>` sits outside the spinner container
(`workspace.component.html:31`) — which is also why the in-page path never
reproduced for me. Tests: the dialog is not opened for wid 0 or for no id, and
the button is disabled until the id arrives; both deletion-checked. 7aa3c77
##########
frontend/src/app/workspace/component/workspace.component.ts:
##########
@@ -173,13 +173,27 @@ export class WorkspaceComponent implements AfterViewInit,
OnInit, OnDestroy {
this.codeEditorService.vc = this.codeEditorViewRef;
}
+ /**
+ * The browser is leaving this document: save the workflow, and change
nothing else.
+ *
+ * Tearing the session down here was the cause of a page that came back
dead. A full-page
+ * navigation away (the Form View switch is one) fires this, and the browser
may then keep the
+ * document in its back/forward cache rather than discarding it. Coming back
restores the
+ * JavaScript state exactly as it was left, so whatever this method had
already destroyed stayed
+ * destroyed: an empty graph on a canvas that answered no clicks, and a
workflow id reset to the
+ * default, which the share dialog then asked the backend about and got an
error for. Nothing
+ * re-runs on a restore, because the component was never re-created.
+ *
+ * There is nothing to tear down on the way out anyway. A document that is
really discarded takes
+ * its websockets and its graph with it, and a document that comes back
needs them.
+ */
@HostListener("window:beforeunload")
- ngOnDestroy() {
- if (this.userService.isLogin() &&
this.workflowPersistService.isWorkflowPersistEnabled()) {
- const workflow = this.workflowActionService.getWorkflow();
-
this.workflowPersistService.persistWorkflow(workflow).pipe(untilDestroyed(this)).subscribe();
- }
+ onBeforeUnload(): void {
Review Comment:
Gated. The Share button is disabled until the workflow id arrives
(`[disabled]="!workflowId"`) and `onClickOpenShareAccess` refuses the same
window, so the dialog can no longer be opened on wid 0. Checked your reading of
it first and it holds: `resetAsNewWorkflow()` at `workspace.component.ts:163`,
no gate on the button, and `<texera-menu>` sits outside the spinner container
(`workspace.component.html:31`) — which is also why the in-page path never
reproduced for me. Tests: the dialog is not opened for wid 0 or for no id, and
the button is disabled until the id arrives; both deletion-checked. 7aa3c77
##########
frontend/src/app/dashboard/component/user/share-access/share-access.component.ts:
##########
@@ -137,10 +137,19 @@ export class ShareAccessComponent implements OnInit,
OnDestroy {
this.owner = name;
});
// Stays null for kinds that cannot be published, which is what hides the
publish buttons.
+ // A failed request leaves it null too, and the buttons are equally gone,
so say so: without
+ // this the dialog looked complete while quietly offering one control
fewer, and the only way
+ // to find out was the network tab.
this.descriptor
Review Comment:
Fixed. `isPublic` is reset in the error handler, so a failed refresh drops
the previous value instead of leaving the buttons on screen showing a state
nothing has confirmed — which is what the toast already claimed. Test: a second
`ngOnInit` whose read fails leaves `isPublic` null; deletion-checked. 7aa3c77
##########
frontend/src/app/workspace/component/workspace.component.spec.ts:
##########
@@ -497,6 +497,35 @@ describe("WorkspaceComponent", () => {
expect(workflowResultService.clearResults).toHaveBeenCalled();
});
+ // A full-page navigation away fires beforeunload, and the browser may
then keep this document
+ // in its back/forward cache instead of discarding it. Coming back
restores the JavaScript
+ // state as it was left and re-runs nothing, so anything torn down here
stays torn down: the
+ // graph came back empty, the workflow id came back as the default, and
the still-subscribed
+ // autosave then wrote that default out as a new, blank workflow (issue
#8599).
+ it("saves on beforeunload and tears nothing down, so a page restored from
the cache still works", async () => {
+ await createFixture();
+ fixture.detectChanges();
+
+ component.onBeforeUnload();
Review Comment:
Fixed. The test dispatches a real `beforeunload` event now, as the Form View
rendered test does. Deletion-checked: removing the `@HostListener` turns it
red, which it would not have before. 7aa3c77
--
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]
