Page MenuHomePhabricator

Fix DM activity handler not to send unnecessary read status update
ClosedPublic

Authored by marcin on Sep 18 2024, 7:36 AM.
Tags
None
Referenced Files
F3013418: D13383.id44403.diff
Sat, Oct 19, 5:44 AM
F3012483: D13383.id44402.diff
Sat, Oct 19, 3:26 AM
Unknown Object (File)
Mon, Oct 14, 12:44 PM
Unknown Object (File)
Mon, Oct 14, 12:44 PM
Unknown Object (File)
Thu, Oct 10, 6:12 AM
Unknown Object (File)
Mon, Oct 7, 1:58 PM
Unknown Object (File)
Fri, Oct 4, 1:28 AM
Unknown Object (File)
Thu, Oct 3, 6:21 PM
Subscribers
None

Details

Summary

This differential prevents DM activity handler from spamming unread status unpdates by sending updates only for threads that are unread when we enter them and using debouncing when receiving messages to currently active thread.

Test Plan
  1. Add console log to each portion of sent dm's
  2. Ensure that when entering thick thread updates are sent once only if the thread was unread.
  3. Ensuure that after receiving stream of messages to thick thread, unread updates are sent only once 5 seconds after the last message.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

lib/handlers/dm-activity-handler.js
89 ↗(On Diff #44314)

If there is pending debounced call from previous active thread we want to call it right now.

110 ↗(On Diff #44314)

Checking for change in latest message makes sense only if we are in the same thread.

web/redux/redux-setup.js
460 ↗(On Diff #44314)

This code was changing thick thread read status before dm activity handler could detect change and send necessary updates.

tomek requested changes to this revision.Sep 19 2024, 3:02 AM

This solution feels a lot more complicated than it should. Maybe it has to be that complicated, with canceling and flushing, but I'm not sure.

Would it be possible to have a memo, with activeThread as a dependency, where we store the denounced function and then simply call this function in an effect, without canceling and flushing?

This revision now requires changes to proceed.Sep 19 2024, 3:02 AM
tomek added inline comments.
lib/handlers/dm-activity-handler.js
53–67 ↗(On Diff #44345)

This can be made even more efficient, because updateDMActivity shouldn't require the whole thread info - it looks like the ID is the only thing that really matters there.

126 ↗(On Diff #44345)

Can't we call updateActivityAfterLatestMessageChangeRef.current() here?

This revision is now accepted and ready to land.Fri, Sep 20, 2:21 AM
lib/handlers/dm-activity-handler.js
126 ↗(On Diff #44345)

We can't since thread will be marked as read with 5 seconds delay after opening it. With current approach it happens immediately.

lib/handlers/dm-activity-handler.js
126 ↗(On Diff #44345)

What do we do for thick threads? Is there no delay for them?

lib/handlers/dm-activity-handler.js
126 ↗(On Diff #44345)

I don't understand this question.

This component handles thick threads exclusively. It works like this:

When unread thick thread is entered unread updates are sent immediately and so does happen UI change. When thick thread is opened and new messages are incoming very shortly one after another unread updates are sent only after the last one with 5 seconds delay due to debouncing mechanism. Does this answer your question?

lib/handlers/dm-activity-handler.js
126 ↗(On Diff #44345)

Sorry, this is my fault... I meant to ask about thin threads, but I wrote "thick threads"

lib/handlers/dm-activity-handler.js
126 ↗(On Diff #44345)

Thin threads become read immediately after entering them. Thin threads are controlled by activity-handlers that communicate directly with the keyserver and work in a different way than this component.

lib/handlers/dm-activity-handler.js
126 ↗(On Diff #44345)

Thanks for explaining!

This revision was landed with ongoing or failed builds.Fri, Sep 20, 1:01 PM
This revision was automatically updated to reflect the committed changes.