Copilot commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r4117011290
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -797,25 +854,99 @@ 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();
+ }
+
+ /**
+ * 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 without announcing an edit. Used while opening a
workflow, so
- * that merely reading one does not look like a change and trigger a save.
+ * 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
+ * also bounded, so an unreachable y-websocket still ends up with the value
in the document.
+ *
+ * `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 {
+ // Whatever the previously open workflow left waiting is not this
workflow's.
+ this.pendingContentMeta.delete(key);
+ const shared = this.texeraGraph.sharedModel;
+ if (value !== undefined) {
+ this.pendingContentMeta.set(key, value);
+ }
+ const seed = () => {
+ // The workflow may have been closed or swapped while we waited; that
document's seed, and
+ // that workflow's value, are no longer ours to write.
+ if (this.texeraGraph.sharedModel !== shared ||
this.pendingContentMeta.get(key) !== value) {
Review Comment:
An absent database value is not represented in `pendingContentMeta`, so this
guard cannot tell a still-pending `undefined` seed from one superseded by a
user edit. For a workflow without `formBinding`,
`hydrateFormBinding(undefined)`, followed by `setFormBinding(...)` before first
sync, reaches this callback and deletes the user's newly written binding at
line 928; the seeding flag also suppresses notification of that deletion. Track
seed identity/presence even for `undefined` and cancel that seed when a real
local edit occurs.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -136,12 +151,53 @@ export class WorkflowActionService {
);
this.sharedModelChangeHandler.setConfigService(this.config);
this.workflowMetadata = DEFAULT_WORKFLOW;
- this.workflowSettings = this.getDefaultSettings();
this.undoRedoService.setUndoManager(this.texeraGraph.sharedModel.undoManager);
+ // Watch the shared content map, re-attaching whenever the shared model is
recreated
+ // (opening another workflow), the same way SharedModelChangeHandler
re-attaches its
+ // graph observers. A formBinding change from a local edit or a co-editor
is republished
+ // on formBindingChanged$ so the Form View re-renders and the existing
autosave picks it
+ // up. The reload seed is skipped -- like the graph seed -- so opening a
workflow is not
+ // announced as an edit and does not save on every open.
+ this.observeContentMeta();
+ this.texeraGraph.newYDocLoadedSubject.subscribe(() =>
this.observeContentMeta());
+
this.handleJointElementDrag();
}
+ private observeContentMeta(): void {
+ // Detach the observer from the previous shared model before attaching to
the new one, so
+ // re-attaching on each opened workflow does not stack listeners on the
same map.
+ this.observedContentMetaMap?.unobserve(this.contentMetaObserver!);
+ // A new document is a new room: nothing in it belongs to the previous
one's co-editors.
+ this.roomOwnedContentMeta.clear();
+ const contentMetaMap = this.texeraGraph.sharedModel.contentMetaMap;
+ this.contentMetaObserver = event => {
+ // A value that arrived from the room belongs to the room: a seed must
not overwrite or
+ // delete it (see seedContentMeta). The first sync delivers a
co-editor's state as a remote
+ // transaction, which is how an opening client learns the room already
holds one.
+ if (!event.transaction.local) {
+ for (const key of event.changes.keys.keys()) {
+ this.roomOwnedContentMeta.add(key as ContentMetaKey);
+ }
+ }
+ if (this.jointGraphWrapper.getReloadingWorkflow()) {
+ return;
+ }
+ if (event.changes.keys.has("formBinding") &&
!this.seedingContentMetaKeys.has("formBinding")) {
Review Comment:
This adds visible collaborative refresh behavior for the Form View/settings
panel, but the PR's testing section provides neither the required side-by-side
before/after screenshot or GIF nor manual interaction steps (click path,
browser, and viewport). Please add that verification evidence for this
interactive frontend flow.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -797,25 +854,99 @@ 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();
+ }
+
+ /**
+ * 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 without announcing an edit. Used while opening a
workflow, so
- * that merely reading one does not look like a change and trigger a save.
+ * 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
+ * also bounded, so an unreachable y-websocket still ends up with the value
in the document.
+ *
+ * `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 {
+ // Whatever the previously open workflow left waiting is not this
workflow's.
+ this.pendingContentMeta.delete(key);
+ const shared = this.texeraGraph.sharedModel;
+ if (value !== undefined) {
+ this.pendingContentMeta.set(key, value);
+ }
+ const seed = () => {
+ // The workflow may have been closed or swapped while we waited; that
document's seed, and
+ // that workflow's value, are no longer ours to write.
+ if (this.texeraGraph.sharedModel !== shared ||
this.pendingContentMeta.get(key) !== value) {
+ return;
+ }
+ this.pendingContentMeta.delete(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) {
+ seed();
+ return;
+ }
+ const fallback = setTimeout(seed, CONTENT_META_SEED_TIMEOUT_MS);
Review Comment:
A fixed 5-second fallback can expire on a slow but reachable websocket,
writing the stale database value before the room's initial state arrives. When
synchronization later completes, this write is concurrent with the co-editor's
value and Yjs can select the stale value by client ID—the lost-update scenario
this seed delay is meant to prevent. Keep using the pending database copy for
reads while disconnected, but do not publish it into the shared document until
a successful initial sync (or a definitive offline mode).
--
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]
