-
Notifications
You must be signed in to change notification settings - Fork 27
fix(variables-dropdown): fixes/improvements search magic wand for variables #2202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
|
Qovery can create a Preview Environment for this PR.
This comment has been generated from Qovery AI 🤖.
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## staging #2202 +/- ##
===========================================
- Coverage 49.33% 49.25% -0.09%
===========================================
Files 1182 708 -474
Lines 20179 15309 -4870
Branches 5982 4555 -1427
===========================================
- Hits 9955 7540 -2415
+ Misses 8279 6353 -1926
+ Partials 1945 1416 -529
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
libs/domains/variables/feature/src/lib/dropdown-variable/dropdown-variable.tsx
Outdated
Show resolved
Hide resolved
| <Popover.Content className="flex max-h-60 w-[248px] min-w-[248px] flex-col p-2"> | ||
| {/* | ||
| <Popover.Content | ||
| container={container} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure to understand why you need to pass down the container here?
Same goes for the other props
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jul-dan Please answer to the comment in your PR 🙏
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I asked Claude for explanation:
The container prop is used when DropdownVariable is rendered inside modals (see create-update-variable-modal.tsx:403 and values-override-yaml-modal.tsx:81). Without it, the popover renders to document.body which causes z-index issues where the dropdown appears behind the modal overlay.
The other props like side, align, etc. aren't passed from parents - they're positioning configuration defined within this component.
- Increase dropdown size to 400px width and 240px max-height for better visibility - Fix search field clickability by adding modal container setup with useState/useEffect - Use capture phase event handlers to prevent DropdownMenu interference - Disable built-in variables in Helm value override with informative tooltip - Position info icon 8px from variable name, horizontally aligned - Add proper text truncation to prevent horizontal scrolling - Enable mouse wheel scrolling with onWheel handler 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove projectId/serviceId props from values-override-yaml-setting - Remove orphaned aliases/overrides handling from sort-variables - These features should be handled on the backend per PR feedback 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Remove projectId/serviceId props from values-override-yaml-setting - Remove orphaned aliases/overrides handling from sort-variables - These features should be handled on the backend per PR feedback 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
- Reduce event handlers from 10 to 3 essential ones (onKeyDown, onPointerDown, onClick) - Remove unnecessary capture phase and touch event handlers - Maintain same functionality with cleaner code 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @jul-dan !
Please take ownership of your PR, make sure to follow up if it's been waiting for too long, and reply directly on the PR so we keep a clear trace of the discussions 🙏
Here's my feedback:
- I've found a simpler way to fix the scroll bug without using
useEffectand adding too much code, available here
https://www.loom.com/share/683e0c27e7a64c6293268ff535a17583
- Missing full variable name on hover:
Your version
libs/domains/variables/feature/src/lib/dropdown-variable/dropdown-variable.tsx
Outdated
Show resolved
Hide resolved
| <Popover.Content className="flex max-h-60 w-[248px] min-w-[248px] flex-col p-2"> | ||
| {/* | ||
| <Popover.Content | ||
| container={container} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jul-dan Please answer to the comment in your PR 🙏
...ains/variables/feature/src/lib/create-update-variable-modal/create-update-variable-modal.tsx
Outdated
Show resolved
Hide resolved
- Remove unused projectId prop from DropdownVariable and CodeEditorVariable - Add tooltip to show full variable name on hover when truncated - Replace useEffect pattern with fakeModal option to prevent scroll lock issues - Remove container prop now that fakeModal handles portal positioning - Add fakeModal: true to all variable modal calls for better UX This fixes PR review feedback about AI-generated useEffect patterns and improves the dropdown behavior when used inside modals. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
- Remove unused projectId prop from DropdownVariable and CodeEditorVariable - Add tooltip to show full variable name on hover when truncated - Replace useEffect pattern with fakeModal option to prevent scroll lock issues - Remove container prop now that fakeModal handles portal positioning - Add fakeModal: true to all variable modal calls for better UX This fixes PR review feedback about AI-generated useEffect patterns and improves the dropdown behavior when used inside modals. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
|
Thanks @RemiBonnet I have reviewed all your comments :) |
| </div> | ||
| <div className="overflow-y-auto"> | ||
| <DropdownMenu.Label asChild> | ||
| <div |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think it’s necessary to change it, we can keep what we had before it already works
The issue was caused by the missing fakeModal
Keep only your logic with isBuiltIn etc not the DOM changes
| <Truncate text={variable.service_name} truncateLimit={30} /> | ||
| <div className="flex w-full min-w-0 items-center gap-2"> | ||
| <Tooltip content={variable.key} side="bottom"> | ||
| <span className="min-w-0 max-w-full overflow-hidden text-ellipsis whitespace-nowrap text-sm font-medium"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Missing the <Truncate /> composant, this is what I've say in the review before
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did not see any comment on this file 🤔 Or your mean the previous comment? Anyway I have updated it !
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added the full variable name on hover
- Revert unnecessary DOM positioning props (side, align, sideOffset, etc.) - Use Truncate component instead of CSS truncation + Tooltip for variable name - Keep simple Popover.Content structure with 400px width (user requested) - Retain isBuiltIn logic and disableBuiltInVariables functionality - Keep fakeModal option for proper modal behavior This addresses PR review comments requesting simpler DOM structure and Truncate component usage while maintaining the new isBuiltIn feature functionality and improved dropdown width. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
libs/domains/variables/feature/src/lib/dropdown-variable/dropdown-variable.tsx
Outdated
Show resolved
Hide resolved
- Revert unnecessary DOM positioning props (side, align, sideOffset, etc.) - Use Truncate component instead of CSS truncation + Tooltip for variable name - Keep simple Popover.Content structure with 400px width (user requested) - Retain isBuiltIn logic and disableBuiltInVariables functionality - Keep fakeModal option for proper modal behavior This addresses PR review comments requesting simpler DOM structure and Truncate component usage while maintaining the new isBuiltIn feature functionality and improved dropdown width. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Sonnet 4.5 <noreply@anthropic.com>
f853901 to
6642d7b
Compare
RemiBonnet
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Last one, after it should be good !
libs/domains/variables/feature/src/lib/dropdown-variable/dropdown-variable.tsx
Outdated
Show resolved
Hide resolved
…ad of custom class Remove redundant 'cursor-not-allowed opacity-50' class as it's already handled by dropdownMenuItemVariants when passing disabled: isDisabled
|
🎉 This PR is included in version 1.264.3 🎉 The release is available on GitHub release Your semantic-release bot 📦🚀 |



Summary
Issue: QOV-1240
Several fixes:
Screenshots / Recordings
Testing
yarn testoryarn test -u(if you need to regenerate snapshots)yarn formatyarn lintPR Checklist
.cursor/rules)feat(service): add new Terraform service) - required for semantic-release