From 0948ce17762e253c0f09a003e90e296f112a99a5 Mon Sep 17 00:00:00 2001 From: M-ZubairAhmed Date: Thu, 25 Apr 2024 14:02:34 +0000 Subject: [PATCH] [MM-57745] Fetch threads of the current channel only when root post is missing from the store in new posted event Statuses and user profiles on each new messages have to be fetched for the post users and its mentions (Blue bar), to solve that polling of these can be done. However after we did that we saw the polling of statuses and user profiles requests got considerably down but the requests to threads was relatively still higher (Pink bar). So this improvement doesn't not fetch the root post along with the complete threads of the incoming new post from another channel. This improved the calls to threads from 120 to just 8 calls per 2 mins an improvement of 93%. --- webapp/channels/src/actions/new_post.ts | 6 +- webapp/channels/src/actions/status_actions.ts | 20 ++- .../src/actions/websocket_actions.jsx | 4 +- .../src/actions/websocket_actions.test.jsx | 6 +- .../channel_layout/channel_controller.tsx | 3 +- .../team_controller/actions/index.ts | 9 +- .../src/actions/posts.test.ts | 170 ++++++++---------- .../mattermost-redux/src/actions/posts.ts | 42 +++-- 8 files changed, 130 insertions(+), 130 deletions(-) diff --git a/webapp/channels/src/actions/new_post.ts b/webapp/channels/src/actions/new_post.ts index 76162ec30a..4b2d31c92a 100644 --- a/webapp/channels/src/actions/new_post.ts +++ b/webapp/channels/src/actions/new_post.ts @@ -43,7 +43,9 @@ export function completePostReceive(post: Post, websocketMessageProps: NewPostMe return async (dispatch, getState) => { const state = getState(); const rootPost = PostSelectors.getPost(state, post.root_id); - if (post.root_id && !rootPost) { + const isPostFromCurrentChannel = post.channel_id === getCurrentChannelId(state); + + if (post.root_id && !rootPost && isPostFromCurrentChannel) { const result = await dispatch(PostActions.getPostThread(post.root_id)); if ('error' in result) { @@ -52,7 +54,7 @@ export function completePostReceive(post: Post, websocketMessageProps: NewPostMe } const actions: AnyAction[] = []; - if (post.channel_id === getCurrentChannelId(getState())) { + if (isPostFromCurrentChannel) { actions.push({ type: ActionTypes.INCREASE_POST_VISIBILITY, data: post.channel_id, diff --git a/webapp/channels/src/actions/status_actions.ts b/webapp/channels/src/actions/status_actions.ts index d1197deaa2..82e024285d 100644 --- a/webapp/channels/src/actions/status_actions.ts +++ b/webapp/channels/src/actions/status_actions.ts @@ -18,32 +18,34 @@ import type {GlobalState} from 'types/store'; export function loadStatusesForChannelAndSidebar(): ActionFunc { return (dispatch, getState) => { const state = getState(); - const statusesToLoad: Record = {}; const channelId = getCurrentChannelId(state); const postsInChannel = getPostsInCurrentChannel(state); + const userIds = new Set(); + if (postsInChannel) { const posts = postsInChannel.slice(0, state.views.channel.postVisibility[channelId] || 0); for (const post of posts) { if (post.user_id) { - statusesToLoad[post.user_id] = true; + userIds.add(post.user_id); } } } - const dmPrefs = getDirectShowPreferences(state); - - for (const pref of dmPrefs) { - if (pref.value === 'true') { - statusesToLoad[pref.name] = true; + const directShowPreferences = getDirectShowPreferences(state); + for (const directShowPreference of directShowPreferences) { + if (directShowPreference.value === 'true') { + // This is the other user's id in the DM + userIds.add(directShowPreference.name); } } const currentUserId = getCurrentUserId(state); - statusesToLoad[currentUserId] = true; + userIds.add(currentUserId); + + dispatch(loadStatusesByIds(Array.from(userIds))); - dispatch(loadStatusesByIds(Object.keys(statusesToLoad))); return {data: true}; }; } diff --git a/webapp/channels/src/actions/websocket_actions.jsx b/webapp/channels/src/actions/websocket_actions.jsx index 081ab1ef65..05e37e6f18 100644 --- a/webapp/channels/src/actions/websocket_actions.jsx +++ b/webapp/channels/src/actions/websocket_actions.jsx @@ -40,7 +40,7 @@ import { getPosts, getPostThread, getMentionsAndStatusesForPosts, - getThreadsForPosts, + getPostThreads, postDeleted, receivedNewPost, receivedPost, @@ -739,7 +739,7 @@ export function handleNewPostEvents(queue) { myDispatch(batchActions(actions)); // Load the posts' threads - myDispatch(getThreadsForPosts(posts)); + myDispatch(getPostThreads(posts)); // And any other data needed for them getMentionsAndStatusesForPosts(posts, myDispatch, myGetState); diff --git a/webapp/channels/src/actions/websocket_actions.test.jsx b/webapp/channels/src/actions/websocket_actions.test.jsx index 83617defdf..8f48186615 100644 --- a/webapp/channels/src/actions/websocket_actions.test.jsx +++ b/webapp/channels/src/actions/websocket_actions.test.jsx @@ -5,7 +5,7 @@ import {UserTypes, CloudTypes} from 'mattermost-redux/action_types'; import {getGroup} from 'mattermost-redux/actions/groups'; import { getMentionsAndStatusesForPosts, - getThreadsForPosts, + getPostThreads, receivedNewPost, } from 'mattermost-redux/actions/posts'; import {getUser} from 'mattermost-redux/actions/users'; @@ -40,7 +40,7 @@ import { jest.mock('mattermost-redux/actions/posts', () => ({ ...jest.requireActual('mattermost-redux/actions/posts'), - getThreadsForPosts: jest.fn(() => ({type: 'GET_THREADS_FOR_POSTS'})), + getPostThreads: jest.fn(() => ({type: 'GET_THREADS_FOR_POSTS'})), getMentionsAndStatusesForPosts: jest.fn(), })); @@ -619,7 +619,7 @@ describe('handleNewPostEvents', () => { type: 'GET_THREADS_FOR_POSTS', }, ]); - expect(getThreadsForPosts).toHaveBeenCalledWith(posts); + expect(getPostThreads).toHaveBeenCalledWith(posts); expect(getMentionsAndStatusesForPosts).toHaveBeenCalledWith(posts, expect.anything(), expect.anything()); }); }); diff --git a/webapp/channels/src/components/channel_layout/channel_controller.tsx b/webapp/channels/src/components/channel_layout/channel_controller.tsx index 8fd6fac33f..a2440771de 100644 --- a/webapp/channels/src/components/channel_layout/channel_controller.tsx +++ b/webapp/channels/src/components/channel_layout/channel_controller.tsx @@ -44,12 +44,13 @@ export default function ChannelController(props: Props) { }, []); useEffect(() => { - let loadStatusesIntervalId: ReturnType; + let loadStatusesIntervalId: NodeJS.Timeout; if (enabledUserStatuses) { loadStatusesIntervalId = setInterval(() => { dispatch(loadStatusesForChannelAndSidebar()); }, Constants.STATUS_INTERVAL); } + return () => { clearInterval(loadStatusesIntervalId); }; diff --git a/webapp/channels/src/components/team_controller/actions/index.ts b/webapp/channels/src/components/team_controller/actions/index.ts index b6f9e949a2..ff63cf1001 100644 --- a/webapp/channels/src/components/team_controller/actions/index.ts +++ b/webapp/channels/src/components/team_controller/actions/index.ts @@ -10,6 +10,7 @@ import {logError} from 'mattermost-redux/actions/errors'; import {getGroups, getAllGroupsAssociatedToChannelsInTeam, getAllGroupsAssociatedToTeam, getGroupsByUserIdPaginated} from 'mattermost-redux/actions/groups'; import {forceLogoutIfNecessary} from 'mattermost-redux/actions/helpers'; import {getTeamByName, selectTeam} from 'mattermost-redux/actions/teams'; +import {getIsUserStatusesConfigEnabled} from 'mattermost-redux/selectors/entities/common'; import {getLicense} from 'mattermost-redux/selectors/entities/general'; import {isCustomGroupsEnabled} from 'mattermost-redux/selectors/entities/preferences'; import {getCurrentUser} from 'mattermost-redux/selectors/entities/users'; @@ -38,7 +39,13 @@ export function initializeTeam(team: Team): ActionFuncAsync { return {error: error as ServerError}; } - dispatch(loadStatusesForChannelAndSidebar()); + const enabledUserStatuses = getIsUserStatusesConfigEnabled(state); + + if (enabledUserStatuses) { + // This is the first time in the pool of user statuses that we request, + // subsequent requests will be done via setInterval at channel_controller.tsx + dispatch(loadStatusesForChannelAndSidebar()); + } const license = getLicense(state); const customGroupEnabled = isCustomGroupsEnabled(state); diff --git a/webapp/channels/src/packages/mattermost-redux/src/actions/posts.test.ts b/webapp/channels/src/packages/mattermost-redux/src/actions/posts.test.ts index 325880d0b2..d8852c681e 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/actions/posts.test.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/actions/posts.test.ts @@ -17,6 +17,8 @@ import {Client4} from 'mattermost-redux/client'; import type {GetStateFunc} from 'mattermost-redux/types/actions'; import {getPreferenceKey} from 'mattermost-redux/utils/preference_utils'; +import mockStore from 'tests/test_store'; + import TestHelper from '../../test/test_helper'; import configureStore from '../../test/test_store'; import {Preferences, Posts, RequestStatus} from '../constants'; @@ -1560,103 +1562,6 @@ describe('Actions.Posts', () => { }); }); - describe('getThreadsForPosts', () => { - beforeAll(() => { - TestHelper.initBasic(Client4); - }); - - afterAll(() => { - TestHelper.tearDown(); - }); - - let channelId = ''; - let post1 = TestHelper.getPostMock(); - let post2 = TestHelper.getPostMock(); - let post3 = TestHelper.getPostMock(); - let comment = TestHelper.getPostMock(); - - beforeEach(async () => { - store = configureStore(); - - channelId = TestHelper.basicChannel!.id; - post1 = TestHelper.getPostMock({id: TestHelper.generateId(), channel_id: channelId, message: ''}); - post2 = TestHelper.getPostMock({id: TestHelper.generateId(), channel_id: channelId, message: ''}); - comment = TestHelper.getPostMock({id: TestHelper.generateId(), root_id: post1.id, channel_id: channelId, message: ''}); - post3 = TestHelper.getPostMock({id: TestHelper.generateId(), channel_id: channelId, message: ''}); - - store.dispatch(Actions.receivedPostsInChannel({ - order: [post2.id, post3.id], - posts: {[post2.id]: post2, [post3.id]: post3}, - } as PostList, channelId)); - - const threadList = { - order: [post1.id], - posts: { - [post1.id]: post1, - [comment.id]: comment, - }, - }; - - nock(Client4.getBaseRoute()). - get(`/posts/${post1.id}/thread?skipFetchThreads=false&collapsedThreads=false&collapsedThreadsExtended=false&direction=down&perPage=60`). - reply(200, threadList); - }); - - it('handlesNull', async () => { - const ret = await store.dispatch(Actions.getThreadsForPosts(null as any)); - expect(ret).toEqual({data: true}); - - const state: GlobalState = store.getState(); - - const getRequest = state.requests.posts.getPostThread; - if (getRequest.status === RequestStatus.FAILURE) { - throw new Error(JSON.stringify(getRequest.error)); - } - - const { - postsInChannel, - postsInThread, - } = state.entities.posts; - - expect(postsInChannel[channelId]).toBeTruthy(); - expect(postsInChannel[channelId][0].order).toEqual([post2.id, post3.id]); - expect(!postsInThread[post1.id]).toBeTruthy(); - - const found = postsInChannel[channelId].find((block) => block.order.indexOf(comment.id) !== -1); - - // should not have found comment in postsInChannel - expect(!found).toBeTruthy(); - }); - - it('pullsUpTheThreadOfAMissingPost', async () => { - await store.dispatch(Actions.getThreadsForPosts([comment])); - - const state: GlobalState = store.getState(); - - const getRequest = state.requests.posts.getPostThread; - if (getRequest.status === RequestStatus.FAILURE) { - throw new Error(JSON.stringify(getRequest.error)); - } - - const { - posts, - postsInChannel, - postsInThread, - } = state.entities.posts; - - expect(posts).toBeTruthy(); - expect(postsInChannel[channelId][0].order).toEqual([post2.id, post3.id]); - expect(posts[post1.id]).toBeTruthy(); - expect(postsInThread[post1.id]).toBeTruthy(); - expect(postsInThread[post1.id]).toEqual([comment.id]); - - const found = postsInChannel[channelId].find((block) => block.order.indexOf(comment.id) !== -1); - - // should not have found comment in postsInChannel - expect(!found).toBeTruthy(); - }); - }); - describe('receivedPostsBefore', () => { it('Should return default false for oldest key if param does not exist', () => { const posts = {} as PostList; @@ -1709,3 +1614,74 @@ describe('Actions.Posts', () => { }); }); }); + +describe('getPostThreads', () => { + beforeAll(() => { + TestHelper.initBasic(Client4); + }); + + afterAll(() => { + TestHelper.tearDown(); + }); + + const channelId = 'currentChannelId'; + const initialState = { + entities: { + channels: { + currentChannelId: channelId, + }, + users: { + currentUserId: 'currentUserId', + statuses: {}, + }, + general: { + config: {}, + }, + posts: { + posts: {}, + }, + preferences: { + myPreferences: {}, + }, + }, + }; + + const post1 = TestHelper.getPostMock({id: TestHelper.generateId(), channel_id: channelId, message: '', user_id: 'currentUserId'}); + const comment = TestHelper.getPostMock({id: TestHelper.generateId(), root_id: post1.id, channel_id: channelId, message: '', user_id: 'currentUserId'}); + + it('handles null', async () => { + const testStore = await mockStore(initialState); + const ret = await testStore.dispatch(Actions.getPostThreads(null as any)); + expect(ret).toEqual({data: true}); + + expect(testStore.getActions()).toEqual([]); + }); + + it('pulls up the thread of missing root post in the same channel', async () => { + const testStore = await mockStore(initialState); + nock(Client4.getBaseRoute()). + get(`/posts/${post1.id}/thread?skipFetchThreads=false&collapsedThreads=false&collapsedThreadsExtended=false&direction=down&perPage=60`). + reply(200, { + order: [post1.id], + posts: { + [post1.id]: post1, + [comment.id]: comment, + }, + }); + + await testStore.dispatch(Actions.getPostThreads([comment])); + + expect(testStore.getActions()[1].payload[0].type).toEqual('RECEIVED_POSTS'); + expect(testStore.getActions()[1].payload[0].data.posts).toEqual({ + [post1.id]: post1, + [comment.id]: comment, + }); + + expect(testStore.getActions()[1].payload[1].type).toEqual('RECEIVED_POSTS_IN_THREAD'); + expect(testStore.getActions()[1].payload[1].rootId).toEqual(post1.id); + expect(testStore.getActions()[1].payload[1].data.posts).toEqual({ + [post1.id]: post1, + [comment.id]: comment, + }); + }); +}); diff --git a/webapp/channels/src/packages/mattermost-redux/src/actions/posts.ts b/webapp/channels/src/packages/mattermost-redux/src/actions/posts.ts index d0c35430c4..894cc39d6b 100644 --- a/webapp/channels/src/packages/mattermost-redux/src/actions/posts.ts +++ b/webapp/channels/src/packages/mattermost-redux/src/actions/posts.ts @@ -955,34 +955,46 @@ export function getPostsAround(channelId: string, postId: string, perPage = Post }; } -// getThreadsForPosts is intended for an array of posts that have been batched -// (see the actions/websocket_actions/handleNewPostEvents function in the webapp) -export function getThreadsForPosts(posts: Post[], fetchThreads = true): ThunkActionFunc { - const rootsSet = new Set(); +/** + * getPostThreads is intended for an array of posts that have been batched + * (see the actions/websocket_actions/handleNewPostEvents function in the webapp) +* */ +export function getPostThreads(posts: Post[], fetchThreads = true): ThunkActionFunc { return (dispatch, getState) => { if (!Array.isArray(posts) || !posts.length) { return {data: true}; } const state = getState(); - const promises: Array> = []; + const currentChannelId = getCurrentChannelId(state); - posts.forEach((post) => { + const getPostThreadPromises: Array>> = []; + + const rootPostIds = new Set(); + + for (const post of posts) { if (!post.root_id) { - return; + continue; } + const rootPost = PostSelectors.getPost(state, post.root_id); - - if (!rootPost) { - rootsSet.add(post.root_id); + if (rootPost) { + continue; } - }); - rootsSet.forEach((rootId) => { - promises.push(dispatch(getPostThread(rootId, fetchThreads))); - }); + if (rootPostIds.has(post.root_id)) { + continue; + } - return Promise.all(promises); + // At this point, we know that this post is a thread/reply and its root post is not in the store + rootPostIds.add(post.root_id); + + if (post.channel_id === currentChannelId) { + getPostThreadPromises.push(dispatch(getPostThread(post.root_id, fetchThreads))); + } + } + + return Promise.all(getPostThreadPromises); }; }