fix: add proper error handling to fetch calls #4687
Conversation
sarthakNITT
requested review from
Mayaleeeee,
akshatnema,
anshgoyalevil,
asyncapi-bot-eve,
derberg,
princerajpoot20 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. |
WalkthroughTwo React components switch from promise chains to async/await and add try/catch around fetch, checking response status and handling response.json() with explicit success/error state updates. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes
Possibly related issues
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ 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 |
|
⚡️ Lighthouse report for the changes in this PR:
Lighthouse ran on https://deploy-preview-4687--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 (2)
components/Feedback.tsx (1)
39-57: Async/await refactor is correct; considerresponse.okand dropping unused JSON parsingThe new async/await + try/catch flow is functionally sound and correctly surfaces network failures via
setError(true). To tighten it up:
- Prefer
response.okinstead ofresponse.status === 200so that any 2xx success is treated as success.- Since the JSON body is not used,
await response.json()is unnecessary and may incorrectly flip a successful request into an error if the body is empty or non‑JSON.You could simplify to:
try { const response = await fetch('/.netlify/functions/github_discussions', { method: 'POST', body: JSON.stringify(data), headers: { 'Content-Type': 'application/json' } }); - if (response.status === 200) { - setSubmitted(true); - } else { - setError(true); - } - - await response.json(); + if (!response.ok) { + setError(true); + return; + } + + setSubmitted(true); } catch (err) { setError(true); }This keeps the intended behavior while avoiding a potential false error state from JSON parsing. As per coding guidelines, this is a good use of try/catch since it directly controls graceful UI degradation.
components/NewsletterSubscribe.tsx (1)
67-94: Async submit logic looks good; refine status check and JSON handlingThe conversion to
async+try/catchcorrectly drives the form status and handles network failures by settingFormStatus.ERROR. A couple of small improvements:
- Use
res.okinstead ofres.status === 200to treat all 2xx responses as success:if (!res.ok) { setFormStatus(FormStatus.ERROR); return; } setFormStatus(FormStatus.SUCCESS);- Since the response body is not used,
await res.json()is redundant and may incorrectly flip a success into an error if the body is empty or invalid JSON. You can safely remove it.These tweaks keep the UX the same in the happy path while making the handler more robust.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/Feedback.tsx(1 hunks)components/NewsletterSubscribe.tsx(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
Learnt from: JeelRajodiya
Repo: asyncapi/website PR: 3423
File: scripts/index.ts:29-31
Timestamp: 2025-03-16T15:55:44.257Z
Learning: In the AsyncAPI codebase, adding try-catch blocks around function calls is unnecessary when the only purpose is to add context to the error message, as errors will be thrown from the functions themselves with full stack traces that provide adequate context. Try-catch blocks should only be added when there's a need for graceful degradation, partial success behavior, or specific error handling logic.
Learnt from: anshgoyalevil
Repo: asyncapi/website PR: 3557
File: scripts/markdown/check-editlinks.js:58-59
Timestamp: 2025-01-08T15:15:00.759Z
Learning: In the AsyncAPI codebase, batch processing operations (like in the Dashboard script and check-editlinks.js) follow a sequential pattern using await in loops, which is the preferred approach for maintaining consistency across the codebase.
Learnt from: sagarkori143
Repo: asyncapi/website PR: 4192
File: npm/runners/build-pages-runner.ts:7-14
Timestamp: 2025-06-19T13:51:27.459Z
Learning: In the AsyncAPI website build system, individual runner functions use try-catch blocks that rethrow errors to maintain consistent error boundaries, while the top-level orchestrator in npm/index.ts provides meaningful error logging with task-specific context using logger.error(`Error building ${taskName}:`, error). This separation of concerns allows for clean error handling at different architectural layers.
Learnt from: sagarkori143
Repo: asyncapi/website PR: 4192
File: npm/runners/build-newsroom-videos-runner.ts:8-15
Timestamp: 2025-06-19T13:49:29.796Z
Learning: In the AsyncAPI website modularization project, error handling is centralized in the top-level orchestrator function (npm/index.ts) with comprehensive logging and context. Individual runner functions in npm/runners/ are kept simple and let errors propagate naturally to the centralized handler, avoiding redundant try-catch blocks that only rethrow errors.
Learnt from: sagarkori143
Repo: asyncapi/website PR: 0
File: :0-0
Timestamp: 2025-06-20T14:47:22.389Z
Learning: The AsyncAPI website modularization PR uses Promise.allSettled for parallel execution of build tasks, which provides better performance and fault isolation compared to sequential execution.
🪛 GitHub Actions: PR testing - if Node project
components/Feedback.tsx
[error] 47-47: prettier/prettier: Delete spaces before line break or trailing spaces.
[error] 47-47: no-trailing-spaces: Trailing spaces not allowed.
[error] 53-53: prettier/prettier: Delete spaces before line break or trailing spaces.
[error] 53-53: no-trailing-spaces: Trailing spaces not allowed.
components/NewsletterSubscribe.tsx
[error] 84-84: prettier/prettier: Delete spaces before line break or trailing spaces.
[error] 84-84: no-trailing-spaces: Trailing spaces not allowed.
[error] 90-90: prettier/prettier: Delete spaces before line break or trailing spaces.
[error] 90-90: no-trailing-spaces: Trailing spaces not allowed.
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: cypress-run
- GitHub Check: Lighthouse CI
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #4687 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 22 22
Lines 799 799
Branches 146 146
=========================================
Hits 799 799 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
components/NewsletterSubscribe.tsx (1)
67-94: Drop unusedres.json()(or gate it) to avoid unnecessary failuresThe async/await + try/catch rewrite looks good and correctly flips the form status on HTTP and network errors. One minor improvement:
await res.json();is called unconditionally, but the parsed body is never used.- If the backend ever returns non‑JSON (even with
200), this will throw and push the form into theERRORstate via thecatch, even though the subscription might have succeeded.Since you don’t currently consume the response body, you can safely remove the parse:
- if (res.status === 200) { - setFormStatus(FormStatus.SUCCESS); - } else { - setFormStatus(FormStatus.ERROR); - } - - await res.json(); + if (res.status === 200) { + setFormStatus(FormStatus.SUCCESS); + } else { + setFormStatus(FormStatus.ERROR); + }If you later need the payload, consider only parsing it on
res.ok(orstatus === 200) and optionally guarding the parse with its own try/catch so you can handle “successful but malformed JSON” separately from network failures.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/Feedback.tsx(1 hunks)components/NewsletterSubscribe.tsx(2 hunks)
⏰ Context from checks skipped due to timeout of 180000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: cypress-run
| try { | ||
| const response = await fetch('/.netlify/functions/github_discussions', { | ||
| method: 'POST', | ||
| body: JSON.stringify(data), | ||
| headers: { | ||
| 'Content-Type': 'application/json' | ||
| } | ||
| }); | ||
|
|
||
| if (response.status === 200) { | ||
| setSubmitted(true); | ||
| } | ||
| if (response.status !== 200) { | ||
| } else { | ||
| setError(true); | ||
| } | ||
| response.json(); | ||
| }); | ||
| await response.json(); | ||
| } catch (err) { | ||
| setError(true); | ||
| } |
There was a problem hiding this comment.
Align success/error state with JSON parsing and remove unused response.json()
The new async/await + try/catch structure is a good improvement, but there’s a subtle edge case:
- For a
200response, you callsetSubmitted(true)and thenawait response.json(). - If
response.json()throws (e.g., invalid JSON body), thecatchruns andsetError(true)is called. - Final state is
submitted === trueanderror === true, and because the render checksif (submitted)beforeif (error), the user sees the success UI even though an error occurred.
Since the parsed body is not used, this extra await response.json() only introduces this failure mode without adding value.
Consider simplifying to something like:
- if (response.status === 200) {
- setSubmitted(true);
- } else {
- setError(true);
- }
- await response.json();
+ if (response.status === 200) {
+ setSubmitted(true);
+ } else {
+ setError(true);
+ }Or, if you later need the body, only parse it when required and handle parse failures in a way that keeps submitted/error consistent with the UI you want.
📝 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.
| try { | |
| const response = await fetch('/.netlify/functions/github_discussions', { | |
| method: 'POST', | |
| body: JSON.stringify(data), | |
| headers: { | |
| 'Content-Type': 'application/json' | |
| } | |
| }); | |
| if (response.status === 200) { | |
| setSubmitted(true); | |
| } | |
| if (response.status !== 200) { | |
| } else { | |
| setError(true); | |
| } | |
| response.json(); | |
| }); | |
| await response.json(); | |
| } catch (err) { | |
| setError(true); | |
| } | |
| try { | |
| const response = await fetch('/.netlify/functions/github_discussions', { | |
| method: 'POST', | |
| body: JSON.stringify(data), | |
| headers: { | |
| 'Content-Type': 'application/json' | |
| } | |
| }); | |
| if (response.status === 200) { | |
| setSubmitted(true); | |
| } else { | |
| setError(true); | |
| } | |
| } catch (err) { | |
| setError(true); | |
| } |
🤖 Prompt for AI Agents
In components/Feedback.tsx around lines 39 to 56, the code sets submitted=true
before awaiting response.json(), so if JSON parsing fails the catch flips
error=true and leaves submitted=true (showing success and error); remove the
unused await response.json() (or if you need the body, await and parse it before
calling setSubmitted, catching parse errors and setting setError(true) instead
of setSubmitted) so submitted and error remain mutually consistent.
2 participants
Description
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.