Merge pull request #17162 from overleaf/em-handle-comments

Handle comment updates in project-history

GitOrigin-RevId: 46f0043c1c20200bdb665e66643a7870c18d797f

libraries/overleaf-editor-core/index.js

@@ -1,30 +1,61 @@
-exports.Author = require('./lib/author')
-exports.AuthorList = require('./lib/author_list')
-exports.Blob = require('./lib/blob')
-exports.Change = require('./lib/change')
-exports.ChangeRequest = require('./lib/change_request')
-exports.ChangeNote = require('./lib/change_note')
-exports.Chunk = require('./lib/chunk')
-exports.ChunkResponse = require('./lib/chunk_response')
-exports.File = require('./lib/file')
-exports.FileMap = require('./lib/file_map')
-exports.History = require('./lib/history')
-exports.Label = require('./lib/label')
-exports.AddFileOperation = require('./lib/operation/add_file_operation')
-exports.MoveFileOperation = require('./lib/operation/move_file_operation')
-exports.EditFileOperation = require('./lib/operation/edit_file_operation')
-exports.EditNoOperation = require('./lib/operation/edit_no_operation')
-exports.AddCommentOperation = require('./lib/operation/add_comment_operation')
-exports.DeleteCommentOperation = require('./lib/operation/delete_comment_operation')
-exports.SetFileMetadataOperation = require('./lib/operation/set_file_metadata_operation')
-exports.NoOperation = require('./lib/operation/no_operation')
-exports.Operation = require('./lib/operation')
-exports.RestoreOrigin = require('./lib/origin/restore_origin')
-exports.Origin = require('./lib/origin')
-exports.OtClient = require('./lib/ot_client')
-exports.TextOperation = require('./lib/operation/text_operation')
-exports.EditOperation = require('./lib/operation/edit_operation')
-exports.safePathname = require('./lib/safe_pathname')
-exports.Snapshot = require('./lib/snapshot')
-exports.util = require('./lib/util')
-exports.V2DocVersions = require('./lib/v2_doc_versions')
+const AddCommentOperation = require('./lib/operation/add_comment_operation')
+const Author = require('./lib/author')
+const AuthorList = require('./lib/author_list')
+const Blob = require('./lib/blob')
+const Change = require('./lib/change')
+const ChangeRequest = require('./lib/change_request')
+const ChangeNote = require('./lib/change_note')
+const Chunk = require('./lib/chunk')
+const ChunkResponse = require('./lib/chunk_response')
+const DeleteCommentOperation = require('./lib/operation/delete_comment_operation')
+const File = require('./lib/file')
+const FileMap = require('./lib/file_map')
+const History = require('./lib/history')
+const Label = require('./lib/label')
+const AddFileOperation = require('./lib/operation/add_file_operation')
+const MoveFileOperation = require('./lib/operation/move_file_operation')
+const EditFileOperation = require('./lib/operation/edit_file_operation')
+const EditNoOperation = require('./lib/operation/edit_no_operation')
+const SetFileMetadataOperation = require('./lib/operation/set_file_metadata_operation')
+const NoOperation = require('./lib/operation/no_operation')
+const Operation = require('./lib/operation')
+const RestoreOrigin = require('./lib/origin/restore_origin')
+const Origin = require('./lib/origin')
+const OtClient = require('./lib/ot_client')
+const TextOperation = require('./lib/operation/text_operation')
+const EditOperation = require('./lib/operation/edit_operation')
+const safePathname = require('./lib/safe_pathname')
+const Snapshot = require('./lib/snapshot')
+const util = require('./lib/util')
+const V2DocVersions = require('./lib/v2_doc_versions')
+
+exports.AddCommentOperation = AddCommentOperation
+exports.Author = Author
+exports.AuthorList = AuthorList
+exports.Blob = Blob
+exports.Change = Change
+exports.ChangeRequest = ChangeRequest
+exports.ChangeNote = ChangeNote
+exports.Chunk = Chunk
+exports.ChunkResponse = ChunkResponse
+exports.DeleteCommentOperation = DeleteCommentOperation
+exports.File = File
+exports.FileMap = FileMap
+exports.History = History
+exports.Label = Label
+exports.AddFileOperation = AddFileOperation
+exports.MoveFileOperation = MoveFileOperation
+exports.EditFileOperation = EditFileOperation
+exports.EditNoOperation = EditNoOperation
+exports.SetFileMetadataOperation = SetFileMetadataOperation
+exports.NoOperation = NoOperation
+exports.Operation = Operation
+exports.RestoreOrigin = RestoreOrigin
+exports.Origin = Origin
+exports.OtClient = OtClient
+exports.TextOperation = TextOperation
+exports.EditOperation = EditOperation
+exports.safePathname = safePathname
+exports.Snapshot = Snapshot
+exports.util = util
+exports.V2DocVersions = V2DocVersions

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

@@ -29,16 +29,16 @@ class Operation {
    * @return {Operation} one of the subclasses
    */
   static fromRaw(raw) {
-    if (Object.prototype.hasOwnProperty.call(raw, 'file')) {
+    if ('file' in raw) {
       return AddFileOperation.fromRaw(raw)
     }
-    if (Object.prototype.hasOwnProperty.call(raw, 'textOperation')) {
+    if ('textOperation' in raw || 'commentId' in raw) {
       return EditFileOperation.fromRaw(raw)
     }
-    if (Object.prototype.hasOwnProperty.call(raw, 'newPathname')) {
+    if ('newPathname' in raw) {
       return new MoveFileOperation(raw.pathname, raw.newPathname)
     }
-    if (Object.prototype.hasOwnProperty.call(raw, 'metadata')) {
+    if ('metadata' in raw) {
       return new SetFileMetadataOperation(raw.pathname, raw.metadata)
     }
     if (_.isEmpty(raw)) {

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

@@ -32,9 +32,7 @@ describe('DeleteCommentOperation', function () {
     const fileData = new StringFileData('abc')
     const op = new DeleteCommentOperation('123')
     fileData.comments.add('123', new Comment([new Range(0, 1)]))
-    const invertedOp = /** @type {InstanceType<AddCommentOperation>} */ (
-      op.invert(fileData)
-    )
+    const invertedOp = /** @type {AddCommentOperation} */ (op.invert(fileData))
     expect(invertedOp).to.be.instanceOf(AddCommentOperation)
     expect(invertedOp.commentId).to.equal('123')
     expect(invertedOp.comment).to.be.instanceOf(Comment)

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

@@ -36,6 +36,8 @@ const adjustLengthByOp = function (length, op) {
     return length + op.i.length
   } else if (op.d != null) {
     return length - op.d.length
+  } else if (op.c != null) {
+    return length
   } else {
     throw new OError('unexpected op type')
   }
@@ -63,8 +65,7 @@ export function convertToSingleOpUpdates(updates) {
       splitUpdates.push(update)
       continue
     }
-    // Reject any non-insert or delete ops, i.e. comments
-    const ops = update.op.filter(o => o.i != null || o.d != null)
+    const ops = update.op
     let { doc_length: docLength } = update.meta
     for (const op of ops) {
       const splitUpdate = cloneWithOp(update, op)

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

@@ -1,30 +1,41 @@
+// @ts-check
+
 import _ from 'lodash'
 import Core from 'overleaf-editor-core'
 import * as Errors from './Errors.js'
 import * as OperationsCompressor from './OperationsCompressor.js'
 
-export function convertToChanges(projectId, updatesWithBlobs, callback) {
-  let changes
-  try {
-    // convert update to change
-    changes = updatesWithBlobs.map(update =>
-      _convertToChange(projectId, update)
-    )
-  } catch (error1) {
-    const error = error1
-    if (
-      error instanceof Errors.UpdateWithUnknownFormatError ||
-      error instanceof Errors.UnexpectedOpTypeError
-    ) {
-      return callback(error)
-    } else {
-      throw error
-    }
-  }
+/**
+ * @typedef {import('./types.ts').AddDocUpdate} AddDocUpdate
+ * @typedef {import('./types.ts').AddFileUpdate} AddFileUpdate
+ * @typedef {import('./types.ts').CommentOp} CommentOp
+ * @typedef {import('./types.ts').DeleteOp} DeleteOp
+ * @typedef {import('./types.ts').InsertOp} InsertOp
+ * @typedef {import('./types.ts').Op} Op
+ * @typedef {import('./types.ts').RenameUpdate} RenameUpdate
+ * @typedef {import('./types.ts').TextUpdate} TextUpdate
+ * @typedef {import('./types.ts').Update} Update
+ * @typedef {import('./types.ts').UpdateWithBlob} UpdateWithBlob
+ */
 
-  callback(null, changes)
+/**
+ * Convert updates into history changes
+ *
+ * @param {string} projectId
+ * @param {UpdateWithBlob[]} updatesWithBlobs
+ * @returns {Array<Core.Change | null>}
+ */
+export function convertToChanges(projectId, updatesWithBlobs) {
+  return updatesWithBlobs.map(update => _convertToChange(projectId, update))
 }
 
+/**
+ * Convert an update into a history change
+ *
+ * @param {string} projectId
+ * @param {UpdateWithBlob} updateWithBlob
+ * @returns {Core.Change | null}
+ */
 function _convertToChange(projectId, updateWithBlob) {
   let operations
   const { update } = updateWithBlob
@@ -55,11 +66,12 @@ function _convertToChange(projectId, updateWithBlob) {
     let pathname = update.meta.pathname
 
     pathname = _convertPathname(pathname)
-    const builder = new TextOperationsBuilder(docLength, pathname)
+    const builder = new OperationsBuilder(docLength, pathname)
     // convert ops
     for (const op of update.op) {
+      // if this throws an exception it will be caught in convertToChanges
       builder.addOp(op)
-    } // if this throws an exception it will be caught in convertToChanges
+    }
     operations = builder.finish()
     // add doc version information if present
     if (update.v != null) {
@@ -96,28 +108,62 @@ function _convertToChange(projectId, updateWithBlob) {
   }
   const change = Core.Change.fromRaw(rawChange)
 
-  change.operations = OperationsCompressor.compressOperations(change.operations)
+  if (change != null) {
+    change.operations = OperationsCompressor.compressOperations(
+      change.operations
+    )
+  }
 
   return change
 }
 
+/**
+ * @param {Update} update
+ * @returns {update is RenameUpdate}
+ */
 function _isRenameUpdate(update) {
-  return update.new_pathname != null
+  return 'new_pathname' in update && update.new_pathname != null
 }
 
+/**
+ * @param {Update} update
+ * @returns {update is AddDocUpdate}
+ */
 function _isAddDocUpdate(update) {
-  return update.doc != null && update.docLines != null
+  return (
+    'doc' in update &&
+    update.doc != null &&
+    'docLines' in update &&
+    update.docLines != null
+  )
 }
 
+/**
+ * @param {Update} update
+ * @returns {update is AddFileUpdate}
+ */
 function _isAddFileUpdate(update) {
-  return update.file != null && update.url != null
+  return (
+    'file' in update &&
+    update.file != null &&
+    'url' in update &&
+    update.url != null
+  )
 }
 
+/**
+ * @param {Update} update
+ * @returns {update is TextUpdate}
+ */
 export function isTextUpdate(update) {
   return (
+    'doc' in update &&
     update.doc != null &&
+    'op' in update &&
     update.op != null &&
+    'pathname' in update.meta &&
     update.meta.pathname != null &&
+    'doc_length' in update.meta &&
     update.meta.doc_length != null
   )
 }
@@ -126,6 +172,10 @@ export function isProjectStructureUpdate(update) {
   return isAddUpdate(update) || _isRenameUpdate(update)
 }
 
+/**
+ * @param {Update} update
+ * @returns {update is AddDocUpdate | AddFileUpdate}
+ */
 export function isAddUpdate(update) {
   return _isAddDocUpdate(update) || _isAddFileUpdate(update)
 }
@@ -152,20 +202,57 @@ export function _convertPathname(pathname) {
   return pathname
 }
 
-class TextOperationsBuilder {
+class OperationsBuilder {
+  /**
+   * @param {number} docLength
+   * @param {string} pathname
+   */
   constructor(docLength, pathname) {
+    /**
+     * List of operations being built
+     */
     this.operations = []
-    this.currentOperation = []
+
+    /**
+     * Currently built text operation
+     *
+     * @type {(number | string)[]}
+     */
+    this.textOperation = []
+
+    /**
+     * Cursor inside the current text operation
+     */
     this.cursor = 0
+
     this.docLength = docLength
     this.pathname = pathname
   }
 
+  /**
+   * @param {Op} op
+   * @returns {void}
+   */
   addOp(op) {
-    if (op.c != null) {
-      return // ignore comment op
+    if (isComment(op)) {
+      // Close the current text operation
+      this.pushTextOperation()
+
+      // Add a comment operation
+      this.operations.push({
+        pathname: this.pathname,
+        commentId: op.t,
+        ranges: [
+          {
+            pos: op.p,
+            length: op.c.length,
+          },
+        ],
+      })
+      return
     }
-    if (op.i == null && op.d == null) {
+
+    if (!isInsert(op) && !isDelete(op)) {
       throw new Errors.UnexpectedOpTypeError('unexpected op type', { op })
     }
 
@@ -175,7 +262,7 @@ class TextOperationsBuilder {
     const pos = Math.min(op.p, this.docLength)
 
     if (pos < this.cursor) {
-      this.pushCurrentOperation()
+      this.pushTextOperation()
       // At this point, this.cursor === 0 and we can continue
     }
 
@@ -183,47 +270,72 @@ class TextOperationsBuilder {
       this.retain(pos - this.cursor)
     }
 
-    if (op.i != null) {
+    if (isInsert(op)) {
       this.insert(op.i)
     }
 
-    if (op.d != null) {
+    if (isDelete(op)) {
       this.delete(op.d.length)
     }
   }
 
   retain(length) {
-    this.currentOperation.push(length)
+    this.textOperation.push(length)
     this.cursor += length
   }
 
   insert(str) {
-    this.currentOperation.push(str)
+    this.textOperation.push(str)
     this.cursor += str.length
     this.docLength += str.length
   }
 
   delete(length) {
-    this.currentOperation.push(-length)
+    this.textOperation.push(-length)
     this.docLength -= length
   }
 
-  pushCurrentOperation() {
-    if (this.cursor < this.docLength) {
-      this.retain(this.docLength - this.cursor)
-    }
-    if (this.currentOperation.length > 0) {
+  pushTextOperation() {
+    if (this.textOperation.length > 0)
+      if (this.cursor < this.docLength) {
+        this.retain(this.docLength - this.cursor)
+      }
+    if (this.textOperation.length > 0) {
       this.operations.push({
         pathname: this.pathname,
-        textOperation: this.currentOperation,
+        textOperation: this.textOperation,
       })
-      this.currentOperation = []
+      this.textOperation = []
     }
     this.cursor = 0
   }
 
   finish() {
-    this.pushCurrentOperation()
+    this.pushTextOperation()
     return this.operations
   }
 }
+
+/**
+ * @param {Op} op
+ * @returns {op is InsertOp}
+ */
+function isInsert(op) {
+  return 'i' in op && op.i != null
+}
+
+/**
+ * @param {Op} op
+ * @returns {op is DeleteOp}
+ */
+function isDelete(op) {
+  return 'd' in op && op.d != null
+}
+
+/**
+ * @param {Op} op
+ * @returns {op is CommentOp}
+ */
+function isComment(op) {
+  return 'c' in op && op.c != null && 't' in op && op.t != null
+}

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

@@ -408,15 +408,12 @@ function _processUpdates(
                 )
               },
               (updatesWithBlobs, cb) => {
-                cb = profile.wrap('convertToChanges', cb)
-                UpdateTranslator.convertToChanges(
+                const changes = UpdateTranslator.convertToChanges(
                   projectId,
-                  updatesWithBlobs,
-                  cb
-                )
-              },
-              (changes, cb) => {
-                changes = changes.map(change => change.toRaw())
+                  updatesWithBlobs
+                ).map(change => change.toRaw())
+                profile.log('convertToChanges')
+
                 let change
                 const numChanges = changes.length
                 const byteLength = Buffer.byteLength(

services/project-history/app/js/types.ts

@@ -0,0 +1,69 @@
+export type Update = TextUpdate | AddDocUpdate | AddFileUpdate | RenameUpdate
+
+export type UpdateMeta = {
+  user_id: string
+  ts: string
+  source?: string
+  type?: string
+  origin?: RawOrigin
+}
+
+export type TextUpdate = {
+  doc: string
+  op: Op[]
+  v: number
+  meta: UpdateMeta & {
+    pathname: string
+    doc_length: number
+  }
+}
+
+type ProjectUpdateBase = {
+  version: string
+  projectHistoryId: string
+  meta: UpdateMeta
+  doc: string
+}
+
+export type AddDocUpdate = ProjectUpdateBase & {
+  pathname: string
+  docLines: string[]
+}
+
+export type AddFileUpdate = ProjectUpdateBase & {
+  pathname: string
+  file: string
+  url: string
+}
+
+export type RenameUpdate = ProjectUpdateBase & {
+  pathname: string
+  new_pathname: string
+}
+
+export type Op = InsertOp | DeleteOp | CommentOp
+
+export type InsertOp = {
+  i: string
+  p: number
+}
+
+export type DeleteOp = {
+  d: string
+  p: number
+}
+
+export type CommentOp = {
+  c: string
+  p: number
+  t: string
+}
+
+export type UpdateWithBlob = {
+  update: Update
+  blobHash: string
+}
+
+export type RawOrigin = {
+  kind: string
+}

services/project-history/scripts/debug_translate_updates.js

@@ -36,16 +36,13 @@ function expandResyncProjectStructure(chunk, update) {
 
 function expandUpdates(updates) {
   const wrappedUpdates = updates.map(update => ({ update }))
-  UpdateTranslator.convertToChanges(
-    projectId,
-    wrappedUpdates,
-    (err, changes) => {
-      if (err != null) {
-        error(err)
-      }
-      console.log(JSON.stringify(changes, null, 2))
-    }
-  )
+  let changes
+  try {
+    changes = UpdateTranslator.convertToChanges(projectId, wrappedUpdates)
+  } catch (err) {
+    error(err)
+  }
+  console.log(JSON.stringify(changes, null, 2))
 }
 
 if (updates[0].resyncProjectStructure) {

services/project-history/test/acceptance/js/SendingUpdatesTests.js

@@ -73,19 +73,12 @@ function slRenameUpdate(historyId, doc, userId, ts, pathname, newPathname) {
   }
 }
 
-function olTextUpdate(doc, userId, ts, textOperation, v) {
+function olUpdate(doc, userId, ts, operations, v) {
   return {
     v2Authors: [userId],
     timestamp: ts.toJSON(),
     authors: [],
-
-    operations: [
-      {
-        pathname: doc.pathname.replace(/^\//, ''), // Strip leading /
-        textOperation,
-      },
-    ],
-
+    operations,
     v2DocVersions: {
       [doc.id]: {
         pathname: doc.pathname.replace(/^\//, ''), // Strip leading /
@@ -95,26 +88,34 @@ function olTextUpdate(doc, userId, ts, textOperation, v) {
   }
 }
 
-function olTextUpdates(doc, userId, ts, textOperations, v) {
+function olTextOperation(doc, textOperation) {
   return {
-    v2Authors: [userId],
-    timestamp: ts.toJSON(),
-    authors: [],
+    pathname: doc.pathname.replace(/^\//, ''), // Strip leading /
+    textOperation,
+  }
+}
 
-    operations: textOperations.map(textOperation => ({
-      // Strip leading /
-      pathname: doc.pathname.replace(/^\//, ''),
+function olAddCommentOperation(doc, commentId, pos, length) {
+  return {
+    pathname: doc.pathname.replace(/^\//, ''), // Strip leading /
+    commentId,
+    ranges: [{ pos, length }],
+    resolved: false,
+  }
+}
 
-      textOperation,
-    })),
+function olTextUpdate(doc, userId, ts, textOperation, v) {
+  return olUpdate(doc, userId, ts, [olTextOperation(doc, textOperation)], v)
+}
 
-    v2DocVersions: {
-      [doc.id]: {
-        pathname: doc.pathname.replace(/^\//, ''), // Strip leading /
-        v: v || 1,
-      },
-    },
-  }
+function olTextUpdates(doc, userId, ts, textOperations, v) {
+  return olUpdate(
+    doc,
+    userId,
+    ts,
+    textOperations.map(textOperation => olTextOperation(doc, textOperation)),
+    v
+  )
 }
 
 function olRenameUpdate(doc, userId, ts, pathname, newPathname) {
@@ -594,11 +595,21 @@ describe('Sending Updates', function () {
       )
     })
 
-    it('should ignore comment ops', function (done) {
+    it('should handle comment ops', function (done) {
       const createChange = MockHistoryStore()
         .post(`/api/projects/${historyId}/legacy_changes`, body => {
           expect(body).to.deep.equal([
-            olTextUpdate(this.doc, this.userId, this.timestamp, [3, '\nc', 2]),
+            olUpdate(this.doc, this.userId, this.timestamp, [
+              olTextOperation(this.doc, [3, '\nc', 2]),
+              olAddCommentOperation(this.doc, 'comment-id-1', 3, 2),
+            ]),
+            olUpdate(
+              this.doc,
+              this.userId,
+              this.timestamp,
+              [olAddCommentOperation(this.doc, 'comment-id-2', 2, 1)],
+              2
+            ),
           ])
           return true
         })
@@ -618,7 +629,7 @@ describe('Sending Updates', function () {
                 this.timestamp,
                 [
                   { p: 3, i: '\nc' },
-                  { p: 3, c: '\nc' },
+                  { p: 3, c: '\nc', t: 'comment-id-1' },
                 ]
               ),
               cb
@@ -633,7 +644,7 @@ describe('Sending Updates', function () {
                 this.userId,
                 2,
                 this.timestamp,
-                [{ p: 2, c: 'b' }]
+                [{ p: 2, c: 'b', t: 'comment-id-2' }]
               ),
               cb
             )

services/project-history/test/unit/js/UpdateCompressor/UpdateCompressorTests.js

@@ -65,7 +65,7 @@ describe('UpdateCompressor', function () {
       ).to.deep.equal([])
     })
 
-    it('should ignore comment ops', function () {
+    it('should not ignore comment ops', function () {
       expect(
         this.UpdateCompressor.convertToSingleOpUpdates([
           {
@@ -74,19 +74,24 @@ describe('UpdateCompressor', function () {
               (this.op2 = { p: 9, c: 'baz' }),
               (this.op3 = { p: 6, i: 'bar' }),
             ],
-            meta: { ts: this.ts1, user_id: this.user_id },
+            meta: { ts: this.ts1, user_id: this.user_id, doc_length: 10 },
             v: 42,
           },
         ])
       ).to.deep.equal([
         {
           op: this.op1,
-          meta: { ts: this.ts1, user_id: this.user_id },
+          meta: { ts: this.ts1, user_id: this.user_id, doc_length: 10 },
+          v: 42,
+        },
+        {
+          op: this.op2,
+          meta: { ts: this.ts1, user_id: this.user_id, doc_length: 13 },
           v: 42,
         },
         {
           op: this.op3,
-          meta: { ts: this.ts1, user_id: this.user_id },
+          meta: { ts: this.ts1, user_id: this.user_id, doc_length: 13 },
           v: 42,
         },
       ])

services/project-history/test/unit/js/UpdatesManager/UpdatesProcessorTests.js

@@ -279,7 +279,7 @@ describe('UpdatesProcessor', function () {
       this.SyncManager.expandSyncUpdates.yields(null, this.expandedUpdates)
       this.UpdateCompressor.compressRawUpdates.returns(this.compressedUpdates)
       this.BlobManager.createBlobForUpdates.yields(null, this.updatesWithBlobs)
-      this.UpdateTranslator.convertToChanges.yields(null, this.changes)
+      this.UpdateTranslator.convertToChanges.returns(this.changes)
 
       this.UpdatesProcessor._processUpdates(
         this.project_id,