Copilot commented on code in PR #7685:
URL: https://github.com/apache/texera/pull/7685#discussion_r3788470000
##########
frontend/src/app/workspace/service/workflow-graph/model/shared-model-change-handler.spec.ts:
##########
@@ -542,4 +543,56 @@ describe("SharedModelChangeHandler", () => {
sub.unsubscribe();
});
});
+ // Every change so far originated locally. A change that arrives from a peer
runs the same
+ // observers with `transaction.local === false` — the arm that decides
whether local awareness
+ // and undo state are touched. A second Y.Doc stands in for the peer:
mutating it and syncing
+ // the diff back produces a genuinely remote transaction, which
`yDoc.transact` cannot fake.
+ describe("remote changes", () => {
+ /** Runs `mutate` on a peer document and syncs the result in as a remote
update. */
+ function applyAsRemote(mutate: (peerDoc: Y.Doc) => void): void {
+ const localDoc = texeraGraph.sharedModel.yDoc;
+ const peerDoc = new Y.Doc();
+ Y.applyUpdate(peerDoc, Y.encodeStateAsUpdate(localDoc));
+ mutate(peerDoc);
+ Y.applyUpdate(localDoc, Y.encodeStateAsUpdate(peerDoc));
+ }
Review Comment:
applyAsRemote creates a new Y.Doc per call but never destroys it. Over many
test runs this can leak memory/event listeners and make the suite slower,
especially in watch mode. Wrapping the peer doc in a try/finally and calling
peerDoc.destroy() keeps the test isolated; while here, encoding a diff against
the local state vector avoids sending the entire doc back.
##########
frontend/src/app/workspace/service/workflow-graph/model/shared-model-change-handler.spec.ts:
##########
@@ -542,4 +543,56 @@ describe("SharedModelChangeHandler", () => {
sub.unsubscribe();
});
});
+ // Every change so far originated locally. A change that arrives from a peer
runs the same
+ // observers with `transaction.local === false` — the arm that decides
whether local awareness
+ // and undo state are touched. A second Y.Doc stands in for the peer:
mutating it and syncing
+ // the diff back produces a genuinely remote transaction, which
`yDoc.transact` cannot fake.
+ describe("remote changes", () => {
+ /** Runs `mutate` on a peer document and syncs the result in as a remote
update. */
+ function applyAsRemote(mutate: (peerDoc: Y.Doc) => void): void {
+ const localDoc = texeraGraph.sharedModel.yDoc;
+ const peerDoc = new Y.Doc();
+ Y.applyUpdate(peerDoc, Y.encodeStateAsUpdate(localDoc));
+ mutate(peerDoc);
+ Y.applyUpdate(localDoc, Y.encodeStateAsUpdate(peerDoc));
+ }
+
+ it("adds an operator that arrives from another client", () => {
+ let added: OperatorPredicate | undefined;
+ const sub = texeraGraph.getOperatorAddStream().subscribe(o => (added =
o));
+
+ applyAsRemote(peerDoc =>
+ peerDoc.transact(() => {
+ peerDoc.getMap("operatorIDMap").set(mockScanPredicate.operatorID,
createYTypeFromObject(mockScanPredicate));
+
peerDoc.getMap("elementPositionMap").set(mockScanPredicate.operatorID,
mockPoint);
+ })
+ );
+
+ expect(added?.operatorID).toBe(mockScanPredicate.operatorID);
+ expect(jointGraph.getCell(mockScanPredicate.operatorID)).toBeTruthy();
+ sub.unsubscribe();
+ });
+
+ it("applies a remote property change without updating local awareness", ()
=> {
+ addOperatorWithPosition(mockScanPredicate);
+ const awarenessSpy = vi.spyOn(texeraGraph, "updateSharedModelAwareness");
+ let changed = false;
+ const sub = texeraGraph.getOperatorPropertyChangeStream().subscribe(()
=> (changed = true));
Review Comment:
This test is intended to prove the remote transaction skips the
local-awareness update, but updateSharedModelAwareness only runs when
awareness.localState.currentlyEditing matches the operator. Since the test
never sets currentlyEditing, the spy would not be called even if the change
were (incorrectly) treated as local, so the assertion can be a false positive.
--
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]
