refactor: remove idx from HeaderList, extend ReadOnlyPropertyList, add positional method translations#7917
Merged
bijin-bruno merged 6 commits intousebruno:mainfrom May 6, 2026
Conversation
- Eliminated the 'idx' method from both req.headerList and res.headerList to streamline header management. - Updated associated tests and documentation to reflect the removal, ensuring clarity in the API usage and maintaining consistency across the header management system.
sanish-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
sanish-bruno
changed the title
sanish-bruno
changed the title
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:
WalkthroughHeaderList no longer exposes an indexed accessor ( ChangesHeaderList idx Removal & Blocking
Sequence Diagram(s)sequenceDiagram
participant Editor
participant QuickJS
participant Host
participant Tests
participant Translator
Editor->>QuickJS: remove static idx autocomplete hints
QuickJS->>Host: createPropertyListBridge without 'idx' in syncReadObjectMethods
QuickJS->>Host: assign forEach from internal each
QuickJS->>Host: delete temporary each alias
Tests->>Host: drop idx() testcases and run updated read-method tests
Translator->>Host: map pm.request.headers.{prepend,insert,insertAfter} → req.headerList.append
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
🚥 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 |
- Added error handling for unimplemented methods in HeaderList, including idx, add, upsert, remove, each, prepend, insert, and insertAfter. - Each method now throws a descriptive error indicating the appropriate alternative methods to use, enhancing clarity in the API and guiding users towards correct usage.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@packages/bruno-js/src/header-list.js`:
- Around line 159-162: The idx() method in HeaderList throws an error that
misleads users to use get(name) or all(), but idx() returns a numeric index and
the correct replacement is indexOf(); update the error message in the idx()
method (HeaderList::idx or ReadOnlyPropertyList::idx override) to recommend
using indexOf(name) instead of get(name)/all() so it correctly directs callers
to the numeric-index API.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b049edd7-6072-4571-a786-72f61c886fcd
📒 Files selected for processing (1)
packages/bruno-js/src/header-list.js
- Modified the error message in the idx() method to guide users towards using all()[index] or get(name) instead of the unsupported idx() method, enhancing the clarity of the API documentation.
… unimplemented methods - Changed HeaderList to extend ReadOnlyPropertyList instead of PropertyList, streamlining its functionality. - Removed unimplemented methods (prepend, insert, insertAfter) from HeaderList, clarifying the API and guiding users towards using supported methods. - Updated related tests to reflect these changes, ensuring consistency and accuracy in header management.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/bruno-js/tests/header-list.spec.js (1)
1-25: ⚡ Quick winMissing test coverage for the blocked
idxandeachAPI surface.Removing the
idxtests is right, but there's no assertion that the properties are actuallyundefined/non-callable. Without this, a future change that accidentally re-exposes these (e.g. removing the class field declarations) would go undetected.✅ Suggested tests to add
+ test('idx is not exposed (blocked inherited method)', () => { + const { list } = createReqHeaders(); + expect(list.idx).toBeUndefined(); + expect(() => list.idx(0)).toThrow(TypeError); + }); + + test('each is not exposed (blocked inherited method)', () => { + const { list } = createReqHeaders(); + expect(list.each).toBeUndefined(); + expect(() => list.each(() => {})).toThrow(TypeError); + });Note: If the blocked methods are changed to throw descriptive errors (see comment on
header-list.js), updatetoThrow(TypeError)totoThrow('...')to assert the specific message.As per coding guidelines: "If code is added, removed, or significantly modified, corresponding tests should be updated or created."
🤖 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-js/tests/header-list.spec.js` around lines 1 - 25, Add assertions to the header-list.spec.js tests that ensure the blocked API surface on HeaderList is not exposed: verify that the instance properties list.idx and list.each are undefined (or not functions) and, if desired, assert that calling them throws a TypeError (or the specific error string if header-list.js was changed to throw descriptive errors). Refer to the HeaderList instance produced by createReqHeaders() and add expectations that explicitly check list.idx and list.each are absent/non-callable to prevent regressions.
🤖 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.
Inline comments:
In `@packages/bruno-js/src/header-list.js`:
- Around line 153-157: The class currently shadows inherited methods by setting
the fields idx = undefined and each = undefined on HeaderList, causing generic
"is not a function" TypeErrors; replace those class fields with explicit methods
(e.g., idx(...){ throw new TypeError('HeaderList.idx is not supported; use
HeaderList.get(...) or HeaderList.at(...) instead'); } and each(...){ throw new
TypeError('HeaderList.each is not supported; use HeaderList.forEach(...) or
HeaderList.iterateHeaders(...) instead'); }) so callers receive descriptive
errors directing them to the correct HeaderList API; update the HeaderList class
in place of the idx and each fields (they shadow ReadOnlyPropertyList) to
implement these throwing methods.
---
Nitpick comments:
In `@packages/bruno-js/tests/header-list.spec.js`:
- Around line 1-25: Add assertions to the header-list.spec.js tests that ensure
the blocked API surface on HeaderList is not exposed: verify that the instance
properties list.idx and list.each are undefined (or not functions) and, if
desired, assert that calling them throws a TypeError (or the specific error
string if header-list.js was changed to throw descriptive errors). Refer to the
HeaderList instance produced by createReqHeaders() and add expectations that
explicitly check list.idx and list.each are absent/non-callable to prevent
regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9dd1f45d-dc3c-4dce-b99e-24bd86d91c8a
📒 Files selected for processing (5)
packages/bruno-converters/src/utils/postman-to-bruno-translator.jspackages/bruno-js/src/header-list.jspackages/bruno-js/src/sandbox/quickjs/shims/bruno-request.jspackages/bruno-js/src/sandbox/quickjs/shims/bruno-response.jspackages/bruno-js/tests/header-list.spec.js
- Introduced new tests to validate the translation of pm.request.headers methods (prepend, insert, insertAfter) to their corresponding req.headerList.append method. - Enhanced existing tests to ensure accurate conversion and functionality of header management in the Bruno converters.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In
`@packages/bruno-converters/tests/postman/postman-translations/transpiler-tests/request.test.js`:
- Around line 210-220: The tests currently expect the translator to preserve the
second positional "ref" argument for pm.request.headers.insert and
pm.request.headers.insertAfter, but the translator should drop that removed API
semantic and emit a pure append call; update the expectations in the two tests
that reference translateCode to assert that req.headerList.append only receives
the header object (i.e., expect translatedCode toBe 'req.headerList.append({key:
"X-Mid", value: "2"});' and 'req.headerList.append({key: "X-After", value:
"3"});' respectively), referencing the pm.request.headers.insert /
pm.request.headers.insertAfter inputs and the req.headerList.append output to
locate the assertions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 38ac1643-b758-45ba-8e75-3b751c67a58e
📒 Files selected for processing (1)
packages/bruno-converters/tests/postman/postman-translations/transpiler-tests/request.test.js
sanish-bruno
changed the title
- Added translations for additional pm.request.headers methods (get, has, one, all, count, indexOf, find, filter, each, map, reduce, toObject, clear) to their corresponding req.headerList methods. - Updated tests to validate the new translations and ensure accurate header management functionality in the Bruno converters.
bijin-bruno
approved these changes
May 6, 2026
9 tasks
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
2 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
Cleans up the HeaderList API surface by removing
idx, switching the parent class fromPropertyListtoReadOnlyPropertyList, adding missing translator mappings, and ensuring full regex fallback coverage.Changes
1. HeaderList now extends
ReadOnlyPropertyListdirectlyHeaderListwas extendingPropertyList, but it overrode every mutation method with its own implementation that operates on the request config. ThePropertyListlayer only contributed methods (add,upsert,remove,prepend,insert,insertAfter) that HeaderList either renamed or doesn't support — none of them were actually used.By extending
ReadOnlyPropertyListdirectly, those methods simply don't exist on HeaderList. No need to block them with throwing stubs.Only two inherited methods need explicit blocking:
idx— set toundefined(not part of the HeaderList API)each— set toundefined(renamed toforEach)2.
idxremoved from API surfacesyncReadObjectMethodsinbruno-request.jsandbruno-response.js)autocomplete.js)header-list.spec.jsreq/headerList/read-methods.bruandres/headerList/read-methods.bru3.
eachfully removed from QuickJS sandboxThe bridge generates
each(shared with CookieList), then aliasesforEach = eachfor HeaderList. Now also deleteseachafter aliasing, so onlyforEachexists in the sandbox — consistent with the native side.4. Postman positional header methods →
append(complex transform)Added complex AST transformations for
pm.request.headers.prepend,.insert,.insertAfter→req.headerList.append. These drop the second positional argument (e.g.,insert({key, value}, "ref")becomesappend({key, value})) since position is irrelevant for headers stored as a plain object.5. Missing
toString/toJSONtranslations addedAdded
toStringandtoJSONtranslations for both request and response headers across all three layers:6. Complete regex fallback coverage for headerList
Added all missing headerList method entries to the regex fallback (
postman-translations.js), ensuring parity with the AST translator:Request headers:
get,has,one,all,count,indexOf,find,filter,each→forEach,map,reduce,toObject,toString,toJSON,clear,add→append,upsert→set,populate,repopulate,assimilate,prepend/insert/insertAfter→appendResponse headers:
has,one,all,count,indexOf,find,filter,each→forEach,map,reduce,toObject,toString,toJSONTest plan
npm run test --workspace=packages/bruno-js— 488 tests passnpm run test --workspace=packages/bruno-converters— 950 tests pass.bruintegration test filesContribution Checklist: