[lib][native][web] Fix bug where no messages are loaded if most recent 20 are reactions
ClosedPublic
Actions

Authored by ashoat on May 8 2023, 10:40 AM.

Details

Reviewers

atul
tomek
ginsu

Commits

rCOMMa3a4e1535562: [lib][native][web] Fix bug where no messages are loaded if most recent 20 are…

Summary

This resolves ENG-3851. There are two issues here:

oldestMessageServerID is checking messageListData, which does not include reactions by themselves. If new results are all reactions, messageListData will not update, and oldestMessageServerID will stay the same.
The code for setting loadingFromScroll relies on messageListData changing, which (as mentioned above) is no longer guaranteed if returned messages are reactions.

Test Plan

I tested this on both web and native by creating over 20 reactions in my local environment. That allowed me to repro the issue after an initial load on web or a log-in on native. I was able to confirm the issue was resolved with this diff

Diff Detail

Repository

rCOMM Comm

Branch

ashoat/fixrockclimbingbug

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

ashoat created this revision.May 8 2023, 10:40 AM

Unset loadingFromScroll from a finally block

Harbormaster completed remote builds in B19144: Diff 26214.May 8 2023, 11:00 AM

Harbormaster completed remote builds in B19145: Diff 26215.May 8 2023, 11:21 AM

ashoat requested review of this revision.May 8 2023, 11:21 AM

ashoat added inline comments.May 8 2023, 12:42 PM

lib/shared/message-utils.js
383 ↗	(On Diff #26215)	Inspired by the function directly above (`getMostRecentNonLocalMessageID`). I initially just used `findLast` instead of `find`, but `findLast` isn't introduced in Node.js until Node 18, and Flow marked it as a type error
native/chat/message-list.react.js
62 ↗	(On Diff #26215)	At some point we concluded that these aren't needed anymore, but haven't gotten around to fully stripping them from the codebase yet
143–150 ↗	(On Diff #26215)	This is replaced below
288 ↗	(On Diff #26215)	React Native's Flow types prevented me from making `onViewableItemsChanged` return a `Promise`, so I had to add this IIFE wrapper
305 ↗	(On Diff #26215)	I figured this is the cleanest way to make sure `loadingFromScroll` is always unset afterwards
web/chat/chat-message-list.react.js
49 ↗	(On Diff #26215)	At some point we concluded that these aren't needed anymore, but haven't gotten around to fully stripping them from the codebase yet
110–118 ↗	(On Diff #26215)	This is replaced below
255 ↗	(On Diff #26215)	I figured this is the cleanest way to make sure `loadingFromScroll` is always unset afterwards

tomek accepted this revision.May 9 2023, 1:34 AM

tomek added inline comments.

lib/shared/message-utils.js
392–393 ↗	(On Diff #26215)	I think that spreading and reverting messages should be avoided. If we do this, we will have to always perform a number of operations proportional to number of all the messages in a thread. If we instead iterate by using a `for` loop from the end, we will find the message quickly (most of the times).