Modify Snapshot Event to publish updated local changes (#923)

This commit improves snapshot event handling to maintain cursor position integrity. Previously, the cursor would briefly jump to the top of the document during snapshot events, particularly noticeable with backend-snapshot-threshold=1. Key changes: - Updated Snapshot Event to include most recent local changes - Modified CodeMirror example to preserve cursor position during snapshots - Improved synchronization between local state and snapshot data --------- Co-authored-by: Youngteac Hong <[email protected]> Co-authored-by: Yourim Cha <[email protected]>
yorkie-team · Nov 7, 2024 · 924ba4c · 924ba4c
1 parent 9a6ab62
commit 924ba4c
Show file tree

Hide file tree

Showing 9 changed files with 149 additions and 47 deletions.
diff --git a/examples/vanilla-codemirror6/src/main.ts b/examples/vanilla-codemirror6/src/main.ts
@@ -1,17 +1,17 @@
 /* eslint-disable jsdoc/require-jsdoc */
 import yorkie, { DocEventType } from 'yorkie-js-sdk';
-import type { TextOperationInfo, EditOpInfo } from 'yorkie-js-sdk';
+import type { EditOpInfo, OperationInfo } from 'yorkie-js-sdk';
 import { basicSetup, EditorView } from 'codemirror';
 import { keymap } from '@codemirror/view';
 import {
   markdown,
   markdownKeymap,
   markdownLanguage,
 } from '@codemirror/lang-markdown';
-import { Transaction } from '@codemirror/state';
+import { Transaction, TransactionSpec } from '@codemirror/state';
 import { network } from './network';
 import { displayLog, displayPeers } from './utils';
-import { YorkieDoc } from './type';
+import { YorkieDoc, YorkiePresence } from './type';
 import './style.css';
 
 const editorParentElem = document.getElementById('editor')!;
@@ -28,7 +28,7 @@ async function main() {
   await client.activate();
 
   // 02-1. create a document then attach it into the client.
-  const doc = new yorkie.Document<YorkieDoc>(
+  const doc = new yorkie.Document<YorkieDoc, YorkiePresence>(
     `codemirror6-${new Date()
       .toISOString()
       .substring(0, 10)
@@ -55,10 +55,21 @@ async function main() {
   // 02-2. subscribe document event.
   const syncText = () => {
     const text = doc.getRoot().content;
-    view.dispatch({
+    const selection = doc.getMyPresence().selection;
+    const transactionSpec: TransactionSpec = {
       changes: { from: 0, to: view.state.doc.length, insert: text.toString() },
       annotations: [Transaction.remote.of(true)],
-    });
+    };
+
+    if (selection) {
+      // Restore the cursor position when the text is replaced.
+      const cursor = text.posRangeToIndexRange(selection);
+      transactionSpec['selection'] = {
+        anchor: cursor[0],
+        head: cursor[1],
+      };
+    }
+    view.dispatch(transactionSpec);
   };
   doc.subscribe((event) => {
     if (event.type === 'snapshot') {
@@ -98,6 +109,32 @@ async function main() {
         });
       }
     }
+
+    const hasFocus =
+      viewUpdate.view.hasFocus && viewUpdate.view.dom.ownerDocument.hasFocus();
+    const sel = hasFocus ? viewUpdate.state.selection.main : null;
+
+    doc.update((root, presence) => {
+      if (sel && root.content) {
+        const selection = root.content.indexRangeToPosRange([
+          sel.anchor,
+          sel.head,
+        ]);
+
+        if (
+          JSON.stringify(selection) !==
+          JSON.stringify(presence.get('selection'))
+        ) {
+          presence.set({
+            selection,
+          });
+        }
+      } else if (presence.get('selection')) {
+        presence.set({
+          selection: undefined,
+        });
+      }
+    });
   });
 
   // 03-2. create codemirror instance
@@ -113,7 +150,7 @@ async function main() {
   });
 
   // 03-3. define event handler that apply remote changes to local
-  function handleOperations(operations: Array<TextOperationInfo>) {
+  function handleOperations(operations: Array<OperationInfo>) {
     for (const op of operations) {
       if (op.type === 'edit') {
         handleEditOp(op);

diff --git a/examples/vanilla-codemirror6/src/type.ts b/examples/vanilla-codemirror6/src/type.ts
@@ -1,5 +1,9 @@
-import { type Text } from 'yorkie-js-sdk';
+import { TextPosStructRange, type Text } from 'yorkie-js-sdk';
 
 export type YorkieDoc = {
   content: Text;
 };
+
+export type YorkiePresence = {
+  selection?: TextPosStructRange;
+};
diff --git a/packages/sdk/src/document/document.ts b/packages/sdk/src/document/document.ts
@@ -1148,6 +1148,22 @@ export class Document<T, P extends Indexable = Indexable> {
     return targetPath.every((path, index) => path === nodePath[index]);
   }
 
+  /**
+   * `removePushedLocalChanges` removes local changes that have been applied to
+   * the server from the local changes.
+   *
+   * @param clientSeq - client sequence number to remove local changes before it
+   */
+  private removePushedLocalChanges(clientSeq: number) {
+    while (this.localChanges.length) {
+      const change = this.localChanges[0];
+      if (change.getID().getClientSeq() > clientSeq) {
+        break;
+      }
+      this.localChanges.shift();
+    }
+  }
+
   /**
    * `applyChangePack` applies the given change pack into this document.
    * 1. Remove local changes applied to server.
@@ -1160,42 +1176,28 @@ export class Document<T, P extends Indexable = Indexable> {
   public applyChangePack(pack: ChangePack<P>): void {
     const hasSnapshot = pack.hasSnapshot();
 
+    // 01. Apply snapshot or changes to the root object.
     if (hasSnapshot) {
       this.applySnapshot(
         pack.getCheckpoint().getServerSeq(),
         pack.getVersionVector()!,
         pack.getSnapshot()!,
+        pack.getCheckpoint().getClientSeq(),
       );
-    } else if (pack.hasChanges()) {
+    } else {
       this.applyChanges(pack.getChanges(), OpSource.Remote);
+      this.removePushedLocalChanges(pack.getCheckpoint().getClientSeq());
     }
 
-    // 02. Remove local changes applied to server.
-    while (this.localChanges.length) {
-      const change = this.localChanges[0];
-      if (change.getID().getClientSeq() > pack.getCheckpoint().getClientSeq()) {
-        break;
-      }
-      this.localChanges.shift();
-    }
-
-    // NOTE(hackerwins): If the document has local changes, we need to apply
-    // them after applying the snapshot. We need to treat the local changes
-    // as remote changes because the application should apply the local
-    // changes to their own document.
-    if (hasSnapshot) {
-      this.applyChanges(this.localChanges, OpSource.Remote);
-    }
-
-    // 03. Update the checkpoint.
+    // 02. Update the checkpoint.
     this.checkpoint = this.checkpoint.forward(pack.getCheckpoint());
 
-    // 04. Do Garbage collection.
+    // 03. Do Garbage collection.
     if (!hasSnapshot) {
       this.garbageCollect(pack.getVersionVector()!);
     }
 
-    // 05. Filter detached client's lamport from version vector
+    // 04. Filter detached client's lamport from version vector
     if (!hasSnapshot) {
       this.filterVersionVector(pack.getVersionVector()!);
     }
@@ -1412,6 +1414,7 @@ export class Document<T, P extends Indexable = Indexable> {
     serverSeq: bigint,
     snapshotVector: VersionVector,
     snapshot?: Uint8Array,
+    clientSeq: number = -1,
   ) {
     const { root, presences } = converter.bytesToSnapshot<P>(snapshot);
     this.root = new CRDTRoot(root);
@@ -1421,6 +1424,13 @@ export class Document<T, P extends Indexable = Indexable> {
     // drop clone because it is contaminated.
     this.clone = undefined;
 
+    this.removePushedLocalChanges(clientSeq);
+
+    // NOTE(hackerwins): If the document has local changes, we need to apply
+    // them after applying the snapshot, as local changes are not included in the snapshot data.
+    // Afterward, we should publish a snapshot event with the latest
+    // version of the document to ensure the user receives the most up-to-date snapshot.
+    this.applyChanges(this.localChanges, OpSource.Local);
     this.publish([
       {
         type: DocEventType.Snapshot,
@@ -1689,6 +1699,7 @@ export class Document<T, P extends Indexable = Indexable> {
     if (event.type === DocEventType.Snapshot) {
       const { snapshot, serverSeq, snapshotVector } = event.value;
       if (!snapshot) return;
+      // TODO(hackerwins): We need to check the clientSeq of the snapshot.
       this.applySnapshot(
         BigInt(serverSeq),
         converter.hexToVersionVector(snapshotVector),

diff --git a/packages/sdk/test/helper/helper.ts b/packages/sdk/test/helper/helper.ts
@@ -42,6 +42,7 @@ import { InitialActorID } from '@yorkie-js-sdk/src/document/time/actor_id';
 import { VersionVector } from '@yorkie-js-sdk/src/document/time/version_vector';
 
 export type Indexable = Record<string, any>;
+export const DefaultSnapshotThreshold = 1000;
 
 /**
  * EventCollector provides a utility to collect and manage events.

diff --git a/packages/sdk/test/integration/client_test.ts b/packages/sdk/test/integration/client_test.ts
@@ -25,7 +25,10 @@ import yorkie, {
   Tree,
   Text,
 } from '@yorkie-js-sdk/src/yorkie';
-import { EventCollector } from '@yorkie-js-sdk/test/helper/helper';
+import {
+  EventCollector,
+  DefaultSnapshotThreshold,
+} from '@yorkie-js-sdk/test/helper/helper';
 import {
   toDocKey,
   testRPCAddr,
@@ -852,7 +855,7 @@ describe.sequential('Client', function () {
       await c2.sync();
 
       // 01. c1 increases the counter for creating snapshot.
-      for (let i = 0; i < 500; i++) {
+      for (let i = 0; i < DefaultSnapshotThreshold; i++) {
         d1.update((r) => r.counter.increase(1));
       }
       await c1.sync();

diff --git a/packages/sdk/test/integration/presence_test.ts b/packages/sdk/test/integration/presence_test.ts
@@ -11,7 +11,11 @@ import {
   testRPCAddr,
   toDocKey,
 } from '@yorkie-js-sdk/test/integration/integration_helper';
-import { EventCollector, deepSort } from '@yorkie-js-sdk/test/helper/helper';
+import {
+  EventCollector,
+  deepSort,
+  DefaultSnapshotThreshold,
+} from '@yorkie-js-sdk/test/helper/helper';
 
 describe('Presence', function () {
   afterEach(() => {
@@ -32,18 +36,17 @@ describe('Presence', function () {
     const doc2 = new yorkie.Document<{}, PresenceType>(docKey);
     await c2.attach(doc2, { syncMode: SyncMode.Manual });
 
-    const snapshotThreshold = 500;
-    for (let i = 0; i < snapshotThreshold; i++) {
+    for (let i = 0; i < DefaultSnapshotThreshold; i++) {
       doc1.update((root, p) => p.set({ key: `${i}` }));
     }
     assert.deepEqual(doc1.getPresenceForTest(c1.getID()!), {
-      key: `${snapshotThreshold - 1}`,
+      key: `${DefaultSnapshotThreshold - 1}`,
     });
 
     await c1.sync();
     await c2.sync();
     assert.deepEqual(doc2.getPresenceForTest(c1.getID()!), {
-      key: `${snapshotThreshold - 1}`,
+      key: `${DefaultSnapshotThreshold - 1}`,
     });
   });
 

diff --git a/packages/sdk/test/integration/snapshot_test.ts b/packages/sdk/test/integration/snapshot_test.ts
@@ -1,13 +1,14 @@
 import { describe, it, assert } from 'vitest';
+import { DefaultSnapshotThreshold } from '@yorkie-js-sdk/test/helper/helper';
 import { withTwoClientsAndDocuments } from '@yorkie-js-sdk/test/integration/integration_helper';
 import { Text } from '@yorkie-js-sdk/src/yorkie';
 
 describe('Snapshot', function () {
   it('should handle snapshot', async function ({ task }) {
     type TestDoc = Record<string, number> & { key: string };
     await withTwoClientsAndDocuments<TestDoc>(async (c1, d1, c2, d2) => {
-      // 01. Updates 700 changes over snapshot threshold.
-      for (let idx = 0; idx < 700; idx++) {
+      // 01. Updates changes over snapshot threshold.
+      for (let idx = 0; idx < DefaultSnapshotThreshold; idx++) {
         d1.update((root) => {
           root[`${idx}`] = idx;
         });
@@ -30,16 +31,16 @@ describe('Snapshot', function () {
 
   it('should handle snapshot for text object', async function ({ task }) {
     await withTwoClientsAndDocuments<{ k1: Text }>(async (c1, d1, c2, d2) => {
-      for (let idx = 0; idx < 700; idx++) {
+      for (let idx = 0; idx < DefaultSnapshotThreshold; idx++) {
         d1.update((root) => {
           root.k1 = new Text();
         }, 'set new doc by c1');
       }
       await c1.sync();
       await c2.sync();
 
-      // 01. Updates 500 changes over snapshot threshold by c1.
-      for (let idx = 0; idx < 500; idx++) {
+      // 01. Updates changes over snapshot threshold by c1.
+      for (let idx = 0; idx < DefaultSnapshotThreshold; idx++) {
         d1.update((root) => {
           root.k1.edit(idx, idx, 'x');
         });
@@ -69,8 +70,8 @@ describe('Snapshot', function () {
       await c1.sync();
       await c2.sync();
 
-      // 01. Updates 700 changes over snapshot threshold by c1.
-      for (let idx = 0; idx < 700; idx++) {
+      // 01. Updates changes over snapshot threshold by c1.
+      for (let idx = 0; idx < DefaultSnapshotThreshold; idx++) {
         d1.update((root) => {
           root.k1.setStyle(0, 1, { bold: 'true' });
         });

diff --git a/packages/sdk/test/integration/tree_test.ts b/packages/sdk/test/integration/tree_test.ts
@@ -21,7 +21,10 @@ import {
   toDocKey,
   withTwoClientsAndDocuments,
 } from '@yorkie-js-sdk/test/integration/integration_helper';
-import { EventCollector } from '@yorkie-js-sdk/test/helper/helper';
+import {
+  EventCollector,
+  DefaultSnapshotThreshold,
+} from '@yorkie-js-sdk/test/helper/helper';
 import {
   TreeEditOpInfo,
   TreeStyleOpInfo,
@@ -1558,8 +1561,7 @@ describe('Tree.style', function () {
     await c2.attach(d2, { syncMode: SyncMode.Manual });
 
     // Perform a dummy update to apply changes up to the snapshot threshold.
-    const snapshotThreshold = 500;
-    for (let idx = 0; idx < snapshotThreshold; idx++) {
+    for (let idx = 0; idx < DefaultSnapshotThreshold; idx++) {
       d1.update((root) => {
         root.num = 0;
       });