Page MenuHomePhabricator

[lib] Handle replies count
ClosedPublic

Authored by tomek on Jul 29 2024, 8:43 AM.
Tags
None
Referenced Files
Unknown Object (File)
Sun, Oct 6, 7:59 PM
Unknown Object (File)
Sun, Oct 6, 7:59 PM
Unknown Object (File)
Sun, Oct 6, 7:59 PM
Unknown Object (File)
Sun, Oct 6, 7:59 PM
Unknown Object (File)
Sun, Oct 6, 7:59 PM
Unknown Object (File)
Sun, Oct 6, 7:28 PM
Unknown Object (File)
Thu, Sep 26, 1:13 AM
Unknown Object (File)
Mon, Sep 23, 3:57 AM
Subscribers

Details

Summary

Check if a message should be counted in replies count and generate appropriate updates.

https://linear.app/comm/issue/ENG-8892/handle-replies-count-as-a-part-of-dmoperationspecs

Depends on D12881

Test Plan

Just the Flow. Further testing will be performed after it is connected with the rest of the app.

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

tomek requested review of this revision.Jul 29 2024, 9:04 AM

Can't we handle this in useProcessDMOperation just like canBeProcessed?

This is a little messy. Curious about @inka's suggestion. If there's some specific information needed from processDMOperation, I'd consider returning that explicitly, and handling it from the caller – it would improve readability, and simplify the logic in the specs

Agree that it would be a lot better to extract the logic from the specs. I'm going to think about it more but at this point, it seems hard because:

  1. Updating the replies count happens through an update. If we extract the logic from the specs, we would need to inspect returned updates and modify or create a new one with the replies count update.
  2. Messages can be returned both in rawMessageInfos and as a part of some updateInfos.

There are some possible ideas, e.g.

  1. We can consider returning something different than rawMessageInfos and updateInfos from the specs, so that it's easier to know if the count should be updated.
  2. We can start updating the count using a separate action and return a new count next to rawMessageInfos and updateInfos.
  3. We can modify reducers to compute the replies count based on new messages automatically.

Each of these could be complicated, and it seems they mostly move complexity from one place to another.

Discussed something similar for updating unread status here.

  1. Updating the replies count happens through an update. If we extract the logic from the specs, we would need to inspect returned updates and modify or create a new one with the replies count update.

This is true, but we could implement a utility that handles it. I don't think it would be that complicated.

  1. Messages can be returned both in rawMessageInfos and as a part of some updateInfos.

Perhaps mergeUpdatesWithMessageInfos could be helpful here?

Each of these could be complicated, and it seems they mostly move complexity from one place to another.

Ultimately my goal would be to unify the complexity in one place, so we can mitigate the risk of the programmer forgetting to handle it in a specific case, and so we could reduce code duplication.

Discussed something similar for updating unread status here.

  1. Updating the replies count happens through an update. If we extract the logic from the specs, we would need to inspect returned updates and modify or create a new one with the replies count update.

This is true, but we could implement a utility that handles it. I don't think it would be that complicated.

  1. Messages can be returned both in rawMessageInfos and as a part of some updateInfos.

Perhaps mergeUpdatesWithMessageInfos could be helpful here?

Each of these could be complicated, and it seems they mostly move complexity from one place to another.

Ultimately my goal would be to unify the complexity in one place, so we can mitigate the risk of the programmer forgetting to handle it in a specific case, and so we could reduce code duplication.

Makes sense! Created https://linear.app/comm/issue/ENG-8930/unify-the-approach-to-replies-count-and-unread-status-updates to track.

This revision is now accepted and ready to land.Aug 2 2024, 7:04 AM
This revision was automatically updated to reflect the committed changes.