fix: resolved false success message on newsletter subscription #4397
Conversation
Jetshree
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg and
sambhavgupta0705
as code owners
✅ Deploy Preview for asyncapi-website ready!Built without sensitive environment variables
To edit notification comments on pull requests, go to your Netlify project configuration. |
WalkthroughRefactors the newsletter subscription handler to use async/await with try/catch for the POST to Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant C as NewsletterSubscribe (Component)
participant F as Netlify Function\n/.netlify/functions/newsletter_subscription
participant S as FormStatus
U->>C: Submit form (name, email)
rect rgb(235, 245, 255)
note right of C: async handler (try/catch)
C->>F: POST name,email
F-->>C: HTTP Response (status, json)
end
alt status 200
C->>S: set SUCCESS
note right of C: clear name & email
else status != 200
C->>S: set ERROR
end
opt network/runtime error
C->>S: set ERROR
note right of C: error caught in catch
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
🧪 Generate unit tests
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4397 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 778 778
Branches 144 144
=========================================
Hits 778 778 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4397--asyncapi-website.netlify.app/ |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (3)
components/NewsletterSubscribe.tsx (3)
62-66: Reset-on-success is good; avoid timer leaks/races.You schedule a new reset timer every status change but never clear prior ones or on unmount. Keep a ref to the timer, clear before setting a new one, and clean up on unmount.
Example (outside selected lines):
// add imports: useRef, useEffect const resetTimerRef = React.useRef<number | null>(null); const setFormStatus = (formResponse: FormStatus) => { setStatus(formResponse); if (formResponse === FormStatus.SUCCESS) { setName(''); setEmail(''); } if (resetTimerRef.current) window.clearTimeout(resetTimerRef.current); resetTimerRef.current = window.setTimeout(() => setStatus(FormStatus.NORMAL), 10_000); }; React.useEffect(() => () => { if (resetTimerRef.current) window.clearTimeout(resetTimerRef.current); }, []);
72-75: Call preventDefault before setting state.Minor ordering nit to avoid any chance of form submission racing with state updates.
- const handleSubmit = async (event: React.FormEvent<HTMLFormElement>) => { - setStatus(FormStatus.LOADING); - event.preventDefault(); + const handleSubmit = async (event: React.FormEvent<HTMLFormElement>) => { + event.preventDefault(); + setStatus(FormStatus.LOADING);
81-89: Add a fetch timeout to avoid indefinite LOADING if the backend stalls.Use AbortController to fail fast and surface a proper error state.
Example (outside selected lines):
const controller = new AbortController(); const timeoutId = window.setTimeout(() => controller.abort(), 10_000); try { const res = await fetch('/.netlify/functions/newsletter_subscription', { method: 'POST', body: JSON.stringify(data), headers: { 'Content-Type': 'application/json' }, signal: controller.signal, }); // ... } finally { window.clearTimeout(timeoutId); }
| if (res.status === 200) { | ||
| setFormStatus(FormStatus.SUCCESS); | ||
| } else { | ||
| setFormStatus(FormStatus.ERROR); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Treat any 2xx as success (use res.ok).
Hard-coding 200 will mark 201/202/204 as failures, reintroducing “false error” states.
- if (res.status === 200) {
+ if (res.ok) {
setFormStatus(FormStatus.SUCCESS);
} else {
setFormStatus(FormStatus.ERROR);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (res.status === 200) { | |
| setFormStatus(FormStatus.SUCCESS); | |
| } else { | |
| setFormStatus(FormStatus.ERROR); | |
| } | |
| if (res.ok) { | |
| setFormStatus(FormStatus.SUCCESS); | |
| } else { | |
| setFormStatus(FormStatus.ERROR); | |
| } |
🤖 Prompt for AI Agents
In components/NewsletterSubscribe.tsx around lines 90 to 94, the code treats
only status 200 as success which will misclassify other successful 2xx responses
(201/202/204) as errors; update the conditional to use res.ok (or check
res.status >= 200 && res.status < 300) so any 2xx is treated as success, and
keep the existing setFormStatus(FormStatus.SUCCESS) /
setFormStatus(FormStatus.ERROR) calls otherwise.
| await res.json(); | ||
| } catch (error) { | ||
| setFormStatus(FormStatus.ERROR); | ||
| } |
There was a problem hiding this comment.
Remove unused await res.json(); it can flip SUCCESS to ERROR.
If the backend returns 204/no body or non-JSON, this throws and the catch sets ERROR even after a 2xx. Since the body isn’t used, drop the parse or guard it.
- await res.json();Optional guarded read:
if (res.headers.get('content-type')?.includes('application/json')) {
await res.json().catch(() => null);
}🤖 Prompt for AI Agents
In components/NewsletterSubscribe.tsx around lines 96 to 99, remove the
unconditional await res.json() which can throw on 204/no-body or non-JSON
responses and incorrectly trigger the catch branch; either drop the parse
entirely since the body isn't used, or perform a guarded parse by checking
res.headers.get('content-type') for 'application/json' and only then await
res.json().catch(() => null) so JSON parse errors don't flip a successful 2xx
response into FormStatus.ERROR.
2 participants
Description
Fixed newsletter form from showing fake success with console errors to properly displaying "Something went wrong" when it actually fails. The newsletter subscription form has been enhanced to handle network errors more gracefully by converting its submission logic to use async/await, implementing robust try-catch blocks for error handling, and resetting form fields upon successful submission to improve user experience and reliability.
Before:
After:
Related issue(s)
See also #4366
Summary by CodeRabbit
New Features
Bug Fixes
Refactor