feat: migrate Newsletter Service#5292
feat: migrate Newsletter Service#5292princerajpoot20 wants to merge 10 commits intoasyncapi:masterfrom
Conversation
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
📝 WalkthroughWalkthroughThe changes migrate newsletter subscription handling from Mailchimp to a Kit API-based approach. The Mailchimp configuration file and dependencies are removed, the subscription handler is refactored with new validation and timeout logic, and email footer styling is updated. Changes
Sequence DiagramsequenceDiagram
actor Client
participant Handler as newsletter_subscription<br/>handler
participant KitAPI as Kit API
Client->>Handler: POST /subscribe<br/>{email, interest}
activate Handler
Handler->>Handler: Validate JSON,<br/>email, interest
Handler->>Handler: Check KIT_API_KEY<br/>and resolve tagId
Handler->>KitAPI: POST /subscribers<br/>(email)
activate KitAPI
KitAPI-->>Handler: 200 or error
deactivate KitAPI
Handler->>KitAPI: POST /tags/{tagId}/subscribers<br/>(email)
activate KitAPI
KitAPI-->>Handler: 200 or error
deactivate KitAPI
Handler->>Handler: Map response<br/>(timeout, error,<br/>or success)
Handler-->>Client: 200/400/502/503/504/500<br/>+ message
deactivate Handler
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
princerajpoot20
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg and
sambhavgupta0705
as code owners
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-5292--asyncapi-website.netlify.app/ |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #5292 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 830 830
Branches 159 159
=========================================
Hits 830 830 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@config/kit-config.json`:
- Around line 3-5: The hardcoded tag IDs ("Newsletter", "Meetings", "TSC
Voting") in config/kit-config.json should be moved out of source control and
loaded from deploy-specific secrets; replace their static values with lookups to
environment/config variables (e.g., process.env or your secret manager) and
update any code that reads kit-config.json to fall back to those env vars when
present; ensure the KIT_API_KEY remains separate and that deployment configs
(staging/production) provide the correct tag IDs so the code that references the
"Newsletter", "Meetings", and "TSC Voting" keys continues to receive IDs at
runtime without committing them to the repo.
In `@netlify/functions/newsletter_subscription.ts`:
- Around line 54-66: The tagging call (tagRes) can fail after the subscriber was
created upstream, leaving an orphaned active subscriber; update the handler to
perform compensation: after creating the subscriber (capture the subscriber
id/response from the POST /subscribers call), if tagRes.ok is false attempt a
retry of the tagging request a limited number of times and if still failing call
the Kit endpoint to deactivate or delete the newly created subscriber (use the
subscriber id from the create response), log both tagging and compensation
outcomes, and return a clear response to the client indicating partial success
(subscriber created but tag failed) rather than a generic 502; ensure you
reference and use the existing tagRes variable and the create-subscriber
response payload to implement the rollback.
- Around line 32-58: The two open-ended fetch calls that create subRes and
tagRes against KIT_BASE (posting subscribers and tagging via tagId) need request
timeouts: create an AbortController for each request, start a short timer (e.g.,
3–10s) that calls controller.abort(), pass controller.signal into fetch, and
clear the timer when the response arrives; catch the abort (or FetchError) and
return a suitable error response (e.g., 504/timeout message) instead of letting
the Netlify invocation hang. Ensure you still set headers and body as before and
reuse the same pattern for both the subscriber POST (subRes) and the tag POST
(tagRes).
- Around line 15-29: Parse and validate the incoming payload safely before
looking up interest: wrap JSON.parse(event.body) in a try/catch and return 400
for invalid JSON; validate that parsed.email, parsed.name, and parsed.interest
exist and are strings (and that email conforms to a simple email pattern) before
using interest; when mapping to tagId, use a safe property check like
Object.prototype.hasOwnProperty.call(config.tags, interest) (or typeof
config.tags[interest] !== 'undefined') to avoid inherited properties and then
coerce/validate the resulting tagId (reject NaN/0) and return 400 for invalid
client input instead of 503; update the logic around the variables email, name,
interest and tagId in newsletter_subscription.ts accordingly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bbbea95a-15ea-4a30-8fec-b447e988c02f
📒 Files selected for processing (4)
config/kit-config.jsonconfig/mailchimp-config.jsonnetlify/functions/newsletter_subscription.tspackage.json
💤 Files with no reviewable changes (2)
- package.json
- config/mailchimp-config.json
| const tagRes = await fetch(`${KIT_BASE}/tags/${tagId}/subscribers`, { | ||
| method: 'POST', | ||
| headers, | ||
| body: JSON.stringify({ email_address: email }) | ||
| }); | ||
|
|
||
| if (!tagRes.ok) { | ||
| await tagRes.text().catch(() => undefined); | ||
| return { | ||
| statusCode: err.status, | ||
| body: JSON.stringify(err) | ||
| statusCode: 502, | ||
| body: JSON.stringify({ | ||
| message: 'Subscription could not be completed. Please try again later.' | ||
| }) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat netlify/functions/newsletter_subscription.tsRepository: asyncapi/website
Length of output: 2256
Handle the partial-success path when tagging fails.
If POST /subscribers succeeds and POST /tags/{tagId}/subscribers fails, this returns 502 after the upstream state has already changed. The subscriber is now active in Kit but untagged, and the client receives the same error message as if the subscription entirely failed, making retries ambiguous and leaving orphaned subscribers behind. Add compensation logic (e.g., deactivate/delete the subscriber on tagging failure) or implement a retry/reconciliation mechanism to handle this scenario.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@netlify/functions/newsletter_subscription.ts` around lines 54 - 66, The
tagging call (tagRes) can fail after the subscriber was created upstream,
leaving an orphaned active subscriber; update the handler to perform
compensation: after creating the subscriber (capture the subscriber id/response
from the POST /subscribers call), if tagRes.ok is false attempt a retry of the
tagging request a limited number of times and if still failing call the Kit
endpoint to deactivate or delete the newly created subscriber (use the
subscriber id from the create response), log both tagging and compensation
outcomes, and return a clear response to the client indicating partial success
(subscriber created but tag failed) rather than a generic 502; ensure you
reference and use the existing tagRes variable and the create-subscriber
response payload to implement the rollback.
There was a problem hiding this comment.
how kit endpoints works is, even if you already have an account, we call that endpoint with that email. it's more like a put endpoint, where it will create entry if you are new. in case you are already there in our database, it then updates the name as provided by you currently.
so in this case, if the tag updation fails, if we simply go ahead and remove the user, it does not make sense as that user might already be subscribed to different newsletters, and removing that removes those newsletter subscriptions as well. hence, since in case tag updation fails, then we should simply show that request is not processed and not proceed with deletion of that account
There was a problem hiding this comment.
@princerajpoot20, that's a great clarification — thank you. Since Kit's POST /subscribers is effectively an upsert (creates if new, updates if existing), a subscriber who calls the endpoint may already have pre-existing tag subscriptions in Kit. Deleting or deactivating that subscriber as compensation would silently remove those other subscriptions, which is clearly worse than just surfacing an error.
Given this, the current behavior — returning 502 when the tagging step fails, without touching the subscriber record — is the correct and safe approach. The client receives a clear signal that the full subscription flow did not complete, and the subscriber's existing state in Kit is preserved. I'll withdraw the rollback suggestion.
✏️ Learnings added
Learnt from: princerajpoot20
Repo: asyncapi/website PR: 5292
File: netlify/functions/newsletter_subscription.ts:0-0
Timestamp: 2026-04-12T14:05:00.648Z
Learning: In the asyncapi/website repository (PR `#5292`), the Kit API's `POST /subscribers` endpoint is an upsert: it creates a new subscriber if the email doesn't exist, or updates the existing subscriber's details (e.g., first name) if it does. Therefore, rolling back (deleting/deactivating) a subscriber after a tag-assignment failure is incorrect and harmful, as the subscriber may already have pre-existing tag subscriptions that would be destroyed. The correct behavior on tag-assignment failure is to return an error (e.g., 502) without modifying the subscriber record.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: asyncapi-bot
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-02-18T12:07:42.211Z
Learning: The following PR commands are supported in the asyncapi/website repository:
- `/please-take-a-look` or `/ptal`: Requests attention from reviewers who haven't reviewed the PR
- `/ready-to-merge` or `/rtm`: Triggers automerge when all conditions are met
- `/do-not-merge` or `/dnm`: Blocks automerge even if all conditions are met
- `/autoupdate` or `/au`: Adds autoupdate label to keep PR in sync with target branch
- `/update` or `/u`: One-time update of PR with latest changes from target branch
There was a problem hiding this comment.
♻️ Duplicate comments (1)
netlify/functions/newsletter_subscription.ts (1)
100-114:⚠️ Potential issue | 🟠 MajorPartial-success scenario still leaves orphaned subscribers.
If the subscriber POST succeeds (lines 84-88) but the tagging POST fails (lines 100-104), the subscriber remains active in Kit but untagged. The client receives a generic 502 error without knowing subscription partially succeeded.
Consider one of these mitigations:
- Retry tagging before giving up (simple improvement)
- Compensate on failure by deleting/deactivating the subscriber if tagging fails
- Return a distinct status (e.g., 207 Partial) so the client knows state diverged
Given the migration context, option 1 (retry) may be sufficient for now.
♻️ Suggested retry approach
- const tagRes = await kitFetch(`${KIT_BASE}/tags/${tagId}/subscribers`, { - method: 'POST', - headers, - body: JSON.stringify({ email_address: email }) - }); - - if (!tagRes.ok) { - await tagRes.text().catch(() => undefined); - return { - statusCode: 502, - body: JSON.stringify({ - message: 'Subscription could not be completed. Please try again later.' - }) - }; - } + let tagRes = await kitFetch(`${KIT_BASE}/tags/${tagId}/subscribers`, { + method: 'POST', + headers, + body: JSON.stringify({ email_address: email }) + }); + + // Retry tagging once on failure + if (!tagRes.ok) { + await tagRes.text().catch(() => undefined); + tagRes = await kitFetch(`${KIT_BASE}/tags/${tagId}/subscribers`, { + method: 'POST', + headers, + body: JSON.stringify({ email_address: email }) + }); + } + + if (!tagRes.ok) { + await tagRes.text().catch(() => undefined); + // Subscriber exists but is untagged - still return success + // since the core subscription succeeded + return { + statusCode: 200, + body: JSON.stringify({ message: 'Subscribed successfully.' }) + }; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netlify/functions/newsletter_subscription.ts` around lines 100 - 114, The tag POST (tagRes) can fail after the initial subscriber POST, leaving orphaned subscribers; implement a retry loop when calling kitFetch(`${KIT_BASE}/tags/${tagId}/subscribers`) — e.g., 3 attempts with small exponential backoff — and only return an error after retries exhaust; if tagging still fails, perform compensation by deleting/deactivating the newly created subscriber (use the subscriber ID from the successful subscriber response, e.g., subscriberRes/subscriberJson.id) before returning an error, and ensure errors and final status distinguish a partial failure (or return 207) so the client can know the divergent state.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@netlify/functions/newsletter_subscription.ts`:
- Around line 100-114: The tag POST (tagRes) can fail after the initial
subscriber POST, leaving orphaned subscribers; implement a retry loop when
calling kitFetch(`${KIT_BASE}/tags/${tagId}/subscribers`) — e.g., 3 attempts
with small exponential backoff — and only return an error after retries exhaust;
if tagging still fails, perform compensation by deleting/deactivating the newly
created subscriber (use the subscriber ID from the successful subscriber
response, e.g., subscriberRes/subscriberJson.id) before returning an error, and
ensure errors and final status distinguish a partial failure (or return 207) so
the client can know the divergent state.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: c9e50be1-ddd0-48ee-ad17-3897ae34118f
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (2)
netlify/functions/newsletter_subscription.tspackage.json
💤 Files with no reviewable changes (1)
- package.json
|
@asyncapi/bounty_team |
fmvilas
left a comment
fmvilas
left a comment
There was a problem hiding this comment.
LGTM 👍 Just left a comment as a suggestion, not a blocker.
| "Newsletter": 18208674, | ||
| "Meetings": 18208677, | ||
| "TSC Voting": 18208683 |
There was a problem hiding this comment.
Perhaps these values could be environment variables too so we can change them without the need of a PR.
|
There was a problem hiding this comment.
🧹 Nitpick comments (2)
netlify/functions/newsletter_subscription.ts (2)
68-75: Optional: validate email format, not just type.
typeof email !== 'string'passes values like"foo"or""straight through to Kit, which then returns a non-OK response and the user sees a generic502 "Subscription could not be completed"instead of a clear400. A lightweight regex check (or reusing whatever validationNewsletterSubscribe.tsxapplies client-side) would give better UX and shed obviously-bad traffic before hitting Kit.- if (typeof email !== 'string' || typeof interest !== 'string' || !isValidInterest(interest)) { + const emailPattern = /^[^\s@]+@[^\s@]+\.[^\s@]+$/; + if ( + typeof email !== 'string' || + !emailPattern.test(email) || + typeof interest !== 'string' || + !isValidInterest(interest) + ) { return { statusCode: 400, body: JSON.stringify({ message: 'Invalid subscription request.' }) }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netlify/functions/newsletter_subscription.ts` around lines 68 - 75, The current validation only checks that email is a string; update the request validation in the newsletter subscription handler to also validate email format (e.g., add a lightweight regex or reuse the client-side validator from NewsletterSubscribe.tsx) before proceeding; modify the block that inspects the destructured email, name, interest (and the isValidInterest(interest) check) to reject malformed emails by returning the same 400 JSON response (message: 'Invalid subscription request.') so obviously-bad inputs are rejected early.
108-153: Failures are silently swallowed — add minimal logging for observability.Both Kit failure paths read the error body and discard it (
await subRes.text().catch(() => undefined)/await tagRes.text().catch(() => undefined)), and the outercatchnever logserr. In production this means operators cannot distinguish between a 4xx misconfiguration from Kit (e.g., badKIT_API_KEY, wrong tag ID), a 5xx upstream outage, or a timeout — all are indistinguishable502/500responses to the client with no trace in Netlify function logs.Since you are already consuming the body, capture it and emit a
console.error(orconsole.warn) with status + a truncated body. Netlify aggregates function logs, so this is essentially free.🪵 Proposed fix
if (!subRes.ok) { - await subRes.text().catch(() => undefined); + const detail = await subRes.text().catch(() => ''); + console.error('[newsletter] Kit subscribers POST failed', { + status: subRes.status, + body: detail.slice(0, 500) + }); return { statusCode: 502, body: JSON.stringify({ message: 'Subscription could not be completed. Please try again later.' }) }; } @@ if (!tagRes.ok) { - await tagRes.text().catch(() => undefined); + const detail = await tagRes.text().catch(() => ''); + console.error('[newsletter] Kit tag assignment failed', { + status: tagRes.status, + tagId, + body: detail.slice(0, 500) + }); return { statusCode: 502, body: JSON.stringify({ message: 'Subscription could not be completed. Please try again later.' }) }; } @@ } catch (err) { if (isAbortError(err)) { + console.error('[newsletter] Kit request timed out after', REQUEST_TIMEOUT_MS, 'ms'); return { statusCode: 504, body: JSON.stringify({ message: 'Subscription service timed out. Please try again later.' }) }; } + console.error('[newsletter] Unexpected error', err); return { statusCode: 500,Note: avoid logging
nameto stay clear of PII retention concerns — status code and truncated upstream body are sufficient for triage.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@netlify/functions/newsletter_subscription.ts` around lines 108 - 153, The Kit error bodies are being consumed and discarded and the outer catch never logs the thrown error; update the failure paths around subRes and tagRes so they capture the response text (e.g., const subBody = await subRes.text().catch(() => undefined)) and emit a concise console.error including subRes.status and a truncated subBody (avoid PII), and do the same for tagRes; also log the caught err in the catch block (but redact sensitive fields) so isAbortError handling remains unchanged but unexpected errors are recorded for observability.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@netlify/functions/newsletter_subscription.ts`:
- Around line 68-75: The current validation only checks that email is a string;
update the request validation in the newsletter subscription handler to also
validate email format (e.g., add a lightweight regex or reuse the client-side
validator from NewsletterSubscribe.tsx) before proceeding; modify the block that
inspects the destructured email, name, interest (and the
isValidInterest(interest) check) to reject malformed emails by returning the
same 400 JSON response (message: 'Invalid subscription request.') so
obviously-bad inputs are rejected early.
- Around line 108-153: The Kit error bodies are being consumed and discarded and
the outer catch never logs the thrown error; update the failure paths around
subRes and tagRes so they capture the response text (e.g., const subBody = await
subRes.text().catch(() => undefined)) and emit a concise console.error including
subRes.status and a truncated subBody (avoid PII), and do the same for tagRes;
also log the caught err in the catch block (but redact sensitive fields) so
isAbortError handling remains unchanged but unexpected errors are recorded for
observability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 2076f12f-2db8-4b54-bf18-9d778f8f358b
📒 Files selected for processing (1)
netlify/functions/newsletter_subscription.ts
4 participants
Resloves #5127 asyncapi/community#2223 Related: asyncapi/community#2223
Related PRs: asyncapi/community#3505 asyncapi/.github#396
Things need to be done later
Newsletter,Meetings,TSC Votingand update the value in the config file. (As of now, the tag value are of my personal account)KIT_API_KEYTesting Evidence
https://asyncapi.princerajpoot.com/en/newsletter
Summary by CodeRabbit
Release Notes
Refactor
Style
Chores