Merge pull request #18710 from overleaf/mj-web-chat-send-thread-data

[chat+web] Inform frontend when duplicating threads

GitOrigin-RevId: 285afee8f5a016a8e7ac58e9538cc3ec8362681d

services/chat/app/js/Features/Messages/MessageHttpController.js

@@ -86,6 +86,10 @@ export async function duplicateCommentThreads(context) {
   return await callMessageHttpController(context, _duplicateCommentThreads)
 }
 
+export async function generateThreadData(context) {
+  return await callMessageHttpController(context, _generateThreadData)
+}
+
 export async function getStatus(context) {
   const message = 'chat is alive'
   context.res.status(200).setBody(message)
@@ -124,6 +128,18 @@ const _getAllThreads = async (req, res) => {
   res.json(threads)
 }
 
+const _generateThreadData = async (req, res) => {
+  const { projectId } = req.params
+  const { threads } = req.body
+  logger.debug({ projectId }, 'getting all threads')
+  const rooms = await ThreadManager.findThreadsById(projectId, threads)
+  const roomIds = rooms.map(r => r._id)
+  const messages = await MessageManager.findAllMessagesInRooms(roomIds)
+  logger.debug({ rooms, messages }, 'looked up messages in the rooms')
+  const threadData = MessageFormatter.groupMessagesByThreads(rooms, messages)
+  res.json(threadData)
+}
+
 const _resolveThread = async (req, res) => {
   const { projectId, threadId } = req.params
   const { user_id: userId } = req.body

services/chat/app/js/Features/Threads/ThreadManager.js

@@ -146,3 +146,12 @@ export async function duplicateThread(projectId, threadId) {
   newRoom._id = confirmation.insertedId
   return { oldRoom: room, newRoom }
 }
+
+export async function findThreadsById(projectId, threadIds) {
+  return await db.rooms
+    .find({
+      project_id: new ObjectId(projectId),
+      thread_id: { $in: threadIds.map(id => new ObjectId(id)) },
+    })
+    .toArray()
+}

services/chat/chat.yaml

@@ -334,6 +334,33 @@ paths:
                     type: object
                 description: Mapping of old thread ids to their duplicated thread ids
       description: Duplicate a list of comment threads
+  '/project/{projectId}/generate-thread-data':
+    parameters:
+      - schema:
+          type: string
+        name: projectId
+        in: path
+        required: true
+    post:
+      summary: Generate thread data to load into the frontend
+      operationId: generateThreadData
+      requestBody:
+        content:
+          application/json:
+            schema:
+              type: object
+              properties:
+                threads:
+                  type: array
+                  items:
+                    type: string
+      responses:
+        '200':
+          content:
+            application/json:
+              schema:
+                type: object
+      description: Load threads and generate a json blob containing all messages in all the threads
 components:
   schemas:
     Message:

services/web/app/src/Features/Chat/ChatApiHandler.js

@@ -109,6 +109,16 @@ async function duplicateCommentThreads(projectId, threads) {
   )
 }
 
+async function generateThreadData(projectId, threads) {
+  return await fetchJson(
+    chatApiUrl(`/project/${projectId}/generate-thread-data`),
+    {
+      method: 'POST',
+      json: { threads },
+    }
+  )
+}
+
 function chatApiUrl(path) {
   return new URL(path, settings.apis.chat.internal_url)
 }
@@ -126,6 +136,7 @@ module.exports = {
   deleteMessage: callbackify(deleteMessage),
   getResolvedThreadIds: callbackify(getResolvedThreadIds),
   duplicateCommentThreads: callbackify(duplicateCommentThreads),
+  generateThreadData: callbackify(generateThreadData),
   promises: {
     getThreads,
     destroyProject,
@@ -139,5 +150,6 @@ module.exports = {
     deleteMessage,
     getResolvedThreadIds,
     duplicateCommentThreads,
+    generateThreadData,
   },
 }

services/web/app/src/Features/Chat/ChatController.js

@@ -18,7 +18,8 @@ const EditorRealTimeController = require('../Editor/EditorRealTimeController')
 const SessionManager = require('../Authentication/SessionManager')
 const UserInfoManager = require('../User/UserInfoManager')
 const UserInfoController = require('../User/UserInfoController')
-const async = require('async')
+const ChatManager = require('./ChatManager')
+const logger = require('@overleaf/logger')
 
 module.exports = ChatController = {
   sendMessage(req, res, next) {
@@ -68,7 +69,7 @@ module.exports = ChatController = {
         if (err != null) {
           return next(err)
         }
-        return ChatController._injectUserInfoIntoThreads(
+        return ChatManager.injectUserInfoIntoThreads(
           { global: { messages } },
           function (err) {
             if (err != null) {
@@ -80,55 +81,4 @@ module.exports = ChatController = {
       }
     )
   },
-
-  _injectUserInfoIntoThreads(threads, callback) {
-    // There will be a lot of repitition of user_ids, so first build a list
-    // of unique ones to perform db look ups on, then use these to populate the
-    // user fields
-    let message, thread, threadId, userId
-    if (callback == null) {
-      callback = function () {}
-    }
-    const userIds = {}
-    for (threadId in threads) {
-      thread = threads[threadId]
-      if (thread.resolved) {
-        userIds[thread.resolved_by_user_id] = true
-      }
-      for (message of Array.from(thread.messages)) {
-        userIds[message.user_id] = true
-      }
-    }
-
-    const jobs = []
-    const users = {}
-    for (userId in userIds) {
-      const _ = userIds[userId]
-      ;(userId =>
-        jobs.push(cb =>
-          UserInfoManager.getPersonalInfo(userId, function (error, user) {
-            if (error != null) return cb(error)
-            user = UserInfoController.formatPersonalInfo(user)
-            users[userId] = user
-            cb()
-          })
-        ))(userId)
-    }
-
-    return async.series(jobs, function (error) {
-      if (error != null) {
-        return callback(error)
-      }
-      for (threadId in threads) {
-        thread = threads[threadId]
-        if (thread.resolved) {
-          thread.resolved_by_user = users[thread.resolved_by_user_id]
-        }
-        for (message of Array.from(thread.messages)) {
-          message.user = users[message.user_id]
-        }
-      }
-      return callback(null, threads)
-    })
-  },
 }

services/web/app/src/Features/Chat/ChatManager.js

@@ -0,0 +1,61 @@
+const async = require('async')
+const UserInfoManager = require('../User/UserInfoManager')
+const UserInfoController = require('../User/UserInfoController')
+const { promisify } = require('@overleaf/promise-utils')
+
+function injectUserInfoIntoThreads(threads, callback) {
+  // There will be a lot of repitition of user_ids, so first build a list
+  // of unique ones to perform db look ups on, then use these to populate the
+  // user fields
+  let message, thread, threadId, userId
+  if (callback == null) {
+    callback = function () {}
+  }
+  const userIds = {}
+  for (threadId in threads) {
+    thread = threads[threadId]
+    if (thread.resolved) {
+      userIds[thread.resolved_by_user_id] = true
+    }
+    for (message of Array.from(thread.messages)) {
+      userIds[message.user_id] = true
+    }
+  }
+
+  const jobs = []
+  const users = {}
+  for (userId in userIds) {
+    ;(userId =>
+      jobs.push(cb =>
+        UserInfoManager.getPersonalInfo(userId, function (error, user) {
+          if (error != null) return cb(error)
+          user = UserInfoController.formatPersonalInfo(user)
+          users[userId] = user
+          cb()
+        })
+      ))(userId)
+  }
+
+  return async.series(jobs, function (error) {
+    if (error != null) {
+      return callback(error)
+    }
+    for (threadId in threads) {
+      thread = threads[threadId]
+      if (thread.resolved) {
+        thread.resolved_by_user = users[thread.resolved_by_user_id]
+      }
+      for (message of Array.from(thread.messages)) {
+        message.user = users[message.user_id]
+      }
+    }
+    return callback(null, threads)
+  })
+}
+
+module.exports = {
+  injectUserInfoIntoThreads,
+  promises: {
+    injectUserInfoIntoThreads: promisify(injectUserInfoIntoThreads),
+  },
+}

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

@@ -12,6 +12,8 @@ const DocumentUpdaterHandler = require('../DocumentUpdater/DocumentUpdaterHandle
 const ChatApiHandler = require('../Chat/ChatApiHandler')
 const DocstoreManager = require('../Docstore/DocstoreManager')
 const logger = require('@overleaf/logger')
+const EditorRealTimeController = require('../Editor/EditorRealTimeController')
+const ChatManager = require('../Chat/ChatManager')
 
 const RestoreManager = {
   async restoreFileFromV2(userId, projectId, version, pathname) {
@@ -158,6 +160,19 @@ const RestoreManager = {
       newRanges.comments = ranges.comments
     }
 
+    const newCommentThreadData =
+      await ChatApiHandler.promises.generateThreadData(
+        projectId,
+        newRanges.comments.map(({ op: { t } }) => t)
+      )
+    await ChatManager.promises.injectUserInfoIntoThreads(newCommentThreadData)
+    logger.debug({ newCommentThreadData }, 'emitting new comment threads')
+    EditorRealTimeController.emitToRoom(
+      projectId,
+      'new-comment-threads',
+      newCommentThreadData
+    )
+
     return await EditorController.promises.addDocWithRanges(
       projectId,
       parentFolderId,

services/web/frontend/js/features/ide-react/context/review-panel/hooks/use-review-panel-state.ts

@@ -1452,6 +1452,27 @@ function useReviewPanelState(): ReviewPanel.ReviewPanelState {
       [getThread]
     )
   )
+  useSocketListener(
+    socket,
+    'new-comment-threads',
+    useCallback(
+      (threads: ReviewPanelCommentThreadsApi) => {
+        setCommentThreads(prevState => {
+          const newThreads = { ...prevState }
+          for (const threadIdString of Object.keys(threads)) {
+            const threadId = threadIdString as ThreadId
+            const { submitting: _, ...thread } = getThread(threadId)
+            // Replace already loaded messages with the server provided ones
+            thread.messages = threads[threadId].messages.map(formatComment)
+            newThreads[threadId] = thread
+          }
+          return newThreads
+        })
+        handleLayoutChange({ async: true })
+      },
+      [getThread]
+    )
+  )
 
   const openSubView = useRef<typeof subView>('cur_file')
   useEffect(() => {

services/web/test/unit/src/Chat/ChatControllerTests.js

@@ -19,13 +19,13 @@ const modulePath = path.join(
   __dirname,
   '../../../../app/src/Features/Chat/ChatController'
 )
-const { expect } = require('chai')
 
 describe('ChatController', function () {
   beforeEach(function () {
     this.user_id = 'mock-user-id'
     this.settings = {}
     this.ChatApiHandler = {}
+    this.ChatManager = {}
     this.EditorRealTimeController = { emitToRoom: sinon.stub() }
     this.SessionManager = {
       getLoggedInUserId: sinon.stub().returns(this.user_id),
@@ -34,6 +34,7 @@ describe('ChatController', function () {
       requires: {
         '@overleaf/settings': this.settings,
         './ChatApiHandler': this.ChatApiHandler,
+        './ChatManager': this.ChatManager,
         '../Editor/EditorRealTimeController': this.EditorRealTimeController,
         '../Authentication/SessionManager': this.SessionManager,
         '../User/UserInfoManager': (this.UserInfoManager = {}),
@@ -106,7 +107,7 @@ describe('ChatController', function () {
         limit: (this.limit = '30'),
         before: (this.before = '12345'),
       }
-      this.ChatController._injectUserInfoIntoThreads = sinon.stub().yields()
+      this.ChatManager.injectUserInfoIntoThreads = sinon.stub().yields()
       this.ChatApiHandler.getGlobalMessages = sinon
         .stub()
         .yields(null, (this.messages = ['mock', 'messages']))
@@ -123,107 +124,4 @@ describe('ChatController', function () {
       return this.res.json.calledWith(this.messages).should.equal(true)
     })
   })
-
-  describe('_injectUserInfoIntoThreads', function () {
-    beforeEach(function () {
-      this.users = {
-        user_id_1: {
-          mock: 'user_1',
-        },
-        user_id_2: {
-          mock: 'user_2',
-        },
-      }
-      this.UserInfoManager.getPersonalInfo = (userId, callback) => {
-        return callback(null, this.users[userId])
-      }
-      sinon.spy(this.UserInfoManager, 'getPersonalInfo')
-      return (this.UserInfoController.formatPersonalInfo = user => ({
-        formatted: user.mock,
-      }))
-    })
-
-    it('should inject a user object into messaged and resolved data', function (done) {
-      return this.ChatController._injectUserInfoIntoThreads(
-        {
-          thread1: {
-            resolved: true,
-            resolved_by_user_id: 'user_id_1',
-            messages: [
-              {
-                user_id: 'user_id_1',
-                content: 'foo',
-              },
-              {
-                user_id: 'user_id_2',
-                content: 'bar',
-              },
-            ],
-          },
-          thread2: {
-            messages: [
-              {
-                user_id: 'user_id_1',
-                content: 'baz',
-              },
-            ],
-          },
-        },
-        (error, threads) => {
-          expect(threads).to.deep.equal({
-            thread1: {
-              resolved: true,
-              resolved_by_user_id: 'user_id_1',
-              resolved_by_user: { formatted: 'user_1' },
-              messages: [
-                {
-                  user_id: 'user_id_1',
-                  user: { formatted: 'user_1' },
-                  content: 'foo',
-                },
-                {
-                  user_id: 'user_id_2',
-                  user: { formatted: 'user_2' },
-                  content: 'bar',
-                },
-              ],
-            },
-            thread2: {
-              messages: [
-                {
-                  user_id: 'user_id_1',
-                  user: { formatted: 'user_1' },
-                  content: 'baz',
-                },
-              ],
-            },
-          })
-          return done()
-        }
-      )
-    })
-
-    it('should only need to look up each user once', function (done) {
-      return this.ChatController._injectUserInfoIntoThreads(
-        [
-          {
-            messages: [
-              {
-                user_id: 'user_id_1',
-                content: 'foo',
-              },
-              {
-                user_id: 'user_id_1',
-                content: 'bar',
-              },
-            ],
-          },
-        ],
-        (error, threads) => {
-          this.UserInfoManager.getPersonalInfo.calledOnce.should.equal(true)
-          return done()
-        }
-      )
-    })
-  })
 })

services/web/test/unit/src/Chat/ChatManagerTests.js

@@ -0,0 +1,135 @@
+const SandboxedModule = require('sandboxed-module')
+const path = require('path')
+const sinon = require('sinon')
+const modulePath = path.join(
+  __dirname,
+  '../../../../app/src/Features/Chat/ChatManager'
+)
+const { expect } = require('chai')
+
+describe('ChatManager', function () {
+  beforeEach(function () {
+    this.user_id = 'mock-user-id'
+    this.ChatManager = SandboxedModule.require(modulePath, {
+      requires: {
+        '../User/UserInfoManager': (this.UserInfoManager = {}),
+        '../User/UserInfoController': (this.UserInfoController = {}),
+      },
+    })
+    this.req = {
+      params: {
+        project_id: this.project_id,
+      },
+    }
+    this.res = {
+      json: sinon.stub(),
+      send: sinon.stub(),
+      sendStatus: sinon.stub(),
+    }
+  })
+
+  describe('injectUserInfoIntoThreads', function () {
+    beforeEach(function () {
+      this.users = {
+        user_id_1: {
+          mock: 'user_1',
+        },
+        user_id_2: {
+          mock: 'user_2',
+        },
+      }
+      this.UserInfoManager.getPersonalInfo = (userId, callback) => {
+        return callback(null, this.users[userId])
+      }
+      sinon.spy(this.UserInfoManager, 'getPersonalInfo')
+      return (this.UserInfoController.formatPersonalInfo = user => ({
+        formatted: user.mock,
+      }))
+    })
+
+    it('should inject a user object into messaged and resolved data', function (done) {
+      return this.ChatManager.injectUserInfoIntoThreads(
+        {
+          thread1: {
+            resolved: true,
+            resolved_by_user_id: 'user_id_1',
+            messages: [
+              {
+                user_id: 'user_id_1',
+                content: 'foo',
+              },
+              {
+                user_id: 'user_id_2',
+                content: 'bar',
+              },
+            ],
+          },
+          thread2: {
+            messages: [
+              {
+                user_id: 'user_id_1',
+                content: 'baz',
+              },
+            ],
+          },
+        },
+        (error, threads) => {
+          expect(error).to.be.null
+          expect(threads).to.deep.equal({
+            thread1: {
+              resolved: true,
+              resolved_by_user_id: 'user_id_1',
+              resolved_by_user: { formatted: 'user_1' },
+              messages: [
+                {
+                  user_id: 'user_id_1',
+                  user: { formatted: 'user_1' },
+                  content: 'foo',
+                },
+                {
+                  user_id: 'user_id_2',
+                  user: { formatted: 'user_2' },
+                  content: 'bar',
+                },
+              ],
+            },
+            thread2: {
+              messages: [
+                {
+                  user_id: 'user_id_1',
+                  user: { formatted: 'user_1' },
+                  content: 'baz',
+                },
+              ],
+            },
+          })
+          return done()
+        }
+      )
+    })
+
+    it('should only need to look up each user once', function (done) {
+      return this.ChatManager.injectUserInfoIntoThreads(
+        [
+          {
+            messages: [
+              {
+                user_id: 'user_id_1',
+                content: 'foo',
+              },
+              {
+                user_id: 'user_id_1',
+                content: 'bar',
+              },
+            ],
+          },
+        ],
+        (error, threads) => {
+          expect(error).to.be.null
+          this.UserInfoManager.getPersonalInfo.calledOnce.should.equal(true)
+          return done()
+        }
+      )
+    })
+  })
+})

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

@@ -31,6 +31,9 @@ describe('RestoreManager', function () {
           promises: {},
         }),
         '../Chat/ChatApiHandler': (this.ChatApiHandler = { promises: {} }),
+        '../Chat/ChatManager': (this.ChatManager = { promises: {} }),
+        '../Editor/EditorRealTimeController': (this.EditorRealTimeController =
+          {}),
       },
     })
     this.user_id = 'mock-user-id'
@@ -324,6 +327,13 @@ describe('RestoreManager', function () {
               },
             },
           })
+        this.ChatApiHandler.promises.generateThreadData = sinon
+          .stub()
+          .resolves({})
+        this.ChatManager.promises.injectUserInfoIntoThreads = sinon
+          .stub()
+          .resolves()
+        this.EditorRealTimeController.emitToRoom = sinon.stub()
         this.tracked_changes = [
           {
             op: { pos: 4, i: 'bar' },