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