Copilot commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r4117908568
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -797,25 +861,117 @@ export class WorkflowActionService {
}
public setWorkflowSettings(workflowSettings: WorkflowSettings | undefined):
void {
- if (this.workflowSettings === workflowSettings) {
+ const newSettings = workflowSettings === undefined ?
this.getDefaultSettings() : workflowSettings;
+ // Skip a redundant write: setting the same value would still cut a Yjs
update and observer
+ // churn for every collaborator.
+ if (isEqual(this.getWorkflowSettings(), newSettings)) {
return;
}
-
- const newSettings = workflowSettings === undefined ?
this.getDefaultSettings() : workflowSettings;
- this.workflowSettings = newSettings;
+ this.texeraGraph.sharedModel.contentMetaMap.set("settings", newSettings);
}
public getWorkflowSettings(): WorkflowSettings {
- return this.workflowSettings;
+ return (this.readContentMeta("settings") as WorkflowSettings) ??
this.getDefaultSettings();
}
/**
- * Load a definition without announcing an edit. Used while opening a
workflow, so
- * that merely reading one does not look like a change and trigger a save.
+ * Seed workflowSettings into the shared model while opening a workflow (see
seedContentMeta).
+ * Called under the reloading flag, so the seed is not announced as an edit.
+ */
+ public hydrateSettings(workflowSettings: WorkflowSettings | undefined): void
{
+ this.seedContentMeta("settings", workflowSettings);
+ }
+
+ /**
+ * Load a definition into the shared model while opening a workflow (see
seedContentMeta).
+ * Called under the reloading flag, so the shared-map observer skips this
seed and opening a
+ * workflow is not announced as an edit.
*/
public hydrateFormBinding(formBinding: FormBindingConfig | undefined): void {
- this.formBindingLoaded = formBinding !== undefined;
- this.formBinding = formBinding ?? getDefaultFormBinding();
+ this.seedContentMeta("formBinding", formBinding);
+ }
+
+ /**
+ * Put the database's copy of a content-meta value into the shared model as
a workflow opens,
+ * without taking a co-editor's newer one away.
+ *
+ * The document is created and connected just before this runs, so the
provider has usually not
+ * synced yet. Writing straight away is a concurrent whole-value write, and
Yjs resolves those
+ * by client id rather than by recency: the copy the database handed us
could win over an edit a
+ * co-editor has not saved yet -- the lost update this all exists to
prevent. So wait for the
+ * first sync, then write only when the key is still absent; a value that
came from the room is
+ * authoritative, whether it is a co-editor's edit or our own from another
tab.
+ *
+ * Meanwhile the value is not lost: reads fall back to the copy held here,
so the page shows it
+ * and an autosave that fires before the sync carries it rather than an
empty one. The wait is
+ * not bounded: a copy published before the sync would be the concurrent
write above, on a room
+ * that is slow to answer as much as on one that never does. For as long as
the room stays out
+ * of reach the copy is read from here and every save carries it, and it
goes into the document
+ * when the sync eventually comes -- or never, which loses nothing, since
nobody else was in that
+ * document either.
+ *
+ * A local edit of the key while the seed waits cancels it (see
observeContentMeta): the document
+ * then carries the author's value, which the seed must neither write over
nor delete.
+ *
+ * `undefined` clears the key, so a workflow that carries no settings or no
form definition does
+ * not inherit the one the document already held (reloading a version into
the open document);
+ * a value the room put there is left alone here too.
+ */
+ private seedContentMeta(key: ContentMetaKey, value: ContentMetaValue |
undefined): void {
+ // This seed replaces whatever the previously open workflow left waiting
for the key. It has an
+ // identity of its own, since a cleared key (undefined) has no value to be
told apart by.
+ const seed = {};
+ this.contentMetaSeeds.set(key, seed);
+ this.pendingContentMeta.delete(key);
+ if (value !== undefined) {
+ this.pendingContentMeta.set(key, value);
+ }
+ const shared = this.texeraGraph.sharedModel;
+ const land = () => {
+ // Not ours to write any more: the workflow was closed or swapped while
we waited (the seed
+ // or the document was replaced), or the author edited the key meanwhile
(the seed was
+ // cancelled).
+ if (this.texeraGraph.sharedModel !== shared ||
this.contentMetaSeeds.get(key) !== seed) {
+ return;
+ }
+ this.cancelContentMetaSeed(key);
+ // The room's value wins, whether it is a co-editor's edit or this
client's from another tab.
+ if (this.roomOwnedContentMeta.has(key)) {
+ return;
Review Comment:
`roomOwnedContentMeta` remains set for the lifetime of the document, so this
also blocks intentional later reloads. For example, a client that joined an
existing room marks these keys as room-owned;
`WorkflowVersionService.displayReadonlyWorkflow()` then disconnects and calls
`reloadWorkflow()`, but both seeds return here, leaving the preview (and a
subsequent revert) with the current settings/form binding while the graph shows
the selected version. Agent-driven `reloadWorkflow()` has the same
inconsistency. Scope room precedence to initial-open hydration and allow
explicit later reloads to replace/delete these values, with a regression test
for a reload after receiving room metadata.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -797,25 +861,117 @@ export class WorkflowActionService {
}
public setWorkflowSettings(workflowSettings: WorkflowSettings | undefined):
void {
- if (this.workflowSettings === workflowSettings) {
+ const newSettings = workflowSettings === undefined ?
this.getDefaultSettings() : workflowSettings;
+ // Skip a redundant write: setting the same value would still cut a Yjs
update and observer
+ // churn for every collaborator.
+ if (isEqual(this.getWorkflowSettings(), newSettings)) {
return;
}
-
- const newSettings = workflowSettings === undefined ?
this.getDefaultSettings() : workflowSettings;
- this.workflowSettings = newSettings;
+ this.texeraGraph.sharedModel.contentMetaMap.set("settings", newSettings);
}
public getWorkflowSettings(): WorkflowSettings {
- return this.workflowSettings;
+ return (this.readContentMeta("settings") as WorkflowSettings) ??
this.getDefaultSettings();
}
/**
- * Load a definition without announcing an edit. Used while opening a
workflow, so
- * that merely reading one does not look like a change and trigger a save.
+ * Seed workflowSettings into the shared model while opening a workflow (see
seedContentMeta).
+ * Called under the reloading flag, so the seed is not announced as an edit.
+ */
+ public hydrateSettings(workflowSettings: WorkflowSettings | undefined): void
{
+ this.seedContentMeta("settings", workflowSettings);
+ }
+
+ /**
+ * Load a definition into the shared model while opening a workflow (see
seedContentMeta).
+ * Called under the reloading flag, so the shared-map observer skips this
seed and opening a
+ * workflow is not announced as an edit.
*/
public hydrateFormBinding(formBinding: FormBindingConfig | undefined): void {
- this.formBindingLoaded = formBinding !== undefined;
- this.formBinding = formBinding ?? getDefaultFormBinding();
+ this.seedContentMeta("formBinding", formBinding);
+ }
+
+ /**
+ * Put the database's copy of a content-meta value into the shared model as
a workflow opens,
+ * without taking a co-editor's newer one away.
+ *
+ * The document is created and connected just before this runs, so the
provider has usually not
+ * synced yet. Writing straight away is a concurrent whole-value write, and
Yjs resolves those
+ * by client id rather than by recency: the copy the database handed us
could win over an edit a
+ * co-editor has not saved yet -- the lost update this all exists to
prevent. So wait for the
+ * first sync, then write only when the key is still absent; a value that
came from the room is
+ * authoritative, whether it is a co-editor's edit or our own from another
tab.
+ *
+ * Meanwhile the value is not lost: reads fall back to the copy held here,
so the page shows it
+ * and an autosave that fires before the sync carries it rather than an
empty one. The wait is
+ * not bounded: a copy published before the sync would be the concurrent
write above, on a room
+ * that is slow to answer as much as on one that never does. For as long as
the room stays out
+ * of reach the copy is read from here and every save carries it, and it
goes into the document
+ * when the sync eventually comes -- or never, which loses nothing, since
nobody else was in that
+ * document either.
+ *
+ * A local edit of the key while the seed waits cancels it (see
observeContentMeta): the document
+ * then carries the author's value, which the seed must neither write over
nor delete.
+ *
+ * `undefined` clears the key, so a workflow that carries no settings or no
form definition does
+ * not inherit the one the document already held (reloading a version into
the open document);
+ * a value the room put there is left alone here too.
+ */
+ private seedContentMeta(key: ContentMetaKey, value: ContentMetaValue |
undefined): void {
+ // This seed replaces whatever the previously open workflow left waiting
for the key. It has an
+ // identity of its own, since a cleared key (undefined) has no value to be
told apart by.
+ const seed = {};
+ this.contentMetaSeeds.set(key, seed);
+ this.pendingContentMeta.delete(key);
+ if (value !== undefined) {
+ this.pendingContentMeta.set(key, value);
+ }
+ const shared = this.texeraGraph.sharedModel;
+ const land = () => {
+ // Not ours to write any more: the workflow was closed or swapped while
we waited (the seed
+ // or the document was replaced), or the author edited the key meanwhile
(the seed was
+ // cancelled).
+ if (this.texeraGraph.sharedModel !== shared ||
this.contentMetaSeeds.get(key) !== seed) {
+ return;
+ }
+ this.cancelContentMetaSeed(key);
+ // The room's value wins, whether it is a co-editor's edit or this
client's from another tab.
+ if (this.roomOwnedContentMeta.has(key)) {
+ return;
+ }
+ // Opening a workflow is not an edit, so the seed is not announced (see
observeContentMeta).
+ this.seedingContentMetaKeys.add(key);
+ try {
+ if (value === undefined) {
+ shared.contentMetaMap.delete(key);
+ } else if (!shared.contentMetaMap.has(key)) {
+ shared.contentMetaMap.set(key, value);
+ }
+ } finally {
+ this.seedingContentMetaKeys.delete(key);
+ }
+ };
+ if (!shared.wsProvider.shouldConnect || shared.wsProvider.synced) {
+ land();
+ return;
+ }
+ shared.wsProvider.once("sync", land);
Review Comment:
Each superseding seed registers another `once("sync", land)` listener, but
cancellation only invalidates its token and never unregisters the old callback.
Because this PR deliberately permits an unbounded wait, repeated agent/version
reloads while the websocket is unreachable accumulate two listeners per reload
indefinitely. Track the pending callback per key and remove it when a seed is
superseded, cancelled, or its document is replaced.
--
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]
