mengw15 commented on code in PR #7685:
URL: https://github.com/apache/texera/pull/7685#discussion_r3788505196
##########
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:
Applied — the peer doc is now created and destroyed in a `try/finally`, and
the sync back encodes only the diff against the local state vector rather than
the whole document, which is what a real peer would send.
##########
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:
Correct, and worse than it looked — the assertion could not fail at all. Two
checks:
- Removing the `if (isLocal)` guard from `onOperatorPropertyChanged` (so a
remote change is treated as local) left this spec **green**.
- A positive control showed why: with `currentlyEditing` unset, even a
genuinely local property change never reaches `updateSharedModelAwareness`, so
the spy was always at zero calls.
The first fix I tried —
`texeraGraph.updateSharedModelAwareness("currentlyEditing", …)` — does not
help: with no provider connected, the shared model's awareness local state
stays `{}`, so the guard still fails. Setting the field directly on the
awareness the handler reads does work (`getLocalState()` becomes
`{"currentlyEditing":"1"}`, and a local change then produces the two expected
calls).
The test now does that before spying, so `transaction.local` is the only
thing left that can keep the call away. Verified: with the fix in place,
removing the `if (isLocal)` guard makes this spec **fail**, where the same
mutation previously passed.
--
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]
