Page MenuHomePhabricator

[keyserver/lib/web/native] Modify fetchRelatedMessages to fetch toggle_pin messages

Authored by rohan on Mon, Aug 28, 9:49 AM.
Referenced Files
Unknown Object (File)
Fri, Sep 22, 2:32 AM
Unknown Object (File)
Thu, Sep 21, 1:59 AM
Unknown Object (File)
Sat, Sep 16, 11:36 PM
Unknown Object (File)
Sat, Sep 16, 2:30 AM
Unknown Object (File)
Fri, Sep 8, 12:09 PM
Unknown Object (File)
Wed, Sep 6, 7:30 PM
Unknown Object (File)
Sat, Sep 2, 1:29 PM
Unknown Object (File)
Sat, Sep 2, 12:43 PM



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

rCOMM Comm
Lint Not Applicable
Tests Not Applicable

Event Timeline

rohan edited the test plan for this revision. (Show Details)
rohan edited the test plan for this revision. (Show Details)
184 ↗(On Diff #30457)

We could probably use a better name for this but just kept it as a simple rename change

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 ChatMessageInfos 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.

86 ↗(On Diff #30457)

Same comment as above

inka added inline comments.
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

This revision is now accepted and ready to land.Tue, Aug 29, 4:07 AM

Revert to using isPinned checks

This revision was landed with ongoing or failed builds.Tue, Aug 29, 10:22 AM
This revision was automatically updated to reflect the committed changes.