fix: prompt variable in URL path incorrectly parsed as query parameter#7216
fix: prompt variable in URL path incorrectly parsed as query parameter#7216pooja-bruno wants to merge 1 commit intousebruno:mainfrom
Conversation
WalkthroughThe Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-app/src/utils/url/index.spec.js (1)
290-315: Add an edge case: delimiter exists only inside template variable(s).The three new tests all have a real
?outside the template variable. There's no test for the case where?appears only inside{{...}}—which is where the masking logic'sindex === -1branch becomes load-bearing after this change. Without it, a regression that over-eagerly matches and assigns index!= -1would go undetected.🧪 Suggested additional test
it('should handle prompt variable in query param value', () => { const url = 'https://example.com/api?token={{?token}}&active=true'; const params = splitOnFirst(url, '?'); expect(params).toEqual([ 'https://example.com/api', 'token={{?token}}&active=true' ]); }); + + it('should return full URL when ? only exists inside template variable', () => { + const url = 'https://example.com/{{?resource}}'; + const params = splitOnFirst(url, '?'); + expect(params).toEqual(['https://example.com/{{?resource}}']); + }); });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/url/index.spec.js` around lines 290 - 315, Add a unit test that covers the edge case where the only occurrences of the delimiter '?' are inside template variables so the masking logic must return "no split" (exercise the index === -1 branch). For function splitOnFirst, add a case like url = 'https://example.com/domain/{{?domain_id}}' and assert splitOnFirst(url, '?') returns [url, ''] (i.e., the entire string as the first element and an empty second element), ensuring we detect regressions that accidentally treat masked '?' as a real delimiter.
🤖 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-app/src/utils/url/index.spec.js`:
- Around line 290-315: Add a unit test that covers the edge case where the only
occurrences of the delimiter '?' are inside template variables so the masking
logic must return "no split" (exercise the index === -1 branch). For function
splitOnFirst, add a case like url = 'https://example.com/domain/{{?domain_id}}'
and assert splitOnFirst(url, '?') returns [url, ''] (i.e., the entire string as
the first element and an empty second element), ensuring we detect regressions
that accidentally treat masked '?' as a real delimiter.
Description
issue: #7204
JIRA
Contribution Checklist:
Before
https://github.com/user-attachments/assets/260f0ecb-b999-4e1c-b83e-e30d4267187b
After
https://github.com/user-attachments/assets/10ebe866-085d-440a-a2f2-3d0d197fb437
Summary by CodeRabbit
Bug Fixes
Tests