Page MenuHomePhabricator

[lib] generate message store ops for threads while merging messages
ClosedPublic

Authored by kamil on Apr 12 2023, 9:56 AM.
Tags
None
Referenced Files
F3526032: D7398.diff
Mon, Dec 23, 7:16 PM
Unknown Object (File)
Thu, Dec 5, 2:25 AM
Unknown Object (File)
Wed, Dec 4, 1:01 PM
Unknown Object (File)
Nov 15 2024, 8:32 PM
Unknown Object (File)
Nov 15 2024, 8:30 PM
Unknown Object (File)
Nov 15 2024, 7:36 PM
Unknown Object (File)
Nov 15 2024, 6:07 PM
Unknown Object (File)
Nov 8 2024, 2:46 PM
Subscribers

Details

Summary

This code generates operations needed for updating threads part of message store. Currently, only for comparing results, but after some time we can rely only on generated ops.

Depends on D7397

Test Plan
  1. Test app (multiple flows) and check if assert does not fail
  2. Comment some ops code and make sure assert will fail (to check if asserting it works as supposed to)

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

kamil held this revision as a draft.
kamil published this revision for review.Apr 13 2023, 8:24 AM
tomek requested changes to this revision.Apr 14 2023, 5:29 AM
tomek added inline comments.
lib/reducers/message-reducer.js
256 ↗(On Diff #25101)

I guess we can make this a Set

279–290 ↗(On Diff #25101)

We should add these only if they are meaningful. It seems like threadsToRemove can be empty and we don't need to create an op for it.

536 ↗(On Diff #25101)

This comment sounds confusing to me. Is it something you will do after "sorting" or "sorting" is a part of this code and the value is somehow updated after it? Please make this less confusing.

620 ↗(On Diff #25101)

Should we also take threads prop of processedMessageStore?

689–692 ↗(On Diff #25101)

Why do we remove these threads?

This revision now requires changes to proceed.Apr 14 2023, 5:29 AM
kamil requested review of this revision.Apr 18 2023, 5:50 AM

Back to you with response to comments - I will update two nits (not creating op for empty array and removing comment) shortly

lib/reducers/message-reducer.js
256 ↗(On Diff #25101)

What are the advantages of using Set here?

In the next lines, I iterate through message store threads only once and if the condition is met I add value to the array, which means each value will occur only once

279–290 ↗(On Diff #25101)

right, good catch

536 ↗(On Diff #25101)

this comment was a note I made while developing and forgot to remove, I will simply get rid of this comment

620 ↗(On Diff #25101)

Lets assume that:
A - old state
B - state modified in reducer, the current logic
C - state modified by ops

previously it worked like this:

  1. We have an old state (l A)
  2. We modify the state (A -> B)
  3. We return the modified state (B)

this diff this changes to:

  1. We have an old state (A)
  2. We modify the state and generate ops (A ->B)
  3. We take the old state, apply ops, and check if this matches the modified state (A -> C, and then checking B === C)
  4. We return modified state (B)

In future, if devs will not encounter any issues:

  1. We have old state (A)
  2. We will remove modifying the state and only generate ops (A not changed)
  3. Apply ops to old state (A -> C)
  4. Return state modified by ops (C)
689–692 ↗(On Diff #25101)

in this function as threads we return filteredThreads which are threads from the message store whose ids are "watched".

  1. We can either take the diff of reassignedMessageStore.threads and filteredThreads and remove those
  2. Use threadsToRemoveMessagesFrom which is created in a loop that iterates through all reassignedMessageStore.threads (see line 667) and select those which are not present in watchedThreadInfos (current solution and better in terms of perf I think)
tomek added inline comments.
lib/reducers/message-reducer.js
256 ↗(On Diff #25101)

Sounds ok. Overall, sets are beneficial if we want to deduplicate or when we check if the set contains something - none of these are the case here, so an array is fine.

620 ↗(On Diff #25101)

Thanks for the explanation - it makes sense!

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

remove comment, create ops only when it makes sense and rebase