feat: add Monaco Editor as beta alternative for script editors#7793
Open
sanish-bruno wants to merge 6 commits intousebruno:mainfrom
Open
feat: add Monaco Editor as beta alternative for script editors#7793sanish-bruno wants to merge 6 commits intousebruno:mainfrom
sanish-bruno wants to merge 6 commits intousebruno:mainfrom
Conversation
- Added Monaco Editor as a beta feature for scripting in pre-request, post-response, and test scripts. - Updated ScriptEditor component to conditionally render MonacoEditor or CodeEditor based on feature flag. - Implemented variable highlighting and custom theme registration for Monaco. - Updated package-lock.json to include new dependencies for Monaco Editor. - Enhanced user experience with improved autocomplete and multi-cursor editing capabilities.
sanish-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds opt-in Monaco Editor: dependency and bundler rule, a new MonacoEditor component with theme/types/autocomplete/variable tooltip/workers, a ScriptEditor wrapper toggled by a beta flag, replaces CodeEditor usages with ScriptEditor across several UIs, updates preferences/schema, and adds tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant UI as "App UI\n(ScriptEditor)"
participant Script as "ScriptEditor"
participant Editor as "MonacoEditor"
participant Monaco as "monaco.editor"
participant Worker as "Monaco Worker Loader"
participant Store as "Redux Store"
UI->>Script: render(props, ref)
Script->>Editor: lazy-load (if beta enabled)
Editor->>Monaco: register theme & types (brunoTheme, brunoApiTypes)
Editor->>Worker: ensure getWorker routes (json/js/ts/worker)
Editor->>Monaco: create(editorContainer, options)
Monaco-->>Editor: editor instance
Editor->>Editor: setupVariableHighlighting + setupAutoComplete
UI->>Editor: user edits -> onEdit callback
Editor->>Store: dispatch update/save/run actions
UI->>Editor: unmount
Editor->>Monaco: dispose editor, remove listeners, persist scroll
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes 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 |
sanish-bruno
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/bruno-app/src/components/CollectionSettings/Script/index.js (1)
42-52:⚠️ Potential issue | 🟠 MajorDownstream of the
ScriptEditorref issue —preRequestEditorRef.current/postResponseEditorRef.currentwill benull.The
useEffecton Lines 42-52 callseditor.refresh()on tab change to avoid CodeMirror layout glitches. After the swap toScriptEditor(which doesn't forward refs), both refs staynulland the optional chaining silently no-ops. Fix lives inScriptEditor/index.js(useforwardRef); once that lands, this file needs no further change.Also applies to: 103-104, 118-119
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/CollectionSettings/Script/index.js` around lines 42 - 52, useEffect in this file calls preRequestEditorRef.current.editor.refresh() / postResponseEditorRef.current.editor.refresh() but those refs are null because the new ScriptEditor component doesn't forward refs; fix by updating ScriptEditor (ScriptEditor/index.js) to use React.forwardRef and attach the inner editor instance (or an object with an .editor property) to the forwarded ref so preRequestEditorRef and postResponseEditorRef receive the expected shape when ScriptEditor mounts.packages/bruno-app/src/components/FolderSettings/Script/index.js (1)
105-132:⚠️ Potential issue | 🟡 Minor
MonacoEditordoesn't forward refs, sopreRequestEditorRefandpostResponseEditorRefwill benullwhen the MONACO_EDITOR beta flag is enabled.
ScriptEditorconditionally renders eitherCodeEditor(class component) orMonacoEditor(function component) and spreads all props through.MonacoEditorhas noforwardRefwrapper, so refs won't populate. TheuseEffectat line 43 calls.editor.refresh()(CodeMirror-only API), which silently no-ops for Monaco due to optional chaining, and React warns about unforwarded refs on function components (React 18+).Either wrap
MonacoEditorwithforwardRefand expose{ editor: { refresh() } }(noop is fine—Monaco usesautomaticLayout: true), or conditionally omit refs when rendering Monaco.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/FolderSettings/Script/index.js` around lines 105 - 132, ScriptEditor currently forwards refs to both CodeEditor and MonacoEditor but MonacoEditor is a function component without forwardRef so preRequestEditorRef/postResponseEditorRef end up null and the useEffect that calls .editor.refresh() (line ~43) triggers React warnings; fix by either wrapping MonacoEditor with React.forwardRef and exposing an object with editor.refresh() (noop is acceptable) so the ref shape matches CodeEditor, or detect the MONACO_EDITOR path in ScriptEditor and do not pass ref props to MonacoEditor; update MonacoEditor (or ScriptEditor) so refs are consistent with the expected { editor: { refresh() } } shape referenced by the useEffect.
♻️ Duplicate comments (1)
packages/bruno-app/src/components/RequestPane/Script/index.js (1)
37-48:⚠️ Potential issue | 🟠 MajorSame ref regression as
CollectionSettings/Script— refresh-on-tab-switch becomes a no-op.
preRequestEditorRef/postResponseEditorRefwon't be populated untilScriptEditorforwards its ref. Root-cause fix is inScriptEditor/index.js; no change required here afterwards.Also applies to: 99-100, 115-116
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/RequestPane/Script/index.js` around lines 37 - 48, The refresh-on-tab-switch is a no-op because ScriptEditor does not forward its ref, so preRequestEditorRef and postResponseEditorRef never get populated; fix by updating ScriptEditor (the component in ScriptEditor/index.js) to use React.forwardRef and pass the received ref through to the inner ScriptEditor instance or the underlying editor component (ensuring the forwarded ref points to an object with .editor), i.e., accept (props, ref) and attach/ref={ref} (or assign ref.current = innerRef) so that preRequestEditorRef.current.editor and postResponseEditorRef.current.editor are defined and the useEffect refresh logic in RequestPane/Script works as intended.
🧹 Nitpick comments (8)
packages/bruno-app/src/utils/monaco/brunoApiTypes.js (2)
49-66: Optional: giveexpect/testa minimal chai-like shape instead ofany.Typing
expectasanysilences diagnostics but also kills the main value prop here — autocomplete for.to.equal(...),.to.be.a(...), etc. A lightweight structural type (even a looseChaiExpect-like interface covering the common chainables you actually want to surface) would make the beta feel materially better. Not blocking.🧠 Sketch
-declare const expect: any; +interface BrunoAssertion { + to: BrunoAssertion; + be: BrunoAssertion; + have: BrunoAssertion; + a(type: string): BrunoAssertion; + equal(value: any): BrunoAssertion; + eql(value: any): BrunoAssertion; + include(value: any): BrunoAssertion; + property(name: string, value?: any): BrunoAssertion; + // ...extend as needed +} +declare const expect: (value: any) => BrunoAssertion;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/monaco/brunoApiTypes.js` around lines 49 - 66, The current declarations declare expect as any which disables IDE autocomplete; replace it with a minimal structural type (e.g., declare an interface like ChaiExpect/ChaiAssertion exposing the common chain properties and methods you want: .to and .be as nested objects that return an assertion type, and assertion methods like equal(expected: any): void, a(type: string): void, eql(expected: any): void, etc.) and then change "declare const expect: any;" to "declare const expect: ChaiExpect;" (or the chosen name); keep the test declaration as-is but ensure any async return types remain supported (declare function test(name: string, fn: () => void | Promise<void>): void;). Use the existing symbols expect and test to locate where to replace the any with the new minimal interface so editors can surface chained completions without full Chai typing.
19-642: Move the.d.tspayload out of the JS string literal.Keeping ~600 lines of TypeScript declarations inside a backtick template works, but it nukes editor tooling on the source-of-truth itself (no TS diagnostics/formatting, escaped backticks in JSDoc examples are easy to miss). Consider extracting into
bruno-api.d.tsand importing with a raw/text loader (rsbuild supports this viaoutput.dataUriLimit/source.assetsIncludeor a?rawimport). Improves maintainability as the API surface grows.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/monaco/brunoApiTypes.js` around lines 19 - 642, The large TypeScript declaration string assigned to the typeDefs constant in brunoApiTypes.js should be moved into a separate .d.ts file (e.g., bruno-api.d.ts) and loaded as raw text at build time; create the .d.ts containing the declarations, update the build config or enable raw imports (esbuild source.assetsInclude or ?raw import) and replace the template literal in the typeDefs declaration with an import of the raw file (assign the imported string to typeDefs), and remove the in-file backtick block so the editor gets real TS diagnostics and formatting.packages/bruno-app/src/utils/monaco/variableHighlighting.js (1)
29-29: Nit:maskedEnvVariablesis destructured but never used.Pull it out of the destructure (or start using it to mask hover values — secrets in hover tooltips would be an actual problem). Same pattern on line 133.
Also applies to: 133-133
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js` at line 29, The destructured variable maskedEnvVariables in variableHighlighting.js (from the line that declares const { pathParams = {}, maskedEnvVariables = [], ...varLookup } = variables) is unused; either remove maskedEnvVariables from the destructure or implement masking logic so hover/tooltips do not reveal secrets (also fix the identical unused destructure at the later occurrence around line 133). Update the destructure to drop maskedEnvVariables if you won't use it, or use maskedEnvVariables in the hover/tooltip generation functions (search for varLookup usage and the hover rendering code in this module) to replace/mask any environment variable values before they are displayed.packages/bruno-app/rsbuild.config.mjs (1)
35-44: Nit: trailing comma inrulesarray.Per the repo style ("No trailing commas"), drop the comma after the closing
}on Line 43.✂️ Proposed fix
rules: [ { test: /monaco-editor\/esm\/vs\/.*\.worker/, parser: { javascript: { dynamicImportMode: 'lazy' } } - }, + } ],As per coding guidelines: "No trailing commas."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/rsbuild.config.mjs` around lines 35 - 44, Remove the trailing comma after the rules array entry: locate the rules array object whose test is /monaco-editor\/esm\/vs\/.*\.worker/ (the object containing parser.javascript.dynamicImportMode = 'lazy') and delete the comma following its closing brace so the array ends without a trailing comma to comply with the "No trailing commas" style rule.packages/bruno-app/src/components/MonacoEditor/StyledWrapper.js (1)
16-20: Overriding Monaco internal selectors with!importantis fragile.Targeting
.monaco-editor .margin-view-overlays .line-numbersbypasses Monaco's theming API. If Monaco changes its DOM structure/class names in a minor version bump, this silently stops working. PreferlineNumbersMinChars/lineDecorationsWidthoptions on the editor (already set inMonacoEditor/index.js) and drop this override if possible, or at least add a comment pinning the Monaco version this was validated against.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/MonacoEditor/StyledWrapper.js` around lines 16 - 20, Remove the fragile CSS override in StyledWrapper.js that targets ".monaco-editor .margin-view-overlays .line-numbers" and instead rely on the editor options already configured in MonacoEditor/index.js (lineNumbersMinChars and lineDecorationsWidth); if you cannot remove the override for layout reasons, replace it with a clear comment in StyledWrapper.js noting the exact Monaco version this override was validated against and why it’s needed, and add a TODO to remove it when upgrading Monaco—reference the ".monaco-editor .margin-view-overlays .line-numbers" selector and the MonacoEditor/index.js settings in your change.packages/bruno-app/src/components/MonacoEditor/index.js (1)
145-157: Variable-highlighting effect clobbers the mount-time setup on first render.The mount effect at Line 117–121 already wires up
setupVariableHighlighting/registerVariableHoverProvider. This effect then fires immediately after mount with the samecollection/item, tears the pair down, and re-creates them. Harmless but duplicate work on every editor mount.Consider moving the setup solely into this effect (which also handles updates), or guard it with an
initializedref so it skips the first run.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/MonacoEditor/index.js` around lines 145 - 157, The variable-highlighting useEffect that depends on collection and item immediately runs after the mount effect and unnecessarily tears down and re-creates highlighting; modify this by consolidating mount/setup into this single useEffect (the one using editorRef, variableCleanupRef, hoverProviderRef) so it handles both initial mount and subsequent updates, or add an initialized ref (e.g., initializedRef) checked at the top of this effect to skip the first run so you don't call variableCleanupRef.current?.() and hoverProviderRef.current?.dispose() on mount; ensure you still call setupVariableHighlighting(editor, collection, item) and registerVariableHoverProvider(collection, item) and store their cleanup/disposable in variableCleanupRef.current and hoverProviderRef.current respectively so updates continue to work.packages/bruno-app/src/components/ScriptEditor/index.spec.js (1)
26-38: Mock store is missing slicesScriptEditorlikely reads.If
ScriptEditor(or its children once not mocked) ever selects fromstate.tabsor other slices, these tests will crash. Consider adding minimaltabs/collectionsreducers to the mock store now to avoid future flakiness, or drop the reduxProviderentirely and mockuseSelectordirectly to return just thebetaflag — leaner and more focused on the behaviour under test.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/ScriptEditor/index.spec.js` around lines 26 - 38, The test's createMockStore currently only provides app.preferences.beta ('monaco-editor') but ScriptEditor (or its children) may read other slices like state.tabs or state.collections causing brittle failures; either extend createMockStore to include minimal stub reducers for tabs and collections (e.g., return sensible default shapes used by ScriptEditor) or remove the redux Provider and mock useSelector directly to return only the beta flag — update tests to reference createMockStore and ScriptEditor (or replace Provider usage) so the component sees the expected slices during rendering.packages/bruno-app/src/components/MonacoEditor/index.spec.js (1)
188-191: Consider asserting which keybindings are registered.
toHaveBeenCalledTimes(2)will silently pass if someone later swaps Ctrl+Enter for some other chord. Inspect the first argument to verify theCtrlCmd | EnterandCtrlCmd | KeySchord values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/MonacoEditor/index.spec.js` around lines 188 - 191, The test currently only checks mockAddCommand was called twice; instead assert the specific key chords were registered: after calling renderEditor({ onRun, onSave }) inspect mockAddCommand calls (mockAddCommand.mock.calls or Jest's toHaveBeenNthCalledWith) and verify the first argument for the onRun registration equals the CtrlCmd|Enter chord and the first argument for the onSave registration equals the CtrlCmd|KeyS chord (use your monaco enums/bitmasks used elsewhere), while still allowing the handler callback to be any function; reference the test name 'registers keybindings for onRun and onSave', the renderEditor helper, and mockAddCommand to locate where to add these assertions.
🤖 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/package.json`:
- Line 63: MonacoEditor is currently statically imported causing the ~2MB bundle
to load for all users; change the import in ScriptEditor (where useMonaco/beta
flag is checked) to lazy-load the component instead of importing it from
MonacoEditor/index.js at module top-level: replace the static import with
React.lazy(() => import('components/MonacoEditor')) and render it only when the
beta flag/useMonaco is true inside a Suspense fallback, otherwise render
CodeEditor; ensure no other files keep a top-level import of MonacoEditor so the
bundle is deferred until the component is actually needed.
In `@packages/bruno-app/src/components/MonacoEditor/index.js`:
- Around line 160-171: The external `value` sync currently calls editor.setValue
which resets selection and scroll; update the effect in useEffect to preserve
and restore editor state or apply edits instead: get the model via
editor.getModel(), capture current selections via editor.getSelections() and
scrollTop via editor.getScrollTop(), then either replace text using
model.pushEditOperations (or model.applyEdits) to update content without
resetting caret/viewport, or if setValue must be used, call editor.setValue(...)
and immediately restore selections with editor.setSelections(...) and scroll
with editor.setScrollTop(...); reference editorRef.current,
cachedValueRef.current, editor.getPosition / setValue, and
model.pushEditOperations when making the change.
- Around line 54-143: Remove the redundant registerBrunoTheme call from the
mount useEffect (the one that runs editor creation) and let the existing
theme-sync effect be the single source of truth; during editor creation
(monaco.editor.create) do not call registerBrunoTheme — instead either set a
stable initial theme name or omit the theme option and after create call
monaco.editor.setTheme(...) (or editor.updateOptions({ theme: stableName }))
from the theme-sync effect where registerBrunoTheme and setTheme already run so
Monaco only gets a single, stable theme registration; update references to
registerBrunoTheme, monaco.editor.create, monaco.editor.setTheme, and
editor.updateOptions accordingly.
- Around line 216-228: The returned JSX is missing a data-testid on the editor
container (blocking Playwright) and is passing dead props (font, fontSize) into
StyledWrapper which aren't consumed; update the component (MonacoEditor) so the
inner div with ref={containerRef} gets a data-testid (e.g.,
"monaco-editor-container") for tests, and either remove the unused font and
fontSize props from the StyledWrapper invocation or wire them into
StyledWrapper.js (e.g., read props.font/props.fontSize in StyledWrapper) so they
are actually applied—adjust whichever approach matches project conventions and
update any related prop typings.
In `@packages/bruno-app/src/components/MonacoEditor/index.spec.js`:
- Around line 214-217: The test currently only asserts mockGetDomNode was
called; update the spec to spy on the returned element's classList.add and
assert it was called with 'mousetrap' by making mockGetDomNode return an object
whose querySelector returns { classList: { add: jest.fn() } }, call
renderEditor(), then assert that the captured classList.add spy was invoked with
'mousetrap' (reference mocks: mockGetDomNode, renderEditor, and the element's
classList.add).
In `@packages/bruno-app/src/components/ScriptEditor/index.js`:
- Around line 6-10: The current ScriptEditor chooses Editor = useMonaco ?
MonacoEditor : CodeEditor which re-mounts the editor when the
BETA_FEATURES.MONACO_EDITOR flag toggles and loses in-memory state; change
ScriptEditor to render both MonacoEditor and CodeEditor simultaneously (both
mounted) and switch visibility via CSS/props based on useMonaco so the active
editor is shown but the inactive one remains mounted and preserves state;
reference ScriptEditor, useMonaco (BETA_FEATURES.MONACO_EDITOR), MonacoEditor
and CodeEditor and ensure both receive the same props so their state is retained
across toggles.
- Around line 6-10: ScriptEditor currently drops any ref because it's a plain
function component; forward the ref through ScriptEditor to the chosen Editor
(use React.forwardRef and accept (props, ref)), so callers using
preRequestEditorRef / postResponseEditorRef receive the inner instance;
additionally update MonacoEditor to implement the same imperative API as
CodeEditor (expose an .editor property and a refresh() method) using
useImperativeHandle so the forwarded ref for the Monaco path supports
editor.refresh(), keeping the CodeEditor class ref behavior intact.
In `@packages/bruno-app/src/components/ScriptEditor/index.spec.js`:
- Around line 71-92: The test currently never verifies props because the mock is
registered after the ScriptEditor import; change the mock setup to run before
the component import and capture props with a jest.fn. Replace the
jest.isolateModules/doMock pattern with a top-level mock for
'components/CodeEditor' that delegates to a jest.fn (e.g., codeEditorMock) so
ScriptEditor uses it when rendered, then assert codeEditorMock was called with
an objectContaining({ mode: 'javascript', onEdit: mockOnEdit }) instead of only
checking getByTestId; ensure you reference ScriptEditor and the mocked
CodeEditor (codeEditorMock) in the assertions.
In `@packages/bruno-app/src/utils/monaco/brunoTheme.js`:
- Line 26: The themeName currently uses Date.now() which creates a new unique
name on every registerBrunoTheme call (const themeName =
`bruno-${displayedTheme}-${Date.now()}`), leaking Monaco theme registrations;
change registerBrunoTheme to use a deterministic name keyed by displayedTheme
(e.g., `bruno-${displayedTheme}`) and, if you need cache-busting when the
palette contents change, append a short content hash of the palette instead of
Date.now(); ensure the same name is reused so monaco.editor.defineTheme is
idempotent and themes are not continuously accumulated.
- Around line 22-24: normalizeColor can return undefined for hsl(...) inputs so
editorBg, editorFg, and gutterBgNorm may be undefined when assigned and then
passed into Monaco (e.g., colors['editor.background']), causing Monaco to ignore
them; fix by making normalizeColor return a concrete theme-aware hex fallback
(use the theme param to choose a light/dark default) or, alternatively, ensure
each assignment uses a sensible fallback (e.g., const editorBg =
normalizeColor(bg) || '#ffffff' for light theme / '#0b0b0b' for dark) where
editorBg, editorFg, and gutterBgNorm are computed (also apply same pattern at
the other occurrences around lines 58-60); reference normalizeColor, editorBg,
editorFg, gutterBgNorm, and the colors['editor.background'] assignments when
applying the change.
In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js`:
- Line 47: The expression using
mockDataFunctions.hasOwnProperty(varName.substring(1)) is unsafe and triggers
no-prototype-builtins; update the checks that reference mockDataFunctions (e.g.,
the isMockVariable assignment and the other occurrence noted) to use a safe
own-property test such as Object.hasOwn(mockDataFunctions, varName.substring(1))
(or Object.prototype.hasOwnProperty.call(mockDataFunctions,
varName.substring(1)) if you need older runtimes), keeping varName and
mockDataFunctions as the referenced symbols.
- Around line 24-70: The editor decoration update is triggered on every
keystroke by editor.onDidChangeModelContent causing expensive work in
updateDecorations (getAllVariables, model.getValue, VARIABLE_REGEX.exec,
model.getPositionAt, editor.deltaDecorations) — wrap calls to updateDecorations
in a short debounce (≈100–150ms) so rapid keystrokes batch into one run;
implement by keeping a timeout id scoped next to updateDecorations, call
clearTimeout and setTimeout in the onDidChangeModelContent handler (or use an
existing debounce util) and ensure the initial call still runs immediately to
populate decorations.
---
Outside diff comments:
In `@packages/bruno-app/src/components/CollectionSettings/Script/index.js`:
- Around line 42-52: useEffect in this file calls
preRequestEditorRef.current.editor.refresh() /
postResponseEditorRef.current.editor.refresh() but those refs are null because
the new ScriptEditor component doesn't forward refs; fix by updating
ScriptEditor (ScriptEditor/index.js) to use React.forwardRef and attach the
inner editor instance (or an object with an .editor property) to the forwarded
ref so preRequestEditorRef and postResponseEditorRef receive the expected shape
when ScriptEditor mounts.
In `@packages/bruno-app/src/components/FolderSettings/Script/index.js`:
- Around line 105-132: ScriptEditor currently forwards refs to both CodeEditor
and MonacoEditor but MonacoEditor is a function component without forwardRef so
preRequestEditorRef/postResponseEditorRef end up null and the useEffect that
calls .editor.refresh() (line ~43) triggers React warnings; fix by either
wrapping MonacoEditor with React.forwardRef and exposing an object with
editor.refresh() (noop is acceptable) so the ref shape matches CodeEditor, or
detect the MONACO_EDITOR path in ScriptEditor and do not pass ref props to
MonacoEditor; update MonacoEditor (or ScriptEditor) so refs are consistent with
the expected { editor: { refresh() } } shape referenced by the useEffect.
---
Duplicate comments:
In `@packages/bruno-app/src/components/RequestPane/Script/index.js`:
- Around line 37-48: The refresh-on-tab-switch is a no-op because ScriptEditor
does not forward its ref, so preRequestEditorRef and postResponseEditorRef never
get populated; fix by updating ScriptEditor (the component in
ScriptEditor/index.js) to use React.forwardRef and pass the received ref through
to the inner ScriptEditor instance or the underlying editor component (ensuring
the forwarded ref points to an object with .editor), i.e., accept (props, ref)
and attach/ref={ref} (or assign ref.current = innerRef) so that
preRequestEditorRef.current.editor and postResponseEditorRef.current.editor are
defined and the useEffect refresh logic in RequestPane/Script works as intended.
---
Nitpick comments:
In `@packages/bruno-app/rsbuild.config.mjs`:
- Around line 35-44: Remove the trailing comma after the rules array entry:
locate the rules array object whose test is /monaco-editor\/esm\/vs\/.*\.worker/
(the object containing parser.javascript.dynamicImportMode = 'lazy') and delete
the comma following its closing brace so the array ends without a trailing comma
to comply with the "No trailing commas" style rule.
In `@packages/bruno-app/src/components/MonacoEditor/index.js`:
- Around line 145-157: The variable-highlighting useEffect that depends on
collection and item immediately runs after the mount effect and unnecessarily
tears down and re-creates highlighting; modify this by consolidating mount/setup
into this single useEffect (the one using editorRef, variableCleanupRef,
hoverProviderRef) so it handles both initial mount and subsequent updates, or
add an initialized ref (e.g., initializedRef) checked at the top of this effect
to skip the first run so you don't call variableCleanupRef.current?.() and
hoverProviderRef.current?.dispose() on mount; ensure you still call
setupVariableHighlighting(editor, collection, item) and
registerVariableHoverProvider(collection, item) and store their
cleanup/disposable in variableCleanupRef.current and hoverProviderRef.current
respectively so updates continue to work.
In `@packages/bruno-app/src/components/MonacoEditor/index.spec.js`:
- Around line 188-191: The test currently only checks mockAddCommand was called
twice; instead assert the specific key chords were registered: after calling
renderEditor({ onRun, onSave }) inspect mockAddCommand calls
(mockAddCommand.mock.calls or Jest's toHaveBeenNthCalledWith) and verify the
first argument for the onRun registration equals the CtrlCmd|Enter chord and the
first argument for the onSave registration equals the CtrlCmd|KeyS chord (use
your monaco enums/bitmasks used elsewhere), while still allowing the handler
callback to be any function; reference the test name 'registers keybindings for
onRun and onSave', the renderEditor helper, and mockAddCommand to locate where
to add these assertions.
In `@packages/bruno-app/src/components/MonacoEditor/StyledWrapper.js`:
- Around line 16-20: Remove the fragile CSS override in StyledWrapper.js that
targets ".monaco-editor .margin-view-overlays .line-numbers" and instead rely on
the editor options already configured in MonacoEditor/index.js
(lineNumbersMinChars and lineDecorationsWidth); if you cannot remove the
override for layout reasons, replace it with a clear comment in StyledWrapper.js
noting the exact Monaco version this override was validated against and why it’s
needed, and add a TODO to remove it when upgrading Monaco—reference the
".monaco-editor .margin-view-overlays .line-numbers" selector and the
MonacoEditor/index.js settings in your change.
In `@packages/bruno-app/src/components/ScriptEditor/index.spec.js`:
- Around line 26-38: The test's createMockStore currently only provides
app.preferences.beta ('monaco-editor') but ScriptEditor (or its children) may
read other slices like state.tabs or state.collections causing brittle failures;
either extend createMockStore to include minimal stub reducers for tabs and
collections (e.g., return sensible default shapes used by ScriptEditor) or
remove the redux Provider and mock useSelector directly to return only the beta
flag — update tests to reference createMockStore and ScriptEditor (or replace
Provider usage) so the component sees the expected slices during rendering.
In `@packages/bruno-app/src/utils/monaco/brunoApiTypes.js`:
- Around line 49-66: The current declarations declare expect as any which
disables IDE autocomplete; replace it with a minimal structural type (e.g.,
declare an interface like ChaiExpect/ChaiAssertion exposing the common chain
properties and methods you want: .to and .be as nested objects that return an
assertion type, and assertion methods like equal(expected: any): void, a(type:
string): void, eql(expected: any): void, etc.) and then change "declare const
expect: any;" to "declare const expect: ChaiExpect;" (or the chosen name); keep
the test declaration as-is but ensure any async return types remain supported
(declare function test(name: string, fn: () => void | Promise<void>): void;).
Use the existing symbols expect and test to locate where to replace the any with
the new minimal interface so editors can surface chained completions without
full Chai typing.
- Around line 19-642: The large TypeScript declaration string assigned to the
typeDefs constant in brunoApiTypes.js should be moved into a separate .d.ts file
(e.g., bruno-api.d.ts) and loaded as raw text at build time; create the .d.ts
containing the declarations, update the build config or enable raw imports
(esbuild source.assetsInclude or ?raw import) and replace the template literal
in the typeDefs declaration with an import of the raw file (assign the imported
string to typeDefs), and remove the in-file backtick block so the editor gets
real TS diagnostics and formatting.
In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js`:
- Line 29: The destructured variable maskedEnvVariables in
variableHighlighting.js (from the line that declares const { pathParams = {},
maskedEnvVariables = [], ...varLookup } = variables) is unused; either remove
maskedEnvVariables from the destructure or implement masking logic so
hover/tooltips do not reveal secrets (also fix the identical unused destructure
at the later occurrence around line 133). Update the destructure to drop
maskedEnvVariables if you won't use it, or use maskedEnvVariables in the
hover/tooltip generation functions (search for varLookup usage and the hover
rendering code in this module) to replace/mask any environment variable values
before they are displayed.
🪄 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: 89fc457a-5dc3-401a-9883-266fb77b22b4
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (22)
packages/bruno-app/package.jsonpackages/bruno-app/rsbuild.config.mjspackages/bruno-app/src/components/CollectionSettings/Script/index.jspackages/bruno-app/src/components/CollectionSettings/Tests/index.jspackages/bruno-app/src/components/FolderSettings/Script/index.jspackages/bruno-app/src/components/FolderSettings/Tests/index.jspackages/bruno-app/src/components/MonacoEditor/StyledWrapper.jspackages/bruno-app/src/components/MonacoEditor/index.jspackages/bruno-app/src/components/MonacoEditor/index.spec.jspackages/bruno-app/src/components/Preferences/Beta/index.jspackages/bruno-app/src/components/RequestPane/Script/index.jspackages/bruno-app/src/components/RequestPane/Tests/index.jspackages/bruno-app/src/components/ScriptEditor/index.jspackages/bruno-app/src/components/ScriptEditor/index.spec.jspackages/bruno-app/src/utils/beta-features.jspackages/bruno-app/src/utils/monaco/brunoApiTypes.jspackages/bruno-app/src/utils/monaco/brunoTheme.jspackages/bruno-app/src/utils/monaco/languageMapping.jspackages/bruno-app/src/utils/monaco/languageMapping.spec.jspackages/bruno-app/src/utils/monaco/variableHighlighting.jspackages/bruno-app/src/utils/monaco/workers.jspackages/bruno-electron/src/store/preferences.js
| "mime-types": "^3.0.2", | ||
| "moment": "^2.30.1", | ||
| "moment-timezone": "^0.5.47", | ||
| "monaco-editor": "^0.55.1", |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check how ScriptEditor imports MonacoEditor — static vs dynamic.
fd -t f 'index.js' packages/bruno-app/src/components/ScriptEditor | xargs rg -n -C3 -e "MonacoEditor" -e "React\.lazy" -e "import\("Repository: usebruno/bruno
Length of output: 469
🏁 Script executed:
fd -t f 'index.js' packages/bruno-app/src/components/MonacoEditor | head -5 | xargs cat -nRepository: usebruno/bruno
Length of output: 8586
🏁 Script executed:
rg -n "React\.lazy|dynamic.*import|import.*monaco" packages/bruno-app/src/components/MonacoEditor --type js --type jsxRepository: usebruno/bruno
Length of output: 85
Lazy-load Monaco behind the beta flag.
monaco-editor is statically imported in MonacoEditor/index.js (line 3), which means the entire ~2MB minified bundle loads for all users on app startup—even though the feature is opt-in beta only. Currently, ScriptEditor conditionally renders either MonacoEditor or CodeEditor, but this conditional happens after the static import has already pulled the bundle.
Move the MonacoEditor import in ScriptEditor/index.js to use React.lazy(), and defer loading until the beta flag is actually enabled:
const MonacoEditor = lazy(() => import('components/MonacoEditor'));
const Editor = useMonaco ? <Suspense fallback={...}><MonacoEditor {...props} /></Suspense> : <CodeEditor {...props} />;This prevents non-beta users from incurring the bundle cost on initial load.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/package.json` at line 63, MonacoEditor is currently
statically imported causing the ~2MB bundle to load for all users; change the
import in ScriptEditor (where useMonaco/beta flag is checked) to lazy-load the
component instead of importing it from MonacoEditor/index.js at module
top-level: replace the static import with React.lazy(() =>
import('components/MonacoEditor')) and render it only when the beta
flag/useMonaco is true inside a Suspense fallback, otherwise render CodeEditor;
ensure no other files keep a top-level import of MonacoEditor so the bundle is
deferred until the component is actually needed.
…d tooltip support
- Added variable autocomplete functionality to the Monaco Editor, providing suggestions for {{variable}} patterns.
- Implemented a rich tooltip system for variable hover interactions, improving user experience.
- Updated ScriptEditor and RequestBody components to utilize the new Monaco Editor features.
- Refactored variable highlighting and tooltip logic for better performance and maintainability.
- Added tests to ensure proper functionality of the new autocomplete and tooltip features.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (5)
packages/bruno-app/src/utils/monaco/variableHighlighting.js (3)
493-528: Tooltip doesn't follow or dismiss on editor scroll.Popup uses
position: fixedwith coordinates computed at show time. Scrolling the editor moves the{{variable}}but the popup stays pinned, drifting away from its target. Consider subscribing toeditor.onDidScrollChangeto either reposition or hide the active popup (alongside the existingonDidChangeModelContenthide).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js` around lines 493 - 528, The tooltip is positioned once (showTooltip) and uses fixed coordinates so it drifts when the editor is scrolled; subscribe to the editor.onDidScrollChange event and in that handler either call the existing showTooltip logic to recompute and reposition the active popup (using currentVarName/varInfo) or call hidePopup/startHideTimer to dismiss it; attach the listener when setting up (alongside onMouseMove/onMouseLeave/onDidChangeModelContent) and ensure you clear the scroll listener when disposing so hoverTimeout/hideTimeout and activePopup are correctly handled.
538-539: Import statement at bottom of file.
import { updateVariableInScope } from 'providers/ReduxStore/slices/collections/actions'sits at Line 539, below the comment at 538. ES module imports are hoisted so this works, but placing it at the bottom (presumably to paper over a circular dep withactions.js) is easy to miss, hostile to bundler tree-shaking heuristics, and tripwire for lint rules likeimport/first. Hoist it to the top with the other imports, and if it genuinely introduces a cycle, refactor the action into a cycle-free module.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js` around lines 538 - 539, Move the import "updateVariableInScope" out of the bottom of variableHighlighting.js and place it with the other top-level imports so all ES module imports are hoisted consistently; specifically, relocate the line importing updateVariableInScope from 'providers/ReduxStore/slices/collections/actions' up into the module's import block (near other imports in variableHighlighting.js) and run the linter/build; if doing so exposes a circular dependency involving actions.js, extract the updateVariableInScope logic (or a small dispatch wrapper) into a new cycle-free module that both variableHighlighting.js and collections/actions can import to break the cycle.
6-6: Duplicate inline{{…}}regex.Line 124 re-declares the same pattern as the module-level
VARIABLE_REGEX(minusgbehavior since it's inlined formatchAll). Reuse the constant (or a shared factory) to keep regex maintenance in one place.Also applies to: 124-124
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js` at line 6, The inline duplication of the same {{…}} pattern should be removed: replace the ad-hoc regex used where matchAll is called with the module-level VARIABLE_REGEX (or create a RegExp from VARIABLE_REGEX.source with the proper flags for matchAll) so the pattern is maintained in one place; update the code that currently declares the inline regex (used with matchAll) to reuse VARIABLE_REGEX (or new RegExp(VARIABLE_REGEX.source, 'g') if specific flags are needed) so maintenance and behavior are consistent.packages/bruno-app/src/utils/monaco/autocomplete.js (2)
177-194: Redundant suggest trigger may double-fire.
triggerCharacters: ['{', '.', '$']already covers the typical entry points. TheonDidChangeModelContenthandler additionally fireseditor.action.triggerSuggeston every keystroke when the pattern matches — useful when typing word chars inside{{…, but it will also re-trigger while the widget is already open on each keystroke. Monaco tolerates this, but you can gate it to only fire when the widget isn't already visible (e.g., viaeditor.getContribution('editor.contrib.suggestController')) to avoid churn.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/monaco/autocomplete.js` around lines 177 - 194, The onDidChangeModelContent handler (contentChangeDisposable) unconditionally calls editor.action.triggerSuggest when VARIABLE_PATTERN matches, which can re-trigger suggestions on every keystroke; update the handler to first check the suggest widget visibility via the suggest controller (editor.getContribution('editor.contrib.suggestController')) and only call editor.trigger('bruno-autocomplete','editor.action.triggerSuggest',{}) when the controller indicates the suggest widget is not currently visible. Ensure you reference VARIABLE_PATTERN, contentChangeDisposable, editor.onDidChangeModelContent and the suggest controller call to gate the trigger.
128-135:getAllVariablesrecomputed on every keystroke.Combined with
incomplete: trueand the content-change re-trigger at Lines 177‑194,getAllVariables(collection, item)— which walks the collection tree, merges envs, masked vars, oauth, path params, etc. — runs per keystroke per language provider. On large collections this adds up. Consider caching the lookup per model version or behind a short memoization keyed on(collection.uid, item?.uid, activeEnvironmentUid).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/monaco/autocomplete.js` around lines 128 - 135, getAllVariables(collection, item) is being recomputed on every keystroke; wrap it in a short-lived memoization/cache so you avoid walking the entire collection per keystroke. Implement a module-level Map cache keyed by `${collection.uid}:${item?.uid || ''}:${activeEnvironmentUid}` (or include the editor/model version if available) and return cached allVariables when the key matches; on cache miss compute via getAllVariables and store the result. Also ensure the cache is invalidated/updated when collection/item/environment/model version changes (e.g., clear entry when collection.uid or activeEnvironmentUid changes) so buildVariableHints and subsequent filters use the cached lookup instead of recomputing each keystroke.
🤖 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/MonacoEditor/index.js`:
- Around line 127-136: The mount effect currently initializes variable
highlighting/tooltips only using the initial enableVariableHighlighting value
and the re-apply effect (which references variableCleanupRef, tooltipCleanupRef
and setupVariableHighlighting/setupVariableTooltip) skips when
enableVariableHighlighting is false and lacks enableVariableHighlighting in its
dependency list; update the re-apply effect to include
enableVariableHighlighting in its deps and on change handle first-time setup and
cleanup: when enableVariableHighlighting becomes true call
setupVariableHighlighting(editor, collectionRef.current, itemRef.current) and
setupVariableTooltip(editor, () => collectionRef.current, () => itemRef.current,
dispatch), storing cleanup into variableCleanupRef.current and
tooltipCleanupRef.current; when it becomes false run any stored cleanups and
clear the refs so toggling post-mount properly enables/disables highlighting and
tooltips.
In `@packages/bruno-app/src/utils/monaco/autocomplete.js`:
- Line 7: The regex stored in VARIABLE_PATTERN currently excludes the question
mark so prompt-style variables like "{{?name" won't trigger completions; update
VARIABLE_PATTERN to allow '?' inside the character class (e.g., include '?' or
escape the hyphen appropriately) so "{{?..." matches, and add a short JSDoc
above VARIABLE_PATTERN describing that it intentionally includes prompt-style
prefixes (or, if you decide to leave it excluding '?', document that choice) to
make the intent clear for future readers.
In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js`:
- Around line 453-457: The tooltip positioning divides pixel coordinates by 16
to produce rem units, which breaks placement when the root font-size differs;
instead remove the /16 conversion and set popup.style.top and popup.style.left
using pixel values (e.g., `${top}px` and `${left}px`) where popup, top, and left
are computed (via getBoundingClientRect()/getScrolledVisiblePosition()) so the
tooltip aligns correctly in all zoom/font-size settings.
- Around line 59-65: The captured variable name from VARIABLE_REGEX is not
trimmed, so whitespace like in "{{ foo }}" yields " foo " and fails scope
lookup; update the handling in variableHighlighting.js to use a trimmed name for
resolution by changing the usage of match[1] to match[1].trim() (e.g., replace
const varName = match[1]; with a trimmed variant) before any scope/variable
lookup and validation, but keep startOffset/endOffset and
model.getPositionAt(...) logic unchanged so highlighting ranges remain correct;
apply the same trim fix to the other occurrence around lines 123-130 as well.
---
Nitpick comments:
In `@packages/bruno-app/src/utils/monaco/autocomplete.js`:
- Around line 177-194: The onDidChangeModelContent handler
(contentChangeDisposable) unconditionally calls editor.action.triggerSuggest
when VARIABLE_PATTERN matches, which can re-trigger suggestions on every
keystroke; update the handler to first check the suggest widget visibility via
the suggest controller
(editor.getContribution('editor.contrib.suggestController')) and only call
editor.trigger('bruno-autocomplete','editor.action.triggerSuggest',{}) when the
controller indicates the suggest widget is not currently visible. Ensure you
reference VARIABLE_PATTERN, contentChangeDisposable,
editor.onDidChangeModelContent and the suggest controller call to gate the
trigger.
- Around line 128-135: getAllVariables(collection, item) is being recomputed on
every keystroke; wrap it in a short-lived memoization/cache so you avoid walking
the entire collection per keystroke. Implement a module-level Map cache keyed by
`${collection.uid}:${item?.uid || ''}:${activeEnvironmentUid}` (or include the
editor/model version if available) and return cached allVariables when the key
matches; on cache miss compute via getAllVariables and store the result. Also
ensure the cache is invalidated/updated when collection/item/environment/model
version changes (e.g., clear entry when collection.uid or activeEnvironmentUid
changes) so buildVariableHints and subsequent filters use the cached lookup
instead of recomputing each keystroke.
In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js`:
- Around line 493-528: The tooltip is positioned once (showTooltip) and uses
fixed coordinates so it drifts when the editor is scrolled; subscribe to the
editor.onDidScrollChange event and in that handler either call the existing
showTooltip logic to recompute and reposition the active popup (using
currentVarName/varInfo) or call hidePopup/startHideTimer to dismiss it; attach
the listener when setting up (alongside
onMouseMove/onMouseLeave/onDidChangeModelContent) and ensure you clear the
scroll listener when disposing so hoverTimeout/hideTimeout and activePopup are
correctly handled.
- Around line 538-539: Move the import "updateVariableInScope" out of the bottom
of variableHighlighting.js and place it with the other top-level imports so all
ES module imports are hoisted consistently; specifically, relocate the line
importing updateVariableInScope from
'providers/ReduxStore/slices/collections/actions' up into the module's import
block (near other imports in variableHighlighting.js) and run the linter/build;
if doing so exposes a circular dependency involving actions.js, extract the
updateVariableInScope logic (or a small dispatch wrapper) into a new cycle-free
module that both variableHighlighting.js and collections/actions can import to
break the cycle.
- Line 6: The inline duplication of the same {{…}} pattern should be removed:
replace the ad-hoc regex used where matchAll is called with the module-level
VARIABLE_REGEX (or create a RegExp from VARIABLE_REGEX.source with the proper
flags for matchAll) so the pattern is maintained in one place; update the code
that currently declares the inline regex (used with matchAll) to reuse
VARIABLE_REGEX (or new RegExp(VARIABLE_REGEX.source, 'g') if specific flags are
needed) so maintenance and behavior are consistent.
🪄 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: eca7ed6c-f76c-43d4-841c-c60ebcb15b8d
📒 Files selected for processing (9)
packages/bruno-app/src/components/FolderSettings/Script/index.jspackages/bruno-app/src/components/FolderSettings/Tests/index.jspackages/bruno-app/src/components/MonacoEditor/index.jspackages/bruno-app/src/components/MonacoEditor/index.spec.jspackages/bruno-app/src/components/RequestPane/RequestBody/index.jspackages/bruno-app/src/components/RequestPane/Script/index.jspackages/bruno-app/src/components/RequestPane/Tests/index.jspackages/bruno-app/src/utils/monaco/autocomplete.jspackages/bruno-app/src/utils/monaco/variableHighlighting.js
✅ Files skipped from review due to trivial changes (2)
- packages/bruno-app/src/components/FolderSettings/Tests/index.js
- packages/bruno-app/src/components/MonacoEditor/index.spec.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bruno-app/src/components/RequestPane/Tests/index.js
- packages/bruno-app/src/components/RequestPane/Script/index.js
Comment on lines
+59
to
+65
| while ((match = VARIABLE_REGEX.exec(text)) !== null) { | ||
| const varName = match[1]; | ||
| const startOffset = match.index; | ||
| const endOffset = startOffset + match[0].length; | ||
| const startPos = model.getPositionAt(startOffset); | ||
| const endPos = model.getPositionAt(endOffset); | ||
|
|
Contributor
There was a problem hiding this comment.
Variable name isn't trimmed — inconsistent with user expectation.
Both match sites capture [^}]* and use the raw group for scope lookup. {{ foo }} becomes variable name " foo " (with spaces), which won't match any declared variable and gets flagged invalid/red even though CodeMirror tolerates surrounding whitespace in many flows. Trim match[1] before scope resolution to keep behavior consistent across editors.
Also applies to: 123-130
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js` around lines 59
- 65, The captured variable name from VARIABLE_REGEX is not trimmed, so
whitespace like in "{{ foo }}" yields " foo " and fails scope lookup; update the
handling in variableHighlighting.js to use a trimmed name for resolution by
changing the usage of match[1] to match[1].trim() (e.g., replace const varName =
match[1]; with a trimmed variant) before any scope/variable lookup and
validation, but keep startOffset/endOffset and model.getPositionAt(...) logic
unchanged so highlighting ranges remain correct; apply the same trim fix to the
other occurrence around lines 123-130 as well.
…tip logic - Updated comment in MonacoEditor to clarify the registration of Bruno API types. - Added a condition in variableHighlighting to skip tooltips for prompt variables, improving tooltip management.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/MonacoEditor/index.js`:
- Around line 103-109: The onDidChangeModelContent listener (registered as
contentDisposable) is firing when the component updates the editor via
editor.setValue(...) and incorrectly invoking onEditRef.current; add a boolean
guard (e.g., isSettingValueRef) to skip calling onEditRef during programmatic
updates: set isSettingValueRef.current = true immediately before
editor.setValue(...) and reset to false after, and update the contentDisposable
callback to return early when isSettingValueRef.current is true; ensure
cachedValueRef updates still occur when syncing external values.
In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js`:
- Around line 430-433: The code currently tracks the active tooltip only by
variable name (currentVarName), causing the popup to persist when hovering a
different occurrence of the same variable; update positionAndShowPopup to set a
unique occurrence identifier (e.g., currentVarOccurrence =
`${varName}@${lineNumber}:${startCol}`) and use that identifier in all checks
where currentVarName is compared (replace uses of currentVarName in the
hover/leave logic with currentVarOccurrence), and ensure the old popup is
removed or replaced when the new occurrence id differs so each occurrence is
tracked independently.
- Around line 51-72: The validity check is excluding pathParams because
pathParams is destructured out of getAllVariables into its own variable and
never consulted; update the logic in variableHighlighting.js (around where
getAllVariables, pathParams, and varLookup are defined and where isValid is
computed) so pathParams are considered when determining validity—either merge
pathParams into varLookup after the destructure or change the isValid condition
(used with VARIABLE_REGEX matches, varName, mockDataFunctions, and
pathFoundInVariables) to also check pathFoundInVariables(varName, pathParams);
ensure references to getAllVariables, pathParams, varLookup, VARIABLE_REGEX,
mockDataFunctions, and pathFoundInVariables are used consistently.
- Around line 341-345: The blur handler currently calls
dispatch(updateVariableInScope(varName, newValue, scopeInfo, collection.uid))
without handling the returned Promise; modify the editorEl 'blur' listener so
that the Promise returned by updateVariableInScope is handled (either mark the
handler async and await the dispatch, or attach .catch) and surface failures
(e.g., log via console or process logger and optionally revert to rawValue or
show UI feedback). Locate the editorEl.addEventListener('blur', ...) block and
update the call to updateVariableInScope/updateVariableInScope(...) to handle
rejection for varName, newValue, scopeInfo, collection.uid.
🪄 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: b9e4bf4a-b485-4450-be64-fa1228e23e7e
📒 Files selected for processing (2)
packages/bruno-app/src/components/MonacoEditor/index.jspackages/bruno-app/src/utils/monaco/variableHighlighting.js
Comment on lines
+103
to
+109
| const contentDisposable = editor.onDidChangeModelContent(() => { | ||
| const newValue = editor.getValue(); | ||
| cachedValueRef.current = newValue; | ||
| if (onEditRef.current) { | ||
| onEditRef.current(newValue); | ||
| } | ||
| }); |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and examine the MonacoEditor file
find . -path "*/packages/bruno-app/src/components/MonacoEditor/index.js" -type fRepository: usebruno/bruno
Length of output: 115
🏁 Script executed:
# If found, check the file size and read the relevant sections
if [ -f "packages/bruno-app/src/components/MonacoEditor/index.js" ]; then
wc -l "packages/bruno-app/src/components/MonacoEditor/index.js"
fiRepository: usebruno/bruno
Length of output: 117
🏁 Script executed:
# Read the file with line numbers to examine lines 103-109 and 183-195
if [ -f "packages/bruno-app/src/components/MonacoEditor/index.js" ]; then
cat -n "packages/bruno-app/src/components/MonacoEditor/index.js" | sed -n '95,115p'
echo "---"
cat -n "packages/bruno-app/src/components/MonacoEditor/index.js" | sed -n '175,200p'
fiRepository: usebruno/bruno
Length of output: 1783
🏁 Script executed:
# Also check the entire structure around the useEffect hooks to understand the context
if [ -f "packages/bruno-app/src/components/MonacoEditor/index.js" ]; then
cat -n "packages/bruno-app/src/components/MonacoEditor/index.js" | sed -n '1,50p'
fiRepository: usebruno/bruno
Length of output: 2021
Suppress onEdit during external value sync.
Line 190 calls editor.setValue(...), which triggers the onDidChangeModelContent listener (lines 103-109). This causes prop-driven reloads to call onEdit and mark data as if the user typed it. Implement the flag-based guard to prevent this.
Proposed fix
const tooltipCleanupRef = useRef(null);
const autocompleteCleanupRef = useRef(null);
+ const isSyncingExternalValueRef = useRef(false);
@@
const contentDisposable = editor.onDidChangeModelContent(() => {
const newValue = editor.getValue();
cachedValueRef.current = newValue;
- if (onEditRef.current) {
+ if (!isSyncingExternalValueRef.current && onEditRef.current) {
onEditRef.current(newValue);
}
});
@@
if (value !== cachedValueRef.current) {
cachedValueRef.current = value;
const position = editor.getPosition();
- editor.setValue(value || '');
+ isSyncingExternalValueRef.current = true;
+ try {
+ editor.setValue(value || '');
+ } finally {
+ isSyncingExternalValueRef.current = false;
+ }
if (position) {
editor.setPosition(position);
}📝 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.
Suggested change
| const contentDisposable = editor.onDidChangeModelContent(() => { | |
| const newValue = editor.getValue(); | |
| cachedValueRef.current = newValue; | |
| if (onEditRef.current) { | |
| onEditRef.current(newValue); | |
| } | |
| }); | |
| const contentDisposable = editor.onDidChangeModelContent(() => { | |
| const newValue = editor.getValue(); | |
| cachedValueRef.current = newValue; | |
| if (!isSyncingExternalValueRef.current && onEditRef.current) { | |
| onEditRef.current(newValue); | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/components/MonacoEditor/index.js` around lines 103 -
109, The onDidChangeModelContent listener (registered as contentDisposable) is
firing when the component updates the editor via editor.setValue(...) and
incorrectly invoking onEditRef.current; add a boolean guard (e.g.,
isSettingValueRef) to skip calling onEditRef during programmatic updates: set
isSettingValueRef.current = true immediately before editor.setValue(...) and
reset to false after, and update the contentDisposable callback to return early
when isSettingValueRef.current is true; ensure cachedValueRef updates still
occur when syncing external values.
Comment on lines
+51
to
+72
| const variables = getAllVariables(collection, item); | ||
| const { pathParams = {}, maskedEnvVariables = [], ...varLookup } = variables; | ||
| const text = model.getValue(); | ||
| const newDecorations = []; | ||
|
|
||
| let match; | ||
| VARIABLE_REGEX.lastIndex = 0; | ||
|
|
||
| while ((match = VARIABLE_REGEX.exec(text)) !== null) { | ||
| const varName = match[1]; | ||
| const startOffset = match.index; | ||
| const endOffset = startOffset + match[0].length; | ||
| const startPos = model.getPositionAt(startOffset); | ||
| const endPos = model.getPositionAt(endOffset); | ||
|
|
||
| let className; | ||
| if (varName.startsWith('?')) { | ||
| className = 'bruno-variable-prompt'; | ||
| } else { | ||
| const isMockVariable = varName.startsWith('$') && mockDataFunctions.hasOwnProperty(varName.substring(1)); | ||
| const isValid = isMockVariable || pathFoundInVariables(varName, varLookup); | ||
| className = isValid ? 'bruno-variable-valid' : 'bruno-variable-invalid'; |
Contributor
There was a problem hiding this comment.
Include path params in the validity check.
Line 52 removes pathParams from varLookup, but Lines 70-72 never check it, so valid path params can be highlighted as invalid.
Proposed fix
- const isValid = isMockVariable || pathFoundInVariables(varName, varLookup);
+ const isValid = isMockVariable
+ || pathFoundInVariables(varName, varLookup)
+ || pathFoundInVariables(varName, pathParams);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js` around lines 51
- 72, The validity check is excluding pathParams because pathParams is
destructured out of getAllVariables into its own variable and never consulted;
update the logic in variableHighlighting.js (around where getAllVariables,
pathParams, and varLookup are defined and where isValid is computed) so
pathParams are considered when determining validity—either merge pathParams into
varLookup after the destructure or change the isValid condition (used with
VARIABLE_REGEX matches, varName, mockDataFunctions, and pathFoundInVariables) to
also check pathFoundInVariables(varName, pathParams); ensure references to
getAllVariables, pathParams, varLookup, VARIABLE_REGEX, mockDataFunctions, and
pathFoundInVariables are used consistently.
Comment on lines
+341
to
+345
| editorEl.addEventListener('blur', () => { | ||
| const newValue = editorEl.value; | ||
| if (newValue !== rawValue && dispatch) { | ||
| dispatch(updateVariableInScope(varName, newValue, scopeInfo, collection.uid)); | ||
| } |
Contributor
There was a problem hiding this comment.
Handle failed variable updates.
updateVariableInScope(...) returns a Promise and can reject; leaving the dispatch unhandled can surface unhandled rejections when persistence fails.
Proposed fix
editorEl.addEventListener('blur', () => {
const newValue = editorEl.value;
if (newValue !== rawValue && dispatch) {
- dispatch(updateVariableInScope(varName, newValue, scopeInfo, collection.uid));
+ dispatch(updateVariableInScope(varName, newValue, scopeInfo, collection.uid))
+ .catch(() => {
+ editorEl.value = rawValue;
+ });
}
editorEl.style.display = 'none';
valueDisplay.style.display = '';📝 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.
Suggested change
| editorEl.addEventListener('blur', () => { | |
| const newValue = editorEl.value; | |
| if (newValue !== rawValue && dispatch) { | |
| dispatch(updateVariableInScope(varName, newValue, scopeInfo, collection.uid)); | |
| } | |
| editorEl.addEventListener('blur', () => { | |
| const newValue = editorEl.value; | |
| if (newValue !== rawValue && dispatch) { | |
| dispatch(updateVariableInScope(varName, newValue, scopeInfo, collection.uid)) | |
| .catch(() => { | |
| editorEl.value = rawValue; | |
| }); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js` around lines 341
- 345, The blur handler currently calls dispatch(updateVariableInScope(varName,
newValue, scopeInfo, collection.uid)) without handling the returned Promise;
modify the editorEl 'blur' listener so that the Promise returned by
updateVariableInScope is handled (either mark the handler async and await the
dispatch, or attach .catch) and surface failures (e.g., log via console or
process logger and optionally revert to rawValue or show UI feedback). Locate
the editorEl.addEventListener('blur', ...) block and update the call to
updateVariableInScope/updateVariableInScope(...) to handle rejection for
varName, newValue, scopeInfo, collection.uid.
Comment on lines
+430
to
+433
| const positionAndShowPopup = (popup, varName, lineNumber, startCol) => { | ||
| document.body.appendChild(popup); | ||
| activePopup = popup; | ||
| currentVarName = varName; |
Contributor
There was a problem hiding this comment.
Track the active tooltip by occurrence, not only name.
When the same variable appears multiple times, Lines 506-510 keep the old popup because currentVarName matches, even if the hover moved to a different occurrence.
Proposed fix
- let currentVarName = null;
+ let currentVarKey = null;
@@
- currentVarName = varName;
+ currentVarKey = `${lineNumber}:${startCol}:${varName}`;
@@
- currentVarName = null;
+ currentVarKey = null;
@@
// Same variable — keep current popup
- if (activePopup && currentVarName === varInfo.varName) {
+ const varKey = `${varInfo.lineNumber}:${varInfo.startCol}:${varInfo.varName}`;
+ if (activePopup && currentVarKey === varKey) {
clearTimeout(hideTimeout);
return;
}Also applies to: 506-510
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js` around lines 430
- 433, The code currently tracks the active tooltip only by variable name
(currentVarName), causing the popup to persist when hovering a different
occurrence of the same variable; update positionAndShowPopup to set a unique
occurrence identifier (e.g., currentVarOccurrence =
`${varName}@${lineNumber}:${startCol}`) and use that identifier in all checks
where currentVarName is compared (replace uses of currentVarName in the
hover/leave logic with currentVarOccurrence), and ensure the old popup is
removed or replaced when the new occurrence id differs so each occurrence is
tracked independently.
- Changed variable handling to use let for rawValue, interpolatedValue, and displayValue for better clarity and mutability. - Updated tooltip display logic to ensure values are correctly updated when changes occur in the editor.
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (4)
packages/bruno-app/src/components/MonacoEditor/index.js (1)
112-119:⚠️ Potential issue | 🟠 MajorSuppress
onEditduring external value sync.
model.pushEditOperations(...)still emits a model-content change, so prop-driven reloads can callonEditand mark the request dirty as if the user typed.Proposed fix
const variableCleanupRef = useRef(null); const tooltipCleanupRef = useRef(null); const autocompleteCleanupRef = useRef(null); + const isSyncingExternalValueRef = useRef(false); ... const contentDisposable = editor.onDidChangeModelContent(() => { const newValue = editor.getValue(); cachedValueRef.current = newValue; + if (isSyncingExternalValueRef.current) return; if (onEditRef.current) { onEditRef.current(newValue); } }); ... - model.pushEditOperations( - selections, - [{ range: fullRange, text: value || '' }], - () => selections - ); + isSyncingExternalValueRef.current = true; + try { + model.pushEditOperations( + selections, + [{ range: fullRange, text: value || '' }], + () => selections + ); + } finally { + isSyncingExternalValueRef.current = false; + }#!/bin/bash # Verify the MonacoEditor value-sync path has an explicit guard that prevents # programmatic model edits from invoking onEdit. sed -n '30,230p' packages/bruno-app/src/components/MonacoEditor/index.js \ | rg -n -C3 'isSyncingExternalValueRef|onDidChangeModelContent|pushEditOperations|onEditRef'Also applies to: 209-229
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/components/MonacoEditor/index.js` around lines 112 - 119, The model change handler currently calls onEditRef whenever editor.onDidChangeModelContent fires, which includes programmatic edits from model.pushEditOperations; add and use a boolean guard (e.g., isSyncingExternalValueRef) around any programmatic updates so that before performing pushEditOperations you set isSyncingExternalValueRef = true and clear it immediately after, and update the onDidChangeModelContent callback to skip invoking onEditRef (and avoid updating cachedValueRef or marking dirty) when isSyncingExternalValueRef is true; reference the existing symbols editor.onDidChangeModelContent, model.pushEditOperations, onEditRef, cachedValueRef and the new isSyncingExternalValueRef flag to implement this guard.packages/bruno-app/src/utils/monaco/variableHighlighting.js (3)
436-439:⚠️ Potential issue | 🟡 MinorTrack tooltip identity by occurrence, not only variable name.
Moving between two occurrences of the same variable keeps the old popup because
currentVarNamematches. Include the line/column in the active key.Proposed fix
- let currentVarName = null; + let currentVarKey = null; ... - currentVarName = varName; + currentVarKey = `${lineNumber}:${startCol}:${varName}`; ... - currentVarName = null; + currentVarKey = null; ... - if (activePopup && currentVarName === varInfo.varName) { + const varKey = `${varInfo.lineNumber}:${varInfo.startCol}:${varInfo.varName}`; + if (activePopup && currentVarKey === varKey) { clearTimeout(hideTimeout); return; }Also applies to: 512-517
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js` around lines 436 - 439, The popup identity is currently tracked only by currentVarName so moving between occurrences of the same variable keeps the old popup; update the tracking to include occurrence position (line and column) — in positionAndShowPopup build a unique key (e.g., `${varName}:${lineNumber}:${startCol}`) and set that to a new tracker (replace currentVarName with currentVarKey or add currentPopupKey), use that key wherever you compare/clear popups (including the logic referenced around lines 512-517) so popups are considered different for different occurrences even if the variable name is the same.
343-350:⚠️ Potential issue | 🟡 MinorHandle failed inline variable saves.
dispatch(updateVariableInScope(...))can reject, but the tooltip immediately mutatesrawValue/displayValue, leaving stale UI and an unhandled rejection on persistence failure.Proposed fix
- editorEl.addEventListener('blur', () => { + editorEl.addEventListener('blur', async () => { const newValue = editorEl.value; if (newValue !== rawValue && dispatch) { - dispatch(updateVariableInScope(varName, newValue, scopeInfo, collection.uid)); - rawValue = newValue; - interpolatedValue = getInterpolatedValue(rawValue); - displayValue = interpolatedValue; - updateValueDisplay(); + try { + await dispatch(updateVariableInScope(varName, newValue, scopeInfo, collection.uid)); + rawValue = newValue; + interpolatedValue = getInterpolatedValue(rawValue); + displayValue = interpolatedValue; + updateValueDisplay(); + } catch { + editorEl.value = rawValue; + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js` around lines 343 - 350, The blur handler currently calls dispatch(updateVariableInScope(...)) and immediately mutates rawValue/interpolatedValue/displayValue and calls updateValueDisplay(), causing stale UI and unhandled rejections if persistence fails; change the blur handler (editorEl.addEventListener('blur')) to treat dispatch(updateVariableInScope(varName, newValue, scopeInfo, collection.uid)) as an async operation: wait for the promise (via await or .then/.catch), only update rawValue, interpolatedValue (via getInterpolatedValue) and call updateValueDisplay() on success, and on failure revert the input/display to the previous rawValue (and call updateValueDisplay()) and handle/log the error to avoid unhandled rejection.
52-73:⚠️ Potential issue | 🟠 MajorNormalize names and include path params in validation.
{{ foo }}still resolves as" foo ", and validpathParamsare still excluded fromisValid, so both can be highlighted as invalid incorrectly.Proposed fix
- const varName = match[1]; + const varName = match[1].trim(); ... - const isValid = isMockVariable || pathFoundInVariables(varName, varLookup); + const isValid = isMockVariable + || pathFoundInVariables(varName, varLookup) + || pathFoundInVariables(varName, pathParams); ... - return { varName: match[1], startCol, endCol, lineNumber: position.lineNumber }; + return { varName: match[1].trim(), startCol, endCol, lineNumber: position.lineNumber };Also applies to: 125-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js` around lines 52 - 73, The VARIABLE_REGEX match yields varName with possible surrounding whitespace and pathParams were left out of the validation lookup, causing valid variables like "{{ foo }}" and pathParams to be marked invalid; to fix, trim the matched varName before tests (use a trimmedVarName variable) and use that trimmed name for prompt ('?') and mock ('$') checks and when calling pathFoundInVariables, and include pathParams in the validation lookup (or pass an object that merges pathParams into varLookup) so path parameters are considered valid; update references around VARIABLE_REGEX.exec loop, varName usage, isMockVariable (mockDataFunctions), and the pathFoundInVariables call accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@packages/bruno-app/src/components/MonacoEditor/index.js`:
- Around line 112-119: The model change handler currently calls onEditRef
whenever editor.onDidChangeModelContent fires, which includes programmatic edits
from model.pushEditOperations; add and use a boolean guard (e.g.,
isSyncingExternalValueRef) around any programmatic updates so that before
performing pushEditOperations you set isSyncingExternalValueRef = true and clear
it immediately after, and update the onDidChangeModelContent callback to skip
invoking onEditRef (and avoid updating cachedValueRef or marking dirty) when
isSyncingExternalValueRef is true; reference the existing symbols
editor.onDidChangeModelContent, model.pushEditOperations, onEditRef,
cachedValueRef and the new isSyncingExternalValueRef flag to implement this
guard.
In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js`:
- Around line 436-439: The popup identity is currently tracked only by
currentVarName so moving between occurrences of the same variable keeps the old
popup; update the tracking to include occurrence position (line and column) — in
positionAndShowPopup build a unique key (e.g.,
`${varName}:${lineNumber}:${startCol}`) and set that to a new tracker (replace
currentVarName with currentVarKey or add currentPopupKey), use that key wherever
you compare/clear popups (including the logic referenced around lines 512-517)
so popups are considered different for different occurrences even if the
variable name is the same.
- Around line 343-350: The blur handler currently calls
dispatch(updateVariableInScope(...)) and immediately mutates
rawValue/interpolatedValue/displayValue and calls updateValueDisplay(), causing
stale UI and unhandled rejections if persistence fails; change the blur handler
(editorEl.addEventListener('blur')) to treat
dispatch(updateVariableInScope(varName, newValue, scopeInfo, collection.uid)) as
an async operation: wait for the promise (via await or .then/.catch), only
update rawValue, interpolatedValue (via getInterpolatedValue) and call
updateValueDisplay() on success, and on failure revert the input/display to the
previous rawValue (and call updateValueDisplay()) and handle/log the error to
avoid unhandled rejection.
- Around line 52-73: The VARIABLE_REGEX match yields varName with possible
surrounding whitespace and pathParams were left out of the validation lookup,
causing valid variables like "{{ foo }}" and pathParams to be marked invalid; to
fix, trim the matched varName before tests (use a trimmedVarName variable) and
use that trimmed name for prompt ('?') and mock ('$') checks and when calling
pathFoundInVariables, and include pathParams in the validation lookup (or pass
an object that merges pathParams into varLookup) so path parameters are
considered valid; update references around VARIABLE_REGEX.exec loop, varName
usage, isMockVariable (mockDataFunctions), and the pathFoundInVariables call
accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: ee61ea2e-0e96-47af-b971-00580acac639
📒 Files selected for processing (8)
packages/bruno-app/src/components/MonacoEditor/index.jspackages/bruno-app/src/components/MonacoEditor/index.spec.jspackages/bruno-app/src/components/RequestPane/RequestBody/index.jspackages/bruno-app/src/components/ScriptEditor/index.jspackages/bruno-app/src/components/ScriptEditor/index.spec.jspackages/bruno-app/src/utils/monaco/autocomplete.jspackages/bruno-app/src/utils/monaco/brunoTheme.jspackages/bruno-app/src/utils/monaco/variableHighlighting.js
🚧 Files skipped from review as they are similar to previous changes (4)
- packages/bruno-app/src/components/ScriptEditor/index.js
- packages/bruno-app/src/utils/monaco/autocomplete.js
- packages/bruno-app/src/components/MonacoEditor/index.spec.js
- packages/bruno-app/src/utils/monaco/brunoTheme.js
…bal styles - Introduced a new ScriptEditor component to replace CodeEditor for improved scripting capabilities. - Updated global styles to include specific styles for Monaco tooltip textarea editor. - Refactored variable tooltip logic to utilize shared styles, enhancing consistency across the application. - Improved autocomplete functionality by ensuring proper context resolution for variable hints in the Monaco Editor.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
packages/bruno-app/src/utils/monaco/variableHighlighting.js (3)
26-26:⚠️ Potential issue | 🟠 MajorResolve path params and normalize captured variable names.
pathParamsare extracted but not used, whiledetectScope()never returns the existingpathParamscope. This can mark valid path params as invalid and show the tooltip as undefined/editable. Also trimmatch[1]before lookup so{{ foo }}resolves like{{foo}}.Proposed fix
-const READ_ONLY_SCOPES = new Set(['process.env', 'runtime', 'dynamic', 'oauth2', 'undefined']); +const READ_ONLY_SCOPES = new Set(['process.env', 'runtime', 'dynamic', 'oauth2', 'undefined', 'pathParam']); @@ - const varName = match[1]; + const varName = match[1].trim(); @@ - const isValid = isMockVariable || pathFoundInVariables(varName, varLookup); + const isValid = isMockVariable + || pathFoundInVariables(varName, varLookup) + || pathFoundInVariables(varName, pathParams); @@ - return { varName: match[1], startCol, endCol, lineNumber: position.lineNumber }; + return { varName: match[1].trim(), startCol, endCol, lineNumber: position.lineNumber }; @@ if (varName.startsWith('process.env.')) { const envKey = varName.replace('process.env.', ''); const variables = getAllVariables(collection, item); const value = get(variables, varName); return { type: 'process.env', value, data: { envKey } }; } + + const variables = getAllVariables(collection, item); + const pathParamValue = get(variables.pathParams, varName); + if (pathParamValue !== undefined) { + return { type: 'pathParam', value: pathParamValue, data: {} }; + } const scopeInfo = getVariableScope(varName, collection, item);Also applies to: 52-73, 121-166
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js` at line 26, The code currently ignores extracted pathParams and doesn't normalize captured variable names, so update the logic to trim whitespace from the captured name (e.g., use match[1].trim()) before any lookup and use the extracted pathParams map to resolve scope: modify detectScope() to check the trimmed key against pathParams and return the appropriate 'pathParam' scope when present, and ensure all places that call detectScope()/lookup (the blocks around READ_ONLY_SCOPES and the sections referenced in the comment) use the trimmed name and pathParams resolution so valid path params are treated as read-only/non-editable and tooltips show the correct value.
330-338:⚠️ Potential issue | 🟡 MinorHandle failed inline variable updates.
The tooltip updates local UI immediately, but the dispatched async update can reject. Catch failures so the tooltip does not silently show a value that failed to persist.
Proposed fix
- dispatch(updateVariableInScope(varName, newValue, scopeInfo, collection.uid)); - rawValue = newValue; - interpolatedValue = getInterpolatedValue(rawValue); - displayValue = interpolatedValue; - updateValueDisplay(); + dispatch(updateVariableInScope(varName, newValue, scopeInfo, collection.uid)) + .then(() => { + rawValue = newValue; + interpolatedValue = getInterpolatedValue(rawValue); + displayValue = interpolatedValue; + updateValueDisplay(); + }) + .catch(() => { + editorEl.value = rawValue; + updateValueDisplay(); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js` around lines 330 - 338, The inline update currently updates the tooltip UI immediately inside the editorEl.blur handler but doesn't handle a rejected async update; capture the previous value (e.g., prevValue = rawValue) before calling dispatch(updateVariableInScope(...)), call Promise.resolve(dispatch(...)) and on success keep the new state, but on failure revert rawValue/interpolatedValue/displayValue back to prevValue (recompute interpolatedValue via getInterpolatedValue(prevValue)) and call updateValueDisplay(), and optionally surface an error (e.g., via a passed error dispatcher) so the tooltip doesn't silently show a value that failed to persist; apply this change in the editorEl.addEventListener('blur', ...) block that invokes updateVariableInScope and updateValueDisplay.
423-426:⚠️ Potential issue | 🟡 MinorTrack tooltip identity by occurrence, not only variable name.
Hovering another
{{var}}occurrence with the same name keeps the old popup becausecurrentVarNamestill matches. Include line/column in the active key.Proposed fix
- let currentVarName = null; + let currentVarKey = null; @@ - currentVarName = varName; + currentVarKey = `${lineNumber}:${startCol}:${varName}`; @@ - currentVarName = null; + currentVarKey = null; @@ - if (activePopup && currentVarName === varInfo.varName) { + const varKey = `${varInfo.lineNumber}:${varInfo.startCol}:${varInfo.varName}`; + if (activePopup && currentVarKey === varKey) { clearTimeout(hideTimeout); return; }Also applies to: 499-504
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js` around lines 423 - 426, The popup identity is only tracked by currentVarName so popups for the same variable name at different locations are reused; update the tracking to include occurrence (line and column): in positionAndShowPopup create and set an active key like activeVarKey = `${varName}:${lineNumber}:${startCol}` (or similar) instead of only currentVarName, use that key wherever currentVarName is currently checked (e.g., the code paths that decide to reuse/hide popups), and ensure hide/compare logic (the other popup-handling functions referenced in the diff) also uses activeVarKey so each {{var}} occurrence is treated uniquely.
🤖 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/utils/monaco/variableHighlighting.js`:
- Around line 376-389: The click handler on copyBtn currently calls
navigator.clipboard.writeText(...) without handling rejection; update the
handler in the copyBtn.addEventListener callback to append a .catch(...) to
navigator.clipboard.writeText(textToCopy) to handle failures (use copyBtn,
copyTimeout, scopeInfo, interpolatedValue, CHECK_ICON, COPY_ICON,
COPY_SUCCESS_TIMEOUT to locate the code) — in the .catch block revert any UI
changes (ensure copyBtn.innerHTML and class state are consistent), optionally
show an error state/message or console.error, and clear/reset copyTimeout so a
subsequent click can retry.
---
Duplicate comments:
In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js`:
- Line 26: The code currently ignores extracted pathParams and doesn't normalize
captured variable names, so update the logic to trim whitespace from the
captured name (e.g., use match[1].trim()) before any lookup and use the
extracted pathParams map to resolve scope: modify detectScope() to check the
trimmed key against pathParams and return the appropriate 'pathParam' scope when
present, and ensure all places that call detectScope()/lookup (the blocks around
READ_ONLY_SCOPES and the sections referenced in the comment) use the trimmed
name and pathParams resolution so valid path params are treated as
read-only/non-editable and tooltips show the correct value.
- Around line 330-338: The inline update currently updates the tooltip UI
immediately inside the editorEl.blur handler but doesn't handle a rejected async
update; capture the previous value (e.g., prevValue = rawValue) before calling
dispatch(updateVariableInScope(...)), call Promise.resolve(dispatch(...)) and on
success keep the new state, but on failure revert
rawValue/interpolatedValue/displayValue back to prevValue (recompute
interpolatedValue via getInterpolatedValue(prevValue)) and call
updateValueDisplay(), and optionally surface an error (e.g., via a passed error
dispatcher) so the tooltip doesn't silently show a value that failed to persist;
apply this change in the editorEl.addEventListener('blur', ...) block that
invokes updateVariableInScope and updateValueDisplay.
- Around line 423-426: The popup identity is only tracked by currentVarName so
popups for the same variable name at different locations are reused; update the
tracking to include occurrence (line and column): in positionAndShowPopup create
and set an active key like activeVarKey = `${varName}:${lineNumber}:${startCol}`
(or similar) instead of only currentVarName, use that key wherever
currentVarName is currently checked (e.g., the code paths that decide to
reuse/hide popups), and ensure hide/compare logic (the other popup-handling
functions referenced in the diff) also uses activeVarKey so each {{var}}
occurrence is treated uniquely.
🪄 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: 8d69cafa-b63a-43cc-b3f6-9e2d2f7eac77
📒 Files selected for processing (7)
packages/bruno-app/src/components/RequestPane/RequestBody/index.jspackages/bruno-app/src/components/ScriptEditor/index.jspackages/bruno-app/src/globalStyles.jspackages/bruno-app/src/utils/monaco/autocomplete.jspackages/bruno-app/src/utils/monaco/brunoApiTypes.jspackages/bruno-app/src/utils/monaco/brunoTheme.jspackages/bruno-app/src/utils/monaco/variableHighlighting.js
✅ Files skipped from review due to trivial changes (2)
- packages/bruno-app/src/globalStyles.js
- packages/bruno-app/src/components/RequestPane/RequestBody/index.js
🚧 Files skipped from review as they are similar to previous changes (2)
- packages/bruno-app/src/utils/monaco/brunoApiTypes.js
- packages/bruno-app/src/utils/monaco/brunoTheme.js
Comment on lines
+376
to
+389
| copyBtn.addEventListener('click', (e) => { | ||
| e.stopPropagation(); | ||
| if (copyTimeout) return; | ||
| const textToCopy = typeof scopeInfo.value === 'object' | ||
| ? JSON.stringify(scopeInfo.value) : interpolatedValue; | ||
| navigator.clipboard.writeText(textToCopy).then(() => { | ||
| copyBtn.innerHTML = CHECK_ICON; | ||
| copyBtn.classList.add('copy-success'); | ||
| copyTimeout = setTimeout(() => { | ||
| copyBtn.innerHTML = COPY_ICON; | ||
| copyBtn.classList.remove('copy-success'); | ||
| copyTimeout = null; | ||
| }, COPY_SUCCESS_TIMEOUT); | ||
| }); |
Contributor
There was a problem hiding this comment.
Handle clipboard write failures.
navigator.clipboard.writeText() can reject; add a .catch() path so copy failures do not surface as unhandled promise rejections.
Proposed fix
- navigator.clipboard.writeText(textToCopy).then(() => {
- copyBtn.innerHTML = CHECK_ICON;
- copyBtn.classList.add('copy-success');
- copyTimeout = setTimeout(() => {
- copyBtn.innerHTML = COPY_ICON;
- copyBtn.classList.remove('copy-success');
- copyTimeout = null;
- }, COPY_SUCCESS_TIMEOUT);
- });
+ navigator.clipboard.writeText(textToCopy)
+ .then(() => {
+ copyBtn.innerHTML = CHECK_ICON;
+ copyBtn.classList.add('copy-success');
+ copyTimeout = setTimeout(() => {
+ copyBtn.innerHTML = COPY_ICON;
+ copyBtn.classList.remove('copy-success');
+ copyTimeout = null;
+ }, COPY_SUCCESS_TIMEOUT);
+ })
+ .catch(() => {
+ copyTimeout = null;
+ });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@packages/bruno-app/src/utils/monaco/variableHighlighting.js` around lines 376
- 389, The click handler on copyBtn currently calls
navigator.clipboard.writeText(...) without handling rejection; update the
handler in the copyBtn.addEventListener callback to append a .catch(...) to
navigator.clipboard.writeText(textToCopy) to handle failures (use copyBtn,
copyTimeout, scopeInfo, interpolatedValue, CHECK_ICON, COPY_ICON,
COPY_SUCCESS_TIMEOUT to locate the code) — in the .catch block revert any UI
changes (ensure copyBtn.innerHTML and class state are consistent), optionally
show an error state/message or console.error, and clear/reset copyTimeout so a
subsequent click can retry.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Jira
Summary
What's included
bru,req,resobjects with JSDoc, overloads, and@exampletags{{var}}patterns highlighted green (valid), red (invalid), blue (prompt) with hover tooltips showing variable value, scope, and inline editing (gated byenableVariableHighlightingprop — enabled for body editors, disabled for scripts){{triggers variable suggestions across all editor languages (JSON, XML, plaintext, JavaScript, etc.), including environment/collection/runtime variables,$mockFunctionnames, andprocess.env.*keys{{$randomInt}}etc. show "Generates random value on each request" (or "Generates current timestamp on each request" for time-based vars) matching CodeMirror behavior — no value input or copy buttonbrunoVarInfobehaviorArchitecture
Feature parity with CodeMirror
{{var}})enableVariableHighlighting)enableVariableHighlighting)brunoVarInfooptionsetupVariableTooltip{{}}setupAutoComplete(keyup-based)registerCompletionItemProvider+ content change triggerbru.,req.,res.)brunoApiTypes.js)Files changed
item/folderfor correct scope detection:RequestPane/Script,RequestPane/Tests,FolderSettings/Script,FolderSettings/Tests,CollectionSettings/Script,CollectionSettings/TestsRequestPane/RequestBodyswitches between CodeEditor and MonacoEditor based on beta flagDeferred (not in this PR)
{{var}}syntax integrationBefore (CodeMirror)
Screen.Recording.2026-04-17.at.5.44.46.PM.mov
After (Monaco)
Screen.Recording.2026-04-17.at.5.45.15.PM.mov
Variable highlighting
Screen.Recording.2026-04-18.at.1.07.23.AM.mov
Test plan
bru.— autocomplete shows all Bruno API methods with JSDoc{{varName}}in script — no text coloring, but autocomplete suggests variables{{in body — variable autocomplete suggests env/collection/runtime variables{{ho— suggestions filter to matching variables (e.g.,host){{var}}in body — tooltip shows variable value, scope, and allows editing{{$randomInt}}— tooltip shows "Generates random value on each request" with no value input{{newVar}}— tooltip shows "Request" scope, allows adding a valueContribution Checklist:
Summary by CodeRabbit
New Features
Chores
Tests