[FIX] Rooms list not being updated on some cases (#2765)

* Request subscriptions on RoomsListView.constructor * Removes opened rooms from last message persisting * Change server reducer * Prevent undefined ids causing query error
RocketChat · Jan 13, 2021 · 7f0abe1 · 7f0abe1
1 parent 3b4f457
commit 7f0abe1
Show file tree

Hide file tree

Showing 5 changed files with 45 additions and 36 deletions.
diff --git a/app/actions/server.js b/app/actions/server.js
@@ -1,11 +1,12 @@
 import { SERVER } from './actionsTypes';
 
-export function selectServerRequest(server, version, fetchVersion = true) {
+export function selectServerRequest(server, version, fetchVersion = true, changeServer = false) {
 	return {
 		type: SERVER.SELECT_REQUEST,
 		server,
 		version,
-		fetchVersion
+		fetchVersion,
+		changeServer
 	};
 }
 

diff --git a/app/reducers/server.js b/app/reducers/server.js
@@ -8,7 +8,8 @@ const initialState = {
 	version: null,
 	loading: true,
 	adding: false,
-	previousServer: null
+	previousServer: null,
+	changingServer: false
 };
 
 
@@ -34,7 +35,8 @@ export default function server(state = initialState, action) {
 				version: action.version,
 				connecting: true,
 				connected: false,
-				loading: true
+				loading: true,
+				changingServer: action.changeServer
 			};
 		case SERVER.SELECT_SUCCESS:
 			return {
@@ -43,14 +45,16 @@ export default function server(state = initialState, action) {
 				version: action.version,
 				connecting: false,
 				connected: true,
-				loading: false
+				loading: false,
+				changingServer: false
 			};
 		case SERVER.SELECT_FAILURE:
 			return {
 				...state,
 				connecting: false,
 				connected: false,
-				loading: false
+				loading: false,
+				changingServer: false
 			};
 		case SERVER.INIT_ADD:
 			return {

diff --git a/app/sagas/rooms.js b/app/sagas/rooms.js
@@ -61,10 +61,16 @@ const handleRoomsRequest = function* handleRoomsRequest({ params }) {
 			const subsToCreate = subscriptions.filter(i1 => !existingSubs.find(i2 => i1._id === i2._id));
 			const subsToDelete = existingSubs.filter(i1 => !subscriptions.find(i2 => i1._id === i2._id));
 
+			const openedRooms = yield select(state => state.room.rooms);
 			const lastMessages = subscriptions
+				/** Checks for opened rooms and filter them out.
+				 * It prevents this process to try persisting the same last message on the room messages fetch.
+				 * This race condition is easy to reproduce on push notification tap.
+				 */
+				.filter(sub => !openedRooms.includes(sub.rid))
 				.map(sub => sub.lastMessage && buildMessage(sub.lastMessage))
 				.filter(lm => lm);
-			const lastMessagesIds = lastMessages.map(lm => lm._id);
+			const lastMessagesIds = lastMessages.map(lm => lm._id).filter(lm => lm);
 			const existingMessages = yield messagesCollection.query(Q.where('id', Q.oneOf(lastMessagesIds))).fetch();
 			const messagesToUpdate = existingMessages.filter(i1 => lastMessages.find(i2 => i1.id === i2._id));
 			const messagesToCreate = lastMessages.filter(i1 => !existingMessages.find(i2 => i1._id === i2.id));

diff --git a/app/views/RoomsListView/ServerDropdown.js b/app/views/RoomsListView/ServerDropdown.js
@@ -313,7 +313,7 @@ const mapStateToProps = state => ({
 
 const mapDispatchToProps = dispatch => ({
 	toggleServerDropdown: () => dispatch(toggleServerDropdownAction()),
-	selectServerRequest: server => dispatch(selectServerRequestAction(server)),
+	selectServerRequest: server => dispatch(selectServerRequestAction(server, null, null, true)),
 	appStart: params => dispatch(appStartAction(params)),
 	initAdd: previousServer => dispatch(serverInitAddAction(previousServer))
 });

diff --git a/app/views/RoomsListView/index.js b/app/views/RoomsListView/index.js
@@ -117,6 +117,7 @@ class RoomsListView extends React.Component {
 		}),
 		server: PropTypes.string,
 		searchText: PropTypes.string,
+		changingServer: PropTypes.bool,
 		loadingServer: PropTypes.bool,
 		showServerDropdown: PropTypes.bool,
 		showSortDropdown: PropTypes.bool,
@@ -150,8 +151,8 @@ class RoomsListView extends React.Component {
 		console.time(`${ this.constructor.name } init`);
 		console.time(`${ this.constructor.name } mount`);
 
-		this.gotSubscriptions = false;
 		this.animated = false;
+		this.mounted = false;
 		this.count = 0;
 		this.state = {
 			searching: false,
@@ -162,24 +163,14 @@ class RoomsListView extends React.Component {
 			item: {}
 		};
 		this.setHeader();
+		this.getSubscriptions();
 	}
 
 	componentDidMount() {
 		const {
-			navigation, closeServerDropdown, appState
+			navigation, closeServerDropdown
 		} = this.props;
-
-		/**
-		 * - When didMount is triggered and appState is foreground,
-		 * it means the user is logging in and selectServer has ran, so we can getSubscriptions
-		 *
-		 * - When didMount is triggered and appState is background,
-		 * it means the user has resumed the app, so selectServer needs to be triggered,
-		 * which is going to change server and getSubscriptions will be triggered by componentWillReceiveProps
-		 */
-		if (appState === 'foreground') {
-			this.getSubscriptions();
-		}
+		this.mounted = true;
 
 		if (isTablet) {
 			EventEmitter.addEventListener(KEY_COMMAND, this.handleCommands);
@@ -206,17 +197,17 @@ class RoomsListView extends React.Component {
 	}
 
 	UNSAFE_componentWillReceiveProps(nextProps) {
-		const { loadingServer, searchText, server } = this.props;
+		const {
+			loadingServer, searchText, server, changingServer
+		} = this.props;
 
-		if (nextProps.server && loadingServer !== nextProps.loadingServer) {
-			if (nextProps.loadingServer) {
-				this.setState({ loading: true });
-			} else {
-				this.getSubscriptions();
-			}
+		// when the server is changed
+		if (server !== nextProps.server && loadingServer !== nextProps.loadingServer && nextProps.loadingServer) {
+			this.setState({ loading: true });
 		}
-		if (server && server !== nextProps.server) {
-			this.gotSubscriptions = false;
+		// when the server is changing and stopped loading
+		if (changingServer && loadingServer !== nextProps.loadingServer && !nextProps.loadingServer) {
+			this.getSubscriptions();
 		}
 		if (searchText !== nextProps.searchText) {
 			this.search(nextProps.searchText);
@@ -508,11 +499,17 @@ class RoomsListView extends React.Component {
 				tempChats = chats;
 			}
 
-			this.internalSetState({
-				chats: tempChats,
-				chatsUpdate,
-				loading: false
-			});
+			if (this.mounted) {
+				this.internalSetState({
+					chats: tempChats,
+					chatsUpdate,
+					loading: false
+				});
+			} else {
+				this.state.chats = tempChats;
+				this.state.chatsUpdate = chatsUpdate;
+				this.state.loading = false;
+			}
 		});
 	}
 
@@ -1023,6 +1020,7 @@ const mapStateToProps = state => ({
 	user: getUserSelector(state),
 	isMasterDetail: state.app.isMasterDetail,
 	server: state.server.server,
+	changingServer: state.server.changingServer,
 	connected: state.server.connected,
 	searchText: state.rooms.searchText,
 	loadingServer: state.server.loading,