Merge pull request #23089 from overleaf/em-fix-ranges-snapshot

Fix edge cases when restoring comments

GitOrigin-RevId: 6ce2426bfb7233a3c0915bcc8c53bf0588702847

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

@@ -124,7 +124,6 @@ async function getRangesSnapshot(projectId, version, pathname) {
   )
   const docUpdaterCompatibleComments = []
   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
@@ -138,69 +137,60 @@ async function getRangesSnapshot(projectId, version, pathname) {
       })
       continue
     }
-    for (const commentRange of comment.ranges) {
-      let commentRangeContent = ''
-      let offsetFromOverlappingRangeAtStart = 0
-      while (
-        trackedDeletionIndex < trackedDeletions.length &&
-        trackedDeletions[trackedDeletionIndex].range.start <
-          commentRange.start &&
-        trackedDeletions[trackedDeletionIndex].range.end <= commentRange.start
-      ) {
-        // Skip over tracked deletions that are before the current comment range
-        trackedDeletionOffset +=
-          trackedDeletions[trackedDeletionIndex].range.length
-        trackedDeletionIndex++
-      }
 
-      if (
-        trackedDeletions[trackedDeletionIndex]?.range.start < commentRange.start
-      ) {
-        // There's overlap with a tracked deletion, move the position left and
-        // truncate the overlap
-        offsetFromOverlappingRangeAtStart =
-          commentRange.start -
-          trackedDeletions[trackedDeletionIndex].range.start
-      }
+    // Consider a multiple range comment as a single comment that joins all its
+    // ranges
+    const commentStart = comment.ranges[0].start
+    const commentEnd = comment.ranges[comment.ranges.length - 1].end
+
+    let commentContent = ''
+    // Docupdater position
+    let position = commentStart
+    while (trackedDeletions[trackedDeletionIndex]?.range.end <= commentStart) {
+      // Skip over tracked deletions that are before the current comment range
+      position -= trackedDeletions[trackedDeletionIndex].range.length
+      trackedDeletionIndex++
+    }
 
-      // The position of the comment in the document after tracked deletions
-      const position =
-        commentRange.start -
-        trackedDeletionOffset -
-        offsetFromOverlappingRangeAtStart
+    if (trackedDeletions[trackedDeletionIndex]?.range.start < commentStart) {
+      // There's overlap with a tracked deletion, move the position left and
+      // truncate the overlap
+      position -=
+        commentStart - trackedDeletions[trackedDeletionIndex].range.start
+    }
+
+    // Cursor in the history content
+    let cursor = commentStart
+    while (cursor < commentEnd) {
+      const trackedDeletion = trackedDeletions[trackedDeletionIndex]
+      if (!trackedDeletion || trackedDeletion.range.start >= commentEnd) {
+        // We've run out of relevant tracked changes
+        commentContent += content.slice(cursor, commentEnd)
+        break
+      }
+      if (trackedDeletion.range.start > cursor) {
+        // There's a gap between the current cursor and the tracked deletion
+        commentContent += content.slice(cursor, trackedDeletion.range.start)
+      }
 
-      let cursor = commentRange.start
-      while (cursor < commentRange.end) {
-        const trackedDeletion = trackedDeletions[trackedDeletionIndex]
-        if (
-          !trackedDeletion ||
-          trackedDeletion.range.start >= commentRange.end
-        ) {
-          // We've run out of relevant tracked changes
-          commentRangeContent += content.slice(cursor, commentRange.end)
-          break
-        }
-        if (trackedDeletion.range.start > cursor) {
-          // There's a gap between the current cursor and the tracked deletion
-          commentRangeContent += content.slice(
-            cursor,
-            trackedDeletion.range.start
-          )
-        }
+      if (trackedDeletion.range.end <= commentEnd) {
         // Skip to the end of the tracked delete
         cursor = trackedDeletion.range.end
         trackedDeletionIndex++
-        trackedDeletionOffset += trackedDeletion.range.length
+      } else {
+        // We're done with that comment
+        break
       }
-      docUpdaterCompatibleComments.push({
-        op: {
-          p: position,
-          c: commentRangeContent,
-          t: comment.id,
-          resolved: comment.resolved,
-        },
-      })
     }
+    docUpdaterCompatibleComments.push({
+      op: {
+        p: position,
+        c: commentContent,
+        t: comment.id,
+        resolved: comment.resolved,
+      },
+      id: comment.id,
+    })
   }
 
   return {

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

@@ -723,10 +723,10 @@ Four five six\
               },
             },
             {
-              // 'er th'
+              // 'er the la'
               range: {
                 pos: 28,
-                length: 5,
+                length: 9,
               },
               tracking: {
                 type: 'delete',
@@ -754,10 +754,23 @@ Four five six\
                   pos: 26,
                   length: 4,
                 },
+                // 'lazy'
+                {
+                  pos: 35,
+                  length: 4,
+                },
               ],
               resolved: false,
             },
             { id: 'comment-2', ranges: [], resolved: true },
+            {
+              id: 'comment-3',
+              ranges: [
+                // 'q'
+                { pos: 4, length: 1 },
+              ],
+              resolved: true,
+            },
           ],
         })
         this.data = await this.SnapshotManager.promises.getRangesSnapshot(
@@ -769,36 +782,29 @@ Four five six\
 
       it('should move the comment to the start of the tracked delete and remove overlapping text', function () {
         expect(this.data.comments[0].op.p).to.eq(2)
-        expect(this.data.comments[0].op.c).to.eq('ck')
-      })
-
-      it('should remove overlapping text in middle of comment', function () {
-        expect(this.data.comments[1].op.p).to.eq(5)
-        expect(this.data.comments[1].op.c).to.eq('bown')
-      })
-
-      it('should remove overlapping text at end of comment', function () {
-        expect(this.data.comments[2].op.p).to.eq(20)
-        expect(this.data.comments[2].op.c).to.eq('ov')
+        expect(this.data.comments[0].op.c).to.eq('ck bown fox jumps ovzy')
       })
 
       it('should put resolved status in op', function () {
         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
+        expect(this.data.comments[1].op.resolved).to.be.true
+        expect(this.data.comments[2].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')
+        expect(this.data.comments[1].op.t).to.eq('comment-2')
+        expect(this.data.comments[2].op.t).to.eq('comment-3')
       })
 
-      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('')
+      it('should translate detached comment to zero length op', function () {
+        expect(this.data.comments[1].op.p).to.eq(0)
+        expect(this.data.comments[1].op.c).to.eq('')
+      })
+
+      it('should position a comment entirely in a tracked delete next to the tracked delete', function () {
+        expect(this.data.comments[2].op.p).to.eq(2)
+        expect(this.data.comments[2].op.c).to.eq('')
       })
     })
 
@@ -936,43 +942,21 @@ Four five six\
           comments: [
             {
               op: {
-                c: '',
-                p: 4,
-                t: 'comment-1',
-                resolved: false,
-              },
-            },
-            {
-              op: {
-                c: 'brown',
+                c: 'brown fox jumps over the ',
                 p: 4,
                 t: 'comment-1',
                 resolved: false,
               },
+              id: 'comment-1',
             },
             {
               op: {
-                c: '',
-                p: 29,
-                t: 'comment-1',
-                resolved: false,
-              },
-            },
-            {
-              op: {
-                c: 'the',
+                c: 'the brown fox jumps over the',
                 p: 0,
                 t: 'comment-2',
                 resolved: true,
               },
-            },
-            {
-              op: {
-                c: 'the',
-                p: 25,
-                t: 'comment-2',
-                resolved: true,
-              },
+              id: 'comment-2',
             },
           ],
         })