Fixed unread message count in react chat (#3529)

* Fixed unread message count in react chat

The problem was caused by ChatStore being instantiated multiple times on each `useRef` call, plus also incorrectly cleaned-up, since it should be calling `socket.removeListener` instead of `socket.off` on effect destroy.

* deferred loading messages until chat is opened

GitOrigin-RevId: b990cd06cea6630472b0911b56219766717aaff6

services/web/app/views/project/editor/chat-react.pug

@@ -1,4 +1,4 @@
 aside.chat(
 	ng-controller="ReactChatController"
 )
-	chat(reset-unread-messages="resetUnreadMessages")
+	chat(reset-unread-messages="resetUnreadMessages" chat-is-open="chatIsOpen")

services/web/frontend/js/features/chat/components/chat-pane.js

@@ -1,4 +1,4 @@
-import React, { useEffect } from 'react'
+import React, { useEffect, useState } from 'react'
 import PropTypes from 'prop-types'
 import MessageList from './message-list'
 import MessageInput from './message-input'
@@ -8,7 +8,7 @@ import { useTranslation } from 'react-i18next'
 import { useChatStore } from '../store/chat-store-effect'
 import withErrorBoundary from '../../../infrastructure/error-boundary'
 
-function ChatPane({ resetUnreadMessages }) {
+function ChatPane({ resetUnreadMessages, chatIsOpen }) {
   const { t } = useTranslation()
 
   const {
@@ -20,10 +20,14 @@ function ChatPane({ resetUnreadMessages }) {
     userId
   } = useChatStore()
 
+  const [initialMessagesLoaded, setInitialMessagesLoaded] = useState(false)
+
   useEffect(() => {
-    loadMoreMessages()
-    // eslint-disable-next-line react-hooks/exhaustive-deps
-  }, [])
+    if (chatIsOpen && !initialMessagesLoaded) {
+      loadMoreMessages()
+      setInitialMessagesLoaded(true)
+    }
+  }, [initialMessagesLoaded, loadMoreMessages, chatIsOpen])
 
   const shouldDisplayPlaceholder = !loading && messages.length === 0
 
@@ -85,7 +89,8 @@ function Placeholder() {
 }
 
 ChatPane.propTypes = {
-  resetUnreadMessages: PropTypes.func.isRequired
+  resetUnreadMessages: PropTypes.func.isRequired,
+  chatIsOpen: PropTypes.bool
 }
 
 export default withErrorBoundary(ChatPane)

services/web/frontend/js/features/chat/controllers/chat-controller.js

@@ -5,11 +5,22 @@ import { rootContext } from '../../../shared/context/root-context'
 import ChatPane from '../components/chat-pane'
 
 App.controller('ReactChatController', function($scope, ide) {
+  $scope.chatIsOpen = ide.$scope.ui.chatOpen
+
+  ide.$scope.$watch('ui.chatOpen', newValue => {
+    $scope.$applyAsync(() => {
+      $scope.chatIsOpen = newValue
+    })
+  })
+
   $scope.resetUnreadMessages = () =>
     ide.$scope.$broadcast('chat:resetUnreadMessages')
 })
 
 App.component(
   'chat',
-  react2angular(rootContext.use(ChatPane), ['resetUnreadMessages'])
+  react2angular(rootContext.use(ChatPane), [
+    'resetUnreadMessages',
+    'chatIsOpen'
+  ])
 )

services/web/frontend/js/features/chat/store/chat-store-effect.js

@@ -1,4 +1,4 @@
-import { useState, useEffect, useRef } from 'react'
+import { useState, useCallback, useEffect } from 'react'
 import { ChatStore } from './chat-store'
 import { useApplicationContext } from '../../../shared/context/application-context'
 import { useEditorContext } from '../../../shared/context/editor-context'
@@ -7,29 +7,32 @@ export function useChatStore() {
   const { user } = useApplicationContext()
   const { projectId } = useEditorContext()
 
-  const chatStoreRef = useRef(new ChatStore(user, projectId))
+  const [atEnd, setAtEnd] = useState(false)
+  const [loading, setLoading] = useState(false)
+  const [messages, setMessages] = useState([])
 
-  const [atEnd, setAtEnd] = useState(chatStoreRef.current.atEnd)
-  const [loading, setLoading] = useState(chatStoreRef.current.loading)
-  const [messages, setMessages] = useState(chatStoreRef.current.messages)
+  const [store] = useState(() => new ChatStore(user, projectId))
+  const loadMoreMessages = useCallback(() => store.loadMoreMessages(), [store])
+  const sendMessage = useCallback(message => store.sendMessage(message), [
+    store
+  ])
 
   useEffect(() => {
-    const chatStore = chatStoreRef.current
     function handleStoreUpdated() {
-      setAtEnd(chatStore.atEnd)
-      setLoading(chatStore.loading)
-      setMessages(chatStore.messages)
+      setAtEnd(store.atEnd)
+      setLoading(store.loading)
+      setMessages(store.messages)
     }
-    chatStore.on('updated', handleStoreUpdated)
-    return () => chatStore.destroy()
-  }, [chatStoreRef])
+    store.on('updated', handleStoreUpdated)
+    return () => store.destroy()
+  }, [store])
 
   return {
     userId: user.id,
     atEnd,
     loading,
     messages,
-    loadMoreMessages: () => chatStoreRef.current.loadMoreMessages(),
-    sendMessage: message => chatStoreRef.current.sendMessage(message)
+    loadMoreMessages,
+    sendMessage
   }
 }

services/web/frontend/js/features/chat/store/chat-store.js

@@ -35,7 +35,10 @@ export class ChatStore {
   }
 
   destroy() {
-    window._ide.socket.off('new-chat-message', this._onNewChatMessage)
+    window._ide.socket.removeListener(
+      'new-chat-message',
+      this._onNewChatMessage
+    )
     this._emitter.off() // removes all listeners
   }
 

services/web/test/frontend/features/chat/components/chat-pane.test.js

@@ -52,7 +52,7 @@ describe('<ChatPane />', function() {
     fetchMock.get(/messages/, testMessages)
     // unmounting before `beforeEach` block is executed is required to prevent cleanup errors
     const { unmount } = renderWithEditorContext(
-      <ChatPane resetUnreadMessages={() => {}} />
+      <ChatPane resetUnreadMessages={() => {}} chatIsOpen />
     )
 
     await screen.findByText('a message')
@@ -63,7 +63,7 @@ describe('<ChatPane />', function() {
   it('A loading spinner is rendered while the messages are loading, then disappears', async function() {
     fetchMock.get(/messages/, [])
     const { unmount } = renderWithEditorContext(
-      <ChatPane resetUnreadMessages={() => {}} />
+      <ChatPane resetUnreadMessages={() => {}} chatIsOpen />
     )
     await waitForElementToBeRemoved(() => screen.getByText('Loading…'))
     unmount()
@@ -73,7 +73,7 @@ describe('<ChatPane />', function() {
     it('is rendered when there are no messages ', async function() {
       fetchMock.get(/messages/, [])
       const { unmount } = renderWithEditorContext(
-        <ChatPane resetUnreadMessages={() => {}} />
+        <ChatPane resetUnreadMessages={() => {}} chatIsOpen />
       )
       await screen.findByText('Send your first message to your collaborators')
       unmount()
@@ -82,7 +82,7 @@ describe('<ChatPane />', function() {
     it('is not rendered when messages are displayed', function() {
       fetchMock.get(/messages/, testMessages)
       const { unmount } = renderWithEditorContext(
-        <ChatPane resetUnreadMessages={() => {}} />
+        <ChatPane resetUnreadMessages={() => {}} chatIsOpen />
       )
       expect(
         screen.queryByText('Send your first message to your collaborators')

services/web/test/frontend/features/chat/components/stubs.js

@@ -27,7 +27,7 @@ export function tearDownMathJaxStubs() {
 }
 
 export function stubChatStore({ user }) {
-  window._ide = { socket: { on: sinon.stub(), off: sinon.stub() } }
+  window._ide = { socket: { on: sinon.stub(), removeListener: sinon.stub() } }
   window.user = user
 }
 

services/web/test/frontend/features/chat/store/chat-store.test.js

@@ -26,7 +26,7 @@ describe('ChatStore', function() {
 
     window.csrfToken = 'csrf_tok'
 
-    socket = { on: sinon.stub(), off: sinon.stub() }
+    socket = { on: sinon.stub(), removeListener: sinon.stub() }
     window._ide = { socket }
     mockSocketMessage = message => socket.on.getCall(0).args[1](message)