MM-69886: Refresh Channel Members RHS on websocket add and reconnect (#37584)

* MM-69886: Refresh Channel Members RHS on websocket add and reconnect

The Channel Members RHS (participant list) could omit members in two
general cases:

- On a user_added websocket event, handleUserAddedEvent recorded the
  profile-in-channel entry and loaded the profile but never loaded the
  ChannelMembership. buildProfileList drops any user without a
  membership, so a member added while the channel was open did not
  appear until the list was fully reloaded. It also affected members
  whose profile was already loaded (e.g. after they posted) but who
  still had no membership. Fetch the membership via getChannelMember
  when it is missing.

- On websocket reconnect, reconnect() reloads only the current user's
  own memberships, never the channel roster, and missed user_added /
  user_removed events are not replayed on a fresh (non-resumed)
  connection. Members added or removed while disconnected stayed stale
  until the channel was re-opened. The RHS now reloads the roster when
  the websocket connection id changes.

This extends the reconcile-on-load work from MM-68660 (PR #36964, same
loadProfilesAndReloadChannelMembers path), which pruned removals only
on channel (re)load and did not cover live adds or reconnects.

Found while re-testing MM-67616 (Shared Channels), where synced members
were not reflected in the participant list even though the server-side
sync was correct.

* Add comment to caution about changing remote cluster service ping frequency.

server/platform/services/remotecluster/service.go

@@ -27,14 +27,16 @@ const (
 	SendTimeout                   = time.Minute
 	SendFileTimeout               = time.Minute * 5
 	PingURL                       = "api/v4/remotecluster/ping"
-	PingFreq                      = time.Minute
-	PingTimeout                   = time.Second * 15
-	ConfirmInviteURL              = "api/v4/remotecluster/confirm_invite"
-	InvitationTopic               = "invitation"
-	PingTopic                     = "ping"
-	ResponseStatusOK              = model.StatusOk
-	ResponseStatusFail            = model.StatusFail
-	InviteExpiresAfter            = time.Hour * 48
+	// Raising PingFreq also raises the transport's IdleConnTimeout (PingFreq / 2),
+	// which must stay below peer idle timeouts (60s default). See MM-69982.
+	PingFreq           = time.Minute
+	PingTimeout        = time.Second * 15
+	ConfirmInviteURL   = "api/v4/remotecluster/confirm_invite"
+	InvitationTopic    = "invitation"
+	PingTopic          = "ping"
+	ResponseStatusOK   = model.StatusOk
+	ResponseStatusFail = model.StatusFail
+	InviteExpiresAfter = time.Hour * 48
 )
 
 type ServerIface interface {

webapp/channels/src/actions/websocket_actions.test.jsx

@@ -7,7 +7,7 @@ import {WebSocketEvents} from '@mattermost/client';
 
 import {ChannelTypes, CloudTypes, JobTypes, TeamTypes} from 'mattermost-redux/action_types';
 import {fetchMyCategories} from 'mattermost-redux/actions/channel_categories';
-import {fetchAllMyTeamsChannels} from 'mattermost-redux/actions/channels';
+import {fetchAllMyTeamsChannels, getChannelMember} from 'mattermost-redux/actions/channels';
 import {getCustomProfileAttributeFields} from 'mattermost-redux/actions/general';
 import {getGroup} from 'mattermost-redux/actions/groups';
 import {getJobsByType} from 'mattermost-redux/actions/jobs';
@@ -100,6 +100,7 @@ jest.mock('mattermost-redux/actions/users', () => ({
 
 jest.mock('mattermost-redux/actions/channels', () => ({
     getChannelStats: jest.fn(() => ({type: 'GET_CHANNEL_STATS'})),
+    getChannelMember: jest.fn(() => ({type: 'GET_CHANNEL_MEMBER'})),
     fetchAllMyChannelMembers: jest.fn(() => ({type: 'FETCH_ALL_MY_CHANNEL_MEMBERS'})),
     fetchAllMyTeamsChannels: jest.fn(),
 }));
@@ -471,7 +472,26 @@ describe('handleUserAddedEvent', () => {
         },
     };
 
-    test('should load the added user profile when it is not already in the store', async () => {
+    // A state where the added user already has both a loaded profile and a loaded
+    // ChannelMembership in the current channel, so nothing needs to be fetched.
+    const stateWithLoadedMember = mergeObjects(stateWithLicense, {
+        entities: {
+            users: {
+                profiles: {
+                    loadedMember: {id: 'loadedMember', roles: 'system_user'},
+                },
+            },
+            channels: {
+                membersInChannel: {
+                    [currentChannelId]: {
+                        loadedMember: {channel_id: currentChannelId, user_id: 'loadedMember'},
+                    },
+                },
+            },
+        },
+    });
+
+    test('should load both the profile and the channel membership for a newly synced remote member', async () => {
         const testStore = configureStore(stateWithLicense);
         const msg = {
             data: {
@@ -483,10 +503,19 @@ describe('handleUserAddedEvent', () => {
         };
 
         await testStore.dispatch(handleUserAddedEvent(msg));
+
+        // Both the profile and the ChannelMembership are required for the member to
+        // render in the participant list.
         expect(getUser).toHaveBeenCalledWith('remoteUser');
+        expect(getChannelMember).toHaveBeenCalledWith(currentChannelId, 'remoteUser');
     });
 
-    test('should not load the added user profile when it is already in the store', async () => {
+    test('should still load the channel membership when the profile is already loaded but the membership is not', async () => {
+        // Regression test for MM-67616: a synced remote member whose profile was
+        // already loaded (e.g. because they posted) but who has no ChannelMembership
+        // was silently dropped from the participant list, because the member list
+        // requires the membership relation. The membership must be fetched even when
+        // the profile fetch is skipped.
         const testStore = configureStore(stateWithLicense);
         const msg = {
             data: {
@@ -498,10 +527,29 @@ describe('handleUserAddedEvent', () => {
         };
 
         await testStore.dispatch(handleUserAddedEvent(msg));
+
         expect(getUser).not.toHaveBeenCalled();
+        expect(getChannelMember).toHaveBeenCalledWith(currentChannelId, 'user');
     });
 
-    test('should not load the added user profile when the channel is not the current channel', async () => {
+    test('should not load the profile or membership when both are already in the store', async () => {
+        const testStore = configureStore(stateWithLoadedMember);
+        const msg = {
+            data: {
+                user_id: 'loadedMember',
+            },
+            broadcast: {
+                channel_id: currentChannelId,
+            },
+        };
+
+        await testStore.dispatch(handleUserAddedEvent(msg));
+
+        expect(getUser).not.toHaveBeenCalled();
+        expect(getChannelMember).not.toHaveBeenCalled();
+    });
+
+    test('should not load the profile or membership when the channel is not the current channel', async () => {
         const testStore = configureStore(stateWithLicense);
         const msg = {
             data: {
@@ -513,7 +561,9 @@ describe('handleUserAddedEvent', () => {
         };
 
         await testStore.dispatch(handleUserAddedEvent(msg));
+
         expect(getUser).not.toHaveBeenCalled();
+        expect(getChannelMember).not.toHaveBeenCalled();
     });
 });
 

webapp/channels/src/actions/websocket_actions.ts

@@ -52,6 +52,7 @@ import {fetchAppBindings, fetchRHSAppsBindings} from 'mattermost-redux/actions/a
 import {addChannelToInitialCategory, fetchMyCategories, handleManagedCategoryPropertyValuesUpdated, receivedCategoryOrder} from 'mattermost-redux/actions/channel_categories';
 import {
     getChannelAndMyMember,
+    getChannelMember,
     getMyChannelMember,
     getChannelStats,
     markMultipleChannelsAsRead,
@@ -1384,14 +1385,23 @@ export function handleUserAddedEvent(msg: WebSocketMessages.UserAddedToChannel):
                 data: {id: msg.broadcast.channel_id, user_id: msg.data.user_id},
             });
 
-            // The membership relation alone is not enough to render the member in the
-            // participant list: the member list selectors drop users whose profile is
-            // not loaded. This happens for remote users synced into a shared channel,
-            // since the viewer has never loaded their profile. Fetch it if missing.
+            // The member list selectors drop users whose profile is not loaded. This
+            // happens for remote users synced into a shared channel, since the viewer
+            // has never loaded their profile. Fetch it if missing.
             if (!getUser(state, msg.data.user_id)) {
                 doDispatch(loadUser(msg.data.user_id));
             }
 
+            // The member list also requires the ChannelMembership relation, which the
+            // user_added event does not carry. Without it, the participant list filters
+            // the user out even when their profile is loaded (e.g. after they post), so
+            // members added while the channel is open (notably remote shared-channel
+            // members synced in from another server) never appear until the member list
+            // is fully reloaded. Fetch the membership if we don't already have it.
+            if (!getChannelMembersInChannels(state)[currentChannelId]?.[msg.data.user_id]) {
+                doDispatch(getChannelMember(currentChannelId, msg.data.user_id));
+            }
+
             if (license?.IsLicensed === 'true' && license?.LDAPGroups === 'true' && config.EnableConfirmNotificationsToChannel === 'true') {
                 doDispatch(getChannelMemberCountsByGroup(currentChannelId));
             }

webapp/channels/src/components/channel_members_rhs/channel_members_rhs.test.tsx

@@ -171,6 +171,61 @@ describe('channel_members_rhs/channel_members_rhs', () => {
         expect(loadProfilesAndReloadChannelMembers).toHaveBeenCalledWith(0, 100, 'channel_id', 'admin', {}, true);
     });
 
+    test('reloads the roster when the websocket reconnects with a new connection id', () => {
+        // Regression test for MM-67616: membership changes (e.g. a shared-channel
+        // sync) that happen while the client is disconnected are delivered as
+        // websocket events that are never replayed on a fresh, non-resumed
+        // connection. The roster must be reloaded when the connection id changes.
+        const loadProfilesAndReloadChannelMembers = jest.fn();
+        const props = {
+            ...baseProps,
+            actions: {
+                ...baseProps.actions,
+                loadProfilesAndReloadChannelMembers,
+            },
+        };
+
+        const {updateStoreState} = renderWithContext(
+            <ChannelMembersRHS
+                {...props as any}
+            />,
+            {websocket: {connectionId: 'connection1'}},
+        );
+
+        // Ignore the authoritative load that happens on mount.
+        loadProfilesAndReloadChannelMembers.mockClear();
+
+        updateStoreState({websocket: {connectionId: 'connection2'}});
+
+        expect(loadProfilesAndReloadChannelMembers).toHaveBeenCalledWith(0, 100, 'channel_id', 'admin', {}, true);
+    });
+
+    test('does not reload the roster when the connection id is unchanged', () => {
+        const loadProfilesAndReloadChannelMembers = jest.fn();
+        const props = {
+            ...baseProps,
+            actions: {
+                ...baseProps.actions,
+                loadProfilesAndReloadChannelMembers,
+            },
+        };
+
+        const {updateStoreState} = renderWithContext(
+            <ChannelMembersRHS
+                {...props as any}
+            />,
+            {websocket: {connectionId: 'connection1'}},
+        );
+
+        loadProfilesAndReloadChannelMembers.mockClear();
+
+        // An unrelated state change that keeps the same connection id must not
+        // trigger a reload.
+        updateStoreState({websocket: {connectionId: 'connection1'}});
+
+        expect(loadProfilesAndReloadChannelMembers).not.toHaveBeenCalled();
+    });
+
     test('should show search bar when there are more than 20 members', () => {
         const props = {
             ...baseProps,

webapp/channels/src/components/channel_members_rhs/channel_members_rhs.tsx

@@ -12,7 +12,7 @@ import type {UserProfile} from '@mattermost/types/users';
 
 import {ProfilesInChannelSortBy} from 'mattermost-redux/actions/users';
 
-import {areChannelAccessControlIndicatorsEnabled} from 'selectors/general';
+import {areChannelAccessControlIndicatorsEnabled, getConnectionId} from 'selectors/general';
 
 import AlertBanner from 'components/alert_banner';
 import ChannelInviteModal from 'components/channel_invite_modal';
@@ -100,6 +100,11 @@ export default function ChannelMembersRHS({
     // Admins can disable the attribute indicators to avoid leaking policy
     // details; when off we skip fetching/rendering the tags entirely.
     const indicatorsEnabled = useSelector(areChannelAccessControlIndicatorsEnabled);
+
+    // The websocket connection id changes whenever we get a fresh connection that
+    // could not be resumed (e.g. after a server restart), meaning membership events
+    // emitted while we were disconnected were not replayed. Reload the roster then.
+    const connectionId = useSelector(getConnectionId);
     const {structuredAttributes, loading} = useAccessControlAttributes(
         EntityType.Channel,
         channel.id,
@@ -212,7 +217,12 @@ export default function ChannelMembersRHS({
             actions.getChannelJoinRequests(channel.id, {status: 'pending'});
             actions.countPendingChannelJoinRequests(channel.id);
         }
-    }, [channel.id, channel.type, canManageJoinRequests]);
+
+    // connectionId is included so the roster is reloaded on a fresh websocket
+    // connection: adds/removes that happened while we were disconnected (e.g. a
+    // shared-channel membership sync during a server restart) are otherwise never
+    // reflected, since the missed websocket events are not replayed.
+    }, [channel.id, channel.type, canManageJoinRequests, connectionId]);
 
     const setSearchTerms = async (terms: string) => {
         actions.setChannelMembersRhsSearchTerm(terms);