Conversation
ganesh-bruno
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 due to trivial changes (1)
WalkthroughSet NewRequest's Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~8 minutes 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
packages/bruno-app/src/components/Sidebar/NewRequest/index.js (1)
340-363: Consider resetting method to GET when switching back to HTTP.The GraphQL radio now forces
POST, but the HTTP radio (lines 340-348) still uses plainformik.handleChange. If a user selects GraphQL (sets method to POST) then switches back to HTTP, the method remains POST. This asymmetry may confuse users expecting a "clean slate."For consistency, consider applying the same pattern to the HTTP radio:
♻️ Suggested change for HTTP radio
<input type="radio" id="http-request" name="requestType" value="http-request" checked={formik.values.requestType === 'http-request'} - onChange={formik.handleChange} + onChange={(e) => { + formik.handleChange(e); + formik.setFieldValue('requestMethod', 'GET'); + }} data-testid="http-request" />🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/Sidebar/NewRequest/index.js` around lines 340 - 363, The HTTP radio input (id "http-request", name "requestType") currently only calls formik.handleChange so if the user previously picked GraphQL (which sets requestMethod to 'POST' in the graphql radio onChange), the method remains POST; update the HTTP radio's onChange to call formik.handleChange(e) and also call formik.setFieldValue('requestMethod', 'GET') so switching back to HTTP resets the method to GET.
🤖 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/components/Sidebar/NewRequest/index.js`:
- Around line 340-363: The HTTP radio input (id "http-request", name
"requestType") currently only calls formik.handleChange so if the user
previously picked GraphQL (which sets requestMethod to 'POST' in the graphql
radio onChange), the method remains POST; update the HTTP radio's onChange to
call formik.handleChange(e) and also call formik.setFieldValue('requestMethod',
'GET') so switching back to HTTP resets the method to GET.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 079d4406-40ed-472f-b823-b92d34645244
📒 Files selected for processing (1)
packages/bruno-app/src/components/Sidebar/NewRequest/index.js
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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-app/src/components/Sidebar/NewRequest/index.js`:
- Around line 264-267: The WebSocket radio currently calls formik.handleChange
directly which leaves requestMethod set to 'POST' after switching from GraphQL;
update the WebSocket radio to use handleRequestTypeChange (or create a new
onChange handler) so that when request type is 'websocket' you call
formik.setFieldValue('requestMethod', 'ws') (or set it to 'ws' explicitly) to
mirror other WS creation paths and prevent newWsRequest from inheriting an HTTP
method; locate the radio input that currently uses formik.handleChange and
replace it with handleRequestTypeChange or a small wrapper that sets
requestMethod to 'ws'.
🪄 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: b3a34f5b-2c9d-42d3-9db7-2c29fb2e2bc5
📒 Files selected for processing (1)
packages/bruno-app/src/components/Sidebar/NewRequest/index.js
sanish-bruno
left a comment
sanish-bruno
left a comment
There was a problem hiding this comment.
PR Review
Found 3 issue(s): 1 major, 1 minor, 1 nit.
Summary
- Major: The
graphql-requestradio button is missing the matchingonChangehandler, so switching from HTTP to GraphQL does not set the method toPOST— this contradicts the PR description ("Switching a request to GraphQL applies the POST method by default"). - Minor: The initial
requestMethodonly accounts for thegraphql-requestpreset;ws-requestpresets will incorrectly start withGETand that value is forwarded tonewWsRequest. - Nit:
getRequestType(collectionPresets)is now called twice ininitialValues.
| value="grpc-request" | ||
| checked={formik.values.requestType === 'grpc-request'} | ||
| onChange={formik.handleChange} | ||
| onChange={(e) => { |
There was a problem hiding this comment.
Bug (Major): The graphql-request radio button (new line ~362) was not updated with the same pattern applied to HTTP/gRPC/WS. It still reads:
onChange={formik.handleChange}This means when a user starts with the default http-request type (method GET) and then clicks the GraphQL radio, requestMethod is not switched to POST. The resulting request is created as GraphQL-type with method GET, which is exactly the method/type mismatch this PR set out to prevent and contradicts the PR description ("Switching a request to GraphQL applies the POST method by default").
Note: the actual issue is on line 362 (outside any diff hunk) — attached to this added block for visibility.
Suggested fix (apply to the graphql radio on line ~362):
onChange={(e) => {
formik.handleChange(e);
formik.setFieldValue('requestMethod', 'POST');
}}| requestType: getRequestType(collectionPresets), | ||
| requestUrl: collectionPresets.requestUrl || '', | ||
| requestMethod: 'GET', | ||
| requestMethod: getRequestType(collectionPresets) === 'graphql-request' ? 'POST' : 'GET', |
There was a problem hiding this comment.
Bug (Minor): This ternary only handles the graphql-request preset. If a collection has presets.requestType === 'ws', getRequestType returns 'ws-request', but requestMethod is initialized to 'GET'. That value is then passed through to newWsRequest in onSubmit (see line 176), producing a WebSocket request whose method is GET instead of ws — the same mismatch the new radio onChange handler for ws-request is trying to prevent when the user switches types manually.
Suggested fix:
const defaultRequestType = getRequestType(collectionPresets);
const defaultRequestMethod =
defaultRequestType === 'graphql-request' ? 'POST'
: defaultRequestType === 'ws-request' ? 'ws'
: 'GET';
// ...
initialValues: {
// ...
requestType: defaultRequestType,
requestMethod: defaultRequestMethod,
// ...
}(grpc-request does not pass requestMethod to newGrpcRequest, so it does not need special handling, but hoisting defaultRequestType also removes the double call flagged in the nit below.)
| requestType: getRequestType(collectionPresets), | ||
| requestUrl: collectionPresets.requestUrl || '', | ||
| requestMethod: 'GET', | ||
| requestMethod: getRequestType(collectionPresets) === 'graphql-request' ? 'POST' : 'GET', |
There was a problem hiding this comment.
Nit: getRequestType(collectionPresets) is now invoked twice in initialValues (once for requestType, once inside this ternary). Hoisting it to a local const just above the useFormik call keeps the two derived values in sync and avoids re-running the switch in getRequestType for no reason.
Follow-up:
|
| Radio | onChange sets method |
Method is consumed for this type | Handler needed? |
|---|---|---|---|
http-request |
'GET' |
yes (serialized + sent) | yes |
graphql-request |
(missing) | yes (serialized as method: 'POST') |
yes — still the real bug |
grpc-request |
'GET' |
no (not in model, not serialized, not read) | no — dead write |
ws-request |
'ws' |
no (stored on in-memory item only, never serialized, never read at runtime) | no — dead write |
So the grpc/ws handlers aren't wrong in behavior, but they imply those types need a method, which they don't. That uniform "every radio sets the method" pattern is probably part of why the GraphQL handler got missed during review — at a glance, all four radios look structurally similar.
Retracting one piece of my earlier review
My inline comment on line 114 ("ws-request preset will incorrectly start with GET and that value is forwarded to newWsRequest") is technically correct that the value is forwarded, but the follow-on claim ("producing a WebSocket request whose method is GET instead of ws") is not actually observable — the stored method is never persisted or read for ws. So the user-facing behavior is unchanged either way. Please disregard that part of the comment. The graphql bug is still real.
Suggested minimum-correctness patch
const DEFAULT_METHOD_BY_TYPE = {
'http-request': 'GET',
'graphql-request': 'POST',
// grpc-request and ws-request intentionally omitted:
// request.method is not part of their model / not serialized / not read at runtime.
};
// initialValues
requestMethod: DEFAULT_METHOD_BY_TYPE[getRequestType(collectionPresets)] || 'GET',
// single handler, reused:
const handleTypeChange = (type) => (e) => {
formik.handleChange(e);
const nextMethod = DEFAULT_METHOD_BY_TYPE[type];
if (nextMethod) formik.setFieldValue('requestMethod', nextMethod);
};
// usage:
onChange={handleTypeChange('http-request')}
onChange={handleTypeChange('graphql-request')} // this is the currently-missing case
onChange={handleTypeChange('grpc-request')} // no-op for method, keeps symmetry
onChange={handleTypeChange('ws-request')} // no-op for method, keeps symmetryThat encodes the real invariant (only HTTP and GraphQL carry a method) in one place and makes the graphql omission a compile-time-obvious gap rather than a copy-paste oversight.
2 participants
Screen.Recording.2026-04-08.at.6.44.17.PM.mov
Summary by CodeRabbit