Page MenuHomePhabricator
Differential D13785

[keyserver] reply on farcaster with invite link
ClosedPublic
Actions

Authored by will on Oct 23 2024, 11:48 PM.
Tags
None
Referenced Files
F3366176: D13785.id45507.diff
Mon, Nov 25, 9:59 AM
F3366133: D13785.id45496.diff
Mon, Nov 25, 9:42 AM
F3366126: D13785.id45368.diff
Mon, Nov 25, 9:40 AM
F3366073: D13785.id45546.diff
Mon, Nov 25, 9:25 AM
F3365943: D13785.id45368.diff
Mon, Nov 25, 8:35 AM
F3365764: D13785.id45488.diff
Mon, Nov 25, 7:37 AM
Unknown Object (File)
Sat, Nov 23, 6:40 AM
Unknown Object (File)
Sat, Nov 23, 6:04 AM
View All 56 Files
Subscribers
tomek

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
Branch
farcaster_bot
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

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

We shouldn't hardcode the link - instead, we should use inviteLinkURL function.

will planned changes to this revision.Oct 24 2024, 6:23 AM

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

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

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

will updated this revision to Diff 45488.Wed, Oct 30, 1:23 PM

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 ↗(On Diff #45488)

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 ↗(On Diff #45488)

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 ↗(On Diff #45488)

Why do we wait until this point to start this promise?

288–290 ↗(On Diff #45488)

Please keep all lines to a max of 80 chars

292–294 ↗(On Diff #45488)

I'd probably move it higher

296 ↗(On Diff #45488)
  1. Typo
  2. If the condition on line 292 passes, doesn't that mean we have a neynarClient?
297 ↗(On Diff #45488)

Doesn't the condition on line 292 mean we have a signerUUID?

302 ↗(On Diff #45488)

Do we need the question mark?

This revision now requires changes to proceed.Wed, Oct 30, 2:56 PM
will updated this revision to Diff 45496.Wed, Oct 30, 7:01 PM

feedback

will added inline comments.Wed, Oct 30, 7:02 PM
keyserver/src/responders/farcaster-webhook-responders.js
286 ↗(On Diff #45488)

Addressed by moving up higher as a promise

292–294 ↗(On Diff #45488)

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 ↗(On Diff #45488)

This is correct. Removing ?

302 ↗(On Diff #45488)

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 ↗(On Diff #45488)

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 awaits at the top level the better

296 ↗(On Diff #45488)

Oops yeah I confused the two

This revision is now accepted and ready to land.Wed, Oct 30, 7:11 PM
Harbormaster completed remote builds in B32463: Diff 45496.Wed, Oct 30, 7:20 PM
will updated this revision to Diff 45504.Thu, Oct 31, 6:00 AM

address nit

will marked an inline comment as done.Thu, Oct 31, 6:00 AM
Harbormaster completed remote builds in B32471: Diff 45504.Thu, Oct 31, 6:17 AM
will updated this revision to Diff 45507.Thu, Oct 31, 7:19 AM

rebase

Harbormaster completed remote builds in B32474: Diff 45507.Thu, Oct 31, 7:41 AM
will updated this revision to Diff 45546.Thu, Oct 31, 11:58 AM

rebase before landing

Harbormaster completed remote builds in B32502: Diff 45546.Thu, Oct 31, 12:15 PM
Closed by commit rCOMM6f4b8cee79bc: [keyserver] reply on farcaster with invite link (authored by will). · Explain WhyThu, Oct 31, 12:19 PM
This revision was automatically updated to reflect the committed changes.
will marked an inline comment as not done.
will added a commit: rCOMM6f4b8cee79bc: [keyserver] reply on farcaster with invite link.

Revision Contents
Changeset List

PathSize
keyserver/
src/
responders/
23 lines

Diff 45368

View Options

keyserver/src/responders/farcaster-webhook-responders.js

Loading...