mengw15 commented on code in PR #8581:
URL: https://github.com/apache/texera/pull/8581#discussion_r4075619278
##########
frontend/src/app/workspace/component/menu/menu.component.ts:
##########
@@ -711,9 +711,10 @@ export class MenuComponent implements OnInit, OnDestroy {
}
/**
- * Open the Form View -- a full page load, not a route: the two views share
root-level
- * singletons (graph, Yjs shared model), and routing left the old
collaboration client
- * alive (you appeared as your own coeditor). A fresh document is the clean
handover.
+ * Open the Form View. A route, not a page load: the two views are views of
one open workflow,
Review Comment:
`handingOverToFormView` is set at line 742 and cleared only in the `error`
branch at line 764. The success path reaches `openFormViewPage`, which does
`void this.router.navigateByUrl(...)` and discards the result — `false` for a
navigation that is cancelled or fails.
With `window.location.href` both were free: the document was discarded, so
the flag went with it and a failure was not something this page could observe.
With a route the page survives, and a navigation that does not complete leaves
the guard at line 721 permanently true, so the Form View button stops
responding for the rest of the session with nothing logged.
I could not construct a reliable way to reach it — most cancellations
destroy this component anyway — so this is a loose end from the old assumption
rather than a bug I can show you. Taking the promise's result and putting the
flag back on `false` covers both.
##########
frontend/src/app/workspace/component/workspace.component.ts:
##########
@@ -197,11 +210,19 @@ export class WorkspaceComponent implements AfterViewInit,
OnInit, OnDestroy {
ngOnDestroy() {
this.persistBeforeLeaving();
this.codeEditorViewRef.clear();
- this.workflowActionService.clearWorkflow();
// Tear down the connection and all websocket-derived session state so a
- // re-entered workflow starts clean instead of reusing the previous one.
- this.computingUnitStatusService.disconnect();
- this.resetWorkflowSessionState();
+ // re-entered workflow starts clean instead of reusing the previous one --
unless the Form
Review Comment:
The switch is no longer a full page load, but eight comments across three
files still say it is, including the two that explain why the `beforeunload`
handlers tear nothing down.
Those two are the ones that matter:
- `workspace.component.ts:194-195` — "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"
- `workflow-form.component.ts:2025` — "The canvas switch is a full-page
navigation, and the browser may keep this document in its back/forward cache"
Both name the view switch as the full-page navigation that fires
`beforeunload`. After this PR it does not fire it at all. The handlers are
still right — a refresh, a closed tab or a typed URL still unloads the
document, which is what #8599 was about — but someone auditing them later would
read the stated case, find it impossible, and have a reason to delete them.
The other six are the save-ordering rationale, whose premise is gone: a
routed switch aborts no request.
- `menu.component.ts:135` — "saving, then a full-page load"
- `menu.component.ts:730-731` — "The full-page load that follows unloads
this document, and a request still in flight at that moment is aborted"
- `menu.component.ts:741` — "rather than letting the full-page load abort
that edit's own debounced autosave"
- `menu.component.ts:770` — "the full-page load would kill its autosave"
- `workflow-form.component.ts:244` — "would lose it to the full-page load"
- `workflow-form.component.ts:1888` — "the full-page load would kill that
edit"
Saving before handing over is still worth doing, so it is the reason rather
than the behaviour that needs updating.
--
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]
