From c4f36201b3643cb7c2cea1a1c189366e13ca1373 Mon Sep 17 00:00:00 2001 From: Saturnino Abril Date: Tue, 19 Jun 2018 03:46:15 +0800 Subject: [PATCH] Fix memory leak on combined_system_message component (#1775) * fix memory leak on combined_system_message component * updated per comment * move makeGetProfilesByIdsAndUsernames to mattermost-redux * updated mattermost-redux --- .../combined_system_message.js | 37 ++++-------- .../combined_system_message.test.js | 56 +++++++++++++------ .../combined_system_message/index.js | 30 ++++++---- package-lock.json | 4 +- package.json | 2 +- 5 files changed, 72 insertions(+), 57 deletions(-) diff --git a/app/components/combined_system_message/combined_system_message.js b/app/components/combined_system_message/combined_system_message.js index d904ae19e..7bb790121 100644 --- a/app/components/combined_system_message/combined_system_message.js +++ b/app/components/combined_system_message/combined_system_message.js @@ -156,8 +156,8 @@ const postTypeMessage = { export default class CombinedSystemMessage extends React.PureComponent { static propTypes = { actions: PropTypes.shape({ - getProfilesByIds: PropTypes.func.isRequired, - getProfilesByUsernames: PropTypes.func.isRequired, + getMissingProfilesByIds: PropTypes.func.isRequired, + getMissingProfilesByUsernames: PropTypes.func.isRequired, }).isRequired, allUserIds: PropTypes.array.isRequired, allUsernames: PropTypes.array.isRequired, @@ -168,6 +168,7 @@ export default class CombinedSystemMessage extends React.PureComponent { showJoinLeave: PropTypes.bool.isRequired, teammateNameDisplay: PropTypes.string.isRequired, theme: PropTypes.object.isRequired, + userProfiles: PropTypes.array.isRequired, }; static defaultProps = { @@ -179,14 +180,6 @@ export default class CombinedSystemMessage extends React.PureComponent { intl: intlShape, }; - constructor(props) { - super(props); - - this.state = { - userProfiles: [], - }; - } - componentDidMount() { this.loadUserProfiles(this.props.allUserIds, this.props.allUsernames); } @@ -197,34 +190,24 @@ export default class CombinedSystemMessage extends React.PureComponent { } } - loadUserProfiles = async (allUserIds, allUsernames) => { - const {actions} = this.props; - const userProfiles = []; + loadUserProfiles = (allUserIds, allUsernames) => { if (allUserIds.length > 0) { - const {data} = await actions.getProfilesByIds(allUserIds); - if (data.length > 0) { - userProfiles.push(...data); - } + this.props.actions.getMissingProfilesByIds(allUserIds); } if (allUsernames.length > 0) { - const {data} = await actions.getProfilesByUsernames(allUsernames); - if (data.length > 0) { - userProfiles.push(...data); - } + this.props.actions.getMissingProfilesByUsernames(allUsernames); } - - this.setState({userProfiles}); } getAllUsersDisplayName = () => { - const {userProfiles} = this.state; const { allUserIds, allUsernames, currentUserId, currentUsername, teammateNameDisplay, + userProfiles, } = this.props; const {formatMessage} = this.context.intl; const usersDisplayName = userProfiles.reduce((acc, user) => { @@ -248,7 +231,11 @@ export default class CombinedSystemMessage extends React.PureComponent { const usersDisplayName = this.getAllUsersDisplayName(); const displayNames = userIds. filter((userId) => { - return userId !== currentUserId && userId !== currentUsername; + return ( + usersDisplayName[userId] && + userId !== currentUserId && + userId !== currentUsername + ); }). map((userId) => { return usersDisplayName[userId]; diff --git a/app/components/combined_system_message/combined_system_message.test.js b/app/components/combined_system_message/combined_system_message.test.js index fe043dbd8..3823bd81e 100644 --- a/app/components/combined_system_message/combined_system_message.test.js +++ b/app/components/combined_system_message/combined_system_message.test.js @@ -13,13 +13,13 @@ import {Posts} from 'mattermost-redux/constants'; import CombinedSystemMessage from './combined_system_message'; -/* eslint-disable max-nested-callbacks, no-console */ +/* eslint-disable max-nested-callbacks */ describe('CombinedSystemMessage', () => { const baseProps = { actions: { - getProfilesByIds: emptyFunction, - getProfilesByUsernames: emptyFunction, + getMissingProfilesByIds: emptyFunction, + getMissingProfilesByUsernames: emptyFunction, }, allUserIds: ['user_id_1', 'user_id_2', 'user_id_3'], currentUserId: 'user_id_3', @@ -31,46 +31,68 @@ describe('CombinedSystemMessage', () => { showJoinLeave: true, teammateNameDisplay: 'username', theme: {centerChannelColor: '#aaa'}, + userProfiles: [{id: 'user_id_1', username: 'user1'}, {id: 'user_id_2', username: 'user2'}, {id: 'user_id_3', username: 'user3'}], }; test('should match snapshot', () => { const props = { ...baseProps, actions: { - getProfilesByIds: jest.fn(() => Promise.resolve({data: true})), - getProfilesByUsernames: emptyFunction, + getMissingProfilesByIds: jest.fn(), + getMissingProfilesByUsernames: emptyFunction, }, }; const wrapper = shallowWithIntl( ); - wrapper.setState({userProfiles: [{id: 'user_id_1', username: 'user1'}, {id: 'user_id_2', username: 'user2'}, {id: 'user_id_3', username: 'user3'}]}); const {postType, userIds, actorId} = baseProps.messageData[0]; expect(wrapper.instance().renderSystemMessage(postType, userIds, actorId, {activityType: {fontSize: 14}, text: {opacity: 0.6}}, 1)).toMatchSnapshot(); // on componentDidMount - expect(props.actions.getProfilesByIds).toHaveBeenCalledTimes(1); - expect(props.actions.getProfilesByIds).toHaveBeenCalledWith(props.allUserIds); + expect(props.actions.getMissingProfilesByIds).toHaveBeenCalledTimes(1); + expect(props.actions.getMissingProfilesByIds).toHaveBeenCalledWith(props.allUserIds); }); test('should match snapshot', () => { - const props = { - ...baseProps, - actions: { - getProfilesByIds: jest.fn(() => Promise.resolve({data: true})), - getProfilesByUsernames: emptyFunction, - }, - }; const localeFormat = { id: ['combined_system_message.first_user_and_second_user_were', 'combined_system_message.removed_from_team'], defaultMessage: ['{firstUser} and {secondUser} were ', 'removed from the team'], }; + const wrapper = shallowWithIntl( + + ); + + expect(wrapper.instance().renderFormattedMessage(localeFormat, 'first_user', 'second_user', 'actor', {activityType: {fontSize: 14}, text: {opacity: 0.6}})).toMatchSnapshot(); + }); + + test('should call getMissingProfilesByIds and/or getMissingProfilesByUsernames on loadUserProfiles', () => { + const props = { + ...baseProps, + allUserIds: [], + actions: { + getMissingProfilesByIds: jest.fn(), + getMissingProfilesByUsernames: jest.fn(), + }, + }; + const wrapper = shallowWithIntl( ); - wrapper.setState({userProfiles: [{id: 'user_id_1', username: 'user1'}, {id: 'user_id_2', username: 'user2'}, {id: 'user_id_3', username: 'user3'}]}); - expect(wrapper.instance().renderFormattedMessage(localeFormat, 'first_user', 'second_user', 'actor', {activityType: {fontSize: 14}, text: {opacity: 0.6}})).toMatchSnapshot(); + wrapper.instance().loadUserProfiles([], []); + expect(props.actions.getMissingProfilesByIds).toHaveBeenCalledTimes(0); + expect(props.actions.getMissingProfilesByUsernames).toHaveBeenCalledTimes(0); + + wrapper.instance().loadUserProfiles(['user_id_1'], []); + expect(props.actions.getMissingProfilesByIds).toHaveBeenCalledTimes(1); + expect(props.actions.getMissingProfilesByIds).toHaveBeenCalledWith(['user_id_1']); + expect(props.actions.getMissingProfilesByUsernames).toHaveBeenCalledTimes(0); + + wrapper.instance().loadUserProfiles(['user_id_1', 'user_id_2'], ['user1']); + expect(props.actions.getMissingProfilesByIds).toHaveBeenCalledTimes(2); + expect(props.actions.getMissingProfilesByIds).toHaveBeenCalledWith(['user_id_1', 'user_id_2']); + expect(props.actions.getMissingProfilesByUsernames).toHaveBeenCalledTimes(1); + expect(props.actions.getMissingProfilesByUsernames).toHaveBeenCalledWith(['user1']); }); }); diff --git a/app/components/combined_system_message/index.js b/app/components/combined_system_message/index.js index 1c225c266..43e8022dd 100644 --- a/app/components/combined_system_message/index.js +++ b/app/components/combined_system_message/index.js @@ -4,30 +4,36 @@ import {connect} from 'react-redux'; import {bindActionCreators} from 'redux'; -import {getProfilesByIds, getProfilesByUsernames} from 'mattermost-redux/actions/users'; +import {getMissingProfilesByIds, getMissingProfilesByUsernames} from 'mattermost-redux/actions/users'; import {Preferences} from 'mattermost-redux/constants'; import {getBool, getTeammateNameDisplaySetting} from 'mattermost-redux/selectors/entities/preferences'; -import {getCurrentUser} from 'mattermost-redux/selectors/entities/users'; +import {getCurrentUser, makeGetProfilesByIdsAndUsernames} from 'mattermost-redux/selectors/entities/users'; import CombinedSystemMessage from './combined_system_message'; -function mapStateToProps(state) { - const currentUser = getCurrentUser(state); - return { - currentUserId: currentUser.id, - currentUsername: currentUser.username, - showJoinLeave: getBool(state, Preferences.CATEGORY_ADVANCED_SETTINGS, Preferences.ADVANCED_FILTER_JOIN_LEAVE, true), - teammateNameDisplay: getTeammateNameDisplaySetting(state), +function makeMapStateToProps() { + const getProfilesByIdsAndUsernames = makeGetProfilesByIdsAndUsernames(); + + return (state, ownProps) => { + const currentUser = getCurrentUser(state); + const {allUserIds, allUsernames} = ownProps; + return { + currentUserId: currentUser.id, + currentUsername: currentUser.username, + showJoinLeave: getBool(state, Preferences.CATEGORY_ADVANCED_SETTINGS, Preferences.ADVANCED_FILTER_JOIN_LEAVE, true), + teammateNameDisplay: getTeammateNameDisplaySetting(state), + userProfiles: getProfilesByIdsAndUsernames(state, {allUserIds, allUsernames}), + }; }; } function mapDispatchToProps(dispatch) { return { actions: bindActionCreators({ - getProfilesByIds, - getProfilesByUsernames, + getMissingProfilesByIds, + getMissingProfilesByUsernames, }, dispatch), }; } -export default connect(mapStateToProps, mapDispatchToProps)(CombinedSystemMessage); +export default connect(makeMapStateToProps, mapDispatchToProps)(CombinedSystemMessage); diff --git a/package-lock.json b/package-lock.json index 3abc5f6f7..e6e7aaf7b 100644 --- a/package-lock.json +++ b/package-lock.json @@ -9878,8 +9878,8 @@ } }, "mattermost-redux": { - "version": "github:mattermost/mattermost-redux#85edf0d1cae3b786d9be1946f19a23603af18417", - "from": "github:mattermost/mattermost-redux#85edf0d1cae3b786d9be1946f19a23603af18417", + "version": "github:mattermost/mattermost-redux#c9b633da7fc3fc9ba3cc40ecc9665e088defbe73", + "from": "github:mattermost/mattermost-redux#c9b633da7fc3fc9ba3cc40ecc9665e088defbe73", "requires": { "deep-equal": "1.0.1", "eslint-plugin-header": "1.2.0", diff --git a/package.json b/package.json index 835fb3bac..9684edde4 100644 --- a/package.json +++ b/package.json @@ -15,7 +15,7 @@ "intl": "1.2.5", "jail-monkey": "1.0.0", "jsc-android": "216113.0.3", - "mattermost-redux": "github:mattermost/mattermost-redux#85edf0d1cae3b786d9be1946f19a23603af18417", + "mattermost-redux": "github:mattermost/mattermost-redux#c9b633da7fc3fc9ba3cc40ecc9665e088defbe73", "mime-db": "1.33.0", "prop-types": "15.6.1", "react": "16.3.2",