Fix axios error parsing #104
Conversation
|
Warning Rate limit exceeded@narekhovhannisyan has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 8 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
WalkthroughCentralizes and refactors Axios error handling into helper functions that extract and format error messages from varied response shapes, updates handleSendingError to use the new builder, and adjusts/extends unit tests to validate the new message formats and edge cases. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Client
participant Axios as AxiosRequest
participant Logger as handleSendingError
participant Builder as buildErrorMessage
participant Extractor as extractErrorMessage
participant Formatter as formatErrorMessage
participant MailtrapError as MailtrapError
Client->>Axios: perform request
Axios-->>Client: throws AxiosError (response.data)
Client->>Logger: handleSendingError(error)
Logger->>Builder: buildErrorMessage(error)
Builder->>Extractor: extractErrorMessage(response.data, default)
note right of Extractor `#f0f4ff`: Inspect `data.error` / `data.errors` / nested shapes
alt errors is array of objects
Extractor->>Formatter: extractMessagesFromErrorObjects(...)
Formatter-->>Extractor: concatenated message
else errors is field map
Extractor->>Formatter: extractFieldErrors(...)
Formatter-->>Extractor: joined field messages
else plain string / unknown
Extractor-->>Builder: return default or `error.message`
end
Extractor-->>Builder: composed message
Builder-->>Logger: final message
Logger->>MailtrapError: throw MailtrapError(message) with cause = original error
MailtrapError-->>Client: propagated MailtrapError
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
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)
src/lib/axios-logger.ts (1)
65-76: Capture scalar direct errors before they fall back to[object Object].Objects like
{ errors: [{ email: "[email protected]", message: "already taken" }] }still degrade because non-array properties are discarded inextractDirectErrors, forcing the final fallback to[object Object]. Wrapping simple scalars (string/number/boolean) into arrays preserves those messages and keeps them human-readable.Apply this diff so scalars survive the pipeline:
function extractDirectErrors(errorObj: Record<string, unknown>): string[] { const directErrors = Object.entries(errorObj) .filter( ([field]) => field !== "email" && field !== "id" && field !== "errors" ) .reduce((acc, [field, value]) => { - acc[field] = value; + if (Array.isArray(value)) { + acc[field] = value; + } else if ( + typeof value === "string" || + typeof value === "number" || + typeof value === "boolean" + ) { + acc[field] = [value]; + } return acc; - }, {} as Record<string, unknown>); + }, {} as Record<string, unknown>); return extractFieldErrors(directErrors); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/__tests__/lib/api/resources/ContactImports.test.ts(1 hunks)src/lib/axios-logger.ts(1 hunks)
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/__tests__/lib/axios-logger.test.ts (2)
360-527: Duplicate test cases for null response data.The tests "falls back to default message when response data is invalid" (lines 360-390) and "returns default message when data is null" (lines 497-527) are duplicates. Both test the same scenario where
responseData = nulland verify the error message falls back to the default.Remove one of the duplicate tests:
- it("returns default message when data is null", () => { - const responseData = null; - // @ts-ignore - const axiosError = new AxiosError( - "Network error", - "500", - { headers: {} } as any, - { - data: responseData, - status: 500, - } - ); - axiosError.response = { - data: responseData, - status: 500, - statusText: "Internal Server Error", - headers: {}, - config: {} as any, - }; - - expect.assertions(2); - - try { - axiosLogger(axiosError); - } catch (error) { - expect(error).toBeInstanceOf(MailtrapError); - if (error instanceof MailtrapError) { - expect(error.message).toBe("Network error"); - } - } - }); -
53-673: Consider extracting test helper to reduce boilerplate.The test cases follow a repetitive pattern with significant code duplication: creating
AxiosError, setting the response, wrapping in try/catch, and asserting error type/message. A helper function could improve maintainability and readability.Example helper function to add at the beginning of the describe block:
function createAxiosErrorAndExpect( message: string, code: string, status: number, statusText: string, responseData: any, expectedMessage: string, additionalAssertions?: (error: MailtrapError) => void ) { const axiosError = new AxiosError( message, code, { headers: {} } as any, { data: responseData, status } ); axiosError.response = { data: responseData, status, statusText, headers: {}, config: {} as any, }; try { axiosLogger(axiosError); fail("Expected axiosLogger to throw"); } catch (error) { expect(error).toBeInstanceOf(MailtrapError); if (error instanceof MailtrapError) { expect(error.message).toBe(expectedMessage); additionalAssertions?.(error); } } }Then tests can be simplified to:
it("extracts single error string from response data", () => { const responseData = { error: "Not Found" }; createAxiosErrorAndExpect( "Request failed", "404", 404, "Not Found", responseData, "Not Found" ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/__tests__/lib/axios-logger.test.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/lib/axios-logger.test.ts (1)
src/lib/MailtrapError.ts (1)
MailtrapError(1-1)
🔇 Additional comments (3)
src/__tests__/lib/axios-logger.test.ts (3)
1-51: LGTM! Baseline error handling tests are well-structured.The initial tests appropriately verify that non-axios errors are re-thrown unchanged and axios errors are wrapped in MailtrapError. The test structure is clear and the assertions are appropriate.
53-359: Excellent coverage of error extraction scenarios.These tests comprehensively validate error message extraction for the various formats mentioned in the PR objectives:
- Single error strings and arrays of strings
- Nested error objects with field-specific errors
- Direct base field errors
- Legacy name/base formats
- Multiple field errors with proper concatenation
The expected outputs (e.g., line 168:
"invalid-email-1: email: is invalid, is required | invalid-email-2: Contact limit exceeded") demonstrate proper formatting without[object Object]issues.
392-424: LGTM! Cause property preservation is properly tested.This test appropriately verifies that the original AxiosError is preserved in the
causeproperty of the thrown MailtrapError, maintaining error context for debugging. The// @ts-expect-errorcomment on line 419 is appropriate given ES5 type limitations.
…text and non-object error responses
…with Axios error handling
…-text and non-object responses
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/__tests__/lib/api/resources/ContactExports.test.ts(1 hunks)src/__tests__/lib/axios-logger.test.ts(1 hunks)src/lib/axios-logger.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/__tests__/lib/axios-logger.test.ts (1)
src/lib/MailtrapError.ts (1)
MailtrapError(1-1)
🪛 GitHub Actions: test
src/lib/axios-logger.ts
[error] 132-132: TS2638: Type '{}' may represent a primitive value, which is not permitted as the right operand of the 'in' operator.
🪛 GitHub Check: lint
src/lib/axios-logger.ts
[failure] 137-137:
Type '{}' may represent a primitive value, which is not permitted as the right operand of the 'in' operator.
[failure] 132-132:
Type '{}' may represent a primitive value, which is not permitted as the right operand of the 'in' operator.
| // error is in `data.error` | ||
| if ("error" in data && data.error) { | ||
| return String(data.error); | ||
| } | ||
|
|
||
| // errors are in `data.errors` | ||
| if ("errors" in data && data.errors) { | ||
| const { errors } = data; | ||
|
|
||
| // errors is an array of strings | ||
| if (Array.isArray(errors) && errors.length > 0) { | ||
| if (typeof errors[0] === "string") { | ||
| return errors.join(","); | ||
| } | ||
|
|
||
| // errors is an array of objects | ||
| if (typeof errors[0] === "object" && errors[0] !== null) { | ||
| const extracted = extractMessagesFromErrorObjects( | ||
| errors as Array<Record<string, unknown>> | ||
| ); | ||
| if (extracted) { | ||
| return extracted; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // errors is an object (could have name/base or field-specific errors) | ||
| if ( | ||
| typeof errors === "object" && | ||
| !Array.isArray(errors) && | ||
| errors !== null | ||
| ) { | ||
| // check for name/base properties first (legacy format) | ||
| const errorNames = | ||
| hasErrorProperty(errors, "name") && errors.name.join(", "); | ||
| const errorBase = | ||
| hasErrorProperty(errors, "base") && errors.base.join(", "); | ||
|
|
||
| if (errorNames) return errorNames; | ||
| if (errorBase) return errorBase; | ||
|
|
||
| // extract field-specific errors (e.g., { "email": ["is invalid", ...] }) | ||
| const fieldMessages = extractFieldErrors( | ||
| errors as Record<string, unknown> | ||
| ); | ||
|
|
||
| if (fieldMessages.length > 0) { | ||
| return fieldMessages.join("; "); | ||
| } | ||
|
|
||
| // If errors object doesn't match any recognized pattern, fall back to default message | ||
| return defaultMessage; | ||
| } | ||
|
|
||
| // If errors doesn't match any recognized format, fall back to default message | ||
| return defaultMessage; | ||
| } |
There was a problem hiding this comment.
Fix the object guard before using "in" on data.
data is still typed as unknown when we hit the "error" in data / "errors" in data checks, so TypeScript rightfully raises TS2638 and the build fails. Please narrow data to a non-null object (e.g., via if (typeof data !== "object" || data === null)) before running these property checks, or assign it to a Record<string, unknown> helper variable.
Apply this diff to resolve the type error and keep the runtime behavior intact:
- // error is in `data.error`
- if ("error" in data && data.error) {
- return String(data.error);
+ const dataRecord = data as Record<string, unknown>;
+
+ // error is in `data.error`
+ if ("error" in dataRecord && dataRecord.error) {
+ return String(dataRecord.error);
}
// errors are in `data.errors`
- if ("errors" in data && data.errors) {
- const { errors } = data;
+ if ("errors" in dataRecord && dataRecord.errors) {
+ const { errors } = dataRecord;(Be sure to adjust the subsequent uses of errors to reference dataRecord.) This satisfies the compiler and prevents the CI failure.
🧰 Tools
🪛 GitHub Actions: test
[error] 132-132: TS2638: Type '{}' may represent a primitive value, which is not permitted as the right operand of the 'in' operator.
🪛 GitHub Check: lint
[failure] 137-137:
Type '{}' may represent a primitive value, which is not permitted as the right operand of the 'in' operator.
[failure] 132-132:
Type '{}' may represent a primitive value, which is not permitted as the right operand of the 'in' operator.
🤖 Prompt for AI Agents
In src/lib/axios-logger.ts around lines 131 to 187, the code uses `"error" in
data` and `"errors" in data` while data is still typed as unknown, causing
TS2638; narrow data to a non-null object first (e.g., if (typeof data !==
"object" || data === null) return defaultMessage; or const dataRecord = data as
Record<string, unknown> after that guard) and then use dataRecord for all
subsequent property checks and accesses (including replacing uses of errors with
dataRecord.errors) so the TypeScript compiler is satisfied and runtime behavior
remains unchanged.
…ndle null and undefined values
…ing redundant comments
narekhovhannisyan
requested review from
VladimirTaytor,
leonid-shevtsov and
mklocek
Motivation
The axios logger was failing to properly extract error messages from API responses, resulting in unhelpful error messages like
[object Object]instead of meaningful root cause information. This made debugging difficult for developers using the SDK, as they couldn't identify what went wrong with their API requests.The logger only handled basic error formats (single error strings, arrays of strings, and objects with
name/baseproperties) but failed to handle:{ "email": ["is invalid"] })basefield (no identifier)Additionally, the code used
for...ofloops which violated linting rules requiring array methods instead.Changes
Enhanced error message extraction - Now properly handles all error response formats:
{ "email": ["is invalid"] })basefieldname/baseproperties (legacy format)Refactored to use array methods - Replaced all
for...ofloops with functional array methods (map,filter,reduce) to comply with linting rulesExtracted simple, focused functions - Broke down complex logic into smaller, testable functions:
formatFieldError- Formats a single field error messageextractFieldErrors- Extracts all field errors from an objectgetErrorIdentifier- Gets email/id identifier from error objectextractNestedErrors- Extracts errors from nested "errors" propertyextractDirectErrors- Extracts errors directly from object propertiesformatErrorMessage- Formats a single error object into message stringUpdated tests - Fixed test expectations to match the improved error message extraction behavior
How to test
Run existing test suite:
npm test -- src/__tests__/lib/axios-logger.test.ts src/__tests__/lib/api/resources/ContactImports.test.ts src/__tests__/lib/mailtrap-client.test.tsTest with real API calls that trigger various error formats:
Verify error messages are human-readable and show root cause (no more
[object Object])Verify linting passes:
npm run lintVerify build succeeds:
npm run buildSummary by CodeRabbit
Bug Fixes
Tests