Page MenuHomePhabricator

[web] Display pinned messages not present in Redux
ClosedPublic

Authored by rohan on Jun 5 2023, 3:05 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Dec 15, 1:45 PM
Unknown Object (File)
Sun, Dec 15, 1:45 PM
Unknown Object (File)
Sun, Dec 15, 1:45 PM
Unknown Object (File)
Sun, Dec 15, 1:42 PM
Unknown Object (File)
Sun, Dec 15, 1:19 PM
Unknown Object (File)
Tue, Nov 26, 4:18 PM
Unknown Object (File)
Nov 23 2024, 2:52 AM
Unknown Object (File)
Nov 10 2024, 1:59 PM
Subscribers

Details

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
No Lint Coverage
Unit
No Test Coverage

Event Timeline

rohan requested review of this revision.Jun 5 2023, 3:24 PM
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.

This revision is now accepted and ready to land.Jun 6 2023, 6:49 AM