fix(websocket): add API Key query params support and OAuth2 inheritan… #6271
Conversation
pooja-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
WalkthroughAdds API-key-as-query-param handling for WebSocket requests (with variable interpolation and safe error logging), exports Changes
Sequence Diagram(s)sequenceDiagram
participant UI as Browser/UI
participant IPC as Electron IPC / prepareWsRequest
participant WS as WebSocket Server
participant Runtime as Env/Runtime Vars
UI->>IPC: Trigger send WebSocket request (wsRequest)
IPC->>Runtime: Interpolate key/value using env & runtime vars
alt interpolation succeeds
IPC->>IPC: Append interpolated key/value to wsRequest.url query
else interpolation error
IPC-->>IPC: Log error, continue without failing
end
IPC->>WS: Open WS connection to modified URL and send message
WS->>WS: Parse incoming message, detect func === 'query'
WS->>IPC: Respond with { query: { ...parsed query params... } }
IPC->>UI: Deliver response messages to UI for display
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Suggested labels
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/bruno-electron/tests/network/prepare-ws-request.spec.js (1)
35-54: Consider additional test coverage for API key query param handling.The current test validates the happy path, but consider adding tests for:
- Variable interpolation in key/value (e.g.,
key: '{{envVar}}')- Error handling with invalid URLs
- Verification that
apiKeyAuthValueForQueryParamsis properly cleaned up- API key placement in headers (if supported) vs. query params
Minor note: Line 51 is redundant since line 52 already validates the exact URL (which implicitly checks containment).
packages/bruno-app/src/components/RequestPane/WSRequestPane/WSAuth/index.js (1)
94-103: Consider extracting the OAuth2 message to reduce duplication.The JSX block (lines 96-102) is identical to the direct OAuth2 case (lines 81-89). To improve maintainability:
Extract the message into a constant or helper function:
+const OAuth2NotSupportedMessage = () => ( + <div className="flex flex-row w-full mt-2 gap-2"> + <div> + OAuth 2 not <strong>yet</strong> supported by WebSockets. Using no auth instead. + </div> + </div> +); + const getAuthView = () => { switch (authMode) { // ... other cases ... case 'oauth2': { - return ( - <> - <div className="flex flex-row w-full mt-2 gap-2"> - <div> - OAuth 2 not <strong>yet</strong> supported by WebSockets. Using no auth instead. - </div> - </div> - </> - ); + return <OAuth2NotSupportedMessage />; } case 'inherit': { const source = getEffectiveAuthSource(); if (source?.auth?.mode === 'oauth2') { - return ( - <> - <div className="flex flex-row w-full mt-2 gap-2"> - <div> - OAuth 2 not <strong>yet</strong> supported by WebSockets. Using no auth instead. - </div> - </div> - </> - ); + return <OAuth2NotSupportedMessage />; }Note: The PR description mentions "OAuth2" but the code says "OAuth 2" - ensure consistent terminology throughout.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/bruno-app/src/components/RequestPane/WSRequestPane/WSAuth/index.js(1 hunks)packages/bruno-electron/src/ipc/network/ws-event-handlers.js(2 hunks)packages/bruno-electron/tests/network/prepare-ws-request.spec.js(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses. Keep 'em tight
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Avoid single line abstractions where all that's being done is increasing the call stack with one additional function
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
packages/bruno-app/src/components/RequestPane/WSRequestPane/WSAuth/index.jspackages/bruno-electron/tests/network/prepare-ws-request.spec.jspackages/bruno-electron/src/ipc/network/ws-event-handlers.js
🧠 Learnings (2)
📓 Common learnings
Learnt from: bijin-bruno
Repo: usebruno/bruno PR: 6263
File: packages/bruno-requests/src/auth/oauth2-helper.ts:249-249
Timestamp: 2025-12-02T07:24:50.299Z
Learning: In OAuth2 Basic Auth headers for Bruno, clientSecret is optional and can be omitted. When constructing the Authorization header in `packages/bruno-requests/src/auth/oauth2-helper.ts`, use `clientSecret || ''` instead of `clientSecret!` to properly handle cases where only clientId is provided, per community requests.
📚 Learning: 2025-12-02T07:24:50.299Z
Learnt from: bijin-bruno
Repo: usebruno/bruno PR: 6263
File: packages/bruno-requests/src/auth/oauth2-helper.ts:249-249
Timestamp: 2025-12-02T07:24:50.299Z
Learning: In OAuth2 Basic Auth headers for Bruno, clientSecret is optional and can be omitted. When constructing the Authorization header in `packages/bruno-requests/src/auth/oauth2-helper.ts`, use `clientSecret || ''` instead of `clientSecret!` to properly handle cases where only clientId is provided, per community requests.
Applied to files:
packages/bruno-app/src/components/RequestPane/WSRequestPane/WSAuth/index.jspackages/bruno-electron/src/ipc/network/ws-event-handlers.js
🧬 Code graph analysis (2)
packages/bruno-electron/tests/network/prepare-ws-request.spec.js (2)
packages/bruno-electron/src/ipc/network/ws-event-handlers.js (1)
prepareWsRequest(27-218)packages/bruno-cli/src/commands/run.js (1)
runtimeVariables(334-334)
packages/bruno-electron/src/ipc/network/ws-event-handlers.js (3)
packages/bruno-electron/src/ipc/network/interpolate-vars.js (1)
globalEnvironmentVariables(22-22)packages/bruno-electron/src/ipc/network/cert-utils.js (1)
interpolationOptions(46-52)packages/bruno-lang/v2/src/bruToJson.js (2)
key(882-882)value(883-883)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: SSL Tests - Windows
- GitHub Check: Playwright E2E Tests
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Linux
- GitHub Check: CLI Tests
- GitHub Check: Unit Tests
🔇 Additional comments (3)
packages/bruno-electron/src/ipc/network/ws-event-handlers.js (2)
384-385: LGTM!Exporting
prepareWsRequestalongsidewsClientallows external modules (like tests) to use this utility function. The export is clean and follows the existing module pattern.
187-213: Verify variable references for interpolation options.Lines 192-193 reference
request.globalEnvironmentVariablesandcollection?.promptVariables. Confirm:
- Line 192: Whether
wsRequest.globalEnvironmentVariables(set on line 82) should be used instead ofrequest.globalEnvironmentVariablesfor consistency- Line 193: That
collection?.promptVariablesis the correct access path (verify it aligns with how promptVariables is accessed elsewhere in the codebase)packages/bruno-electron/tests/network/prepare-ws-request.spec.js (1)
1-34: LGTM!The test setup and mock helpers are well-structured. The mocks include all necessary fields for
prepareWsRequestto function correctly.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
tests/websockets/query.spec.ts (2)
6-19: Consider usingtest.stepand locator variables for readabilityThe flow has clear phases (open collection, select request, run, assert). Wrapping these in
test.stepand extracting locators into variables would make the test easier to read and debug in reports:await test.step('Open the most recent collection', async () => { const collectionName = page.locator('#sidebar-collection-name'); await collectionName.click(); }); await test.step('Run the websocket request', async () => { const requestItem = page.getByTitle(BRU_REQ_NAME); await requestItem.click(); await locators.runner().click(); });This stays within existing behavior but improves structure and trace output.
11-19: Make the assertion more robust and add a second checkRelying on
messages().nth(2)is a bit brittle if the UI ever changes how many messages are shown. You could both (a) assert that at least 3 messages exist and (b) search for the matching payload instead of hard-coding the index, e.g.:const messages = locators.messages(); await expect(messages).toHaveCount(3); // or >= 3 if appropriate const messageTexts = await messages.locator('.text-ellipsis').allInnerTexts(); expect(messageTexts.some((t) => /"testParam"\s*:\s*"testValue"/.test(t))).toBe(true);That keeps the intent (assert presence of the query param) while reducing coupling to the exact ordering.
packages/bruno-tests/src/ws/index.js (1)
36-41: Query-param echo behavior looks correct and consistentThe new
func === 'query'branch cleanly parsesrequest.urlwith a host base, turnssearchParamsinto a plain object, and returns it underquery, mirroring the existing headers branch. This is a good fit for the new tests; at most you could simplify the payload with{ query }, but the current form is perfectly fine.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/bruno-tests/src/ws/index.js(1 hunks)tests/websockets/fixtures/collection/ws-test-request-with-query.bru(1 hunks)tests/websockets/query.spec.ts(1 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation. No tabs, just spaces
Stick to single quotes for strings
Always add semicolons at the end of statements
No trailing commas
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line, and ensure consistency. Minimum 2 elements for multiline
No newlines inside function parentheses. Keep 'em tight
Space before and after the arrow in arrow functions.() => {}is good
No space between function name and parentheses.func()notfunc ()
Semicolons go at the end of the line, not on a new line
Names for functions need to be concise and descriptive
Add in JSDoc comments to add more details to the abstractions if needed
Avoid single line abstractions where all that's being done is increasing the call stack with one additional function
Add in meaningful comments instead of obvious ones where complex code flow is explained properly
Files:
tests/websockets/query.spec.tspackages/bruno-tests/src/ws/index.js
tests/**/**.*
⚙️ CodeRabbit configuration file
tests/**/**.*: Review the following e2e test code written using the Playwright test library. Ensure that:
Follow best practices for Playwright code and e2e automation
Try to reduce usage of
page.waitForTimeout();in code unless absolutely necessary and the locator cannot be found using existingexpect()playwright callsAvoid using
page.pause()in codeUse locator variables for locators
Avoid using test.only
Use multiple assertions
Promote the use of
test.stepas much as possible so the generated reports are easier to readEnsure that the
fixtureslike the collections are nested inside thefixturesfolderFixture Example*: Here's an example of possible fixture and test pair
. ├── fixtures │ └── collection │ ├── base.bru │ ├── bruno.json │ ├── collection.bru │ ├── ws-test-request-with-headers.bru │ ├── ws-test-request-with-subproto.bru │ └── ws-test-request.bru ├── connection.spec.ts # <- Depends on the collection in ./fixtures/collection ├── headers.spec.ts ├── persistence.spec.ts ├── variable-interpolation │ ├── fixtures │ │ └── collection │ │ ├── environments │ │ ├── bruno.json │ │ └── ws-interpolation-test.bru │ ├── init-user-data │ └── variable-interpolation.spec.ts # <- Depends on the collection in ./variable-interpolation/fixtures/collection └── subproto.spec.ts
Files:
tests/websockets/query.spec.tstests/websockets/fixtures/collection/ws-test-request-with-query.bru
🧬 Code graph analysis (2)
tests/websockets/query.spec.ts (1)
tests/utils/page/locators.ts (1)
buildWebsocketCommonLocators(57-77)
packages/bruno-tests/src/ws/index.js (4)
packages/bruno-electron/src/ipc/network/ws-event-handlers.js (4)
url(118-118)url(148-148)url(178-178)request(28-28)packages/bruno-electron/src/ipc/network/index.js (5)
url(174-174)url(190-190)url(206-206)url(222-222)request(333-333)packages/bruno-app/src/components/RequestPane/WsQueryUrl/index.js (1)
url(24-24)packages/bruno-electron/src/ipc/network/axios-instance.js (1)
URL(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: SSL Tests - Linux
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - macOS
- GitHub Check: Playwright E2E Tests
- GitHub Check: Unit Tests
- GitHub Check: CLI Tests
🔇 Additional comments (1)
tests/websockets/fixtures/collection/ws-test-request-with-query.bru (1)
1-19: Fixture structure and URL look aligned with the WS query testName, URL (including query string and variable), and
auth: inheritall look correct, and the file is placed underfixtures/collectionas per the documented pattern.
1 participant
Description
Changes:
JIRA
#5946
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
New Features
Improvements
Tests
✏️ Tip: You can customize this high-level summary in your review settings.