Details

Reviewers

ashoat
varun

Commits

rCOMM6f4b8cee79bc: [keyserver] reply on farcaster with invite link

Summary

Putting this diff for review. Currently debugging our invalid signerUUID. This diff replies on farcaster with the invite link

Depends on D13784

Test Plan

Tested with my local iOS simulator comm app. I grabbed the invite link posted to warpcast and pasted it to my safari. Was able to successfully join new communities and/or sidebars

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

will created this revision.Oct 23 2024, 11:48 PM

Herald added a subscriber: tomek. · View Herald TranscriptOct 23 2024, 11:48 PM

will published this revision for review.Oct 23 2024, 11:51 PM

will added inline comments.

keyserver/src/responders/farcaster-webhook-responders.js
262 ↗	(On Diff #45368)	Definitely wanted to get an opinion on what we might change this to

Harbormaster completed remote builds in B32363: Diff 45368.Oct 24 2024, 12:05 AM

tomek added inline comments.Oct 24 2024, 4:19 AM

keyserver/src/responders/farcaster-webhook-responders.js
262 ↗	(On Diff #45368)	We shouldn't hardcode the link - instead, we should use `inviteLinkURL` function.

Planning changes based on Tomek's feedback

will added inline comments.Oct 24 2024, 9:48 AM

keyserver/src/responders/farcaster-webhook-responders.js
266 ↗	(On Diff #45368)	Following discussion at https://linear.app/comm/issue/ENG-8460/create-and-reply-on-farcaster-with-sidebar-invite-link#comment-8582a37a, we'll reply to the tagging cast always

will added inline comments.Oct 24 2024, 1:17 PM

keyserver/src/responders/farcaster-webhook-responders.js
262 ↗	(On Diff #45368)	Starting a linear conversation on what this might look like: https://linear.app/comm/issue/ENG-8460/create-and-reply-on-farcaster-with-sidebar-invite-link#comment-85796917

review feedback and changes

will edited the test plan for this revision. (Show Details)Wed, Oct 30, 1:24 PM

will added inline comments.

keyserver/src/responders/farcaster-webhook-responders.js
288–290	When with option 2 in https://linear.app/comm/issue/ENG-8460/create-and-reply-on-farcaster-with-sidebar-invite-link#comment-85796917
292–294	Curious if I should move the neynarConfig higher in the code and exit early if missing or let the community + sidebar creation code run through

Harbormaster completed remote builds in B32455: Diff 45488.Wed, Oct 30, 2:18 PM

ashoat requested changes to this revision.Wed, Oct 30, 2:56 PM

ashoat added inline comments.

keyserver/src/responders/farcaster-webhook-responders.js
286	Why do we wait until this point to start this promise?
288–290	Please keep all lines to a max of 80 chars
292–294	I'd probably move it higher
296	Typo If the condition on line 292 passes, doesn't that mean we have a `neynarClient`?
297	Doesn't the condition on line 292 mean we have a `signerUUID`?
302	Do we need the question mark?

This revision now requires changes to proceed.Wed, Oct 30, 2:56 PM

feedback

will added inline comments.Wed, Oct 30, 7:02 PM

keyserver/src/responders/farcaster-webhook-responders.js
286	Addressed by moving up higher as a promise
292–294	Ended up moving it higher as a promise but kept the `missing_signer_uuid` check at the same spot as I don't think we're expecting this to happen
297	This is correct. Removing `?`
302	Looks like we do. Flow complains if I remove it and says its type is `void \| NeynarPostCastResponse`

will added inline comments.Wed, Oct 30, 7:05 PM

keyserver/src/responders/farcaster-webhook-responders.js
296	Confused. 292 is for `neynarConfig` right? I found several examples in the codebase of using it as `neynarClient?.method`

ashoat accepted this revision.Wed, Oct 30, 7:11 PM

ashoat added inline comments.

keyserver/src/responders/farcaster-webhook-responders.js
284–293 ↗	(On Diff #45496)	Nit: probably best to group these two `awaits` into a single `Promise.all`. The fewer `await`s at the top level the better
296		Oops yeah I confused the two