bruno icon indicating copy to clipboard operation
bruno copied to clipboard

feat: decode msgpack response type to JSON

Open ryanshepps opened this issue 7 months ago • 2 comments

Fixes: #372

Description

Decodes a msgpack response to JSON.

Useful links:

  • https://msgpack.org/
  • https://www.npmjs.com/package/@msgpack/msgpack

Open to any comments/suggestions.

Contribution Checklist:

  • [x] The pull request only addresses one issue or adds one feature.
  • [x] The pull request does not introduce any breaking changes
  • [x] I have added screenshots or gifs to help explain the change if applicable.
  • [x] I have read the contribution guidelines.
  • [x] 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.

ryanshepps avatar May 18 '25 17:05 ryanshepps

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 avatar May 19 '25 08:05 helloanoop

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 1b9abf00d301502c851971930155b5c38a497948. 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 avatar May 20 '25 12:05 ryanshepps

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

ryanshepps avatar May 27 '25 11:05 ryanshepps

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

helloanoop avatar May 27 '25 22:05 helloanoop

Just wanted to bump this!

asinghbrar avatar Aug 15 '25 14:08 asinghbrar

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

helloanoop avatar Oct 18 '25 17:10 helloanoop

[!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

  • usebruno/bruno#6395 — changes to response content-type detection/formatting pipeline; likely overlaps with mode detection and rendering adjustments.

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 8b207a5fc924e44dc063e1f2193af6d74f92ecad and 0fdfab432a0160ad669e44b48d72ffbbc489ca52.

⛔ 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., <svg xmlns="..." viewBox="...">) 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.

coderabbitai[bot] avatar Dec 18 '25 14:12 coderabbitai[bot]

@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!

ryanshepps avatar Dec 18 '25 14:12 ryanshepps