Details

Reviewers

ashoat
rohan

Commits

rCOMM26d61f565ef6: [lib] Handle `pinnedCount` in `convertClientDBThreadInfoToRawThreadInfo`

Summary

Even after persist.js migration 36 it looked like the threadInfos stored on the client were missing the pinnedCount field.

After taking a look at CommCoreModule it seemed like everything was good. However, noticed that convertClientDBThreadInfoToRawThreadInfo was missing pinnedCount entirely. This means that RawThreadInfos we get from SQLite will ALWAYS have pinnedCount stripped out even if it is properly persisted in SQLite.

Test Plan

Ongoing... as a start will check SET_CLIENT_DB_STORE action in Redux Dev Tools and see if ThreadInfos include pinnedCount field.

Diff Detail

Repository

rCOMM Comm

Branch

master

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

atul created this revision.Apr 24 2023, 11:14 AM

Herald added a subscriber: tomek. · View Herald TranscriptApr 24 2023, 11:14 AM

atul published this revision for review.Apr 24 2023, 11:19 AM

I don't think this is correct. You'll exclude pinnedCount if it's 0, whereas the keyserver always includes it, which will mean that there will be inconsistency

Is there a reason we shouldn't always just include pinnedCount?

This revision now requires changes to proceed.Apr 24 2023, 11:22 AM

atul added inline comments.Apr 24 2023, 11:23 AM

lib/utils/thread-ops-utils.js
57–62 ↗	(On Diff #25612)	Intentionally left this out of the `let rawThreadInfo` block even though we expect going forward this to be `number` where it's 0 if "unset." We use `convertClientDBThreadInfoToRawThreadInfo` in previous migrations where we DON'T yet have `pinnedCount`... it's probably fine to have an extraneous `pinnedCount` field in those migrations, but figure it's safer to include this check. Open to changing

check if not null and not undefined instead of truthiness

I don't think this is correct. You'll exclude pinnedCount if it's 0

Thanks for catching that, was hasty in putting this up. Will update.

Is there a reason we shouldn't always just include pinnedCount?

This was my reasoning:

Intentionally left this out of the let rawThreadInfo block even though we expect going forward this to be number where it's 0 if "unset."

We use convertClientDBThreadInfoToRawThreadInfo in previous migrations where we DON'T yet have pinnedCount... it's probably fine to have an extraneous pinnedCount field in those migrations, but figure it's safer to include this check.

but you're right we can probably just always include. Will update this diff again.

just include

Harbormaster completed remote builds in B18701: Diff 25612.Apr 24 2023, 11:32 AM

Harbormaster completed remote builds in B18703: Diff 25614.Apr 24 2023, 11:42 AM

Harbormaster completed remote builds in B18704: Diff 25615.Apr 24 2023, 11:48 AM

ashoat accepted this revision.Apr 24 2023, 12:28 PM

This revision is now accepted and ready to land.Apr 24 2023, 12:28 PM

Thanks for catching this!

rebase + land

This revision was landed with ongoing or failed builds.Apr 25 2023, 10:35 AM

Closed by commit rCOMM26d61f565ef6: [lib] Handle `pinnedCount` in `convertClientDBThreadInfoToRawThreadInfo` (authored by atul). · Explain Why

This revision was automatically updated to reflect the committed changes.