[web] let docstore determine a projects comment thread ids (#26364)

* [docstore] add endpoint for getting a projects comment thread ids

* [web] let docstore determine a projects comment thread ids

Also fetch the comment thread ids once when reverting project.

GitOrigin-RevId: c3ebab976821509c9627962e58918f9c6ebb0e1d

services/docstore/app.js

@@ -50,6 +50,10 @@ app.param('doc_id', function (req, res, next, docId) {
 app.get('/project/:project_id/doc-deleted', HttpController.getAllDeletedDocs)
 app.get('/project/:project_id/doc', HttpController.getAllDocs)
 app.get('/project/:project_id/ranges', HttpController.getAllRanges)
+app.get(
+  '/project/:project_id/comment-thread-ids',
+  HttpController.getCommentThreadIds
+)
 app.get(
   '/project/:project_id/tracked-changes-user-ids',
   HttpController.getTrackedChangesUserIds

services/docstore/app/js/DocManager.js

@@ -132,6 +132,22 @@ const DocManager = {
     return docs
   },
 
+  async getCommentThreadIds(projectId) {
+    const docs = await DocManager.getAllNonDeletedDocs(projectId, {
+      _id: true,
+      ranges: true,
+    })
+    const byDoc = new Map()
+    for (const doc of docs) {
+      const ids = new Set()
+      for (const comment of doc.ranges?.comments || []) {
+        ids.add(comment.op.t)
+      }
+      if (ids.size > 0) byDoc.set(doc._id.toString(), Array.from(ids))
+    }
+    return Object.fromEntries(byDoc.entries())
+  },
+
   async getTrackedChangesUserIds(projectId) {
     const docs = await DocManager.getAllNonDeletedDocs(projectId, {
       ranges: true,

services/docstore/app/js/HttpController.js

@@ -83,6 +83,12 @@ async function getAllRanges(req, res) {
   res.json(_buildDocsArrayView(projectId, docs))
 }
 
+async function getCommentThreadIds(req, res) {
+  const { project_id: projectId } = req.params
+  const threadIds = await DocManager.getCommentThreadIds(projectId)
+  res.json(threadIds)
+}
+
 async function getTrackedChangesUserIds(req, res) {
   const { project_id: projectId } = req.params
   const userIds = await DocManager.getTrackedChangesUserIds(projectId)
@@ -239,6 +245,7 @@ module.exports = {
   getAllDeletedDocs: expressify(getAllDeletedDocs),
   getAllRanges: expressify(getAllRanges),
   getTrackedChangesUserIds: expressify(getTrackedChangesUserIds),
+  getCommentThreadIds: expressify(getCommentThreadIds),
   projectHasRanges: expressify(projectHasRanges),
   updateDoc: expressify(updateDoc),
   patchDoc: expressify(patchDoc),

services/docstore/test/acceptance/js/GettingAllDocsTests.js

@@ -25,6 +25,9 @@ describe('Getting all docs', function () {
         _id: new ObjectId(),
         lines: ['one', 'two', 'three'],
         ranges: {
+          comments: [
+            { id: new ObjectId().toString(), op: { t: 'thread-id-1' } },
+          ],
           changes: [
             {
               id: new ObjectId().toString(),
@@ -51,6 +54,9 @@ describe('Getting all docs', function () {
         _id: new ObjectId(),
         lines: ['111', '222', '333'],
         ranges: {
+          comments: [
+            { id: new ObjectId().toString(), op: { t: 'thread-id-2' } },
+          ],
           changes: [
             {
               id: new ObjectId().toString(),
@@ -65,6 +71,7 @@ describe('Getting all docs', function () {
       _id: new ObjectId(),
       lines: ['deleted'],
       ranges: {
+        comments: [{ id: new ObjectId().toString(), op: { t: 'thread-id-3' } }],
         changes: [
           { id: new ObjectId().toString(), metadata: { user_id: 'user-id-3' } },
         ],
@@ -147,4 +154,20 @@ describe('Getting all docs', function () {
       }
     )
   })
+
+  it('getCommentThreadIds should return all the thread ids from (non-deleted) ranges', function (done) {
+    DocstoreClient.getCommentThreadIds(
+      this.project_id,
+      (error, res, threadIds) => {
+        if (error != null) {
+          throw error
+        }
+        threadIds.should.deep.equal({
+          [this.docs[0]._id.toString()]: ['thread-id-1'],
+          [this.docs[2]._id.toString()]: ['thread-id-2'],
+        })
+        done()
+      }
+    )
+  })
 })

services/docstore/test/acceptance/js/helpers/DocstoreClient.js

@@ -100,6 +100,16 @@ module.exports = DocstoreClient = {
     )
   },
 
+  getCommentThreadIds(projectId, callback) {
+    request.get(
+      {
+        url: `http://127.0.0.1:${settings.internal.docstore.port}/project/${projectId}/comment-thread-ids`,
+        json: true,
+      },
+      callback
+    )
+  },
+
   getTrackedChangesUserIds(projectId, callback) {
     request.get(
       {

services/web/app/src/Features/Docstore/DocstoreManager.js

@@ -87,6 +87,14 @@ function getAllDeletedDocs(projectId, callback) {
   })
 }
 
+/**
+ * @param {string} projectId
+ */
+async function getCommentThreadIds(projectId) {
+  const url = `${settings.apis.docstore.url}/project/${projectId}/comment-thread-ids`
+  return fetchJson(url, { signal: AbortSignal.timeout(TIMEOUT) })
+}
+
 /**
  * @param {string} projectId
  */
@@ -301,6 +309,7 @@ module.exports = {
   getAllDeletedDocs,
   getAllRanges,
   getDoc,
+  getCommentThreadIds: callbackify(getCommentThreadIds),
   getTrackedChangesUserIds: callbackify(getTrackedChangesUserIds),
   isDocDeleted,
   updateDoc,
@@ -314,6 +323,7 @@ module.exports = {
     getAllDeletedDocs: promisify(getAllDeletedDocs),
     getAllRanges: promisify(getAllRanges),
     getDoc: promisifyMultiResult(getDoc, ['lines', 'rev', 'version', 'ranges']),
+    getCommentThreadIds,
     getTrackedChangesUserIds,
     isDocDeleted: promisify(isDocDeleted),
     updateDoc: promisifyMultiResult(updateDoc, ['modified', 'rev']),

services/web/app/src/Features/History/RestoreManager.js

@@ -18,6 +18,12 @@ const OError = require('@overleaf/o-error')
 const ProjectGetter = require('../Project/ProjectGetter')
 const ProjectEntityHandler = require('../Project/ProjectEntityHandler')
 
+async function getCommentThreadIds(projectId) {
+  await DocumentUpdaterHandler.promises.flushProjectToMongo(projectId)
+  const raw = await DocstoreManager.promises.getCommentThreadIds(projectId)
+  return new Map(Object.entries(raw).map(([doc, ids]) => [doc, new Set(ids)]))
+}
+
 const RestoreManager = {
   async restoreFileFromV2(userId, projectId, version, pathname) {
     const fsPath = await RestoreManager._writeFileVersionToDisk(
@@ -52,6 +58,25 @@ const RestoreManager = {
   },
 
   async revertFile(userId, projectId, version, pathname, options = {}) {
+    const threadIds = await getCommentThreadIds(projectId)
+    return await RestoreManager._revertSingleFile(
+      userId,
+      projectId,
+      version,
+      pathname,
+      threadIds,
+      options
+    )
+  },
+
+  async _revertSingleFile(
+    userId,
+    projectId,
+    version,
+    pathname,
+    threadIds,
+    options = {}
+  ) {
     const project = await ProjectGetter.promises.getProject(projectId, {
       overleaf: true,
     })
@@ -115,6 +140,7 @@ const RestoreManager = {
         origin,
         userId
       )
+      threadIds.delete(file.element._id.toString())
     }
 
     const { metadata } = await RestoreManager._getMetadataFromHistory(
@@ -154,22 +180,12 @@ const RestoreManager = {
     const documentCommentIds = new Set(
       ranges.comments?.map(({ op: { t } }) => t)
     )
-
-    await DocumentUpdaterHandler.promises.flushProjectToMongo(projectId)
-
-    const docsWithRanges =
-      await DocstoreManager.promises.getAllRanges(projectId)
-
-    const nonOrphanedThreadIds = new Set()
-    for (const { ranges } of docsWithRanges) {
-      for (const comment of ranges.comments ?? []) {
-        nonOrphanedThreadIds.add(comment.op.t)
+    const commentIdsToDuplicate = Array.from(documentCommentIds).filter(id => {
+      for (const ids of threadIds.values()) {
+        if (ids.has(id)) return true
       }
-    }
-
-    const commentIdsToDuplicate = Array.from(documentCommentIds).filter(id =>
-      nonOrphanedThreadIds.has(id)
-    )
+      return false
+    })
 
     const newRanges = { changes: ranges.changes, comments: [] }
 
@@ -257,6 +273,11 @@ const RestoreManager = {
       origin,
       userId
     )
+    // For revertProject: The next doc that gets reverted will need to duplicate all the threads seen here.
+    threadIds.set(
+      _id.toString(),
+      new Set(newRanges.comments.map(({ op: { t } }) => t))
+    )
 
     return {
       _id,
@@ -319,11 +340,17 @@ const RestoreManager = {
       version,
       timestamp: new Date(updateAtVersion.meta.end_ts).toISOString(),
     }
+    const threadIds = await getCommentThreadIds(projectId)
 
     for (const pathname of pathsAtPastVersion) {
-      await RestoreManager.revertFile(userId, projectId, version, pathname, {
-        origin,
-      })
+      await RestoreManager._revertSingleFile(
+        userId,
+        projectId,
+        version,
+        pathname,
+        threadIds,
+        { origin }
+      )
     }
 
     const entitiesAtLiveVersion =

services/web/test/unit/src/History/RestoreManagerTests.js

@@ -9,6 +9,12 @@ const tk = require('timekeeper')
 const moment = require('moment')
 const { expect } = require('chai')
 
+function nestedMapWithSetToObject(m) {
+  return Object.fromEntries(
+    Array.from(m.entries()).map(([key, set]) => [key, Array.from(set)])
+  )
+}
+
 describe('RestoreManager', function () {
   beforeEach(function () {
     tk.freeze(Date.now()) // freeze the time for these tests
@@ -28,7 +34,7 @@ describe('RestoreManager', function () {
             promises: { flushProjectToMongo: sinon.stub().resolves() },
           }),
         '../Docstore/DocstoreManager': (this.DocstoreManager = {
-          promises: {},
+          promises: { getCommentThreadIds: sinon.stub().resolves({}) },
         }),
         '../Chat/ChatApiHandler': (this.ChatApiHandler = { promises: {} }),
         '../Chat/ChatManager': (this.ChatManager = { promises: {} }),
@@ -269,13 +275,9 @@ describe('RestoreManager', function () {
           { op: { t: 'single-comment', p: 10, c: 'bar' } },
         ]
         this.ProjectLocator.promises.findElementByPath = sinon.stub().rejects()
-        this.DocstoreManager.promises.getAllRanges = sinon.stub().resolves([
-          {
-            ranges: {
-              comments: this.comments.slice(0, 1),
-            },
-          },
-        ])
+        this.DocstoreManager.promises.getCommentThreadIds = sinon
+          .stub()
+          .resolves({ 'other-doc': [this.comments[0].op.t] })
         this.ChatApiHandler.promises.duplicateCommentThreads = sinon
           .stub()
           .resolves({
@@ -355,7 +357,7 @@ describe('RestoreManager', function () {
           expect(
             this.DocumentUpdaterHandler.promises.flushProjectToMongo
           ).to.have.been.calledBefore(
-            this.DocstoreManager.promises.getAllRanges
+            this.DocstoreManager.promises.getCommentThreadIds
           )
         })
 
@@ -451,19 +453,11 @@ describe('RestoreManager', function () {
           )
         })
 
-        it('should delete the document before flushing', function () {
-          expect(
-            this.EditorController.promises.deleteEntity
-          ).to.have.been.calledBefore(
-            this.DocumentUpdaterHandler.promises.flushProjectToMongo
-          )
-        })
-
         it('should flush the document before fetching ranges', function () {
           expect(
             this.DocumentUpdaterHandler.promises.flushProjectToMongo
           ).to.have.been.calledBefore(
-            this.DocstoreManager.promises.getAllRanges
+            this.DocstoreManager.promises.getCommentThreadIds
           )
         })
 
@@ -499,6 +493,143 @@ describe('RestoreManager', function () {
           )
         })
       })
+
+      describe('with comments in same doc', function () {
+        // copy of the above, addition: inject and later inspect threadIds set
+        beforeEach(async function () {
+          this.ProjectLocator.promises.findElementByPath = sinon
+            .stub()
+            .resolves({ type: 'doc', element: { _id: 'mock-file-id' } })
+          this.EditorController.promises.deleteEntity = sinon.stub().resolves()
+          this.ChatApiHandler.promises.generateThreadData = sinon
+            .stub()
+            .resolves(
+              (this.threadData = {
+                [this.comments[0].op.t]: {
+                  messages: [
+                    {
+                      content: 'message',
+                      timestamp: '2024-01-01T00:00:00.000Z',
+                      user_id: 'user-1',
+                    },
+                  ],
+                },
+                [this.comments[1].op.t]: {
+                  messages: [
+                    {
+                      content: 'other message',
+                      timestamp: '2024-01-01T00:00:00.000Z',
+                      user_id: 'user-1',
+                    },
+                  ],
+                },
+              })
+            )
+
+          this.threadIds = new Map([
+            [
+              'mock-file-id',
+              new Set([this.comments[0].op.t, this.comments[1].op.t]),
+            ],
+          ])
+          // Comments are updated in-place. Look up threads before reverting.
+          this.afterThreadIds = {
+            // mock-file-id removed
+            [this.addedFile._id]: [
+              this.comments[0].op.t,
+              this.comments[1].op.t,
+            ],
+          }
+          this.data = await this.RestoreManager.promises._revertSingleFile(
+            this.user_id,
+            this.project_id,
+            this.version,
+            this.pathname,
+            this.threadIds
+          )
+        })
+
+        it('should import the file with original comments minus the deleted one', function () {
+          expect(
+            this.EditorController.promises.addDocWithRanges
+          ).to.have.been.calledWith(
+            this.project_id,
+            this.folder_id,
+            'foo.tex',
+            ['foo', 'bar', 'baz'],
+            {
+              changes: this.tracked_changes,
+              comments: this.comments.slice(0, 2),
+            },
+            {
+              kind: 'file-restore',
+              path: this.pathname,
+              version: this.version,
+              timestamp: new Date(this.endTs).toISOString(),
+            }
+          )
+        })
+
+        it('should add the seen thread ids to the map', function () {
+          expect(nestedMapWithSetToObject(this.threadIds)).to.deep.equal(
+            this.afterThreadIds
+          )
+        })
+      })
+
+      describe('with remapped comments during revertProject', function () {
+        // copy of the above, addition: inject and later inspect threadIds set
+        beforeEach(async function () {
+          this.ProjectLocator.promises.findElementByPath = sinon
+            .stub()
+            .resolves({ type: 'doc', element: { _id: 'mock-file-id' } })
+          this.EditorController.promises.deleteEntity = sinon.stub().resolves()
+
+          this.threadIds = new Map([
+            ['other-doc', new Set([this.comments[0].op.t])],
+          ])
+          // Comments are updated in-place. Look up threads before reverting.
+          this.afterThreadIds = {
+            // mock-file-id removed
+            'other-doc': [this.comments[0].op.t],
+            [this.addedFile._id]: [
+              this.remappedComments[0].op.t,
+              this.remappedComments[1].op.t,
+            ],
+          }
+          this.data = await this.RestoreManager.promises._revertSingleFile(
+            this.user_id,
+            this.project_id,
+            this.version,
+            this.pathname,
+            this.threadIds
+          )
+        })
+
+        it('should import the file', function () {
+          expect(
+            this.EditorController.promises.addDocWithRanges
+          ).to.have.been.calledWith(
+            this.project_id,
+            this.folder_id,
+            'foo.tex',
+            ['foo', 'bar', 'baz'],
+            { changes: this.tracked_changes, comments: this.remappedComments },
+            {
+              kind: 'file-restore',
+              path: this.pathname,
+              version: this.version,
+              timestamp: new Date(this.endTs).toISOString(),
+            }
+          )
+        })
+
+        it('should add the seen thread ids to the map', function () {
+          expect(nestedMapWithSetToObject(this.threadIds)).to.deep.equal(
+            this.afterThreadIds
+          )
+        })
+      })
     })
 
     describe('reverting a file or document with metadata', function () {
@@ -524,7 +655,9 @@ describe('RestoreManager', function () {
           .stub()
           .resolves((this.addedFile = { _id: 'mock-doc-id', type: 'doc' }))
 
-        this.DocstoreManager.promises.getAllRanges = sinon.stub().resolves([])
+        this.DocstoreManager.promises.getCommentThreadIds = sinon
+          .stub()
+          .resolves({})
         this.ChatApiHandler.promises.generateThreadData = sinon
           .stub()
           .resolves({})
@@ -741,7 +874,7 @@ describe('RestoreManager', function () {
       this.ProjectGetter.promises.getProject
         .withArgs(this.project_id)
         .resolves({ overleaf: { history: { rangesSupportEnabled: true } } })
-      this.RestoreManager.promises.revertFile = sinon.stub().resolves()
+      this.RestoreManager.promises._revertSingleFile = sinon.stub().resolves()
       this.RestoreManager.promises._getProjectPathsAtVersion = sinon
         .stub()
         .resolves([])
@@ -832,21 +965,27 @@ describe('RestoreManager', function () {
       })
 
       it('should revert the old files', function () {
-        expect(this.RestoreManager.promises.revertFile).to.have.been.calledWith(
+        expect(
+          this.RestoreManager.promises._revertSingleFile
+        ).to.have.been.calledWith(
           this.user_id,
           this.project_id,
           this.version,
           'main.tex'
         )
 
-        expect(this.RestoreManager.promises.revertFile).to.have.been.calledWith(
+        expect(
+          this.RestoreManager.promises._revertSingleFile
+        ).to.have.been.calledWith(
           this.user_id,
           this.project_id,
           this.version,
           'figures/image.png'
         )
 
-        expect(this.RestoreManager.promises.revertFile).to.have.been.calledWith(
+        expect(
+          this.RestoreManager.promises._revertSingleFile
+        ).to.have.been.calledWith(
           this.user_id,
           this.project_id,
           this.version,
@@ -856,7 +995,7 @@ describe('RestoreManager', function () {
 
       it('should not revert the current files', function () {
         expect(
-          this.RestoreManager.promises.revertFile
+          this.RestoreManager.promises._revertSingleFile
         ).to.not.have.been.calledWith(
           this.user_id,
           this.project_id,