[web] Display pinned messages not present in Redux
ClosedPublic
Actions

Authored by rohan on Jun 5 2023, 3:05 PM.

Details

Reviewers

atul
inka
ginsu

Commits

rCOMMdfd7ca746c5e: [web] Display pinned messages not present in Redux

Summary

As mentioned in ENG-3924, there is an issue with some pinned messages where if they are
not present in Redux, they will not show up in the modal.

This is because messageListData will incorrectly set the messageInfo's
isPinned flag to false, and therefore we will filter it out. This diff
resolves this on web by keeping a Set of pinned messgae IDs, and
filtering out messages that are not part of the set, rather than based on
their isPinned value. This maintains the same behaviour for pinned
messages that were fetched, but also covers the case where they are not
part of Redux.

native will come in the next diff

Test Plan

Please see the before and after for a pinned message that had
no reactions/related messages and was not present in Redux. Confirmed
separately that pinned messages on web still behave as expected.

Before:

After:

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Jun 5 2023, 3:05 PM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptJun 5 2023, 3:05 PM

rohan edited the summary of this revision. (Show Details)Jun 5 2023, 3:06 PM

rohan edited the test plan for this revision. (Show Details)

rohan attached a referenced file: F572653: Screen Recording 2023-06-05 at 5.58.54 PM.mov. (Show Details)

rohan attached a referenced file: F572652: Screen Recording 2023-06-05 at 5.58.31 PM.mov. (Show Details)

rohan added a child revision: D8097: [native] Display pinned messages not present in Redux.Jun 5 2023, 3:13 PM

Harbormaster completed remote builds in B20027: Diff 27447.Jun 5 2023, 3:24 PM

rohan requested review of this revision.Jun 5 2023, 3:24 PM

inka added inline comments.Jun 6 2023, 2:02 AM

web/modals/chat/message-results-modal.react.js
78–82 ↗	(On Diff #27447)	I think this comment doesn't make sense for someone who has not seen the previous version of this code - we are mentioning that "Some pinned messages (...) will get excluded since their `isPinned` flag will not be set as true", but this makes no sense in with this implementation.