Details

Reviewers

ashoat
ginsu
tomek
rohan
kamil

Commits

rCOMM246557e54623: [lib] Add `ThreadActivityStore` to Redux

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:

Introduce ThreadActivityStore
Got through every scenario where lastNavigatedTo or lastPruned are updated and ensure we're updating ThreadActivityStore in the same way.
Modify ThreadPruner to pull from ThreadActivityStore instead of MessageStore.messages (Set to a couple seconds instead of 6 hours to "force" it)
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:

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Sep 21 2023, 1:45 PM

atul added a reviewer: kamil.Sep 21 2023, 1:52 PM

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)

Harbormaster completed remote builds in B22791: Diff 31357.Sep 21 2023, 2:05 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 D9262#272502, @ashoat 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.

address feedback

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

Harbormaster completed remote builds in B22792: Diff 31358.Sep 21 2023, 2:49 PM

Harbormaster completed remote builds in B22793: Diff 31359.

In D9262#272510, @atul wrote:

In D9262#272502, @ashoat 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)

rebase and land

This revision was landed with ongoing or failed builds.Sep 25 2023, 1:10 PM

Closed by commit rCOMM246557e54623: [lib] Add `ThreadActivityStore` to Redux (authored by atul). · Explain Why

This revision was automatically updated to reflect the committed changes.

atul added a commit: rCOMM246557e54623: [lib] Add `ThreadActivityStore` to Redux.

Harbormaster completed remote builds in B22840: Diff 31414.Sep 25 2023, 1:16 PM

[lib] Add `ThreadActivityStore` to Redux
ClosedPublic
Actions

Details

Diff Detail

Event Timeline

Revision Contents
Changeset List

Diff 31415

lib/reducers/master-reducer.js

lib/reducers/message-reducer.js

lib/reducers/thread-activity-reducer.js

lib/reducers/thread-activity-reducer.test.js

lib/types/redux-types.js

lib/types/thread-activity-types.js

native/navigation/thread-screen-tracker.react.js

native/redux/action-types.js

native/redux/default-state.js

native/redux/redux-setup.js

native/redux/state-types.js

web/redux/default-state.js

web/redux/redux-setup.js

[lib] Add `ThreadActivityStore` to ReduxClosedPublicActions

Details

Diff Detail

Event Timeline

Revision ContentsChangeset List

Diff 31415

lib/reducers/master-reducer.js

lib/reducers/message-reducer.js

lib/reducers/thread-activity-reducer.js

lib/reducers/thread-activity-reducer.test.js

lib/types/redux-types.js

lib/types/thread-activity-types.js

native/navigation/thread-screen-tracker.react.js

native/redux/action-types.js

native/redux/default-state.js

native/redux/redux-setup.js

native/redux/state-types.js

web/redux/default-state.js

web/redux/redux-setup.js

[lib] Add `ThreadActivityStore` to Redux
ClosedPublic
Actions

Revision Contents
Changeset List