This differential adds user to thick sidebar when another user mentions them in a message
Details
Details
- Create thick sidebar
- Send message mentioning a user
- Ensure that they are added to the sidebar
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
Lint Not Applicable - Unit
Tests Not Applicable
Event Timeline
Comment Actions
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!
Comment Actions
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.
| native/input/input-state-container.react.js | ||
|---|---|---|
| 576 ↗ | (On Diff #44264) | 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 |
Comment Actions
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:
- Why do the side effects need to happen after the text message is created for thick threads?
- Should we just have the side effects run after the text message for thin threads as well?
Comment Actions
This is explained in detail here
- 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).