Merge pull request #18838 from overleaf/mj-history-detached-ranges

[project-history] Handle detached ranges

GitOrigin-RevId: cdbe9b46a03d55fd7b865fdd87092aaad1920c62

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

@@ -91,10 +91,6 @@ export function createRangeBlobDataFromUpdate(update) {
   /** @type {Map<string, {ranges: Range[], resolved: boolean}>} */
   const commentMap = new Map()
   for (const comment of comments) {
-    const range = new Range(
-      comment.op.hpos ?? comment.op.p,
-      comment.op.hlen ?? comment.op.c.length
-    )
     const id = comment.op.t
     if (!commentMap.has(id)) {
       commentMap.set(id, {
@@ -109,7 +105,13 @@ export function createRangeBlobDataFromUpdate(update) {
     if (entry.resolved !== (comment.op.resolved ?? false)) {
       throw new Error('Mismatching resolved status for comment')
     }
-    entry.ranges.push(range)
+
+    const commentLength = comment.op.c.length
+    if (commentLength > 0) {
+      // Empty comments in operations are translated to detached comments
+      const range = new Range(comment.op.hpos ?? comment.op.p, commentLength)
+      entry.ranges.push(range)
+    }
   }
   const commentList = new CommentList(
     [...commentMap.entries()].map(

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

@@ -126,6 +126,18 @@ async function getRangesSnapshot(projectId, version, pathname) {
   for (const comment of comments) {
     trackedDeletionOffset = 0
     let trackedDeletionIndex = 0
+    if (comment.ranges.length === 0) {
+      // Translate detached comments into zero length comments at position 0
+      docUpdaterCompatibleComments.push({
+        op: {
+          p: 0,
+          c: '',
+          t: comment.id,
+          resolved: comment.resolved,
+        },
+      })
+      continue
+    }
     for (const commentRange of comment.ranges) {
       let commentRangeContent = ''
       let offsetFromOverlappingRangeAtStart = 0

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

@@ -906,13 +906,18 @@ class SyncUpdateExpander {
  * @param {Comment} expectedComment
  */
 function commentRangesAreInSync(persistedComment, expectedComment) {
+  const expectedPos = expectedComment.op.hpos ?? expectedComment.op.p
+  const expectedLength = expectedComment.op.hlen ?? expectedComment.op.c.length
+  if (expectedLength === 0) {
+    // A zero length comment from RangesManager is a detached comment in history
+    return persistedComment.ranges.length === 0
+  }
+
   if (persistedComment.ranges.length !== 1) {
     // The editor only supports single range comments
     return false
   }
   const persistedRange = persistedComment.ranges[0]
-  const expectedPos = expectedComment.op.hpos ?? expectedComment.op.p
-  const expectedLength = expectedComment.op.hlen ?? expectedComment.op.c.length
   return (
     persistedRange.pos === expectedPos &&
     persistedRange.length === expectedLength

services/project-history/test/unit/js/HistoryBlobTranslator/HistoryBlobTranslatorTests.js

@@ -66,6 +66,25 @@ describe('HistoryBlobTranslator', function () {
       })
     })
 
+    describe('for update with zero length comments', function () {
+      beforeEach(function () {
+        this.result = createRangeBlobDataFromUpdate(
+          update('pathname', this.text, {
+            changes: [],
+            comments: [
+              { op: { c: '', p: 4, t: 'comment-1', resolved: false } },
+            ],
+          })
+        )
+      })
+      it('should treat them as detached comments', function () {
+        expect(this.result).to.deep.equal({
+          comments: [{ id: 'comment-1', ranges: [] }],
+          trackedChanges: [],
+        })
+      })
+    })
+
     describe('for update with ranges object with only comments', function () {
       it('should return unmoved ranges', function () {
         const result = createRangeBlobDataFromUpdate(

services/project-history/test/unit/js/SnapshotManager/SnapshotManagerTests.js

@@ -757,6 +757,7 @@ Four five six\
               ],
               resolved: false,
             },
+            { id: 'comment-2', ranges: [], resolved: true },
           ],
         })
         this.data = await this.SnapshotManager.promises.getRangesSnapshot(
@@ -785,6 +786,19 @@ Four five six\
         expect(this.data.comments[0].op.resolved).to.be.false
         expect(this.data.comments[1].op.resolved).to.be.false
         expect(this.data.comments[2].op.resolved).to.be.false
+        expect(this.data.comments[3].op.resolved).to.be.true
+      })
+
+      it('should include thread id', function () {
+        expect(this.data.comments[0].op.t).to.eq('comment-1')
+        expect(this.data.comments[1].op.t).to.eq('comment-1')
+        expect(this.data.comments[2].op.t).to.eq('comment-1')
+        expect(this.data.comments[3].op.t).to.eq('comment-2')
+      })
+
+      it('should translated detached comment to zero length op', function () {
+        expect(this.data.comments[3].op.p).to.eq(0)
+        expect(this.data.comments[3].op.c).to.eq('')
       })
     })
 

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

@@ -1286,6 +1286,35 @@ 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 = []
+
+        const updates = [
+          docContentSyncUpdate(
+            this.persistedDoc,
+            this.persistedDocContent,
+            {
+              comments: this.comments,
+            },
+            this.resolvedCommentIds
+          ),
+        ]
+
+        const expandedUpdates =
+          await this.SyncManager.promises.expandSyncUpdates(
+            this.projectId,
+            this.historyId,
+            updates,
+            this.extendLock
+          )
+
+        expect(expandedUpdates).to.deep.equal([])
+      })
     })
 
     describe('syncing tracked changes', function () {