fix: add size and duration fields to CLI bru.runRequest() response#7429
Conversation
chirag-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughAdds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-cli/src/runner/run-single-request.js`:
- Around line 968-970: The duration field is sometimes a string because
responseTime is read from response.headers.get('request-duration'); coerce it to
a numeric type before assigning to the result object (where duration is
currently set alongside responseTime and size) so arithmetic works downstream.
Update the code that reads and assigns responseTime (the variable named
responseTime from response.headers.get(...)) to parse/Number-coerce it (with a
fallback of 0) and then use that numeric value for both responseTime and
duration in the object being returned/recorded.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8c62a001-dbcf-4b30-b8ab-e4fcde3196d7
📒 Files selected for processing (1)
packages/bruno-cli/src/runner/run-single-request.js
sanish-bruno
left a comment
sanish-bruno
left a comment
There was a problem hiding this comment.
PR Review
Found 1 issue(s): 0 major, 0 minor, 1 suggestion.
Summary
- All 6 response object locations are consistently updated with
durationandsize— no missed spots. sizecalculation usingBuffer.byteLength(response.dataBuffer)matches the GUI implementation.- The
dataBufferguard (response.dataBuffer ? ... : 0) is safe sinceparseDataFromResponsealways returns a Buffer, but it's fine as defensive code. - One semantic note about
durationvsresponseTime— see inline comment.
| url: response.request ? response.request.protocol + '//' + response.request.host + response.request.path : null, | ||
| responseTime | ||
| responseTime, | ||
| duration: responseTime, |
There was a problem hiding this comment.
Suggestion: In the GUI (bruno-electron/src/ipc/network/index.js), duration and responseTime are actually different values:
duration=timeEnd - timeStart(wall-clock elapsed time measured around the axios call)responseTime= from the customrequest-durationheader injected by the axios interceptor
Setting duration: responseTime here makes them identical in the CLI, which is a reasonable approximation since the CLI doesn't have the same timeStart/timeEnd timing wrapper. But it might be worth adding a brief comment so future readers understand this is intentional:
// In the GUI, duration is wall-clock time (timeEnd - timeStart).
// In the CLI we use responseTime as a close approximation.
duration: responseTime,This also means scripts using bru.runRequest() will see identical duration and responseTime values in the CLI but potentially different values in the GUI. Probably fine for most use cases, but worth documenting.
|
@chirag-bruno can we have test cases to verify this? |
Add `size` and `duration` fields to the response object in CLI to match GUI behavior, ensuring consistent API for bru.runRequest() across both environments. - `duration` is an alias for `responseTime` for GUI compatibility - `size` is the byte length of the response buffer (0 for errors/skipped) Fixes usebruno#7352
- Coerce responseTime header to number (was string from headers.get()) - Add comment explaining duration vs responseTime difference between GUI (wall-clock) and CLI (approximation using responseTime) - Add integration tests for duration/size fields across skipped, success, and network error response paths
chirag-bruno
force-pushed
the
fix/cli-runrequest-response-consistency
branch
from
288c995 to
b3ccd79
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-cli/tests/runner/response-fields.spec.js (1)
138-140: Remove the unusedmockHeadersvariable and its self-recursivedeleteoverride.The
mockHeadersvariable (lines 138–139) is dead code and is never used in the test. The self-recursivedeletefunction is also a footgun that would cause infinite recursion if called. The properheadersobject below already provides the correct mock.Suggested cleanup
it('should return numeric duration and size on successful request', async () => { const responseBody = JSON.stringify({ message: 'ok' }); - const mockHeaders = new Map([['request-duration', '253']]); - mockHeaders.delete = function (key) { this.delete(key); }; // Use a plain object with get/delete to simulate axios headers const headers = { get: (key) => key === 'request-duration' ? '253' : null, delete: jest.fn() };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-cli/tests/runner/response-fields.spec.js` around lines 138 - 140, Remove the unused mockHeaders declaration and its self-recursive delete override in the test: delete the lines that define const mockHeaders = new Map([...]) and the mockHeaders.delete = function (key) { this.delete(key); }; so the test relies on the existing headers object that already provides the proper get/delete behavior; ensure no references to mockHeaders remain and run tests to confirm nothing else uses that variable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/bruno-cli/tests/runner/response-fields.spec.js`:
- Around line 138-140: Remove the unused mockHeaders declaration and its
self-recursive delete override in the test: delete the lines that define const
mockHeaders = new Map([...]) and the mockHeaders.delete = function (key) {
this.delete(key); }; so the test relies on the existing headers object that
already provides the proper get/delete behavior; ensure no references to
mockHeaders remain and run tests to confirm nothing else uses that variable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1d1c1297-3e72-43f6-8cfc-58865f8249cc
📒 Files selected for processing (2)
packages/bruno-cli/src/runner/run-single-request.jspackages/bruno-cli/tests/runner/response-fields.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-cli/src/runner/run-single-request.js
The success path calls setupProxyAgents which was missing from the proxy-util mock, causing CI failure.
sanish-bruno
left a comment
sanish-bruno
left a comment
There was a problem hiding this comment.
PR Review
Found 1 issue(s): 0 major, 0 minor, 1 nit.
Summary
- Production code changes are clean and correct — all 6 return paths consistently include
durationandsizefields - Good fix coercing
request-durationheader from string to number withNumber(...) || 0 durationas an alias forresponseTimein CLI is a reasonable approximation (documented with a comment)Buffer.byteLength(response.dataBuffer)is the correct way to compute response size- Test coverage hits the three key paths (skipped, success, error)
| prepareRequest.mockResolvedValue({ | ||
| method: 'GET', | ||
| url: 'http://example.com/api', | ||
| headers: {}, |
There was a problem hiding this comment.
Nit: Dead code — mockHeaders Map is created but never used (the plain headers object on line 153 is what gets passed to the mock response).
Also, the delete override on line 150 would cause infinite recursion if called (this.delete(key) calls itself since this.delete was just replaced).
Suggested fix: remove lines 149-150.
// Remove these two lines:
const mockHeaders = new Map([['request-duration', '253']]);
mockHeaders.delete = function (key) { this.delete(key); };
4 participants
Description
Add
sizeanddurationfields to the response object in CLI to match GUI behavior, ensuring consistent API for bru.runRequest() across both environments.durationis an alias forresponseTimefor GUI compatibilitysizeis the byte length of the response buffer (0 for errors/skipped)Fixes #7352
JIRA
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
New Features
Bug Fixes
Tests