fix: variable-tooltip hover issues#7907
Open
shubh-bruno wants to merge 7 commits intousebruno:feat/7458-improve-variable-tooltip-internalfrom
Open
fix: variable-tooltip hover issues#7907shubh-bruno wants to merge 7 commits intousebruno:feat/7458-improve-variable-tooltip-internalfrom
shubh-bruno wants to merge 7 commits intousebruno:feat/7458-improve-variable-tooltip-internalfrom
Conversation
shubh-bruno
requested review from
bijin-bruno,
helloanoop,
lohit-bruno,
naman-bruno and
sid-bruno
as code owners
Contributor
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (2)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThe PR changes the variable tooltip popup: the copy button resolves the value at click time via a getter and the tooltip tracks a live ChangesVariable Tooltip Lifecycle & Copy Behavior
Sequence DiagramsequenceDiagram
actor User
participant Popup as Variable Tooltip
participant Editor as CodeMirror Editor
participant Var as Variable Resolver
participant Clipboard as Clipboard
User->>Popup: Hover variable
Popup->>Popup: showPopup() (cleans prior popup)
Popup-->>User: Render tooltip with value
User->>Popup: Click/pin or click value
Popup->>Popup: isPinned = true / open editor
Editor->>Editor: visibility staging (hide → raf → refresh → reveal)
Popup-->>User: Editor visible
User->>Editor: Type + Enter
Editor->>Var: Save updated value
Var-->>Editor: Resolved interpolated result
Editor->>Popup: currentInterpolatedValue = resolved
User->>Popup: Click copy
Popup->>Clipboard: Copy currentInterpolatedValue
Clipboard-->>User: Clipboard updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
packages/bruno-app/src/utils/codemirror/brunoVarInfo.js (1)
135-140: ⚡ Quick winExpose a stable test id on the copy button.
This PR adds more Playwright coverage around the tooltip copy flow, but the button is still only addressable through a styling class. Adding a
data-testidhere will make those tests less brittle when tooltip/CodeMirror markup shifts.As per coding guidelines, "Add
data-testidto testable elements for Playwright."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/codemirror/brunoVarInfo.js` around lines 135 - 140, In getCopyButton (the factory that creates the copy button), add a stable data-testid attribute—e.g., copyButton.setAttribute('data-testid', 'bruno-copy-button')—so tests can target it reliably; update the element created in getCopyButton (used with getVariableValue and onCopyCallback) to include this data-testid alongside the existing className and type attributes.
🤖 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/codemirror/brunoVarInfo.js`:
- Around line 421-423: The live tooltip's cached copy value
currentInterpolatedValue is initialized from variableValue which drops valid
falsy values and never advances after saves; update the code so that after every
successful save path you assign the resolved/saved value (not just
variableValue) into currentInterpolatedValue and also update any mask/reveal
state used by the tooltip; specifically, in the save-success handler(s) (the
code paths referenced around currentInterpolatedValue initialization and the
other block at lines 566-576) set currentInterpolatedValue to the newly saved
value using an explicit check for undefined/null (instead of truthiness) so
0/false are preserved, and ensure any reveal/mask flags that affect redraw are
updated from the same saved-state so future redraws use the new value.
In `@tests/variable-tooltip/variable-tooltip.spec.ts`:
- Around line 435-640: Three tests are accidentally committed with test.only
which will focus and skip the rest of the suite; remove the `.only` from the
three focused test invocations — the tests whose titles begin "should keep
tooltip open while editing when mouse leaves popup area", "should persist
subsequent edits while popup stays open", and "should copy latest value after
editing within the same tooltip" — so they use plain test(...) instead of
test.only(...); ensure no other test.only remains in this file.
---
Nitpick comments:
In `@packages/bruno-app/src/utils/codemirror/brunoVarInfo.js`:
- Around line 135-140: In getCopyButton (the factory that creates the copy
button), add a stable data-testid attribute—e.g.,
copyButton.setAttribute('data-testid', 'bruno-copy-button')—so tests can target
it reliably; update the element created in getCopyButton (used with
getVariableValue and onCopyCallback) to include this data-testid alongside the
existing className and type attributes.
🪄 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: 259d6c5f-61b4-4222-901c-d337fe5b4650
📒 Files selected for processing (2)
packages/bruno-app/src/utils/codemirror/brunoVarInfo.jstests/variable-tooltip/variable-tooltip.spec.ts
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/bruno-app/src/utils/codemirror/brunoVarInfo.js (1)
624-624:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
??for consistency with editable path.The read-only copy button uses
||which coerces valid falsy values (0,false) to empty string. For consistency with the editable path (line 157), use nullish coalescing.Suggested fix
- const copyButton = getCopyButton(variableValue || ''); + const copyButton = getCopyButton(variableValue ?? '');🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@packages/bruno-app/src/utils/codemirror/brunoVarInfo.js` at line 624, Replace the null-coercing fallback in the read-only copy button creation so valid falsy values like 0 or false are preserved: in the expression that constructs copyButton (getCopyButton(variableValue || '')), swap the logical OR for nullish coalescing (use ??) to match the editable path behavior used elsewhere (see the editable path at line with ??), i.e., call getCopyButton with variableValue ?? '' so only null/undefined fall back to empty string.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@packages/bruno-app/src/utils/codemirror/brunoVarInfo.js`:
- Line 624: Replace the null-coercing fallback in the read-only copy button
creation so valid falsy values like 0 or false are preserved: in the expression
that constructs copyButton (getCopyButton(variableValue || '')), swap the
logical OR for nullish coalescing (use ??) to match the editable path behavior
used elsewhere (see the editable path at line with ??), i.e., call getCopyButton
with variableValue ?? '' so only null/undefined fall back to empty string.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 31c8ed0b-2496-47aa-8425-0085cd385ff5
📒 Files selected for processing (1)
packages/bruno-app/src/utils/codemirror/brunoVarInfo.js
* Enhance JSON schema validation by integrating ajv-formats for additional format support in tests and runtime assertions. * fix: update pre-request script to stop execution instead of running it * fix: ensure newline at end of file in pre-request script for ping.bru * refactor: update JSON schema validation tests to assert rejection of invalid formats * refactor: streamline JSON schema validation by using a default AJV instance and enhance tests for various ajvOptions scenarios * refactor: update JSON schema tests to use more descriptive property names and improve error handling for invalid formats * feat: add support for Draft-07 JSON Schema validation and improve error handling for unsupported schema versions * fix: improve error message for unsupported JSON Schema versions in runtime assertions and tests
sid-bruno
changed the base branch from
main
to
feat/7458-improve-variable-tooltip-internal
…le-tooltip-internal
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
3 participants
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.
Description
JIRA
Contribution Checklist:
Note: Keeping the PR small and focused helps make it easier to review and merge. If you have multiple changes you want to make, please consider submitting them as separate pull requests.
Publishing to New Package Managers
Please see here for more information.
Summary by CodeRabbit
Bug Fixes
New Features
Tests