Page MenuHomePhabricator

Call textMessageCreationSideEffectsFunc after sending DM text message
ClosedPublic

Authored by marcin on Sep 13 2024, 6:12 AM.
Tags
None
Referenced Files
F3378207: D13334.diff
Wed, Nov 27, 9:35 AM
Unknown Object (File)
Wed, Nov 20, 10:58 AM
Unknown Object (File)
Wed, Nov 20, 10:58 AM
Unknown Object (File)
Wed, Nov 20, 10:58 AM
Unknown Object (File)
Mon, Nov 18, 11:35 PM
Unknown Object (File)
Fri, Nov 15, 2:02 AM
Unknown Object (File)
Thu, Nov 14, 2:52 PM
Unknown Object (File)
Thu, Nov 14, 1:02 PM
Subscribers

Details

Summary

This differential adds user to thick sidebar when another user mentions them in a message

Test Plan
  1. Create thick sidebar
  2. Send message mentioning a user
  3. Ensure that they are added to the sidebar

Diff Detail

Repository
rCOMM Comm
Branch
marcin/eng-9100
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

This revision is now accepted and ready to land.Sep 16 2024, 1:17 AM

Setting @kamil to blocking on this as he recently refactored this logic – I want to make sure the changes still make sense to him. Sorry for any annoyance!

This revision now requires review to proceed.Sep 16 2024, 9:59 AM

Before I start reviewing - can you rebase to the newest master?

You using code that was removed in D13329 so I guess this will change significantly.

kamil requested changes to this revision.Sep 17 2024, 12:52 AM

Requesting changes to put back into your queue only

This revision now requires changes to proceed.Sep 17 2024, 12:52 AM
native/input/input-state-container.react.js
576

this is very ugly but recent changes on master (extracting sending message to hook) and the fact that we have this issue: https://linear.app/comm/issue/ENG-9103/update-processandsenddmoperation-to-support-multiple-dm-ops-at-once forces us to this change

It appears that the only thing this diff is doing is having textMessageCreationSideEffectsFunc called after the text message is created for thick threads, but before the text message is created for thin threads.

Two questions:

  1. Why do the side effects need to happen after the text message is created for thick threads?
  2. Should we just have the side effects run after the text message for thin threads as well?

It appears that the only thing this diff is doing is having textMessageCreationSideEffectsFunc called after the text message is created for thick threads, but before the text message is created for thin threads.

Two questions:

  1. Why do the side effects need to happen after the text message is created for thick threads?

This is explained in detail here

  1. Should we just have the side effects run after the text message for thin threads as well?

I haven't investigated this direction since current approach has been around for some time so I concluded that it is better not to change it we don't have to. However I think that we should keep the current approach for thin threads. If sending message succeeds but adding to thread fails the user receives a message form thread they don't belong to which is strange UX. We should update approach for thick threads as soon as it is possible ( after this is done).

This revision is now accepted and ready to land.Sep 18 2024, 5:51 AM
This revision was landed with ongoing or failed builds.Sep 18 2024, 7:13 AM
This revision was automatically updated to reflect the committed changes.