fix(select): remove asChild usage in SelectPrimitive.Icon#13929
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. 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:
WalkthroughRemoves the ChangesSelect Icon Compatibility Fix
Estimated code review effort: 1 (Trivial) | ~5 minutes Estimated code review effort: 1 (Trivial) | ~5 minutes 🚥 Pre-merge checks | ✅ 8 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (8 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
github-actions
Bot
added
bug
![coderabbitai[bot]](https://cdn.statically.io/img/avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/frontend/src/components/ui/__tests__/select-custom.test.tsx (1)
12-31: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTests don't assert the icon actually renders.
These tests only check that rendering doesn't throw and that trigger text appears, but the actual change under test is icon composition (
SelectPrimitive.Icon). None of the assertions verify the icon element is present in the DOM, so this suite would not catch a regression where the icon disappears (as currently happens inselect-custom.tsx).Consider adding an assertion checking for the icon's presence (e.g., via
data-testidor role/svg query) inrenderSelect.
As per path instructions, frontend tests should "verify the tests actually cover the new or changed behavior rather than acting as placeholders."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/frontend/src/components/ui/__tests__/select-custom.test.tsx` around lines 12 - 31, The Select tests only validate that rendering succeeds and text is present, but they do not cover the icon composition introduced via SelectPrimitive.Icon. Update the Select - Radix Slot 1.3.0 compatibility suite in renderSelect to assert that the icon is actually rendered in the DOM, using a stable query such as a test id or svg/role lookup. Keep the existing checks, but add an explicit icon-presence assertion so this test fails if the icon disappears from select-custom.tsx.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/frontend/src/components/ui/select-custom.tsx`:
- Line 23: The Select icon wrapper was left empty in SelectCustom, so the
dropdown indicator no longer renders. Update SelectPrimitive.Icon in
select-custom.tsx to keep a real icon element as its direct child, and remove
only the asChild usage while preserving the previous chevron-style icon
behavior.
---
Nitpick comments:
In `@src/frontend/src/components/ui/__tests__/select-custom.test.tsx`:
- Around line 12-31: The Select tests only validate that rendering succeeds and
text is present, but they do not cover the icon composition introduced via
SelectPrimitive.Icon. Update the Select - Radix Slot 1.3.0 compatibility suite
in renderSelect to assert that the icon is actually rendered in the DOM, using a
stable query such as a test id or svg/role lookup. Keep the existing checks, but
add an explicit icon-presence assertion so this test fails if the icon
disappears from select-custom.tsx.
🪄 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: f735e55d-a478-425f-8e38-05e2cc67dacc
📒 Files selected for processing (2)
src/frontend/src/components/ui/__tests__/select-custom.test.tsxsrc/frontend/src/components/ui/select-custom.tsx
| > | ||
| {children} | ||
| <SelectPrimitive.Icon asChild></SelectPrimitive.Icon> | ||
| <SelectPrimitive.Icon></SelectPrimitive.Icon> |
There was a problem hiding this comment.
🎯 Functional Correctness | 🔴 Critical | ⚡ Quick win
Icon child was dropped, not just asChild.
The fix should remove asChild while keeping the icon element as a regular child of SelectPrimitive.Icon. As written, <SelectPrimitive.Icon></SelectPrimitive.Icon> has no children at all, so the dropdown icon will no longer render — contradicting the PR's claim that visual UI behavior is unaffected.
🐛 Proposed fix
- <SelectPrimitive.Icon></SelectPrimitive.Icon>
+ <SelectPrimitive.Icon>
+ <ChevronDownIcon />
+ </SelectPrimitive.Icon>Please restore whatever icon element was previously passed via asChild (e.g. a chevron icon) as a direct child.
📝 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.
| <SelectPrimitive.Icon></SelectPrimitive.Icon> | |
| <SelectPrimitive.Icon> | |
| <ChevronDownIcon /> | |
| </SelectPrimitive.Icon> |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/frontend/src/components/ui/select-custom.tsx` at line 23, The Select icon
wrapper was left empty in SelectCustom, so the dropdown indicator no longer
renders. Update SelectPrimitive.Icon in select-custom.tsx to keep a real icon
element as its direct child, and remove only the asChild usage while preserving
the previous chevron-style icon behavior.
1 participant
Summary
This PR addresses UI runtime rendering issues observed after upgrading
@radix-ui/react-slotfrom^1.2.4tov1.3.0.After the upgrade, stricter type safety and improved component composition rules in
@radix-ui/react-slot@1.3.0exposed previously implicit or unsafe usage patterns in the Select component. These patterns led to runtime rendering issues in specific composition scenarios.This PR updates the implementation to align with safer and more explicit Slot composition patterns, improving compatibility with the current Radix version and ensuring resilience against future updates.
Changes
Fix Slot usage in Select Icon component
File:
src/frontend/src/components/ui/select-custom.tsxBefore:
After:
Why this change is needed
SelectPrimitive.Icondoes not requireasChildbecause it does not rely on external children injection.Removing
asChildensures correct Slot composition behavior and avoids relying on implicit child injection patterns that may not be compatible with stricter Slot composition rules introduced in@radix-ui/react-slot@1.3.0.Motivation
The upgrade to
@radix-ui/react-slot@1.3.0introduced stricter enforcement of Slot composition correctness and improved handling of edge cases in component structure.While this is not a breaking API change, it surfaced previously valid but implicit usage patterns that were not fully aligned with the updated composition model.
This PR resolves these issues by adopting explicit and safe Slot usage patterns.
Behavior Impact
Notes
This change improves forward compatibility with future versions of
@radix-ui/react-slotby ensuring explicit and valid Slot composition usage.Summary by CodeRabbit
Bug Fixes
Tests