[overleaf-core-editor] Make comment properties immutable (#17022)

* make range class immutable

* rename variable

* use newRanges

* range readonly props

* skrinkBy test

* Fix range shrinking bug

* [overleaf-core-editor] Make comment properties immutable

* remove added line

* format fix

* make readonly array

* [overleaf-editor-core] AddCommentOperation and DeleteCommentOperation (#16871)

* [overleaf-editor-core] AddCommentOperation and DeleteCommentOperation

* added add comment op test

* delete comment op test

* import core to escape circle deps

* desctructure in tests

* require directly in builder

* invert of add comment is always delete comment

* no merging on compose

* NoOp if comment is not found

* use comment.clone()

* update test

* change CommentRawData type

* jsdoc assert type

* fix formating

* EditNoOperation

* return other in compose

* use ReturnType

* Revert "use ReturnType"

This reverts commit 2c7e04f1541310e9fc08963170a783a437ed1992.

* transorm add comment operation

* transform delete comment operation

* moved comment.js

* format fix

* fix transform addComment and textoperation

* fix merge

* test more complex test operations

* change to else if

* move range.js

* fix types

* fix AddComment and TextOperation transform

* fixed AddComment-TextOperation trasform, added test

* deletecommentoperation should win

* should not delete comment

* remove unused function, fix type

* fix format

* add resolved for existing comment

* transform EditNoOperation

* fix test description

* change the order of EditNoOperation

* fix DeleteCommentOperation-DeleteCommentOperation transform

* fix types after merging main

* refactor operation types

* fix errors after merging

* fix bad merge

* format fix

* removed comment.clone()

* return old comment

* remove unused var

GitOrigin-RevId: e31d723075cb04b0b7177e7cae0014c295f92a68

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

@@ -9,34 +9,24 @@ const Range = require('./range')
 
 class Comment {
   /**
-   * @type {Range[]}
+   * @readonly
+   * @type {ReadonlyArray<Range>}
    */
   ranges = []
 
   /**
+   * @readonly
    * @type {boolean}
    */
   resolved = false
 
   /**
-   * @param {Range[]} ranges
+   * @param {ReadonlyArray<Range>} ranges
    * @param {boolean} [resolved]
    */
   constructor(ranges, resolved = false) {
     this.resolved = resolved
-    for (const range of ranges) {
-      this.addRange(range)
-    }
-  }
-
-  /**
-   *
-   * @param {Range} range
-   */
-  addRange(range) {
-    this.ranges.push(range)
-    this.ranges.sort((a, b) => a.start - b.start)
-    this.mergeRanges()
+    this.ranges = this.mergeRanges(ranges)
   }
 
   /**
@@ -44,6 +34,7 @@ class Comment {
    * @param {number} cursor
    * @param {number} length
    * @param {boolean} [extendComment]
+   * @returns {Comment}
    */
   applyInsert(cursor, length, extendComment = false) {
     let existingRangeExtended = false
@@ -91,17 +82,18 @@ class Comment {
       }
     }
 
-    this.ranges = newRanges
-
     // if the insert is not inside any range, add a new range
     if (extendComment && !existingRangeExtended) {
-      this.addRange(new Range(cursor, length))
+      newRanges.push(new Range(cursor, length))
     }
+
+    return new Comment(newRanges, this.resolved)
   }
 
   /**
    *
    * @param {Range} deletedRange
+   * @returns {Comment}
    */
   applyDelete(deletedRange) {
     const newRanges = []
@@ -116,26 +108,29 @@ class Comment {
       }
     }
 
-    this.ranges = newRanges
-    this.mergeRanges()
+    return new Comment(newRanges, this.resolved)
   }
 
   /**
    *
    * @param {TextOperation} operation
+   * @returns {Comment}
    */
   applyTextOperation(operation) {
+    /** @type {Comment} */
+    let comment = this
     let cursor = 0
     for (const op of operation.ops) {
       if (op instanceof RetainOp) {
         cursor += op.length
       } else if (op instanceof InsertOp) {
-        this.applyInsert(cursor, op.insertion.length)
+        comment = comment.applyInsert(cursor, op.insertion.length)
         cursor += op.insertion.length
       } else if (op instanceof RemoveOp) {
-        this.applyDelete(new Range(cursor, op.length))
+        comment = comment.applyDelete(new Range(cursor, op.length))
       }
     }
+    return comment
   }
 
   isEmpty() {
@@ -153,11 +148,16 @@ class Comment {
     }
   }
 
-  mergeRanges() {
+  /**
+   * @param {ReadonlyArray<Range>} ranges
+   * @returns {ReadonlyArray<Range>}
+   */
+  mergeRanges(ranges) {
     /** @type {Range[]} */
     const mergedRanges = []
 
-    for (const range of this.ranges) {
+    const sortedRanges = [...ranges].sort((a, b) => a.start - b.start)
+    for (const range of sortedRanges) {
       if (range.isEmpty()) {
         continue
       }
@@ -170,14 +170,7 @@ class Comment {
       }
     }
 
-    this.ranges = mergedRanges
-  }
-
-  /**
-   * @returns {Comment}
-   */
-  clone() {
-    return Comment.fromRaw(this.toRaw())
+    return mergedRanges
   }
 
   /**

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

@@ -41,10 +41,9 @@ class CommentList {
   add(id, newComment) {
     const existingComment = this.getComment(id)
     if (existingComment) {
-      existingComment.resolved = existingComment.resolved && newComment.resolved
-      for (const range of newComment.ranges) {
-        existingComment.addRange(range)
-      }
+      const resolved = existingComment.resolved && newComment.resolved
+      const mergedRanges = [...existingComment.ranges, ...newComment.ranges]
+      this.comments.set(id, new Comment(mergedRanges, resolved))
     } else {
       this.comments.set(id, newComment)
     }
@@ -77,11 +76,12 @@ class CommentList {
       opts.commentIds = []
     }
     for (const [commentId, comment] of this.comments) {
-      comment.applyInsert(
+      const commentAfterInsert = comment.applyInsert(
         range.pos,
         range.length,
         opts.commentIds.includes(commentId)
       )
+      this.comments.set(commentId, commentAfterInsert)
     }
   }
 
@@ -89,8 +89,9 @@ class CommentList {
    * @param {Range} range
    */
   applyDelete(range) {
-    for (const [, comment] of this.comments) {
-      comment.applyDelete(range)
+    for (const [commentId, comment] of this.comments) {
+      const commentAfterDelete = comment.applyDelete(range)
+      this.comments.set(commentId, commentAfterDelete)
     }
   }
 

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

@@ -50,7 +50,7 @@ class DeleteCommentOperation extends EditOperation {
       return new EditNoOperation()
     }
 
-    return new core.AddCommentOperation(this.commentId, comment.clone())
+    return new core.AddCommentOperation(this.commentId, comment)
   }
 
   /**

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

@@ -31,14 +31,12 @@ class EditOperationTransformer {
     }
 
     if (a instanceof AddCommentOperation && b instanceof TextOperation) {
-      const comment = a.comment.clone()
-      comment.applyTextOperation(b)
+      const comment = a.comment.applyTextOperation(b)
       return [new AddCommentOperation(a.commentId, comment), b]
     }
 
     if (a instanceof TextOperation && b instanceof AddCommentOperation) {
-      const comment = b.comment.clone()
-      comment.applyTextOperation(a)
+      const comment = b.comment.applyTextOperation(a)
       return [a, new AddCommentOperation(b.commentId, comment)]
     }
 

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

@@ -8,77 +8,72 @@ const Range = require('../lib/range')
 describe('Comment', function () {
   it('should move ranges to the right of insert', function () {
     const comment = new Comment([new Range(5, 10)])
-    comment.applyInsert(3, 5, false)
-    expect(comment.ranges).to.eql([new Range(10, 10)])
+    const resComment = comment.applyInsert(3, 5, false)
+    expect(resComment.ranges).to.eql([new Range(10, 10)])
   })
 
-  it('should expand the range after insert inside it', function () {
-    const comment = new Comment([new Range(5, 10)])
-    comment.applyInsert(4, 1) // inserting 1 char before the range
-    expect(comment.ranges).to.eql([new Range(6, 10)])
-    comment.applyInsert(6, 1) // inserting 1 char at the edge (without expandCommand = false)
-    expect(comment.ranges).to.eql([new Range(7, 10)])
-    comment.applyInsert(7, 1, true) // inserting 1 char at the edge (with expandCommand = true)
-    expect(comment.ranges).to.eql([new Range(7, 11)])
-    comment.applyInsert(8, 1, true) // inserting 1 char inside the range
-    expect(comment.ranges).to.eql([new Range(7, 12)])
+  describe('applyInsert', function () {
+    it('should insert 1 char before the range', function () {
+      const comment = new Comment([new Range(5, 10)])
+      expect(comment.applyInsert(4, 1).ranges).to.eql([new Range(6, 10)])
+    })
+
+    it('should insert 1 char at the edge, without expandCommand', function () {
+      const comment = new Comment([new Range(5, 10)])
+      expect(comment.applyInsert(5, 1).ranges).to.eql([new Range(6, 10)])
+    })
+
+    it('should insert 1 char at the edge, with expandCommand', function () {
+      const comment = new Comment([new Range(5, 10)])
+      expect(comment.applyInsert(5, 1, true).ranges).to.eql([new Range(5, 11)])
+    })
+
+    it('should expand the range after insert inside it', function () {
+      const comment = new Comment([new Range(5, 10)])
+      expect(comment.applyInsert(6, 1, true).ranges).to.eql([new Range(5, 11)])
+    })
   })
 
   it('should split the range if inside another and expandComment is false', function () {
-    const comment1 = new Comment([new Range(5, 10)])
-    comment1.applyInsert(6, 10, false)
-    expect(comment1.ranges).to.eql([new Range(5, 1), new Range(16, 9)])
-
-    // insert at the end of the range
-    const comment2 = new Comment([new Range(5, 10)])
-    comment2.applyInsert(14, 10, false)
-    expect(comment2.ranges).to.eql([new Range(5, 9), new Range(24, 1)])
+    const comment = new Comment([new Range(5, 10)])
+    const commentRes = comment.applyInsert(6, 10, false)
+    expect(commentRes.ranges).to.eql([new Range(5, 1), new Range(16, 9)])
+  })
+
+  it('should insert the range if expandComment is false', function () {
+    const comment = new Comment([new Range(5, 10)])
+    const commentRes = comment.applyInsert(14, 10, false)
+    expect(commentRes.ranges).to.eql([new Range(5, 9), new Range(24, 1)])
   })
 
   it('should move the range if insert is at range start and expandComment is false', function () {
     const comment = new Comment([new Range(5, 10)])
-    comment.applyInsert(5, 10, false)
-    expect(comment.ranges).to.eql([new Range(15, 10)])
+    const commentRes = comment.applyInsert(5, 10, false)
+    expect(commentRes.ranges).to.eql([new Range(15, 10)])
   })
 
   it('should ignore the range if insert is at range end and expandComment is false', function () {
     const comment = new Comment([new Range(5, 10)])
-    comment.applyInsert(15, 10, false)
-    expect(comment.ranges).to.eql([new Range(5, 10)])
+    const commentRes = comment.applyInsert(15, 10, false)
+    expect(commentRes.ranges).to.eql([new Range(5, 10)])
   })
 
   it('should expand the range after inserting on the edge of it if expandComment is true', function () {
     const comment = new Comment([new Range(5, 10)])
-    comment.applyInsert(15, 10, true)
-    expect(comment.ranges).to.eql([new Range(5, 20)])
+    const commentRes = comment.applyInsert(15, 10, true)
+    expect(commentRes.ranges).to.eql([new Range(5, 20)])
   })
 
-  it('should add a new range if expandComment is true and not inside any range', function () {
-    const commentNoRanges = new Comment([])
-    commentNoRanges.applyInsert(5, 10, true)
-    expect(commentNoRanges.ranges).to.eql([new Range(5, 10)])
-
-    const commentWithRanges = new Comment([new Range(5, 10)])
-    commentWithRanges.applyInsert(50, 10, true)
-    expect(commentWithRanges.ranges).to.eql([
-      new Range(5, 10),
-      new Range(50, 10),
-    ])
-  })
-
-  it('should move ranges if delete is before it', function () {
-    const from5to14 = new Comment([new Range(5, 10)])
-    const from3to7 = new Range(3, 5)
-
-    from5to14.applyDelete(from3to7)
-    const from3to9 = new Range(3, 7)
-    expect(from5to14.ranges).to.eql([from3to9])
+  it('should move comment ranges if delete is before it', function () {
+    const comment = new Comment([new Range(5, 10)])
+    const commentRes = comment.applyDelete(new Range(3, 5))
+    expect(commentRes.ranges).to.eql([new Range(3, 7)])
   })
 
   it('should merge ranges after delete', function () {
     const comment = new Comment([new Range(5, 10), new Range(20, 10)])
-    comment.applyDelete(new Range(7, 18))
-    expect(comment.ranges).to.eql([new Range(5, 7)])
+    const commentRes = comment.applyDelete(new Range(7, 18))
+    expect(commentRes.ranges).to.eql([new Range(5, 7)])
   })
 
   it('should merge overlapping ranges', function () {
@@ -87,7 +82,6 @@ describe('Comment', function () {
       new Range(15, 20),
       new Range(50, 10),
     ])
-    comment.mergeRanges()
     expect(comment.ranges).to.eql([new Range(5, 30), new Range(50, 10)])
   })
 
@@ -97,7 +91,6 @@ describe('Comment', function () {
       new Range(50, 10),
       new Range(5, 10),
     ])
-    comment.mergeRanges()
     expect(comment.ranges).to.eql([new Range(5, 30), new Range(50, 10)])
   })
 
@@ -107,7 +100,6 @@ describe('Comment', function () {
       new Range(10, 5),
       new Range(50, 10),
     ])
-    comment.mergeRanges()
     expect(comment.ranges).to.eql([new Range(5, 10), new Range(50, 10)])
   })
 
@@ -117,7 +109,6 @@ describe('Comment', function () {
       new Range(15, 5),
       new Range(50, 10),
     ])
-    comment.mergeRanges()
     expect(comment.ranges).to.eql([new Range(5, 15), new Range(50, 10)])
   })
 })