Prevent empty ops are applied during undo/redo (#687)

This commit addresses the issue of client sequences being absent on
the server. It accomplishes this by preventing the update of changeID.

src/document/document.ts

@@ -550,6 +550,7 @@ export class Document<T, P extends Indexable = Indexable> {
       this.changeID = change.getID();
 
       // 03. Publish the document change event.
+      // NOTE(chacha912): Check opInfos, which represent the actually executed operations.
       if (opInfos.length > 0) {
         this.publish({
           type: DocEventType.LocalChange,
@@ -846,6 +847,15 @@ export class Document<T, P extends Indexable = Indexable> {
     return this.checkpoint;
   }
 
+  /**
+   * `getChangeID` returns the change id of this document.
+   *
+   * @internal
+   */
+  public getChangeID(): ChangeID {
+    return this.changeID;
+  }
+
   /**
    * `hasLocalChanges` returns whether this document has local changes or not.
    *
@@ -1309,18 +1319,15 @@ export class Document<T, P extends Indexable = Indexable> {
       this.internalHistory.pushRedo(reverseOps);
     }
 
-    // TODO(chacha912): When there is no applied operation or presence
+    // NOTE(chacha912): When there is no applied operation or presence
     // during undo/redo, skip propagating change remotely.
-    // In the database, it may appear as if the client sequence is missing.
-    if (change.hasPresenceChange() || opInfos.length > 0) {
-      this.localChanges.push(change);
+    if (!change.hasPresenceChange() && opInfos.length === 0) {
+      return;
     }
 
+    this.localChanges.push(change);
     this.changeID = change.getID();
     const actorID = this.changeID.getActorID()!;
-    // NOTE(chacha912): Although operations are included in the change, they
-    // may not be executable (e.g., when the target element has been deleted).
-    // So we check opInfos, which represent the actually executed operations.
     if (opInfos.length > 0) {
       this.publish({
         type: DocEventType.LocalChange,
@@ -1400,15 +1407,13 @@ export class Document<T, P extends Indexable = Indexable> {
 
     // NOTE(chacha912): When there is no applied operation or presence
     // during undo/redo, skip propagating change remotely.
-    if (change.hasPresenceChange() || opInfos.length > 0) {
-      this.localChanges.push(change);
+    if (!change.hasPresenceChange() && opInfos.length === 0) {
+      return;
     }
 
+    this.localChanges.push(change);
     this.changeID = change.getID();
     const actorID = this.changeID.getActorID()!;
-    // NOTE(chacha912): Although operations are included in the change, they
-    // may not be executable (e.g., when the target element has been deleted).
-    // So we check opInfos, which represent the actually executed operations.
     if (opInfos.length > 0) {
       this.publish({
         type: DocEventType.LocalChange,

test/integration/object_test.ts

@@ -507,6 +507,82 @@ describe('Object', function () {
       await client1.sync();
     });
 
+    it.skip(`Should not propagate changes when there is no applied undo operation`, async function ({
+      task,
+    }) {
+      interface TestDoc {
+        shape?: { color: string };
+      }
+      const docKey = toDocKey(`${task.name}-${new Date().getTime()}`);
+      const doc1 = new Document<TestDoc>(docKey);
+      const doc2 = new Document<TestDoc>(docKey);
+
+      const client1 = new Client(testRPCAddr);
+      const client2 = new Client(testRPCAddr);
+      await client1.activate();
+      await client2.activate();
+
+      await client1.attach(doc1, { isRealtimeSync: false });
+      let doc1ChangeID = doc1.getChangeID();
+      let doc1Checkpoint = doc1.getCheckpoint();
+      assert.equal(doc1ChangeID.getClientSeq(), 1);
+      assert.equal(doc1Checkpoint.getClientSeq(), 1);
+      assert.equal(doc1Checkpoint.getServerSeq().toInt(), 1);
+
+      doc1.update((root) => {
+        root.shape = { color: 'black' };
+      }, 'init doc');
+      await client1.sync();
+      assert.equal(doc1.toSortedJSON(), '{"shape":{"color":"black"}}');
+      doc1ChangeID = doc1.getChangeID();
+      doc1Checkpoint = doc1.getCheckpoint();
+      assert.equal(doc1ChangeID.getClientSeq(), 2);
+      assert.equal(doc1Checkpoint.getClientSeq(), 2);
+      assert.equal(doc1Checkpoint.getServerSeq().toInt(), 2);
+
+      await client2.attach(doc2, { isRealtimeSync: false });
+      assert.equal(doc2.toSortedJSON(), '{"shape":{"color":"black"}}');
+
+      doc2.update((root) => {
+        delete root.shape;
+      }, 'delete shape');
+      await client2.sync();
+      await client1.sync();
+      assert.equal(doc1.toSortedJSON(), '{}');
+      assert.equal(doc2.toSortedJSON(), '{}');
+      doc1ChangeID = doc1.getChangeID();
+      doc1Checkpoint = doc1.getCheckpoint();
+      assert.equal(doc1ChangeID.getClientSeq(), 2);
+      assert.equal(doc1Checkpoint.getClientSeq(), 2);
+      assert.equal(doc1Checkpoint.getServerSeq().toInt(), 4);
+
+      // c2 deleted the shape, so the reverse operation cannot be applied
+      doc1.history.undo();
+      assert.equal(doc1.toSortedJSON(), '{}');
+      assert.equal(doc1.getRedoStackForTest().length, 0);
+      assert.equal(doc1.history.canRedo(), false);
+      await client1.sync();
+      await client2.sync();
+      await client1.sync();
+      // Since there are no applied operations, there should be no change in the sequence.
+      doc1ChangeID = doc1.getChangeID();
+      doc1Checkpoint = doc1.getCheckpoint();
+      assert.equal(doc1ChangeID.getClientSeq(), 2);
+      assert.equal(doc1Checkpoint.getClientSeq(), 2);
+      assert.equal(doc1Checkpoint.getServerSeq().toInt(), 4);
+
+      doc1.update((root) => {
+        root.shape = { color: 'red' };
+      });
+      await client1.sync();
+      assert.equal(doc1.toSortedJSON(), '{"shape":{"color":"red"}}');
+      doc1ChangeID = doc1.getChangeID();
+      doc1Checkpoint = doc1.getCheckpoint();
+      assert.equal(doc1ChangeID.getClientSeq(), 3);
+      assert.equal(doc1Checkpoint.getClientSeq(), 3);
+      assert.equal(doc1Checkpoint.getServerSeq().toInt(), 5);
+    });
+
     it('Can handle concurrent undo/redo: local undo & global redo', async function ({
       task,
     }) {