fix: req.getPath() when URL uses {{BASEURL}} or other vars#7280
Open
sanjaikumar-bruno wants to merge 4 commits intousebruno:mainfrom
Open
fix: req.getPath() when URL uses {{BASEURL}} or other vars#7280sanjaikumar-bruno wants to merge 4 commits intousebruno:mainfrom
sanjaikumar-bruno wants to merge 4 commits intousebruno:mainfrom
Conversation
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:
WalkthroughReworks BrunoRequest.getPath() to parse/derive pathnames robustly, extracts interpolation into a new _interpolatePathParams(pathname) helper, adds error handling for non-string/unparseable URLs, and introduces Jest tests covering interpolation and edge cases. Changes
Sequence Diagram(s)(Skipped — changes are localized refactor and tests; no multi-component sequential flow requiring visualization.) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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 |
sanjaikumar-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
![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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@packages/bruno-js/src/bruno-request.js`:
- Around line 82-83: The interpolation currently skips valid falsy values
because it checks `if (pathParam && pathParam.value)`; change the guard to allow
0/false by checking for presence rather than truthiness (e.g. verify pathParam
exists and pathParam.value is not undefined/null) so the placeholder is replaced
when pathParam.value is 0 or false; update the condition around the pathParam
handling in the interpolation logic (the `pathParam` check in the code snippet)
to use an explicit undefined/null check.
- Around line 61-67: The fallback in getPath() assumes this.req.url is a string
and calls this.req.url.split('?'), which can throw if req.url is null/undefined;
add a type guard before splitting (e.g., check typeof this.req.url === 'string'
or coerce with a safe default) and handle the non-string case by setting
pathname to '' (or another safe default) instead of calling split; update the
logic around the withoutQuery variable and the existing startsWith/substring
branches to operate only when a valid string URL is present.
b03dcef to
73a9047
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-js/tests/bruno-request.spec.js (1)
49-57: Add one more degraded-path test for non-stringurlvalues.Line 49 currently covers
undefined, but a non-string input (e.g. number/object) is another realistic failure mode and worth pinning down in this suite.Proposed test addition
it('returns empty string when req.url is missing', () => { const req = new BrunoRequest({ url: undefined, method: 'GET', headers: {}, pathParams: [] }); expect(req.getPath()).toBe(''); }); + + it('returns empty string when req.url is not a string', () => { + const req = new BrunoRequest({ + url: 12345, + method: 'GET', + headers: {}, + pathParams: [] + }); + expect(req.getPath()).toBe(''); + });As per coding guidelines: "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-js/tests/bruno-request.spec.js` around lines 49 - 57, Add a degraded-path unit test to cover non-string url values for the BrunoRequest.getPath behavior: create a new spec (another it block) that constructs a BrunoRequest with url set to a non-string (e.g. a number or object), keep other fields like method/headers/pathParams as in the existing test, and assert that req.getPath() returns an empty string; this complements the existing undefined test and ensures BrunoRequest.getPath handles non-string inputs consistently.
🤖 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-js/tests/bruno-request.spec.js`:
- Around line 49-57: Add a degraded-path unit test to cover non-string url
values for the BrunoRequest.getPath behavior: create a new spec (another it
block) that constructs a BrunoRequest with url set to a non-string (e.g. a
number or object), keep other fields like method/headers/pathParams as in the
existing test, and assert that req.getPath() returns an empty string; this
complements the existing undefined test and ensures BrunoRequest.getPath handles
non-string inputs consistently.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bruno-js/src/bruno-request.jspackages/bruno-js/tests/bruno-request.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-js/src/bruno-request.js
Collaborator
sanish-bruno
left a comment
sanish-bruno
left a comment
There was a problem hiding this comment.
PR Review
Found 2 issue(s): 1 minor bug, 1 test gap.
Summary
- Good fix for the core issue (
{{BASEURL}}/pathpattern). The fallback string parsing and extraction of_interpolatePathParamsare clean. - URLs with an explicit protocol + template host (e.g.,
https://{{HOST}}/path) produce incorrect pathnames becauseindexOf('/')matches the/in://rather than the path separator. - The
pathParam.valuenull-check improvement (!= nullinstead of truthiness) is a nice fix for edge cases likevalue: 0.
sanjaikumar-bruno
force-pushed
the
fix/req-getpath-with-baseurl
branch
from
f672cce to
8a608fe
Compare
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
packages/bruno-js/src/bruno-request.js (1)
66-68:⚠️ Potential issue | 🟠 MajorBug:
indexOf('/')matches the/in://, producing incorrect pathnames.This issue was flagged in a previous review but remains unaddressed. For a URL like
https://{{HOST}}/path:
indexOf('/')returns6(the slash inhttps://)substring(6)yields//{{HOST}}/pathinstead of/pathProposed fix to strip protocol separator before searching
} else { - const firstSlash = withoutQuery.indexOf('/'); - pathname = firstSlash >= 0 ? withoutQuery.substring(firstSlash) : ''; + let searchIn = withoutQuery; + const protoEnd = searchIn.indexOf('://'); + if (protoEnd >= 0) { + searchIn = searchIn.substring(protoEnd + 3); + } + const firstSlash = searchIn.indexOf('/'); + pathname = firstSlash >= 0 ? searchIn.substring(firstSlash) : ''; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-js/src/bruno-request.js` around lines 66 - 68, The bug is that withoutQuery.indexOf('/') picks up the slash in the protocol ("https://"), producing an incorrect pathname; update the logic around withoutQuery, firstSlash and pathname to search for the first slash after the protocol separator: detect the protocol separator position (e.g. idxProto = withoutQuery.indexOf('://')), set the search start to idxProto >= 0 ? idxProto + 3 : 0, then compute firstSlash = withoutQuery.indexOf('/', searchStart) and derive pathname = firstSlash >= 0 ? withoutQuery.substring(firstSlash) : ''. Apply this change where firstSlash and pathname are defined so URLs like "https://host/path" yield "/path".
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/bruno-js/src/bruno-request.js`:
- Around line 66-68: The bug is that withoutQuery.indexOf('/') picks up the
slash in the protocol ("https://"), producing an incorrect pathname; update the
logic around withoutQuery, firstSlash and pathname to search for the first slash
after the protocol separator: detect the protocol separator position (e.g.
idxProto = withoutQuery.indexOf('://')), set the search start to idxProto >= 0 ?
idxProto + 3 : 0, then compute firstSlash = withoutQuery.indexOf('/',
searchStart) and derive pathname = firstSlash >= 0 ?
withoutQuery.substring(firstSlash) : ''. Apply this change where firstSlash and
pathname are defined so URLs like "https://host/path" yield "/path".
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bruno-js/src/bruno-request.jspackages/bruno-js/tests/bruno-request.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-js/tests/bruno-request.spec.js
refactor: enhance path parameter interpolation and URL handling in BrunoRequest class fix: enhance URL path extraction in BrunoRequest class to handle protocols correctly
sanjaikumar-bruno
force-pushed
the
fix/req-getpath-with-baseurl
branch
from
8a608fe to
dfdd00a
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-js/src/bruno-request.js (1)
79-96: Add JSDoc for_interpolatePathParams(pathname).This is a new abstraction and would benefit from a brief contract (
pathnameexpectations + return behavior).As per coding guidelines: "Add JSDoc comments to abstractions for additional details."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-js/src/bruno-request.js` around lines 79 - 96, Add a JSDoc comment for the _interpolatePathParams(pathname) method describing the expected input and return behavior: document that pathname is a string (or falsy), that this.req.pathParams should be an array of {name, value} objects, that any segment starting with ':' will be replaced by the matching pathParam.value, and that the function returns the interpolated pathname (or the original pathname if no interpolation occurs). Include parameter and return annotations and note that null/undefined pathname is returned unchanged, and that pathParams entries with null/undefined values are skipped.
🤖 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-js/src/bruno-request.js`:
- Around line 79-96: Add a JSDoc comment for the
_interpolatePathParams(pathname) method describing the expected input and return
behavior: document that pathname is a string (or falsy), that
this.req.pathParams should be an array of {name, value} objects, that any
segment starting with ':' will be replaced by the matching pathParam.value, and
that the function returns the interpolated pathname (or the original pathname if
no interpolation occurs). Include parameter and return annotations and note that
null/undefined pathname is returned unchanged, and that pathParams entries with
null/undefined values are skipped.
ℹ️ Review info
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bruno-js/src/bruno-request.jspackages/bruno-js/tests/bruno-request.spec.js
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/bruno-js/tests/bruno-request.spec.js
sanish-bruno
suggested changes
Feb 26, 2026
sanish-bruno
suggested changes
Feb 26, 2026
…proved clarity and efficiency
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-js/src/bruno-request.js (1)
75-92: Add JSDoc for_interpolatePathParams()helper.Line 75 introduces a new abstraction without JSDoc. Please document expected input/output and behavior when params are missing to match project standards.
As per coding guidelines: "Add JSDoc comments to abstractions for additional details."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-js/src/bruno-request.js` around lines 75 - 92, Add a JSDoc block for the _interpolatePathParams(pathname) helper that documents the expected inputs and outputs: describe that pathname is a string (may be falsy), this.req.pathParams should be an array of objects with {name:string, value:string|number|null|undefined}, and the function returns a string with colon-prefixed segments replaced by matching pathParam.value or left unchanged if no match or value is null/undefined; explicitly note behavior when pathname is falsy or this.req.pathParams is missing/not an array (returns pathname unchanged), include `@param` and `@returns` tags, and mark visibility (e.g. `@private`) to match project standards.
🤖 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-js/src/bruno-request.js`:
- Around line 75-92: Add a JSDoc block for the _interpolatePathParams(pathname)
helper that documents the expected inputs and outputs: describe that pathname is
a string (may be falsy), this.req.pathParams should be an array of objects with
{name:string, value:string|number|null|undefined}, and the function returns a
string with colon-prefixed segments replaced by matching pathParam.value or left
unchanged if no match or value is null/undefined; explicitly note behavior when
pathname is falsy or this.req.pathParams is missing/not an array (returns
pathname unchanged), include `@param` and `@returns` tags, and mark visibility (e.g.
`@private`) to match project standards.
sanish-bruno
approved these changes
Feb 27, 2026
bijin-bruno
requested changes
Feb 27, 2026
…ty function for improved modularity and clarity
…g variable assignments for improved readability
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.
BRU-2778
Fixes: #7248
Pre-request scripts run before URL interpolation, so for URLs like
{{BASEURL}}/path/:p1/:p2,req.urlis still a template andnew URL(req.url)throws, causingreq.getPath()to return''