Skip to content

Conversation

@ryanshepps
Copy link

@ryanshepps ryanshepps commented May 18, 2025

Fixes: #372

Description

Decodes a msgpack response to JSON.

Useful links:

Open to any comments/suggestions.

Contribution Checklist:

  • The pull request only addresses one issue or adds one feature.
  • The pull request does not introduce any breaking changes
  • I have added screenshots or gifs to help explain the change if applicable.
  • I have read the contribution guidelines.
  • Create an issue and link to the pull request.

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

  • New Features
    • MessagePack responses are decoded and shown as formatted JSON.
  • Bug Fixes / Improvements
    • Invalid MessagePack now falls back to raw/UTF‑8 display.
    • Improved runtime compatibility for MessagePack decoding in the app and test environment.
  • Tests
    • Added unit tests covering valid MessagePack decoding, invalid data fallback, and large-response handling.

✏️ Tip: You can customize this high-level summary in your review settings.

@helloanoop
Copy link
Contributor

This looks good @ryanshepps !

Would you mind also adding a unit test to verify the formatResponse behavior with msgpack? You can create a new test file at: packages/bruno-app/src/components/ResponsePane/QueryResult/index.spec.js

To enable testing, please export the formatResponse function like so:
export const formatResponse = (data, dataBuffer, encoding, mode, filter) => {

@pull-request-size pull-request-size bot added size/L and removed size/S labels May 20, 2025
@ryanshepps
Copy link
Author

This looks good @ryanshepps !

Would you mind also adding a unit test to verify the formatResponse behavior with msgpack? You can create a new test file at: packages/bruno-app/src/components/ResponsePane/QueryResult/index.spec.js

To enable testing, please export the formatResponse function like so: export const formatResponse = (data, dataBuffer, encoding, mode, filter) => {

@helloanoop Added unit tests in 1b9abf0. Note that Jest was giving me trouble when testing formatResponse from index.js since it has jsx in it, so I moved formatResponse into a utils.js file.

Let me know if this doesn't fit the rest of the codebase. Feel free to make/request changes.

@ryanshepps
Copy link
Author

@helloanoop Any requested changes for this? Sorry, I know you're busy.

Its-treason added a commit to Its-treason/bruno that referenced this pull request May 27, 2025
helloanoop
helloanoop previously approved these changes May 27, 2025
@helloanoop
Copy link
Contributor

Thank you @ryanshepps !
We should have this merged over the next few days.

@asinghbrar
Copy link

Just wanted to bump this!

@helloanoop
Copy link
Contributor

@ryanshepps Sorry, lost track of this. Do you mind fixing the conflicts, I'll merge once you do.

Copilot AI review requested due to automatic review settings December 18, 2025 14:19
@ryanshepps ryanshepps force-pushed the feature/decode-msgpack-response branch from 1b9abf0 to 8b207a5 Compare December 18, 2025 14:19
@pull-request-size pull-request-size bot added size/M and removed size/L labels Dec 18, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 18, 2025

Note

Other AI code review bot(s) detected

CodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review.

Walkthrough

Adds msgpack decoding support: dependency, test polyfills, content-type → CodeMirror mode mapping, formatResponse decoding branch (base64→msgpack→JSON) with UTF-8 fallback, and tests for valid/invalid msgpack payloads.

Changes

Cohort / File(s) Summary
Jest configuration
packages/bruno-app/jest.setup.js
Adds TextEncoder/TextDecoder polyfill via util for jsdom and a nanoid mock.
Dependency
packages/bruno-app/package.json
Adds dependency @msgpack/msgpack ^3.1.1.
CodeMirror mode mapping
packages/bruno-app/src/utils/common/codemirror.js
Maps content types containing "msgpack" to 'application/msgpack' mode.
Msgpack decoding logic
packages/bruno-app/src/utils/common/index.js
Imports decode from @msgpack/msgpack; adds msgpack handling in formatResponse to decode base64 msgpack, stringify decoded object, and fallback to UTF‑8/raw on decode errors (logs warning).
Tests
packages/bruno-app/src/utils/common/format-response.spec.js
Adds "msgpack mode" test suite covering successful decode, invalid msgpack fallback, and large-response UTF‑8 fallback behavior.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

  • Inspect formatResponse msgpack branch for correct Buffer usage and safe stringification (edge cases like typed arrays, nested binary).
  • Verify tests accurately cover fallback paths and that jest polyfills/mock won't mask real runtime issues.
  • Confirm package.json dependency addition matches lockfile / build tooling expectations.

Possibly related PRs

Suggested reviewers

  • lohit-bruno
  • naman-bruno
  • helloanoop
  • bijin-bruno

Poem

📦 A tiny pack of bytes came through,
Bruno woke up and read it true.
Base64 unwrapped, JSON in view,
No blank screen—just data anew. ✨

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately and concisely describes the main feature: adding msgpack response decoding to JSON format, matching the primary objective of the PR.
Linked Issues check ✅ Passed All coding requirements from #372 are met: msgpack detection via Content-Type handling, decoding to JSON via @msgpack/msgpack library, and fallback error handling for invalid data.
Out of Scope Changes check ✅ Passed All changes are scoped to msgpack support: jest setup for compatibility, dependency addition, CodeMirror mode detection, and response formatting logic with comprehensive tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 8b207a5 and 0fdfab4.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (5)
  • packages/bruno-app/jest.setup.js (1 hunks)
  • packages/bruno-app/package.json (1 hunks)
  • packages/bruno-app/src/utils/common/codemirror.js (1 hunks)
  • packages/bruno-app/src/utils/common/format-response.spec.js (1 hunks)
  • packages/bruno-app/src/utils/common/index.js (2 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • packages/bruno-app/package.json
  • packages/bruno-app/jest.setup.js
  • packages/bruno-app/src/utils/common/codemirror.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() not func ()
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/common/index.js
  • packages/bruno-app/src/utils/common/format-response.spec.js
🧠 Learnings (3)
📚 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/common/index.js
  • packages/bruno-app/src/utils/common/format-response.spec.js
📚 Learning: 2025-12-05T20:31:33.005Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.test.{js,jsx,ts,tsx} : Add tests for any new functionality or meaningful changes. If code is added, removed, or significantly modified, corresponding tests should be updated or created

Applied to files:

  • packages/bruno-app/src/utils/common/format-response.spec.js
📚 Learning: 2025-12-16T07:16:08.934Z
Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 6090
File: tests/scripting/hooks/init-user-data/ui-state-snapshot.json:1-8
Timestamp: 2025-12-16T07:16:08.934Z
Learning: For e2e tests in the bruno repository: Collections that are shared between CLI and UI tests (comprehensive test suites testing core functionality) should be placed in `packages/bruno-tests/` to avoid duplication. The `tests/**/fixtures/collection` pattern should be used for test-specific collections that test particular UI behaviors or are specific to a single test file.

Applied to files:

  • packages/bruno-app/src/utils/common/format-response.spec.js
🧬 Code graph analysis (1)
packages/bruno-app/src/utils/common/format-response.spec.js (1)
packages/bruno-app/src/utils/common/index.js (4)
  • dataBuffer (285-285)
  • dataBuffer (405-405)
  • formatResponse (278-450)
  • formatResponse (278-450)
🔇 Additional comments (2)
packages/bruno-app/src/utils/common/index.js (1)

9-9: LGTM!

The import is correct and follows the project's coding conventions.

packages/bruno-app/src/utils/common/format-response.spec.js (1)

103-109: LGTM!

This test correctly validates the happy path for valid msgpack decoding and formatted output.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
packages/bruno-app/src/utils/common/format-response.spec.js (1)

102-116: LGTM!

The msgpack tests cover both successful decoding and error handling paths. The test data is well-constructed using actual msgpack binary format.

Consider adding a test case for large msgpack responses to verify the behavior when the buffer exceeds the threshold, similar to the existing tests for JSON and XML modes.

📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between aac219d and 8b207a5.

⛔ Files ignored due to path filters (1)
  • package-lock.json is excluded by !**/package-lock.json
📒 Files selected for processing (5)
  • packages/bruno-app/jest.setup.js (1 hunks)
  • packages/bruno-app/package.json (1 hunks)
  • packages/bruno-app/src/utils/common/codemirror.js (1 hunks)
  • packages/bruno-app/src/utils/common/format-response.spec.js (1 hunks)
  • packages/bruno-app/src/utils/common/index.js (2 hunks)
🧰 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() not func ()
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/common/codemirror.js
  • packages/bruno-app/src/utils/common/index.js
  • packages/bruno-app/jest.setup.js
  • packages/bruno-app/src/utils/common/format-response.spec.js
🧠 Learnings (3)
📚 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/common/codemirror.js
  • packages/bruno-app/src/utils/common/index.js
  • packages/bruno-app/jest.setup.js
  • packages/bruno-app/src/utils/common/format-response.spec.js
📚 Learning: 2025-12-05T20:31:33.005Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-05T20:31:33.005Z
Learning: Applies to **/*.test.{js,jsx,ts,tsx} : Add tests for any new functionality or meaningful changes. If code is added, removed, or significantly modified, corresponding tests should be updated or created

Applied to files:

  • packages/bruno-app/src/utils/common/format-response.spec.js
📚 Learning: 2025-12-16T07:16:08.934Z
Learnt from: sanish-bruno
Repo: usebruno/bruno PR: 6090
File: tests/scripting/hooks/init-user-data/ui-state-snapshot.json:1-8
Timestamp: 2025-12-16T07:16:08.934Z
Learning: For e2e tests in the bruno repository: Collections that are shared between CLI and UI tests (comprehensive test suites testing core functionality) should be placed in `packages/bruno-tests/` to avoid duplication. The `tests/**/fixtures/collection` pattern should be used for test-specific collections that test particular UI behaviors or are specific to a single test file.

Applied to files:

  • packages/bruno-app/src/utils/common/format-response.spec.js
🧬 Code graph analysis (2)
packages/bruno-app/src/utils/common/codemirror.js (2)
packages/bruno-app/src/utils/common/index.js (1)
  • contentType (98-98)
packages/bruno-app/src/components/ResponsePane/QueryResult/index.js (1)
  • contentType (102-102)
packages/bruno-app/src/utils/common/format-response.spec.js (1)
packages/bruno-app/src/utils/common/index.js (4)
  • dataBuffer (285-285)
  • dataBuffer (399-399)
  • formatResponse (278-444)
  • formatResponse (278-444)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
  • GitHub Check: Prepare
🔇 Additional comments (4)
packages/bruno-app/src/utils/common/codemirror.js (1)

120-121: LGTM!

The msgpack content-type handling follows the existing pattern and integrates cleanly into the mode detection logic.

packages/bruno-app/jest.setup.js (1)

1-4: LGTM!

The TextEncoder/TextDecoder polyfill is necessary for @msgpack/msgpack to work in the jsdom test environment. The implementation is correct.

packages/bruno-app/src/utils/common/index.js (1)

9-9: LGTM!

The import follows the established pattern for other formatters in this file.

packages/bruno-app/package.json (1)

16-16: @msgpack/msgpack@3.1.1 exists and has no known vulnerabilities.

The package has been scanned for malware, tampering, risky behaviors, and known vulnerabilities, with no risks detected. The high-severity vulnerability CVE-2021-23410 affects the msgpack package only, not @msgpack/msgpack. The dependency is safe to use.

Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR adds msgpack response decoding functionality to the Bruno app, allowing users to decode and view msgpack-formatted API responses as JSON. The implementation uses the @msgpack/msgpack library (v3.1.1) to decode binary msgpack data and display it in a formatted, readable JSON structure.

Key changes:

  • Integrated msgpack decoding into the response formatter with automatic base64-to-msgpack-to-JSON conversion
  • Added CodeMirror content type detection for msgpack responses
  • Included Jest polyfills (TextEncoder/TextDecoder) required for testing msgpack functionality in jsdom environment

Reviewed changes

Copilot reviewed 5 out of 6 changed files in this pull request and generated 3 comments.

Show a summary per file
File Description
packages/bruno-app/src/utils/common/index.js Added msgpack decoding logic to formatResponse function with error handling
packages/bruno-app/src/utils/common/format-response.spec.js Added test cases for valid and invalid msgpack data decoding
packages/bruno-app/src/utils/common/codemirror.js Added msgpack content type detection for proper syntax highlighting mode
packages/bruno-app/package.json Added @msgpack/msgpack v3.1.1 dependency
packages/bruno-app/jest.setup.js Added TextEncoder/TextDecoder polyfills for msgpack library compatibility in Jest tests
package-lock.json Locked @msgpack/msgpack dependency version

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

@ryanshepps ryanshepps force-pushed the feature/decode-msgpack-response branch from 8b207a5 to 0fdfab4 Compare December 18, 2025 14:42
@ryanshepps
Copy link
Author

@ryanshepps Sorry, lost track of this. Do you mind fixing the conflicts, I'll merge once you do.

Hi @helloanoop, apologies for the lateness on this. I've fixed the conflicts and also added a test for decoding large invalid responses (suggested by CodeRabbit). Let me know if you would like any other changes. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug/Feature] Allow msgpack as response type (de-serialize to JSON)

3 participants