Skip to content
This repository has been archived by the owner on Mar 13, 2024. It is now read-only.

[MM-37048] Do not save drafts until typing has stopped for X milliseconds #8396

Merged
merged 4 commits into from
Jul 15, 2021
Merged

[MM-37048] Do not save drafts until typing has stopped for X milliseconds #8396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
b9a5d0a
Do not save drafts until typing has stopped for X milliseconds
jwilander Jul 13, 2021
bf7e1bf
Fix comment box relying on draft stored in redux
jwilander Jul 13, 2021
ca8b7e1
Fix tests
jwilander Jul 13, 2021
790cc8e
Fix another test
jwilander Jul 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 1 addition & 2 deletions actions/views/create_comment.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,8 +146,7 @@ export function submitCommand(channelId, rootId, draft) {
}

export function makeOnSubmit(channelId, rootId, latestPostId) {
return (options = {}) => async (dispatch, getState) => {
const draft = getPostDraft(getState(), StoragePrefixes.COMMENT_DRAFT, rootId);
return (draft, options = {}) => async (dispatch, getState) => {
const {message} = draft;

dispatch(addMessageIntoHistory(message));
Expand Down
97 changes: 27 additions & 70 deletions actions/views/create_comment.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -332,9 +332,14 @@ describe('rhs view actions', () => {

describe('makeOnSubmit', () => {
const onSubmit = makeOnSubmit(channelId, rootId, latestPostId);
const draft = {
message: '',
fileInfos: [],
uploadsInProgress: [],
};

test('it adds message into history', () => {
store.dispatch(onSubmit());
store.dispatch(onSubmit(draft));

const testStore = mockStore(initialState);
testStore.dispatch(addMessageIntoHistory(''));
Expand All @@ -345,7 +350,7 @@ describe('rhs view actions', () => {
});

test('it clears comment draft', () => {
store.dispatch(onSubmit());
store.dispatch(onSubmit(draft));

const testStore = mockStore(initialState);
testStore.dispatch(updateCommentDraft(rootId, null));
Expand All @@ -356,23 +361,11 @@ describe('rhs view actions', () => {
});

test('it submits a reaction when message is +:smile:', () => {
store = mockStore({
...initialState,
storage: {
storage: {
[`${StoragePrefixes.COMMENT_DRAFT}${rootId}`]: {
value: {
message: '+:smile:',
fileInfos: [],
uploadsInProgress: [],
},
timestamp: new Date(),
},
},
},
});

store.dispatch(onSubmit());
store.dispatch(onSubmit({
message: '+:smile:',
fileInfos: [],
uploadsInProgress: [],
}));

const testStore = mockStore(initialState);
testStore.dispatch(submitReaction(latestPostId, '+', 'smile'));
Expand All @@ -383,23 +376,11 @@ describe('rhs view actions', () => {
});

test('it submits a command when message is /away', () => {
store = mockStore({
...initialState,
storage: {
storage: {
[`${StoragePrefixes.COMMENT_DRAFT}${latestPostId}`]: {
value: {
message: '/away',
fileInfos: [],
uploadsInProgress: [],
},
timestamp: new Date(),
},
},
},
});

store.dispatch(onSubmit());
store.dispatch(onSubmit({
message: '/away',
fileInfos: [],
uploadsInProgress: [],
}));

const testStore = mockStore(initialState);
testStore.dispatch(submitCommand(channelId, rootId, {message: '/away', fileInfos: [], uploadsInProgress: []}));
Expand All @@ -412,23 +393,11 @@ describe('rhs view actions', () => {
});

test('it submits a regular post when options.ignoreSlash is true', () => {
store = mockStore({
...initialState,
storage: {
storage: {
[`${StoragePrefixes.COMMENT_DRAFT}${latestPostId}`]: {
value: {
message: '/fakecommand',
fileInfos: [],
uploadsInProgress: [],
},
timestamp: new Date(),
},
},
},
});

store.dispatch(onSubmit({ignoreSlash: true}));
store.dispatch(onSubmit({
message: '/fakecommand',
fileInfos: [],
uploadsInProgress: [],
}, {ignoreSlash: true}));

const testStore = mockStore(initialState);
testStore.dispatch(submitPost(channelId, rootId, {message: '/fakecommand', fileInfos: [], uploadsInProgress: []}));
Expand All @@ -440,23 +409,11 @@ describe('rhs view actions', () => {
});

test('it submits a regular post when message is something else', () => {
store = mockStore({
...initialState,
storage: {
storage: {
[`${StoragePrefixes.COMMENT_DRAFT}${latestPostId}`]: {
value: {
message: 'test msg',
fileInfos: [],
uploadsInProgress: [],
},
timestamp: new Date(),
},
},
},
});

store.dispatch(onSubmit());
store.dispatch(onSubmit({
message: 'test msg',
fileInfos: [],
uploadsInProgress: [],
}));

const testStore = mockStore(initialState);
testStore.dispatch(submitPost(channelId, rootId, {message: 'test msg', fileInfos: [], uploadsInProgress: []}));
Expand Down
30 changes: 18 additions & 12 deletions components/create_comment/create_comment.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
// See LICENSE.txt for license information.

/* eslint-disable max-lines */

import PropTypes from 'prop-types';
import React from 'react';
import classNames from 'classnames';
Expand Down Expand Up @@ -38,6 +40,8 @@ import MessageSubmitError from 'components/message_submit_error';

const KeyCodes = Constants.KeyCodes;

const CreateCommentDraftTimeoutMilliseconds = 500;

class CreateComment extends React.PureComponent {
static propTypes = {

Expand Down Expand Up @@ -75,11 +79,6 @@ class CreateComment extends React.PureComponent {
fileInfos: PropTypes.array.isRequired,
}).isRequired,

/**
* Whether the submit button is enabled
*/
enableAddButton: PropTypes.bool.isRequired,

/**
* Force message submission on CTRL/CMD + ENTER
*/
Expand Down Expand Up @@ -306,7 +305,7 @@ class CreateComment extends React.PureComponent {
document.removeEventListener('keydown', this.focusTextboxIfNecessary);

if (this.saveDraftFrame) {
cancelAnimationFrame(this.saveDraftFrame);
clearTimeout(this.saveDraftFrame);

this.props.onUpdateCommentDraft(this.state.draft);
}
Expand Down Expand Up @@ -629,7 +628,7 @@ class CreateComment extends React.PureComponent {
const options = {ignoreSlash};

try {
await this.props.onSubmit(options);
await this.props.onSubmit(draft, options);

this.setState({
postError: null,
Expand All @@ -644,7 +643,9 @@ class CreateComment extends React.PureComponent {
return;
}

clearTimeout(this.saveDraftFrame);
this.setState({draft: {...this.props.draft, uploadsInProgress: []}});
this.draftsForPost[this.props.rootId] = null;
}

commentMsgKeyPress = (e) => {
Expand Down Expand Up @@ -708,10 +709,10 @@ class CreateComment extends React.PureComponent {
const {draft} = this.state;
const updatedDraft = {...draft, message};

cancelAnimationFrame(this.saveDraftFrame);
this.saveDraftFrame = requestAnimationFrame(() => {
clearTimeout(this.saveDraftFrame);
this.saveDraftFrame = setTimeout(() => {
this.props.onUpdateCommentDraft(updatedDraft);
});
}, CreateCommentDraftTimeoutMilliseconds);

this.setState({draft: updatedDraft, serverError}, () => {
if (this.props.scrollToBottom) {
Expand Down Expand Up @@ -964,8 +965,13 @@ class CreateComment extends React.PureComponent {
}

shouldEnableAddButton = () => {
if (this.props.enableAddButton) {
return true;
const {draft} = this.state;
if (draft) {
const message = draft.message ? draft.message.trim() : '';
const fileInfos = draft.fileInfos ? draft.fileInfos : [];
if (message.trim().length !== 0 || fileInfos.length !== 0) {
return true;
}
}

return isErrorInvalidSlashCommand(this.state.serverError);
Expand Down
24 changes: 20 additions & 4 deletions components/create_comment/create_comment.test.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -510,7 +510,11 @@ describe('components/CreateComment', () => {

await wrapper.instance().handleSubmit({preventDefault: jest.fn()});

expect(onSubmit).toHaveBeenCalledWith({ignoreSlash: false});
expect(onSubmit).toHaveBeenCalledWith({
message: '/fakecommand other text',
uploadsInProgress: [],
fileInfos: [{}, {}, {}],
}, {ignoreSlash: false});
expect(wrapper.find('[id="postServerError"]').exists()).toBe(true);

wrapper.instance().handleChange({
Expand All @@ -521,7 +525,11 @@ describe('components/CreateComment', () => {

wrapper.instance().handleSubmit({preventDefault: jest.fn()});

expect(onSubmit).toHaveBeenCalledWith({ignoreSlash: false});
expect(onSubmit).toHaveBeenCalledWith({
message: 'some valid text',
uploadsInProgress: [],
fileInfos: [{}, {}, {}],
}, {ignoreSlash: false});
});

test('should scroll to bottom when uploadsInProgress increase', () => {
Expand Down Expand Up @@ -925,14 +933,22 @@ describe('components/CreateComment', () => {

await wrapper.instance().handleSubmit({preventDefault});

expect(onSubmitWithError).toHaveBeenCalledWith({ignoreSlash: false});
expect(onSubmitWithError).toHaveBeenCalledWith({
message: '/fakecommand other text',
uploadsInProgress: [],
fileInfos: [{}, {}, {}],
}, {ignoreSlash: false});
expect(preventDefault).toHaveBeenCalled();
expect(wrapper.find('[id="postServerError"]').exists()).toBe(true);

wrapper.setProps({onSubmit});
await wrapper.instance().handleSubmit({preventDefault});

expect(onSubmit).toHaveBeenCalledWith({ignoreSlash: true});
expect(onSubmit).toHaveBeenCalledWith({
message: '/fakecommand other text',
uploadsInProgress: [],
fileInfos: [{}, {}, {}],
}, {ignoreSlash: true});
expect(wrapper.find('[id="postServerError"]').exists()).toBe(false);
});

Expand Down
2 changes: 0 additions & 2 deletions components/create_comment/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ function makeMapStateToProps() {
const err = state.requests.posts.createPost.error || {};

const draft = getPostDraft(state, StoragePrefixes.COMMENT_DRAFT, ownProps.rootId);
const enableAddButton = draft.message.trim().length !== 0 || draft.fileInfos.length !== 0;

const channelMembersCount = getAllChannelStats(state)[ownProps.channelId] ? getAllChannelStats(state)[ownProps.channelId].member_count : 1;
const messageInHistory = getMessageInHistoryItem(state);
Expand All @@ -65,7 +64,6 @@ function makeMapStateToProps() {
return {
draft,
messageInHistory,
enableAddButton,
channelMembersCount,
codeBlockOnCtrlEnter: getBool(state, Preferences.CATEGORY_ADVANCED_SETTINGS, 'code_block_ctrl_enter', true),
ctrlSend: getBool(state, Preferences.CATEGORY_ADVANCED_SETTINGS, 'send_on_ctrl_enter'),
Expand Down
14 changes: 9 additions & 5 deletions components/create_post/create_post.jsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
// See LICENSE.txt for license information.

/* eslint-disable max-lines */

import PropTypes from 'prop-types';
import React from 'react';
import classNames from 'classnames';
Expand Down Expand Up @@ -46,6 +48,8 @@ import MessageSubmitError from 'components/message_submit_error';

const KeyCodes = Constants.KeyCodes;

const CreatePostDraftTimeoutMilliseconds = 500;
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I initially started lower at 250ms but during my testing it would still fire when I was typing at my normal speed. At 500ms it would consistently only fire after I had a significant pause in my typing.

@hmhealey I know you wanted to lean on the lower end but for this to have the performance impact I'm hoping it will I think it's important that saving doesn't occur during typing.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jwilander Non-blocking, but is there any benefit to having this constant declared here and the same one declared in create_comment. Is there any point where we'd want those intervals to be different? If not it might be worth be putting the constant in a constants file.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was originally thinking we could lean on the side of having it fire periodically as the user was typing, but I guess if saving the draft is taking as long as we've seen, you're probably right that it's better to be on the safe side with this. If we hear people are losing drafts, we can always back off on the interval.

As for the duplicated constants, I don't think it's important enough to move to the constants file. That thing is bloated with a lot of things that are only used in one or two places if they're even still used at all. I think having two values is fine since I can't see us using this anywhere else, and ideally, both the CreatePost and CreateComment would be rewritten to share this logic entirely anyway.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the duplicated constants, I don't think it's important enough to move to the constants file. That thing is bloated with a lot of things that are only used in one or two places if they're even still used at all. I think having two values is fine since I can't see us using this anywhere else, and ideally, both the CreatePost and CreateComment would be rewritten to share this logic entirely anyway.

This was pretty much my exact reasoning.


// Temporary fix for IE-11, see MM-13423
function trimRight(str) {
if (String.prototype.trimRight) {
Expand Down Expand Up @@ -375,7 +379,7 @@ class CreatePost extends React.PureComponent {
if (this.saveDraftFrame) {
const channelId = this.props.currentChannel.id;
this.props.actions.setDraft(StoragePrefixes.DRAFT + channelId, this.draftsForChannel[channelId]);
cancelAnimationFrame(this.saveDraftFrame);
clearTimeout(this.saveDraftFrame);
}
}

Expand Down Expand Up @@ -532,7 +536,7 @@ class CreatePost extends React.PureComponent {
postError: null,
});

cancelAnimationFrame(this.saveDraftFrame);
clearTimeout(this.saveDraftFrame);
this.props.actions.setDraft(StoragePrefixes.DRAFT + channelId, null);
this.draftsForChannel[channelId] = null;
}
Expand Down Expand Up @@ -802,10 +806,10 @@ class CreatePost extends React.PureComponent {
...this.props.draft,
message,
};
cancelAnimationFrame(this.saveDraftFrame);
this.saveDraftFrame = requestAnimationFrame(() => {
clearTimeout(this.saveDraftFrame);
this.saveDraftFrame = setTimeout(() => {
this.props.actions.setDraft(StoragePrefixes.DRAFT + channelId, draft);
});
}, CreatePostDraftTimeoutMilliseconds);
this.draftsForChannel[channelId] = draft;
}

Expand Down
2 changes: 2 additions & 0 deletions components/threading/thread_viewer/thread_viewer.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@
// Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
// See LICENSE.txt for license information.

/* eslint-disable max-lines */
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unrelated, but we should really get rid of this...it's more annoying than it helps.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe we could increase the limit or disable it for older files, but I'd like to keep it for newer files since it's nice to discourage people from adding new files with too much stuff going on in them. Only 1/5 on that though.


import React, {HTMLAttributes} from 'react';
import Scrollbars from 'react-custom-scrollbars';
import classNames from 'classnames';
Expand Down
1 change: 1 addition & 0 deletions package-lock.json
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.