Page MenuHomePhabricator

[native][web] Hide edit label and reactions of deleted messages
ClosedPublic

Authored by tomek on Fri, Apr 4, 7:11 AM.
Tags
None
Referenced Files
Unknown Object (File)
Mon, Apr 21, 5:19 AM
Unknown Object (File)
Sun, Apr 20, 10:24 PM
Unknown Object (File)
Fri, Apr 18, 12:11 AM
Unknown Object (File)
Thu, Apr 17, 7:22 AM
Unknown Object (File)
Thu, Apr 17, 2:09 AM
Unknown Object (File)
Tue, Apr 15, 10:45 AM
Unknown Object (File)
Mon, Apr 14, 11:48 PM
Unknown Object (File)
Mon, Apr 14, 8:03 PM
Subscribers
None

Details

Summary

When a message is deleted, we should hide all the inline engagement components but a sidebars - because it is possible that a sidebar was created before the message was deleted.

https://linear.app/comm/issue/ENG-10494/update-reaction-logic
https://linear.app/comm/issue/ENG-10493/update-sidebar-creation-logic

Depends on D14547

Test Plan

Checked on both native and web that reactions and edit label aren't shown for a deleted message.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested review of this revision.Fri, Apr 4, 7:30 AM
ashoat added inline comments.
lib/shared/chat-message-item-utils.js
101

The implication is that it's impossible to delete a message from which a sidebar has been created, right? Do we make sure to prevent this on both sender/recipient sides for both keyserver-hosted and DM messages?

native/chat/chat-item-height-measurer.react.js
126

This can be false, since there is no way to get to this code branch if item.deleted is true

This revision is now accepted and ready to land.Fri, Apr 4, 8:57 AM

Simplify

lib/shared/chat-message-item-utils.js
101

The implication is that it's impossible to delete a message from which a sidebar has been created, right?

Yes, that's correct.

Do we make sure to prevent this on both sender/recipient sides for both keyserver-hosted and DM messages?

The UI handles it well - it isn't possible to create a sidebar from a deleted message.

For the DMs, when we receive an operation about creating a sidebar from a deleted message, we should accept it, as it might be a result of a race condition (user A creates a sidebar from a message, and at the same time user B creates a sidebar from this message, and user C receives an operation from A and then from B).

For the thin threads we can consider adding some more validation on the keyserver side. We block creating sidebars from DELETE_MESSAGE but we don't check if e.g. TEXT_MESSAGE that is the source of a sidebar currently being created was deleted. Introducing this check isn't too complicated, but I'm not sure if it is that necessary - the clients should handle this case well because creating a sidebar and then deleting a message from which it was created produces the same result and is a valid scenario.

lib/shared/chat-message-item-utils.js
101

The implication is that it's impossible to delete a message from which a sidebar has been created, right?

I got confused. It is possible to delete a message from which a sidebar was created. It isn't possible to create a sidebar from deleted message.

lib/shared/chat-message-item-utils.js
101–102 ↗(On Diff #47729)

It seems that if a message has a sidebar and is then deleted, this function still returns true.

Does that mean the sidebar is still shown underneath the message? Are the reactions also shown, or do we hide those by not including them in chat-selectors.js?

lib/shared/chat-message-item-utils.js
101–102 ↗(On Diff #47729)

Yes, the sidebar is still shown. Reactions and edit label aren't - this is handled in this diff by the changes in inline-engagement for both platforms, where we hide reactions and label if deleted === true.