Details

Reviewers

ashoat
varun

Commits

rCOMM65ca88236580: [keyserver] create message and sidebar based on cast

Summary

This creates a message based on a cast and converts it to a sidebar. If there is a parent cast included in the webhook event, we use the parent cast, if not, we use the tagger's cast.

Depends D13815

Example sidebar creation with commbot

Test Plan

Triggered the creation of a new community with a webhook POST request and confirmed sidebar was correctly created in the community

Diff Detail

Repository

rCOMM Comm

Lint

Lint Not Applicable

Unit

Tests Not Applicable

Event Timeline

will created this revision.Oct 23 2024, 10:38 PM

Herald added a subscriber: tomek. · View Herald TranscriptOct 23 2024, 10:38 PM

will added a child revision: D13783: [keyserver] create invite link based on newly created sidebar.Oct 23 2024, 10:42 PM

will added a parent revision: D13775: [keyserver] add community creation logic to farcaster webhook responder.

Harbormaster completed remote builds in B32359: Diff 45364.Oct 23 2024, 10:55 PM

will requested review of this revision.Oct 23 2024, 10:55 PM

will added inline comments.Oct 24 2024, 6:18 AM

keyserver/src/responders/farcaster-webhook-responders.js
246 ↗	(On Diff #45364)	Placeholder for now

Can you a share a screenshot of an example message/sidebar that is created this way?

keyserver/src/responders/farcaster-webhook-responders.js
44–47 ↗	(On Diff #45364)	Can we move this logic to the caller, and avoid passing both params in here? Can you add a code comment explaining the logic here? Seems like it would cleaner like this (see code change). Note that this is slightly different in the case where `parentHash` is an empty string (I think `??` only considers `null` / `undefined`)... not sure if this matters here, but if it does you could do `parentHash ? parentHash : castHash` instead
57 ↗	(On Diff #45364)	Not sure the newline makes sense here
58 ↗	(On Diff #45364)	Can we put quotes around the cast text? Might want to consider "escaping" quotes inside of the text, although not sure how important that is
76 ↗	(On Diff #45364)	Is the condition necessary here? Why'd you choose to diverge from the approach we use for sidebar titles in the rest of the codebase?

This revision now requires changes to proceed.Oct 24 2024, 6:20 AM

will removed a parent revision: D13775: [keyserver] add community creation logic to farcaster webhook responder.Oct 29 2024, 4:35 PM

will added a parent revision: D13812: [keyserver] Add dontCheckJoinPermissions override option to createThread for sidebar.

will edited the summary of this revision. (Show Details)Oct 29 2024, 11:27 PM

will edited parent revisions, added: D13815: [keyserver] check if viewer role is positive for updateThread; removed: D13812: [keyserver] Add dontCheckJoinPermissions override option to createThread for sidebar.

will edited the summary of this revision. (Show Details)Oct 30 2024, 12:15 AM

will edited parent revisions, added: D13816: [keyserver] add dontAddViewerAsMember to createThread; removed: D13815: [keyserver] check if viewer role is positive for updateThread.

will removed a parent revision: D13816: [keyserver] add dontAddViewerAsMember to createThread.Wed, Oct 30, 10:11 AM

will edited the summary of this revision. (Show Details)Wed, Oct 30, 11:12 AM

will added a parent revision: D13815: [keyserver] check if viewer role is positive for updateThread.

feedback and rebase

keyserver/src/responders/farcaster-webhook-responders.js
44–47 ↗	(On Diff #45364)	Moved logic to the caller by passing a single `sidebarCastHash` as an arg Added this code comment to the caller like so: `// we use the parent cast to create the sidebar source message if it exists` Used `const sidebarCastHash = parentHash ?? castHash;`. parentHash should only be a full non-empty hash or null
57 ↗	(On Diff #45364)	Included in latest rebase
58 ↗	(On Diff #45364)	Included in latest rebase
76 ↗	(On Diff #45364)	condition was not necessary didn't find this approach. Added usage of trimText in latest rebase

accidentally left a console.log

keyserver/src/responders/farcaster-webhook-responders.js
82 ↗	(On Diff #45486)	Following previous feedback, we're now going to create the message with the viewer

will edited the summary of this revision. (Show Details)Wed, Oct 30, 1:22 PM

Harbormaster completed remote builds in B32453: Diff 45486.Wed, Oct 30, 2:03 PM

Harbormaster completed remote builds in B32454: Diff 45487.Wed, Oct 30, 2:10 PM

ashoat accepted this revision.Wed, Oct 30, 2:39 PM

ashoat added inline comments.

keyserver/src/responders/farcaster-webhook-responders.js
92 ↗	(On Diff #45487)	Instead of copy-pasting this from the place I linked, please factor it out. Copy-paste is almost never appropriate... please consider DRY principles going forward

This revision is now accepted and ready to land.Wed, Oct 30, 2:39 PM

will mentioned this in D13775: [keyserver] add community creation logic to farcaster webhook responder.Wed, Oct 30, 6:11 PM

rebase

Harbormaster completed remote builds in B32462: Diff 45495.Wed, Oct 30, 6:54 PM

remove trimText code duplication with sidebar thread name function

will added inline comments.Thu, Oct 31, 6:10 AM

keyserver/src/responders/farcaster-webhook-responders.js
92 ↗	(On Diff #45487)	Created a function for this called `createSidebarThreadName` which performs the trimText in the latest rebase. I thought about where to put it but decided that `lib/shared/sidebar-utils.js` was an appropriate place to keep it.