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