[overleaf-editor-core+project-history] Clean up TrackedChangeList api (#17740)

* [overleaf-editor-core+project-history] Mark TC list backing array as private

* [overleaf-editor-core] Add invariant for overlapping comment ranges

* [overleaf-editor-core] Assert that ranges are non-empty

GitOrigin-RevId: e60a3712eba2326e0767a75a3ffc75333311c057

libraries/overleaf-editor-core/lib/comment.js

@@ -167,7 +167,12 @@ class Comment {
         continue
       }
       const lastMerged = mergedRanges[mergedRanges.length - 1]
-
+      if (lastMerged?.overlaps(range)) {
+        throw new Error('Ranges cannot overlap')
+      }
+      if (range.isEmpty()) {
+        throw new Error('Comment range cannot be empty')
+      }
       if (lastMerged?.canMerge(range)) {
         mergedRanges[mergedRanges.length - 1] = lastMerged.merge(range)
       } else {

libraries/overleaf-editor-core/lib/file_data/string_file_data.js

@@ -74,7 +74,7 @@ class StringFileData extends FileData {
     let content = ''
     let cursor = 0
     if (opts.filterTrackedDeletes) {
-      for (const tc of this.trackedChanges.trackedChanges) {
+      for (const tc of this.trackedChanges.asSorted()) {
         if (tc.tracking.type !== 'delete') {
           continue
         }

libraries/overleaf-editor-core/lib/file_data/tracked_change_list.js

@@ -13,7 +13,10 @@ class TrackedChangeList {
    * @param {TrackedChange[]} trackedChanges
    */
   constructor(trackedChanges) {
-    this.trackedChanges = trackedChanges
+    /**
+     * @type {TrackedChange[]}
+     */
+    this._trackedChanges = trackedChanges
   }
 
   /**
@@ -30,11 +33,20 @@ class TrackedChangeList {
    * @returns {TrackedChangeRawData[]}
    */
   toRaw() {
-    return this.trackedChanges.map(change => change.toRaw())
+    return this._trackedChanges.map(change => change.toRaw())
   }
 
   get length() {
-    return this.trackedChanges.length
+    return this._trackedChanges.length
+  }
+
+  /**
+   * @returns {readonly TrackedChange[]}
+   */
+  asSorted() {
+    // NOTE: Once all code dependent on this is typed, we can just return
+    // _trackedChanges.
+    return Array.from(this._trackedChanges)
   }
 
   /**
@@ -43,7 +55,7 @@ class TrackedChangeList {
    * @returns {TrackedChange[]}
    */
   inRange(range) {
-    return this.trackedChanges.filter(change => range.contains(change.range))
+    return this._trackedChanges.filter(change => range.contains(change.range))
   }
 
   /**
@@ -52,7 +64,7 @@ class TrackedChangeList {
    * @returns {TrackingProps | undefined}
    */
   propsAtRange(range) {
-    return this.trackedChanges.find(change => change.range.contains(range))
+    return this._trackedChanges.find(change => change.range.contains(range))
       ?.tracking
   }
 
@@ -61,7 +73,7 @@ class TrackedChangeList {
    * @param {Range} range
    */
   removeInRange(range) {
-    this.trackedChanges = this.trackedChanges.filter(
+    this._trackedChanges = this._trackedChanges.filter(
       change => !range.contains(change.range)
     )
   }
@@ -71,7 +83,7 @@ class TrackedChangeList {
    * @param {TrackedChange} trackedChange
    */
   add(trackedChange) {
-    this.trackedChanges.push(trackedChange)
+    this._trackedChanges.push(trackedChange)
     this._mergeRanges()
   }
 
@@ -80,22 +92,28 @@ class TrackedChangeList {
    * @returns {void}
    */
   _mergeRanges() {
-    if (this.trackedChanges.length < 2) {
+    if (this._trackedChanges.length < 2) {
       return
     }
     // ranges are non-overlapping so we can sort based on their first indices
-    this.trackedChanges.sort((a, b) => a.range.start - b.range.start)
-    const newTrackedChanges = [this.trackedChanges[0]]
-    for (let i = 1; i < this.trackedChanges.length; i++) {
+    this._trackedChanges.sort((a, b) => a.range.start - b.range.start)
+    const newTrackedChanges = [this._trackedChanges[0]]
+    for (let i = 1; i < this._trackedChanges.length; i++) {
       const last = newTrackedChanges[newTrackedChanges.length - 1]
-      const current = this.trackedChanges[i]
+      const current = this._trackedChanges[i]
+      if (last.range.overlaps(current.range)) {
+        throw new Error('Ranges cannot overlap')
+      }
+      if (current.range.isEmpty()) {
+        throw new Error('Tracked changes range cannot be empty')
+      }
       if (last.canMerge(current)) {
         newTrackedChanges[newTrackedChanges.length - 1] = last.merge(current)
       } else {
         newTrackedChanges.push(current)
       }
     }
-    this.trackedChanges = newTrackedChanges
+    this._trackedChanges = newTrackedChanges
   }
 
   /**
@@ -106,7 +124,7 @@ class TrackedChangeList {
    */
   applyInsert(cursor, insertedText, opts = {}) {
     const newTrackedChanges = []
-    for (const trackedChange of this.trackedChanges) {
+    for (const trackedChange of this._trackedChanges) {
       if (
         // If the cursor is before or at the insertion point, we need to move
         // the tracked change
@@ -152,7 +170,7 @@ class TrackedChangeList {
       )
       newTrackedChanges.push(newTrackedChange)
     }
-    this.trackedChanges = newTrackedChanges
+    this._trackedChanges = newTrackedChanges
     this._mergeRanges()
   }
 
@@ -163,7 +181,7 @@ class TrackedChangeList {
    */
   applyDelete(cursor, length) {
     const newTrackedChanges = []
-    for (const trackedChange of this.trackedChanges) {
+    for (const trackedChange of this._trackedChanges) {
       const deletedRange = new Range(cursor, length)
       // If the tracked change is after the deletion, we need to move it
       if (deletedRange.contains(trackedChange.range)) {
@@ -186,7 +204,7 @@ class TrackedChangeList {
         newTrackedChanges.push(trackedChange)
       }
     }
-    this.trackedChanges = newTrackedChanges
+    this._trackedChanges = newTrackedChanges
     this._mergeRanges()
   }
 
@@ -202,7 +220,7 @@ class TrackedChangeList {
     }
     const newTrackedChanges = []
     const retainedRange = new Range(cursor, length)
-    for (const trackedChange of this.trackedChanges) {
+    for (const trackedChange of this._trackedChanges) {
       if (retainedRange.contains(trackedChange.range)) {
         // Remove the range
       } else if (retainedRange.overlaps(trackedChange.range)) {
@@ -250,7 +268,7 @@ class TrackedChangeList {
       const newTrackedChange = new TrackedChange(retainedRange, opts.tracking)
       newTrackedChanges.push(newTrackedChange)
     }
-    this.trackedChanges = newTrackedChanges
+    this._trackedChanges = newTrackedChanges
     this._mergeRanges()
   }
 }

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

@@ -31,6 +31,7 @@ const TrackingProps = require('../file_data/tracking_props')
  * @typedef {import('../file_data/string_file_data')} StringFileData
  * @typedef {import('../types').RawTextOperation} RawTextOperation
  * @typedef {import('../operation/scan_op').ScanOp} ScanOp
+ * @typedef {import('../file_data/tracked_change_list')} TrackedChangeList
  */
 
 /**
@@ -826,7 +827,7 @@ function getStartIndex(operation) {
  * @param {number} cursor
  * @param {number} length
  * @param {import('../file_data/comment_list')} commentsList
- * @param {import('../file_data/tracked_change_list')} trackedChangeList
+ * @param {TrackedChangeList} trackedChangeList
  * @returns {{length: number, commentIds?: string[], tracking?: TrackingProps}[]}
  */
 function calculateTrackingCommentSegments(
@@ -853,7 +854,7 @@ function calculateTrackingCommentSegments(
     }
   }
   // Add tracked change boundaries
-  for (const trackedChange of trackedChangeList.trackedChanges) {
+  for (const trackedChange of trackedChangeList.asSorted()) {
     addBreak(trackedChange.range.start)
     addBreak(trackedChange.range.end)
   }

libraries/overleaf-editor-core/test/comment.test.js

@@ -94,13 +94,10 @@ describe('Comment', function () {
     expect(comment.ranges).to.eql([new Range(5, 30), new Range(50, 10)])
   })
 
-  it('should ignore overlapped range', function () {
-    const comment = new Comment([
-      new Range(5, 10),
-      new Range(10, 5),
-      new Range(50, 10),
-    ])
-    expect(comment.ranges).to.eql([new Range(5, 10), new Range(50, 10)])
+  it('should throw error when ranges overlap', function () {
+    expect(
+      () => new Comment([new Range(5, 10), new Range(10, 5), new Range(50, 10)])
+    ).to.throw()
   })
 
   it('should join touching ranges', function () {

libraries/overleaf-editor-core/test/tracked_change_list.test.js

@@ -25,7 +25,7 @@ describe('TrackedChangeList', function () {
             ts: '2024-01-01T00:00:00.000Z',
           }),
         })
-        expect(trackedChanges.trackedChanges.length).to.equal(1)
+        expect(trackedChanges.length).to.equal(1)
         expect(trackedChanges.toRaw()).to.deep.equal([
           {
             range: { pos: 0, length: 6 },
@@ -56,7 +56,7 @@ describe('TrackedChangeList', function () {
             ts: '2024-01-01T00:00:00.000Z',
           }),
         })
-        expect(trackedChanges.trackedChanges.length).to.equal(1)
+        expect(trackedChanges.length).to.equal(1)
         expect(trackedChanges.toRaw()).to.deep.equal([
           {
             range: { pos: 0, length: 16 },
@@ -87,7 +87,7 @@ describe('TrackedChangeList', function () {
             ts: '2023-01-01T00:00:00.000Z',
           }),
         })
-        expect(trackedChanges.trackedChanges.length).to.equal(1)
+        expect(trackedChanges.length).to.equal(1)
         expect(trackedChanges.toRaw()).to.deep.equal([
           {
             range: { pos: 0, length: 6 },
@@ -118,7 +118,7 @@ describe('TrackedChangeList', function () {
             ts: '2024-01-01T00:00:00.000Z',
           }),
         })
-        expect(trackedChanges.trackedChanges.length).to.equal(2)
+        expect(trackedChanges.length).to.equal(2)
         expect(trackedChanges.toRaw()).to.deep.equal([
           {
             range: { pos: 0, length: 3 },
@@ -157,7 +157,7 @@ describe('TrackedChangeList', function () {
             ts: '2024-01-01T00:00:00.000Z',
           }),
         })
-        expect(trackedChanges.trackedChanges.length).to.equal(2)
+        expect(trackedChanges.length).to.equal(2)
         expect(trackedChanges.toRaw()).to.deep.equal([
           {
             range: { pos: 4, length: 3 },
@@ -198,7 +198,7 @@ describe('TrackedChangeList', function () {
             ts: '2024-01-01T00:00:00.000Z',
           }),
         })
-        expect(trackedChanges.trackedChanges.length).to.equal(2)
+        expect(trackedChanges.length).to.equal(2)
         expect(trackedChanges.toRaw()).to.deep.equal([
           {
             range: { pos: 0, length: 3 },
@@ -237,7 +237,7 @@ describe('TrackedChangeList', function () {
             ts: '2024-01-01T00:00:00.000Z',
           }),
         })
-        expect(trackedChanges.trackedChanges.length).to.equal(2)
+        expect(trackedChanges.length).to.equal(2)
         expect(trackedChanges.toRaw()).to.deep.equal([
           {
             range: { pos: 0, length: 3 },
@@ -276,7 +276,7 @@ describe('TrackedChangeList', function () {
             ts: '2024-01-01T00:00:00.000Z',
           }),
         })
-        expect(trackedChanges.trackedChanges.length).to.equal(3)
+        expect(trackedChanges.length).to.equal(3)
         expect(trackedChanges.toRaw()).to.deep.equal([
           {
             range: { pos: 0, length: 5 },
@@ -323,7 +323,7 @@ describe('TrackedChangeList', function () {
             ts: '2024-01-01T00:00:00.000Z',
           }),
         })
-        expect(trackedChanges.trackedChanges.length).to.equal(2)
+        expect(trackedChanges.length).to.equal(2)
         expect(trackedChanges.toRaw()).to.deep.equal([
           {
             range: { pos: 0, length: 5 },
@@ -362,7 +362,7 @@ describe('TrackedChangeList', function () {
             ts: '2024-01-01T00:00:00.000Z',
           }),
         })
-        expect(trackedChanges.trackedChanges.length).to.equal(3)
+        expect(trackedChanges.length).to.equal(3)
         expect(trackedChanges.toRaw()).to.deep.equal([
           {
             range: { pos: 0, length: 4 },
@@ -409,7 +409,7 @@ describe('TrackedChangeList', function () {
             ts: '2024-01-01T00:00:00.000Z',
           }),
         })
-        expect(trackedChanges.trackedChanges.length).to.equal(2)
+        expect(trackedChanges.length).to.equal(2)
         expect(trackedChanges.toRaw()).to.deep.equal([
           {
             range: { pos: 5, length: 6 },
@@ -445,7 +445,7 @@ describe('TrackedChangeList', function () {
         },
       ])
       trackedChanges.applyDelete(5, 2)
-      expect(trackedChanges.trackedChanges.length).to.equal(1)
+      expect(trackedChanges.length).to.equal(1)
       expect(trackedChanges.toRaw()).to.deep.equal([
         {
           range: { pos: 0, length: 8 },
@@ -470,7 +470,7 @@ describe('TrackedChangeList', function () {
         },
       ])
       trackedChanges.applyDelete(0, 10)
-      expect(trackedChanges.trackedChanges.length).to.equal(0)
+      expect(trackedChanges.length).to.equal(0)
       expect(trackedChanges.toRaw()).to.deep.equal([])
     })
 
@@ -486,7 +486,7 @@ describe('TrackedChangeList', function () {
         },
       ])
       trackedChanges.applyDelete(0, 25)
-      expect(trackedChanges.trackedChanges.length).to.equal(0)
+      expect(trackedChanges.length).to.equal(0)
       expect(trackedChanges.toRaw()).to.deep.equal([])
     })
 
@@ -502,7 +502,7 @@ describe('TrackedChangeList', function () {
         },
       ])
       trackedChanges.applyDelete(1, 9)
-      expect(trackedChanges.trackedChanges.length).to.equal(1)
+      expect(trackedChanges.length).to.equal(1)
       expect(trackedChanges.toRaw()).to.deep.equal([
         {
           range: { pos: 0, length: 1 },
@@ -527,7 +527,7 @@ describe('TrackedChangeList', function () {
         },
       ])
       trackedChanges.applyDelete(0, 9)
-      expect(trackedChanges.trackedChanges.length).to.equal(1)
+      expect(trackedChanges.length).to.equal(1)
       expect(trackedChanges.toRaw()).to.deep.equal([
         {
           range: { pos: 0, length: 1 },
@@ -574,7 +574,7 @@ describe('TrackedChangeList', function () {
           ts: '2024-01-01T00:00:00.000Z',
         }),
       })
-      expect(trackedChanges.trackedChanges.length).to.equal(1)
+      expect(trackedChanges.length).to.equal(1)
       expect(trackedChanges.toRaw()).to.deep.equal([
         {
           range: { pos: 0, length: 10 },
@@ -605,7 +605,7 @@ describe('TrackedChangeList', function () {
           ts: '2024-01-01T00:00:00.000Z',
         }),
       })
-      expect(trackedChanges.trackedChanges.length).to.equal(2)
+      expect(trackedChanges.length).to.equal(2)
       expect(trackedChanges.toRaw()).to.deep.equal([
         {
           range: { pos: 0, length: 5 },
@@ -644,7 +644,7 @@ describe('TrackedChangeList', function () {
           ts: '2024-01-01T00:00:00.000Z',
         }),
       })
-      expect(trackedChanges.trackedChanges.length).to.equal(3)
+      expect(trackedChanges.length).to.equal(3)
       expect(trackedChanges.toRaw()).to.deep.equal([
         {
           range: { pos: 0, length: 5 },
@@ -691,7 +691,7 @@ describe('TrackedChangeList', function () {
           ts: '2024-01-01T00:00:00.000Z',
         }),
       })
-      expect(trackedChanges.trackedChanges.length).to.equal(1)
+      expect(trackedChanges.length).to.equal(1)
       expect(trackedChanges.toRaw()).to.deep.equal([
         {
           range: { pos: 0, length: 13 },
@@ -716,7 +716,7 @@ describe('TrackedChangeList', function () {
         },
       ])
       trackedChanges.applyRetain(0, 10)
-      expect(trackedChanges.trackedChanges.length).to.equal(1)
+      expect(trackedChanges.length).to.equal(1)
       expect(trackedChanges.toRaw()).to.deep.equal([
         {
           range: { pos: 0, length: 10 },
@@ -741,7 +741,7 @@ describe('TrackedChangeList', function () {
         },
       ])
       trackedChanges.applyRetain(4, 1)
-      expect(trackedChanges.trackedChanges.length).to.equal(1)
+      expect(trackedChanges.length).to.equal(1)
       expect(trackedChanges.toRaw()).to.deep.equal([
         {
           range: { pos: 0, length: 10 },
@@ -772,7 +772,7 @@ describe('TrackedChangeList', function () {
           ts: '2024-01-01T00:00:00.000Z',
         }),
       })
-      expect(trackedChanges.trackedChanges.length).to.equal(0)
+      expect(trackedChanges.length).to.equal(0)
       expect(trackedChanges.toRaw()).to.deep.equal([])
     })
 
@@ -794,7 +794,7 @@ describe('TrackedChangeList', function () {
           ts: '2024-01-01T00:00:00.000Z',
         }),
       })
-      expect(trackedChanges.trackedChanges.length).to.equal(0)
+      expect(trackedChanges.length).to.equal(0)
       expect(trackedChanges.toRaw()).to.deep.equal([])
     })
 
@@ -816,7 +816,7 @@ describe('TrackedChangeList', function () {
           ts: '2024-01-01T00:00:00.000Z',
         }),
       })
-      expect(trackedChanges.trackedChanges.length).to.equal(0)
+      expect(trackedChanges.length).to.equal(0)
       expect(trackedChanges.toRaw()).to.deep.equal([])
     })
 
@@ -838,7 +838,7 @@ describe('TrackedChangeList', function () {
           ts: '2024-01-01T00:00:00.000Z',
         }),
       })
-      expect(trackedChanges.trackedChanges.length).to.equal(0)
+      expect(trackedChanges.length).to.equal(0)
       expect(trackedChanges.toRaw()).to.deep.equal([])
     })
 
@@ -860,7 +860,7 @@ describe('TrackedChangeList', function () {
           ts: '2024-01-01T00:00:00.000Z',
         }),
       })
-      expect(trackedChanges.trackedChanges.length).to.equal(2)
+      expect(trackedChanges.length).to.equal(2)
       expect(trackedChanges.toRaw()).to.deep.equal([
         {
           range: { pos: 4, length: 4 },

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

@@ -263,9 +263,9 @@ class UpdateSetBuilder {
 function removeTrackedDeletesFromString(content, trackedChanges) {
   let result = ''
   let cursor = 0
-  const trackedDeletes = trackedChanges.trackedChanges.filter(
-    tc => tc.tracking.type === 'delete'
-  )
+  const trackedDeletes = trackedChanges
+    .asSorted()
+    .filter(tc => tc.tracking.type === 'delete')
   for (const trackedChange of trackedDeletes) {
     if (cursor < trackedChange.range.start) {
       result += content.slice(cursor, trackedChange.range.start)
@@ -455,7 +455,9 @@ class TextUpdateBuilder {
       // We are modifying existing tracked deletes. We need to treat removal
       // (type insert/none) of a tracked delete as an insertion. Similarly, any
       // range we introduce as a tracked deletion must be reported as a deletion.
-      const trackedDeletes = this.trackedChanges.trackedChanges.filter(
+      const trackedDeletes = this.trackedChanges
+        .asSorted()
+        .filter(
           tc =>
             tc.tracking.type === 'delete' &&
             tc.range.overlaps(resultRetentionRange)
@@ -554,7 +556,8 @@ class TextUpdateBuilder {
     const sourceDeletionRange = new Range(this.sourceCursor, deletion.length)
     const resultDeletionRange = new Range(this.result.length, deletion.length)
 
-    const trackedDeletes = this.trackedChanges.trackedChanges
+    const trackedDeletes = this.trackedChanges
+      .asSorted()
       .filter(
         tc =>
           tc.tracking.type === 'delete' &&
@@ -604,7 +607,8 @@ class TextUpdateBuilder {
       if ('p' in op && typeof op.p === 'number') {
         // Maybe we have to move the position of the deletion to account for
         // tracked changes that we're hiding in the UI.
-        op.p -= this.trackedChanges.trackedChanges
+        op.p -= this.trackedChanges
+          .asSorted()
           .filter(tc => tc.tracking.type === 'delete' && tc.range.start < op.p)
           .map(tc => {
             if (tc.range.end < op.p) {