feat: enhance axios shim error handling and add comprehensive tests#6349
feat: enhance axios shim error handling and add comprehensive tests#6349Pragadesh-45 wants to merge 4 commits intousebruno:mainfrom
Conversation
Pragadesh-45
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
WalkthroughChanged the QuickJS axios shim to reject promises on errors and return a richer error object (message, code, isAxiosError, optional response: status/statusText/headers/data, and request config: url/method/headers/data) instead of resolving with minimal error info; added tests and updated ErrorCapture serialization. Changes
Sequence Diagram(s)sequenceDiagram
actor SandboxScript
participant QuickJSVM
participant Shim
participant Axios
SandboxScript->>QuickJSVM: invoke axios method (e.g., post)
QuickJSVM->>Shim: call shim function with args
Shim->>Axios: perform HTTP call
Axios-->>Shim: success response OR error (network/4xx/5xx/timeout)
alt success
Shim-->>QuickJSVM: marshallToVm(cleanJson(response)) (promise resolved)
QuickJSVM-->>SandboxScript: returns resolved data
else error
Shim-->>QuickJSVM: marshallToVm(cleanJson(buildAxiosErrorData(err))) (promise rejected)
QuickJSVM-->>SandboxScript: throws / rejects with rich error object
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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: 2
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bruno-js/src/sandbox/quickjs/shims/lib/axios.js(2 hunks)packages/bruno-js/src/sandbox/quickjs/shims/lib/axios.spec.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-js/src/sandbox/quickjs/shims/lib/axios.spec.jspackages/bruno-js/src/sandbox/quickjs/shims/lib/axios.js
🧠 Learnings (1)
📚 Learning: 2025-12-05T20:31:33.005Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.test.{js,jsx,ts,tsx} : Add tests for any new functionality or meaningful changes. If code is added, removed, or significantly modified, corresponding tests should be updated or created
Applied to files:
packages/bruno-js/src/sandbox/quickjs/shims/lib/axios.spec.js
🧬 Code graph analysis (1)
packages/bruno-js/src/sandbox/quickjs/shims/lib/axios.js (2)
packages/bruno-js/src/sandbox/quickjs/utils/index.js (1)
marshallToVm(1-31)packages/bruno-js/src/utils.js (1)
cleanJson(144-194)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: SSL Tests - Windows
- GitHub Check: CLI Tests
- GitHub Check: Unit Tests
- GitHub Check: Playwright E2E Tests
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Linux
🔇 Additional comments (6)
packages/bruno-js/src/sandbox/quickjs/shims/lib/axios.js (1)
17-38: LGTM! Error handling now properly rejects with comprehensive error data.The enhanced error structure correctly includes message, response details (status, statusText, headers, data), and request config, fixing issue #6342 where the shim was resolving on 4xx/5xx responses.
packages/bruno-js/src/sandbox/quickjs/shims/lib/axios.spec.js (5)
22-107: LGTM! Comprehensive coverage of successful request scenarios.The tests properly validate data marshalling across the VM boundary and ensure all HTTP methods work correctly. Handle disposal is consistent throughout.
109-252: LGTM! Thorough validation of 4xx/5xx error rejection behavior.These tests confirm the fix for issue #6342, ensuring errors are properly rejected with comprehensive error data including response details and request config.
254-332: LGTM! Network error scenarios are well covered.The tests validate that errors without responses (network failures, timeouts) still provide the error message and config data to callers.
334-411: LGTM! Base axios function coverage ensures API consistency.The tests confirm that calling
axios()directly behaves identically to the method shortcuts for both success and error cases.
413-472: LGTM! Excellent real-world scenario validation.This test directly addresses the token refresh use case from issue #6342, ensuring that authentication error handling patterns work as expected with full error details preserved.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/bruno-js/src/sandbox/quickjs/shims/lib/axios.js (1)
7-27: Helper correctly centralizes error shaping; consider exposing a bit more Axios metadata
buildAxiosErrorDatadoes the right thing formessage,response, andconfig, and nicely de-duplicates the logic. If you want closer parity with typical Axios errors, you might optionally also pass through lightweight fields such aserr.codeanderr.isAxiosError(and maybename) so existing guards likeif (error.isAxiosError)keep working across environments.Example (non-breaking) extension:
const buildAxiosErrorData = (err) => { return { message: err.message, + code: err.code, + isAxiosError: err.isAxiosError, ...(err.response && { response: { status: err.response.status, statusText: err.response.statusText, headers: err.response.headers, data: err.response.data } }), ...(err.config && { config: { url: err.config.url, method: err.config.method, headers: err.config.headers, data: err.config.data } }) }; };packages/bruno-js/src/sandbox/quickjs/shims/lib/axios.spec.js (1)
53-66: Consider a small helper to DRY the QuickJS async eval patternMost tests repeat the same boilerplate to run async code in the VM, resolve the promise, dump the value, and dispose handles. A tiny helper would make the intent clearer and reduce chances of mistakes if this pattern evolves.
For example:
const evalAsyncInVm = async (vm, code) => { const result = vm.evalCode(code); const promiseHandle = vm.unwrapResult(result); const resolvedResult = await vm.resolvePromise(promiseHandle); const resolvedHandle = vm.unwrapResult(resolvedResult); const value = vm.dump(resolvedHandle); resolvedHandle.dispose(); promiseHandle.dispose(); return value; };Then each test can do:
const responseData = await evalAsyncInVm(vm, ` (async () => { const response = await axios.get('https://api.example.com/data'); return response; })() `);This keeps the tests focused on behavior (status, data, error shape) instead of VM plumbing.
Also applies to: 82-95, 112-125, 168-175, 217-224, 262-269, 304-311, 343-350, 374-381, 421-428, 478-485
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bruno-js/src/sandbox/quickjs/shims/lib/axios.js(3 hunks)packages/bruno-js/src/sandbox/quickjs/shims/lib/axios.spec.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings. For JSX/TSX attributes, use double quotes (e.g., )
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single params
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-js/src/sandbox/quickjs/shims/lib/axios.spec.jspackages/bruno-js/src/sandbox/quickjs/shims/lib/axios.js
🧠 Learnings (1)
📚 Learning: 2025-12-05T20:31:33.005Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.test.{js,jsx,ts,tsx} : Add tests for any new functionality or meaningful changes. If code is added, removed, or significantly modified, corresponding tests should be updated or created
Applied to files:
packages/bruno-js/src/sandbox/quickjs/shims/lib/axios.spec.js
🧬 Code graph analysis (1)
packages/bruno-js/src/sandbox/quickjs/shims/lib/axios.js (2)
packages/bruno-js/src/sandbox/quickjs/utils/index.js (1)
marshallToVm(1-31)packages/bruno-js/src/utils.js (1)
cleanJson(144-194)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Linux
- GitHub Check: Playwright E2E Tests
- GitHub Check: Unit Tests
- GitHub Check: CLI Tests
🔇 Additional comments (3)
packages/bruno-js/src/sandbox/quickjs/shims/lib/axios.js (1)
40-41: Rejection semantics and error marshalling now align with Axios expectationsSwitching the
.catchpaths topromise.reject(marshallToVm(cleanJson(buildAxiosErrorData(err)), vm))for both method shims and the baseaxiosshim fixes the original problem: 4xx/5xx and other Axios errors now surface as rejections with richresponseandconfigdata available in the sandbox. The marshalling pipeline (buildAxiosErrorData→cleanJson→marshallToVm) looks consistent with the success path and should behave well for binary-safe payloads.Also applies to: 56-57
packages/bruno-js/src/sandbox/quickjs/shims/lib/axios.spec.js (2)
22-42: VM and module cleanup look correct and address leak concernsThe
afterEach/afterAllhooks that dispose ofvmandmodule(with guarded try/catch and nulling the references) nicely close the lifecycle gap from earlier reviews and should prevent QuickJS contexts and the underlying WASM module from leaking across tests.
1-8: [rewritten comment]
[classification_tag]
* refactor: improve error handling in axios shim to include detailed error information such as response and config data * add: implement extensive unit tests for axios shim covering successful requests, error handling, and real-world scenarios * test: ensure proper handling of various HTTP status codes and network errors in the tests
…ties * add: include error code and isAxiosError flag in the error object returned by the axios shim for improved error handling
Pragadesh-45
force-pushed
the
fix/6342
branch
from
72f8f91 to
588a917
Compare
…ences * add: implement a replacer function to enhance JSON serialization of error objects, preventing circular reference issues and preserving error properties * refactor: apply similar serialization logic in both ErrorCapture and utility functions for consistency * tests
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@packages/bruno-app/src/components/ErrorCapture/index.js`:
- Around line 46-52: The serialized error object created in the ErrorCapture
code (the block using Object.getOwnPropertyNames on value) can miss the
Error.name because it's on the prototype; update the serialization to ensure
name is preserved by adding a fallback after copying properties: if error.name
is falsy, set it from value.name or value.constructor?.name (or a default like
'Error') so the error type is retained in payloads. Target the same block that
builds the local error object from value.
🧹 Nitpick comments (1)
packages/bruno-app/src/components/ErrorCapture/index.js (1)
35-36: Add JSDoc forserializeArgsto document serialization rules.This abstraction now handles circular refs and Error normalization; adding JSDoc keeps the contract clear.
As per coding guidelines: Add JSDoc comments to abstractions for additional details.📝 Proposed JSDoc
+/** + * Serializes console error arguments into JSON-safe values. + * Handles circular references and Error-like objects. + * `@param` {any[]} args + * `@returns` {Array<any>} + */ const serializeArgs = (args) => {
| if (value instanceof Error || Object.prototype.toString.call(value) === '[object Error]' || (typeof value.message === 'string' && typeof value.stack === 'string')) { | ||
| const error = {}; | ||
| Object.getOwnPropertyNames(value).forEach((prop) => { | ||
| error[prop] = value[prop]; | ||
| }); | ||
| return error; | ||
| } |
There was a problem hiding this comment.
Preserve Error.name in serialized payloads.
Object.getOwnPropertyNames skips the prototype name on standard Error instances, so logs lose the error type. Add a fallback copy.
🐛 Proposed fix
if (value instanceof Error || Object.prototype.toString.call(value) === '[object Error]' || (typeof value.message === 'string' && typeof value.stack === 'string')) {
const error = {};
Object.getOwnPropertyNames(value).forEach((prop) => {
error[prop] = value[prop];
});
+ if (typeof value.name === 'string' && error.name === undefined) {
+ error.name = value.name;
+ }
return error;
}🤖 Prompt for AI Agents
In `@packages/bruno-app/src/components/ErrorCapture/index.js` around lines 46 -
52, The serialized error object created in the ErrorCapture code (the block
using Object.getOwnPropertyNames on value) can miss the Error.name because it's
on the prototype; update the serialization to ensure name is preserved by adding
a fallback after copying properties: if error.name is falsy, set it from
value.name or value.constructor?.name (or a default like 'Error') so the error
type is retained in payloads. Target the same block that builds the local error
object from value.
1 participant
Jira
fixes: #6342
Description
feat: improve serialization of error objects to handle circular references
Contribution Checklist:
Before:
After:
Summary by CodeRabbit
Bug Fixes
Tests
Chores