fix(cli): preserve request item type during import and fail on unsupported types#7207
Merged
bijin-bruno merged 7 commits intousebruno:mainfrom Feb 19, 2026
Merged
Conversation
rameshsunkara
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughPreserve original Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-cli/src/utils/collection.js (1)
518-521: Nit: redundant intermediate variable.
const type = item.typeis only referenced once at line 521. You could use shorthand property syntax directly.Suggested simplification
- // Keep schema item type so filestore can stringify request correctly - const type = item.type; const bruJson = { - type: type, + type: item.type, name: item.name,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-cli/src/utils/collection.js` around lines 518 - 521, Remove the redundant intermediate variable by deleting "const type = item.type" and assign the property directly in the bruJson object (use item.type when constructing bruJson) so the code still preserves the "type" property for filestore without an unused local variable; update references to the symbols item, bruJson, and type accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/bruno-cli/src/utils/collection.js`:
- Around line 518-521: Remove the redundant intermediate variable by deleting
"const type = item.type" and assign the property directly in the bruJson object
(use item.type when constructing bruJson) so the code still preserves the "type"
property for filestore without an unused local variable; update references to
the symbols item, bruJson, and type accordingly.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
packages/bruno-cli/src/utils/collection.js (2)
518-521: Optional: inlineitem.typedirectly or use ES6 shorthand.
const type = item.typeis a one-use binding; the comment on line 518 already explains the intent clearly.♻️ Simplification
- // Keep schema item type so filestore can stringify request correctly - const type = item.type; const bruJson = { - type: type, + type: item.type,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-cli/src/utils/collection.js` around lines 518 - 521, The local one-use binding "const type = item.type" is unnecessary; update the construction of bruJson in the function that builds schema items to either inline item.type (e.g., set type: item.type) or use ES6 object shorthand by destructuring { type } = item and then using { type, ... } so the comment's intent remains but the redundant binding is removed; adjust references to "type" inside the creation of bruJson accordingly.
503-503: Remove unnecessaryawaiton synchronous functions.Both
stringifyFolder()andstringifyRequest()return plainstring, notPromise<string>. Theawaitat lines 503 and 542 is redundant and misrepresents the API as async.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-cli/src/utils/collection.js` at line 503, The code is using await on functions that return plain strings; remove the unnecessary await keywords when calling stringifyFolder and stringifyRequest (e.g., the call assigning folderContent and the call assigning requestContent), so these calls treat returned values as synchronous strings rather than Promises; update the call sites in packages/bruno-cli/src/utils/collection.js to simply assign the return values without await and ensure the surrounding function does not incorrectly rely on them being asynchronous.packages/bruno-cli/tests/utils/collection/create-collection-from-bruno-object.spec.js (1)
17-69: Consider splitting the combined HTTP + GraphQL assertion block into separateitcases.When the
http-requestwrite/parse fails, thegraphql-requestassertions never execute. Two focused tests provide clearer failure messages and independent signal.♻️ Suggested split
- it('writes http and graphql requests from imported collection items', async () => { + it('writes an http-request item and parses the correct type and method', async () => { outputDir = fs.mkdtempSync(path.join(os.tmpdir(), 'bruno-cli-import-')); await createCollectionFromBrunoObject( { name: 'imported-collection', items: [ { type: 'http-request', name: 'Get Users', filename: 'get-users.bru', seq: 1, request: { method: 'GET', url: 'https://api.example.com/users' } } ] }, outputDir ); const httpPath = path.join(outputDir, 'get-users.bru'); expect(fs.existsSync(httpPath)).toBe(true); const httpRequest = parseRequest(fs.readFileSync(httpPath, 'utf8'), { format: 'bru' }); expect(httpRequest).toHaveProperty('type', 'http-request'); expect(httpRequest).toHaveProperty('request.method', 'GET'); }); + it('writes a graphql-request item and parses the correct type and method', async () => { + outputDir = fs.mkdtempSync(path.join(os.tmpdir(), 'bruno-cli-import-')); + + await createCollectionFromBrunoObject( + { + name: 'imported-collection', + items: [ + { + type: 'graphql-request', + name: 'Get Viewer', + filename: 'get-viewer.bru', + seq: 1, + request: { + method: 'POST', + url: 'https://api.example.com/graphql', + body: { mode: 'graphql', graphql: { query: 'query { viewer { id } }', variables: '{}' } } + } + } + ] + }, + outputDir + ); + + const graphqlPath = path.join(outputDir, 'get-viewer.bru'); + expect(fs.existsSync(graphqlPath)).toBe(true); + const graphqlRequest = parseRequest(fs.readFileSync(graphqlPath, 'utf8'), { format: 'bru' }); + expect(graphqlRequest).toHaveProperty('type', 'graphql-request'); + expect(graphqlRequest).toHaveProperty('request.method', 'POST'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-cli/tests/utils/collection/create-collection-from-bruno-object.spec.js` around lines 17 - 69, Split the combined test into two focused tests so failures are isolated: keep using createCollectionFromBrunoObject and parseRequest but create one it block that only writes/parses the http-request (asserts exists and request.method === 'GET' for get-users.bru) and a separate it block that only writes/parses the graphql-request (asserts exists and request.method === 'POST' for get-viewer.bru); each test should create its own temporary outputDir with fs.mkdtempSync and call createCollectionFromBrunoObject with only the relevant item to avoid cross-dependencies and ensure clear failure signals.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@packages/bruno-cli/src/utils/collection.js`:
- Around line 518-521: The local one-use binding "const type = item.type" is
unnecessary; update the construction of bruJson in the function that builds
schema items to either inline item.type (e.g., set type: item.type) or use ES6
object shorthand by destructuring { type } = item and then using { type, ... }
so the comment's intent remains but the redundant binding is removed; adjust
references to "type" inside the creation of bruJson accordingly.
- Line 503: The code is using await on functions that return plain strings;
remove the unnecessary await keywords when calling stringifyFolder and
stringifyRequest (e.g., the call assigning folderContent and the call assigning
requestContent), so these calls treat returned values as synchronous strings
rather than Promises; update the call sites in
packages/bruno-cli/src/utils/collection.js to simply assign the return values
without await and ensure the surrounding function does not incorrectly rely on
them being asynchronous.
In
`@packages/bruno-cli/tests/utils/collection/create-collection-from-bruno-object.spec.js`:
- Around line 17-69: Split the combined test into two focused tests so failures
are isolated: keep using createCollectionFromBrunoObject and parseRequest but
create one it block that only writes/parses the http-request (asserts exists and
request.method === 'GET' for get-users.bru) and a separate it block that only
writes/parses the graphql-request (asserts exists and request.method === 'POST'
for get-viewer.bru); each test should create its own temporary outputDir with
fs.mkdtempSync and call createCollectionFromBrunoObject with only the relevant
item to avoid cross-dependencies and ensure clear failure signals.
Member
sanjaikumar-bruno
left a comment
sanjaikumar-bruno
left a comment
There was a problem hiding this comment.
To keep this PR focused on the main fix, could we remove the other unrelated changes other than these?
@@ -499,7 +499,7 @@ const processCollectionItems = async (items = [], currentPath) => {
if (item.seq) {
item.root.meta.seq = item.seq;
}
- const folderContent = await stringifyFolder(item.root);
+ const folderContent = await stringifyFolder(item.root, { format: 'bru' });
safeWriteFileSync(folderBruFilePath, folderContent);
}
@@ -538,7 +538,7 @@ const processCollectionItems = async (items = [], currentPath) => {
};
// Convert to BRU format and write to file
- const content = await stringifyRequest(bruJson);
+ const content = await stringifyRequest(bruJson, { format: 'bru' });
safeWriteFileSync(path.join(currentPath, sanitizedFilename), content);
}
}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-cli/src/utils/collection.js (1)
488-543:⚠️ Potential issue | 🟠 MajorMissing fail-fast
elsebranch for unsupported item types.The PR objective explicitly states "adds fail-fast handling for unsupported item types in the import writer path", but
processCollectionItemssilently skips items whosetypeisn't'folder','http-request', or'graphql-request'. Any future or unexpected item type is dropped without notice, causing silent data loss during import.🐛 Proposed fix — throw on unsupported types
} else if (['http-request', 'graphql-request'].includes(item.type)) { // ... request handling safeWriteFileSync(path.join(currentPath, sanitizedFilename), content); + } else { + throw new Error(`Unsupported item type: ${item.type}`); }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-cli/src/utils/collection.js` around lines 488 - 543, The function processCollectionItems currently ignores items whose item.type isn't 'folder' or request types; add a fail-fast else branch after the existing else-if that throws a clear Error when an unsupported item type is encountered (include item.type and item.name and currentPath in the message) so imports don't silently drop unknown types; update any callers/tests if they rely on silent skipping.
🧹 Nitpick comments (2)
packages/bruno-cli/tests/utils/collection/create-collection-from-bruno-object.spec.js (1)
17-69: Two test scenarios stated in the PR objectives are missing.Per the PR's own stated objectives, regression tests should also verify (1) writing/reading
folder.bruand (2) throwing on unsupported item types. Neither case is covered. As per coding guidelines, tests should cover both happy-path and the realistically problematic paths.♻️ Suggested additional test cases
+ it('writes a folder.bru file when folder root has a name', async () => { + outputDir = fs.mkdtempSync(path.join(os.tmpdir(), 'bruno-cli-import-')); + + await createCollectionFromBrunoObject( + { + name: 'imported-collection', + items: [ + { + type: 'folder', + name: 'users', + root: { meta: { name: 'users' } }, + items: [] + } + ] + }, + outputDir + ); + + const folderBruPath = path.join(outputDir, 'users', 'folder.bru'); + expect(fs.existsSync(folderBruPath)).toBe(true); + }); + + it('throws on unsupported item types', async () => { + outputDir = fs.mkdtempSync(path.join(os.tmpdir(), 'bruno-cli-import-')); + + await expect( + createCollectionFromBrunoObject( + { + name: 'imported-collection', + items: [{ type: 'unknown-type', name: 'Bad Item' }] + }, + outputDir + ) + ).rejects.toThrow('Unsupported item type: unknown-type'); + });Based on learnings from
CODING_STANDARDS.md: "Cover both the 'happy path' and the realistically problematic paths. Validate expected success behaviour, but also validate error handling, edge cases, and degraded-mode behaviour."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-cli/tests/utils/collection/create-collection-from-bruno-object.spec.js` around lines 17 - 69, Add two tests to the existing spec: (1) a test that passes a collection with at least one folder item (type: 'folder', name, seq, items: [...]) to createCollectionFromBrunoObject, asserts that a 'folder.bru' file is written in outputDir, reads it back with parseRequest (or fs.readFileSync) and validates expected folder metadata and contained item files; (2) a test that passes a collection containing an unsupported item type (e.g., type: 'unsupported-type') to createCollectionFromBrunoObject and asserts that the call rejects/throws (use expect(...).rejects.toThrow or try/catch) with an appropriate error. Reference the createCollectionFromBrunoObject helper and existing parsing/assertion pattern (parseRequest, fs.existsSync) so the new tests follow the same structure and cleanup approach.packages/bruno-cli/src/utils/collection.js (1)
502-502: Remove unnecessaryawaitcalls on synchronous functions.
stringifyFolderandstringifyRequestreturnstring, notPromise<string>. Theawaiton lines 502 and 540 is harmless but misleading.♻️ Remove unnecessary awaits
- const folderContent = await stringifyFolder(item.root, { format: 'bru' }); + const folderContent = stringifyFolder(item.root, { format: 'bru' });- const content = await stringifyRequest(bruJson, { format: 'bru' }); + const content = stringifyRequest(bruJson, { format: 'bru' });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-cli/src/utils/collection.js` at line 502, Remove the unnecessary awaits on synchronous string-returning helpers: drop the await when calling stringifyFolder and stringifyRequest (e.g., the call that sets folderContent from stringifyFolder(item.root, { format: 'bru' }) and the call that sets requestContent from stringifyRequest(...)). Update those lines to call the functions directly and treat their results as plain strings (no async handling or try/catch changes required), ensuring any surrounding code that assumed a Promise is left unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/bruno-cli/src/utils/collection.js`:
- Around line 488-543: The function processCollectionItems currently ignores
items whose item.type isn't 'folder' or request types; add a fail-fast else
branch after the existing else-if that throws a clear Error when an unsupported
item type is encountered (include item.type and item.name and currentPath in the
message) so imports don't silently drop unknown types; update any callers/tests
if they rely on silent skipping.
---
Nitpick comments:
In `@packages/bruno-cli/src/utils/collection.js`:
- Line 502: Remove the unnecessary awaits on synchronous string-returning
helpers: drop the await when calling stringifyFolder and stringifyRequest (e.g.,
the call that sets folderContent from stringifyFolder(item.root, { format: 'bru'
}) and the call that sets requestContent from stringifyRequest(...)). Update
those lines to call the functions directly and treat their results as plain
strings (no async handling or try/catch changes required), ensuring any
surrounding code that assumed a Promise is left unchanged.
In
`@packages/bruno-cli/tests/utils/collection/create-collection-from-bruno-object.spec.js`:
- Around line 17-69: Add two tests to the existing spec: (1) a test that passes
a collection with at least one folder item (type: 'folder', name, seq, items:
[...]) to createCollectionFromBrunoObject, asserts that a 'folder.bru' file is
written in outputDir, reads it back with parseRequest (or fs.readFileSync) and
validates expected folder metadata and contained item files; (2) a test that
passes a collection containing an unsupported item type (e.g., type:
'unsupported-type') to createCollectionFromBrunoObject and asserts that the call
rejects/throws (use expect(...).rejects.toThrow or try/catch) with an
appropriate error. Reference the createCollectionFromBrunoObject helper and
existing parsing/assertion pattern (parseRequest, fs.existsSync) so the new
tests follow the same structure and cleanup approach.
rameshsunkara
commented
Feb 19, 2026
Contributor
Author
There was a problem hiding this comment.
Thanks for the quick review and good callout. I removed the unrelated changes and kept this one intentionally.
collection.js supports reading both yml and bru, but this import path (createCollectionFromBrunoObject) only writes Bruno files (bruno.json, collection.bru, folder.bru, *.bru).
Since filestore’s default stringify format is yml, passing { format: 'bru' } here prevents writing yml-formatted content into .bru files.
Contributor
Author
There was a problem hiding this comment.
I kept the changes as coderabbit correctly called out about not having error handling for unsupported type.
And for this change,
- await is not needed at all, it just returns string
- format bru is needed as explained above
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-cli/tests/utils/collection/create-collection-from-bruno-object.spec.js (1)
8-97: Missing test for unsupported item type (the fail-fast error path).The PR description explicitly lists "throwing on unsupported item types" as one of the three regression tests to add. No such case exists in the spec. Per coding guidelines, error paths should be covered alongside happy paths.
💡 Suggested additional test case
+ it('throws on unsupported item type', async () => { + outputDir = fs.mkdtempSync(path.join(os.tmpdir(), 'bruno-cli-import-')); + + await expect( + createCollectionFromBrunoObject( + { + name: 'bad-collection', + items: [ + { + type: 'http', + name: 'Legacy Item', + filename: 'legacy.bru', + seq: 1, + request: { + method: 'GET', + url: 'https://api.example.com/users' + } + } + ] + }, + outputDir + ) + ).rejects.toThrow(/unsupported item type/i); + });As per coding guidelines: "Cover both the 'happy path' and the realistically problematic paths."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-cli/tests/utils/collection/create-collection-from-bruno-object.spec.js` around lines 8 - 97, Add a new test case in create-collection-from-bruno-object.spec.js that verifies createCollectionFromBrunoObject throws for unsupported item types: call createCollectionFromBrunoObject with a collection containing an item whose type is something like 'unsupported-type' (and include minimal required fields such as name/seq), run it against a temporary outputDir (using the same mkdtempSync pattern), and assert the call rejects/throws (use async/await with expect(...).rejects.toThrow or expect(() => ...).toThrow as appropriate) to cover the fail-fast error path when createCollectionFromBrunoObject encounters an unknown item.type.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In
`@packages/bruno-cli/tests/utils/collection/create-collection-from-bruno-object.spec.js`:
- Around line 8-97: Add a new test case in
create-collection-from-bruno-object.spec.js that verifies
createCollectionFromBrunoObject throws for unsupported item types: call
createCollectionFromBrunoObject with a collection containing an item whose type
is something like 'unsupported-type' (and include minimal required fields such
as name/seq), run it against a temporary outputDir (using the same mkdtempSync
pattern), and assert the call rejects/throws (use async/await with
expect(...).rejects.toThrow or expect(() => ...).toThrow as appropriate) to
cover the fail-fast error path when createCollectionFromBrunoObject encounters
an unknown item.type.
sanjaikumar-bruno
approved these changes
Feb 19, 2026
Contributor
Author
|
@sanjaikumar-bruno sorry I didn't see you approved it and pushed a change based on coderabbit comments. I think these are good changes, if you dont agree please let me know, I will revert. Please look at my comment above |
bijin-bruno
approved these changes
Feb 19, 2026
Collaborator
|
Hey @rameshsunkara, Thanks for your contribution! We’re merging this PR. There’s also an ongoing internal effort to add support for the YML format: #7028 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix CLI import serialization in
createCollectionFromBrunoObjectso generated.brufiles are valid and compatible with filestore expectations.Fixes #7172.
Problem
bru import openapicould fail with:Unsupported item type: httpRoot cause:
http-request,graphql-request) to legacy values (http,graphql) before callingstringifyRequest.Changes
http-requeststayshttp-requestgraphql-requeststaysgraphql-requeststringifyRequest(..., { format: 'bru' })stringifyFolder(..., { format: 'bru' }).brurequests correctlyfolder.brucorrectlyFiles
packages/bruno-cli/src/utils/collection.jspackages/bruno-cli/tests/utils/collection/create-collection-from-bruno-object.spec.jsValidation
Target test added:
packages/bruno-cli/tests/utils/collection/create-collection-from-bruno-object.spec.jsRecommended command:
npm run test --workspace=packages/bruno-cli -- tests/utils/collection/create-collection-from-bruno-object.spec.jsSummary by CodeRabbit
Refactor
Tests