native] Modify fetchRelatedMessages to fetch toggle_pin messages
ClosedPublic
Actions

Authored by rohan on Aug 28 2023, 9:49 AM.

Details

Reviewers

inka
ginsu
atul

Commits

rCOMM6b8fce7f0f21: [keyserver/lib/web/native] Modify fetchRelatedMessages to fetch toggle_pin…

Summary

Currently, if a message is pinned, a toggle_pin robotext message appears in chat. This is how we determine whether a message's status is 'pinned' or 'unpinned' in order to display the appropriate action for the user if they want to toggle that. The problem with this is that if a message is not fetched when the chat is loaded, then when the message results modal is opened, the pinned message doesn't
have the right information and will always be unpinned until the chat is scrolled up enough.

This diff modifies fetchRelatedMessages to include all messages of the TOGGLE_PIN type, so we know what messages should be pinned when creating the ChatMessageItems in chat-selectors. Since these are actual, visible messages I also had to filter them out of the view in message-results-modal and message-results-screen on web and native.

The last thing done is here a quick rename to update the callsites since we're now including pinned messages, so I've renamed isMessageSidebarSourceReactionOrEdit to isMessageSidebarSourceReactionEditOrPin. I can also do this rename in a separate diff if it's too complicated to review the changes.

Resolves ENG-4671

Test Plan

Before - old pinned messages show the 'pin' action in the message results modal until rendered in chat

After - old pinned messages show the 'unpin' action in the message results modal even before the user scrolls to render them

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

rohan created this revision.Aug 28 2023, 9:49 AM

Herald added subscribers: tomek, ashoat. · View Herald TranscriptAug 28 2023, 9:49 AM

rohan edited the summary of this revision. (Show Details)Aug 28 2023, 9:50 AM

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

rohan added inline comments.Aug 28 2023, 9:54 AM

lib/types/message-types.js
184 ↗	(On Diff #30457)	We could probably use a better name for this but just kept it as a simple rename change
native/chat/message-results-screen.react.js
81 ↗	(On Diff #30457)	As mentioned in the description, this is because toggle_pin messages are visible robotext, so if we don't filter those out then they will become visible in the message results modal. This is done here, after the `ChatMessageInfo`s have been created, so we can accurately use the updated fetched related messages to determine the `isPinned` flag. If we filtered this earlier, then we'd be in the same situation as before This also is fine because we only allow the user to pin composable message types so we won't exclude anything from the view that we need to show.
web/modals/chat/message-results-modal.react.js
86 ↗	(On Diff #30457)	Same comment as above

Harbormaster completed remote builds in B22181: Diff 30457.Aug 28 2023, 10:07 AM

rohan requested review of this revision.Aug 28 2023, 10:07 AM

inka accepted this revision.Aug 29 2023, 4:07 AM

inka added inline comments.

native/chat/message-results-screen.react.js
74–75 ↗	(On Diff #30457)	We could probably go back to using `isPinned` property here. The thing we undid in D8097. This would make the code more readable, but it's up to you