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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
