[DRAFT] POC - feat(grpc): added scripting for grpc request#8368
[DRAFT] POC - feat(grpc): added scripting for grpc request#8368sharan-bruno wants to merge 2 commits into
Conversation
WalkthroughThe PR threads ChangesgRPC on-message scripting
Sequence Diagram(s)sequenceDiagram
participant startGrpcRequest
participant grpcEventHandlers
participant GrpcClient
participant ScriptRuntime
participant collectionsSlice
participant GrpcResponsePane
startGrpcRequest->>grpcEventHandlers: grpc:start-connection(requestUid, runtimeVariables)
grpcEventHandlers->>ScriptRuntime: runBeforeMessageScript(script.req)
grpcEventHandlers->>GrpcClient: startConnection(..., onMessage, onAfterResponse)
GrpcClient->>ScriptRuntime: runOnMessageScript(script.stream, message)
grpcEventHandlers->>ScriptRuntime: runAfterResponseScript(script.res)
grpcEventHandlers-->>collectionsSlice: on-message-script-execution event
collectionsSlice-->>GrpcResponsePane: show ScriptError
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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.
Actionable comments posted: 8
🧹 Nitpick comments (3)
packages/bruno-app/src/components/ResponsePane/GrpcResponsePane/index.js (1)
33-37: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider deriving the card visibility instead of an effect.
This effect only mirrors
hasScriptErrorinto state to allow a manual dismiss. It works, but arequestUid-keyed reset (or tracking the dismissed error) avoids the extra render and aligns with the project's preference to avoiduseEffect. As per coding guidelines: "MUST: AvoiduseEffectunless absolutely needed. Prefer derived state and event handlers."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-app/src/components/ResponsePane/GrpcResponsePane/index.js` around lines 33 - 37, The GrpcResponsePane visibility logic is just mirroring hasScriptError into local state via useEffect, so replace that effect with derived state or a requestUid-scoped reset. Update the GrpcResponsePane component to manage the dismiss behavior through the existing state/event handlers (for example, reset when the requestUid changes or track the dismissed error) rather than syncing in useEffect, keeping showScriptErrorCard driven directly from the current error state and dismissal state.Source: Coding guidelines
packages/bruno-js/src/sandbox/quickjs/shims/bruno-onmessage.js (1)
5-25: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
toHostQueryArgis copied verbatim frombruno-response.js.Identical helper now lives in two shim files. Per the repo's "abstract only when used in >3 places" rule it's not yet worth extracting, but worth a mental note — once a third caller appears, hoist this into a shared shim util to avoid the copies drifting apart.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-js/src/sandbox/quickjs/shims/bruno-onmessage.js` around lines 5 - 25, The helper toHostQueryArg in bruno-onmessage.js is duplicated from bruno-response.js, but this is not yet a refactor target under the repo rule. Leave the implementation in place for now, and add a brief TODO/maintenance note near toHostQueryArg (and the mirrored helper in bruno-response.js if needed) indicating that the shared utility should be extracted only when a third caller appears, so the two copies do not drift silently.packages/bruno-filestore/src/formats/yml/common/scripts.ts (1)
16-21: 📐 Maintainability & Code Quality | 🔵 TrivialReplace script type
'on-message'with'hooks'to resolve the type mismatch and remove unsafe casts.The
Scripttype is imported from@opencollection/types@0.9.1, which currently defines the valid script types as'before-request' | 'after-response' | 'tests' | 'hooks'. The current code uses the literal'on-message', which is not part of this union, forcing the use ofas unknown as Scriptat line 20 and a type assertion at line 58.If this event maps to the standard OpenCollection script hook, update the string to
'hooks'to satisfy the type system natively. This preserves type safety for the code object and removes the workaround.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/bruno-filestore/src/formats/yml/common/scripts.ts` around lines 16 - 21, The script object built in common/scripts.ts uses an invalid Script type value, causing the unsafe cast workaround. Update the request.script.stream handling in the code that pushes to ocScripts to use the valid OpenCollection script type 'hooks' instead of 'on-message', and then remove the as unknown as Script assertion so the object matches the imported Script union natively.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bruno-app/src/components/RequestPane/GrpcRequestPane/index.js`:
- Around line 106-110: The Scripts tab indicator is using the wrong data source
in GrpcRequestPane, so it follows docs instead of scripts. Update the indicator
logic for the 'scripts' tab entry in RequestPane/GrpcRequestPane/index.js to
check the scripts collection/state used by that pane, matching the pattern used
for the docs tab but referencing the correct variable. Make the change in the
tab configuration object for the Scripts key so the dot appears only when gRPC
scripts exist.
In `@packages/bruno-app/src/components/RequestPane/Script/index.js`:
- Around line 17-55: The phase metadata in getScriptPhases is generating
mismatched error-field names for the tabs, so the error dot state never matches
the item state. Update each phase object in getScriptPhases to include an
explicit error-field metadata value that maps to the actual camelCase script
error properties used by the state (for example, the fields used for
preRequestScriptErrorMessage, postResponseScriptErrorMessage,
beforeInvokeScriptErrorMessage, and onMessageScriptErrorMessage), and make the
tab error logic consume that metadata instead of deriving names from phase.key.
- Around line 112-119: The hook usage in the RequestPane Script component
violates the Rules of Hooks because useFocusErrorLine is being called inside
SCRIPT_PHASES.forEach. Refactor the SCRIPT_PHASES iteration in the
Script/index.js component so the hook is invoked from a stable React component
boundary, such as a small child component rendered per phase or a map-based
component wrapper, and keep the call order fixed regardless of how many phases
are present.
In `@packages/bruno-app/src/components/ResponsePane/GrpcResponsePane/index.js`:
- Around line 133-135: The ScriptErrorIcon usage in GrpcResponsePane is passing
the wrong prop name, leaving the tooltip id unresolved. Update the render in
GrpcResponsePane to pass the item UID using the ScriptErrorIcon contract
(itemUid) instead of item, keeping onClick unchanged so the icon receives the
identifier it expects.
In `@packages/bruno-app/src/utils/codemirror/autocomplete.js`:
- Line 423: Make API word classification protocol-aware: the current API-root
check in getCurrentWordWithContext/determineWordContext is using API_ROOTS
directly, so HTTP editors can misclassify gRPC-only roots like stream as API
context. Thread options.protocol through those helpers and replace the root
check with Object.keys(getApiHints(protocol)) so the classification matches the
available hints for the active protocol.
In `@packages/bruno-electron/src/ipc/network/grpc-event-handlers.js`:
- Around line 404-428: The current `onMessage`/`onAfterResponse` flow in
`grpc-event-handlers.js` is racy because `runOnMessageScript` is started without
being awaited, so `onAfterResponse` can run before the last on-message script
finishes. Update the `onMessage` and `onAfterResponse` handlers to serialize
on-message execution (for example by chaining work through a shared promise) and
make `onAfterResponse` wait for that pending chain before checking
`onMessageErrored` and calling `runAfterResponseScript`. Keep the fix localized
around `runOnMessageScript`, `runAfterResponseScript`, and the
`onMessageErrored` gate so overlapping `propagateScriptEnvUpdates` calls cannot
interleave.
In `@packages/bruno-js/src/bruno-response.js`:
- Around line 144-148: The test import is still treating the bruno-response
module as a default class export, but it now exposes named exports from
createBrunoResponse, HttpResponse, and GrpcResponse. Update header-list.spec.js
to destructure createBrunoResponse from require('../src/bruno-response') and use
that symbol wherever BrunoResponse was previously instantiated, so the test
matches the current export contract.
In `@packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.js`:
- Around line 139-157: The gRPC response shim currently mounts res as a plain
object with only messages, which breaks scripts that expect the HTTP-style
callable res helper. Update addGrpcResponseShim in bruno-response.js to either
expose a query helper on res or otherwise preserve the res(...) call surface for
gRPC responses, and make addBrunoResponseShimToContext route gRPC responses
through that parity-preserving behavior.
---
Nitpick comments:
In `@packages/bruno-app/src/components/ResponsePane/GrpcResponsePane/index.js`:
- Around line 33-37: The GrpcResponsePane visibility logic is just mirroring
hasScriptError into local state via useEffect, so replace that effect with
derived state or a requestUid-scoped reset. Update the GrpcResponsePane
component to manage the dismiss behavior through the existing state/event
handlers (for example, reset when the requestUid changes or track the dismissed
error) rather than syncing in useEffect, keeping showScriptErrorCard driven
directly from the current error state and dismissal state.
In `@packages/bruno-filestore/src/formats/yml/common/scripts.ts`:
- Around line 16-21: The script object built in common/scripts.ts uses an
invalid Script type value, causing the unsafe cast workaround. Update the
request.script.stream handling in the code that pushes to ocScripts to use the
valid OpenCollection script type 'hooks' instead of 'on-message', and then
remove the as unknown as Script assertion so the object matches the imported
Script union natively.
In `@packages/bruno-js/src/sandbox/quickjs/shims/bruno-onmessage.js`:
- Around line 5-25: The helper toHostQueryArg in bruno-onmessage.js is
duplicated from bruno-response.js, but this is not yet a refactor target under
the repo rule. Leave the implementation in place for now, and add a brief
TODO/maintenance note near toHostQueryArg (and the mirrored helper in
bruno-response.js if needed) indicating that the shared utility should be
extracted only when a third caller appears, so the two copies do not drift
silently.
🪄 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: 9607b478-6e21-4ca8-8d0f-046c853bb363
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (35)
packages/bruno-app/src/components/CodeEditor/index.jspackages/bruno-app/src/components/RequestPane/GrpcRequestPane/index.jspackages/bruno-app/src/components/RequestPane/Script/index.jspackages/bruno-app/src/components/ResponsePane/GrpcResponsePane/index.jspackages/bruno-app/src/components/ResponsePane/ScriptError/index.jspackages/bruno-app/src/providers/ReduxStore/slices/collections/index.jspackages/bruno-app/src/utils/codemirror/autocomplete.jspackages/bruno-app/src/utils/codemirror/javascript-lint.jspackages/bruno-app/src/utils/network/index.jspackages/bruno-electron/src/ipc/network/grpc-event-handlers.jspackages/bruno-electron/src/ipc/network/index.jspackages/bruno-electron/src/ipc/network/prepare-grpc-request.jspackages/bruno-electron/src/utils/collection.jspackages/bruno-filestore/src/formats/yml/common/scripts.tspackages/bruno-filestore/src/formats/yml/items/parseGrpcRequest.tspackages/bruno-js/src/bruno-onmessage.jspackages/bruno-js/src/bruno-request.jspackages/bruno-js/src/bruno-response.jspackages/bruno-js/src/runtime/assert-runtime.jspackages/bruno-js/src/runtime/script-runtime.jspackages/bruno-js/src/runtime/test-runtime.jspackages/bruno-js/src/runtime/vars-runtime.jspackages/bruno-js/src/sandbox/quickjs/index.jspackages/bruno-js/src/sandbox/quickjs/shims/bruno-onmessage.jspackages/bruno-js/src/sandbox/quickjs/shims/bruno-request.jspackages/bruno-js/src/sandbox/quickjs/shims/bruno-response.jspackages/bruno-js/src/utils.jspackages/bruno-js/src/utils/error-formatter.jspackages/bruno-js/tests/bruno-request-auth-mode.spec.jspackages/bruno-js/tests/bruno-request-delete-header.spec.jspackages/bruno-lang/v2/src/bruToJson.jspackages/bruno-lang/v2/src/jsonToBru.jspackages/bruno-requests/src/grpc/grpc-client.jspackages/bruno-schema-types/src/common/scripts.tspackages/bruno-schema/src/collections/index.js
| const getScriptPhases = (protocol = 'http') => { | ||
| if (protocol === 'grpc') { | ||
| return [ | ||
| { | ||
| key: 'before-invoke', | ||
| label: 'Before Invoke', | ||
| field: 'req', | ||
| hints: ['req', 'bru'] | ||
| }, | ||
| { | ||
| key: 'on-message', | ||
| label: 'On Message', | ||
| field: 'stream', | ||
| hints: ['req', 'stream', 'bru'] | ||
| }, | ||
| { | ||
| key: 'after-response', | ||
| label: 'After Response', | ||
| field: 'res', | ||
| hints: ['req', 'res', 'bru'] | ||
| } | ||
| ]; | ||
| } | ||
|
|
||
| return [ | ||
| { | ||
| key: 'pre-request', | ||
| label: 'Pre Request', | ||
| field: 'req', | ||
| hints: ['req', 'bru'] | ||
| }, | ||
| { | ||
| key: 'post-response', | ||
| label: 'Post Response', | ||
| field: 'res', | ||
| hints: ['req', 'res', 'bru'] | ||
| } | ||
| ]; | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Use explicit error-field metadata for each phase.
${phase.key}ScriptErrorMessage produces names like pre-requestScriptErrorMessage, before-invokeScriptErrorMessage, and on-messageScriptErrorMessage, but the item state uses camelCase fields such as preRequestScriptErrorMessage, onMessageScriptErrorMessage, and postResponseScriptErrorMessage. The tab error dots will not turn red.
Proposed fix
{
key: 'before-invoke',
label: 'Before Invoke',
field: 'req',
- hints: ['req', 'bru']
+ hints: ['req', 'bru'],
+ errorField: 'preRequestScriptErrorMessage'
},
{
key: 'on-message',
label: 'On Message',
field: 'stream',
- hints: ['req', 'stream', 'bru']
+ hints: ['req', 'stream', 'bru'],
+ errorField: 'onMessageScriptErrorMessage'
},
{
key: 'after-response',
label: 'After Response',
field: 'res',
- hints: ['req', 'res', 'bru']
+ hints: ['req', 'res', 'bru'],
+ errorField: 'postResponseScriptErrorMessage'
} {
key: 'pre-request',
label: 'Pre Request',
field: 'req',
- hints: ['req', 'bru']
+ hints: ['req', 'bru'],
+ errorField: 'preRequestScriptErrorMessage'
},
{
key: 'post-response',
label: 'Post Response',
field: 'res',
- hints: ['req', 'res', 'bru']
+ hints: ['req', 'res', 'bru'],
+ errorField: 'postResponseScriptErrorMessage'
}- item?.[`${phase.key}ScriptErrorMessage`]
+ item?.[phase.errorField]Also applies to: 192-194
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-app/src/components/RequestPane/Script/index.js` around lines
17 - 55, The phase metadata in getScriptPhases is generating mismatched
error-field names for the tabs, so the error dot state never matches the item
state. Update each phase object in getScriptPhases to include an explicit
error-field metadata value that maps to the actual camelCase script error
properties used by the state (for example, the fields used for
preRequestScriptErrorMessage, postResponseScriptErrorMessage,
beforeInvokeScriptErrorMessage, and onMessageScriptErrorMessage), and make the
tab error logic consume that metadata instead of deriving names from phase.key.
| SCRIPT_PHASES.forEach((phase) => { | ||
| useFocusErrorLine({ | ||
| uid: item.uid, | ||
| editorRef: getEditorRef(phase.key), | ||
| scriptPhase: phase.key, | ||
| isVisible: activeTab === phase.key | ||
| }); | ||
| }); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Inspect the hook call site and React hooks lint configuration.
rg -n -C4 "SCRIPT_PHASES\\.forEach|useFocusErrorLine" packages/bruno-app/src/components/RequestPane/Script/index.js
rg -n "react-hooks/rules-of-hooks|eslint-plugin-react-hooks" .Repository: usebruno/bruno
Length of output: 871
🏁 Script executed:
cat packages/bruno-app/.eslintrc.jsRepository: usebruno/bruno
Length of output: 216
🏁 Script executed:
find packages/bruno-app -maxdepth 2 -name ".eslintrc*" -o -name "eslint.config.*" 2>/dev/null | head -20Repository: usebruno/bruno
Length of output: 152
🏁 Script executed:
ls -la packages/bruno-app/ | grep -E "eslint|\.js$"Repository: usebruno/bruno
Length of output: 484
🏁 Script executed:
find . -maxdepth 3 -name "eslint*" -o -name ".eslintrc*" -o -name "eslint.config.*" 2>/dev/null | head -20Repository: usebruno/bruno
Length of output: 171
🏁 Script executed:
cat eslint.config.jsRepository: usebruno/bruno
Length of output: 7457
Move useFocusErrorLine out of the loop
React calls hooks sequentially. Invoking useFocusErrorLine inside SCRIPT_PHASES.forEach violates the Rules of Hooks; if the number of phases changes (e.g., switching protocols), the hook count shifts and React will throw.
Refactor to extract the logic into a separate child component or use SCRIPT_PHASES.map to render a functional component for each phase.
Proposed fix
+const ScriptErrorFocus = ({ uid, editorRef, scriptPhase, isVisible }) => {
+ useFocusErrorLine({
+ uid,
+ editorRef,
+ scriptPhase,
+ isVisible
+ });
+ return null;
+};
const Script = ({ item, collection, protocol = 'http' }) => {
// ... existing code ...
- SCRIPT_PHASES.forEach((phase) => {
- useFocusErrorLine({
- uid: item.uid,
- editorRef: getEditorRef(phase.key),
- scriptPhase: phase.key,
- isVisible: activeTab === phase.key
- });
- });
return (
<div className="w-full h-full flex flex-col">
+ {SCRIPT_PHASES.map((phase) => (
+ <ScriptErrorFocus
+ key={phase.key}
+ uid={item.uid}
+ editorRef={getEditorRef(phase.key)}
+ scriptPhase={phase.key}
+ isVisible={activeTab === phase.key}
+ />
+ ))}
<Tabs value={activeTab} onValueChange={onScriptTabChange}>📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SCRIPT_PHASES.forEach((phase) => { | |
| useFocusErrorLine({ | |
| uid: item.uid, | |
| editorRef: getEditorRef(phase.key), | |
| scriptPhase: phase.key, | |
| isVisible: activeTab === phase.key | |
| }); | |
| }); | |
| const ScriptErrorFocus = ({ uid, editorRef, scriptPhase, isVisible }) => { | |
| useFocusErrorLine({ | |
| uid, | |
| editorRef, | |
| scriptPhase, | |
| isVisible | |
| }); | |
| return null; | |
| }; | |
| const Script = ({ item, collection, protocol = 'http' }) => { | |
| return ( | |
| <div className="w-full h-full flex flex-col"> | |
| {SCRIPT_PHASES.map((phase) => ( | |
| <ScriptErrorFocus | |
| key={phase.key} | |
| uid={item.uid} | |
| editorRef={getEditorRef(phase.key)} | |
| scriptPhase={phase.key} | |
| isVisible={activeTab === phase.key} | |
| /> | |
| ))} | |
| <Tabs value={activeTab} onValueChange={onScriptTabChange}> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-app/src/components/RequestPane/Script/index.js` around lines
112 - 119, The hook usage in the RequestPane Script component violates the Rules
of Hooks because useFocusErrorLine is being called inside SCRIPT_PHASES.forEach.
Refactor the SCRIPT_PHASES iteration in the Script/index.js component so the
hook is invoked from a stable React component boundary, such as a small child
component rendered per phase or a map-based component wrapper, and keep the call
order fixed regardless of how many phases are present.
| {hasScriptError && !showScriptErrorCard && ( | ||
| <ScriptErrorIcon item={item} onClick={() => setShowScriptErrorCard(true)} /> | ||
| )} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Prop mismatch: ScriptErrorIcon wants itemUid, not item.
ScriptErrorIcon = ({ itemUid, onClick, className }), so passing item={item} leaves itemUid undefined and the toolhint id resolves to script-error-icon-undefined, breaking the tooltip association.
🩹 Proposed fix
- {hasScriptError && !showScriptErrorCard && (
- <ScriptErrorIcon item={item} onClick={() => setShowScriptErrorCard(true)} />
- )}
+ {hasScriptError && !showScriptErrorCard && (
+ <ScriptErrorIcon itemUid={item.uid} onClick={() => setShowScriptErrorCard(true)} />
+ )}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| {hasScriptError && !showScriptErrorCard && ( | |
| <ScriptErrorIcon item={item} onClick={() => setShowScriptErrorCard(true)} /> | |
| )} | |
| {hasScriptError && !showScriptErrorCard && ( | |
| <ScriptErrorIcon itemUid={item.uid} onClick={() => setShowScriptErrorCard(true)} /> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-app/src/components/ResponsePane/GrpcResponsePane/index.js`
around lines 133 - 135, The ScriptErrorIcon usage in GrpcResponsePane is passing
the wrong prop name, leaving the tooltip id unresolved. Update the render in
GrpcResponsePane to pass the item UID using the ScriptErrorIcon contract
(itemUid) instead of item, keeping onClick unchanged so the icon receives the
identifier it expects.
| */ | ||
| const determineWordContext = (word) => { | ||
| const isApiHint = Object.keys(STATIC_API_HINTS).some( | ||
| const isApiHint = API_ROOTS.some( |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Make API word classification protocol-aware.
API_ROOTS includes gRPC-only roots like stream, so HTTP script editors now classify stream... as API context but then have no matching HTTP API hints. Thread options.protocol into getCurrentWordWithContext/determineWordContext and use Object.keys(getApiHints(protocol)) there.
Proposed direction
-const determineWordContext = (word) => {
- const isApiHint = API_ROOTS.some(
+const determineWordContext = (word, protocol = 'http') => {
+ const apiRoots = Object.keys(getApiHints(protocol));
+ const isApiHint = apiRoots.some(
(apiRoot) => apiRoot.toLowerCase().startsWith(word.toLowerCase()) || word.toLowerCase().startsWith(apiRoot.toLowerCase())
);-const getCurrentWordWithContext = (cm) => {
+const getCurrentWordWithContext = (cm, protocol = 'http') => {
...
- const context = determineWordContext(word);
+ const context = determineWordContext(word, protocol);- const wordInfo = getCurrentWordWithContext(cm);
+ const wordInfo = getCurrentWordWithContext(cm, options.protocol);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const isApiHint = API_ROOTS.some( | |
| const determineWordContext = (word, protocol = 'http') => { | |
| const apiRoots = Object.keys(getApiHints(protocol)); | |
| const isApiHint = apiRoots.some( | |
| (apiRoot) => apiRoot.toLowerCase().startsWith(word.toLowerCase()) || word.toLowerCase().startsWith(apiRoot.toLowerCase()) | |
| ); |
| const isApiHint = API_ROOTS.some( | |
| const getCurrentWordWithContext = (cm, protocol = 'http') => { | |
| ... | |
| const context = determineWordContext(word, protocol); |
| const isApiHint = API_ROOTS.some( | |
| const wordInfo = getCurrentWordWithContext(cm, options.protocol); |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-app/src/utils/codemirror/autocomplete.js` at line 423, Make
API word classification protocol-aware: the current API-root check in
getCurrentWordWithContext/determineWordContext is using API_ROOTS directly, so
HTTP editors can misclassify gRPC-only roots like stream as API context. Thread
options.protocol through those helpers and replace the root check with
Object.keys(getApiHints(protocol)) so the classification matches the available
hints for the active protocol.
| let onMessageErrored = false; | ||
|
|
||
| // 2. On Message — script.stream (one call per received server message) | ||
| const onMessage = preparedRequest?.script?.stream?.length | ||
| ? (message) => { | ||
| runOnMessageScript({ ...scriptContext, message }) | ||
| .then(({ scriptError }) => { | ||
| if (scriptError) onMessageErrored = true; | ||
| }) | ||
| .catch((err) => { | ||
| console.error('Error running gRPC on-message script:', err); | ||
| }); | ||
| } | ||
| : undefined; | ||
|
|
||
| // 3. After Response — script.res (fires once on terminal event). | ||
| // `responses` is the full list of received messages, collected by the gRPC client. | ||
| const onAfterResponse = preparedRequest?.script?.res?.length | ||
| ? (responses) => { | ||
| if (onMessageErrored) return; | ||
| runAfterResponseScript({ ...scriptContext, responses }).catch((err) => { | ||
| console.error('Error running gRPC after-response script:', err); | ||
| }); | ||
| } | ||
| : undefined; |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
The onMessageErrored gate is racy — after-response may slip past in-flight on-message scripts.
onMessage fires runOnMessageScript(...) without awaiting, while onAfterResponse reads onMessageErrored synchronously the moment the call terminates (via #buildOnComplete). The script for the final message (or any still-pending one) can resolve after the terminal event, so onMessageErrored is still false and the after-response script runs anyway. Concurrent propagateScriptEnvUpdates calls from overlapping on-message runs can also interleave with the after-response env update, yielding inconsistent renderer state.
Consider serialising on-message execution (e.g. chain on a promise) and awaiting the tail before running after-response.
🔗 Sketch: chain on-message runs and await before after-response
let onMessageErrored = false;
+ let onMessageChain = Promise.resolve();
const onMessage = preparedRequest?.script?.stream?.length
? (message) => {
- runOnMessageScript({ ...scriptContext, message })
- .then(({ scriptError }) => {
- if (scriptError) onMessageErrored = true;
- })
- .catch((err) => {
- console.error('Error running gRPC on-message script:', err);
- });
+ onMessageChain = onMessageChain
+ .then(() => runOnMessageScript({ ...scriptContext, message }))
+ .then(({ scriptError }) => {
+ if (scriptError) onMessageErrored = true;
+ })
+ .catch((err) => {
+ console.error('Error running gRPC on-message script:', err);
+ });
}
: undefined;
const onAfterResponse = preparedRequest?.script?.res?.length
? (responses) => {
- if (onMessageErrored) return;
- runAfterResponseScript({ ...scriptContext, responses }).catch((err) => {
- console.error('Error running gRPC after-response script:', err);
- });
+ onMessageChain
+ .then(() => {
+ if (onMessageErrored) return;
+ return runAfterResponseScript({ ...scriptContext, responses });
+ })
+ .catch((err) => {
+ console.error('Error running gRPC after-response script:', err);
+ });
}
: undefined;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let onMessageErrored = false; | |
| // 2. On Message — script.stream (one call per received server message) | |
| const onMessage = preparedRequest?.script?.stream?.length | |
| ? (message) => { | |
| runOnMessageScript({ ...scriptContext, message }) | |
| .then(({ scriptError }) => { | |
| if (scriptError) onMessageErrored = true; | |
| }) | |
| .catch((err) => { | |
| console.error('Error running gRPC on-message script:', err); | |
| }); | |
| } | |
| : undefined; | |
| // 3. After Response — script.res (fires once on terminal event). | |
| // `responses` is the full list of received messages, collected by the gRPC client. | |
| const onAfterResponse = preparedRequest?.script?.res?.length | |
| ? (responses) => { | |
| if (onMessageErrored) return; | |
| runAfterResponseScript({ ...scriptContext, responses }).catch((err) => { | |
| console.error('Error running gRPC after-response script:', err); | |
| }); | |
| } | |
| : undefined; | |
| let onMessageErrored = false; | |
| let onMessageChain = Promise.resolve(); | |
| // 2. On Message — script.stream (one call per received server message) | |
| const onMessage = preparedRequest?.script?.stream?.length | |
| ? (message) => { | |
| onMessageChain = onMessageChain | |
| .then(() => runOnMessageScript({ ...scriptContext, message })) | |
| .then(({ scriptError }) => { | |
| if (scriptError) onMessageErrored = true; | |
| }) | |
| .catch((err) => { | |
| console.error('Error running gRPC on-message script:', err); | |
| }); | |
| } | |
| : undefined; | |
| // 3. After Response — script.res (fires once on terminal event). | |
| // `responses` is the full list of received messages, collected by the gRPC client. | |
| const onAfterResponse = preparedRequest?.script?.res?.length | |
| ? (responses) => { | |
| onMessageChain | |
| .then(() => { | |
| if (onMessageErrored) return; | |
| return runAfterResponseScript({ ...scriptContext, responses }); | |
| }) | |
| .catch((err) => { | |
| console.error('Error running gRPC after-response script:', err); | |
| }); | |
| } | |
| : undefined; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-electron/src/ipc/network/grpc-event-handlers.js` around lines
404 - 428, The current `onMessage`/`onAfterResponse` flow in
`grpc-event-handlers.js` is racy because `runOnMessageScript` is started without
being awaited, so `onAfterResponse` can run before the last on-message script
finishes. Update the `onMessage` and `onAfterResponse` handlers to serialize
on-message execution (for example by chaining work through a shared promise) and
make `onAfterResponse` wait for that pending chain before checking
`onMessageErrored` and calling `runAfterResponseScript`. Keep the fix localized
around `runOnMessageScript`, `runAfterResponseScript`, and the
`onMessageErrored` gate so overlapping `propagateScriptEnvUpdates` calls cannot
interleave.
| module.exports = { | ||
| HttpResponse, | ||
| GrpcResponse, | ||
| createBrunoResponse | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find consumers of bruno-response and inspect whether any still expect a default class export.
rg -n -C3 "bruno-response|createBrunoResponse|HttpResponse|GrpcResponse" packages/bruno-jsRepository: usebruno/bruno
Length of output: 9074
Update header-list.spec.js to use named exports
The module packages/bruno-js/src/bruno-response.js now exports an object containing HttpResponse, GrpcResponse, and createBrunoResponse. The test file packages/bruno-js/tests/header-list.spec.js currently imports the module as const BrunoResponse = require('../src/bruno-response'). Under the new contract, BrunoResponse resolves to the exports object, not the class, which will likely cause instantiation errors in tests.
Update the import in packages/bruno-js/tests/header-list.spec.js to:
const { createBrunoResponse } = require('../src/bruno-response');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-js/src/bruno-response.js` around lines 144 - 148, The test
import is still treating the bruno-response module as a default class export,
but it now exposes named exports from createBrunoResponse, HttpResponse, and
GrpcResponse. Update header-list.spec.js to destructure createBrunoResponse from
require('../src/bruno-response') and use that symbol wherever BrunoResponse was
previously instantiated, so the test matches the current export contract.
| const addGrpcResponseShim = (vm, res) => { | ||
| const resObject = vm.newObject(); | ||
|
|
||
| const messages = marshallToVm(res.messages, vm); | ||
| vm.setProp(resObject, 'messages', messages); | ||
| messages.dispose(); | ||
|
|
||
| vm.setProp(vm.global, 'res', resObject); | ||
| resObject.dispose(); | ||
| }; | ||
|
|
||
| const addBrunoResponseShimToContext = (vm, res) => { | ||
| if (res?.isGrpc) { | ||
| addGrpcResponseShim(vm, res); | ||
| } else { | ||
| addHttpResponseShim(vm, res); | ||
| } | ||
| }; | ||
|
|
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Heads up: gRPC res is a non-callable object.
Unlike the HTTP shim where res is a function (res('a.b.c')), the gRPC shim mounts res as a plain object exposing only messages. Any after-response/test script that calls res(...) against a gRPC request will throw res is not a function. If that's the intended surface for now, fine — otherwise consider exposing a query helper for parity.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-js/src/sandbox/quickjs/shims/bruno-response.js` around lines
139 - 157, The gRPC response shim currently mounts res as a plain object with
only messages, which breaks scripts that expect the HTTP-style callable res
helper. Update addGrpcResponseShim in bruno-response.js to either expose a query
helper on res or otherwise preserve the res(...) call surface for gRPC
responses, and make addBrunoResponseShimToContext route gRPC responses through
that parity-preserving behavior.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/bruno-app/src/components/RequestPane/GrpcRequestPane/index.js`:
- Around line 66-67: The Scripts indicator in GrpcRequestPane is using
request.script without guarding against the null default returned by
getPropertyFromDraftOrRequest(), so it can crash on gRPC requests that have no
script yet. Update the indicator logic around the existing script/tests handling
in GrpcRequestPane to safely check for a script object before reading its
content, and include script in the useMemo dependency list so the indicator
recomputes when script content changes. Use the existing symbols
getPropertyFromDraftOrRequest, script, and useMemo to locate the block.
In `@packages/bruno-electron/src/ipc/network/grpc-event-handlers.js`:
- Around line 377-445: The fallback path in runResponseTests is losing
collection-level globals when a test script throws before partialResults is
available. Update the fallback testResults object in runResponseTests to source
globalEnvironmentVariables from collection (the same place
propagateScriptEnvUpdates and the rest of this handler treat as authoritative)
instead of request, so the error recovery path doesn’t overwrite existing
globals with an empty object. Keep the fix localized to the runResponseTests
error branch and preserve the existing shape expected by sendEvent and
propagateScriptEnvUpdates.
🪄 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: f2a63120-bd20-45eb-957d-802650f47dbf
📒 Files selected for processing (6)
packages/bruno-app/src/components/RequestPane/GrpcRequestPane/index.jspackages/bruno-app/src/components/RequestPane/Script/index.jspackages/bruno-app/src/components/RequestPane/Tests/index.jspackages/bruno-app/src/components/ResponsePane/GrpcResponsePane/index.jspackages/bruno-electron/src/ipc/network/grpc-event-handlers.jspackages/bruno-electron/src/ipc/network/prepare-grpc-request.js
🚧 Files skipped from review as they are similar to previous changes (3)
- packages/bruno-electron/src/ipc/network/prepare-grpc-request.js
- packages/bruno-app/src/components/ResponsePane/GrpcResponsePane/index.js
- packages/bruno-app/src/components/RequestPane/Script/index.js
| const script = getPropertyFromDraftOrRequest(item, 'request.script'); | ||
| const tests = getPropertyFromDraftOrRequest(item, 'request.tests'); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win
Guard the Scripts indicator against missing request.script, and memoize it on script content.
getPropertyFromDraftOrRequest() defaults to null, so Line 119 will throw on any gRPC request that does not have a request.script object yet. The same block also omits script from the useMemo deps, so the dot can stay stale after edits.
Minimal fix
- const script = getPropertyFromDraftOrRequest(item, 'request.script');
+ const script = getPropertyFromDraftOrRequest(item, 'request.script', {});
...
{
key: 'scripts',
label: 'Scripts',
indicator: (script.req || script.stream || script.res) ? (hasScriptError ? <StatusDot type="error" /> : <StatusDot />) : null
},
...
- }, [grpcMessagesCount, isClientStreaming, activeHeadersLength, hasAuth, tests, hasTestError, docs, item.preRequestScriptErrorMessage, item.onMessageScriptErrorMessage, item.postResponseScriptErrorMessage]);
+ }, [grpcMessagesCount, isClientStreaming, activeHeadersLength, hasAuth, script.req, script.stream, script.res, tests, hasTestError, docs, item.preRequestScriptErrorMessage, item.onMessageScriptErrorMessage, item.postResponseScriptErrorMessage]);Also applies to: 117-127
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-app/src/components/RequestPane/GrpcRequestPane/index.js`
around lines 66 - 67, The Scripts indicator in GrpcRequestPane is using
request.script without guarding against the null default returned by
getPropertyFromDraftOrRequest(), so it can crash on gRPC requests that have no
script yet. Update the indicator logic around the existing script/tests handling
in GrpcRequestPane to safely check for a script object before reading its
content, and include script in the useMemo dependency list so the indicator
recomputes when script content changes. Use the existing symbols
getPropertyFromDraftOrRequest, script, and useMemo to locate the block.
| const runResponseTests = async ({ | ||
| request, | ||
| collection, | ||
| envVars, | ||
| runtimeVariables, | ||
| processEnvVars, | ||
| scriptingConfig, | ||
| requestUid, | ||
| itemUid, | ||
| responses | ||
| }) => { | ||
| const testFile = get(request, 'tests'); | ||
| if (typeof testFile !== 'string' || !testFile.length) { | ||
| return { testResults: null, testError: null }; | ||
| } | ||
|
|
||
| const testRuntime = new TestRuntime({ runtime: scriptingConfig?.runtime }); | ||
| let testResults = null; | ||
| let testError = null; | ||
| try { | ||
| testResults = await testRuntime.runTests( | ||
| decomment(testFile, { space: true }), | ||
| request, | ||
| { responses: responses || [] }, | ||
| envVars, | ||
| runtimeVariables, | ||
| collection.pathname, | ||
| onConsoleLog, | ||
| processEnvVars, | ||
| scriptingConfig, | ||
| null, | ||
| collection.name | ||
| ); | ||
| } catch (error) { | ||
| testError = error; | ||
| // Preserve any test() calls that passed before the script errored | ||
| testResults = error.partialResults || { | ||
| request, | ||
| envVariables: envVars, | ||
| runtimeVariables, | ||
| globalEnvironmentVariables: request?.globalEnvironmentVariables || {}, | ||
| results: [], | ||
| nextRequestName: null | ||
| }; | ||
| } | ||
|
|
||
| sendEvent('main:run-request-event', { | ||
| type: 'test-results', | ||
| results: testResults.results, | ||
| requestUid, | ||
| itemUid, | ||
| collectionUid: collection.uid | ||
| }); | ||
|
|
||
| sendEvent('main:run-request-event', { | ||
| type: 'test-script-execution', | ||
| requestUid, | ||
| itemUid, | ||
| collectionUid: collection.uid, | ||
| errorMessage: testError ? (testError.message || 'An error occurred while executing the test script') : null, | ||
| errorContext: testError | ||
| ? formatErrorWithContextV2(testError, 'test', request?.testsMetadata, collection.pathname) | ||
| : null | ||
| }); | ||
|
|
||
| propagateScriptEnvUpdates(testResults, request, collection); | ||
|
|
||
| return { testResults, testError }; | ||
| }; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Preserve collection globals when test execution falls back after an error.
Line 417 reads globals from request, but this handler keeps them on collection; then Line 442 propagates the fallback and Line 183 overwrites collection.globalEnvironmentVariables. A test script error before partialResults exists can emit {} and clear current globals.
Proposed minimal fix
- globalEnvironmentVariables: request?.globalEnvironmentVariables || {},
+ globalEnvironmentVariables: collection.globalEnvironmentVariables || {},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const runResponseTests = async ({ | |
| request, | |
| collection, | |
| envVars, | |
| runtimeVariables, | |
| processEnvVars, | |
| scriptingConfig, | |
| requestUid, | |
| itemUid, | |
| responses | |
| }) => { | |
| const testFile = get(request, 'tests'); | |
| if (typeof testFile !== 'string' || !testFile.length) { | |
| return { testResults: null, testError: null }; | |
| } | |
| const testRuntime = new TestRuntime({ runtime: scriptingConfig?.runtime }); | |
| let testResults = null; | |
| let testError = null; | |
| try { | |
| testResults = await testRuntime.runTests( | |
| decomment(testFile, { space: true }), | |
| request, | |
| { responses: responses || [] }, | |
| envVars, | |
| runtimeVariables, | |
| collection.pathname, | |
| onConsoleLog, | |
| processEnvVars, | |
| scriptingConfig, | |
| null, | |
| collection.name | |
| ); | |
| } catch (error) { | |
| testError = error; | |
| // Preserve any test() calls that passed before the script errored | |
| testResults = error.partialResults || { | |
| request, | |
| envVariables: envVars, | |
| runtimeVariables, | |
| globalEnvironmentVariables: request?.globalEnvironmentVariables || {}, | |
| results: [], | |
| nextRequestName: null | |
| }; | |
| } | |
| sendEvent('main:run-request-event', { | |
| type: 'test-results', | |
| results: testResults.results, | |
| requestUid, | |
| itemUid, | |
| collectionUid: collection.uid | |
| }); | |
| sendEvent('main:run-request-event', { | |
| type: 'test-script-execution', | |
| requestUid, | |
| itemUid, | |
| collectionUid: collection.uid, | |
| errorMessage: testError ? (testError.message || 'An error occurred while executing the test script') : null, | |
| errorContext: testError | |
| ? formatErrorWithContextV2(testError, 'test', request?.testsMetadata, collection.pathname) | |
| : null | |
| }); | |
| propagateScriptEnvUpdates(testResults, request, collection); | |
| return { testResults, testError }; | |
| }; | |
| const runResponseTests = async ({ | |
| request, | |
| collection, | |
| envVars, | |
| runtimeVariables, | |
| processEnvVars, | |
| scriptingConfig, | |
| requestUid, | |
| itemUid, | |
| responses | |
| }) => { | |
| const testFile = get(request, 'tests'); | |
| if (typeof testFile !== 'string' || !testFile.length) { | |
| return { testResults: null, testError: null }; | |
| } | |
| const testRuntime = new TestRuntime({ runtime: scriptingConfig?.runtime }); | |
| let testResults = null; | |
| let testError = null; | |
| try { | |
| testResults = await testRuntime.runTests( | |
| decomment(testFile, { space: true }), | |
| request, | |
| { responses: responses || [] }, | |
| envVars, | |
| runtimeVariables, | |
| collection.pathname, | |
| onConsoleLog, | |
| processEnvVars, | |
| scriptingConfig, | |
| null, | |
| collection.name | |
| ); | |
| } catch (error) { | |
| testError = error; | |
| // Preserve any test() calls that passed before the script errored | |
| testResults = error.partialResults || { | |
| request, | |
| envVariables: envVars, | |
| runtimeVariables, | |
| globalEnvironmentVariables: collection.globalEnvironmentVariables || {}, | |
| results: [], | |
| nextRequestName: null | |
| }; | |
| } | |
| sendEvent('main:run-request-event', { | |
| type: 'test-results', | |
| results: testResults.results, | |
| requestUid, | |
| itemUid, | |
| collectionUid: collection.uid | |
| }); | |
| sendEvent('main:run-request-event', { | |
| type: 'test-script-execution', | |
| requestUid, | |
| itemUid, | |
| collectionUid: collection.uid, | |
| errorMessage: testError ? (testError.message || 'An error occurred while executing the test script') : null, | |
| errorContext: testError | |
| ? formatErrorWithContextV2(testError, 'test', request?.testsMetadata, collection.pathname) | |
| : null | |
| }); | |
| propagateScriptEnvUpdates(testResults, request, collection); | |
| return { testResults, testError }; | |
| }; |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/bruno-electron/src/ipc/network/grpc-event-handlers.js` around lines
377 - 445, The fallback path in runResponseTests is losing collection-level
globals when a test script throws before partialResults is available. Update the
fallback testResults object in runResponseTests to source
globalEnvironmentVariables from collection (the same place
propagateScriptEnvUpdates and the rest of this handler treat as authoritative)
instead of request, so the error recovery path doesn’t overwrite existing
globals with an empty object. Keep the fix localized to the runResponseTests
error branch and preserve the existing shape expected by sendEvent and
propagateScriptEnvUpdates.
Description
Added scripting for grpc request, 3 phases of scripting Before invoke, on message and After response
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
Bug Fixes
streamas a valid predefined global.