fix: improve error logging to serialize Response objects#1115
fix: improve error logging to serialize Response objects#1115
Conversation
console.error was logging raw Response objects, producing "(undefined) [object Response]" in observability tools like HyperDX. Now logs status, URL and response body for actionable debugging.
Tagging OptionsShould a new tag be published when this PR is merged?
|
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
1 issue found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="utils/fetchAPI.ts">
<violation number="1" location="utils/fetchAPI.ts:19">
P2: Log the request URL explicitly for `Request` inputs; interpolating `input` directly still serializes as `[object Request]`.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| console.error(input, response); | ||
| const errorBody = await response.text(); | ||
| console.error( | ||
| `fetchAPI error ${response.status} ${response.statusText} - ${input}`, |
There was a problem hiding this comment.
P2: Log the request URL explicitly for Request inputs; interpolating input directly still serializes as [object Request].
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At utils/fetchAPI.ts, line 19:
<comment>Log the request URL explicitly for `Request` inputs; interpolating `input` directly still serializes as `[object Request]`.</comment>
<file context>
@@ -14,6 +14,10 @@ export const fetchAPI = async <T>(
- console.error(input, response);
+ const errorBody = await response.text();
+ console.error(
+ `fetchAPI error ${response.status} ${response.statusText} - ${input}`,
+ errorBody,
+ );
</file context>
| `fetchAPI error ${response.status} ${response.statusText} - ${input}`, | |
| `fetchAPI error ${response.status} ${response.statusText} - ${typeof input === "string" ? input : input.url}`, | |
📝 WalkthroughWalkthroughThe PR enhances error handling and logging in two API utility files. Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
utils/fetchAPI.ts (1)
17-20: NormalizeRequestinputs before logging.
fetchAPIacceptsRequest, but${input}still logs as[object Request]. That keeps this path opaque in observability even after fixing theResponsecase. Useinput.url(and ideallyinput.method) wheninputis aRequest.Proposed change
const errorBody = await response.text(); + const requestLabel = typeof input === "string" + ? input + : `${input.method} ${input.url}`; console.error( - `fetchAPI error ${response.status} ${response.statusText} - ${input}`, + `fetchAPI error ${response.status} ${response.statusText} - ${requestLabel}`, errorBody, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@utils/fetchAPI.ts` around lines 17 - 20, The fetchAPI error logging currently prints Request objects as "[object Request]" making failures opaque; in the fetchAPI function, detect when the input is a Request (e.g., using input instanceof Request or checking for input.url) and log input.url and input.method instead of interpolating the whole object; for non-Request inputs fall back to the existing string/URL logging (or input.toString()) so the console.error line uses either `${input.url} ${input.method}` when a Request or the original input value otherwise, and keep the rest of the errorBody/response.status/statusText logging unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@clients/withManifest.ts`:
- Around line 167-172: The content-type check and JSON parsing need to be made
robust: change the header check to detect JSON media types with parameters
(e.g., use response.headers.get("content-type")?.includes("application/json") or
startsWith) and wrap JSON.parse(errorBody) in a try/catch; if parse succeeds use
the parsed object as the InvokeError cause, otherwise fall back to using the raw
errorBody (or undefined) as the cause so you still throw the normalized
InvokeError with the correlationId. Ensure you update the branch around
response, errorBody, InvokeError, and correlationId to always throw an
InvokeError (with correlationId) even when JSON parsing fails.
---
Nitpick comments:
In `@utils/fetchAPI.ts`:
- Around line 17-20: The fetchAPI error logging currently prints Request objects
as "[object Request]" making failures opaque; in the fetchAPI function, detect
when the input is a Request (e.g., using input instanceof Request or checking
for input.url) and log input.url and input.method instead of interpolating the
whole object; for non-Request inputs fall back to the existing string/URL
logging (or input.toString()) so the console.error line uses either
`${input.url} ${input.method}` when a Request or the original input value
otherwise, and keep the rest of the errorBody/response.status/statusText logging
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: dce82650-bcbe-47fb-b998-d2b8e724700e
📒 Files selected for processing (2)
clients/withManifest.tsutils/fetchAPI.ts
| if (response.headers.get("content-type") === "application/json") { | ||
| const errorObj = JSON.parse(error); | ||
| const errorObj = JSON.parse(errorBody); | ||
| throw new InvokeError(`${response.status}: ${response.statusText}`, { | ||
| cause: errorObj, | ||
| correlationId: response.headers.get("x-correlation-id"), | ||
| correlationId, | ||
| }); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate the file and examine the specific lines mentioned
git ls-files | grep -E "withManifest\.(ts|js)$"Repository: deco-cx/deco
Length of output: 79
🏁 Script executed:
# Read the file around the specified lines
wc -l clients/withManifest.tsRepository: deco-cx/deco
Length of output: 83
🏁 Script executed:
# Get context around lines 167-172
sed -n '155,180p' clients/withManifest.tsRepository: deco-cx/deco
Length of output: 815
🏁 Script executed:
# Search for other JSON.parse calls in the codebase to see error handling patterns
rg 'JSON\.parse' -B 2 -A 2 --type ts --type jsRepository: deco-cx/deco
Length of output: 13161
🏁 Script executed:
# Check the InvokeError definition and how correlationId is used
rg 'class InvokeError|interface InvokeError' -A 10 --type tsRepository: deco-cx/deco
Length of output: 627
🏁 Script executed:
# Check if there's any error handling around JSON parsing elsewhere
rg 'try.*JSON\.parse|JSON\.parse.*catch' --type ts --type jsRepository: deco-cx/deco
Length of output: 774
Guard JSON error parsing and accept charset-suffixed JSON.
The current branch only matches an exact application/json header, which misses common variants like application/json; charset=utf-8. Additionally, JSON.parse(errorBody) has no guard, so malformed or empty JSON bodies throw a raw SyntaxError, causing callers to lose the normalized InvokeError path and its correlationId for observability.
Proposed fix
+ const contentType = response.headers.get("content-type") ?? "";
- if (response.headers.get("content-type") === "application/json") {
- const errorObj = JSON.parse(errorBody);
+ if (contentType.startsWith("application/json")) {
+ let errorObj: unknown = errorBody;
+ try {
+ errorObj = errorBody ? JSON.parse(errorBody) : null;
+ } catch {
+ // Fall back to the raw body and preserve correlationId on InvokeError.
+ }
throw new InvokeError(`${response.status}: ${response.statusText}`, {
cause: errorObj,
correlationId,
});
}📝 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 (response.headers.get("content-type") === "application/json") { | |
| const errorObj = JSON.parse(error); | |
| const errorObj = JSON.parse(errorBody); | |
| throw new InvokeError(`${response.status}: ${response.statusText}`, { | |
| cause: errorObj, | |
| correlationId: response.headers.get("x-correlation-id"), | |
| correlationId, | |
| }); | |
| const contentType = response.headers.get("content-type") ?? ""; | |
| if (contentType.startsWith("application/json")) { | |
| let errorObj: unknown = errorBody; | |
| try { | |
| errorObj = errorBody ? JSON.parse(errorBody) : null; | |
| } catch { | |
| // Fall back to the raw body and preserve correlationId on InvokeError. | |
| } | |
| throw new InvokeError(`${response.status}: ${response.statusText}`, { | |
| cause: errorObj, | |
| correlationId, | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@clients/withManifest.ts` around lines 167 - 172, The content-type check and
JSON parsing need to be made robust: change the header check to detect JSON
media types with parameters (e.g., use
response.headers.get("content-type")?.includes("application/json") or
startsWith) and wrap JSON.parse(errorBody) in a try/catch; if parse succeeds use
the parsed object as the InvokeError cause, otherwise fall back to using the raw
errorBody (or undefined) as the cause so you still throw the normalized
InvokeError with the correlationId. Ensure you update the branch around
response, errorBody, InvokeError, and correlationId to always throw an
InvokeError (with correlationId) even when JSON parsing fails.
1 participant
Summary
console.error(init?.body, response)inclients/withManifest.tsandconsole.error(input, response)inutils/fetchAPI.tswhich logged(undefined) [object Response]in observability tools (HyperDX)Invoke error 500 Internal Server Error - /live/invoke/... {"message":"..."}Test plan
undefined [object Response], fix producesInvoke error 500 Internal Server Error - /live/invoke/... {body}Summary by cubic
Improves error logging by serializing fetch Response objects. Logs status, URL, and body for clearer debugging in tools like HyperDX.
console.error(..., response)with formatted messages inclients/withManifest.tsandutils/fetchAPI.ts.x-correlation-idand pass it toInvokeError.Written for commit e0e9a41. Summary will update on new commits.
Summary by CodeRabbit