Page MenuHomePhabricator

[lib] modify set aux user fids and replace old behavior with add aux user fids action type
ClosedPublic

Authored by will on May 20 2024, 1:29 AM.
Tags
None
Referenced Files
F3387216: D12105.id40802.diff
Fri, Nov 29, 8:17 AM
Unknown Object (File)
Thu, Nov 28, 12:49 PM
Unknown Object (File)
Mon, Nov 25, 5:10 AM
Unknown Object (File)
Sun, Nov 24, 8:43 AM
Unknown Object (File)
Sun, Nov 24, 4:46 AM
Unknown Object (File)
Sun, Nov 24, 12:41 AM
Unknown Object (File)
Thu, Nov 14, 12:31 PM
Unknown Object (File)
Fri, Nov 8, 3:18 AM
Subscribers

Details

Summary

Following feedback on D12011, decided to refactor the aux user store actions.

In D12011, we want a single action type that both clears prior fids and adds payload fids.

As setAuxUserStoreFIDs has yet to be used anywhere in our codebase, I have the action type first clear fids and add payload fids.

The previous functionality of just adding fids becomes addAuxUserFIDs.

Depends on D12095

Test Plan

added aux user reducer tests

Diff Detail

Repository
rCOMM Comm
Lint
Lint Not Applicable
Unit
Tests Not Applicable

Event Timeline

will requested review of this revision.May 20 2024, 1:45 AM
lib/reducers/aux-user-reducer.js
29–54 ↗(On Diff #40350)

Is it possible for us to only have one op per userID?

lib/reducers/aux-user-reducer.js
29–54 ↗(On Diff #40350)

For sqlite operations, we typically have a pattern of a single op per single replace.

I did introduce some grouping logic in https://phab.comm.dev/D11298 that would group together any successive replace operations paired with a replace op in sqlite that took multiple entries, but it's not standard in our codebase.

If we did want to have a single op do this, we'd need to introduce a new sqlite operation.

I was curious on implementing this if there'd be a noticeable performance decline as we're doing this for our entire user store (for anyone with a non-null fid).

If you think we should make the necessary sqlite additions/modifications, I can create a follow up task

lib/reducers/aux-user-reducer.js
29–54 ↗(On Diff #40350)

To clarify, we could also just remove the previous replace operation and make all actions use the multiple entry replace operation, so not technically a full new sqlite op but a rewrite of a portion of it

lib/reducers/aux-user-reducer.js
29–54 ↗(On Diff #40350)

@ashoat I realize I read your question wrong.

I think we can definitely do one op per userID. We'd first update with the new farcaster fids in a first loop.

In the second loop, we only clear if the userID wasn't in action.payload.farcasterUsers

I think we can set the farcaster userIDs into a set so that we can easily lookup if it doesn't require an fid clear in the second loop

will planned changes to this revision.May 22 2024, 7:18 AM

Planning changes to address review feedback

ashoat added inline comments.
lib/reducers/aux-user-reducer.js
28–30 ↗(On Diff #40572)

Let's use a Set for performance

35 ↗(On Diff #40572)

Once it's a Set we can use Set.has, which is O(1) instead of O(n)

This revision is now accepted and ready to land.May 24 2024, 3:40 AM

use set for constant time lookups