fix(ws): remove default '{}' message body and make all message headers sticky#8565
Conversation
pooja-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno,
sid-bruno,
utkarsh-bruno and
vijayh-bruno
as code owners
pooja-bruno
changed the title
WalkthroughWebSocket message serialization now emits empty content instead of ChangesWebSocket message defaults
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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.
🧹 Nitpick comments (2)
packages/bruno-lang/v2/src/jsonToBru.js (1)
663-663: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a unit test for the empty-content fallback.
The existing
jsonToBru.spec.jssmoke test only covers non-empty content ({"foo":"bar"}). A unit test asserting that falsycontentserializes to an emptycontent: ''' ... '''block would lock in this behavioral change at the serialization layer.🤖 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-lang/v2/src/jsonToBru.js` at line 663, Add a unit test in jsonToBru.spec.js covering the serialization path in jsonToBru where content is falsy, such as an empty string or null. Assert that the generated Bru output contains an empty content: ''' ... ''' block, while preserving the existing non-empty content coverage.tests/websockets/message-body-default.spec.ts (1)
27-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove inline
.CodeMirrorCSS selectors to the page module.The spec uses inline CSS selectors (
.CodeMirror,.CodeMirror-placeholder,.CodeMirror-code) directly in the test body. Per path instructions, selectors should be single-sourced in page modules undertests/utils/page/*rather than inline in specs. Consider adding acodeEditorlocator (e.g.,editor: (index: number) => ws.message.body(index).locator('.CodeMirror')) and placeholder/code-content sub-locators tobuildWebsocketCommonLocatorsso the spec destructures them instead of chaining raw CSS classes.♻️ Suggested page-module additions
// in buildWebsocketCommonLocators, inside the message object: body: (index: number) => page.getByTestId(`ws-message-body-${index}`), + editor: (index: number) => page.getByTestId(`ws-message-body-${index}`).locator('.CodeMirror'), + editorPlaceholder: (index: number) => page.getByTestId(`ws-message-body-${index}`).locator('.CodeMirror-placeholder'), + editorCode: (index: number) => page.getByTestId(`ws-message-body-${index}`).locator('.CodeMirror-code'),Then in the spec:
- const editor = newBody.locator('.CodeMirror'); - await expect(editor.locator('.CodeMirror-placeholder')).toHaveText('...'); - await expect(editor.locator('.CodeMirror-code')).not.toContainText('{}'); + await expect(ws.message.editorPlaceholder(beforeCount)).toHaveText('...'); + await expect(ws.message.editorCode(beforeCount)).not.toContainText('{}');Also applies to: 46-50
🤖 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 `@tests/websockets/message-body-default.spec.ts` around lines 27 - 31, Move all inline CodeMirror selectors from the test into the websocket page-module locators. Extend buildWebsocketCommonLocators with an editor locator and placeholder/code-content sub-locators, then update the spec to destructure and use those shared locators instead of directly referencing .CodeMirror, .CodeMirror-placeholder, or .CodeMirror-code.Source: Path instructions
🤖 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.
Nitpick comments:
In `@packages/bruno-lang/v2/src/jsonToBru.js`:
- Line 663: Add a unit test in jsonToBru.spec.js covering the serialization path
in jsonToBru where content is falsy, such as an empty string or null. Assert
that the generated Bru output contains an empty content: ''' ... ''' block,
while preserving the existing non-empty content coverage.
In `@tests/websockets/message-body-default.spec.ts`:
- Around line 27-31: Move all inline CodeMirror selectors from the test into the
websocket page-module locators. Extend buildWebsocketCommonLocators with an
editor locator and placeholder/code-content sub-locators, then update the spec
to destructure and use those shared locators instead of directly referencing
.CodeMirror, .CodeMirror-placeholder, or .CodeMirror-code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d91e6b35-cf00-4286-a9c9-268526a78a05
📒 Files selected for processing (5)
packages/bruno-app/src/components/RequestPane/WsBody/SingleWSMessage/StyledWrapper.jspackages/bruno-lang/v2/src/jsonToBru.jstests/utils/page/actions.tstests/utils/page/locators.tstests/websockets/message-body-default.spec.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-lang/v2/tests/jsonToBru.spec.js (1)
56-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering other falsy content values.
The upstream serialization uses
content || '', sonull,undefined,0, andfalseall serialize identically to''. Adding a quick parameterized case fornullandundefinedwould strengthen confidence that the{}fallback removal holds across all falsy inputs, not just empty strings.🤖 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-lang/v2/tests/jsonToBru.spec.js` around lines 56 - 73, Add parameterized test cases in the existing “serializes falsy content to an empty content block” test in jsonToBru.spec.js for null and undefined content, asserting each serializes as an empty content block and does not emit '{}'.
🤖 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.
Nitpick comments:
In `@packages/bruno-lang/v2/tests/jsonToBru.spec.js`:
- Around line 56-73: Add parameterized test cases in the existing “serializes
falsy content to an empty content block” test in jsonToBru.spec.js for null and
undefined content, asserting each serializes as an empty content block and does
not emit '{}'.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a9793950-8f6e-4873-b367-c1352d4c7f0d
📒 Files selected for processing (3)
packages/bruno-lang/v2/tests/jsonToBru.spec.jstests/utils/page/locators.tstests/websockets/message-body-default.spec.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/utils/page/locators.ts
- tests/websockets/message-body-default.spec.ts
3 participants
Description
JIRA
Empty message body no longer defaults to
{}.Sticky headers for all messages. Only the selected message's header stuck to the top when scrolling; now every message header is sticky.
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
Summary by CodeRabbit
Bug Fixes
body:wscontent now defaults to an empty payload (no{}) when content is missing/falsy.Tests