Call textMessageCreationSideEffectsFunc after sending DM text message
ClosedPublic
Actions

Authored by marcin on Sep 13 2024, 6:12 AM.

Details

Reviewers

tomek
kamil
inka
ashoat

Commits

rCOMM189c1fde4eed: Call textMessageCreationSideEffectsFunc after sending DM text message

Summary

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

Test Plan

Create thick sidebar
Send message mentioning a user
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

marcin created this revision.Sep 13 2024, 6:12 AM

Herald added a subscriber: ashoat. · View Herald TranscriptSep 13 2024, 6:12 AM

Harbormaster completed remote builds in B31612: Diff 44172.Sep 13 2024, 6:29 AM

marcin requested review of this revision.Sep 13 2024, 6:29 AM

tomek accepted this revision.Sep 16 2024, 1:17 AM

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.

Requesting changes to put back into your queue only

This revision now requires changes to proceed.Sep 17 2024, 12:52 AM

Rebase master

marcin added inline comments.Sep 17 2024, 2:43 AM

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

Harbormaster completed remote builds in B31661: Diff 44264.Sep 17 2024, 2:57 AM

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?

In D13334#376477, @ashoat wrote:

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?

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

ashoat accepted this revision.Sep 18 2024, 4:11 AM

kamil accepted this revision.Sep 18 2024, 5:51 AM

This revision is now accepted and ready to land.Sep 18 2024, 5:51 AM

Rebase before landing

This revision was landed with ongoing or failed builds.Sep 18 2024, 7:13 AM

Closed by commit rCOMM189c1fde4eed: Call textMessageCreationSideEffectsFunc after sending DM text message (authored by marcin). · Explain Why

This revision was automatically updated to reflect the committed changes.

marcin added a commit: rCOMM189c1fde4eed: Call textMessageCreationSideEffectsFunc after sending DM text message.

Harbormaster completed remote builds in B31693: Diff 44305.Sep 18 2024, 7:22 AM