fix(SSE-text/event-stream): sse response body is empty in res.getBody() for app and cli#8212
Open
sachin-thakur-bruno wants to merge 2 commits into
Open
Conversation
sachin-thakur-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
|
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)
WalkthroughThis PR updates the Electron IPC network SSE/response-stream flow to buffer raw streamed chunks into ChangesSSE Stream Chunk Accumulation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
sachin-thakur-bruno
changed the title
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/bruno-electron/src/ipc/network/index.js (1)
1017-1017: Consider memory implications for long-running SSE streams.The
sseChunksarray accumulates all chunks for the entire duration of the stream without any size limits. For long-running SSE connections or high-throughput streams, this could lead to significant memory consumption.While this is acceptable for typical SSE use cases, consider whether:
- A maximum size threshold or warning is warranted for production deployments
- The collected chunks could be periodically flushed or limited (though this might conflict with the goal of providing the full body to post-scripts)
This is primarily an operational consideration rather than a functional defect.
Also applies to: 1348-1349
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-electron/src/ipc/network/index.js` at line 1017, The sseChunks array used to accumulate SSE data (referenced as sseChunks) can grow unbounded; add a configurable cap (e.g., MAX_SSE_BUFFER_SIZE) and enforce it in the code that appends to sseChunks (the SSE data handler function where sseChunks.push is used) so that when the cap is exceeded you either drop oldest segments, stop buffering and log a warning via the existing logger, or flush the buffer to a temporary store; also expose the cap as a config/env variable and apply the same safeguard to the other occurrence noted around lines 1348-1349 so long-running/high-throughput streams won’t cause unbounded memory growth.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bruno-electron/src/ipc/network/index.js`:
- Around line 1247-1258: The close event handler must guard against exceptions
from Buffer.concat and parseDataFromResponse and handle rejections from
runPostScripts: wrap the SSE reconstruction logic (the Buffer.concat(...) and
parseDataFromResponse(...) calls) in a try-catch that logs the error and
performs any necessary cleanup/rejects the related promise, and change the
runPostScripts().then(...) call to include a .catch(...) (or use await with
try-catch) that logs the error and prevents an unhandled rejection; reference
the close event handler and the functions runPostScripts, Buffer.concat, and
parseDataFromResponse when making these changes.
- Around line 1348-1349: The SSE handler currently uses optional chaining with
response.sseChunks?.push(newData) which will silently drop chunks if
response.sseChunks is unset; ensure response.sseChunks is initialized before the
handler runs (e.g., set response.sseChunks = [] where the response is created)
or add an explicit fallback/assertion in the handler (e.g., if
(!response.sseChunks) throw or assign an empty array before calling push) so
chunks are never lost silently; locate the code that references
response.sseChunks?.push(newData) and either initialize response.sseChunks at
creation or replace the optional push with a guarded push/assertion to catch
misconfiguration.
---
Nitpick comments:
In `@packages/bruno-electron/src/ipc/network/index.js`:
- Line 1017: The sseChunks array used to accumulate SSE data (referenced as
sseChunks) can grow unbounded; add a configurable cap (e.g.,
MAX_SSE_BUFFER_SIZE) and enforce it in the code that appends to sseChunks (the
SSE data handler function where sseChunks.push is used) so that when the cap is
exceeded you either drop oldest segments, stop buffering and log a warning via
the existing logger, or flush the buffer to a temporary store; also expose the
cap as a config/env variable and apply the same safeguard to the other
occurrence noted around lines 1348-1349 so long-running/high-throughput streams
won’t cause unbounded memory growth.
🪄 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: c4269bda-2b56-4bf6-b6af-6e95ab316e4d
📒 Files selected for processing (1)
packages/bruno-electron/src/ipc/network/index.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
JIRA
Bug
The desktop app always requests responses as a stream
(
responseType: 'stream'). For non-streaming responses it drains the streaminto a full buffer via
promisifyStreambefore running post-response logic.For SSE responses, the single-request path deliberately skips that drain so
chunks can be forwarded to the renderer live, and hard-codes the body to empty:
The streamed chunks were forwarded to the renderer for the live view and then
discarded — nothing ever reconstituted them into response.data. So when
post-response scripts, assertions, and tests ran on the stream's close event,
they read an empty body.
Fix
Accumulate the streamed chunks (in the existing renderer-forwarding data
listener — the single stream consumer) and, on stream close, rebuild
response.data / response.dataBuffer from them before running
post-response scripts. This mirrors what the Collection Runner already does,
without adding a second stream listener (which would change flowing-mode timing
and risk dropping early chunks from the live UI).
All changes are in packages/bruno-electron/src/ipc/network/index.js:
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