Page MenuHomePhabricator

[DRAFT][WIP] alternative approach to composable DM messages
AbandonedPublicDraft

Authored by kamil on Wed, Sep 11, 7:21 AM.
Tags
None
Referenced Files
F2753994: D13290.id44043.diff
Wed, Sep 18, 6:39 PM
F2753678: D13290.id44045.diff
Wed, Sep 18, 6:09 PM
Unknown Object (File)
Tue, Sep 17, 7:05 PM
Unknown Object (File)
Tue, Sep 17, 2:59 AM
Unknown Object (File)
Mon, Sep 16, 4:23 PM
Unknown Object (File)
Sat, Sep 14, 3:30 PM
Unknown Object (File)
Wed, Sep 11, 11:22 AM
Unknown Object (File)
Wed, Sep 11, 11:21 AM
Subscribers

Details

Reviewers
tomek
Summary

This is an alternative approach to an existing mechanism for manual retries and sending composable DM messages.

Changes here:

  1. Using dedicated actions (sendMultimediaMessageActionTypes and sendTextMessageActionTypes) and removing additional sendDMActionTypes.
  2. Removed complicated code from useProcessAndSendDMOperation and removed entirely useRetrySendDMOperation (because we using dedicated actions, it should work out-of-the-box).
  3. Change approach to not always return success with messages that succeded, but failure when at least one message failed and handled this on sendTextMessageActionTypes.failed or sendMultimediaMessageActionTypes.failed - which is more intuitive.
  4. In the case of text and multimedia messages, DMOperation is sendOnly - changes to our store are made using dedicated actions.
  5. Logic with localID is now the same as for thin threads. When sending actual DM Operation we generate a new uuid for the message, which works the same way as serverID for think threads.
  6. InputStateContainer works regardless of whether this is a thin or thick thread, only when making an actual API request, we decide whether we should hit keyserver or tunnelbroker.

With that change, code is more readable and easier to maintain, and it reduces some complexity.

Additionally, because there are no additional changes related to multimedia in InputStateContainer, it makes it realistic to deliver it this week.

I would love high-level feedback - if this is okay I can slice it for smaller diffs and improve the code.

Test Plan
  1. Sending text messages.
  2. Uploading and sending multimedia.
  3. Message statuses.
  4. Retries.
  5. Sent & failed messages are persisted.

Tested both web and native.

Diff Detail

Repository
rCOMM Comm
Branch
sending
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil retitled this revision from [DRAFT] alternative approach to composable DM messages to [DRAFT][WIP] alternative approach to composable DM messages.Wed, Sep 11, 7:44 AM
kamil edited the summary of this revision. (Show Details)

I haven't looked too closely, but this feels like a huge improvement to me! The changes listed in the diff description seem great, and the readability seems significantly improved. As a next step, I think we can go ahead and break this down into smaller diffs as you suggested

lib/reducers/message-reducer.js
1885–1915

This logic confuses me. It looks like we're setting the localMessageInfo (if it doesn't already exist) to say sendFailed: false, but how do we know that the message succeeded?

Perhaps we don't know yet if the send failed, but we want to mark the outboundP2PMessageIDs immediately?

1893–1894

I think "auto retry" is confusing naming for this, because it affects so much other logic

I wonder if "composable" would be a better term

lib/shared/dm-ops/process-dm-ops.js
67

Is the idea that we will only specify this for composable messages? And in other cases it will be null / undefined? I wonder if the API could document this better. Some ideas:

  1. Instead of having a single useProcessDMOperation used for both composable / non-composable messages, we could have a separate one for composable messages that requires localMessageID. Then we could export useProcessDMOperation but with a different type that doesn't mention localMessageID for use with non-composable messages
  2. We could make the input a union of types, one for composable and one for non-composable
lib/types/redux-types.js
725

Can the naming be improved so it's clear that this is only for thick threads?

Thanks @ashoat for the feedback!

Everything here was finished, improved, and moved to stack starting with D13319.

All the comments are addressed (linked in the summary) or explained (linked in diffs too).