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

jwilander · 2021-07-13T13:06:02Z

Summary

Currently, we were saving drafts for messages being typed into the post and comment textboxes on animation frames which can occur quite frequently. Saving drafts also seems to be less performant on some browser/OS/device combinations than on others. They also fire off a number of dispatches and change redux state which can negatively impact performance too. During some local testing, where I added a console log for each draft save, I was seeing it saved on every single key press which is 1) saving significantly more often than necessary and 2) could be negatively impacting performance, particularly in certain environments.

This is after typing about 100 characters into the post textbox:

In addition some very useful profiles from the community on Firefox are showing saveDraftFrame being called about every 100ms during typing and was taking up the majority of the JS processing time in the profile:

This PR moves from using animation frames to save the draft to a more controlled setTimeout which will only save the draft after 500ms of no typing has occurred (or the component unmounts).

Release Note

Improve typing performance in affected environments by reducing the frequency at which drafts are saved

Ticket link: https://mattermost.atlassian.net/browse/MM-37048

jwilander · 2021-07-13T13:10:01Z

components/create_post/create_post.jsx

@@ -46,6 +48,8 @@ import MessageSubmitError from 'components/message_submit_error';

 const KeyCodes = Constants.KeyCodes;

+const CreatePostDraftTimeoutMilliseconds = 500;


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.

@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.

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.

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.

jwilander · 2021-07-13T13:11:10Z

@jgilliam17 can you test to make sure that post and comment drafts are still saved correctly?

jwilander · 2021-07-13T15:27:45Z

Found a minor issue with the comment button staying disabled too long that might be related to this change. Going to investigate that

jwilander · 2021-07-13T20:03:03Z

Fixed the issues. @jgilliam17 can you smoke test commenting on posts/threads with this change too?

jwilander · 2021-07-13T20:10:51Z

Also @jgilliam17 (and anyone else) please leave the test server up since I'm going to ask some people to create accounts and see if improves performance for them

jgilliam17 · 2021-07-13T20:23:43Z

@jwilander Do we have a Jira ticket for this?

jwilander · 2021-07-13T20:24:23Z

No, I didn't create one since it was a bit reactionary. I can create one if you want, though.

jgilliam17 · 2021-07-13T20:25:26Z

I'll add one as a reminded to test again after merge. 🙂

devinbinnie · 2021-07-14T12:50:13Z

components/threading/thread_viewer/thread_viewer.tsx

@@ -1,6 +1,8 @@
 // Copyright (c) 2015-present Mattermost, Inc. All Rights Reserved.
 // See LICENSE.txt for license information.

+/* eslint-disable max-lines */


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

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.

hmhealey

This looks good to me. I was thinking we might use lodash's debounce utility since that avoids having to manage the timeout manually, but the more I think about it, doing that manually has me less concerned about potentially making mistakes like saving a draft to the wrong channel.

jgilliam17

Thanks @jwilander
Tested, looks good to merge.

Verified post and comment drafts are saved correctly, no issues commenting on threads and single posts.

mm-cloud-bot · 2021-07-15T14:55:33Z

Test server destroyed

mm-cloud-bot · 2021-07-15T14:55:34Z

Test server destroyed

…onds (#8396) * Do not save drafts until typing has stopped for X milliseconds * Fix comment box relying on draft stored in redux * Fix tests * Fix another test

* [MM-37048] Do not save drafts until typing has stopped for X milliseconds (#8396) * Do not save drafts until typing has stopped for X milliseconds * Fix comment box relying on draft stored in redux * Fix tests * Fix another test * MM-37048 Temporarily store drafts in localStorage and rehydrate manually to fix persistence issues (#8411) * Temporarily store drafts in localStorage and rehydrate manually to fix persistence issues * Fix console error on reload immediately after typing * Null out saveDraftFrame to prevent potential double saves * Fix draft removal on create_comment and unload in create_post (#8428)

* [MM-37048] Do not save drafts until typing has stopped for X milliseconds (#8396) * Do not save drafts until typing has stopped for X milliseconds * Fix comment box relying on draft stored in redux * Fix tests * Fix another test * MM-37048 Temporarily store drafts in localStorage and rehydrate manually to fix persistence issues (#8411) * Temporarily store drafts in localStorage and rehydrate manually to fix persistence issues * Fix console error on reload immediately after typing * Null out saveDraftFrame to prevent potential double saves * Fix draft removal on create_comment and unload in create_post (#8428) * Fix style * Fix test

Do not save drafts until typing has stopped for X milliseconds

b9a5d0a

jwilander added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Jul 13, 2021

mm-cloud-bot added the release-note label Jul 13, 2021

jwilander commented Jul 13, 2021

View reviewed changes

jwilander requested review from hmhealey, devinbinnie and jgilliam17 July 13, 2021 13:10

jwilander added the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Jul 13, 2021

jwilander added the Do Not Merge Should not be merged until this label is removed label Jul 13, 2021

jwilander removed request for jgilliam17, hmhealey and devinbinnie July 13, 2021 18:41

jwilander added Work in Progress Not yet ready for review and removed 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester labels Jul 13, 2021

jwilander added 2 commits July 13, 2021 15:32

Fix comment box relying on draft stored in redux

bf7e1bf

Fix tests

ca8b7e1

jwilander requested review from hmhealey, devinbinnie and jgilliam17 July 13, 2021 20:03

jwilander added 2: Dev Review Requires review by a core commiter 3: QA Review Requires review by a QA tester and removed Do Not Merge Should not be merged until this label is removed Work in Progress Not yet ready for review labels Jul 13, 2021

jwilander mentioned this pull request Jul 13, 2021

Slow text entry mattermost/mattermost#15012

Closed

Fix another test

790cc8e

jgilliam17 changed the title ~~Do not save drafts until typing has stopped for X milliseconds~~ [MM-37048] Do not save drafts until typing has stopped for X milliseconds Jul 13, 2021

devinbinnie reviewed Jul 14, 2021

View reviewed changes

devinbinnie approved these changes Jul 14, 2021

View reviewed changes

hmhealey approved these changes Jul 14, 2021

View reviewed changes

hmhealey removed the 2: Dev Review Requires review by a core commiter label Jul 14, 2021

jgilliam17 approved these changes Jul 15, 2021

View reviewed changes

jgilliam17 added 4: Reviews Complete All reviewers have approved the pull request and removed 3: QA Review Requires review by a QA tester labels Jul 15, 2021

jwilander merged commit 95597ba into master Jul 15, 2021

jwilander deleted the draft-saving branch July 15, 2021 14:55

mattermod removed the Setup Cloud Test Server Setup a test server using Mattermost Cloud label Jul 15, 2021

jwilander mentioned this pull request Jul 16, 2021

MM-37048 Temporarily store drafts in localStorage and rehydrate manually to fix persistence issues #8411

Merged

This was referenced Jul 23, 2021

Cherry pick slow typing draft fixes to release-5.31 #8460

Merged

Cherry pick slow typing draft fixes to release-5.37 #8461

Merged

amyblais added Changelog/Done Required changelog entry has been written Docs/Not Needed Does not require documentation labels Jul 30, 2021

amyblais added this to the v5.38.0 milestone Jul 30, 2021

WGH- mentioned this pull request Aug 18, 2021

Slow text entry in search fields mattermost/mattermost#18212

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

jwilander commented Jul 13, 2021 •

edited by jgilliam17

Loading

jwilander Jul 13, 2021

devinbinnie Jul 14, 2021

hmhealey Jul 14, 2021

jwilander Jul 15, 2021

jwilander commented Jul 13, 2021

jwilander commented Jul 13, 2021

jwilander commented Jul 13, 2021

jwilander commented Jul 13, 2021

jgilliam17 commented Jul 13, 2021

jwilander commented Jul 13, 2021

jgilliam17 commented Jul 13, 2021

devinbinnie Jul 14, 2021

hmhealey Jul 14, 2021

hmhealey left a comment

jgilliam17 left a comment

mm-cloud-bot commented Jul 15, 2021

mm-cloud-bot commented Jul 15, 2021

		@@ -46,6 +48,8 @@ import MessageSubmitError from 'components/message_submit_error';

		const KeyCodes = Constants.KeyCodes;

		const CreatePostDraftTimeoutMilliseconds = 500;

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

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

Conversation

jwilander commented Jul 13, 2021 • edited by jgilliam17 Loading

Summary

Release Note

jwilander Jul 13, 2021

Choose a reason for hiding this comment

devinbinnie Jul 14, 2021

Choose a reason for hiding this comment

hmhealey Jul 14, 2021

Choose a reason for hiding this comment

jwilander Jul 15, 2021

Choose a reason for hiding this comment

jwilander commented Jul 13, 2021

jwilander commented Jul 13, 2021

jwilander commented Jul 13, 2021

jwilander commented Jul 13, 2021

jgilliam17 commented Jul 13, 2021

jwilander commented Jul 13, 2021

jgilliam17 commented Jul 13, 2021

devinbinnie Jul 14, 2021

Choose a reason for hiding this comment

hmhealey Jul 14, 2021

Choose a reason for hiding this comment

hmhealey left a comment

Choose a reason for hiding this comment

jgilliam17 left a comment

Choose a reason for hiding this comment

mm-cloud-bot commented Jul 15, 2021

mm-cloud-bot commented Jul 15, 2021

jwilander commented Jul 13, 2021 •

edited by jgilliam17

Loading