fix: var into tooltip for faker vars #6312
Conversation
pooja-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno and
naman-bruno
as code owners
WalkthroughAdds handling for dynamic (faker) variables prefixed with Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/bruno-app/src/utils/codemirror/brunoVarInfo.js (1)
9-9: Dynamic/faker variable handling is cohesive; minor optimization possibleThe integration of
mockDataFunctions, the newdynamicscope label, read‑only behavior, and name/validity checks for$‑prefixed variables hangs together well and matches the intended UX (badge + warning for unknown, read‑only note for valid dynamics). One small optimization:scopeInfo.valuefor dynamic variables is set by callingfakerFunction(), but that value is not used anywhere on the dynamic paths (you return before any value/clipboard/editor logic), so you could avoid invoking the faker function here to save unnecessary work and side effects:- scopeInfo = { - type: 'dynamic', - value: fakerFunction ? fakerFunction() : '', - data: null, - isValidFakerVariable: !!fakerFunction - }; + scopeInfo = { + type: 'dynamic', + value: '', + data: null, + isValidFakerVariable: !!fakerFunction + };Everything else around validation and early returns for dynamic variables looks solid.
Also applies to: 67-80, 182-193, 243-245, 279-311
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/bruno-app/src/utils/codemirror/brunoVarInfo.js(6 hunks)packages/bruno-app/src/utils/codemirror/brunoVarInfo.spec.js(2 hunks)
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation, never tabs
Use single quotes for strings instead of double quotes
Always add semicolons at the end of statements
No trailing commas in code
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line with a minimum of 2 elements for multiline formatting
No newlines inside function parentheses
Space before and after the arrow in arrow functions:() => {}
No space between function name and parentheses:func()notfunc ()
Semicolons should go at the end of the line, not on a new line
Function names should be concise and descriptive
Add JSDoc comments to provide details to abstractions
Avoid single-line abstractions where all that is being done is increasing the call stack with one additional function
Add meaningful comments to explain complex code flow instead of obvious comments
Files:
packages/bruno-app/src/utils/codemirror/brunoVarInfo.jspackages/bruno-app/src/utils/codemirror/brunoVarInfo.spec.js
**/*.{test,spec}.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
Add tests for any new functionality or meaningful changes; update corresponding tests when code is added, removed, or significantly modified
Files:
packages/bruno-app/src/utils/codemirror/brunoVarInfo.spec.js
🧠 Learnings (2)
📚 Learning: 2025-12-03T08:09:57.124Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-03T08:09:57.124Z
Learning: Applies to **/*.{test,spec}.{js,jsx,ts,tsx} : Add tests for any new functionality or meaningful changes; update corresponding tests when code is added, removed, or significantly modified
Applied to files:
packages/bruno-app/src/utils/codemirror/brunoVarInfo.spec.js
📚 Learning: 2025-12-03T08:09:57.124Z
Learnt from: CR
Repo: usebruno/bruno PR: 0
File: CODING_STANDARDS.md:0-0
Timestamp: 2025-12-03T08:09:57.124Z
Learning: Use consistent patterns and helper utilities where they improve clarity; prefer shared test utilities over copy-pasted setup code only when it reduces complexity
Applied to files:
packages/bruno-app/src/utils/codemirror/brunoVarInfo.spec.js
🧬 Code graph analysis (1)
packages/bruno-app/src/utils/codemirror/brunoVarInfo.js (1)
packages/bruno-common/src/index.ts (1)
mockDataFunctions(1-1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: SSL Tests - Windows
- GitHub Check: Playwright E2E Tests
- GitHub Check: SSL Tests - Linux
- GitHub Check: Unit Tests
- GitHub Check: SSL Tests - macOS
- GitHub Check: CLI Tests
🔇 Additional comments (1)
packages/bruno-app/src/utils/codemirror/brunoVarInfo.spec.js (1)
5-13: Dynamic/faker tests and mocks nicely cover the new behaviorThe extended
@usebruno/commonmock (addingmockDataFunctionsstubs) and thedynamic/faker variable renderingtests are well aligned with the implementation: you assert the read‑only note for valid dynamics, absence of a value container, and the warning +Dynamicscope badge for unknown dynamics. This gives good confidence in the new$‑prefixed variable path without overcomplicating the test setup.Also applies to: 434-470
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/bruno-app/src/utils/codemirror/brunoVarInfo.js (2)
253-254: Consider extracting read-only types to a constant.The boolean chain is growing. For maintainability:
+const READ_ONLY_SCOPE_TYPES = new Set(['process.env', 'runtime', 'dynamic', 'oauth2', 'undefined']); + -const isReadOnly = scopeInfo.type === 'process.env' || scopeInfo.type === 'runtime' || scopeInfo.type === 'dynamic' || scopeInfo.type === 'oauth2' || scopeInfo.type === 'undefined'; +const isReadOnly = READ_ONLY_SCOPE_TYPES.has(scopeInfo.type);
321-328: Consider including the token ID in the warning message.The current message is generic. Including the token ID would help users debug more quickly:
- warningNote.textContent = `OAuth2 token not found. Make sure you have fetched the token with the correct Token ID.`; + const tokenId = variableName.substring('$oauth2.'.length); + warningNote.textContent = `OAuth2 token "${tokenId}" not found. Make sure you have fetched the token.`;
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/bruno-app/src/utils/codemirror/brunoVarInfo.js(7 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (CODING_STANDARDS.md)
**/*.{js,jsx,ts,tsx}: Use 2 spaces for indentation, never tabs
Use single quotes for strings instead of double quotes
Always add semicolons at the end of statements
No trailing commas in code
Always use parentheses around parameters in arrow functions, even for single parameters
For multiline constructs, put opening braces on the same line with a minimum of 2 elements for multiline formatting
No newlines inside function parentheses
Space before and after the arrow in arrow functions:() => {}
No space between function name and parentheses:func()notfunc ()
Semicolons should go at the end of the line, not on a new line
Function names should be concise and descriptive
Add JSDoc comments to provide details to abstractions
Avoid single-line abstractions where all that is being done is increasing the call stack with one additional function
Add meaningful comments to explain complex code flow instead of obvious comments
Files:
packages/bruno-app/src/utils/codemirror/brunoVarInfo.js
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (6)
- GitHub Check: CLI Tests
- GitHub Check: Playwright E2E Tests
- GitHub Check: Unit Tests
- GitHub Check: SSL Tests - Windows
- GitHub Check: SSL Tests - macOS
- GitHub Check: SSL Tests - Linux
🔇 Additional comments (5)
packages/bruno-app/src/utils/codemirror/brunoVarInfo.js (5)
9-9: LGTM!Clean addition of
mockDataFunctionsto the existing import.
76-77: LGTM!Scope labels for new variable types are properly defined.
183-210: Logic looks correct; verify OAuth2 variable path structure.The check ordering is correct (OAuth2 before general
$prefix). Note that lodashgetwill parse$oauth2.tokenIdasvariables['$oauth2']['tokenId']due to dot notation. Ensure thevariablesobject structure matches this expectation.Edge case:
$oauth2without a trailing dot falls through to the dynamic handler and will show "Unknown dynamic variable" - this seems acceptable.
303-319: LGTM!Clear user messaging for both valid and invalid dynamic variables. Early returns prevent unnecessary DOM construction.
582-586: LGTM!OAuth2 read-only note is consistent with the existing
process.envhandling.
1 participant
Description
Contribution Checklist:
Summary by CodeRabbit
New Features
UI
Tests
✏️ Tip: You can customize this high-level summary in your review settings.