From 924ba4c80c6fa83ab9bb1511ba6b8387e2e34c61 Mon Sep 17 00:00:00 2001 From: LeeJongBeom <52884648+devleejb@users.noreply.github.com> Date: Thu, 7 Nov 2024 11:57:06 +0900 Subject: [PATCH] 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 Co-authored-by: Yourim Cha --- examples/vanilla-codemirror6/src/main.ts | 51 +++++++++++++++--- examples/vanilla-codemirror6/src/type.ts | 6 ++- packages/sdk/src/document/document.ts | 53 +++++++++++-------- packages/sdk/test/helper/helper.ts | 1 + packages/sdk/test/integration/client_test.ts | 7 ++- .../sdk/test/integration/presence_test.ts | 13 +++-- .../sdk/test/integration/snapshot_test.ts | 15 +++--- packages/sdk/test/integration/tree_test.ts | 8 +-- .../sdk/test/unit/document/document_test.ts | 42 ++++++++++++++- 9 files changed, 149 insertions(+), 47 deletions(-) diff --git a/examples/vanilla-codemirror6/src/main.ts b/examples/vanilla-codemirror6/src/main.ts index 131909325..fed802937 100644 --- a/examples/vanilla-codemirror6/src/main.ts +++ b/examples/vanilla-codemirror6/src/main.ts @@ -1,6 +1,6 @@ /* 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 { @@ -8,10 +8,10 @@ import { 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( + const doc = new yorkie.Document( `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) { + function handleOperations(operations: Array) { 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 index 969e44b29..9d9e52c41 100644 --- 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 index 7ac9dbd0d..5c13df33c 100644 --- a/packages/sdk/src/document/document.ts +++ b/packages/sdk/src/document/document.ts @@ -1148,6 +1148,22 @@ export class Document { 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 { public applyChangePack(pack: ChangePack

): 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 { serverSeq: bigint, snapshotVector: VersionVector, snapshot?: Uint8Array, + clientSeq: number = -1, ) { const { root, presences } = converter.bytesToSnapshot

(snapshot); this.root = new CRDTRoot(root); @@ -1421,6 +1424,13 @@ export class Document { // 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 { 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 index 168f4eee1..22ffee59f 100644 --- 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; +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 index 3ca1deb03..ce9ac6cf7 100644 --- 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 index ec7f37f00..828283c30 100644 --- 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 index 686fa83ae..612d23c49 100644 --- a/packages/sdk/test/integration/snapshot_test.ts +++ b/packages/sdk/test/integration/snapshot_test.ts @@ -1,4 +1,5 @@ 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'; @@ -6,8 +7,8 @@ describe('Snapshot', function () { it('should handle snapshot', async function ({ task }) { type TestDoc = Record & { key: string }; await withTwoClientsAndDocuments(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,7 +31,7 @@ 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'); @@ -38,8 +39,8 @@ describe('Snapshot', function () { 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 index 0c8621411..f1626acc7 100644 --- 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; }); diff --git a/packages/sdk/test/unit/document/document_test.ts b/packages/sdk/test/unit/document/document_test.ts index e0c3bac6e..888551f0f 100644 --- a/packages/sdk/test/unit/document/document_test.ts +++ b/packages/sdk/test/unit/document/document_test.ts @@ -18,12 +18,19 @@ import { describe, it, assert, vi, afterEach } from 'vitest'; import { EventCollector, MaxVersionVector, + DefaultSnapshotThreshold, } from '@yorkie-js-sdk/test/helper/helper'; import { Document, DocEventType } from '@yorkie-js-sdk/src/document/document'; import { OperationInfo } from '@yorkie-js-sdk/src/document/operation/operation'; -import { JSONArray, Text, Counter, Tree } from '@yorkie-js-sdk/src/yorkie'; +import yorkie, { + JSONArray, + Text, + Counter, + Tree, +} from '@yorkie-js-sdk/src/yorkie'; import { CounterType } from '@yorkie-js-sdk/src/document/crdt/counter'; +import { withTwoClientsAndDocuments } from '@yorkie-js-sdk/test/integration/integration_helper'; describe.sequential('Document', function () { afterEach(() => { @@ -1490,4 +1497,37 @@ describe.sequential('Document', function () { }); }); }); + + it('should publish snapshot event with up-to-date document', async function ({ + task, + }) { + type TestDoc = { counter: Counter }; + await withTwoClientsAndDocuments(async (c1, d1, c2, d2) => { + const eventCollector = new EventCollector(); + d2.subscribe((event) => { + if (event.type === DocEventType.Snapshot) { + eventCollector.add(d2.getRoot().counter.getValue() as number); + } + }); + + d1.update((r) => (r.counter = new Counter(yorkie.IntType, 0))); + await c1.sync(); + await c2.sync(); + + // 01. c1 increases the counter for creating snapshot. + for (let i = 0; i < DefaultSnapshotThreshold; i++) { + d1.update((r) => r.counter.increase(1)); + } + await c1.sync(); + + // 02. c2 receives the snapshot and increases the counter simultaneously. + c2.sync(); + d2.update((r) => r.counter.increase(1)); + + await eventCollector.waitAndVerifyNthEvent( + 1, + DefaultSnapshotThreshold + 1, + ); + }, task.name); + }); });