From 864a75c284df003b6448b5d199309892bff41c73 Mon Sep 17 00:00:00 2001 From: Miguel Serrano Date: Mon, 11 Jan 2021 12:57:38 +0100 Subject: [PATCH] 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 --- .../app/views/project/editor/chat-react.pug | 2 +- .../js/features/chat/components/chat-pane.js | 17 ++++++---- .../chat/controllers/chat-controller.js | 13 +++++++- .../features/chat/store/chat-store-effect.js | 31 ++++++++++--------- .../js/features/chat/store/chat-store.js | 5 ++- .../chat/components/chat-pane.test.js | 8 ++--- .../features/chat/components/stubs.js | 2 +- .../features/chat/store/chat-store.test.js | 2 +- 8 files changed, 51 insertions(+), 29 deletions(-) diff --git a/services/web/app/views/project/editor/chat-react.pug b/services/web/app/views/project/editor/chat-react.pug index 9ccc31362e..c85568ddd7 100644 --- a/services/web/app/views/project/editor/chat-react.pug +++ b/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") diff --git a/services/web/frontend/js/features/chat/components/chat-pane.js b/services/web/frontend/js/features/chat/components/chat-pane.js index 361bef3ebc..995cecf96f 100644 --- a/services/web/frontend/js/features/chat/components/chat-pane.js +++ b/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) diff --git a/services/web/frontend/js/features/chat/controllers/chat-controller.js b/services/web/frontend/js/features/chat/controllers/chat-controller.js index 7e6046f14e..54439a927c 100644 --- a/services/web/frontend/js/features/chat/controllers/chat-controller.js +++ b/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' + ]) ) diff --git a/services/web/frontend/js/features/chat/store/chat-store-effect.js b/services/web/frontend/js/features/chat/store/chat-store-effect.js index b67662aa19..bccf1bee8d 100644 --- a/services/web/frontend/js/features/chat/store/chat-store-effect.js +++ b/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 } } diff --git a/services/web/frontend/js/features/chat/store/chat-store.js b/services/web/frontend/js/features/chat/store/chat-store.js index beba1e34f3..a50df61262 100644 --- a/services/web/frontend/js/features/chat/store/chat-store.js +++ b/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 } diff --git a/services/web/test/frontend/features/chat/components/chat-pane.test.js b/services/web/test/frontend/features/chat/components/chat-pane.test.js index 150a20fe07..2fc670688f 100644 --- a/services/web/test/frontend/features/chat/components/chat-pane.test.js +++ b/services/web/test/frontend/features/chat/components/chat-pane.test.js @@ -52,7 +52,7 @@ describe('', function() { fetchMock.get(/messages/, testMessages) // unmounting before `beforeEach` block is executed is required to prevent cleanup errors const { unmount } = renderWithEditorContext( - {}} /> + {}} chatIsOpen /> ) await screen.findByText('a message') @@ -63,7 +63,7 @@ describe('', function() { it('A loading spinner is rendered while the messages are loading, then disappears', async function() { fetchMock.get(/messages/, []) const { unmount } = renderWithEditorContext( - {}} /> + {}} chatIsOpen /> ) await waitForElementToBeRemoved(() => screen.getByText('Loading…')) unmount() @@ -73,7 +73,7 @@ describe('', function() { it('is rendered when there are no messages ', async function() { fetchMock.get(/messages/, []) const { unmount } = renderWithEditorContext( - {}} /> + {}} chatIsOpen /> ) await screen.findByText('Send your first message to your collaborators') unmount() @@ -82,7 +82,7 @@ describe('', function() { it('is not rendered when messages are displayed', function() { fetchMock.get(/messages/, testMessages) const { unmount } = renderWithEditorContext( - {}} /> + {}} chatIsOpen /> ) expect( screen.queryByText('Send your first message to your collaborators') diff --git a/services/web/test/frontend/features/chat/components/stubs.js b/services/web/test/frontend/features/chat/components/stubs.js index 4faed6ac2f..71174dc717 100644 --- a/services/web/test/frontend/features/chat/components/stubs.js +++ b/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 } diff --git a/services/web/test/frontend/features/chat/store/chat-store.test.js b/services/web/test/frontend/features/chat/store/chat-store.test.js index a4226f1fbe..bc125a11f0 100644 --- a/services/web/test/frontend/features/chat/store/chat-store.test.js +++ b/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)