Merge pull request #22650 from overleaf/em-tracked-deletes-at-same-position

Handle multiple tracked deletes at same position

GitOrigin-RevId: 3cbf1c418bcd50cf08e1b90ce6ba3bc480236079

libraries/ranges-tracker/index.cjs

@@ -316,7 +316,7 @@ class RangesTracker {
     const movedChanges = []
     const removeChanges = []
     const newChanges = []
-
+    const trackedDeletesAtOpPosition = []
     for (let i = 0; i < this.changes.length; i++) {
       change = this.changes[i]
       const changeStart = change.op.p
@@ -327,13 +327,15 @@ class RangesTracker {
           change.op.p += opLength
           movedChanges.push(change)
         } else if (opStart === changeStart) {
-          // If we are undoing, then we want to cancel any existing delete ranges if we can.
-          // Check if the insert matches the start of the delete, and just remove it from the delete instead if so.
           if (
+            !(op.orderedRejections && alreadyMerged) &&
             undoing &&
             change.op.d.length >= op.i.length &&
             change.op.d.slice(0, op.i.length) === op.i
           ) {
+            // If we are undoing, then we want to reject any existing tracked delete if we can.
+            // Check if the insert matches the start of the delete, and just
+            // remove it from the delete instead if so.
             change.op.d = change.op.d.slice(op.i.length)
             change.op.p += op.i.length
             if (change.op.d === '') {
@@ -342,9 +344,27 @@ class RangesTracker {
               movedChanges.push(change)
             }
             alreadyMerged = true
+
+            if (op.orderedRejections) {
+              // Any tracked delete that came before this tracked delete
+              // rejection was moved after the incoming insert. Move them back
+              // so that they appear before the tracked delete rejection.
+              for (const trackedDelete of trackedDeletesAtOpPosition) {
+                trackedDelete.op.p -= opLength
+              }
+            }
           } else {
+            // We're not rejecting that tracked delete. Move it after the
+            // insert.
             change.op.p += opLength
             movedChanges.push(change)
+
+            // Keep track of tracked deletes that are at the same position as the
+            // insert. If we find a tracked delete to reject, we'll want to
+            // reposition them.
+            if (!alreadyMerged) {
+              trackedDeletesAtOpPosition.push(change)
+            }
           }
         }
       } else if (change.op.i != null) {
@@ -624,9 +644,13 @@ class RangesTracker {
   }
 
   _addOp(op, metadata) {
+    // Don't take a reference to the existing op since we'll modify this in place with future changes
+    op = this._clone(op)
+    // TODO: Remove this when the orderedRejections transition is over
+    delete op.orderedRejections
     const change = {
       id: this.newId(),
-      op: this._clone(op), // Don't take a reference to the existing op since we'll modify this in place with future changes
+      op,
       metadata: this._clone(metadata),
     }
     this.changes.push(change)

libraries/ranges-tracker/test/unit/ranges-tracker-test.js

@@ -4,6 +4,7 @@ const RangesTracker = require('../..')
 describe('RangesTracker', function () {
   describe('with duplicate change ids', function () {
     beforeEach(function () {
+      this.comments = []
       this.changes = [
         { id: 'id1', op: { p: 1, i: 'hello' } },
         { id: 'id2', op: { p: 10, i: 'world' } },
@@ -26,4 +27,176 @@ describe('RangesTracker', function () {
       expect(this.rangesTracker.changes).to.deep.equal([this.changes[2]])
     })
   })
+
+  describe('with multiple tracked deletes at the same position', function () {
+    beforeEach(function () {
+      this.comments = []
+      this.changes = [
+        { id: 'id1', op: { p: 33, d: 'before' } },
+        { id: 'id2', op: { p: 50, d: 'right before' } },
+        { id: 'id3', op: { p: 50, d: 'this one' } },
+        { id: 'id4', op: { p: 50, d: 'right after' } },
+        { id: 'id5', op: { p: 75, d: 'long after' } },
+      ]
+      this.rangesTracker = new RangesTracker(this.changes, this.comments)
+    })
+
+    describe('with the orderedRejections flag', function () {
+      it('preserves the text order when rejecting changes', function () {
+        this.rangesTracker.applyOp(
+          { p: 50, i: 'this one', u: true, orderedRejections: true },
+          { user_id: 'user-id' }
+        )
+        expect(this.rangesTracker.changes).to.deep.equal([
+          { id: 'id1', op: { p: 33, d: 'before' } },
+          { id: 'id2', op: { p: 50, d: 'right before' } },
+          { id: 'id4', op: { p: 58, d: 'right after' } },
+          { id: 'id5', op: { p: 83, d: 'long after' } },
+        ])
+      })
+
+      it('moves all tracked deletes after the insert if not rejecting changes', function () {
+        this.rangesTracker.applyOp(
+          { p: 50, i: 'some other text', u: true, orderedRejections: true },
+          { user_id: 'user-id' }
+        )
+        expect(this.rangesTracker.changes).to.deep.equal([
+          { id: 'id1', op: { p: 33, d: 'before' } },
+          { id: 'id2', op: { p: 65, d: 'right before' } },
+          { id: 'id3', op: { p: 65, d: 'this one' } },
+          { id: 'id4', op: { p: 65, d: 'right after' } },
+          { id: 'id5', op: { p: 90, d: 'long after' } },
+        ])
+      })
+    })
+
+    describe('without the orderedRejections flag', function () {
+      it('puts the insert before tracked deletes when rejecting changes', function () {
+        this.rangesTracker.applyOp(
+          { p: 50, i: 'this one', u: true },
+          { user_id: 'user-id' }
+        )
+        expect(this.rangesTracker.changes).to.deep.equal([
+          { id: 'id1', op: { p: 33, d: 'before' } },
+          { id: 'id2', op: { p: 58, d: 'right before' } },
+          { id: 'id4', op: { p: 58, d: 'right after' } },
+          { id: 'id5', op: { p: 83, d: 'long after' } },
+        ])
+      })
+
+      it('moves all tracked deletes after the insert if not rejecting changes', function () {
+        this.rangesTracker.applyOp(
+          { p: 50, i: 'some other text', u: true },
+          { user_id: 'user-id' }
+        )
+        expect(this.rangesTracker.changes).to.deep.equal([
+          { id: 'id1', op: { p: 33, d: 'before' } },
+          { id: 'id2', op: { p: 65, d: 'right before' } },
+          { id: 'id3', op: { p: 65, d: 'this one' } },
+          { id: 'id4', op: { p: 65, d: 'right after' } },
+          { id: 'id5', op: { p: 90, d: 'long after' } },
+        ])
+      })
+    })
+  })
+
+  describe('with multiple tracked deletes at the same position with the same content', function () {
+    beforeEach(function () {
+      this.comments = []
+      this.changes = [
+        { id: 'id1', op: { p: 10, d: 'cat' } },
+        { id: 'id2', op: { p: 10, d: 'giraffe' } },
+        { id: 'id3', op: { p: 10, d: 'cat' } },
+        { id: 'id4', op: { p: 10, d: 'giraffe' } },
+      ]
+      this.rangesTracker = new RangesTracker(this.changes, this.comments)
+    })
+
+    describe('with the orderedRejections flag', function () {
+      it('removes only the first matching tracked delete', function () {
+        this.rangesTracker.applyOp(
+          { p: 10, i: 'giraffe', u: true, orderedRejections: true },
+          { user_id: 'user-id' }
+        )
+        expect(this.rangesTracker.changes).to.deep.equal([
+          { id: 'id1', op: { p: 10, d: 'cat' } },
+          { id: 'id3', op: { p: 17, d: 'cat' } },
+          { id: 'id4', op: { p: 17, d: 'giraffe' } },
+        ])
+      })
+    })
+
+    describe('without the orderedRejections flag', function () {
+      it('removes all matching tracked delete', function () {
+        this.rangesTracker.applyOp(
+          { p: 10, i: 'giraffe', u: true },
+          { user_id: 'user-id' }
+        )
+        expect(this.rangesTracker.changes).to.deep.equal([
+          { id: 'id1', op: { p: 17, d: 'cat' } },
+          { id: 'id3', op: { p: 17, d: 'cat' } },
+        ])
+      })
+    })
+  })
+
+  describe('with a tracked insert at the same position as a tracked delete', function () {
+    beforeEach(function () {
+      this.comments = []
+      this.changes = [
+        {
+          id: 'id1',
+          op: { p: 5, d: 'before' },
+          metadata: { user_id: 'user-id' },
+        },
+        {
+          id: 'id2',
+          op: { p: 10, d: 'delete' },
+          metadata: { user_id: 'user-id' },
+        },
+        {
+          id: 'id3',
+          op: { p: 10, i: 'insert' },
+          metadata: { user_id: 'user-id' },
+        },
+      ]
+      this.rangesTracker = new RangesTracker(this.changes, this.comments)
+    })
+
+    describe('with the orderedRejections flag', function () {
+      it('places a tracked insert at the same position before both the delete and the insert', function () {
+        this.rangesTracker.track_changes = true
+        this.rangesTracker.applyOp(
+          { p: 10, i: 'incoming', orderedRejections: true },
+          { user_id: 'user-id' }
+        )
+        expect(
+          this.rangesTracker.changes.map(change => change.op)
+        ).to.deep.equal([
+          { p: 5, d: 'before' },
+          { p: 10, i: 'incoming' },
+          { p: 18, d: 'delete' },
+          { p: 18, i: 'insert' },
+        ])
+      })
+    })
+
+    describe('without the orderedRejections flag', function () {
+      it('places a tracked insert at the same position before both the delete and the insert', function () {
+        this.rangesTracker.track_changes = true
+        this.rangesTracker.applyOp(
+          { p: 10, i: 'incoming' },
+          { user_id: 'user-id' }
+        )
+        expect(
+          this.rangesTracker.changes.map(change => change.op)
+        ).to.deep.equal([
+          { p: 5, d: 'before' },
+          { p: 10, i: 'incoming' },
+          { p: 18, d: 'delete' },
+          { p: 18, i: 'insert' },
+        ])
+      })
+    })
+  })
 })

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

@@ -352,6 +352,12 @@ function getHistoryOpForInsert(op, comments, changes) {
     }
   }
 
+  // If it's determined that the op is a tracked delete rejection, we have to
+  // calculate its proper history position. If multiple tracked deletes are
+  // found at the same position as the insert, the tracked deletes that come
+  // before the tracked delete that was actually rejected offset the history
+  // position.
+  let trackedDeleteRejectionOffset = 0
   for (const change of changes) {
     if (!isDelete(change.op)) {
       // We're only interested in tracked deletes
@@ -362,14 +368,25 @@ function getHistoryOpForInsert(op, comments, changes) {
       // Tracked delete is before the op. Move the op forward.
       hpos += change.op.d.length
     } else if (change.op.p === op.p) {
-      // Tracked delete is at the same position as the op. The insert comes before
-      // the tracked delete so it doesn't move.
+      // Tracked delete is at the same position as the op.
       if (op.u && change.op.d.startsWith(op.i)) {
         // We're undoing and the insert matches the start of the tracked
         // delete. RangesManager treats this as a tracked delete rejection. We
         // will note this in the op so that project-history can take the
         // appropriate action.
         trackedDeleteRejection = true
+
+        // The history must be updated to take into account all preceding
+        // tracked deletes at the same position
+        hpos += trackedDeleteRejectionOffset
+
+        // No need to continue. All subsequent tracked deletes are after the
+        // insert.
+        break
+      } else {
+        // This tracked delete does not match the insert. Note its length in
+        // case we find a tracked delete that matches later.
+        trackedDeleteRejectionOffset += change.op.d.length
       }
     } else {
       // Tracked delete is after the insert. Tracked deletes are ordered, so

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

@@ -323,6 +323,44 @@ describe('RangesManager', function () {
         })
       })
 
+      describe('tracked delete rejections with multiple tracked deletes at the same position', function () {
+        beforeEach(function () {
+          // original text is "one [two ][three ][four ]five"
+          // [] denotes tracked deletes
+          this.ranges = {
+            changes: makeRanges([
+              { d: 'two ', p: 4 },
+              { d: 'three ', p: 4 },
+              { d: 'four ', p: 4 },
+            ]),
+          }
+          this.updates = makeUpdates([{ i: 'three ', p: 4, u: true }])
+          this.newDocLines = ['one three five']
+          this.result = this.RangesManager.applyUpdate(
+            this.project_id,
+            this.doc_id,
+            this.ranges,
+            this.updates,
+            this.newDocLines,
+            { historyRangesSupport: true }
+          )
+        })
+
+        it('should insert the text at the right history position', function () {
+          expect(this.result.historyUpdates.map(x => x.op)).to.deep.equal([
+            [
+              {
+                i: 'three ',
+                p: 4,
+                hpos: 8,
+                u: true,
+                trackedDeleteRejection: true,
+              },
+            ],
+          ])
+        })
+      })
+
       describe('deletes over tracked changes', function () {
         beforeEach(function () {
           // original text is "on[1]e [22](three) f[333]ou[4444]r [55555]five"

services/web/frontend/js/vendor/libs/sharejs.js

@@ -695,6 +695,9 @@ export const { Doc } = (() => {
       var op = { p: pos, i: text };
       if (fromUndo) {
         op.u = true;
+        // TODO: This flag is temporary. It is only necessary while we change
+        // the behaviour of tracked delete rejections in RangesTracker
+        op.orderedRejections = true;
       }
       op = [op];