feat:API robustness enhancement#3670#309
Conversation
Signed-off-by: developharsh <harsh237hk@gmail.com>
|
@amaan-bhati @Achanandhi-M , please review the PR and acknowledge about the changes needed, thank you : ) |
|
@amaan-bhati , @Achanandhi-M please have are view over this |
|
Hey @devlopharsh 👋 — thanks so much for contributing to the project, really appreciate it! Your PR looks great and has been marked for merging. Here's a quick note from the reviewer:
We'll get this merged in soon. Keep the great work coming! 🚀 |
There was a problem hiding this comment.
Pull request overview
Improves robustness of the WordPress WPGraphQL client (fetchAPI) by adding request timeouts and more defensive handling of non-ideal HTTP/network responses.
Changes:
- Add configurable request timeout using
AbortControllerwithWORDPRESS_API_TIMEOUT_MSfallback. - Improve error surfacing for network/timeout failures, non-2xx HTTP responses, and invalid JSON.
- Tighten header typing and add a guard for missing API endpoint configuration.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| { variables, timeoutMs = DEFAULT_REQUEST_TIMEOUT_MS }: Record<string, any> = {} | ||
| ) { | ||
| if (!API_URL) { | ||
| throw new Error("WORDPRESS_API_URL is not configured"); |
There was a problem hiding this comment.
The error message here only mentions WORDPRESS_API_URL, but API_URL is also sourced from NEXT_PUBLIC_WORDPRESS_API_URL. Consider updating the message to reference both env vars and include a clear next step (e.g., set one of them to the WPGraphQL endpoint).
| throw new Error("WORDPRESS_API_URL is not configured"); | |
| throw new Error( | |
| "Neither WORDPRESS_API_URL nor NEXT_PUBLIC_WORDPRESS_API_URL is configured. Set one of them to your WordPress WPGraphQL endpoint." | |
| ); |
| process.env.WORDPRESS_API_TIMEOUT_MS || "", | ||
| 10 | ||
| ); | ||
| // can be overidden by setting the env , else the default will be used |
There was a problem hiding this comment.
Typo/grammar in this comment: "overidden" -> "overridden" (also consider removing extra spaces around punctuation for readability).
| // can be overidden by setting the env , else the default will be used | |
| // Can be overridden by setting the env; otherwise, the default will be used. |
| async function fetchAPI( | ||
| query = "", | ||
| { variables, timeoutMs = DEFAULT_REQUEST_TIMEOUT_MS }: Record<string, any> = {} | ||
| ) { |
There was a problem hiding this comment.
PR description mentions retry/backoff for 5xx/429 and a structured error type, but this implementation currently only adds timeout + richer Error messages and does not retry or expose a typed/structured error. Either implement those behaviors here or adjust the PR description to match the actual change set.
| const contentType = res.headers.get("content-type") || ""; | ||
| const isJson = contentType.includes("application/json"); | ||
| const rawBody = isJson ? null : await res.text(); | ||
|
|
||
| let json: any = null; | ||
| if (isJson) { | ||
| try { | ||
| json = await res.json(); | ||
| } catch (err: any) { | ||
| throw new Error(`Invalid JSON response (status ${res.status}): ${err?.message || "parse failed"}`); | ||
| } | ||
| } |
There was a problem hiding this comment.
If the server responds with a non-JSON Content-Type but a 2xx status, this function will currently return undefined (since json stays null) and callers will fail later with less context. Consider explicitly throwing when !isJson (or when json is null) for successful responses, including a short body snippet for diagnosis.
| async function fetchAPI( | ||
| query = "", | ||
| { variables, timeoutMs = DEFAULT_REQUEST_TIMEOUT_MS }: Record<string, any> = {} | ||
| ) { | ||
| if (!API_URL) { | ||
| throw new Error("WORDPRESS_API_URL is not configured"); | ||
| } | ||
|
|
||
| const headers: Record<string, string> = { "Content-Type": "application/json" }; | ||
|
|
||
| if (process.env.WORDPRESS_AUTH_REFRESH_TOKEN) { | ||
| headers[ | ||
| "Authorization" | ||
| ] = `Bearer ${process.env.WORDPRESS_AUTH_REFRESH_TOKEN}`; | ||
| } | ||
| // WPGraphQL Plugin must be enabled | ||
| const res = await fetch(API_URL, { | ||
| headers, | ||
| method: "POST", | ||
| body: JSON.stringify({ | ||
| query, | ||
| variables, | ||
| }), | ||
| }); | ||
|
|
||
| const json = await res.json(); | ||
| if (json.errors) { | ||
| const controller = new AbortController(); | ||
| const timeoutId = setTimeout(() => controller.abort(), timeoutMs); | ||
|
|
There was a problem hiding this comment.
timeoutMs is accepted as any and can be overridden by callers; if it’s non-finite/<=0, setTimeout may behave unexpectedly (immediate abort / no timeout). Consider validating/coercing timeoutMs to a finite positive integer before using it, and tightening the options type to prevent accidental misuse.
| if (json?.errors) { | ||
| console.error(json.errors); | ||
| throw new Error("Failed to fetch API"); | ||
| throw new Error(`GraphQL errors: ${JSON.stringify(json.errors).slice(0, 500)}`); | ||
| } |
There was a problem hiding this comment.
This path logs full json.errors to stderr and then throws an error containing (a truncated) JSON string. That can lead to duplicate logging and may leak verbose upstream details into logs. Consider removing the console.error, or truncating/sanitizing what gets logged/thrown (and relying on the thrown error for diagnostics).
| async function fetchAPI( | ||
| query = "", | ||
| { variables, timeoutMs = DEFAULT_REQUEST_TIMEOUT_MS }: Record<string, any> = {} | ||
| ) { |
There was a problem hiding this comment.
PR checklist/description indicates corresponding tests were added, but this PR only changes lib/api.ts and the repo currently has no test script in package.json. If tests exist elsewhere, consider adding/including them here; otherwise, please update the PR testing/checklist statements to reflect what was actually done.
Related Tickets & Documents
Fixes: #3670
Description
Issue : fetchAPI does not defensively handle common HTTP and network failure scenarios. As a result, failures propagate in unclear or uncontrolled ways, making them difficult to diagnose and potentially harmful to production stability.
Changes
Type of Change
Testing
Environment and Dependencies
Checklist