Save draft on chat change #4144

maxphilippov · 2024-09-23T17:18:55Z

resolves Message draft discarded when switching to different chat quickly #3733

maxphilippov · 2024-09-23T17:24:37Z

OUTDATED: This is a thing we discussed a long time ago, we had a 1 second debounce for saving draft, so if you're in any way fast at switching between different chats you just lose what you were typing right before switching. We were talking about testing something around 200-300ms, I went for 100.

maxphilippov · 2024-10-15T21:52:03Z

I did keep the original 1 second delay between autosave

WofWca

So do you suggest to save the draft on every key press? Because this is what's happening in this MR, and it doesn't seem intentional.

packages/frontend/src/components/composer/ComposerMessageInput.tsx

CHANGELOG.md

packages/frontend/src/components/composer/ComposerMessageInput.tsx

maxphilippov · 2024-10-16T10:39:38Z

So do you suggest to save the draft on every key press? Because this is what's happening in this MR, and it doesn't seem intentional.

Oops, my bad, rushed that a bit. Didn't notice we have a setState above. I'll draft it for now. Reverted back to 200ms, removed saveDraft from onChange then. Gonna look for a way to maybe flush via ref to messageInputComposer. I don't agree with throttle, since I don't see the point in saving first character immediately and thus pushing next save further in time. Next save is what's more likely to happen on chat change.

WofWca · 2024-10-16T10:57:22Z

thus pushing next save further in time

I don't think that's how throttle works. Throttle is never less frequent than debounce.

maxphilippov · 2024-10-16T15:00:47Z

thus pushing next save further in time

I don't think that's how throttle works. Throttle is never less frequent than debounce.

Oh, my bad again, for some reason I got memorized that debounce is just throttle but without the inital call (so it calls at the end of time interval while throttle in the beginning). Yeah, makes sense then.

maxphilippov · 2024-11-19T18:13:05Z

As discussed with @nicodh, I've added a simple throttle implementation and made saveDraft use this one instead of debounce.

WofWca · 2024-11-20T09:49:12Z

packages/shared/util.ts

+      clearTimeout(timeout)
+      timeout = setTimeout(
+        () => {
+          if (performance.now() - lastTime >= wait) {


So if this condition is false, the last call of the throttled function will not invoke fn?

Why not use an existing package?

CHANGELOG.md

packages/frontend/src/components/composer/ComposerMessageInput.tsx

WofWca · 2024-11-20T09:55:36Z

packages/frontend/src/components/composer/ComposerMessageInput.tsx

      this.props.updateDraftText(text.trim() === '' ? '' : text, chatId)
-    }, 1000)
+    }, 200)


Is the timer supposed to be lowered still?

I think this is sufficient at least in my tests. Somehow the saveDraft seems to be blocking anyway so if I click on a chat or account immediately after typing the click has no effect.
I still managed to have a missing character in the draft or one left over after deleting but I think that's ok. I had to type very fast and click mutiple times on another chat to achieve that.

so if I click on a chat or account immediately after typing the click has no effect.

Uhh, is this also on the 1.48 release version?

I think this is sufficient at least in my tests.

What I meant is that is it supposed to be lowered to 200 instead of staying at 1000. Because switching to throttle is supposed to be enough.

Uhh, is this also on the 1.48 release version?

Yes with current master.

Weird, I can't reproduce it.

It even blocks my mouse pointer whe I try to move it
On current main
I enter fast 5-6 characters and then try to click somewhere - it takes until the draft is saved before I can interact again

Sounds bad, I don't think I've ever seen this. Do you think it's worth to make an issue for it?

packages/frontend/src/components/composer/ComposerMessageInput.tsx

WofWca · 2024-11-20T09:58:34Z

packages/frontend/src/components/composer/ComposerMessageInput.tsx

-    if (!this.state.loadingDraft) {
-      this.saveDraft()
-    }


Why is this removed? Is it just a refactor?

These lines exist also in onComponentDidUpdate what doesn't make sense to me, since switching to another chat also triggers onComponentDidUpdate so in the end a fresh loaded draft is immediately saved again. So why not remove them there and leave them here?

Makes sense.
But the question is if it's directly related to what this commit is doing.
Consider this a nitpick then.

- add throttle to packages/shared/utils.ts - use throttle in saveDraft for messages - remove explicit call to saveDraft when setting state with new text - fixes #3733

WofWca

Tested for a while. Appears to work alright.
I also looked at the throttle implementation again, and it appears to be OK for the current purposes.

If the emoji thing is fine, then let's merge.
Hopefully there are no bad side effects in the refactoring part of this MR.

WofWca · 2024-11-28T10:36:07Z

CHANGELOG.md

@@ -13,6 +13,7 @@
 - "Disappearing Messages" dialog not reflecting the actual current value #4327
 - accessibility: make settings keyboard-navigable #4319
 - Fix documentation for --allow-unsafe-core-replacement #4341
+- fix: save message draft every 200ms if message text changed #3733


Suggested change

- fix: save message draft every 200ms if message text changed #3733

- fix: save message draft every 400ms if message text changed #3733

WofWca · 2024-11-28T10:50:28Z

packages/frontend/src/components/composer/ComposerMessageInput.tsx

-      if (prevState.text !== this.state.text) {
-        if (!this.state.loadingDraft) {
-          this.saveDraft()
-        }
-      }


Now the draft does not get saved after you insert an emoji.

Closes #4430. The bug was introduced in #4144. There we made `throttle` accept the `text` argument instead of reading `this.state.text`, and `this.state.text` would get set to `''` after the message has been sent, but then we'd still invoke the throttled function, with the text from the last `onChange` event.

The bug was introduced in #4144. See #4144 (comment).

Closes #4430. The bug was introduced in #4144. There we made `throttle` accept the `text` argument instead of reading `this.state.text`, and `this.state.text` would get set to `''` after the message has been sent, but then we'd still invoke the throttled function, with the text from the last `onChange` event. This probably doesn't address _all_ the "draft not cleared" issues, e.g. #3586 could be a different issue.

The bug was introduced in #4144. See #4144 (comment).

Closes #4430. The bug was introduced in #4144. There we made `throttle` accept the `text` argument instead of reading `this.state.text`, and `this.state.text` would get set to `''` after the message has been sent, but then we'd still invoke the throttled function, with the text from the last `onChange` event. This probably doesn't address _all_ the "draft not cleared" issues, e.g. #3586 could be a different issue.

The bug was introduced in #4144. See #4144 (comment).

maxphilippov requested review from nicodh and WofWca September 23, 2024 17:18

maxphilippov force-pushed the maxph/composer-quote-debounce-reduce branch 2 times, most recently from c9e78c0 to 2896590 Compare October 15, 2024 20:40

maxphilippov marked this pull request as ready for review October 15, 2024 20:40

maxphilippov changed the title ~~Try to reduce debounce timing for draft change~~ Save draft on chat change Oct 15, 2024

WofWca requested changes Oct 16, 2024

View reviewed changes

packages/frontend/src/components/composer/ComposerMessageInput.tsx Outdated Show resolved Hide resolved

CHANGELOG.md Outdated Show resolved Hide resolved

packages/frontend/src/components/composer/ComposerMessageInput.tsx Outdated Show resolved Hide resolved

maxphilippov force-pushed the maxph/composer-quote-debounce-reduce branch 2 times, most recently from dd5b26b to 3a10e62 Compare October 16, 2024 10:36

maxphilippov marked this pull request as draft October 16, 2024 10:44

maxphilippov force-pushed the maxph/composer-quote-debounce-reduce branch from 3a10e62 to b0a8ac8 Compare November 19, 2024 18:12

maxphilippov marked this pull request as ready for review November 19, 2024 18:13

maxphilippov requested a review from WofWca November 19, 2024 18:46

maxphilippov marked this pull request as draft November 19, 2024 18:46

maxphilippov force-pushed the maxph/composer-quote-debounce-reduce branch from b0a8ac8 to 72b4452 Compare November 19, 2024 19:01

maxphilippov marked this pull request as ready for review November 19, 2024 19:01

WofWca reviewed Nov 20, 2024

View reviewed changes

maxphilippov force-pushed the maxph/composer-quote-debounce-reduce branch from 72b4452 to 75ebd78 Compare November 20, 2024 17:07

maxphilippov force-pushed the maxph/composer-quote-debounce-reduce branch from 75ebd78 to 0398437 Compare November 27, 2024 17:55

Add throttle impl and make saveDraft use throttle

ac42e19

- add throttle to packages/shared/utils.ts - use throttle in saveDraft for messages - remove explicit call to saveDraft when setting state with new text - fixes #3733

maxphilippov force-pushed the maxph/composer-quote-debounce-reduce branch from 0398437 to ac42e19 Compare November 27, 2024 18:01

maxphilippov requested a review from WofWca November 27, 2024 18:04

WofWca approved these changes Nov 28, 2024

View reviewed changes

nicodh approved these changes Nov 28, 2024

View reviewed changes

Merge branch 'main' into maxph/composer-quote-debounce-reduce

d36bb57

nicodh merged commit d1fbc7a into main Nov 28, 2024
10 checks passed

nicodh deleted the maxph/composer-quote-debounce-reduce branch November 28, 2024 16:23

This was referenced Dec 14, 2024

sending a message is often delayed, sometimes buggy #3586

Open

Draft is not cleared after sending sometimes #4430

Closed

WofWca added a commit that referenced this pull request Jan 16, 2025

fix: draft not saving after inserting emoji

ed765d4

The bug was introduced in #4144. See #4144 (comment).

WofWca added a commit that referenced this pull request Jan 16, 2025

fix: draft not saving after inserting emoji

cabea32

The bug was introduced in #4144. See #4144 (comment).

WofWca mentioned this pull request Jan 16, 2025

fix: draft not clearing after sending message #4493

Merged

WofWca added a commit that referenced this pull request Jan 16, 2025

fix: draft not saving after inserting emoji

e750727

The bug was introduced in #4144. See #4144 (comment).

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Save draft on chat change #4144

Save draft on chat change #4144

maxphilippov commented Sep 23, 2024

maxphilippov commented Sep 23, 2024 •

edited

Loading

maxphilippov commented Oct 15, 2024

WofWca left a comment

maxphilippov commented Oct 16, 2024

WofWca commented Oct 16, 2024

maxphilippov commented Oct 16, 2024

maxphilippov commented Nov 19, 2024

WofWca Nov 20, 2024

WofWca Nov 20, 2024

nicodh Nov 21, 2024

WofWca Nov 21, 2024

WofWca Nov 21, 2024

nicodh Nov 21, 2024

WofWca Nov 21, 2024

nicodh Nov 21, 2024

WofWca Nov 21, 2024

WofWca Nov 20, 2024

nicodh Nov 21, 2024

WofWca Nov 21, 2024

WofWca left a comment

WofWca Nov 28, 2024

WofWca Nov 28, 2024

	- fix: save message draft every 200ms if message text changed #3733
	- fix: save message draft every 400ms if message text changed #3733

Save draft on chat change #4144

Save draft on chat change #4144

Conversation

maxphilippov commented Sep 23, 2024

maxphilippov commented Sep 23, 2024 • edited Loading

maxphilippov commented Oct 15, 2024

WofWca left a comment

Choose a reason for hiding this comment

maxphilippov commented Oct 16, 2024

WofWca commented Oct 16, 2024

maxphilippov commented Oct 16, 2024

maxphilippov commented Nov 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

WofWca left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maxphilippov commented Sep 23, 2024 •

edited

Loading