Refactor how ref is passed in cloneElement- Fix broken tests #623
Conversation
…Attributes<HTMLElement>
WalkthroughThe changes in the Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #623 +/- ##
==========================================
+ Coverage 96.64% 96.75% +0.10%
==========================================
Files 15 15
Lines 149 154 +5
Branches 28 29 +1
==========================================
+ Hits 144 149 +5
Misses 5 5 ☔ View full report in Codecov by Sentry. |
![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
🧹 Outside diff range and nitpick comments (1)
src/core/primitives/Primitive/index.tsx (1)
19-22: Consider improving type safety without type assertionsThe current implementation uses a type assertion (
as React.HTMLAttributes<HTMLElement>), which bypasses TypeScript's type checking. Consider using a more type-safe approach.Consider this alternative approach:
type ElementProps<T> = Omit<React.HTMLAttributes<T>, keyof PrimitiveProps>; function cloneElementWithRef<T extends HTMLElement>( element: React.ReactElement, props: ElementProps<T>, ref: React.Ref<T> ) { return React.cloneElement(element, { ...props, ref }); }This would provide better type inference and catch potential type mismatches at compile time rather than runtime.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
src/core/primitives/Primitive/index.tsx(1 hunks)
🔇 Additional comments (2)
src/core/primitives/Primitive/index.tsx (2)
19-22: 🛠️ Refactor suggestion
Consider improving ref handling for component composition
The current implementation might have issues when the child component already has a ref. Consider using a ref merging utility to handle both refs properly.
Here's a suggested improvement:
- return React.cloneElement(children, {
- ...elementProps,
- ref
- } as React.HTMLAttributes<HTMLElement>);
+ // Add this utility function at the top of the file
+ const mergeRefs = <T extends any>(
+ refs: Array<React.MutableRefObject<T> | React.LegacyRef<T> | null | undefined>
+ ): React.RefCallback<T> => {
+ return (value) => {
+ refs.forEach((ref) => {
+ if (typeof ref === 'function') {
+ ref(value);
+ } else if (ref != null) {
+ (ref as React.MutableRefObject<T | null>).current = value;
+ }
+ });
+ };
+ };
+
+ // In the component:
+ const childrenRef = (children as any).ref;
+ return React.cloneElement(children, {
+ ...elementProps,
+ ref: childrenRef ? mergeRefs([childrenRef, ref]) : ref
+ } as React.HTMLAttributes<HTMLElement>);This change will:
- Properly handle cases where both the primitive and child component have refs
- Maintain ref functionality for both components
- Preserve type safety while being more flexible
Let's verify if there are any existing ref merging utilities in the codebase:
19-22: Verify ref handling in test cases
Since this PR aims to fix broken tests, let's ensure all ref-related test cases are covered.
Let's check for existing test cases and potential issues:
✅ Verification successful
Ref handling implementation is correct and consistent
The verification shows that:
- The
Primitivecomponent is the only one usingReact.cloneElementwith ref forwarding, which is the correct pattern for polymorphic components - The implementation follows React's ref forwarding best practices using
React.forwardRef - Other components in the codebase handle refs directly through the
refprop without cloning, which is the expected pattern - The test file for the Primitive component covers basic rendering scenarios, though it could benefit from additional ref-specific test cases
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test files related to Primitive component
fd -e test.tsx -e spec.tsx -e test.ts -e spec.ts
# Search for ref-related test cases
rg -A 5 "ref.*=.*{" --type=typescript
# Look for similar patterns in other components
ast-grep --pattern 'React.cloneElement($_, { ref: $_ })'
Length of output: 591
Script:
#!/bin/bash
# Search for test files specifically related to Primitive component
cat src/core/primitives/Primitive/tests/Primitive.test.tsx
# Search for ref-related test cases with fixed regex
rg "ref\s*=\s*\{" --type=typescript -A 5
# Look for similar patterns with more specific ast-grep pattern
ast-grep --pattern 'React.cloneElement($_, { $$$, ref: $_ })'
Length of output: 1289
Script:
#!/bin/bash
# Search for ref-related test cases with correct file types
rg "ref\s*=\s*\{" -t tsx -t ts -A 5
# Look for ref forwarding patterns in the codebase
ast-grep --pattern 'React.forwardRef<$_, $_>($_)'
# Search for other components using cloneElement
rg "React\.cloneElement" -t tsx -t ts -A 5
Length of output: 1191
Script:
#!/bin/bash
# Search for ref-related test cases with extension
rg "ref\s*=\s*\{" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -t tsx -t ts -A 5
# Search for cloneElement usage with extension
rg "React\.cloneElement" --type-add 'tsx:*.tsx' --type-add 'ts:*.ts' -t tsx -t ts -A 5
# Check if there are any ref-related tests in other components
fd -e test.tsx -e test.ts -x grep -l "ref" {}
Length of output: 7612
Fixes broken tests in CI by specifying how ref is passed - #622
Summary by CodeRabbit
New Features
Primitivecomponent's props.asChildprop for better ref typing.Bug Fixes
Documentation