Merge pull request #18659 from overleaf/em-crop-comments-tracked-deletes

Crop comments when processing tracked deletes

GitOrigin-RevId: 662c9ed86a8ed4959d1671ce466548487f334f45

libraries/overleaf-editor-core/lib/operation/add_comment_operation.js

@@ -23,6 +23,12 @@ class AddCommentOperation extends EditOperation {
   constructor(commentId, ranges, resolved = false) {
     super()
 
+    for (const range of ranges) {
+      if (range.isEmpty()) {
+        throw new Error("AddCommentOperation can't be built with empty ranges")
+      }
+    }
+
     /** @readonly */
     this.commentId = commentId
 

services/document-updater/app/js/RangesManager.js

@@ -56,20 +56,36 @@ const RangesManager = {
       RangesManager._emptyRangesCount(rangesTracker)
     const historyUpdates = []
     for (const update of updates) {
-      rangesTracker.track_changes = Boolean(update.meta?.tc)
+      const trackingChanges = Boolean(update.meta?.tc)
+      rangesTracker.track_changes = trackingChanges
       if (update.meta?.tc) {
         rangesTracker.setIdSeed(update.meta.tc)
       }
       const historyOps = []
       for (const op of update.op) {
+        let croppedCommentOps = []
         if (opts.historyRangesSupport) {
           historyOps.push(
             getHistoryOp(op, rangesTracker.comments, rangesTracker.changes)
           )
+          if (isDelete(op) && trackingChanges) {
+            // If a tracked delete overlaps a comment, the comment must be
+            // cropped. The extent of the cropping is calculated before the
+            // delete is applied, but the cropping operations are applied
+            // later, after the delete is applied.
+            croppedCommentOps = getCroppedCommentOps(op, rangesTracker.comments)
+          }
         } else if (isInsert(op) || isDelete(op)) {
           historyOps.push(op)
         }
         rangesTracker.applyOp(op, { user_id: update.meta?.user_id })
+        if (croppedCommentOps.length > 0) {
+          historyOps.push(
+            ...croppedCommentOps.map(op =>
+              getHistoryOpForComment(op, rangesTracker.changes)
+            )
+          )
+        }
       }
       if (historyOps.length > 0) {
         historyUpdates.push({ ...update, op: historyOps })
@@ -486,4 +502,70 @@ function getHistoryOpForComment(op, changes) {
   return historyOp
 }
 
+/**
+ * Return the ops necessary to properly crop comments when a tracked delete is
+ * received
+ *
+ * The editor treats a tracked delete as a proper delete and updates the
+ * comment range accordingly. The history doesn't do that and remembers the
+ * extent of the comment in the tracked delete. In order to keep the history
+ * consistent with the editor, we'll send ops that will crop the comment in
+ * the history.
+ *
+ * @param {DeleteOp} op
+ * @param {Comment[]} comments
+ * @returns {CommentOp[]}
+ */
+function getCroppedCommentOps(op, comments) {
+  const deleteStart = op.p
+  const deleteLength = op.d.length
+  const deleteEnd = deleteStart + deleteLength
+
+  /** @type {HistoryCommentOp[]} */
+  const historyCommentOps = []
+  for (const comment of comments) {
+    const commentStart = comment.op.p
+    const commentLength = comment.op.c.length
+    const commentEnd = commentStart + commentLength
+
+    if (deleteStart <= commentStart && deleteEnd > commentStart) {
+      // The comment overlaps the start of the comment or all of it.
+      const overlapLength = Math.min(deleteEnd, commentEnd) - commentStart
+
+      /** @type {CommentOp} */
+      const commentOp = {
+        p: deleteStart,
+        c: comment.op.c.slice(overlapLength),
+        t: comment.op.t,
+      }
+      if (comment.op.resolved) {
+        commentOp.resolved = true
+      }
+
+      historyCommentOps.push(commentOp)
+    } else if (
+      deleteStart > commentStart &&
+      deleteStart < commentEnd &&
+      deleteEnd >= commentEnd
+    ) {
+      // The comment overlaps the end of the comment.
+      const overlapLength = commentEnd - deleteStart
+
+      /** @type {CommentOp} */
+      const commentOp = {
+        p: commentStart,
+        c: comment.op.c.slice(0, -overlapLength),
+        t: comment.op.t,
+      }
+      if (comment.op.resolved) {
+        commentOp.resolved = true
+      }
+
+      historyCommentOps.push(commentOp)
+    }
+  }
+
+  return historyCommentOps
+}
+
 module.exports = RangesManager

services/document-updater/test/unit/js/RangesManager/RangesManagerTests.js

@@ -518,6 +518,123 @@ describe('RangesManager', function () {
           ])
         })
       })
+
+      describe('tracked delete that overlaps the start of a comment', function () {
+        beforeEach(function () {
+          // original text is "one three four five"
+          this.ranges = {
+            comments: makeRanges([{ c: 'three', p: 4, t: 'comment-id-1' }]),
+          }
+          this.updates = makeUpdates([{ d: 'ne thr', p: 1 }], {
+            tc: 'tracking-id',
+          })
+          this.newDocLines = ['oee four five']
+          this.result = this.RangesManager.applyUpdate(
+            this.project_id,
+            this.doc_id,
+            this.ranges,
+            this.updates,
+            this.newDocLines,
+            { historyRangesSupport: true }
+          )
+        })
+
+        it('should crop the beginning of the comment', function () {
+          expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([
+            [
+              { d: 'ne thr', p: 1 },
+              { c: 'ee', p: 1, hpos: 7, t: 'comment-id-1' },
+            ],
+          ])
+        })
+      })
+
+      describe('tracked delete that overlaps a whole comment', function () {
+        beforeEach(function () {
+          // original text is "one three four five"
+          this.ranges = {
+            comments: makeRanges([{ c: 'three', p: 4, t: 'comment-id-1' }]),
+          }
+          this.updates = makeUpdates([{ d: 'ne three f', p: 1 }], {
+            tc: 'tracking-id',
+          })
+          this.newDocLines = ['oour five']
+          this.result = this.RangesManager.applyUpdate(
+            this.project_id,
+            this.doc_id,
+            this.ranges,
+            this.updates,
+            this.newDocLines,
+            { historyRangesSupport: true }
+          )
+        })
+
+        it('should crop the beginning of the comment', function () {
+          expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([
+            [
+              { d: 'ne three f', p: 1 },
+              { c: '', p: 1, hpos: 11, t: 'comment-id-1' },
+            ],
+          ])
+        })
+      })
+
+      describe('tracked delete that overlaps the end of a comment', function () {
+        beforeEach(function () {
+          // original text is "one three four five"
+          this.ranges = {
+            comments: makeRanges([{ c: 'three', p: 4, t: 'comment-id-1' }]),
+          }
+          this.updates = makeUpdates([{ d: 'ee f', p: 7 }], {
+            tc: 'tracking-id',
+          })
+          this.newDocLines = ['one throur five']
+          this.result = this.RangesManager.applyUpdate(
+            this.project_id,
+            this.doc_id,
+            this.ranges,
+            this.updates,
+            this.newDocLines,
+            { historyRangesSupport: true }
+          )
+        })
+
+        it('should crop the end of the comment', function () {
+          expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([
+            [
+              { d: 'ee f', p: 7 },
+              { c: 'thr', p: 4, t: 'comment-id-1' },
+            ],
+          ])
+        })
+      })
+
+      describe('tracked delete that overlaps the inside of a comment', function () {
+        beforeEach(function () {
+          // original text is "one three four five"
+          this.ranges = {
+            comments: makeRanges([{ c: 'three', p: 4, t: 'comment-id-1' }]),
+          }
+          this.updates = makeUpdates([{ d: 'hre', p: 5 }], {
+            tc: 'tracking-id',
+          })
+          this.newDocLines = ['one te four five']
+          this.result = this.RangesManager.applyUpdate(
+            this.project_id,
+            this.doc_id,
+            this.ranges,
+            this.updates,
+            this.newDocLines,
+            { historyRangesSupport: true }
+          )
+        })
+
+        it('should not crop the comment', function () {
+          expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([
+            [{ d: 'hre', p: 5 }],
+          ])
+        })
+      })
     })
   })
 

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

@@ -280,15 +280,11 @@ class OperationsBuilder {
       this.pushTextOperation()
 
       // Add a comment operation
+      const commentLength = op.hlen ?? op.c.length
       const commentOp = {
         pathname: this.pathname,
         commentId: op.t,
-        ranges: [
-          {
-            pos,
-            length: op.hlen ?? op.c.length,
-          },
-        ],
+        ranges: commentLength > 0 ? [{ pos, length: commentLength }] : [],
       }
       if ('resolved' in op) {
         commentOp.resolved = op.resolved

services/project-history/test/unit/js/UpdateTranslator/UpdateTranslatorTests.js

@@ -776,6 +776,7 @@ describe('UpdateTranslator', function () {
                 { p: 3, d: 'bar' },
                 { p: 5, c: 'comment this', t: 'comment-id-1' },
                 { p: 7, c: 'another comment', t: 'comment-id-2' },
+                { p: 9, c: '', t: 'comment-id-3' },
                 { p: 10, i: 'baz' },
               ],
               v: this.version,
@@ -812,6 +813,11 @@ describe('UpdateTranslator', function () {
                 commentId: 'comment-id-2',
                 ranges: [{ pos: 7, length: 15 }],
               },
+              {
+                pathname: 'main.tex',
+                commentId: 'comment-id-3',
+                ranges: [],
+              },
               {
                 pathname: 'main.tex',
                 textOperation: [10, 'baz', 10],