Page MenuHomePhabricator

[lib] generate message store ops for threads while starting to send a message
ClosedPublic

Authored by kamil on Apr 13 2023, 7:30 AM.
Tags
None
Referenced Files
F3253605: D7417.id25105.diff
Fri, Nov 15, 7:15 PM
F3253557: D7417.id25350.diff
Fri, Nov 15, 7:12 PM
F3251966: D7417.diff
Fri, Nov 15, 6:01 PM
Unknown Object (File)
Fri, Nov 8, 5:21 AM
Unknown Object (File)
Tue, Nov 5, 4:20 AM
Unknown Object (File)
Thu, Oct 31, 11:43 AM
Unknown Object (File)
Oct 1 2024, 12:00 PM
Unknown Object (File)
Oct 1 2024, 12:00 PM
Subscribers

Details

Summary

This code generates operations needed for updating threads part of message store after starting to send a message.

In this case it's hard to early exist and remove indentation and have one place for processing ops and asserting results.

Depends on D7412

Test Plan

Try to send message using different flows on both web and native

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Apr 13 2023, 8:27 AM
tomek requested changes to this revision.Apr 17 2023, 5:01 AM

This diff is really hard to review. Wondering if splitting it would make it better - if every smaller diff had changes that are easy to review. I guess it might be challenging, so if you think that it's not possible, request a review again and I will give the review another try.

This revision now requires changes to proceed.Apr 17 2023, 5:01 AM
kamil requested review of this revision.Apr 18 2023, 7:12 AM

@tomek more context about this diff:

The order of modifications in this actions is:

  1. Create an operation to replace the message with id localID
  2. Processing this operation
  3. Checking if the message with localID exists in the old state, and if yes:
    • update threads and local
    • return the modified store with messages processed by ops previously
  4. Iterate over messageIDs in this thread and if a message exists and has the same localID return the old store with empty ops (which means 1 and 2 were useless)
  5. Update threads
  6. Return updated threads and messages processed by ops from step 2.

Which I believe is complicated and adding ops for threads will complicate code even more, we can change the order (the only downgrade will be that indentation will increase a bit), I would suggest something like this (new order):

  1. Make sure that is no message with id === localID in the store, and iterate over all messageIDs, and if a message with the same localID exists return old state. (current step 4).
  2. Create an operation to replace the message with id localID (current step 2)
  3. Checking if the message with localID exists, and:
    • a. if yes - update local and threads (generate ops for threads) (current step 3)
    • b. if no - update threads (generate ops for threads) (current step 5)
  4. Process operations
  5. Return state with modified threads and messages processed by ops

Otherwise, we will have to process ops multiple times and compare results multiple times which I think is bad.

@tomek let me know if this helped you, otherwise, I will try to split this or change the approach

lib/reducers/message-reducer.js
1031–1039 ↗(On Diff #25105)

this is new step 1

1041–1046 ↗(On Diff #25105)

new step 2

1050–1074 ↗(On Diff #25105)

new step 3.a

1075–1100 ↗(On Diff #25105)

new step 3.b

1102–1118 ↗(On Diff #25105)

new step 4

1120–1123 ↗(On Diff #25105)

new step 5

After reading the explanation and spending a lot more time, I was able to review this. This code is complicated, but I don't see any serious issues in it.

lib/reducers/message-reducer.js
1090–1099 ↗(On Diff #25105)

This fragment is present in both if and else and it can be extracted, which also would allow threads being const which will improve the readability

This revision is now accepted and ready to land.Apr 19 2023, 1:58 AM

extract shared code and rebase

lib/reducers/message-reducer.js
1090–1099 ↗(On Diff #25105)

right, thanks for catching