mengw15 commented on code in PR #8581:
URL: https://github.com/apache/texera/pull/8581#discussion_r4066677281
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -629,6 +629,42 @@ export class WorkflowActionService {
this.texeraGraph.destroyYModel();
}
+ /**
+ * Whether this page already holds `workflowId` open: the shared document is
in that workflow's
+ * co-editing room, so the graph, the undo history and the room membership
are the live ones.
+ *
+ * The operator canvas and the Form View are two views of one open workflow
and hand the session
+ * over between them rather than each building its own. The arriving view
asks this before
+ * loading: seeding a second document for the same workflow would leave the
room and rejoin it,
+ * which is what used to leave a ghost of yourself in the co-editor list.
+ *
+ * A workflow that was open and has since been left does not answer true
here, and the reason is
+ * worth stating because it is not local: `destroyYModel` destroys the
document but keeps the
+ * object, `wid` and all. What clears it is `clearWorkflow` going on to
`reloadWorkflow(undefined)`,
+ * which seeds a fresh model with no `wid`. Were that to stop happening, a
canvas re-entered from
+ * the dashboard would attach to a destroyed document instead of loading.
Pinned by a test.
+ */
+ public hasWorkflowOpen(workflowId: number): boolean {
+ return this.texeraGraph.sharedModel.wid === workflowId;
Review Comment:
Confirmed, and it is three clicks from the canvas rather than a corner case:
"create new" in the menu bar (`menu.component.html:122`) calls
`resetAsNewWorkflow()`, which seeds a shared model with no `wid`, so the room
falls back to a uuid (`shared-model.ts:82`). `location.go("/")` does not route,
so the canvas stays mounted; the first autosave then creates the workflow and
gives the metadata the assigned id while the document stays in the uuid room.
The two sides ask different questions about the same thing.
`isLeavingWorkspace` is keyed on `getWorkflowMetadata().wid`, and its own doc
says why: "a workflow created by the first autosave has no id in the route it
was opened with". `hasWorkflowOpen` is keyed on `sharedModel.wid`. So the
departing canvas reads this as a hand-over and skips `clearWorkflow()`, while
the arriving Form View declines it and reloads. The one case the departing side
was widened to cover is the one case the arriving side rejects.
What is lost is narrower than the session, though. The save precedes the
navigation, so the content is safe, and the Form View's own
`resetAsNewWorkflow()` destroys the uuid document, so nothing leaks. What goes
is the undo history, and the switch quietly falls back to a full rebuild for
that workflow.
One caution on the first of the two suggested fixes. Recognising the
assigned metadata id would make `hasWorkflowOpen(42)` true while the document
is still in the uuid room, so the Form View would go on co-editing in a private
room under workflow 42's name: a second person opening workflow 42 joins room
"42", the two never see each other, and nothing shows that anything is wrong.
Moving the document to the assigned room on the first save is the sound version
of that, at the cost of a rejoin.
The smaller option is to have the departing side ask the arriving side's
question: skip the teardown only when `hasWorkflowOpen(...)` also holds. A
workflow created in this session then loses the optimisation on its first
switch and is torn down and rebuilt, which is what happens today anyway.
Unrelated, while this file is open: the doc on `onClickOpenFormView`
(`menu.component.ts:688-692`) still describes the switch as "a full page load,
not a route ... A fresh document is the clean handover", which
`openFormViewPage` below it contradicts.
--
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]
