toki/mattermost-mobile
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions

Fix memory leak on combined_system_message component (#1775)

Browse source 
* fix memory leak on combined_system_message component

* updated per comment

* move makeGetProfilesByIdsAndUsernames to mattermost-redux

* updated mattermost-redux
This commit is contained in:
Saturnino Abril 2018-06-19 03:46:15 +08:00 committed by Elias Nahum
parent c6b0beb972
commit c4f36201b3
5 changed files with 72 additions and 57 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

37
app/components/combined_system_message/combined_system_message.js
View file

@ -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];

56
app/components/combined_system_message/combined_system_message.test.js
View file

@ -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(
<CombinedSystemMessage {...props}/>
);
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(
<CombinedSystemMessage {...baseProps}/>
);
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(
<CombinedSystemMessage {...props}/>
);
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']);
});
});

30
app/components/combined_system_message/index.js
View file

@ -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);

4
package-lock.json generated
View file

@ -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",

2
package.json
View file

@ -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",