Merge pull request #23509 from overleaf/em-fix-remove-change

Fix RangesTracker._removeChange() when multiple changes have the same id

GitOrigin-RevId: 2dafcf275b283da320bca70b460aba3051ca9575

libraries/ranges-tracker/index.cjs

@@ -145,11 +145,7 @@ class RangesTracker {
   }
 
   removeChangeId(changeId) {
-    const change = this.getChange(changeId)
-    if (change == null) {
-      return
-    }
-    this._removeChange(change)
+    this.removeChangeIds([changeId])
   }
 
   removeChangeIds(ids) {
@@ -310,6 +306,7 @@ class RangesTracker {
     const opLength = op.i.length
     const opEnd = op.p + opLength
     const undoing = !!op.u
+    const fixedRemoveChange = op.fixedRemoveChange
 
     let alreadyMerged = false
     let previousChange = null
@@ -477,7 +474,11 @@ class RangesTracker {
     }
 
     for (change of removeChanges) {
-      this._removeChange(change)
+      if (fixedRemoveChange) {
+        this._removeChange(change)
+      } else {
+        this._brokenRemoveChange(change)
+      }
     }
 
     for (change of movedChanges) {
@@ -490,6 +491,7 @@ class RangesTracker {
     const opLength = op.d.length
     const opEnd = op.p + opLength
     const removeChanges = []
+    const fixedRemoveChange = op.fixedRemoveChange
     let movedChanges = []
 
     // We might end up modifying our delete op if it merges with existing deletes, or cancels out
@@ -617,7 +619,11 @@ class RangesTracker {
         movedChanges.push(change)
         op.d = '' // stop it being added
       } else {
-        this._removeChange(change)
+        if (fixedRemoveChange) {
+          this._removeChange(change)
+        } else {
+          this._brokenRemoveChange(change)
+        }
       }
     }
 
@@ -633,7 +639,11 @@ class RangesTracker {
       const results = this._scanAndMergeAdjacentUpdates()
       movedChanges = movedChanges.concat(results.movedChanges)
       for (const change of results.removeChanges) {
-        this._removeChange(change)
+        if (fixedRemoveChange) {
+          this._removeChange(change)
+        } else {
+          this._brokenRemoveChange(change)
+        }
         movedChanges = movedChanges.filter(c => c !== change)
       }
     }
@@ -673,6 +683,11 @@ class RangesTracker {
   }
 
   _removeChange(change) {
+    this.changes = this.changes.filter(c => c !== change)
+    this._markAsDirty(change, 'change', 'removed')
+  }
+
+  _brokenRemoveChange(change) {
     this.changes = this.changes.filter(c => c.id !== change.id)
     this._markAsDirty(change, 'change', 'removed')
   }

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

@@ -28,6 +28,101 @@ describe('RangesTracker', function () {
     })
   })
 
+  describe('with duplicate tracked insert ids', function () {
+    beforeEach(function () {
+      this.comments = []
+      this.changes = [
+        { id: 'id1', op: { p: 10, i: 'one' } },
+        { id: 'id1', op: { p: 20, i: 'two' } },
+        { id: 'id1', op: { p: 30, d: 'three' } },
+      ]
+      this.rangesTracker = new RangesTracker(this.changes, this.comments)
+    })
+
+    it("deleting one tracked insert doesn't delete the others", function () {
+      this.rangesTracker.applyOp({ p: 20, d: 'two', fixedRemoveChange: true })
+      expect(this.rangesTracker.changes).to.deep.equal([
+        this.changes[0],
+        this.changes[2],
+      ])
+    })
+  })
+
+  describe('with duplicate tracked delete ids', function () {
+    beforeEach(function () {
+      this.comments = []
+      this.changes = [
+        { id: 'id1', op: { p: 10, d: 'one' } },
+        { id: 'id1', op: { p: 20, d: 'two' } },
+        { id: 'id1', op: { p: 30, d: 'three' } },
+      ]
+      this.rangesTracker = new RangesTracker(this.changes, this.comments)
+    })
+
+    it('deleting over tracked deletes in tracked changes mode removes the tracked deletes covered', function () {
+      this.rangesTracker.track_changes = true
+      this.rangesTracker.applyOp({
+        p: 15,
+        d: '567890123456789012345',
+        fixedRemoveChange: true,
+      })
+      expect(this.rangesTracker.changes.map(c => c.op)).to.deep.equal([
+        { p: 10, d: 'one' },
+        { p: 15, d: '56789two0123456789three012345' },
+      ])
+    })
+
+    it('a tracked delete between two tracked deletes joins them into a single tracked delete', function () {
+      this.rangesTracker.track_changes = true
+      this.rangesTracker.applyOp({
+        p: 20,
+        d: '0123456789',
+        fixedRemoveChange: true,
+      })
+      expect(this.rangesTracker.changes.map(c => c.op)).to.deep.equal([
+        { p: 10, d: 'one' },
+        { p: 20, d: 'two0123456789three' },
+      ])
+    })
+
+    it("rejecting one tracked delete doesn't reject the others", function () {
+      this.rangesTracker.track_changes = true
+      this.rangesTracker.applyOp({
+        p: 20,
+        i: 'two',
+        u: true,
+        fixedRemoveChange: true,
+      })
+      expect(this.rangesTracker.changes.map(c => c.op)).to.deep.equal([
+        { p: 10, d: 'one' },
+        { p: 33, d: 'three' },
+      ])
+    })
+
+    it("rejecting all tracked deletes doesn't introduce tracked inserts", function () {
+      this.rangesTracker.track_changes = true
+      this.rangesTracker.applyOp({
+        p: 10,
+        i: 'one',
+        u: true,
+        fixedRemoveChange: true,
+      })
+      this.rangesTracker.applyOp({
+        p: 23,
+        i: 'two',
+        u: true,
+        fixedRemoveChange: true,
+      })
+      this.rangesTracker.applyOp({
+        p: 36,
+        i: 'three',
+        u: true,
+        fixedRemoveChange: true,
+      })
+      expect(this.rangesTracker.changes.map(c => c.op)).to.deep.equal([])
+    })
+  })
+
   describe('with multiple tracked deletes at the same position', function () {
     beforeEach(function () {
       this.comments = []

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

@@ -699,6 +699,7 @@ export const { Doc } = (() => {
         // TODO: This flag is temporary. It is only necessary while we change
         // the behaviour of tracked delete rejections in RangesTracker
         op.orderedRejections = true;
+        op.fixedRemoveChange = true;
       }
       op = [op];