You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@texera.apache.org by "yangzhang75 (via GitHub)" <gi...@apache.org> on 2026/09/02 20:29:59 UTC
[PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
yangzhang75 opened a new pull request, #8351:
URL: https://github.com/apache/texera/pull/8351
### What changes were proposed in this PR?
`workflowSettings` and the Form View definition (`formBinding`) were held as per-client private fields on `WorkflowActionService`, outside the Yjs shared document. Because autosave persists the whole workflow content, a co-editor whose private copy was stale would overwrite another editor's change on their next canvas edit -- a lost update (#8315).
This moves both into the shared model:
* A shared `contentMetaMap` (`Y.Map`, keyed `"settings"` / `"formBinding"`) is added to `SharedModel`, alongside the graph maps, so it syncs live and every autosave writes the current value rather than a stale copy. It is deliberately kept out of the undo manager, matching the previous (non-undoable) behavior of these values.
* `getWorkflowSettings` / `setWorkflowSettings` / `getFormBinding` / `setFormBinding` / `hydrateFormBinding` / `getWorkflowContent` now read and write the shared map.
* An observer republishes a `formBinding` change -- from a local edit or a co-editor -- on `formBindingChanged$`, so the Form View re-renders and the existing autosave picks it up. The observer skips the reload seed (gated on the reloading flag), so opening a workflow is not announced as an edit, the same way the graph seed is handled.
Whole-value last-writer-wins is accepted for now; field-level Y-types are a possible future refinement.
### Any related issues, documentation, discussions?
Closes #8315.
### How was this PR tested?
Unit tests (vitest): `workflow-action.service.spec` and `shared-model.spec` pass (79 tests). Rewrote the "opening a workflow stays silent" test to drive the real reload path, and added a test that a co-editor's write to the shared map reaches `formBindingChanged$`. 100% coverage of the lines this PR adds.
### Was this PR authored or co-authored using generative AI tooling?
Yes. Co-authored with Claude (Anthropic), reviewed line by line by the author before submission.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "yangzhang75 (via GitHub)" <gi...@apache.org>.
yangzhang75 commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r4030024783
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -747,25 +770,49 @@ 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.texeraGraph.sharedModel.contentMetaMap.get("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, mirroring
+ * hydrateFormBinding: `undefined` deletes the key rather than writing defaults, so opening a
+ * workflow that never saved settings does not overwrite a co-editor's live ones (getWorkflow-
+ * Settings defaults on an absent key). Called under the reloading flag.
+ */
+ public hydrateSettings(workflowSettings: WorkflowSettings | undefined): void {
+ const contentMeta = this.texeraGraph.sharedModel.contentMetaMap;
+ if (workflowSettings === undefined) {
+ contentMeta.delete("settings");
+ } else {
+ contentMeta.set("settings", workflowSettings);
Review Comment:
Fixed. The seed now waits for the provider's first sync (immediate when the document is offline or already synced) and writes only when the key is still absent, and a value that arrived from the room is left alone -- neither overwritten nor deleted -- which also covers opening a workflow that carries none over a co-editor who has one. Meanwhile the database copy is held and read from, so the page shows it and an autosave in that window carries it instead of an empty one; the wait is bounded (5s) so an unreachable y-websocket still ends up with the value. The deferred write is not announced as an edit. Tests: a co-editor's value survives both a seed and an empty open, reads fall back while waiting, the seed lands after the timeout, and the deferred seed stays silent; each is deletion-checked.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -136,12 +137,34 @@ 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!);
+ const contentMetaMap = this.texeraGraph.sharedModel.contentMetaMap;
+ this.contentMetaObserver = event => {
+ if (event.changes.keys.has("formBinding") && !this.jointGraphWrapper.getReloadingWorkflow()) {
+ this.formBindingChangeSubject.next(this.getFormBinding());
+ }
Review Comment:
Fixed. The observer now publishes settings changes too, on a new `workflowSettingsChanged$` that is merged into `workflowChanged()`, so the settings panel (which refreshes from that stream) picks up a co-editor's change and this client's autosave carries it. Covered by a test that writes to the shared map and asserts both streams fire.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r4119334811
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -136,12 +157,70 @@ 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 starts with no seed state: nothing in it came from the previous room, no key
+ // has been seeded on it yet, and whatever copy the previously open workflow left waiting is
+ // not this one's -- dropped here, so it cannot leak into the next save of a workflow opened
+ // (or a blank one started) on this document.
+ for (const key of [...this.contentMetaSeeds.keys()]) {
+ this.cancelContentMetaSeed(key); // drops the copy it holds, and stops its listening
+ }
+ this.roomOwnedContentMeta.clear();
+ this.seededContentMetaKeys.clear();
+ const contentMetaMap = this.texeraGraph.sharedModel.contentMetaMap;
+ this.contentMetaObserver = event => {
+ if (!event.transaction.local) {
+ // An entry that arrived from the room belongs to the room, a co-editor's value or its
+ // cleared mark alike: the seed a workflow is opened with yields to it (see seedContentMeta).
+ // The first sync delivers the room's state as a remote transaction, which is how an
+ // opening client learns what the room already holds.
+ for (const key of event.changes.keys.keys()) {
+ this.roomOwnedContentMeta.add(key as ContentMetaKey);
+ }
+ } else {
+ // A local edit of a key supersedes the database copy a seed is still holding for it: the
+ // document now carries what the author wants, and the seed must neither write over it nor,
+ // for a workflow opened without a value, clear it once the sync lands.
+ for (const key of event.changes.keys.keys()) {
+ if (!this.seedingContentMetaKeys.has(key as ContentMetaKey)) {
+ this.cancelContentMetaSeed(key as ContentMetaKey);
+ }
Review Comment:
A local edit while the initial sync is pending is still a concurrent Yjs write; cancelling the database seed does not make that edit win. If the room already has this key, its first update and the user's `setFormBinding`/`setWorkflowSettings` can be resolved by client ID, so an edit made immediately after opening can be silently lost (or can overwrite unseen room state). Prevent content-meta editing until the first sync, or queue field-level user intent and apply it after synchronization instead of writing the whole stale value immediately.
This issue also appears on line 959 of the same file.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "yangzhang75 (via GitHub)" <gi...@apache.org>.
yangzhang75 commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r4119296411
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -797,25 +876,125 @@ 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.
+ *
+ * The room's precedence is for the first seed of a key on a document, the one a workflow is
+ * opened with. A later reload into the same document -- a version shown, or returned from; an
+ * agent's rewrite -- is the intent, and its seed replaces whatever the document holds, a value
+ * the room put there included; `undefined` then clears the key, so a version that carries no
+ * settings or no form definition does not keep the open workflow's.
+ */
+ private seedContentMeta(key: ContentMetaKey, value: ContentMetaValue | undefined): void {
+ // This seed replaces any the key was still waiting on (and stops its listening); it has an
+ // identity of its own, since a cleared key (undefined) has no value to be told apart by.
+ this.cancelContentMetaSeed(key);
+ const seed: ContentMetaSeed = {};
+ this.contentMetaSeeds.set(key, seed);
+ if (value !== undefined) {
+ this.pendingContentMeta.set(key, value);
+ }
+ const shared = this.texeraGraph.sharedModel;
+ // Runs at once or when the sync comes, and then for this seed only: a later seed of the key, a
+ // local edit of it and the loading of another document all detach the listening before they
+ // would make this seed stale (see cancelContentMetaSeed, observeContentMeta).
+ const land = () => {
+ this.cancelContentMetaSeed(key);
+ const first = !this.seededContentMetaKeys.has(key);
+ this.seededContentMetaKeys.add(key);
+ // Over the first seed the room's value wins, whether it is a co-editor's edit or this
+ // client's from another tab; a later reload replaces it.
+ if (first && this.roomOwnedContentMeta.has(key)) {
+ return;
Review Comment:
Right: a deletion left nothing for a joining client to see. Absence is explicit now, in 5ae4c9308: a workflow opened or reloaded without a value marks the key cleared (`null`) rather than deleting it, and the first seed of a key on a document yields to whatever the document holds by then, a value or that mark. With the mark carrying the room's word, the separate room-ownership bookkeeping went away (`roomOwnedContentMeta`): `has` on the map is the whole test. Reads treat the mark as none, so `getFormBinding()` / `getWorkflowSettings()` fall back to the defaults and the content omits the key. Spec: a co-editor's cleared mark keeps a database copy that still has the definition out of the document; a workflow opened without settings into a fresh room writes the mark.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "yangzhang75 (via GitHub)" <gi...@apache.org>.
yangzhang75 commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r4127219671
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -136,12 +158,61 @@ 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 starts with no seed state: nothing in it came from the previous room, no key
+ // has been seeded on it yet, and whatever copy the previously open workflow left waiting is
+ // not this one's -- dropped here, so it cannot leak into the next save of a workflow opened
+ // (or a blank one started) on this document.
+ for (const key of [...this.contentMetaSeeds.keys()]) {
+ this.cancelContentMetaSeed(key); // drops the copy it holds, and stops its listening
+ }
+ this.roomOwnedContentMeta.clear();
+ this.seededContentMetaKeys.clear();
+ const contentMetaMap = this.texeraGraph.sharedModel.contentMetaMap;
+ this.contentMetaObserver = event => {
+ if (!event.transaction.local) {
+ // An entry that arrived from the room belongs to the room, a co-editor's value or its
+ // cleared mark alike: the seed a workflow is opened with yields to it (see seedContentMeta).
+ // The first sync delivers the room's state as a remote transaction, which is how an
+ // opening client learns what the room already holds.
+ 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")) {
+ this.formBindingChangeSubject.next(this.getFormBinding());
+ }
Review Comment:
Right, the room's value arriving re-announced the held edit. Fixed in 7255e791d: a change to a key is announced only when it is neither a seed's own write nor made while an edit held for the first sync owns the key (`announces`); that edit was announced when it was made, reads answer it, and its landing is a seed's write. Spec: the held edit is announced exactly once across the room's value arriving and the landing.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -774,6 +845,7 @@ export class WorkflowActionService {
this.getTexeraGraph().getPortDisplayNameChangedSubject(),
this.getTexeraGraph().getPortPropertyChangedStream(),
this.formBindingChanged$,
+ this.workflowSettingsChanged$,
Review Comment:
Agreed, and taken the other way round in 7255e791d: a settings change stays on `workflowSettingsChanged$` and is no longer merged into `workflowChanged()`, so the panel's own persist remains the single save path for settings, with its error notification, and the autosave does not cut a second version. The settings panel refreshes from `workflowSettingsChanged$` alongside `workflowChanged()`, so a co-editor's change still reaches it live; any later autosave carries the current settings anyway, since the content reads them from the shared map. (Removing the panel's persist instead would have left a failed settings save silent, as the autosave has no error path.) Spec: a shared-map settings change is announced on `workflowSettingsChanged$` and not on `workflowChanged()`; the panel re-syncs from the settings stream without persisting.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r3936781503
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -667,7 +678,7 @@ export class WorkflowActionService {
}
const workflowContent: WorkflowContent = workflow.content;
- this.workflowSettings = workflowContent.settings || this.getDefaultSettings();
+ this.setWorkflowSettings(workflowContent.settings);
Review Comment:
`hydrateFormBinding` maps absent → delete, so opening a workflow whose DB copy carries no binding writes no op and can't disturb a co-editor's live one. The settings seed doesn't get the same treatment: absent → `set(defaults)` writes a real entry, concurrent with whatever a co-editor holds, so opening a workflow that never saved settings while another editor has just tuned theirs can revert them — the same open-time clobber this PR removes for formBinding. Worth mirroring the delete here; `getWorkflowSettings` already defaults on an absent key.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r3997455685
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -747,25 +770,49 @@ 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.texeraGraph.sharedModel.contentMetaMap.get("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, mirroring
+ * hydrateFormBinding: `undefined` deletes the key rather than writing defaults, so opening a
+ * workflow that never saved settings does not overwrite a co-editor's live ones (getWorkflow-
+ * Settings defaults on an absent key). Called under the reloading flag.
+ */
+ public hydrateSettings(workflowSettings: WorkflowSettings | undefined): void {
+ const contentMeta = this.texeraGraph.sharedModel.contentMetaMap;
+ if (workflowSettings === undefined) {
+ contentMeta.delete("settings");
+ } else {
+ contentMeta.set("settings", workflowSettings);
Review Comment:
This seed is written immediately after `setNewSharedModel()` creates and connects a fresh Yjs document, before the provider's initial sync can complete. If the fetched workflow is stale relative to an active collaborator, this creates a concurrent whole-value write and Yjs may select and broadcast the stale settings—the lost update this PR is intended to prevent. Wait for initial provider sync and seed only when the shared key is still absent, or otherwise make the synchronized shared value authoritative.
This issue also appears on line 814 of the same file.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -136,12 +137,34 @@ 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!);
+ const contentMetaMap = this.texeraGraph.sharedModel.contentMetaMap;
+ this.contentMetaObserver = event => {
+ if (event.changes.keys.has("formBinding") && !this.jointGraphWrapper.getReloadingWorkflow()) {
+ this.formBindingChangeSubject.next(this.getFormBinding());
+ }
Review Comment:
The observer ignores `settings` changes. A co-editor's update reaches this map, but `SettingsComponent` only refreshes its form from `workflowChanged()`, and that stream currently receives only `formBindingChanged$` for content metadata. The settings panel therefore continues displaying stale values until an unrelated workflow event occurs. Please publish settings-map changes through a settings event consumed by the panel (or merged into `workflowChanged()`) and cover a direct/remote map update in the test.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "yangzhang75 (via GitHub)" <gi...@apache.org>.
yangzhang75 commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r4126555727
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -136,12 +157,70 @@ 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 starts with no seed state: nothing in it came from the previous room, no key
+ // has been seeded on it yet, and whatever copy the previously open workflow left waiting is
+ // not this one's -- dropped here, so it cannot leak into the next save of a workflow opened
+ // (or a blank one started) on this document.
+ for (const key of [...this.contentMetaSeeds.keys()]) {
+ this.cancelContentMetaSeed(key); // drops the copy it holds, and stops its listening
+ }
+ this.roomOwnedContentMeta.clear();
+ this.seededContentMetaKeys.clear();
+ const contentMetaMap = this.texeraGraph.sharedModel.contentMetaMap;
+ this.contentMetaObserver = event => {
+ if (!event.transaction.local) {
+ // An entry that arrived from the room belongs to the room, a co-editor's value or its
+ // cleared mark alike: the seed a workflow is opened with yields to it (see seedContentMeta).
+ // The first sync delivers the room's state as a remote transaction, which is how an
+ // opening client learns what the room already holds.
+ for (const key of event.changes.keys.keys()) {
+ this.roomOwnedContentMeta.add(key as ContentMetaKey);
+ }
+ } else {
+ // A local edit of a key supersedes the database copy a seed is still holding for it: the
+ // document now carries what the author wants, and the seed must neither write over it nor,
+ // for a workflow opened without a value, clear it once the sync lands.
+ for (const key of event.changes.keys.keys()) {
+ if (!this.seedingContentMetaKeys.has(key as ContentMetaKey)) {
+ this.cancelContentMetaSeed(key as ContentMetaKey);
+ }
Review Comment:
Agreed for these two single-key values (the graph's per-operator keys do not race this way). Done in 1a5d528c2, the second of the two options: an edit made before the first sync is held the way the database copy is (`seedContentMeta` with `edit`), read from and announced at once so the page shows it and an autosave carries it, and written after the sync as a write that follows the room's state and replaces it -- an edit, not a coin flip -- so it yields to nothing. `setFormBinding` / `setWorkflowSettings` go through `writeContentMeta`, which writes straight into the document once it has synced. Spec: an edit typed right after opening is not in the document before the sync, is what the page shows while a co-editor's value arrives, and is what the document holds after the sync; the settings edit the same.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "codecov-commenter (via GitHub)" <gi...@apache.org>.
codecov-commenter commented on PR #8351:
URL: https://github.com/apache/texera/pull/8351#issuecomment-5515988898
### :warning: JUnit XML file not found
The CLI was unable to find any JUnit XML files to upload.
For more help, visit our [troubleshooting guide](https://docs.codecov.com/docs/test-analytics#troubleshooting).
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r4117027006
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -718,7 +774,7 @@ export class WorkflowActionService {
}
const workflowContent: WorkflowContent = workflow.content;
- this.workflowSettings = workflowContent.settings || this.getDefaultSettings();
+ this.hydrateSettings(workflowContent.settings);
Review Comment:
The blank path does not get this treatment: `reloadWorkflow(undefined)` calls `hydrateFormBinding(undefined)` -- with the comment that a leftover must not leak into the next save -- but nothing calls `hydrateSettings(undefined)`. Settings go through `clearWorkflow`'s `setWorkflowSettings(undefined)` instead, which lands somewhere else:
```
clearWorkflow():
destroySharedModel(); // doc destroyed; the contentMeta observer is still on its map
setWorkflowMetadata(undefined);
setWorkflowSettings(undefined); // isEqual reads the destroyed map, and writes when it differs
reloadWorkflow(undefined); // setReloadingWorkflow(true) only happens here
```
So for a workflow whose settings are not the defaults, this writes `set("settings", defaults)` into the just-destroyed document, and because the reloading flag is not set yet the observer announces it on `workflowSettingsChanged$` -- `clearWorkflow` reports a workflow edit where it reported none before. Calling `hydrateSettings(undefined)` in the blank branch beside `hydrateFormBinding(undefined)` would put the settings seed on the same path as the form one, under the flag.
(The pending-seed half of this is @copilot's point above on the `undefined` seed having no identity; not repeating it here.)
~~~
~~~comment
`has` reads only the shared map, while the value beside it comes from `readContentMeta`, which falls back to the copy a seed is still holding. So during the wait for the first sync the two disagree. For a workflow whose stored binding is present but empty (`{fields: []}` -- an author who added fields and then removed them all), `has` is false and `isFormBindingNonEmpty` is false, so an autosave in that window writes content with no `formBinding` at all and drops the stored one. The `formBindingLoaded` flag this replaces was true for that case.
The consequence is confined to content fidelity -- `getFormBinding()` answers `{fields: []}` either way -- but it is the one thing "an autosave in that window carries it instead of an empty one" does not cover. A `hasContentMeta(key)` mirroring `readContentMeta` would close it.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -865,11 +1002,12 @@ export class WorkflowActionService {
links,
commentBoxes,
settings,
- // Carry formBinding only when the workflow has one (loaded with it, or an author
- // populated it), so a plain workflow's content is unchanged and its save cuts no
- // needless version.
- ...(this.formBindingLoaded || this.isFormBindingNonEmpty(this.formBinding)
- ? { formBinding: this.formBinding }
+ // Carry formBinding only when the workflow has one (opened with it, or an author
+ // populated it since), so a plain workflow's content is unchanged and its save cuts no
+ // needless version. `has` stands in for the old "loaded" flag: hydrate sets the key for
+ // a workflow opened with a binding and deletes it for one without.
+ ...(this.texeraGraph.sharedModel.contentMetaMap.has("formBinding") || this.isFormBindingNonEmpty(formBinding)
Review Comment:
`has` reads only the shared map, while the value beside it comes from `readContentMeta`, which falls back to the copy a seed is still holding. So during the wait for the first sync the two disagree. For a workflow whose stored binding is present but empty (`{fields: []}` -- an author who added fields and then removed them all), `has` is false and `isFormBindingNonEmpty` is false, so an autosave in that window writes content with no `formBinding` at all and drops the stored one. The `formBindingLoaded` flag this replaces was true for that case.
The consequence is confined to content fidelity -- `getFormBinding()` answers `{fields: []}` either way -- but it is the one thing "an autosave in that window carries it instead of an empty one" does not cover. A `hasContentMeta(key)` mirroring `readContentMeta` would close it.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8351:
URL: https://github.com/apache/texera/pull/8351#issuecomment-5515962503
<!-- backport-auto-label-report -->
### Backport auto-label report
This `fix:` PR was checked against each actively-supported release branch. `release/*` labels drive the post-merge backport, so add or remove one to change where this fix lands.
| Release branch | Analysis |
| --- | --- |
| ✅ `release/v1.3` | Change detected on this branch — label added; this fix is queued to backport here. Requested review from @mengw15. |
| ✅ `release/v1.2` | Change detected on this branch — label added; this fix is queued to backport here. Requested review from @xuang7. |
_[Auto-label run](https://github.com/apache/texera/actions/runs/33679633326)._
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r4127082541
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -136,12 +158,61 @@ 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 starts with no seed state: nothing in it came from the previous room, no key
+ // has been seeded on it yet, and whatever copy the previously open workflow left waiting is
+ // not this one's -- dropped here, so it cannot leak into the next save of a workflow opened
+ // (or a blank one started) on this document.
+ for (const key of [...this.contentMetaSeeds.keys()]) {
+ this.cancelContentMetaSeed(key); // drops the copy it holds, and stops its listening
+ }
+ this.roomOwnedContentMeta.clear();
+ this.seededContentMetaKeys.clear();
+ const contentMetaMap = this.texeraGraph.sharedModel.contentMetaMap;
+ this.contentMetaObserver = event => {
+ if (!event.transaction.local) {
+ // An entry that arrived from the room belongs to the room, a co-editor's value or its
+ // cleared mark alike: the seed a workflow is opened with yields to it (see seedContentMeta).
+ // The first sync delivers the room's state as a remote transaction, which is how an
+ // opening client learns what the room already holds.
+ 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")) {
+ this.formBindingChangeSubject.next(this.getFormBinding());
+ }
Review Comment:
When a form-binding edit is queued before the first sync, `writeContentMeta` has already announced it. If the room's initial value then arrives, this branch calls `getFormBinding()`, which intentionally returns that pending edit, so the same edit is announced again. By then `WorkflowFormComponent.reflectingLocally` is false, causing an unnecessary form rebuild/defer and another autosave. Suppress map notifications while a pending edit owns this key; the queued edit is already announced and its eventual landing is deliberately silent.
This issue also appears on line 208 of the same file.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -774,6 +845,7 @@ export class WorkflowActionService {
this.getTexeraGraph().getPortDisplayNameChangedSubject(),
this.getTexeraGraph().getPortPropertyChangedStream(),
this.formBindingChanged$,
+ this.workflowSettingsChanged$,
Review Comment:
A local settings edit now reaches both persistence paths: `SettingsComponent` still calls `persistWorkflow()` immediately (`settings.component.ts:88-109`), while this new stream causes Workspace's debounced autosave to call it again. The backend inserts a workflow version on every persist (`WorkflowResource.scala:461-480`), so each settings change creates duplicate requests/versions and hits the endpoint's documented parallel-persist race. Remove the settings panel's obsolete direct persistence (and update its tests) so this shared stream is the single save path.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r4118029876
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -797,25 +876,125 @@ 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.
+ *
+ * The room's precedence is for the first seed of a key on a document, the one a workflow is
+ * opened with. A later reload into the same document -- a version shown, or returned from; an
+ * agent's rewrite -- is the intent, and its seed replaces whatever the document holds, a value
+ * the room put there included; `undefined` then clears the key, so a version that carries no
+ * settings or no form definition does not keep the open workflow's.
+ */
+ private seedContentMeta(key: ContentMetaKey, value: ContentMetaValue | undefined): void {
+ // This seed replaces any the key was still waiting on (and stops its listening); it has an
+ // identity of its own, since a cleared key (undefined) has no value to be told apart by.
+ this.cancelContentMetaSeed(key);
+ const seed: ContentMetaSeed = {};
+ this.contentMetaSeeds.set(key, seed);
+ if (value !== undefined) {
+ this.pendingContentMeta.set(key, value);
+ }
+ const shared = this.texeraGraph.sharedModel;
+ // Runs at once or when the sync comes, and then for this seed only: a later seed of the key, a
+ // local edit of it and the loading of another document all detach the listening before they
+ // would make this seed stale (see cancelContentMetaSeed, observeContentMeta).
+ const land = () => {
+ this.cancelContentMetaSeed(key);
+ const first = !this.seededContentMetaKeys.has(key);
+ this.seededContentMetaKeys.add(key);
+ // Over the first seed the room's value wins, whether it is a co-editor's edit or this
+ // client's from another tab; a later reload replaces it.
+ if (first && this.roomOwnedContentMeta.has(key)) {
+ return;
Review Comment:
A room-side deletion cannot reliably win this first-seed check. `roomOwnedContentMeta` is populated only from changed keys, but after a collaborator reloads content without `formBinding`, the room's final map state is simply “key absent”; a newly joining client can fetch the still-stale database copy, observe no present room value during sync, and then seed that copy here, resurrecting the deleted definition. Represent absence explicitly in the shared document (for example, with a typed tombstone/presence wrapper) so a synchronized deletion is distinguishable from a room that has never initialized this key.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #8351:
URL: https://github.com/apache/texera/pull/8351#issuecomment-5515962746
<!-- texera-reviewer-suggestion -->
### Automated Reviewer Suggestions
Based on the `git blame` history of the changed files, we recommend the following reviewers:
- No candidates found from `git blame` history.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "yangzhang75 (via GitHub)" <gi...@apache.org>.
yangzhang75 commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r4117998964
##########
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:
Right: the room's precedence outlived the open and reached every later reload into the same document. Fixed in aabcbe763: it applies to the first seed of a key on a document only (`seededContentMetaKeys`, reset with the document); a later reload's seed -- a version shown or returned from (`displayReadonlyWorkflow` / `closeReadonlyWorkflowDisplay`), an agent's rewrite -- replaces the value, the room's included, or clears it for a version that carries none. Spec: a co-editor's value survives the open; a reload then brings another and the document holds that one; a reload without one clears it.
##########
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:
Done in aabcbe763: each seed owns its listener (`on` with a `detach` on the seed, rather than `once`) and drops it when it lands, is superseded or is cancelled; loading a new document cancels every seed of the old one. Spec: one sync listener per waiting key after two reloads, none after a cancelling edit, none once the sync has landed.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -718,7 +774,7 @@ export class WorkflowActionService {
}
const workflowContent: WorkflowContent = workflow.content;
- this.workflowSettings = workflowContent.settings || this.getDefaultSettings();
+ this.hydrateSettings(workflowContent.settings);
Review Comment:
One change since the reply above, in aabcbe763: rather than seeding both keys in the blank branch, the per-document seed state (waiting copies, seeds, room ownership, seeded keys) is reset when a new document is loaded, which is what the blank reload does. So the blank branch seeds nothing and `clearWorkflow` writes nothing; the same two tests pin it (clearing announces nothing and writes nothing; a blank workflow does not read the previous one's waiting copy).
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r3936800228
##########
frontend/src/app/workspace/service/workflow-graph/model/shared-model.ts:
##########
@@ -45,6 +45,11 @@ export class SharedModel {
public operatorLinkMap: Y.Map<OperatorLink>;
public elementPositionMap: Y.Map<Point>;
public debugState: Y.Map<Y.Map<BreakpointInfo>>;
+ // Non-graph workflow content that used to live as a per-client private field and so was
+ // silently overwritten by another editor's whole-content autosave (workflowSettings, and
+ // the Form View definition). Kept in the shared doc, keyed "settings"/"formBinding", so it
+ // syncs live like the graph and every autosave writes the current value, not a stale copy.
+ public contentMetaMap: Y.Map<unknown>;
Review Comment:
`contentMetaMap` is typed as `Y.Map<unknown>`, which forces repeated type assertions at call-sites and makes it easier to accidentally store unexpected values. Consider introducing a small union/type map for known keys (e.g., `"settings"` / `"formBinding"`) so callers can get typed values without casting.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -136,12 +131,28 @@ 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 {
+ this.texeraGraph.sharedModel.contentMetaMap.observe(event => {
+ if (event.changes.keys.has("formBinding") && !this.jointGraphWrapper.getReloadingWorkflow()) {
+ this.formBindingChangeSubject.next(this.getFormBinding());
+ }
+ });
+ }
Review Comment:
`observeContentMeta()` attaches a new Yjs observer each time it’s called, but nothing ever unobserves the previous listener. If `observeContentMeta()` can be invoked multiple times for the same `contentMetaMap` instance (or if old docs remain referenced), this can cause duplicate `formBindingChanged$` emissions and/or memory leaks. Consider storing the observer callback and calling `unobserve` before re-attaching, or guard so you only attach once per map instance.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -136,12 +131,28 @@ 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());
Review Comment:
`observeContentMeta()` attaches a new Yjs observer each time it’s called, but nothing ever unobserves the previous listener. If `observeContentMeta()` can be invoked multiple times for the same `contentMetaMap` instance (or if old docs remain referenced), this can cause duplicate `formBindingChanged$` emissions and/or memory leaks. Consider storing the observer callback and calling `unobserve` before re-attaching, or guard so you only attach once per map instance.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -747,25 +758,29 @@ export class WorkflowActionService {
}
public setWorkflowSettings(workflowSettings: WorkflowSettings | undefined): void {
- if (this.workflowSettings === workflowSettings) {
- return;
- }
-
const newSettings = workflowSettings === undefined ? this.getDefaultSettings() : workflowSettings;
Review Comment:
`setWorkflowSettings` now writes to the shared Yjs map unconditionally. This can generate unnecessary Yjs updates/network traffic when callers set the same value repeatedly (even when referentially equal). Consider restoring a fast-path that compares the current stored value (at least by reference) and returns early if unchanged.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts:
##########
@@ -891,12 +891,41 @@ describe("WorkflowActionService", () => {
sub.unsubscribe();
});
- // Opening a workflow is not an edit; announcing it would save on every open.
+ // The definition lives in the shared model (#8315), so a co-editor's change -- a write to
+ // the shared map from a remote transaction -- is picked up locally and re-rendered, and
+ // this client's next autosave carries the current value instead of a stale private copy.
+ it("should pick up a co-editor's change from the shared model", () => {
+ const seen: unknown[] = [];
+ const sub = service.formBindingChanged$.subscribe(v => seen.push(v));
+
+ texeraGraph.sharedModel.contentMetaMap.set("formBinding", config);
Review Comment:
The test claims it validates a “remote transaction” (co-editor) update, but it uses a local `contentMetaMap.set(...)`. Either adjust the comment to reflect that this is a local-map observer test, or (if feasible in this test harness) simulate a remote update by applying a Yjs update from a separate doc/client to better match the scenario described.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -775,16 +790,17 @@ export class WorkflowActionService {
}
/**
- * Replace the definition as an edit: announced on `formBindingChanged$`, which
- * feeds workflowChanged() and so reaches the existing autosave.
+ * Replace the definition as an edit. The shared map's observer republishes it on
+ * `formBindingChanged$`, which feeds workflowChanged() and so reaches the existing autosave.
*/
public setFormBinding(formBinding: FormBindingConfig): void {
Review Comment:
Similar to settings, `setFormBinding` always writes to `contentMetaMap`, which can create redundant Yjs updates and extra observer churn if invoked with the same value. Consider an early return when the stored value is unchanged (reference check at minimum).
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -814,11 +830,13 @@ export class WorkflowActionService {
links,
commentBoxes,
settings,
- // Carry formBinding only when the workflow has one (loaded with it, or an author
- // populated it), so a plain workflow's content is unchanged and its save cuts no
- // needless version.
- ...(this.formBindingLoaded || this.isFormBindingNonEmpty(this.formBinding)
- ? { formBinding: this.formBinding }
+ // Carry formBinding only when the workflow has one (opened with it, or an author
+ // populated it since), so a plain workflow's content is unchanged and its save cuts no
+ // needless version. `has` stands in for the old "loaded" flag: hydrate sets the key for
+ // a workflow opened with a binding and deletes it for one without.
+ ...(this.texeraGraph.sharedModel.contentMetaMap.has("formBinding") ||
+ this.isFormBindingNonEmpty(this.getFormBinding())
+ ? { formBinding: this.getFormBinding() }
: {}),
Review Comment:
`getFormBinding()` is called multiple times while building the returned object. To avoid repeated reads and keep the returned snapshot consistent (especially if the shared map could change between calls), consider reading it once into a local `const formBinding = this.getFormBinding()` and reusing it in the condition and payload.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts:
##########
@@ -891,12 +891,41 @@ describe("WorkflowActionService", () => {
sub.unsubscribe();
});
- // Opening a workflow is not an edit; announcing it would save on every open.
+ // The definition lives in the shared model (#8315), so a co-editor's change -- a write to
+ // the shared map from a remote transaction -- is picked up locally and re-rendered, and
+ // this client's next autosave carries the current value instead of a stale private copy.
+ it("should pick up a co-editor's change from the shared model", () => {
+ const seen: unknown[] = [];
+ const sub = service.formBindingChanged$.subscribe(v => seen.push(v));
+
+ texeraGraph.sharedModel.contentMetaMap.set("formBinding", config);
+
+ expect(seen).toEqual([config]);
+ expect(service.getFormBinding()).toEqual(config);
+ sub.unsubscribe();
+ });
+
+ // Opening a workflow is not an edit; announcing it would save on every open. The seed
+ // runs under the reloading flag, so the shared-map observer skips it.
it("should stay silent while a workflow is being opened", () => {
const seen: unknown[] = [];
const sub = service.formBindingChanged$.subscribe(v => seen.push(v));
- service.hydrateFormBinding(config);
+ service.reloadWorkflow(
+ {
+ ...DEFAULT_WORKFLOW,
+ content: {
+ operators: [mockScanPredicate],
+ operatorPositions: { [mockScanPredicate.operatorID]: mockPoint },
+ links: [],
+ commentBoxes: [],
+ settings: undefined as any,
Review Comment:
`undefined as any` weakens the test’s type safety and can mask real contract changes to `WorkflowContent`. Prefer omitting `settings` if it’s optional, or supplying a properly typed `undefined`/default value consistent with the type definition (or constructing the object via a typed helper/builder) so the test will fail if the API shape changes.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "yangzhang75 (via GitHub)" <gi...@apache.org>.
yangzhang75 commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r3937494222
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -667,7 +678,7 @@ export class WorkflowActionService {
}
const workflowContent: WorkflowContent = workflow.content;
- this.workflowSettings = workflowContent.settings || this.getDefaultSettings();
+ this.setWorkflowSettings(workflowContent.settings);
Review Comment:
Done. Added `hydrateSettings`, mirroring `hydrateFormBinding`: `undefined` deletes the `settings` key rather than writing defaults, so opening a workflow that never saved settings no longer overwrites a co-editor's live ones (`getWorkflowSettings` defaults on the absent key). `reloadWorkflow` seeds through it now, and there's a test for the delete. 7eeea55.
##########
frontend/src/app/workspace/service/workflow-graph/model/shared-model.ts:
##########
@@ -45,6 +45,11 @@ export class SharedModel {
public operatorLinkMap: Y.Map<OperatorLink>;
public elementPositionMap: Y.Map<Point>;
public debugState: Y.Map<Y.Map<BreakpointInfo>>;
+ // Non-graph workflow content that used to live as a per-client private field and so was
+ // silently overwritten by another editor's whole-content autosave (workflowSettings, and
+ // the Form View definition). Kept in the shared doc, keyed "settings"/"formBinding", so it
+ // syncs live like the graph and every autosave writes the current value, not a stale copy.
+ public contentMetaMap: Y.Map<unknown>;
Review Comment:
Done. `contentMetaMap` is now `Y.Map<ContentMetaValue>` where `ContentMetaValue = WorkflowSettings | FormBindingConfig`, so it is off `unknown` and rejects unrelated values. 7eeea55.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -136,12 +131,28 @@ 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());
Review Comment:
Fixed. `observeContentMeta` now tracks the observer and the map it is attached to, and `unobserve`s the previous one before re-attaching, so re-attaching on each opened workflow no longer stacks listeners. 7eeea55.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -136,12 +131,28 @@ 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 {
+ this.texeraGraph.sharedModel.contentMetaMap.observe(event => {
+ if (event.changes.keys.has("formBinding") && !this.jointGraphWrapper.getReloadingWorkflow()) {
+ this.formBindingChangeSubject.next(this.getFormBinding());
+ }
+ });
+ }
Review Comment:
Fixed. See above -- `observeContentMeta` now detaches the prior observer before re-attaching, so no listener accumulates across opened workflows. 7eeea55.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -747,25 +758,29 @@ export class WorkflowActionService {
}
public setWorkflowSettings(workflowSettings: WorkflowSettings | undefined): void {
- if (this.workflowSettings === workflowSettings) {
- return;
- }
-
const newSettings = workflowSettings === undefined ? this.getDefaultSettings() : workflowSettings;
Review Comment:
Fixed. `setWorkflowSettings` early-returns when the value is unchanged (lodash `isEqual`), so setting the same value cuts no redundant Yjs update. Added a test. 7eeea55.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -775,16 +790,17 @@ export class WorkflowActionService {
}
/**
- * Replace the definition as an edit: announced on `formBindingChanged$`, which
- * feeds workflowChanged() and so reaches the existing autosave.
+ * Replace the definition as an edit. The shared map's observer republishes it on
+ * `formBindingChanged$`, which feeds workflowChanged() and so reaches the existing autosave.
*/
public setFormBinding(formBinding: FormBindingConfig): void {
Review Comment:
Fixed. `setFormBinding` early-returns on an unchanged value too, so a redundant set cuts no Yjs update and does not re-fire the observer. Added a test. 7eeea55.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts:
##########
@@ -891,12 +891,41 @@ describe("WorkflowActionService", () => {
sub.unsubscribe();
});
- // Opening a workflow is not an edit; announcing it would save on every open.
+ // The definition lives in the shared model (#8315), so a co-editor's change -- a write to
+ // the shared map from a remote transaction -- is picked up locally and re-rendered, and
+ // this client's next autosave carries the current value instead of a stale private copy.
+ it("should pick up a co-editor's change from the shared model", () => {
+ const seen: unknown[] = [];
+ const sub = service.formBindingChanged$.subscribe(v => seen.push(v));
+
+ texeraGraph.sharedModel.contentMetaMap.set("formBinding", config);
Review Comment:
Fixed the comment. It now says the observer fires on any change to the shared map (a local edit and a co-editor's remote update flow through the same path), and that writing to the map directly stands in for either source. 7eeea55.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.spec.ts:
##########
@@ -891,12 +891,41 @@ describe("WorkflowActionService", () => {
sub.unsubscribe();
});
- // Opening a workflow is not an edit; announcing it would save on every open.
+ // The definition lives in the shared model (#8315), so a co-editor's change -- a write to
+ // the shared map from a remote transaction -- is picked up locally and re-rendered, and
+ // this client's next autosave carries the current value instead of a stale private copy.
+ it("should pick up a co-editor's change from the shared model", () => {
+ const seen: unknown[] = [];
+ const sub = service.formBindingChanged$.subscribe(v => seen.push(v));
+
+ texeraGraph.sharedModel.contentMetaMap.set("formBinding", config);
+
+ expect(seen).toEqual([config]);
+ expect(service.getFormBinding()).toEqual(config);
+ sub.unsubscribe();
+ });
+
+ // Opening a workflow is not an edit; announcing it would save on every open. The seed
+ // runs under the reloading flag, so the shared-map observer skips it.
it("should stay silent while a workflow is being opened", () => {
const seen: unknown[] = [];
const sub = service.formBindingChanged$.subscribe(v => seen.push(v));
- service.hydrateFormBinding(config);
+ service.reloadWorkflow(
+ {
+ ...DEFAULT_WORKFLOW,
+ content: {
+ operators: [mockScanPredicate],
+ operatorPositions: { [mockScanPredicate.operatorID]: mockPoint },
+ links: [],
+ commentBoxes: [],
+ settings: undefined as any,
Review Comment:
Fixed. Replaced `undefined as any` with a real `WorkflowSettings` (`settings` is required on `WorkflowContent`), keeping the test's type safety. 7eeea55.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -814,11 +830,13 @@ export class WorkflowActionService {
links,
commentBoxes,
settings,
- // Carry formBinding only when the workflow has one (loaded with it, or an author
- // populated it), so a plain workflow's content is unchanged and its save cuts no
- // needless version.
- ...(this.formBindingLoaded || this.isFormBindingNonEmpty(this.formBinding)
- ? { formBinding: this.formBinding }
+ // Carry formBinding only when the workflow has one (opened with it, or an author
+ // populated it since), so a plain workflow's content is unchanged and its save cuts no
+ // needless version. `has` stands in for the old "loaded" flag: hydrate sets the key for
+ // a workflow opened with a binding and deletes it for one without.
+ ...(this.texeraGraph.sharedModel.contentMetaMap.has("formBinding") ||
+ this.isFormBindingNonEmpty(this.getFormBinding())
+ ? { formBinding: this.getFormBinding() }
: {}),
Review Comment:
Fixed. `getWorkflowContent` reads `getFormBinding()` once into a const used by both the `has`-check fallback and the value, so the snapshot is consistent. 7eeea55.
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "mengw15 (via GitHub)" <gi...@apache.org>.
mengw15 merged PR #8351:
URL: https://github.com/apache/texera/pull/8351
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(workflow): keep workflowSettings and formBinding in the shared model [texera]
Posted by "yangzhang75 (via GitHub)" <gi...@apache.org>.
yangzhang75 commented on code in PR #8351:
URL: https://github.com/apache/texera/pull/8351#discussion_r4117888474
##########
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:
Right: with nothing in the pending map for a cleared key, the seed could not tell "still waiting" from "superseded by an edit". Fixed in 6667f6962: each seed has an identity of its own (`contentMetaSeeds`), and a local write to the key (`setFormBinding`, `setWorkflowSettings`, anything but the seed itself) cancels it in the observer, so the sync lands on a cancelled seed and neither writes over nor deletes the author's value. Spec: `hydrateFormBinding(undefined)`, `setFormBinding(config)`, sync -> the document still holds `config` (red before).
##########
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:
Agreed: the fallback wrote before the sync on a slow room, which is the concurrent write the wait exists to avoid. Removed in 6667f6962: the copy goes into the document only on the first sync (or at once when the document is not connecting, or has already synced); until then reads and every save carry it, and never seeding it loses nothing, since nobody else was in that document either. Spec: 60 s without a sync -> document untouched while `getFormBinding()` and `getWorkflowContent().formBinding` still answer the copy; then the sync -> the document holds it.
##########
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:
Manual steps are in the PR body now (two windows on one workflow: A exposes a field in the Form View or changes a setting, B drags an operator, reload A). A side-by-side before/after video is being recorded and will be added to the body; leaving this open until it is.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -718,7 +774,7 @@ export class WorkflowActionService {
}
const workflowContent: WorkflowContent = workflow.content;
- this.workflowSettings = workflowContent.settings || this.getDefaultSettings();
+ this.hydrateSettings(workflowContent.settings);
Review Comment:
Done in 6667f6962: `hydrateSettings(undefined)` now sits beside `hydrateFormBinding(undefined)` in the blank branch, and `clearWorkflow` no longer calls `setWorkflowSettings(undefined)`, so the settings go back to the defaults through the seed under the reloading flag, with nothing written into the destroyed document or announced. Spec: non-default settings, `clearWorkflow()` -> nothing on `workflowChanged()`, settings back to the defaults (red before: one announcement). The pending-seed half is with @copilot's thread: each seed has an identity now, and a local edit of the key cancels it.
##########
frontend/src/app/workspace/service/workflow-graph/model/workflow-action.service.ts:
##########
@@ -865,11 +1002,12 @@ export class WorkflowActionService {
links,
commentBoxes,
settings,
- // Carry formBinding only when the workflow has one (loaded with it, or an author
- // populated it), so a plain workflow's content is unchanged and its save cuts no
- // needless version.
- ...(this.formBindingLoaded || this.isFormBindingNonEmpty(this.formBinding)
- ? { formBinding: this.formBinding }
+ // Carry formBinding only when the workflow has one (opened with it, or an author
+ // populated it since), so a plain workflow's content is unchanged and its save cuts no
+ // needless version. `has` stands in for the old "loaded" flag: hydrate sets the key for
+ // a workflow opened with a binding and deletes it for one without.
+ ...(this.texeraGraph.sharedModel.contentMetaMap.has("formBinding") || this.isFormBindingNonEmpty(formBinding)
Review Comment:
Done in 6667f6962: `hasContentMeta(key)` mirrors `readContentMeta` (the document, or the copy still waiting), and `getWorkflowContent` uses it, so presence and value come from the same place. Spec: `hydrateFormBinding({ fields: [] })` before the sync -> `getWorkflowContent().formBinding` is `{ fields: [] }`, not dropped (red before).
--
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: notifications-unsubscribe@texera.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org