Details

Reviewers

ashoat
varun

Commits

rCOMMd2f70e0fe779: [keyserver] add regex #createathread check

Summary

The regex in the Neynar dev portal is OR only, so we need to perform the regex expression check ourselves when we receive webhook events.

Test Plan

Tested on warpcast and verifying through console logging that we early exited when the proper farcaster user was tagged but didn't include #createathread in the text body.
Checked that including #createathread in several parts of the message and lowercase/uppercase did not cause the early exit

Depends on D13754

Diff Detail

Repository

rCOMM Comm

Lint

No Lint Coverage

Unit

No Test Coverage

Event Timeline

will created this revision.Oct 31 2024, 7:14 AM

Herald added a subscriber: tomek. · View Herald TranscriptOct 31 2024, 7:14 AM

will published this revision for review.Oct 31 2024, 7:15 AM

will added a child revision: D13755: [keyserver] moving getting neynar config to function.

will mentioned this in D13755: [keyserver] moving getting neynar config to function.

Do we really need a RegExp if we're just checking if the string contains another string?

Maybe the best argument for RegExp would be if we're trying to match on word boundaries.

In D13841#386565, @ashoat wrote:

Do we really need a RegExp if we're just checking if the string contains another string?

Maybe the best argument for RegExp would be if we're trying to match on word boundaries.

I used it for the case insensitivity option, but we could also just use .includes and undercase the cast text? Don't remember if we wanted case insensitivity with #createathread but I think it might've been mentioned at some point?

Thinking about this. I think we should likely go with a non-regex solution as this is such a simple check. Going to change in next rebase

use .includes instead of regex

Harbormaster completed remote builds in B32485: Diff 45518.Oct 31 2024, 9:37 AM

use regex with word boundries

word boundry fix

Harbormaster completed remote builds in B32490: Diff 45523.Oct 31 2024, 10:11 AM

Harbormaster completed remote builds in B32491: Diff 45525.Oct 31 2024, 10:27 AM

ashoat accepted this revision.Oct 31 2024, 10:44 AM

ashoat added inline comments.

keyserver/src/responders/farcaster-webhook-responders.js
18 ↗	(On Diff #45525)	Can we move this to the top-level scope to avoid redefining it on every invocation? If you do that, you'll probably need to add `g` here so the RegExp can be reused

This revision is now accepted and ready to land.Oct 31 2024, 10:44 AM

will added inline comments.Oct 31 2024, 11:29 AM

keyserver/src/responders/farcaster-webhook-responders.js
18 ↗	(On Diff #45525)	Discussed offline with @ashoat. Feedback for 1. is still valid. For 2. we actually don't want the g flag as we don't want it to be stateful