-
Notifications
You must be signed in to change notification settings - Fork 554
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Social proof #2557
Social proof #2557
Conversation
cc99a1f
to
569c40a
Compare
* Update deactivation through updateSubjectStatus (#2539) * attach hosting status to entryway session responses * integrate account deactivation in with entryway * tidy * tidy * chnageset * update changeset * Version packages (#2546) Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> * lexicon: initial social proof lexicons * feat: add generated types for social proof * feat: social proof implementation --------- Co-authored-by: Daniel Holmgren <dtholmgren@gmail.com> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: Devin Ivy <devinivy@gmail.com>
1f4712c
to
6f68cf7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lookin good! 💪
left a couple notes - big one is to tidy up the protos file
packages/bsky/src/views/index.ts
Outdated
const followers = mapDefined(knownFollowers.followers, (did) => { | ||
return this.profileBasic(did, state) | ||
}) | ||
return { count: knownFollowers.count, followers } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe that there is a minimum of one item for followers
. It would be nice to have some logic enforcing that colocated around here, or perhaps drop the requirement from the lexicon if you think the frontend wont mind.
We may also want to consider enforcing blocks, and possibly mutes, in this list.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We updated this the other day to 0
. Yeah I'll make sure frontend can handle it, no issue there.
Yeah should def filter out blocks, I think mutes are OK to leave in though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ya know, hadn't codegenned though, glad you brought this up.
Blocks are now filtered, added a test
const knownFollowers = carolForAlice.data.viewer?.knownFollowers | ||
expect(knownFollowers?.count).toBe(1) | ||
expect(knownFollowers?.followers).toHaveLength(0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just noting here count
is 1, but the blocked user has been filtered out. We return 5 to the client, and the client uses only 3, so we're already overfetching. Discussed with Devin and decided this was a rare enough case to leave in for now. Frontend will handle the difference.
Adds
knownFollowers
toViewerState
onProfileDetailed
views. This includes:getProfiles
getProfile
getSuggestedFollowsByActor
getSuggestions
Also adds
getKnownFollowers
method that returns the full list of known followers for a givenactor
. This is set up for pagination, but is not actually paginated atm due to dataplane constraints.