Page MenuHomePhabricator

[web] Fetch preview messages from non-watched threads in thread subchannels modal
ClosedPublic

Authored by jacek on Apr 1 2022, 7:34 AM.
Tags
None
Referenced Files
F3487086: D3601.id11010.diff
Wed, Dec 18, 6:53 AM
F3486386: D3601.diff
Wed, Dec 18, 4:46 AM
F3484346: D3601.diff
Tue, Dec 17, 11:33 AM
Unknown Object (File)
Nov 17 2024, 9:53 PM
Unknown Object (File)
Nov 9 2024, 4:02 PM
Unknown Object (File)
Nov 3 2024, 1:23 PM
Unknown Object (File)
Oct 19 2024, 12:17 PM
Unknown Object (File)
Oct 19 2024, 12:17 PM

Details

Summary

The diff introduces fetching missing data using fetchSingleMostRecentMessages action for a thread that user is not member of.

Test Plan

After the change, user that is not member of some subchannel should see preview message for this subchannel in subchannels list for parent thread.

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

tomek requested changes to this revision.Apr 4 2022, 3:11 AM
tomek added inline comments.
web/modals/threads/subchannels/subchannels-modal.react.js
62–90 ↗(On Diff #10961)

It looks like this logic is invalid:

  1. We display a subchannel that has no messages
  2. We register a thread watcher for it
  3. We dispatch an action to fetch the most recent message
  4. The most recent message arrives
  5. threadIDsWithNoMessages is updated - the subchannel is no longer there as it has messages
  6. The effect is called, where we unwatch the subchannel
  7. We don't add the subchannel to watched list as it has messages

So basically, we watch for threads that do not have messages and fetch messages for them, but when the messages are there, we unwatch the threads.

The solution to this would be to find a different condition by which we choose which threads should be included in threadIDsWithNoMessages

79–82 ↗(On Diff #10961)

It's not related to this diff, but we could do something similar for threads in which the most recent message is more than 2 weeks old. Currently we show no messages in that case.

This revision now requires changes to proceed.Apr 4 2022, 3:11 AM
web/modals/threads/subchannels/subchannels-modal.react.js
62–90 ↗(On Diff #10961)

You're absolutely right. Sorry for missing it

web/modals/threads/subchannels/subchannels-modal.react.js
79–82 ↗(On Diff #10961)

I don't think if it's necessary here, as we fetch max. n messages for n (missing in chat list) threads. I think, it won't affect performance much, but the user experience will be worse.

Refactor some code & fix logic

tomek requested changes to this revision.Apr 5 2022, 9:13 AM
tomek added inline comments.
web/modals/threads/subchannels/subchannels-modal.react.js
56 ↗(On Diff #11010)
56 ↗(On Diff #11010)

This should be defined closer to its usage (effect)

75–83 ↗(On Diff #11010)

Should we also check if these are in subchannelsIDsNotInChatList?

75–87 ↗(On Diff #11010)

Move these closer to their usage

79–82 ↗(On Diff #10961)

Right, it is not necessary here. I was suggesting a fix for the current state where on login we only fetch recent messages and display no messages on thread list where the most recent message is old enough. We could dispatch this action for them after the login is successful. (This is not related to this diff)

This revision now requires changes to proceed.Apr 5 2022, 9:13 AM
web/modals/threads/subchannels/subchannels-modal.react.js
75–83 ↗(On Diff #11010)

I don't think we should do it. In my opinion, we should try to fetch a single message from all threads that don't contain any. If we had a thread in our chat list, but we wouldn't have any message from it (e.g. because all had been sent more than 14 days before), we should probably try to fetch one instead of displaying No messages.

Moved code as suggested and fix typo

tomek added inline comments.
web/modals/threads/subchannels/subchannels-modal.react.js
67–69 ↗(On Diff #11090)

Technically, this if is not needed, but let's keep it for readability

This revision is now accepted and ready to land.Apr 6 2022, 6:43 AM

fix missed problems caused by rebase