fix: measure total response time instead of TTFB (#7594)#7628
Open
pkrisko wants to merge 4 commits intousebruno:mainfrom
Open
fix: measure total response time instead of TTFB (#7594)#7628pkrisko wants to merge 4 commits intousebruno:mainfrom
pkrisko wants to merge 4 commits intousebruno:mainfrom
Conversation
pkrisko
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
Contributor
WalkthroughCentralized response-time computation into a new Changes
Sequence Diagram(s)(omitted) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 |
5 tasks
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-electron/src/ipc/network/index.js (1)
1626-1627:⚠️ Potential issue | 🟠 MajorInconsistent timing: collection runner still reports TTFB.
The
run-collection-folderhandler directly readsrequest-durationfrom headers, which was set by the axios interceptor at TTFB (when headers arrived). This bypasses the newmeasureResponseTimelogic that recalculates total duration for non-stream responses.The
standalonerequest path (lines 919, 942) now correctly reports total time, but collection/folder runs will still show TTFB.Consider applying the same pattern here:
Proposed fix
- response.responseTime = response.headers.get('request-duration'); + response.responseTime = measureResponseTime(response.config, response.headers, false);And similarly at line 1664 for the error path.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/src/ipc/network/index.js` around lines 1626 - 1627, The collection/folder runner reads request-duration directly from headers (response.headers.get('request-duration')) which reflects TTFB and bypasses measureResponseTime; update the run-collection-folder handler to call the same measureResponseTime routine used by the standalone path (the function named measureResponseTime) to compute total duration, set response.responseTime to that computed value, then remove the request-duration header; apply the same change in the error handling path (the block currently at the later error path) so both success and error branches use measureResponseTime rather than the raw header.
🧹 Nitpick comments (1)
packages/bruno-electron/tests/network/index.spec.js (1)
62-87:measureResponseTimetests cover the branching logic well.However, the test at lines 63-68 has potential flakiness:
const start = Date.now() - 150; // ... expect(measureResponseTime(config, headers, false)).toBeGreaterThanOrEqual(150);Under heavy CI load, the elapsed time between
Date.now() - 150and themeasureResponseTimecall could be significant, causing intermittent failures. Consider using a fixed mock forDate.now()or a wider tolerance.Alternative approach using jest.spyOn
it('returns elapsed ms from request-start-time for non-stream response', () => { - const start = Date.now() - 150; - const config = { headers: { 'request-start-time': start } }; - const headers = { get: () => null }; - expect(measureResponseTime(config, headers, false)).toBeGreaterThanOrEqual(150); + const now = 1000; + jest.spyOn(Date, 'now').mockReturnValue(now); + const config = { headers: { 'request-start-time': 850 } }; + const headers = { get: () => null }; + expect(measureResponseTime(config, headers, false)).toBe(150); + jest.restoreAllMocks(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/tests/network/index.spec.js` around lines 62 - 87, The test "returns elapsed ms from request-start-time for non-stream response" is flaky because it uses the real Date.now(); mock Date.now() (e.g., with jest.spyOn(Date, 'now').mockReturnValue(fixedNow)) so you can compute const start = Date.now() - 150 deterministically before calling measureResponseTime, then restore the spy after the test; alternatively increase the tolerance check, but the preferred fix is to mock Date.now() around the test that uses measureResponseTime to ensure stable timing.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/bruno-electron/src/ipc/network/index.js`:
- Around line 1626-1627: The collection/folder runner reads request-duration
directly from headers (response.headers.get('request-duration')) which reflects
TTFB and bypasses measureResponseTime; update the run-collection-folder handler
to call the same measureResponseTime routine used by the standalone path (the
function named measureResponseTime) to compute total duration, set
response.responseTime to that computed value, then remove the request-duration
header; apply the same change in the error handling path (the block currently at
the later error path) so both success and error branches use measureResponseTime
rather than the raw header.
---
Nitpick comments:
In `@packages/bruno-electron/tests/network/index.spec.js`:
- Around line 62-87: The test "returns elapsed ms from request-start-time for
non-stream response" is flaky because it uses the real Date.now(); mock
Date.now() (e.g., with jest.spyOn(Date, 'now').mockReturnValue(fixedNow)) so you
can compute const start = Date.now() - 150 deterministically before calling
measureResponseTime, then restore the spy after the test; alternatively increase
the tolerance check, but the preferred fix is to mock Date.now() around the test
that uses measureResponseTime to ensure stable timing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 10e62954-628c-42bf-9dd7-f8fea7194826
📒 Files selected for processing (2)
packages/bruno-electron/src/ipc/network/index.jspackages/bruno-electron/tests/network/index.spec.js
Contributor
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-electron/tests/network/index.spec.js`:
- Around line 63-70: The test enabling fake timers around measureResponseTime
should always restore real timers even if an assertion or helper throws; wrap
the setup/useRealTimers sequence in a try/finally so jest.useRealTimers() is
called in the finally block. Locate the test that calls jest.useFakeTimers(),
jest.setSystemTime(...), and measureResponseTime(config, headers, false) and
change it so jest.useRealTimers() is executed inside a finally, preserving the
existing config and headers variables while ensuring timers are restored.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9f609450-20f7-43ee-9694-2e1835fdcd87
📒 Files selected for processing (2)
packages/bruno-electron/src/ipc/network/index.jspackages/bruno-electron/tests/network/index.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-electron/src/ipc/network/index.js
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-electron/tests/network/index.spec.js (1)
26-60: Minor style inconsistency:emit('data')vspush().Lines 30-31 use
stream.emit('data', ...)while lines 40 and 49 usestream.push(...). Both work (push triggers 'data' in flowing mode), but mixing them is inconsistent. Consider standardizing on one approach for clarity.♻️ Option: standardize on emit() throughout
it('resolves immediately on first data chunk when closeOnFirst is true', async () => { const stream = new Readable({ read() {} }); const resultPromise = promisifyStream(stream, null, true); - stream.push(Buffer.from('first')); + stream.emit('data', Buffer.from('first')); const result = await resultPromise; expect(Buffer.from(result).toString()).toBe('first'); }); it('calls abortController.abort() when closeOnFirst is true', async () => { const stream = new Readable({ read() {} }); const abortController = { abort: jest.fn() }; const resultPromise = promisifyStream(stream, abortController, true); - stream.push(Buffer.from('data')); + stream.emit('data', Buffer.from('data')); await resultPromise; expect(abortController.abort).toHaveBeenCalled(); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-electron/tests/network/index.spec.js` around lines 26 - 60, Tests for promisifyStream mix stream.emit('data', ...) and stream.push(...), causing a style inconsistency; pick one approach and make all tests consistent (e.g., replace the emit('data', Buffer.from(...)) calls in the "resolves with concatenated chunks on close" test with stream.push(Buffer.from(...)) so every test uses Readable.prototype.push to deliver data), keeping the rest of the assertions and abortController usage unchanged.
🤖 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-electron/tests/network/index.spec.js`:
- Around line 26-60: Tests for promisifyStream mix stream.emit('data', ...) and
stream.push(...), causing a style inconsistency; pick one approach and make all
tests consistent (e.g., replace the emit('data', Buffer.from(...)) calls in the
"resolves with concatenated chunks on close" test with
stream.push(Buffer.from(...)) so every test uses Readable.prototype.push to
deliver data), keeping the rest of the assertions and abortController usage
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 8692283c-9580-4f85-b9d1-4614c58232b4
📒 Files selected for processing (1)
packages/bruno-electron/tests/network/index.spec.js
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Fixes #7594.
Since #4472,
request.responseType = 'stream'is set for all requests so SSE responses don't hang. A side-effect: Axios fires its response interceptor at TTFB (when headers arrive), not after the body is fully received. The interceptor setsrequest-durationat that point, so Bruno reports TTFB as "Response Time" — even when the body takes 30+ seconds to download.For non-
text/event-streamresponses,responseTimeis now re-measured afterpromisifyStream()resolves, capturing total end-to-end duration. SSE responses keep the interceptor's TTFB timing, which is appropriate for streams. Both the happy path and the error-response path are fixed.I created a small server to reproduce the issue:
And confirmed that we now show 5000ms instead of 0s.
Contribution Checklist:
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
Bug Fixes
Tests