fix: broken sse event rendering in ui - #8763
Conversation
|
@sammaji is attempting to deploy a commit to the Bruno Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe network IPC SSE path now buffers streamed chunks, reconstructs complete events across boundaries, preserves UTF-8 data, dispatches sequenced events, and flushes trailing data on stream close. Jest coverage validates buffering and SSE initial-response timeout behavior. ChangesSSE streaming
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant HTTPStream
participant sseEventBuffer
participant sendSseEvent
participant Renderer
HTTPStream->>sseEventBuffer: Push streamed chunk
sseEventBuffer->>sendSseEvent: Return complete SSE event
sendSseEvent->>Renderer: Dispatch parsed sequenced event
HTTPStream->>sseEventBuffer: Flush on close
sseEventBuffer->>sendSseEvent: Return trailing event
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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.github.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/sse-event-buffer.js`:
- Around line 23-26: Update the flush function to preserve leading and trailing
whitespace in the decoded SSE payload, removing the trim() call from the
remaining-data calculation. Keep returning null only when no data remains, while
forwarding non-JSON payload strings unchanged through parseDataFromResponse.
- Around line 10-18: Normalize decoded SSE text only after appending it to the
existing buffer in push, so CRLF sequences split across chunks remain a single
newline; preserve lone-CR normalization. In
packages/bruno-electron/src/ipc/network/sse-event-buffer.js lines 10-18, update
push accordingly. In
packages/bruno-electron/src/ipc/network/sse-event-buffer.spec.js lines 40-45,
add regression coverage for data: 1\r followed by \n (no emission until the
subsequent blank line) and for the promised lone-CR behavior.
In `@packages/bruno-electron/src/ipc/network/sse-event-buffer.spec.js`:
- Around line 33-35: Reformat the sseEventBuffer.push call in the test so its
single Buffer.from argument remains on the same line as the function call,
avoiding newlines inside the function parentheses.
🪄 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 Plus
Run ID: f816a76e-faca-4d70-8fbf-faa9d65aacd9
📒 Files selected for processing (3)
packages/bruno-electron/src/ipc/network/index.jspackages/bruno-electron/src/ipc/network/sse-event-buffer.jspackages/bruno-electron/src/ipc/network/sse-event-buffer.spec.js
|
@sammaji thanks for the PR, a few sanity cases, can we add additional tests for super slow streams so the request timeout is being respected, for cases where a malformed stream or never ending stream doesn't just keep waiting (this should mostly be handled by axios but a test would help) |
|
@sid-bruno from my testing, here's what I found: For streams, the request timeout is only enforced until the first chunk has arrived. So if I set a timeout that triggers before the first chunk has arrived, I get timeout exceeded. But once first chunk has arrived, timeout is not longer enforced.
Right now streams never end (even if request timeout is set). This is how axios handles request timeout, and it makes sense. SSE streams are meant to be long running events, request level timeout should not stop long running streams. If we want to enforce timeout, many applications have a "Stream Idle timeout" which triggers if the stream is idle for said time. |
|
@sammaji Yeah i just need the first one to be tested by the e2e, the idle timer will need a lot more work in the codebase so I'd like to spend some time thinking all impacted areas before working on that. just the e2e for the first chunk timeout should be good, if you don't have the time, i'll take over the PR as I find time. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/sse-event-buffer.spec.js`:
- Around line 107-122: Update the SSE test around the response returned by
makeAxiosInstance().get so response.data is listened to for errors before the
finally block calls close(). Attach a no-op or otherwise handled error listener
to the IncomingMessage stream before cleanup, while preserving the existing
status and content-type assertions.
🪄 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 Plus
Run ID: a89a83fd-75a1-4fbb-8708-58844a70a61f
📒 Files selected for processing (3)
packages/bruno-electron/src/ipc/network/index.jspackages/bruno-electron/src/ipc/network/sse-event-buffer.jspackages/bruno-electron/src/ipc/network/sse-event-buffer.spec.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bruno-electron/src/ipc/network/sse-event-buffer.js
- packages/bruno-electron/src/ipc/network/index.js
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/sse-event-buffer.spec.js`:
- Around line 1-142: Add semicolons throughout the new sse-event-buffer.spec.js
test file, including after statements, declarations, function expressions, and
object properties where required by the project’s JavaScript style. Preserve the
existing test behavior and structure.
- Around line 40-45: Add focused tests in the existing sseEventBuffer
normalization suite for lone \r line endings and for a CRLF delimiter split
across two push calls. Assert both produce the same single normalized event
without an extra blank line, reusing createSseEventBuffer and the existing
events expectation style.
🪄 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 Plus
Run ID: 67ef9dbe-d236-405c-8377-a6956fb567f8
📒 Files selected for processing (3)
packages/bruno-electron/src/ipc/network/index.jspackages/bruno-electron/src/ipc/network/sse-event-buffer.jspackages/bruno-electron/src/ipc/network/sse-event-buffer.spec.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bruno-electron/src/ipc/network/sse-event-buffer.js
- packages/bruno-electron/src/ipc/network/index.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/sse-event-buffer.js`:
- Around line 32-43: Update the SSE event buffer’s event extraction loop to skip
empty slices when consecutive separators produce a blank event, while preserving
valid event dispatch. Update flush to return null for whitespace/newline-only
leftovers, rather than forwarding them as events.
🪄 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 Plus
Run ID: 31029cf4-05e1-4171-85a3-ece8206f40e5
📒 Files selected for processing (3)
packages/bruno-electron/src/ipc/network/index.jspackages/bruno-electron/src/ipc/network/sse-event-buffer.jspackages/bruno-electron/src/ipc/network/sse-event-buffer.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-electron/src/ipc/network/index.js
|
hey @sid-bruno @anusree-bruno have added the test cases and resolved all CRs. can you review once.
agreed, even the ux of the sse response viewer needs a bit of work. right now its just displaying json (minified version). ideally we should show a json editor (if we can detect the object is a json). let me know if you need a hand 🙇 |
Description
Added
sse-event-buffer.js(createSseEventBuffer), which buffers incoming bytes, decoding with StringDecoder so multi-byte UTF-8 characters are never split across chunks, and normalizing CRLF/CR line endings until a full SSE event (blank-line terminated) is available, only then handing it off to the renderer.flush()emits any trailing event left over when the stream closes without a final blank line. The send-http-request handler now pushes each chunk through this buffer instead of parsing raw chunks in isolation. The raw-byte accumulation used to rebuild the final persisted response body is untouched, so that stays byte-accurate.Added unit tests in
sse-event-buffer.spec.jscovering: buffering until a full event arrives, reassembling a data: payload split mid-string (the reported bug), multiple events in one chunk, CRLF/CR normalization, multi-byte UTF-8 characters split across chunks, andflush()behavior.Before:
After:
Closes #8762
Contribution Checklist:
Summary by CodeRabbit
\r\n/\rnormalization and UTF-8 multi-byte boundaries.