MM-62710 Ensure CommentedOn message is never missing due to unloaded data (#30572)

* MM-62710 Ensure CommentedOn message is never missing due to unloaded data

There's more information in the Jira ticket on how this occurs, but the
short version is that there's a way for either the root post and/or the
root post's author to be missing which previously caused the "Commented
On" line not to be rendered for a comment with CRT disabled. That caused
the post to appear as if it was part of the wrong thread.

Now, the CommentedOn component is always mounted when for the first
non-consecutive reply to a thread, and it knows how to load that missing
data itself.

I also took the opportunity to implement basic `useUser` and `usePost`
hooks which will fetch the user/post if needed. They're both implemented
using a shared `makeUseEntity` which should be usable for other entities
(objects stored in Redux state and fetched using a Redux action). These
also both use actions that use `DelayedDataLoader` to batch and debounce
requests for that data as well!

Assuming this works well, I'm hoping we can start using this pattern
elsewhere in the app.

* Update tests and remove unneeded mapStateToProps

* Address feedback

* Fix playwright tests

* revert Fix playwright tests

---------

Co-authored-by: yasserfaraazkhan <attitude3cena.yf@gmail.com>

webapp/channels/src/components/common/hooks/useEntity.ts

@@ -0,0 +1,39 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+import {useEffect} from 'react';
+import {useDispatch, useSelector} from 'react-redux';
+import type {Action} from 'redux';
+import type {ThunkAction} from 'redux-thunk';
+
+import type {GlobalState} from 'types/store';
+
+export type UseDataOptions<Entity, Identifier = string, State = GlobalState> = {
+    name: string;
+
+    fetch: (identifier: Identifier) => Action | ThunkAction<unknown, State, unknown, Action>;
+    selector: (state: State, identifier: Identifier) => Entity | undefined;
+}
+
+export function makeUseEntity<Entity, Identifier = string, State = GlobalState>(options: UseDataOptions<Entity, Identifier, State>) {
+    function useEntity(identifier: Identifier): Entity | undefined {
+        const dispatch = useDispatch();
+
+        const entity = useSelector((state: State) => {
+            return identifier ? options.selector(state, identifier) : undefined;
+        });
+
+        const entityLoaded = Boolean(entity);
+        useEffect(() => {
+            if (!entityLoaded && identifier) {
+                dispatch(options.fetch(identifier));
+            }
+        }, [dispatch, entityLoaded, identifier]);
+
+        return entity;
+    }
+
+    Object.defineProperty(useEntity, 'name', {value: options.name, writable: false});
+
+    return useEntity;
+}

webapp/channels/src/components/common/hooks/usePost.test.ts

@@ -0,0 +1,235 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+import nock from 'nock';
+import * as ReactRedux from 'react-redux';
+
+import {Client4} from 'mattermost-redux/client';
+
+import {renderHookWithContext} from 'tests/react_testing_utils';
+import {TestHelper} from 'utils/test_helper';
+
+import {usePost} from './usePost';
+
+describe('usePost', () => {
+    const post1 = TestHelper.getPostMock({id: 'post1'});
+    const post2 = TestHelper.getPostMock({id: 'post2'});
+
+    describe('with fake dispatch', () => {
+        const dispatchMock = jest.fn();
+
+        beforeAll(() => {
+            jest.spyOn(ReactRedux, 'useDispatch').mockImplementation(() => dispatchMock);
+        });
+
+        afterAll(() => {
+            jest.restoreAllMocks();
+        });
+
+        test("should return the post if it's already in the store", () => {
+            const {result} = renderHookWithContext(
+                () => usePost('post1'),
+                {
+                    entities: {
+                        posts: {
+                            posts: {
+                                post1,
+                            },
+                        },
+                    },
+                },
+            );
+
+            expect(result.current).toBe(post1);
+            expect(dispatchMock).not.toHaveBeenCalled();
+        });
+
+        test("should fetch the post if it's not in the store", () => {
+            const {result} = renderHookWithContext(
+                () => usePost('post1'),
+            );
+
+            expect(result.current).toBe(undefined);
+            expect(dispatchMock).toHaveBeenCalledTimes(1);
+        });
+
+        test('should only attempt to fetch the post once regardless of how many times the hook is used', () => {
+            const {result, rerender} = renderHookWithContext(
+                () => usePost('post1'),
+            );
+
+            expect(result.current).toBe(undefined);
+            expect(dispatchMock).toHaveBeenCalledTimes(1);
+
+            for (let i = 0; i < 10; i++) {
+                rerender();
+            }
+
+            expect(result.current).toBe(undefined);
+            expect(dispatchMock).toHaveBeenCalledTimes(1);
+        });
+
+        test('should attempt to fetch different posts if the post ID changes', () => {
+            let postId = 'post1';
+            const {result, rerender} = renderHookWithContext(
+                () => usePost(postId),
+            );
+
+            expect(result.current).toBe(undefined);
+            expect(dispatchMock).toHaveBeenCalledTimes(1);
+
+            postId = 'post2';
+            rerender();
+
+            expect(result.current).toBe(undefined);
+            expect(dispatchMock).toHaveBeenCalledTimes(2);
+        });
+
+        test("should only attempt to fetch each post once when they aren't loaded", () => {
+            let postId = 'post1';
+            const {result, replaceStoreState, rerender} = renderHookWithContext(
+                () => usePost(postId),
+            );
+
+            // Initial state without post1 loaded
+            expect(result.current).toBe(undefined);
+            expect(dispatchMock).toHaveBeenCalledTimes(1);
+
+            // Simulate the response to loading post1
+            replaceStoreState({
+                entities: {
+                    posts: {
+                        posts: {
+                            post1,
+                        },
+                    },
+                },
+            });
+
+            expect(result.current).toBe(post1);
+            expect(dispatchMock).toHaveBeenCalledTimes(1);
+
+            // Switch to post2
+            postId = 'post2';
+
+            rerender();
+
+            expect(result.current).toBe(undefined);
+            expect(dispatchMock).toHaveBeenCalledTimes(2);
+
+            // Simulate the response to loading post2
+            replaceStoreState({
+                entities: {
+                    posts: {
+                        posts: {
+                            post1,
+                            post2,
+                        },
+                    },
+                },
+            });
+
+            expect(result.current).toBe(post2);
+            expect(dispatchMock).toHaveBeenCalledTimes(2);
+
+            // Switch back to post1 which has already been loaded
+            postId = 'post1';
+
+            rerender();
+
+            expect(result.current).toBe(post1);
+            expect(dispatchMock).toHaveBeenCalledTimes(2);
+        });
+
+        test("shouldn't attempt to load anything when given an empty post ID", () => {
+            const {result} = renderHookWithContext(
+                () => usePost(''),
+            );
+
+            expect(result.current).toBe(undefined);
+            expect(dispatchMock).toHaveBeenCalledTimes(0);
+        });
+    });
+
+    describe('with real dispatch', () => {
+        beforeAll(() => {
+            Client4.setUrl('http://localhost:8065');
+        });
+
+        test("should only attempt to fetch each post once when they aren't loaded", async () => {
+            const post1Mock = nock(Client4.getBaseRoute()).
+                post('/posts/ids', [post1.id]).
+                once().
+                reply(200, [post1]);
+            const post2Mock = nock(Client4.getBaseRoute()).
+                post('/posts/ids', [post2.id]).
+                once().
+                reply(200, [post2]);
+
+            let postId = 'post1';
+            const {result, rerender, waitForNextUpdate} = renderHookWithContext(
+                () => usePost(postId),
+            );
+
+            // Initial state without post1 loaded
+            expect(result.current).toEqual(undefined);
+            expect(post1Mock.isDone()).toBe(false);
+            expect(post2Mock.isDone()).toBe(false);
+
+            // Wait for the response with post1
+
+            await waitForNextUpdate();
+
+            expect(post1Mock.isDone()).toBe(true);
+            expect(post2Mock.isDone()).toBe(false);
+            expect(result.current).toEqual(post1);
+
+            // Switch to post2
+            postId = 'post2';
+            rerender();
+
+            expect(result.current).toEqual(undefined);
+
+            // Wait for the response with post2
+            await waitForNextUpdate();
+
+            expect(post1Mock.isDone()).toBe(true);
+            expect(post2Mock.isDone()).toBe(true);
+            expect(result.current).toEqual(post2);
+
+            // Switch back to post1 which has already been loaded
+            postId = 'post1';
+            rerender();
+
+            expect(result.current).toEqual(post1);
+
+            // We know there's no second call because nock is set to only mock the first request for each post
+        });
+
+        test('should batch multiple requests to fetch posts', async () => {
+            const mock = nock(Client4.getBaseRoute()).
+                post('/posts/ids', [post1.id, post2.id]).
+                once().
+                reply(200, [post1, post2]);
+
+            const {result, waitForNextUpdate} = renderHookWithContext(
+                () => {
+                    return [
+                        usePost('post1'),
+                        usePost('post2'),
+                    ];
+                },
+            );
+
+            // Initial state without post1 loaded
+            expect(result.current).toEqual([undefined, undefined]);
+            expect(mock.isDone()).toBe(false);
+
+            // Wait for the response
+            await waitForNextUpdate();
+
+            expect(result.current).toEqual([post1, post2]);
+            expect(mock.isDone()).toBe(true);
+        });
+    });
+});

webapp/channels/src/components/common/hooks/usePost.ts

@@ -0,0 +1,15 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+import type {Post} from '@mattermost/types/posts';
+
+import {getPostsByIdsBatched} from 'mattermost-redux/actions/posts';
+import {getPost} from 'mattermost-redux/selectors/entities/posts';
+
+import {makeUseEntity} from './useEntity';
+
+export const usePost = makeUseEntity<Post>({
+    name: 'usePost',
+    fetch: (postId: string) => getPostsByIdsBatched([postId]),
+    selector: getPost,
+});

webapp/channels/src/components/common/hooks/useUser.test.ts

@@ -0,0 +1,234 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+import nock from 'nock';
+import * as ReactRedux from 'react-redux';
+
+import {Client4} from 'mattermost-redux/client';
+
+import {renderHookWithContext} from 'tests/react_testing_utils';
+import {TestHelper} from 'utils/test_helper';
+
+import {useUser} from './useUser';
+
+describe('useUser', () => {
+    const user1 = TestHelper.getUserMock({id: 'user1'});
+    const user2 = TestHelper.getUserMock({id: 'user2'});
+
+    describe('useUser with fake dispatch', () => {
+        const dispatchMock = jest.fn();
+
+        beforeAll(() => {
+            jest.spyOn(ReactRedux, 'useDispatch').mockImplementation(() => dispatchMock);
+        });
+
+        afterAll(() => {
+            jest.restoreAllMocks();
+        });
+
+        test("should return the user if they're already in the store", () => {
+            const {result} = renderHookWithContext(
+                () => useUser('user1'),
+                {
+                    entities: {
+                        users: {
+                            profiles: {
+                                user1,
+                            },
+                        },
+                    },
+                },
+            );
+
+            expect(result.current).toBe(user1);
+            expect(dispatchMock).not.toHaveBeenCalled();
+        });
+
+        test("should fetch the user if they're not in the store", () => {
+            const {result} = renderHookWithContext(
+                () => useUser('user1'),
+            );
+
+            expect(result.current).toBe(undefined);
+            expect(dispatchMock).toHaveBeenCalledTimes(1);
+        });
+
+        test('should only attempt to fetch the user once regardless of how many times the hook is used', () => {
+            const {result, rerender} = renderHookWithContext(
+                () => useUser('user1'),
+            );
+
+            expect(result.current).toBe(undefined);
+            expect(dispatchMock).toHaveBeenCalledTimes(1);
+
+            for (let i = 0; i < 10; i++) {
+                rerender();
+            }
+
+            expect(result.current).toBe(undefined);
+            expect(dispatchMock).toHaveBeenCalledTimes(1);
+        });
+
+        test('should attempt to fetch different users if the user changes', () => {
+            let userId = 'user1';
+            const {result, rerender} = renderHookWithContext(
+                () => useUser(userId),
+            );
+
+            expect(result.current).toBe(undefined);
+            expect(dispatchMock).toHaveBeenCalledTimes(1);
+
+            userId = 'user2';
+            rerender();
+
+            expect(result.current).toBe(undefined);
+            expect(dispatchMock).toHaveBeenCalledTimes(2);
+        });
+
+        test("should only attempt to fetch each user once when they aren't loaded", () => {
+            let userId = 'user1';
+            const {result, replaceStoreState, rerender} = renderHookWithContext(
+                () => useUser(userId),
+            );
+
+            // Initial state without user1 loaded
+            expect(result.current).toBe(undefined);
+            expect(dispatchMock).toHaveBeenCalledTimes(1);
+
+            // Simulate the response to loading user1
+            replaceStoreState({
+                entities: {
+                    users: {
+                        profiles: {
+                            user1,
+                        },
+                    },
+                },
+            });
+
+            expect(result.current).toBe(user1);
+            expect(dispatchMock).toHaveBeenCalledTimes(1);
+
+            // Switch to user2
+            userId = 'user2';
+
+            rerender();
+
+            expect(result.current).toBe(undefined);
+            expect(dispatchMock).toHaveBeenCalledTimes(2);
+
+            // Simulate the response to loading user2
+            replaceStoreState({
+                entities: {
+                    users: {
+                        profiles: {
+                            user1,
+                            user2,
+                        },
+                    },
+                },
+            });
+
+            expect(result.current).toBe(user2);
+            expect(dispatchMock).toHaveBeenCalledTimes(2);
+
+            // Switch back to user1 which has already been loaded
+            userId = 'user1';
+
+            rerender();
+
+            expect(result.current).toBe(user1);
+            expect(dispatchMock).toHaveBeenCalledTimes(2);
+        });
+
+        test("shouldn't attempt to load anything when given an empty user ID", () => {
+            const {result} = renderHookWithContext(
+                () => useUser(''),
+            );
+
+            expect(result.current).toBe(undefined);
+            expect(dispatchMock).toHaveBeenCalledTimes(0);
+        });
+    });
+
+    describe('with real dispatch', () => {
+        beforeAll(() => {
+            Client4.setUrl('http://localhost:8065');
+        });
+
+        test("should only attempt to fetch each user once when they aren't loaded", async () => {
+            const user1Mock = nock(Client4.getBaseRoute()).
+                post('/users/ids', [user1.id]).
+                once().
+                reply(200, [user1]);
+            const user2Mock = nock(Client4.getBaseRoute()).
+                post('/users/ids', [user2.id]).
+                once().
+                reply(200, [user2]);
+
+            let userId = 'user1';
+            const {result, rerender, waitForNextUpdate} = renderHookWithContext(
+                () => useUser(userId),
+            );
+
+            // Initial state without user1 loaded
+            expect(result.current).toEqual(undefined);
+            expect(user1Mock.isDone()).toBe(false);
+            expect(user2Mock.isDone()).toBe(false);
+
+            // Wait for the response with user1
+            await waitForNextUpdate();
+
+            expect(user1Mock.isDone()).toBe(true);
+            expect(user2Mock.isDone()).toBe(false);
+            expect(result.current).toEqual(user1);
+
+            // Switch to user2
+            userId = 'user2';
+            rerender();
+
+            expect(result.current).toEqual(undefined);
+
+            // Wait for the response with user2
+            await waitForNextUpdate();
+
+            expect(user1Mock.isDone()).toBe(true);
+            expect(user2Mock.isDone()).toBe(true);
+            expect(result.current).toEqual(user2);
+
+            // Switch back to user1 which has already been loaded
+            userId = 'user1';
+            rerender();
+
+            expect(result.current).toEqual(user1);
+
+            // We know there's no second call because nock is set to only mock the first request for each user
+        });
+
+        test('should batch multiple requests to fetch users', async () => {
+            const mock = nock(Client4.getBaseRoute()).
+                post('/users/ids', [user1.id, user2.id]).
+                once().
+                reply(200, [user1, user2]);
+
+            const {result, waitForNextUpdate} = renderHookWithContext(
+                () => {
+                    return [
+                        useUser('user1'),
+                        useUser('user2'),
+                    ];
+                },
+            );
+
+            // Initial state without user1 loaded
+            expect(result.current).toEqual([undefined, undefined]);
+            expect(mock.isDone()).toBe(false);
+
+            // Wait for the response
+            await waitForNextUpdate();
+
+            expect(result.current).toEqual([user1, user2]);
+            expect(mock.isDone()).toBe(true);
+        });
+    });
+});

webapp/channels/src/components/common/hooks/useUser.ts

@@ -0,0 +1,15 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+import type {UserProfile} from '@mattermost/types/users';
+
+import {getMissingProfilesByIds} from 'mattermost-redux/actions/users';
+import {getUser} from 'mattermost-redux/selectors/entities/users';
+
+import {makeUseEntity} from './useEntity';
+
+export const useUser = makeUseEntity<UserProfile>({
+    name: 'useUser',
+    fetch: (userId) => getMissingProfilesByIds([userId]),
+    selector: (state, userId) => getUser(state, userId),
+});

webapp/channels/src/components/post/index.test.tsx

@@ -0,0 +1,176 @@
+// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
+// See LICENSE.txt for license information.
+
+import nock from 'nock';
+import React from 'react';
+
+import {CollapsedThreads} from '@mattermost/types/config';
+
+import {Client4} from 'mattermost-redux/client';
+
+import {renderWithContext, screen, waitFor} from 'tests/react_testing_utils';
+import {Locations} from 'utils/constants';
+import {TestHelper} from 'utils/test_helper';
+
+import ConnectedPostComponent from './index';
+
+describe('PostComponent', () => {
+    beforeAll(() => {
+        Client4.setUrl('http://localhost:8065');
+    });
+
+    test('MM-62710 should attempt to load missing root post when CRT is disabled', async () => {
+        const team1 = TestHelper.getTeamMock({id: 'team1'});
+        const channel1 = TestHelper.getChannelMock({id: 'channel1', team_id: team1.id});
+
+        const currentUser = TestHelper.getUserMock({id: 'currentUser', username: 'current_user'});
+        const otherUser = TestHelper.getUserMock({id: 'otherUser', username: 'other_user'});
+
+        const post1 = TestHelper.getPostMock({
+            id: 'post1',
+            user_id: otherUser.id,
+            channel_id: channel1.id,
+            create_at: 1000,
+            message: 'This is the root post that will need to be loaded',
+        });
+        const post2 = TestHelper.getPostMock({
+            id: 'post2',
+            user_id: currentUser.id,
+            channel_id: channel1.id,
+            create_at: 1001,
+            message: 'This is a different root post',
+        });
+        const post3 = TestHelper.getPostMock({
+            id: 'post3',
+            user_id: currentUser.id,
+            channel_id: channel1.id,
+            root_id: post1.id,
+            create_at: 1002,
+            message: 'This is the test post',
+        });
+
+        const postsMock = nock(Client4.getBaseRoute()).
+            post('/posts/ids', [post1.id]).
+            reply(200, [post1]);
+        const usersMock = nock(Client4.getBaseRoute()).
+            post('/users/ids', [otherUser.id]).
+            reply(200, [otherUser]);
+
+        renderWithContext(
+            <ConnectedPostComponent
+                location={Locations.CENTER}
+                post={post3}
+                previousPostId={post2.id}
+            />,
+            {
+                entities: {
+                    channels: {
+                        channels: {
+                            channel1,
+                        },
+                    },
+                    posts: {
+                        posts: {
+                            post2,
+                        },
+                    },
+                },
+            },
+        );
+
+        expect(screen.getByText('This is the test post')).toBeInTheDocument();
+
+        // The Commented on line will be missing until the root post and user are loaded
+        expect(screen.queryByText('other_user')).not.toBeInTheDocument();
+        expect(screen.queryByText('This is the root post that will need to be loaded')).not.toBeInTheDocument();
+
+        // The required user and post will be loaded async
+        await waitFor(() => {
+            expect(screen.queryByText('other_user')).toBeInTheDocument();
+            expect(screen.queryByText('This is the root post that will need to be loaded')).toBeInTheDocument();
+        });
+
+        expect(usersMock.isDone()).toBe(true);
+        expect(postsMock.isDone()).toBe(true);
+    });
+
+    test('should not attempt to load missing root post when CRT is enabled', async () => {
+        const team1 = TestHelper.getTeamMock({id: 'team1'});
+        const channel1 = TestHelper.getChannelMock({id: 'channel1', team_id: team1.id});
+
+        const currentUser = TestHelper.getUserMock({id: 'currentUser', username: 'current_user'});
+        const otherUser = TestHelper.getUserMock({id: 'otherUser', username: 'other_user'});
+
+        const post1 = TestHelper.getPostMock({
+            id: 'post1',
+            user_id: otherUser.id,
+            channel_id: channel1.id,
+            create_at: 1000,
+            message: 'This is the root post that will need to be loaded',
+        });
+        const post2 = TestHelper.getPostMock({
+            id: 'post2',
+            user_id: currentUser.id,
+            channel_id: channel1.id,
+            create_at: 1001,
+            message: 'This is a different root post',
+        });
+        const post3 = TestHelper.getPostMock({
+            id: 'post3',
+            user_id: currentUser.id,
+            channel_id: channel1.id,
+            root_id: post1.id,
+            create_at: 1002,
+            message: 'This is the test post',
+        });
+
+        const postsMock = nock(Client4.getBaseRoute()).
+            post('/posts/ids', [post1.id]).
+            reply(200, [post1]);
+        const usersMock = nock(Client4.getBaseRoute()).
+            post('/users/ids', [otherUser.id]).
+            reply(200, [otherUser]);
+
+        renderWithContext(
+            <ConnectedPostComponent
+                location={Locations.CENTER}
+                post={post3}
+                previousPostId={post2.id}
+            />,
+            {
+                entities: {
+                    channels: {
+                        channels: {
+                            channel1,
+                        },
+                    },
+                    general: {
+                        config: {
+                            CollapsedThreads: CollapsedThreads.ALWAYS_ON,
+                        },
+                    },
+                    posts: {
+                        posts: {
+                            post2,
+                        },
+                    },
+                },
+            },
+        );
+
+        expect(screen.getByText('This is the test post')).toBeInTheDocument();
+
+        // The Commented on line will be missing until the root post and user are loaded
+        expect(screen.queryByText('other_user')).not.toBeInTheDocument();
+        expect(screen.queryByText('This is the root post that will need to be loaded')).not.toBeInTheDocument();
+
+        // The required user and post will be loaded async
+        await waitFor(() => {
+            expect(screen.queryByText('other_user')).toBeInTheDocument();
+            expect(screen.queryByText('This is the root post that will need to be loaded')).toBeInTheDocument();
+        });
+
+        expect(usersMock.isDone()).toBe(true);
+        expect(postsMock.isDone()).toBe(true);
+    });
+});

webapp/channels/src/components/post/post_component.tsx

@@ -402,12 +402,11 @@ function PostComponent(props: Props) {
     const postClass = classNames('post__body', {'post--edited': PostUtils.isEdited(post), 'search-item-snippet': isSearchResultItem});
 
     let comment;
-    if (props.isFirstReply && props.parentPost && props.parentPostUser && post.type !== Constants.PostTypes.EPHEMERAL) {
+    if (props.isFirstReply && post.type !== Constants.PostTypes.EPHEMERAL) {
         comment = (
             <CommentedOn
-                post={props.parentPost}
-                parentPostUser={props.parentPostUser}
                 onCommentClick={handleCommentClick}
+                rootId={post.root_id}
             />
         );
     }

webapp/channels/src/components/post_view/commented_on/__snapshots__/commented_on.test.tsx.snap

@@ -1,220 +0,0 @@
-// Jest Snapshot v1, https://goo.gl/fbAQLP
-
-exports[`components/post_view/CommentedOn should match snapshot 1`] = `
-<div
-  className="post__link"
-  data-testid="post-link"
->
-  <span>
-    <MemoizedFormattedMessage
-      defaultMessage="Commented on {name}'s message: "
-      id="post_body.commentedOn"
-      values={
-        Object {
-          "name": <a
-            className="theme user_name"
-          >
-            <Memo(Connect(UserProfile))
-              userId=""
-            />
-          </a>,
-        }
-      }
-    />
-    <a
-      className="theme"
-      onClick={[MockFunction]}
-    >
-      text message
-    </a>
-  </span>
-</div>
-`;
-
-exports[`components/post_view/CommentedOn should match snapshot 2`] = `
-<div
-  className="post__link"
-  data-testid="post-link"
->
-  <span>
-    <MemoizedFormattedMessage
-      defaultMessage="Commented on {name}'s message: "
-      id="post_body.commentedOn"
-      values={
-        Object {
-          "name": <a
-            className="theme user_name"
-          >
-            <Memo(Connect(UserProfile))
-              userId=""
-            />
-          </a>,
-        }
-      }
-    />
-    <a
-      className="theme"
-      onClick={[MockFunction]}
-    >
-      text message
-    </a>
-  </span>
-</div>
-`;
-
-exports[`components/post_view/CommentedOn should match snapshot 3`] = `
-<div
-  className="post__link"
-  data-testid="post-link"
->
-  <span>
-    <MemoizedFormattedMessage
-      defaultMessage="Commented on {name}'s message: "
-      id="post_body.commentedOn"
-      values={
-        Object {
-          "name": <a
-            className="theme user_name"
-          >
-            <Memo(Connect(UserProfile))
-              userId=""
-            />
-          </a>,
-        }
-      }
-    />
-    <a
-      className="theme"
-      onClick={[MockFunction]}
-    >
-      <Connect(Component)
-        parentPostId="post_id"
-      />
-    </a>
-  </span>
-</div>
-`;
-
-exports[`components/post_view/CommentedOn should match snapshots for post with props.fallback as message 1`] = `
-<div
-  className="post__link"
-  data-testid="post-link"
->
-  <span>
-    <MemoizedFormattedMessage
-      defaultMessage="Commented on {name}'s message: "
-      id="post_body.commentedOn"
-      values={
-        Object {
-          "name": <a
-            className="theme user_name"
-          >
-            <Memo(Connect(UserProfile))
-              userId=""
-            />
-          </a>,
-        }
-      }
-    />
-    <a
-      className="theme"
-      onClick={[MockFunction]}
-    >
-      This is fallback message
-    </a>
-  </span>
-</div>
-`;
-
-exports[`components/post_view/CommentedOn should match snapshots for post with props.pretext as message 1`] = `
-<div
-  className="post__link"
-  data-testid="post-link"
->
-  <span>
-    <MemoizedFormattedMessage
-      defaultMessage="Commented on {name}'s message: "
-      id="post_body.commentedOn"
-      values={
-        Object {
-          "name": <a
-            className="theme user_name"
-          >
-            <Memo(Connect(UserProfile))
-              userId=""
-            />
-          </a>,
-        }
-      }
-    />
-    <a
-      className="theme"
-      onClick={[MockFunction]}
-    >
-      This is a pretext
-    </a>
-  </span>
-</div>
-`;
-
-exports[`components/post_view/CommentedOn should match snapshots for post with props.text as message 1`] = `
-<div
-  className="post__link"
-  data-testid="post-link"
->
-  <span>
-    <MemoizedFormattedMessage
-      defaultMessage="Commented on {name}'s message: "
-      id="post_body.commentedOn"
-      values={
-        Object {
-          "name": <a
-            className="theme user_name"
-          >
-            <Memo(Connect(UserProfile))
-              userId=""
-            />
-          </a>,
-        }
-      }
-    />
-    <a
-      className="theme"
-      onClick={[MockFunction]}
-    >
-      This is a text
-    </a>
-  </span>
-</div>
-`;
-
-exports[`components/post_view/CommentedOn should match snapshots for post with props.title as message 1`] = `
-<div
-  className="post__link"
-  data-testid="post-link"
->
-  <span>
-    <MemoizedFormattedMessage
-      defaultMessage="Commented on {name}'s message: "
-      id="post_body.commentedOn"
-      values={
-        Object {
-          "name": <a
-            className="theme user_name"
-          >
-            <Memo(Connect(UserProfile))
-              userId=""
-            />
-          </a>,
-        }
-      }
-    />
-    <a
-      className="theme"
-      onClick={[MockFunction]}
-    >
-      This is a title
-    </a>
-  </span>
-</div>
-`;

webapp/channels/src/components/post_view/commented_on/commented_on.test.tsx

@@ -1,173 +1,293 @@
 // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
 // See LICENSE.txt for license information.
 
-import {shallow} from 'enzyme';
 import React from 'react';
 
 import CommentedOn from 'components/post_view/commented_on/commented_on';
-import CommentedOnFilesMessage from 'components/post_view/commented_on_files_message';
 
+import {renderWithContext, screen} from 'tests/react_testing_utils';
 import {TestHelper} from 'utils/test_helper';
 
 describe('components/post_view/CommentedOn', () => {
-    const baseProps = {
-        displayName: 'user_displayName',
-        enablePostUsernameOverride: false,
-        onCommentClick: jest.fn(),
-        post: TestHelper.getPostMock({
-            id: 'post_id',
-            message: 'text message',
-            props: {
-                from_webhook: 'true',
-                override_username: 'override_username',
-            },
-            update_at: 10,
-            edit_at: 20,
-            delete_at: 30,
-            channel_id: 'channel_id',
-            root_id: 'root_id',
-            original_id: 'original_id',
-            hashtags: 'hashtags',
-            pending_post_id: 'pending_post_id',
-            reply_count: 1,
-        }),
-    };
-
-    test('should match snapshot', () => {
-        const wrapper = shallow(<CommentedOn {...baseProps}/>);
-        expect(wrapper).toMatchSnapshot();
-
-        wrapper.setProps({enablePostUsernameOverride: true});
-        expect(wrapper).toMatchSnapshot();
-        expect(wrapper.find(CommentedOnFilesMessage).exists()).toBe(false);
-
-        const newPost = {
-            id: 'post_id',
-            message: '',
-            file_ids: ['file_id_1', 'file_id_2'],
-        };
-        wrapper.setProps({post: newPost, enablePostUsernameOverride: false});
-        expect(wrapper).toMatchSnapshot();
-        expect(wrapper.find(CommentedOnFilesMessage).exists()).toBe(true);
+    const user1 = TestHelper.getUserMock({
+        id: 'user1',
+    });
+    const post1 = TestHelper.getPostMock({
+        id: 'post1',
+        user_id: user1.id,
+        message: 'text message',
     });
 
-    test('should match snapshots for post with props.pretext as message', () => {
-        const newPost = {
-            id: 'post_id',
-            message: '',
-            props: {
-                from_webhook: 'true',
-                override_username: 'override_username',
-                attachments: [{
-                    pretext: 'This is a pretext',
-                }],
+    test("should render the root post's message and author", () => {
+        renderWithContext(
+            <CommentedOn rootId={post1.id}/>,
+            {
+                entities: {
+                    posts: {
+                        posts: {
+                            post1,
+                        },
+                    },
+                    users: {
+                        profiles: {
+                            user1,
+                        },
+                    },
+                },
             },
-        };
-        const newProps = {
-            ...baseProps,
-            post: {
-                ...baseProps.post,
-                ...newPost,
-            },
-            enablePostUsernameOverride: true,
-        };
+        );
 
-        const wrapper = shallow(<CommentedOn {...newProps}/>);
-        expect(wrapper).toMatchSnapshot();
+        expect(screen.getByText(textInChildren("Commented on some-user's message: text message"))).toBeInTheDocument();
     });
 
-    test('should match snapshots for post with props.title as message', () => {
-        const newPost = {
-            id: 'post_id',
-            message: '',
-            props: {
-                from_webhook: 'true',
-                override_username: 'override_username',
-                attachments: [{
-                    pretext: '',
-                    title: 'This is a title',
-                }],
+    test("should render a placeholder name when the post's author isn't loaded", () => {
+        renderWithContext(
+            <CommentedOn rootId={post1.id}/>,
+            {
+                entities: {
+                    posts: {
+                        posts: {
+                            post1,
+                        },
+                    },
+                },
             },
-        };
-        const newProps = {
-            ...baseProps,
-            post: {
-                ...baseProps.post,
-                ...newPost,
-            },
-            enablePostUsernameOverride: true,
-        };
+        );
 
-        const wrapper = shallow(<CommentedOn {...newProps}/>);
-        expect(wrapper).toMatchSnapshot();
+        expect(screen.getByText(textInChildren("Commented on Someone's message: text message"))).toBeInTheDocument();
     });
 
-    test('should match snapshots for post with props.text as message', () => {
-        const newPost = {
-            id: 'post_id',
-            message: '',
-            props: {
-                from_webhook: 'true',
-                override_username: 'override_username',
-                attachments: [{
-                    pretext: '',
-                    title: '',
-                    text: 'This is a text',
-                }],
-            },
-        };
+    test("should render a placeholder when the post isn't loaded", () => {
+        renderWithContext(
+            <CommentedOn rootId={post1.id}/>,
+        );
 
-        const newProps = {
-            ...baseProps,
-            post: {
-                ...baseProps.post,
-                ...newPost,
-            },
-            enablePostUsernameOverride: true,
-        };
-
-        const wrapper = shallow(<CommentedOn {...newProps}/>);
-        expect(wrapper).toMatchSnapshot();
+        expect(screen.getByText(textInChildren("Commented on Someone's message: Loading…"))).toBeInTheDocument();
     });
 
-    test('should match snapshots for post with props.fallback as message', () => {
-        const newPost = {
-            id: 'post_id',
-            message: '',
-            props: {
-                from_webhook: 'true',
-                override_username: 'override_username',
-                attachments: [{
-                    pretext: '',
-                    title: '',
-                    text: '',
-                    fallback: 'This is fallback message',
-                }],
-            },
-        };
+    test("should render the root post's file attachments when it has no message", () => {
+        const file1 = TestHelper.getFileInfoMock({id: 'file1', create_at: 1000, name: 'image.png'});
+        const file2 = TestHelper.getFileInfoMock({id: 'file2', create_at: 1001, name: 'contract.doc'});
 
-        const newProps = {
-            ...baseProps,
-            post: {
-                ...baseProps.post,
-                ...newPost,
+        renderWithContext(
+            <CommentedOn rootId={post1.id}/>,
+            {
+                entities: {
+                    files: {
+                        fileIdsByPostId: {
+                            post1: [file1.id, file2.id],
+                        },
+                        files: {
+                            file1,
+                            file2,
+                        },
+                    },
+                    posts: {
+                        posts: {
+                            [post1.id]: {
+                                ...post1,
+                                message: '',
+                                file_ids: [file1.id, file2.id],
+                            },
+                        },
+                    },
+                    users: {
+                        profiles: {
+                            user1,
+                        },
+                    },
+                },
             },
-            enablePostUsernameOverride: true,
-        };
+        );
 
-        const wrapper = shallow(<CommentedOn {...newProps}/>);
-        expect(wrapper).toMatchSnapshot();
+        expect(screen.getByText(textInChildren("Commented on some-user's message: image.png plus 1 other file"))).toBeInTheDocument();
+    });
+
+    test("should render the root post's props.pretext as message", () => {
+        renderWithContext(
+            <CommentedOn rootId={post1.id}/>,
+            {
+                entities: {
+                    general: {
+                        config: {
+                            EnablePostUsernameOverride: 'true',
+                        },
+                    },
+                    posts: {
+                        posts: {
+                            [post1.id]: {
+                                ...post1,
+                                message: '',
+                                props: {
+                                    from_webhook: 'true',
+                                    override_username: 'override_username',
+                                    attachments: [{
+                                        pretext: 'This is a pretext',
+                                    }],
+                                },
+                            },
+                        },
+                    },
+                    users: {
+                        profiles: {
+                            user1,
+                        },
+                    },
+                },
+            },
+        );
+
+        // This incorrectly uses the post author's name due to MM-63564
+        expect(screen.getByText(textInChildren("Commented on some-user's message: This is a pretext"))).toBeInTheDocument();
+    });
+
+    test("should render the root post's props.title as message", () => {
+        renderWithContext(
+            <CommentedOn rootId={post1.id}/>,
+            {
+                entities: {
+                    general: {
+                        config: {
+                            EnablePostUsernameOverride: 'true',
+                        },
+                    },
+                    posts: {
+                        posts: {
+                            [post1.id]: {
+                                ...post1,
+                                message: '',
+                                props: {
+                                    from_webhook: 'true',
+                                    override_username: 'override_username',
+                                    attachments: [{
+                                        title: 'This is a title',
+                                    }],
+                                },
+                            },
+                        },
+                    },
+                    users: {
+                        profiles: {
+                            user1,
+                        },
+                    },
+                },
+            },
+        );
+
+        // This incorrectly uses the post author's name due to MM-63564
+        expect(screen.getByText(textInChildren("Commented on some-user's message: This is a title"))).toBeInTheDocument();
+    });
+
+    test("should render the root post's props.text as message", () => {
+        renderWithContext(
+            <CommentedOn rootId={post1.id}/>,
+            {
+                entities: {
+                    general: {
+                        config: {
+                            EnablePostUsernameOverride: 'true',
+                        },
+                    },
+                    posts: {
+                        posts: {
+                            [post1.id]: {
+                                ...post1,
+                                message: '',
+                                props: {
+                                    from_webhook: 'true',
+                                    override_username: 'override_username',
+                                    attachments: [{
+                                        text: 'This is a text',
+                                    }],
+                                },
+                            },
+                        },
+                    },
+                    users: {
+                        profiles: {
+                            user1,
+                        },
+                    },
+                },
+            },
+        );
+
+        // This incorrectly uses the post author's name due to MM-63564
+        expect(screen.getByText(textInChildren("Commented on some-user's message: This is a text"))).toBeInTheDocument();
+    });
+
+    test("should render the root post's props.fallback as message", () => {
+        renderWithContext(
+            <CommentedOn rootId={post1.id}/>,
+            {
+                entities: {
+                    general: {
+                        config: {
+                            EnablePostUsernameOverride: 'true',
+                        },
+                    },
+                    posts: {
+                        posts: {
+                            [post1.id]: {
+                                ...post1,
+                                message: '',
+                                props: {
+                                    from_webhook: 'true',
+                                    override_username: 'override_username',
+                                    attachments: [{
+                                        fallback: 'This is fallback message',
+                                    }],
+                                },
+                            },
+                        },
+                    },
+                    users: {
+                        profiles: {
+                            user1,
+                        },
+                    },
+                },
+            },
+        );
+
+        // This incorrectly uses the post author's name due to MM-63564
+        expect(screen.getByText(textInChildren("Commented on some-user's message: This is fallback message"))).toBeInTheDocument();
     });
 
     test('should call onCommentClick on click of text message', () => {
-        const wrapper = shallow(<CommentedOn {...baseProps}/>);
+        const onCommentClick = jest.fn();
+        renderWithContext(
+            <CommentedOn
+                onCommentClick={onCommentClick}
+                rootId={post1.id}
+            />,
+            {
+                entities: {
+                    posts: {
+                        posts: {
+                            post1,
+                        },
+                    },
+                    users: {
+                        profiles: {
+                            user1,
+                        },
+                    },
+                },
+            },
+        );
 
-        wrapper.find('a').first().simulate('click');
-        expect(baseProps.onCommentClick).toHaveBeenCalledTimes(1);
-    });
+        screen.getByText('text message').click();
 
-    test('Should trigger search with override_username', () => {
-        const wrapper = shallow(<CommentedOn {...baseProps}/>);
-        wrapper.setProps({enablePostUsernameOverride: true});
+        expect(onCommentClick).toHaveBeenCalledTimes(1);
     });
 });
+
+function textInChildren(matchedText: string) {
+    return (content: string, element: Element | null) => {
+        const hasText = element?.textContent === matchedText;
+        const childHasText = element && Array.from(element?.children).some((child) => child?.textContent === matchedText);
+        return hasText && !childHasText;
+    };
+}

webapp/channels/src/components/post_view/commented_on/commented_on.tsx

@@ -5,9 +5,9 @@ import React, {memo} from 'react';
 import {FormattedMessage} from 'react-intl';
 
 import {isMessageAttachmentArray} from '@mattermost/types/message_attachments';
-import type {Post} from '@mattermost/types/posts';
-import type {UserProfile as UserProfileType} from '@mattermost/types/users';
 
+import {usePost} from 'components/common/hooks/usePost';
+import {useUser} from 'components/common/hooks/useUser';
 import CommentedOnFilesMessage from 'components/post_view/commented_on_files_message';
 import UserProfile from 'components/user_profile';
 
@@ -15,36 +15,37 @@ import {stripMarkdown} from 'utils/markdown';
 import * as Utils from 'utils/utils';
 
 type Props = {
-    enablePostUsernameOverride?: boolean;
-    parentPostUser?: UserProfileType;
     onCommentClick?: React.EventHandler<React.MouseEvent>;
-    post: Post;
+    rootId: string;
 };
 
-function CommentedOn({post, parentPostUser, onCommentClick}: Props) {
-    const makeCommentedOnMessage = () => {
-        let message: React.ReactNode = '';
-        if (post.message) {
-            message = Utils.replaceHtmlEntities(post.message);
-        } else if (post.file_ids && post.file_ids.length > 0) {
-            message = (
-                <CommentedOnFilesMessage parentPostId={post.id}/>
-            );
-        } else if (isMessageAttachmentArray(post.props?.attachments) && post.props.attachments.length > 0) {
-            const attachment = post.props.attachments[0];
-            const webhookMessage = attachment.pretext || attachment.title || attachment.text || attachment.fallback || '';
-            message = Utils.replaceHtmlEntities(webhookMessage);
-        }
+function CommentedOn({onCommentClick, rootId}: Props) {
+    const rootPost = usePost(rootId);
+    const rootPostUser = useUser(rootPost?.user_id ?? '');
 
-        return message;
-    };
-
-    const message = makeCommentedOnMessage();
-    const parentPostUserId = parentPostUser?.id ?? '';
+    let message: React.ReactNode = '';
+    if (!rootPost) {
+        message = (
+            <FormattedMessage
+                id='post_body.commentedOn.loadingMessage'
+                defaultMessage='Loading…'
+            />
+        );
+    } else if (rootPost.message) {
+        message = Utils.replaceHtmlEntities(rootPost.message);
+    } else if (rootPost.file_ids && rootPost.file_ids.length > 0) {
+        message = (
+            <CommentedOnFilesMessage parentPostId={rootPost.id}/>
+        );
+    } else if (isMessageAttachmentArray(rootPost.props?.attachments) && rootPost.props.attachments.length > 0) {
+        const attachment = rootPost.props.attachments[0];
+        const webhookMessage = attachment.pretext || attachment.title || attachment.text || attachment.fallback || '';
+        message = Utils.replaceHtmlEntities(webhookMessage);
+    }
 
     const parentUserProfile = (
         <UserProfile
-            userId={parentPostUserId}
+            userId={rootPostUser?.id ?? ''}
         />
     );
 

webapp/channels/src/components/post_view/commented_on/index.ts

@@ -1,37 +1,6 @@
 // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
 // See LICENSE.txt for license information.
 
-import {connect} from 'react-redux';
-
-import type {Post} from '@mattermost/types/posts';
-
-import {getConfig} from 'mattermost-redux/selectors/entities/general';
-import {getUser} from 'mattermost-redux/selectors/entities/users';
-
-import {getDisplayNameByUser} from 'utils/utils';
-
-import type {GlobalState} from 'types/store';
-
 import CommentedOn from './commented_on';
 
-type Props = {
-    post: Post;
-}
-
-function mapStateToProps(state: GlobalState, ownProps: Props) {
-    let displayName = '';
-    if (ownProps.post) {
-        const user = getUser(state, ownProps.post.user_id);
-        displayName = getDisplayNameByUser(state, user);
-    }
-
-    const config = getConfig(state);
-    const enablePostUsernameOverride = config.EnablePostUsernameOverride === 'true';
-
-    return {
-        displayName,
-        enablePostUsernameOverride,
-    };
-}
-
-export default connect(mapStateToProps)(CommentedOn);
+export default CommentedOn;

webapp/channels/src/i18n/en.json

@@ -4684,6 +4684,7 @@
   "post_body.check_for_out_of_channel_mentions.message.one": "did not get notified by this mention because they are not in the channel. Would you like to ",
   "post_body.check_for_out_of_channel_mentions.others": "{numOthers} others",
   "post_body.commentedOn": "Commented on {name}'s message: ",
+  "post_body.commentedOn.loadingMessage": "Loading…",
   "post_body.deleted": "(message deleted)",
   "post_body.plusMore": " plus {count, number} other {count, plural, one {file} other {files}}",
   "post_delete.notPosted": "Comment could not be posted",

webapp/channels/src/packages/mattermost-redux/src/actions/posts.ts

@@ -35,6 +35,7 @@ import * as PostSelectors from 'mattermost-redux/selectors/entities/posts';
 import {getUnreadScrollPositionPreference, isCollapsedThreadsEnabled} from 'mattermost-redux/selectors/entities/preferences';
 import {getCurrentUserId, getUsersByUsername} from 'mattermost-redux/selectors/entities/users';
 import type {ActionResult, DispatchFunc, GetStateFunc, ActionFunc, ActionFuncAsync, ThunkActionFunc} from 'mattermost-redux/types/actions';
+import {DelayedDataLoader} from 'mattermost-redux/utils/data_loader';
 import {isCombinedUserActivityPost} from 'mattermost-redux/utils/post_list';
 
 import {logError, LogErrorBarMode} from './errors';
@@ -164,7 +165,6 @@ export function getPost(postId: string): ActionFuncAsync<Post> {
         }
 
         dispatch(receivedPost(post, crtEnabled));
-        dispatch(batchFetchStatusesProfilesGroupsFromPosts([post]));
 
         return {data: post};
     };
@@ -1035,6 +1035,25 @@ export function getPostsByIds(ids: string[]): ActionFuncAsync {
     };
 }
 
+export function getPostsByIdsBatched(postIds: string[]): ActionFuncAsync<true> {
+    const maxBatchSize = 100;
+    const wait = 100;
+
+    return async (dispatch, getState, {loaders}: any) => {
+        if (!loaders.postsByIdsLoader) {
+            loaders.postsByIdsLoader = new DelayedDataLoader<Post['id']>({
+                fetchBatch: (postIds) => dispatch(getPostsByIds(postIds)),
+                maxBatchSize,
+                wait,
+            });
+        }
+
+        loaders.postsByIdsLoader.queue(postIds);
+
+        return {data: true};
+    };
+}
+
 export function getPostEditHistory(postId: string) {
     return bindClientFunc({
         clientFunc: Client4.getPostEditHistory,

webapp/channels/src/tests/react_testing_utils.tsx

@@ -115,12 +115,25 @@ export const renderHookWithContext = <TProps, TResult>(
     };
     replaceGlobalStore(() => renderState.store);
 
-    return renderHook(callback, {
+    const results = renderHook(callback, {
         wrapper: ({children}) => {
             // Every time this is called, these values should be updated from `renderState`
             return <Providers {...renderState}>{children}</Providers>;
         },
     });
+
+    return {
+        ...results,
+
+        /**
+         * Rerenders the component after replacing the entire store state with the provided one.
+         */
+        replaceStoreState: (newInitialState: DeepPartial<GlobalState>) => {
+            renderState.store = configureOrMockStore(newInitialState, renderState.options.useMockedStore, partialOptions?.pluginReducers);
+
+            results.rerender();
+        },
+    };
 };
 
 function configureOrMockStore<T>(initialState: DeepPartial<T>, useMockedStore: boolean, extraReducersKeys?: string[]) {