Page MenuHomePhabricator

[lib] Add `ThreadActivityStore` to Redux
ClosedPublic

Authored by atul on Sep 21 2023, 1:45 PM.
Tags
None
Referenced Files
Unknown Object (File)
Sat, Oct 19, 2:52 AM
Unknown Object (File)
Mon, Oct 14, 2:03 PM
Unknown Object (File)
Sun, Oct 13, 2:56 AM
Unknown Object (File)
Sat, Oct 12, 10:11 PM
Unknown Object (File)
Sat, Oct 12, 10:06 PM
Unknown Object (File)
Sat, Oct 12, 3:59 AM
Unknown Object (File)
Fri, Oct 11, 8:25 PM
Unknown Object (File)
Fri, Oct 11, 1:57 AM
Subscribers

Details

Summary

As described on Linear (https://linear.app/comm/issue/ENG-4866/improve-performance-of-re-rendering-chat-list-and-thread-list#comment-e78e87ae), moving lastNavigatedTo and lastPruned out of MessageStore.threads will lead to performance improvements. It also generally decouples the actual MessageStore.threads "content" from information tracked/persisted solely for the purpose of pruning.

Here's a rough plan for the stack:

  1. Introduce ThreadActivityStore
  2. Got through every scenario where lastNavigatedTo or lastPruned are updated and ensure we're updating ThreadActivityStore in the same way.
  3. Modify ThreadPruner to pull from ThreadActivityStore instead of MessageStore.messages (Set to a couple seconds instead of 6 hours to "force" it)
  4. Deprecate MessageStore.threads[threadID].[lastNavigatedTo/lastPruned] and introduce a Redux migration that maps previous vaues in MessageStore.threads to ThreadActivityStore.
Test Plan

Used Redux dev tools to ensure that threadActivityStore was as expected. Ensured that it updated along with MessageStore.threads on updateThreadLastNavigatedActionType:

3f708c.png (530×1 px, 71 KB)

Diff Detail

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

Event Timeline

atul added inline comments.
native/redux/redux-setup.js
221 ↗(On Diff #31357)

Moved this to message-reducer so we can avoid early-return for now.

atul published this revision for review.Sep 21 2023, 1:58 PM
atul edited the test plan for this revision. (Show Details)
ashoat requested changes to this revision.Sep 21 2023, 2:14 PM

In the past, we would need to update websites-responders.js when adding a new field to Redux state. Now that Redux state for the web is fetched via an API call, is there a different place we should be updating?

lib/reducers/thread-activity-reducer.js
7 ↗(On Diff #31357)

It's best to keep action type constants in "leaf nodes" of the import graph, since they're used in some many places. Can you move this to some "types file" to reduce the risk of an import cycle?

23 ↗(On Diff #31357)

We need to handle all of the standard action types here for session invalidation / log in / log out / delete account / etc.

lib/types/thread-activity-types.js
4–5 ↗(On Diff #31357)

read-only

native/redux/redux-setup.js
221 ↗(On Diff #31357)

There appear to be changes introduced, such as moving from processDBStoreOperations to processMessageStoreOperations, and sourcing the updated message store from that function call. Can you explain these changes please?

This revision now requires changes to proceed.Sep 21 2023, 2:14 PM

In the past, we would need to update websites-responders.js when adding a new field to Redux state. Now that Redux state for the web is fetched via an API call, is there a different place we should be updating?

Based on my reading of https://phab.comm.dev/D9145, the "new place" is web/redux/default-state.js which I did update in this diff.

atul marked an inline comment as done.

fix

atul marked 2 inline comments as done.Sep 21 2023, 2:37 PM
atul added inline comments.
lib/reducers/thread-activity-reducer.js
7 ↗(On Diff #31357)

Done

23 ↗(On Diff #31357)

Thanks, added those. Will also add deleting/leaving thread.

lib/types/thread-activity-types.js
4–5 ↗(On Diff #31357)

Ah thanks for catching that, cut/paste from ThreadMessageInfo. Was going to update both types to include the + but didn't stage that in diff

native/redux/redux-setup.js
221 ↗(On Diff #31357)

Yeah the "proper" way to handle updates to MessageStore is by applying the operations constructed in message-reducer.

In this case, we're updating Redux "manually" and then applying the changes to SQLite via processDBStoreOperations before immediately returning from reducer.

Inside message-reducer, we construct the ops, apply to Redux via processMessageStoreOperations and then return messageStoreOperations which will then be processed by processDBStoreOperations.

This approach is better because "the same ops are running through" Redux and SQLite. The point of the ops is to be the universal interface for updating MessageStore across persistence "implementations"

atul marked 3 inline comments as done.Sep 21 2023, 2:37 PM
ashoat added a subscriber: michal.
In D9262#272510, @atul wrote:

In the past, we would need to update websites-responders.js when adding a new field to Redux state. Now that Redux state for the web is fetched via an API call, is there a different place we should be updating?

Based on my reading of https://phab.comm.dev/D9145, the "new place" is web/redux/default-state.js which I did update in this diff.

There's also InitialReduxState, which I think represents what's returned by each individual keyserver to a freshly loaded web app. I guess nothing needs to be returned by the keyserver for this, though? cc @michal

This revision is now accepted and ready to land.Sep 21 2023, 7:01 PM

Yeah, you are correct. Basically if the value you would add previously put in website-responders is:

  • some default value than it should go to web/redux/default-state
  • value calculated on the keyserver than it should go in getInitialReduxStateResponder (+ some dummy default in default-state so flow doesn't complain)
This revision was landed with ongoing or failed builds.Sep 25 2023, 1:10 PM
This revision was automatically updated to reflect the committed changes.