feat: add default http protocol when URL scheme is missing#7786
feat: add default http protocol when URL scheme is missing#7786KanhaiyaPandey wants to merge 4 commits intousebruno:mainfrom
Conversation
KanhaiyaPandey
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughReplaced a permissive local protocol regex with a new Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
|
Thanks @KanhaiyaPandey for the PR. We are looking into this. |
…ocol normalization Co-authored-by: KanhaiyaPandey <kanhaiyapandey2232@gmail.com>
|
@KanhaiyaPandey Thanks for raising this PR. It seems to fix the reported bug. I’ve added tests for the scheme check and refactored I also reviewed the scheme syntax in the WHATWG documentation and added relevant case checks. I’ve raised a PR against your repository’s feature branch—could you please take a look and review it? |
- Replace regex-based protocol detection with spec-compliant utility - Ensure correct handling of localhost, IPs, and ports - Add comprehensive tests for URL normalization - Centralize logic in bruno-common for reuse across CLI and Electron
Thanks for the improvements and for taking this forward! The shift to |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
packages/bruno-cli/tests/runner/url-protocol-normalization.spec.js (1)
12-14: LocalnormalizeUrlduplicates prod logic — consider testing the real call site.The helper reimplements the exact one-liner from
packages/bruno-cli/src/runner/run-single-request.js:346-348. If someone changes the production branch (e.g., addshttps://fallback, or gates onrequest.url.startsWith('{{')), these tests will keep passing while the real code regresses.Since
hasExplicitSchemeis already covered directly inpackages/bruno-electron/tests/network/index.spec.js, this suite's value is in exercising the CLI path. Either (a) import and invoke the real normalization fromrun-single-request.js, or (b) rename the suite to make clear it's unit-testinghasExplicitScheme— not "run-single-request" normalization as the describe block currently claims.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-cli/tests/runner/url-protocol-normalization.spec.js` around lines 12 - 14, The test currently reimplements normalizeUrl and duplicates production logic (normalizeUrl and hasExplicitScheme); instead either (A) remove the local normalizeUrl and import the real normalization function from run-single-request.js (use the module export for normalizeUrl or the exported normalization helper used by runSingleRequest) and call that in the tests so the CLI path is exercised, or (B) if you want a pure unit test for hasExplicitScheme only, rename the describe block to clearly state it is testing hasExplicitScheme and move the local helper into that namespaced suite; update test imports/assertions accordingly to reference normalizeUrl and hasExplicitScheme by their actual exported names.
🤖 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-electron/tests/network/index.spec.js`:
- Around line 40-51: The two tests are tautological because they only assert
String.prototype.startsWith rather than Bruno behaviour; replace them with
integration assertions that call configureRequest (and/or the hasExplicitScheme
logic) using the inputs '{{baseUrl}}' and '{{baseUrl}}/api/v1' and assert the
real observable behaviour: e.g., that configureRequest returns a request with
the same template URL (no scheme injection) and that hasExplicitScheme is false
(or that no scheme is added), or simply delete the two startsWith tests if you
prefer to remove redundancy; locate symbols configureRequest and
hasExplicitScheme in the test suite to implement the change.
---
Nitpick comments:
In `@packages/bruno-cli/tests/runner/url-protocol-normalization.spec.js`:
- Around line 12-14: The test currently reimplements normalizeUrl and duplicates
production logic (normalizeUrl and hasExplicitScheme); instead either (A) remove
the local normalizeUrl and import the real normalization function from
run-single-request.js (use the module export for normalizeUrl or the exported
normalization helper used by runSingleRequest) and call that in the tests so the
CLI path is exercised, or (B) if you want a pure unit test for hasExplicitScheme
only, rename the describe block to clearly state it is testing hasExplicitScheme
and move the local helper into that namespaced suite; update test
imports/assertions accordingly to reference normalizeUrl and hasExplicitScheme
by their actual exported names.
🪄 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: d217acf2-2441-4ba7-9cdf-abd2a42a930d
📒 Files selected for processing (6)
packages/bruno-cli/src/runner/run-single-request.jspackages/bruno-cli/tests/runner/url-protocol-normalization.spec.jspackages/bruno-common/src/utils/index.tspackages/bruno-common/src/utils/url/index.tspackages/bruno-electron/src/ipc/network/index.jspackages/bruno-electron/tests/network/index.spec.js
✅ Files skipped from review due to trivial changes (1)
- packages/bruno-common/src/utils/index.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bruno-cli/src/runner/run-single-request.js
- packages/bruno-electron/src/ipc/network/index.js
…figureRequest integration assertions
4 participants
Fixes #7759
🚀 Overview
This PR introduces a fallback mechanism to automatically use the
http://protocol when a URL is provided without an explicit scheme.❗ Problem
Currently, requests like:
GET localhost:8080
result in:
Unsupported protocol localhost:
This creates friction, especially when importing collections from Postman, where such URLs are valid.
✅ Solution
http://before execution💡 Impact
🧪 Testing
localhost:8080→http://localhost:8080http://example.com(unchanged)https://example.com(unchanged)📸 Screenshot
Before
Request:
GET localhost:8080
Error:
Unsupported protocol localhost:
After
Request:
GET localhost:8080
Resolved to:
GET http://localhost:8080 (auto-prepended)
Summary by CodeRabbit