[#489] Set up Farcaster notification system#498
Conversation
- Add notification_tokens Supabase migration (fid, token, url, enabled) - Add notifications.server.ts with token management + sendNotification (batches up to 100, groups by URL, cleans invalid tokens) - Add webhook route for Farcaster miniapp events (added/removed/enabled/disabled) with parseWebhookEvent + Neynar signature verification - Add save-token API for client-side belt-and-suspenders token save - Update FarcasterMiniApp to save notification details from addMiniApp() result and from existing context for returning users - Add webhookUrl to farcaster.json manifest - Add notifyNewPlot() trigger for new plot published notifications Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
project7-interns
left a comment
There was a problem hiding this comment.
LGTM. Full notification pipeline: clean migration (FID PK, partial index on enabled), server lib with batching/URL grouping/invalid token cleanup, webhook with Neynar signature verification, client-side belt-and-suspenders save, and notifyNewPlot() trigger. Matches Dropcast pattern.
Security note (non-blocking): /api/notifications/save-token is unauthenticated — an attacker could overwrite a user's notification token by POSTing an arbitrary FID. Since the webhook is the primary verified path and will correct tokens on subsequent events, the window is narrow. Consider adding authentication (e.g. verify Farcaster context or rate-limit by IP) in a follow-up.
Minor: The webhook falls back to no-verification when NEYNAR_API_KEY is unset — the console.warn is appropriate, but ensure the key is set before going live.
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: REQUEST CHANGES
Summary
The notification pipeline covers a lot of ground, but it currently ships with a fail-open verification path and still does not wire any notification trigger into live PlotLink flows.
Findings
- high
src/app/api/webhook/notifications/route.ts:22falls back tovalid: trueparsing whenNEYNAR_API_KEYis unset, which means anyone who can hit the webhook can forgeminiapp_added/notifications_enabledevents and write arbitrary notification tokens into the database. This needs to fail closed, not silently disable signature verification.- File:
src/app/api/webhook/notifications/route.ts:22 - Suggestion: reject webhook requests when
NEYNAR_API_KEYis missing, or otherwise require real signature verification in all environments that can receive external traffic.
- File:
- medium
lib/notifications.server.ts:164definesnotifyNewPlot(), but nothing in the PR actually calls it. The PR description even lists wiring it into the backfill cron as an operator follow-up, so #489's acceptance criterion of "at least one notification trigger implemented" is not met yet.- File:
lib/notifications.server.ts:164 - Suggestion: invoke
notifyNewPlot()from the actual new-plot indexing/publish path in this PR, so there is one end-to-end trigger implemented rather than just a helper left unused.
- File:
Decision
Request changes because the webhook currently accepts unsigned events when misconfigured and the PR does not yet implement a live notification trigger.
…to backfill - Webhook now returns 503 when NEYNAR_API_KEY is missing instead of accepting unverified events - notifyNewPlot() called from backfill cron after StorylineCreated (genesis) and PlotChained events are indexed Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
project7-interns
left a comment
There was a problem hiding this comment.
Verdict: APPROVE
Summary
The follow-up revision closes the two blocking gaps: the webhook now fails closed when signature verification cannot run, and the new-plot notification helper is wired into the live backfill/indexing flow.
Findings
- None.
Decision
Approve because the previously requested security and trigger-implementation fixes are now in place, so #489 is complete from code review.
Summary
00021_notification_tokens.sql):notification_tokenstable with FID primary key, token, URL, enabled flaglib/notifications.server.ts): Token CRUD +sendNotification()with batching (100/request), URL grouping, invalid token cleanup. IncludesnotifyNewPlot()trigger./api/webhook/notifications): Handlesminiapp_added,miniapp_removed,notifications_enabled,notifications_disabledevents with@farcaster/miniapp-nodesignature verification via Neynar/api/notifications/save-token): Client-side belt-and-suspenders token save fromaddMiniApp()resultaddMiniApp()result and from existing context for returning userswebhookUrltofarcaster.jsonFixes #489
Operator items
00021_notification_tokens.sqlon SupabaseNEYNAR_API_KEYin Vercel env vars for webhook signature verificationnotifyNewPlot()into the backfill cron when new plots are indexedTest plan
miniapp_added/notifications_enabledminiapp_removed/notifications_disablednotifyNewPlot()sends notification to all enabled users🤖 Generated with Claude Code