fix: add a hard sequence in SSE and WS requests #6569
Conversation
SW881
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
WalkthroughPer-request/collection sequence numbers (seq) are introduced and propagated through WsClient, Electron IPC (HTTP streams), Redux actions, and UI rendering. Events now carry seq and timestamp; UI sorts and keys stream/timeline entries by seq (falling back to timestamp) for deterministic ordering. Changes
Sequence DiagramsequenceDiagram
participant WsClient as WsClient
participant IPC as Electron IPC
participant Redux as Redux (collections)
participant UI as WsResponsePane
Note over WsClient,IPC: Events annotated with seq & timestamp
WsClient->>WsClient: seq = sequencer.next(requestId, collectionId)
WsClient->>IPC: send stream/event (data, seq, timestamp)
IPC->>IPC: increment local seq for stream chunks
IPC->>Redux: dispatch streamDataReceived / requestCancelled (seq, timestamp, data)
Redux-->>Redux: attach seq/timestamp to timeline & response entries
Redux->>UI: propagate entries with seq
UI->>UI: sort entries by (seq ?? timestamp) and render
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)**/*.{js,jsx,ts,tsx}📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Files:
🧠 Learnings (2)📚 Learning: 2025-12-02T09:45:31.709ZApplied to files:
📚 Learning: 2025-12-17T21:41:24.730ZApplied to files:
🔇 Additional comments (1)
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
🧹 Nitpick comments (1)
packages/bruno-app/src/components/ResponsePane/WsResponsePane/WSMessagesList/index.js (1)
191-191: Consider using nullish coalescing for the key.The ternary
msg.seq ? msg.seq : msg.timestamptreats0as falsy, which would incorrectly fall back to timestamp ifseqwere ever zero. While current IPC starts seq at 1, using??is more semantically accurate for "use timestamp only when seq is missing."🔎 Suggested fix
- return <WSMessageItem key={msg.seq ? msg.seq : msg.timestamp} inFocus={inFocus} id={idx} message={msg} />; + return <WSMessageItem key={msg.seq ?? msg.timestamp} inFocus={inFocus} id={idx} message={msg} />;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/bruno-app/src/components/ResponsePane/WsResponsePane/WSMessagesList/index.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-electron/src/ipc/network/index.js
🧰 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-app/src/components/ResponsePane/WsResponsePane/WSMessagesList/index.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-electron/src/ipc/network/index.js
🧠 Learnings (1)
📚 Learning: 2025-12-17T21:41:24.730Z
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
Applied to files:
packages/bruno-app/src/components/ResponsePane/WsResponsePane/WSMessagesList/index.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-electron/src/ipc/network/index.js
🧬 Code graph analysis (3)
packages/bruno-app/src/components/ResponsePane/WsResponsePane/WSMessagesList/index.js (1)
packages/bruno-tests/src/ws/index.js (1)
msg(22-22)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (2)
packages/bruno-app/src/utils/collections/index.js (11)
collection(587-587)collection(1175-1175)collection(1177-1177)collection(1605-1605)collection(1615-1615)findCollectionByUid(80-82)findCollectionByUid(80-82)item(1042-1042)item(1276-1276)findItemInCollection(117-121)findItemInCollection(117-121)packages/bruno-cli/src/utils/collection.js (1)
findItemInCollection(336-340)
packages/bruno-electron/src/ipc/network/index.js (2)
packages/bruno-electron/src/utils/common.js (1)
parseDataFromResponse(106-130)packages/bruno-cli/src/utils/common.js (1)
parseDataFromResponse(19-43)
🔇 Additional comments (5)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (2)
385-418: Looks good. The seq-based ordering maintains consistency across stream events.The descending sort (
sb - sa) withPOSITIVE_INFINITYfallback for missingseqvalues ensures backward compatibility—items withoutseqappear first. The "Connection Closed" info entry now carries sequence metadata for proper ordering.
3132-3156: LGTM. Insertion logic correctly maintains descending seq order.The
findIndexapproach efficiently inserts new messages in their proper position without re-sorting the entire array. The nullish fallbackm.seq ?? Infinityaligns with the sort logic inrequestCancelled.packages/bruno-electron/src/ipc/network/index.js (3)
1024-1025: Batching constants look reasonable.10 events per 50ms tick gives a theoretical max of ~200 events/sec to the renderer—should prevent UI freezes under high-volume streams.
1046-1077: SSE parsing follows spec correctly.Handles comments (
:prefix), multi-linedata:,id:, andevent:fields. The line ending normalization (\r\n→\n) ensures cross-platform compatibility.
1079-1101: Chunk accumulation and delimiter detection look solid.Correctly handles both
\n\nand\r\n\r\ndelimiters, finding the earliest occurrence. The loop ensures all complete blocks are parsed before waiting for more data.
|
@SW881 is attempting to deploy a commit to the Bruno Team on Vercel. A member of the Team first needs to authorize it. |
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
packages/bruno-electron/src/ipc/network/index.js (1)
1029-1040: Early return prevents stream-end notification on missing token.The guard
if (!cancelTokens[response.cancelTokenUid]) return;exits without sendingmain:http-stream-end. While this may prevent duplicate notifications if the token was deleted elsewhere, it also means clients won't receive an end event if the token is missing for any reason.Additionally, the stream still lacks an explicit error handler. Although Node.js streams typically emit 'close' after 'error', this isn't guaranteed for all stream implementations, and errors remain unlogged.
Consider:
- Logging why the close handler exits early
- Adding a stream error handler to ensure errors are logged and cleanup occurs
🔎 Suggested error handler
+ stream.on('error', (err) => { + console.error('HTTP stream error:', err); + if (cancelTokens[response.cancelTokenUid]) { + mainWindow.webContents.send('main:http-stream-end', { + collectionUid, + itemUid: item.uid, + seq: seq + 1, + timestamp: Date.now() + }); + deleteCancelToken(response.cancelTokenUid); + } + }); + stream.on('close', () => { - if (!cancelTokens[response.cancelTokenUid]) return; + if (!cancelTokens[response.cancelTokenUid]) { + console.debug('Stream close: cancel token already cleaned up for', item.uid); + return; + } mainWindow.webContents.send('main:http-stream-end', { collectionUid, itemUid: item.uid, seq: seq + 1, timestamp: Date.now() }); deleteCancelToken(response.cancelTokenUid); });
🧹 Nitpick comments (1)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (1)
403-409: Clarify descending sort order intent.The sort is descending (
sb - sa), placing higher sequence numbers first. While this aligns with the prepend pattern used inwsResponseReceived, consider adding a brief comment explaining the sort direction for future maintainers.🔎 Suggested comment
const startTimestamp = item.requestSent.timestamp; item.response.duration = startTimestamp ? Date.now() - startTimestamp : item.response.duration; if (item.response?.data?.length) { + // Sort descending by seq (newest first) item.response.data.sort((a, b) => { const sa = Number.isFinite(a.seq) ? a.seq : Number.POSITIVE_INFINITY; const sb = Number.isFinite(b.seq) ? b.seq : Number.POSITIVE_INFINITY; return sb - sa; }); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/utils/network/ws-event-listeners.jspackages/bruno-electron/src/ipc/network/index.js
🧰 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-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/utils/network/ws-event-listeners.jspackages/bruno-electron/src/ipc/network/index.js
🧠 Learnings (2)
📚 Learning: 2025-12-17T21:41:24.730Z
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
Applied to files:
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/utils/network/ws-event-listeners.jspackages/bruno-electron/src/ipc/network/index.js
📚 Learning: 2025-12-02T20:48:05.670Z
Learnt from: NikHillAgar
Repo: usebruno/bruno PR: 6279
File: packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js:79-86
Timestamp: 2025-12-02T20:48:05.670Z
Learning: In React 19+, ref callbacks support returning cleanup functions. When a ref callback returns a function, React will call it when the element unmounts or the ref changes, similar to useEffect cleanup. This is documented at https://react.dev/learn/manipulating-the-dom-with-refs#how-to-manage-a-list-of-refs-using-a-ref-callback
Applied to files:
packages/bruno-app/src/utils/network/ws-event-listeners.js
🧬 Code graph analysis (3)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (2)
packages/bruno-app/src/utils/collections/index.js (11)
collection(587-587)collection(1175-1175)collection(1177-1177)collection(1605-1605)collection(1615-1615)findCollectionByUid(80-82)findCollectionByUid(80-82)item(1042-1042)item(1276-1276)findItemInCollection(117-121)findItemInCollection(117-121)packages/bruno-cli/src/utils/collection.js (1)
findItemInCollection(336-340)
packages/bruno-app/src/utils/network/ws-event-listeners.js (1)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (2)
eventData(510-510)eventData(3301-3301)
packages/bruno-electron/src/ipc/network/index.js (2)
packages/bruno-electron/src/utils/common.js (1)
parseDataFromResponse(106-130)packages/bruno-cli/src/utils/common.js (1)
parseDataFromResponse(19-43)
🔇 Additional comments (2)
packages/bruno-electron/src/ipc/network/index.js (1)
1006-1006: LGTM—sequence counter initialization.The sequence counter is correctly initialized at the handler level and will maintain ordering across streaming events.
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (1)
3273-3332: The concern aboutseqis unfounded. Sequence numbers are not expected from WebSocket event handlers—they're generated in the IPC listener layer (packages/bruno-app/src/utils/network/ws-event-listeners.js) before Redux dispatch. All WebSocket events from upstream (message, open, close, error, redirect, upgrade) consistently providetimestamp: Date.now(), which is what the Redux code actually depends on.
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js
Outdated
Show resolved
Hide resolved
|
websockets sequencing test test.3.mp4 |
|
websockets sequencing test with respect to each request test.4.mp4 |
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-app/src/utils/network/ws-event-listeners.js (1)
90-98: CallresetSeq(requestId)in the error handler to prevent Map leak.The error handler at lines 91-98 does not clean up the sequence tracker. In the
wslibrary,'error'and'close'events are independent—an error during connection establishment or after the connection is open will emit an error event without guaranteeing a subsequent close event. This leaves orphaned entries in theseqByRequestRefMap.Add
resetSeq(requestId);after the dispatch in the error handler, similar to the close handler at line 87.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/bruno-app/src/utils/network/ws-event-listeners.js
🧰 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-app/src/utils/network/ws-event-listeners.js
🧠 Learnings (2)
📚 Learning: 2025-12-02T20:48:05.670Z
Learnt from: NikHillAgar
Repo: usebruno/bruno PR: 6279
File: packages/bruno-app/src/components/RequestPane/PromptVariables/PromptVariablesModal/index.js:79-86
Timestamp: 2025-12-02T20:48:05.670Z
Learning: In React 19+, ref callbacks support returning cleanup functions. When a ref callback returns a function, React will call it when the element unmounts or the ref changes, similar to useEffect cleanup. This is documented at https://react.dev/learn/manipulating-the-dom-with-refs#how-to-manage-a-list-of-refs-using-a-ref-callback
Applied to files:
packages/bruno-app/src/utils/network/ws-event-listeners.js
📚 Learning: 2025-12-17T21:41:24.730Z
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
Applied to files:
packages/bruno-app/src/utils/network/ws-event-listeners.js
🧬 Code graph analysis (1)
packages/bruno-app/src/utils/network/ws-event-listeners.js (1)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (2)
eventData(510-510)eventData(3301-3301)
🔇 Additional comments (1)
packages/bruno-app/src/utils/network/ws-event-listeners.js (1)
11-23: Per-request sequencing correctly fixes previous ordering issue.The Map-based sequence tracking per
requestIdproperly addresses the previous concern where a global counter would break message ordering across multiple concurrent connections. Clean implementation using nullish coalescing to initialize missing entries.
sid-bruno
changed the title
`WSMessagesList` already handles a lot of the ordering for us, don't modify the order the messages are added since redirect and connection are internal states, it changes the execution trail
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js
🧰 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-app/src/providers/ReduxStore/slices/collections/index.js
🧠 Learnings (1)
📚 Learning: 2025-12-17T21:41:24.730Z
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
Applied to files:
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js
🧬 Code graph analysis (1)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (4)
packages/bruno-app/src/utils/collections/index.js (11)
collection(587-587)collection(1175-1175)collection(1177-1177)collection(1605-1605)collection(1615-1615)findCollectionByUid(80-82)findCollectionByUid(80-82)item(1042-1042)item(1276-1276)findItemInCollection(117-121)findItemInCollection(117-121)packages/bruno-electron/src/utils/collection.js (2)
item(276-276)findItemInCollection(252-256)packages/bruno-app/src/utils/common/index.js (2)
uuid(15-21)uuid(15-21)packages/bruno-app/src/utils/url/index.js (2)
splitOnFirst(71-82)splitOnFirst(71-82)
🔇 Additional comments (2)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (2)
3149-3157: Good use of immutable prepend pattern.The code creates a new array with the incoming message and concatenates the existing array, which maintains immutability—a best practice for Redux state updates. This is preferable to using
unshift(), which would mutate the existing array.
3285-3286: Onlytimestamprequires a fallback;seqis always provided.Lines 3335-3336 (error) and 3340+ (other types) reference
eventData.timestampwithout a fallback. While most WebSocket events includetimestamp: Date.now()from the main process, theconnectingevent receives noeventData, and some error paths (connection initialization/send failures) omittimestamp. Consider addingtimestamp || Date.now()to ensure consistent data structure, or verify that all callers guarantee a timestamp.
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js
Outdated
Show resolved
Hide resolved
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js
Outdated
Show resolved
Hide resolved
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js
Outdated
Show resolved
Hide resolved
Though we always create an empty data buffer at source so shouldn't happen unless that is modified
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (2)
386-397: Add fallback forseqparameter.The
seqparameter is destructured from the payload and used directly without a fallback. If the action dispatcher doesn't provideseq, it will be stored asundefinedinitem.response.data, potentially breaking downstream code that expects numeric sequence values for sorting.Consider adding a fallback similar to how
timestampis handled, or document thatseqmust always be provided by the caller.🔎 Proposed fix
requestCancelled: (state, action) => { const { itemUid, collectionUid, seq, timestamp } = action.payload; const collection = findCollectionByUid(state.collections, collectionUid); if (collection) { const item = findItemInCollection(collection, itemUid); if (item) { if (item.response?.stream?.running) { item.response.stream.running = null; const startTimestamp = item.requestSent.timestamp; item.response.duration = startTimestamp ? Date.now() - startTimestamp : item.response.duration; - item.response.data = [{ type: 'info', timestamp: Date.now(), seq: seq, message: 'Connection Closed' }].concat(item.response.data); + item.response.data = [{ type: 'info', timestamp: Date.now(), seq: seq ?? 0, message: 'Connection Closed' }].concat(item.response.data); } else { item.response = null; item.requestUid = null; } item.cancelTokenUid = null; item.requestStartTime = null; } } },
3121-3134: Add fallback forseqparameter.Similar to
requestCancelled, theseqparameter is used without a fallback (line 3130). If the action dispatcher doesn't provideseq, it will be stored asundefined, potentially causing issues with downstream sorting or filtering logic.Add a fallback value or ensure
seqis always provided by callers.🔎 Proposed fix
streamDataReceived: (state, action) => { const { itemUid, collectionUid, seq, timestamp, data } = action.payload; const collection = findCollectionByUid(state.collections, collectionUid); if (collection) { const item = findItemInCollection(collection, itemUid); if (data.data) { item.response.data ||= []; item.response.data = [{ type: 'incoming', - seq, + seq: seq ?? 0, message: data.data, messageHexdump: hexdump(data.data), timestamp: timestamp || Date.now() }].concat(item.response.data); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js
🧰 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-app/src/providers/ReduxStore/slices/collections/index.js
🧠 Learnings (2)
📚 Learning: 2025-12-02T07:24:50.311Z
Learnt from: bijin-bruno
Repo: usebruno/bruno PR: 6263
File: packages/bruno-requests/src/auth/oauth2-helper.ts:249-249
Timestamp: 2025-12-02T07:24:50.311Z
Learning: In OAuth2 Basic Auth headers for Bruno, clientSecret is optional and can be omitted. When constructing the Authorization header in `packages/bruno-requests/src/auth/oauth2-helper.ts`, use `clientSecret || ''` instead of `clientSecret!` to properly handle cases where only clientId is provided, per community requests.
Applied to files:
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js
📚 Learning: 2025-12-17T21:41:24.730Z
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
Applied to files:
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js
🧬 Code graph analysis (1)
packages/bruno-app/src/providers/ReduxStore/slices/collections/index.js (1)
packages/bruno-electron/src/app/collection-watcher.js (1)
seq(419-419)
|
@sid-bruno i had bit problem in understanding the websockets can you please on confirm are you trying to say sequencing should be handled in bruno\packages\bruno-requests\src\ws\ws-client.js. i did try adding seq here in #addConnection with this.#addConnection(requestId, collectionUid, wsConnection, seq: 0); but latter moved to reduxStore and function to increase sequence. It hard in understanding class based {} code block so is it somewhere else i need to look would help in understanding repo better |
Yeah, it'll need a little work in ws since all connections are in one client, i'll handle it, i'm also making changes to remove additional stuff that AI might have done so I'll take up the PR since I need it ready before the next release Hope you don't mind, if you'd like you can browse for other issues, the PR will still be merged with your authorship |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-requests/src/ws/ws-client.js (1)
407-411: Add sequence number to 'removed' connection event for consistency.The
#removeConnectionmethod emits a 'connections-changed' event without aseqfield, while#addConnectionincludes it (line 382). For consistent event ordering, both should include sequence numbers.🔎 Proposed fix
+ const connectionMeta = this.activeConnections.get(requestId); if (this.activeConnections.has(requestId)) { this.activeConnections.delete(requestId); // Emit an event with all active connection IDs this.eventCallback('main:ws:connections-changed', { type: 'removed', requestId, + seq: seq.next(requestId, connectionMeta?.collectionUid || ''), activeConnectionIds: this.getActiveConnectionIds() }); }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bruno-app/src/components/ResponsePane/WsResponsePane/WSMessagesList/index.jspackages/bruno-requests/src/ws/ws-client.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-app/src/components/ResponsePane/WsResponsePane/WSMessagesList/index.js
🧰 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-requests/src/ws/ws-client.js
🧠 Learnings (1)
📚 Learning: 2025-12-17T21:41:24.730Z
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
Applied to files:
packages/bruno-requests/src/ws/ws-client.js
🔇 Additional comments (4)
packages/bruno-requests/src/ws/ws-client.js (4)
52-62: LGTM!The
createSequencerclosure-based implementation correctly generates monotonically increasing sequence numbers per (requestId, collectionId) pair using lazy initialization.
195-201: LGTM!Outgoing message event correctly includes sequence number via
seq.next().
300-366: LGTM!All WebSocket event handlers (open, redirect, upgrade, message, close, error) correctly include sequence numbers. The implementation consistently applies
seq.next(requestId, collectionUid)across all event types.
64-64: Module-level sequencing is intentional and correctly designed.The
seqinstance maintains per-request-per-collection sequence counters (seq[requestId][collectionId]), not global counters. Sharing at module level ensures consistent sequence ordering across any WsClient instances for the same request-collection pair, which is the intended behavior.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bruno-app/src/components/ResponsePane/WsResponsePane/WSMessagesList/index.jspackages/bruno-requests/src/ws/ws-client.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-app/src/components/ResponsePane/WsResponsePane/WSMessagesList/index.js
🧰 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-requests/src/ws/ws-client.js
🧠 Learnings (1)
📚 Learning: 2025-12-17T21:41:24.730Z
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
Applied to files:
packages/bruno-requests/src/ws/ws-client.js
🔇 Additional comments (3)
packages/bruno-requests/src/ws/ws-client.js (3)
194-200: LGTM!Sequence number correctly generated for outgoing messages after successful send. This ensures proper ordering in the event stream.
286-366: LGTM!All WebSocket lifecycle events now include sequence numbers. The pattern is consistent across open, redirect, upgrade, message (both parsed and raw fallback), close, and error events. This provides deterministic ordering for fast incoming messages as intended.
374-384: Add sequence number to allconnections-changedevent types for consistency.The
seq.next()call is only used for the 'added' type (line 381), but not for 'cleared' (line 269) or 'removed' (line 406) types. This creates inconsistent event payloads. Add seq to all three variants so the UI can properly track the timeline of all connection state changes.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-requests/src/ws/ws-client.js (1)
233-239: clean() doesn't delete all collections when collectionId is omitted.At line 238,
seq.clean(requestId)is called without acollectionId, intending to clean all sequence state for the request. However, thecleanmethod only deletesseq[requestId]if it's already empty (line 69-71). This leaves sequence data in memory.The fix in the earlier comment for lines 52-78 should also handle this case.
🔎 Updated fix for createSequencer
const clean = (requestId, collectionId = undefined) => { + if (!seq[requestId]) { + return; + } + if (collectionId) { delete seq[requestId][collectionId]; + if (!Object.keys(seq[requestId]).length) { + delete seq[requestId]; + } + } else { + delete seq[requestId]; } - if (!Object.keys(seq[requestId]).length) { - delete seq[requestId]; - } };
🧹 Nitpick comments (4)
packages/bruno-requests/src/ws/ws-client.js (4)
12-23: Remove unused helper or apply it to line 346.The
safeParseJSONhelper is defined but never called. The message handler at line 346 still uses plainJSON.parse(data.toString()).Either remove this dead code or replace the JSON.parse call at line 346 with
safeParseJSON(data.toString(), 'WebSocket message')to get the improved error context.🔎 Option 1: Use the helper
- try { - const message = JSON.parse(data.toString()); + try { + const message = safeParseJSON(data.toString(), 'WebSocket message');🔎 Option 2: Remove dead code
-/** - * Safely parse JSON string with error handling - * @param {string} jsonString - The JSON string to parse - * @param {string} context - Context for error messages - * @returns {Object} Parsed object or throws error with context - * @throws {Error} If JSON parsing fails - */ -const safeParseJSON = (jsonString, context = 'JSON string') => { - try { - return JSON.parse(jsonString); - } catch (error) { - const errorMessage = `Failed to parse ${context}: ${error.message}`; - console.error(errorMessage, { - originalString: jsonString, - parseError: error - }); - throw new Error(errorMessage); - } -}; -
156-158: Consider adding seq/timestamp to error events for consistency.Several error callbacks don't include
seqandtimestampfields, unlike most other events:
- Line 156: error in
startConnectioncatch block- Line 207: error in
sendMessagecallback- Line 221: error when connection not available
While these errors may occur before a connection is fully established, adding a sequence number (if available) would maintain consistency with other event emissions like line 380.
Also applies to: 207-207, 221-223
424-428: Add seq to connections-changed event for consistency.The
connections-changedevent in#removeConnectiondoesn't include aseqfield, but#addConnection(line 399) does. This inconsistency makes it harder for clients to maintain a correctly ordered event timeline.🔎 Proposed fix
Note: You'll need to pass
collectionUidto#removeConnectionto generate the sequence number, or retrieve it from the connection metadata before deletion.#removeConnection(requestId) { + const meta = this.activeConnections.get(requestId); + if (this.connectionKeepAlive.has(requestId)) { clearInterval(this.connectionKeepAlive.get(requestId)); this.connectionKeepAlive.delete(requestId); } const mqId = this.#getMessageQueueId(requestId); if (mqId in this.messageQueues) { this.messageQueues[mqId] = []; } if (this.activeConnections.has(requestId)) { this.activeConnections.delete(requestId); // Emit an event with all active connection IDs this.eventCallback('main:ws:connections-changed', { type: 'removed', requestId, + seq: meta?.collectionUid ? seq.next(requestId, meta.collectionUid) : undefined, activeConnectionIds: this.getActiveConnectionIds() }); } }
286-289: Consider seq for connections-changed 'cleared' event.Similar to the inconsistency in
#removeConnection, theconnections-changedevent when clearing all connections doesn't include aseqfield.Since this event affects multiple connections, you may choose to omit
seqhere (as it's ambiguous which connection to sequence against) or emit individual sequenced events per connection during the clear operation.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/bruno-requests/src/ws/ws-client.js
🧰 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-requests/src/ws/ws-client.js
🧠 Learnings (1)
📚 Learning: 2025-12-17T21:41:24.730Z
Learnt from: naman-bruno
Repo: usebruno/bruno PR: 6407
File: packages/bruno-app/src/components/Environments/ConfirmCloseEnvironment/index.js:5-41
Timestamp: 2025-12-17T21:41:24.730Z
Learning: Do not suggest PropTypes validation for React components in the Bruno codebase. The project does not use PropTypes, so reviews should avoid proposing PropTypes and rely on the existing typing/validation approach (e.g., TypeScript or alternative runtime checks) if applicable. This guideline applies broadly to all JavaScript/JSX components in the repo.
Applied to files:
packages/bruno-requests/src/ws/ws-client.js
| const createSequencer = () => { | ||
| const seq = {}; | ||
|
|
||
| const nextSeq = (requestId, collectionId) => { | ||
| seq[requestId] ||= {}; | ||
| seq[requestId][collectionId] ||= 0; | ||
| return ++seq[requestId][collectionId]; | ||
| }; | ||
|
|
||
| /** | ||
| * @param {string} requestId | ||
| * @param {string} [collectionId] | ||
| */ | ||
| const clean = (requestId, collectionId = undefined) => { | ||
| if (collectionId) { | ||
| delete seq[requestId][collectionId]; | ||
| } | ||
| if (!Object.keys(seq[requestId]).length) { | ||
| delete seq[requestId]; | ||
| } | ||
| }; | ||
|
|
||
| return { | ||
| next: nextSeq, | ||
| clean | ||
| }; | ||
| }; |
There was a problem hiding this comment.
Add guard checks to prevent runtime errors in clean().
The clean method will throw if seq[requestId] is undefined. This can happen if close() is called on a connection that failed before emitting any events (since seq[requestId] is only lazily initialized in nextSeq).
🔎 Proposed fix
const clean = (requestId, collectionId = undefined) => {
+ if (!seq[requestId]) {
+ return;
+ }
+
if (collectionId) {
delete seq[requestId][collectionId];
}
+
if (!Object.keys(seq[requestId]).length) {
delete seq[requestId];
}
};🤖 Prompt for AI Agents
In packages/bruno-requests/src/ws/ws-client.js around lines 52 to 78, the
clean() function can throw when seq[requestId] is undefined; add guard checks to
exit early if seq[requestId] is missing, only attempt to delete a collection key
if the request entry exists, and only run Object.keys(...) when seq[requestId]
is defined — i.e. at the top of clean return immediately if !seq[requestId],
then if collectionId delete seq[requestId][collectionId] (or check existence
first), and finally if Object.keys(seq[requestId]).length === 0 delete
seq[requestId].
2 participants
Description
Contribution Checklist:
This PR addresses issue : #6513
When working with super fast SSE responses, the chunks might get re-ordered since they all might have the same timestamp and the delay between electron's ipc and the client might cause the chunks to get re-ordered.
Sequencing for this needs to be hardened at the electron level so that the client can sort it based on the hardened sequence. This impacts both Websocket and SSE in fast incoming messages.
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
Bug Fixes
New Features
✏️ Tip: You can customize this high-level summary in your review settings.