Merge pull request #18930 from overleaf/em-resync-ranges

Fix resyncs when diffs move ranges

GitOrigin-RevId: 121c3a16cf19649538445e6ed8bc0a1129735eb9

services/project-history/app/js/SyncManager.js

@@ -7,7 +7,7 @@ import Settings from '@overleaf/settings'
 import logger from '@overleaf/logger'
 import Metrics from '@overleaf/metrics'
 import OError from '@overleaf/o-error'
-import { File } from 'overleaf-editor-core'
+import { File, Range } from 'overleaf-editor-core'
 import { SyncError } from './Errors.js'
 import { db, ObjectId } from './mongodb.js'
 import * as SnapshotManager from './SnapshotManager.js'
@@ -19,7 +19,7 @@ import * as ErrorRecorder from './ErrorRecorder.js'
 import * as RedisManager from './RedisManager.js'
 import * as HistoryStoreManager from './HistoryStoreManager.js'
 import * as HashManager from './HashManager.js'
-import { isInsert } from './Utils.js'
+import { isInsert, isDelete } from './Utils.js'
 
 /**
  * @typedef {import('overleaf-editor-core').Comment} HistoryComment
@@ -637,12 +637,24 @@ class SyncUpdateExpander {
     }
 
     if (!hashesMatch) {
-      await this.queueUpdateForOutOfSyncContent(
+      const expandedUpdate = await this.queueUpdateForOutOfSyncContent(
         update,
         pathname,
         persistedContent,
         expectedContent
       )
+      if (expandedUpdate != null) {
+        // Adjust the ranges for the changes that have been made to the content
+        for (const op of expandedUpdate.op) {
+          if (isInsert(op)) {
+            file.getComments().applyInsert(new Range(op.p, op.i.length))
+            file.getTrackedChanges().applyInsert(op.p, op.i)
+          } else if (isDelete(op)) {
+            file.getComments().applyDelete(new Range(op.p, op.d.length))
+            file.getTrackedChanges().applyDelete(op.p, op.d.length)
+          }
+        }
+      }
     }
 
     const persistedComments = file.getComments().toArray()
@@ -683,7 +695,7 @@ class SyncUpdateExpander {
       expectedContent
     )
     if (op.length === 0) {
-      return
+      return null
     }
     const expandedUpdate = {
       doc: update.doc,
@@ -704,6 +716,7 @@ class SyncUpdateExpander {
     Metrics.inc('project_history_resync_operation', 1, {
       status: 'update text file contents',
     })
+    return expandedUpdate
   }
 
   /**

services/project-history/app/js/types.ts

@@ -135,12 +135,14 @@ export type CommentOp = {
   resolved?: boolean
 }
 
-export type UpdateWithBlob = {
-  update: Update
-  blobHashes: {
-    file: string
-    ranges?: string
-  }
+export type UpdateWithBlob<T extends Update = Update> = {
+  update: T
+  blobHashes: T extends AddDocUpdate | AddFileUpdate
+    ? {
+        file: string
+        ranges?: string
+      }
+    : never
 }
 
 export type RawOrigin = {

services/project-history/test/unit/js/SyncManager/SyncManagerTests.js

@@ -2,7 +2,7 @@ import sinon from 'sinon'
 import { expect } from 'chai'
 import mongodb from 'mongodb-legacy'
 import tk from 'timekeeper'
-import { Comment, TrackedChange, Range } from 'overleaf-editor-core'
+import { File, Comment, TrackedChange, Range } from 'overleaf-editor-core'
 import { strict as esmock } from 'esmock'
 const { ObjectId } = mongodb
 
@@ -491,17 +491,7 @@ describe('SyncManager', function () {
         path: '1.png',
         _hash: 'abcde',
       }
-      this.loadedSnapshotDoc = {
-        isEditable: sinon.stub().returns(true),
-        getContent: sinon.stub().returns(this.persistedDocContent),
-        getComments: sinon
-          .stub()
-          .returns({ toArray: sinon.stub().returns([]) }),
-        getTrackedChanges: sinon
-          .stub()
-          .returns({ asSorted: sinon.stub().returns([]) }),
-        getHash: sinon.stub().returns(null),
-      }
+      this.loadedSnapshotDoc = File.fromString(this.persistedDocContent)
       this.fileMap = {
         'main.tex': {
           isEditable: sinon.stub().returns(true),
@@ -1050,10 +1040,11 @@ describe('SyncManager', function () {
               [this.persistedDoc],
               [this.persistedFile]
             ),
-            docContentSyncUpdate(this.persistedDoc, 'a'),
+            docContentSyncUpdate(this.persistedDoc, 'the quick brown fox'),
           ]
-          this.loadedSnapshotDoc.getContent.returns('stored content')
-          this.UpdateCompressor.diffAsShareJsOps.returns([{ d: 'sdf', p: 1 }])
+          this.UpdateCompressor.diffAsShareJsOps.returns([
+            { d: ' jumps over the lazy dog', p: 19 },
+          ])
           this.expandedUpdates =
             await this.SyncManager.promises.expandSyncUpdates(
               this.projectId,
@@ -1067,10 +1058,10 @@ describe('SyncManager', function () {
           expect(this.expandedUpdates).to.deep.equal([
             {
               doc: this.persistedDoc.doc,
-              op: [{ d: 'sdf', p: 1 }],
+              op: [{ d: ' jumps over the lazy dog', p: 19 }],
               meta: {
                 pathname: this.persistedDoc.path,
-                doc_length: 'stored content'.length,
+                doc_length: this.persistedDocContent.length,
                 resync: true,
                 ts: TIMESTAMP,
                 origin: { kind: 'history-resync' },
@@ -1084,14 +1075,12 @@ describe('SyncManager', function () {
 
     describe('syncing comments', function () {
       beforeEach(function () {
-        this.loadedSnapshotDoc.getComments.returns({
-          toArray: sinon
-            .stub()
-            .returns([
-              new Comment('comment1', [new Range(4, 5)]),
-              new Comment('comment2', [new Range(10, 5)], true),
-            ]),
-        })
+        this.loadedSnapshotDoc
+          .getComments()
+          .add(new Comment('comment1', [new Range(4, 5)]))
+        this.loadedSnapshotDoc
+          .getComments()
+          .add(new Comment('comment2', [new Range(10, 5)], true))
         this.comments = [
           makeComment('comment1', 4, 'quick'),
           makeComment('comment2', 10, 'brown'),
@@ -1288,11 +1277,12 @@ describe('SyncManager', function () {
       })
 
       it('treats zero length comments as detached comments', async function () {
-        this.loadedSnapshotDoc.getComments.returns({
-          toArray: sinon.stub().returns([new Comment('comment1', [])]),
-        })
-        this.comments = [makeComment('comment1', 16, '')]
-        this.resolvedCommentIds = []
+        this.loadedSnapshotDoc.getComments().add(new Comment('comment1', []))
+        this.comments = [
+          makeComment('comment1', 16, ''),
+          makeComment('comment2', 10, 'brown'),
+        ]
+        this.resolvedCommentIds = ['comment2']
 
         const updates = [
           docContentSyncUpdate(
@@ -1315,34 +1305,101 @@ describe('SyncManager', function () {
 
         expect(expandedUpdates).to.deep.equal([])
       })
+
+      it('adjusts comment positions when the underlying text has changed', async function () {
+        const updates = [
+          docContentSyncUpdate(
+            this.persistedDoc,
+            'quick brown fox',
+            {
+              comments: [
+                makeComment('comment1', 0, 'quick'),
+                makeComment('comment2', 12, 'fox'),
+              ],
+            },
+            this.resolvedCommentIds
+          ),
+        ]
+        this.UpdateCompressor.diffAsShareJsOps.returns([
+          { d: 'the ', p: 0 },
+          { d: ' jumps over the lazy dog', p: 15 },
+        ])
+        const expandedUpdates =
+          await this.SyncManager.promises.expandSyncUpdates(
+            this.projectId,
+            this.historyId,
+            updates,
+            this.extendLock
+          )
+        expect(expandedUpdates).to.deep.equal([
+          {
+            doc: this.persistedDoc.doc,
+            op: [
+              { d: 'the ', p: 0 },
+              { d: ' jumps over the lazy dog', p: 15 },
+            ],
+            meta: {
+              pathname: this.persistedDoc.path,
+              doc_length: this.persistedDocContent.length,
+              resync: true,
+              ts: TIMESTAMP,
+              origin: { kind: 'history-resync' },
+            },
+          },
+          {
+            doc: this.persistedDoc.doc,
+            op: [
+              {
+                c: 'fox',
+                p: 12,
+                t: 'comment2',
+                resolved: true,
+              },
+            ],
+            meta: {
+              origin: {
+                kind: 'history-resync',
+              },
+              pathname: this.persistedDoc.path,
+              resync: true,
+              ts: TIMESTAMP,
+              doc_length: 'quick brown fox'.length,
+            },
+          },
+        ])
+      })
     })
 
     describe('syncing tracked changes', function () {
       beforeEach(function () {
-        this.loadedSnapshotDoc.getTrackedChanges.returns({
-          asSorted: sinon.stub().returns([
-            new TrackedChange(new Range(4, 6), {
-              type: 'delete',
-              userId: USER_ID,
-              ts: new Date(TIMESTAMP),
-            }),
-            new TrackedChange(new Range(10, 6), {
-              type: 'insert',
-              userId: USER_ID,
-              ts: new Date(TIMESTAMP),
-            }),
-            new TrackedChange(new Range(20, 6), {
-              type: 'delete',
-              userId: USER_ID,
-              ts: new Date(TIMESTAMP),
-            }),
-            new TrackedChange(new Range(40, 3), {
-              type: 'insert',
-              userId: USER_ID,
-              ts: new Date(TIMESTAMP),
-            }),
-          ]),
-        })
+        this.loadedSnapshotDoc.getTrackedChanges().add(
+          new TrackedChange(new Range(4, 6), {
+            type: 'delete',
+            userId: USER_ID,
+            ts: new Date(TIMESTAMP),
+          })
+        )
+        this.loadedSnapshotDoc.getTrackedChanges().add(
+          new TrackedChange(new Range(10, 6), {
+            type: 'insert',
+            userId: USER_ID,
+            ts: new Date(TIMESTAMP),
+          })
+        )
+        this.loadedSnapshotDoc.getTrackedChanges().add(
+          new TrackedChange(new Range(20, 6), {
+            type: 'delete',
+            userId: USER_ID,
+            ts: new Date(TIMESTAMP),
+          })
+        )
+        this.loadedSnapshotDoc.getTrackedChanges().add(
+          new TrackedChange(new Range(40, 3), {
+            type: 'insert',
+            userId: USER_ID,
+            ts: new Date(TIMESTAMP),
+          })
+        )
         this.changes = [
           makeTrackedChange('td1', { p: 4, d: 'quick ' }),
           makeTrackedChange('ti1', { p: 4, hpos: 10, i: 'brown ' }),
@@ -1515,6 +1572,50 @@ describe('SyncManager', function () {
           },
         ])
       })
+
+      it('adjusts tracked change positions when the underlying text has changed', async function () {
+        const updates = [
+          docContentSyncUpdate(
+            this.persistedDoc,
+            'every fox jumps over the lazy dog',
+            {
+              changes: [
+                makeTrackedChange('ti1', { p: 5, i: ' ' }), // the space after every is still a tracked insert
+                makeTrackedChange('td2', { p: 10, d: 'jumps ' }),
+                makeTrackedChange('ti2', { p: 24, hpos: 30, i: 'dog' }),
+              ],
+            },
+            this.resolvedCommentIds
+          ),
+        ]
+        this.UpdateCompressor.diffAsShareJsOps.returns([
+          { d: 'the quick brown', p: 0 },
+          { i: 'every', p: 0 },
+        ])
+        const expandedUpdates =
+          await this.SyncManager.promises.expandSyncUpdates(
+            this.projectId,
+            this.historyId,
+            updates,
+            this.extendLock
+          )
+        expect(expandedUpdates).to.deep.equal([
+          {
+            doc: this.persistedDoc.doc,
+            op: [
+              { d: 'the quick brown', p: 0 },
+              { i: 'every', p: 0 },
+            ],
+            meta: {
+              pathname: this.persistedDoc.path,
+              doc_length: this.persistedDocContent.length,
+              resync: true,
+              ts: TIMESTAMP,
+              origin: { kind: 'history-resync' },
+            },
+          },
+        ])
+      })
     })
   })
 })