[lib] Remove `lastNavigatedTo/lastPruned` from `MessageStore.threads`
ClosedPublic
Actions

Authored by atul on Oct 12 2023, 9:04 PM.

Details

Reviewers

ashoat
tomek
ginsu
rohan
kamil

Commits

rCOMMf58d8c77991d: [lib] Remove `lastNavigatedTo/lastPruned` from `MessageStore.threads`

Summary

All of the message pruning logic is now dependent on lastNavigatedTo and lastPruned from ThreadActivityStore instead of MessageStore.threads, so instances of MessageStore.threads.[lastNavigatedTo/lastPruned]` can safely be removed.

This diff just handles removal on the "JS side." We will separately handle removal on the C++ side.

NOTE: I originally intended to migrate values from MessageStore.threads to ThreadActivityStore, but decided against it. I will still introduce a migration to remove the extraneous lastNavigatedTo and lastPruned from MessageStore.threads, but figure it makes sense to start with ThreadActivityStore empty and be populated lazily instead of immediately filling with up with entries for every thread.

Depends on D9469

Test Plan

Putting this up now, but need to do more testing before landing so marking this as DRAFT. This should all be "dead code."

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

atul created this revision.Oct 12 2023, 9:04 PM

Herald added a subscriber: will. · View Herald TranscriptOct 12 2023, 9:04 PM

Harbormaster completed remote builds in B23223: Diff 31979.Oct 12 2023, 9:21 PM

atul requested review of this revision.Oct 12 2023, 9:21 PM

atul retitled this revision from [DRAFT][lib] Remove `lastNavigatedTo/lastPruned` from `MessageStore.threads` to [lib] Remove `lastNavigatedTo/lastPruned` from `MessageStore.threads`.Oct 13 2023, 11:54 AM

In a meeting with @kamil today, he indicated that MessageStore.threads was in the process of being moved to SQLite. Have you synced with @kamil to make sure this won't conflict with that work? (Figure it would be a good idea for him to review this diff as well.)

I will still introduce a migration to remove the extraneous lastNavigatedTo and lastPruned from MessageStore.threads

Is there a diff or task tracking that? Does landing this diff need to be blocked on that, or can it be landed as-is?

lib/types/message-types.js
389–390 ↗	(On Diff #31979)	Can these fields be made read-only? We should always aim to improve types when we touch them
lib/utils/message-ops-utils.js
273–274 ↗	(On Diff #31979)	We will separately handle removal on the C++ side. Is there a diff or task tracking that? Does landing this diff need to be blocked on that, or can it be landed as-is?

Looks good but also curious about answers to @ashoat's questions

In D9475#278548, @ashoat wrote:

In a meeting with @kamil today, he indicated that MessageStore.threads was in the process of being moved to SQLite.

It's already in SQLite, the only thing is that we have it in both SQLite and redux-persist to assert correctness. There is a task where we synced with @atul: ENG-3743.

This revision is now accepted and ready to land.Oct 17 2023, 2:16 AM

atul added a child revision: D9517: [CommCoreModule] Remove `last[NavigatedTo/Pruned]` from `MessageStoreThread` and `message_store_threads` table.Oct 17 2023, 1:56 PM

In D9475#278685, @kamil wrote:

Looks good but also curious about answers to @ashoat's questions

In D9475#278548, @ashoat wrote:

In a meeting with @kamil today, he indicated that MessageStore.threads was in the process of being moved to SQLite.

It's already in SQLite, the only thing is that we have it in both SQLite and redux-persist to assert correctness. There is a task where we synced with @atul: ENG-3743.

That work should be able to continue without issue. We'll still proceed w/ moving MessageStore.threads to SQLite, MessageStore.threads will just have lastNavigatedTo and lastPruned fields dropped. I handle clearing the extraneous fields from both Redux + SQLite in this stack.

lib/utils/message-ops-utils.js
273–274 ↗	(On Diff #31979)	It's handled in subsequent diff in this stack. All diffs in this stack will be landed at once.

atul added inline comments.Oct 19 2023, 7:36 AM

lib/types/message-types.js
389–390 ↗	(On Diff #31979)	There's a flow issue here: I'll put a quick followup diff at the end of this stack. As an aside, wonder if we should use https://immerjs.github.io/immer/. Saw it mentioned in Redux docs a bunch of times.