feat: improve Button primitive and expand tests #1541
Conversation
|
Warning Rate limit exceeded@kotAPI has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 1 minutes and 39 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (71)
WalkthroughAdds a new useMergeProps hook and applies it in Primitive’s asChild flow; updates ButtonPrimitive to handle onClick when disabled (preventing clicks and adding Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Primitive as Primitive (asChild)
participant Hook as useMergeProps
participant Child as React child
Note over Primitive: render with asChild path
Primitive->>Hook: useMergeProps(elementProps, child, ref)
Hook-->>Primitive: mergedProps (child-first, className concat, composed ref)
Primitive->>Child: React.cloneElement(child, mergedProps)
Child-->>Primitive: Rendered child with merged props
sequenceDiagram
autonumber
participant User as User
participant Element as Button element
participant Button as ButtonPrimitive
User->>Element: click
Element->>Button: handleClick invoked
alt disabled === true
Button->>Element: preventDefault(), stopPropagation()
Note right of Button: consumer onClick NOT called
else enabled
Button->>Button: call consumer onClick
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Pre-merge checks (2 passed, 1 inconclusive)❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
Poem
✨ Finishing Touches
🧪 Generate unit tests
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (6)
src/core/hooks/useMergeProps/index.ts (1)
55-59: Also merge inline styles; keep child precedence.Style objects should be shallow-merged so child styles override parent values without dropping non-overlapping keys.
if (elementProps.className || (childProps as any).className) { finalProps.className = [elementProps.className, (childProps as any).className] .filter(Boolean) .join(' '); } + // Merge inline styles with child taking precedence + if ((elementProps as any).style || (childProps as any).style) { + finalProps.style = { + ...(elementProps as any).style, + ...(childProps as any).style, + }; + }src/components/ui/Button/tests/Button.test.tsx (1)
46-57: Good coverage for disabled click + data-disabled. Add keyboard assertion (Enter/Space).Disabled buttons must also ignore keyboard activation.
const button = screen.getByRole('button'); await userEvent.click(button); expect(onClick).not.toHaveBeenCalled(); expect(button).toHaveAttribute('data-disabled', ''); + // Keyboard activation should also be suppressed + button.focus(); + await userEvent.keyboard('{Enter}'); + await userEvent.keyboard(' '); + expect(onClick).not.toHaveBeenCalled();src/core/hooks/useMergeProps/useMergeProps.test.tsx (1)
91-104: Add test for style merging to prevent regressions.test('composes callback refs from parent and child', () => { const parentRef = jest.fn(); const childRef = jest.fn(); const child = <div ref={childRef} />; const { container } = render( <Merge elementProps={{}} child={child} primitiveRef={parentRef} /> ); const div = container.firstChild as HTMLDivElement; expect(parentRef).toHaveBeenCalledWith(div); expect(childRef).toHaveBeenCalledWith(div); }); + + test('merges style objects with child winning conflicts', () => { + const child = <div style={{ color: 'red', padding: '4px' }} />; + const { container } = render( + <Merge elementProps={{ style: { color: 'blue', margin: '8px' } }} child={child} /> + ); + const div = container.firstChild as HTMLDivElement; + expect(div.style.color).toBe('red'); + expect(div.style.margin).toBe('8px'); + expect(div.style.padding).toBe('4px'); + });src/core/primitives/Button/index.tsx (2)
49-56: ARIA tweaks: boolean aria-disabled; be cautious with aria-description.Use boolean for aria-disabled. aria-description has limited AT support; consider pairing with aria-describedby in docs later.
- if (disabled) { + if (disabled) { // If the button is disabled, we should set the aria-disabled attribute - props['aria-disabled'] = 'true'; + props['aria-disabled'] = true; if (!description) { // If description isn't set, we set a default description props['aria-description'] = 'Disabled Button'; } }
66-76: Also guard keyboard activation when disabled (Enter/Space).Prevents unintended activation for non-native targets (e.g., asChild anchor) and aligns with ARIA button behavior.
- return <Primitive.button + const handleKeyDown: React.KeyboardEventHandler<HTMLButtonElement> = (event) => { + if (disabled && (event.key === 'Enter' || event.key === ' ')) { + event.preventDefault(); + event.stopPropagation(); + } + }; + + return <Primitive.button ref={ref} role={role} type={type} disabled={disabled} data-disabled={disabled ? '' : undefined} - onClick={handleClick} + onClick={handleClick} + onKeyDown={handleKeyDown} {...props}src/components/ui/Button/tests/Button.asChild.test.tsx (1)
60-71: Solid disabled coverage; add keyboard case for anchor.test('disabled suppresses clicks and sets data-disabled', async () => { const onClick = jest.fn(); render( <Button asChild disabled onClick={onClick}> <a>Link</a> </Button> ); const anchor = screen.getByRole('button'); await userEvent.click(anchor); expect(onClick).not.toHaveBeenCalled(); expect(anchor).toHaveAttribute('data-disabled', ''); }); + + test('disabled anchor suppresses keyboard activation (Enter/Space)', async () => { + const onClick = jest.fn(); + render( + <Button asChild disabled onClick={onClick}> + <a>Link</a> + </Button> + ); + const anchor = screen.getByRole('button'); + anchor.focus(); + await userEvent.keyboard('{Enter}'); + await userEvent.keyboard(' '); + expect(onClick).not.toHaveBeenCalled(); + });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/components/ui/Button/tests/Button.asChild.test.tsx(1 hunks)src/components/ui/Button/tests/Button.test.tsx(1 hunks)src/core/hooks/useMergeProps/index.ts(1 hunks)src/core/hooks/useMergeProps/useMergeProps.test.tsx(1 hunks)src/core/primitives/Button/index.tsx(2 hunks)src/core/primitives/Primitive/index.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
src/core/primitives/Button/index.tsx (1)
src/core/primitives/Menu/fragments/MenuPrimitiveItem.tsx (1)
onClick(44-46)
src/components/ui/Button/tests/Button.asChild.test.tsx (1)
src/setupTests.ts (1)
ACCESSIBILITY_TEST_TAGS(3-3)
🪛 Biome (2.1.2)
src/core/primitives/Primitive/index.tsx
[error] 32-32: This hook is being called conditionally, but all hooks must be called in the exact same order in every component render.
For React to preserve state between calls, hooks needs to be called unconditionally and always in the same order.
See https://reactjs.org/docs/hooks-rules.html#only-call-hooks-at-the-top-level
(lint/correctness/useHookAtTopLevel)
🔇 Additional comments (4)
src/core/hooks/useMergeProps/index.ts (1)
37-46: Ref composition looks correct.Callback ref composes both sides in the right order and handles object refs.
src/core/primitives/Button/index.tsx (1)
57-64: Good: click guard prevents disabled interactions.Wrapper cleanly suppresses disabled clicks and preserves user onClick when enabled.
src/components/ui/Button/tests/Button.asChild.test.tsx (2)
50-58: Nice: prevents nested buttons with asChild.
73-89: Form interactions look good.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/core/primitives/Primitive/index.tsx (3)
9-12: Fix incorrect prop typing; current union drops tag-specific props like href/src and button attributes.Use per-element props to preserve correct attribute support across tags.
Apply this diff:
-type PrimitiveProps = - | (React.InputHTMLAttributes<HTMLInputElement> & { asChild?: boolean }) - | (React.HTMLAttributes<HTMLElement> & { asChild?: boolean, children?: React.ReactNode }); +type PrimitiveProps<E extends SupportedElement> = + React.ComponentPropsWithoutRef<E> & { asChild?: boolean };
14-16: Make component generic so ref/props match the intrinsic element (not HTMLElement).Aligns ref types (e.g., HTMLButtonElement) and restores accurate IntelliSense.
Apply this diff:
-const createPrimitiveComponent = (elementType: SupportedElement) => { - const PrimitiveComponent = React.forwardRef<HTMLElement, PrimitiveProps>((props, ref) => { +const createPrimitiveComponent = <E extends SupportedElement>(elementType: E) => { + const PrimitiveComponent = React.forwardRef<React.ElementRef<E>, PrimitiveProps<E>>((props, ref) => {
46-53: Preserve per-element typing in the exported Primitive map.The current Record<..., PrimitiveProps & Ref> erases specificity.
Apply this diff:
-const Primitive = SUPPORTED_HTML_ELEMENTS.reduce<Record<SupportedElement, React.ForwardRefExoticComponent<PrimitiveProps & React.RefAttributes<HTMLElement>>>>( - (components, elementType) => { - components[elementType] = createPrimitiveComponent(elementType); - return components; - }, - {} as Record<SupportedElement, React.ForwardRefExoticComponent<PrimitiveProps & React.RefAttributes<HTMLElement>>> -); +const Primitive = SUPPORTED_HTML_ELEMENTS.reduce( + (components, elementType) => { + components[elementType] = createPrimitiveComponent(elementType as any) as any; + return components; + }, + {} as { + [E in SupportedElement]: React.ForwardRefExoticComponent< + React.PropsWithoutRef<PrimitiveProps<E>> & + React.RefAttributes<React.ElementRef<E>> + > + } +);
♻️ Duplicate comments (1)
src/core/primitives/Primitive/index.tsx (1)
24-27: Good: hook now called unconditionally (fixes rule-of-hooks violation).This resolves the prior Biome error.
🧹 Nitpick comments (1)
src/core/primitives/Primitive/index.tsx (1)
29-34: Gate console.warn to dev-only.Avoid noisy logs in production bundles.
Apply this diff:
- if (!child) { - console.warn( - `Primitive.${elementType}: asChild prop requires exactly one valid child element.` - ); + if (!child) { + if (process.env.NODE_ENV !== 'production') { + console.warn( + `Primitive.${elementType}: asChild prop requires exactly one valid child element.` + ); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/hooks/useMergeProps/index.ts(1 hunks)src/core/primitives/Primitive/index.tsx(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/hooks/useMergeProps/index.ts
🔇 Additional comments (3)
src/core/primitives/Primitive/index.tsx (3)
18-23: Confirm fragment semantics for asChild.Children.toArray flattens fragments; asChild should reject fragments with multiple children and accept exactly one effective child.
Add/confirm tests:
- asChild with <>OneChild</> passes.
- asChild with <></> warns and falls back.
2-2: LGTM on import/use.Import path is consistent with aliasing and removes inline merge logic.
24-27: No action needed: useMergeProps already guards against undefined child
Thechildparameter is optional (typedchild?: React.ReactElement), uses the safechild?.propsaccess, and early-returnselementPropswhenchildPropsis undefined—so no further changes are required.
| // Only forward ref if it exists | ||
| ...(ref ? { ref: mergedRef } : {}) | ||
| }); | ||
| return React.cloneElement(child, mergedProps); |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Double-check mergedProps doesn’t include children.
Cloning should not override the child’s own children unless intentional.
Run:
Expected: children is omitted from the merged result.
🏁 Script executed:
#!/bin/bash
rg -n -C2 'return {[^}]*children' src/core/hooks/useMergePropsLength of output: 111
🏁 Script executed:
#!/bin/bash
# Locate the useMergeProps hook implementation
hook_file=$(fd -e ts?x -t f useMergeProps src/core/hooks)
echo "Inspecting: $hook_file"
# Show return object and any children property in it
rg -n -C3 'return\s*{' "$hook_file"
rg -n -C3 'children\s*:' "$hook_file"Length of output: 137
#!/bin/bash
# Show imports and hook usage in Primitive/index.tsx
sed -n '1,50p' src/core/primitives/Primitive/index.tsx
grep -n 'useMergeProps' src/core/primitives/Primitive/index.tsx#!/bin/bash
# Find the definition or reference of useMergeProps in hooks
grep -R -n "useMergeProps" src/core/hooks
# If a file is found, inspect its return object for inclusion of 'children'
for file in $(grep -R -l "useMergeProps" src/core/hooks); do
echo "------ $file ------"
rg -n -C3 'return\s*{' "$file"
rg -n -C3 'children\s*:' "$file"
done🏁 Script executed:
#!/bin/bash
# Locate any TypeScript files defining or referencing mergeProps
files=$(fd -e ts -e tsx mergeProps .) || true
echo "Found mergeProps references in:"
echo "$files"
for file in $files; do
echo "===== $file ====="
# Show first 100 lines to identify hook or util
sed -n '1,100p' "$file"
doneLength of output: 32
🏁 Script executed:
#!/bin/bash
# Display the top of the Primitive component to locate the import of useMergeProps
sed -n '1,60p' src/core/primitives/Primitive/index.tsx
# Globally search for useMergeProps definition or references
grep -R -nC3 'useMergeProps' .
# Globally search for any occurrence of mergeProps
grep -R -nC3 'mergeProps' .Length of output: 6171
🏁 Script executed:
#!/bin/bash
# Display the implementation of useMergeProps to verify if 'children' is omitted
sed -n '1,200p' src/core/hooks/useMergeProps/index.tsLength of output: 2494
Omit children from mergedProps
In useMergeProps, you’re spreading in childProps, which includes the child’s own children prop—passing that into cloneElement will override its children. Destructure to remove children before merging, for example:
const { children: _omit, ...childPropsWithoutChildren } = childProps!;
const finalProps: any = {
...elementProps,
...childPropsWithoutChildren,
...(ref || childRef ? { ref: mergedRef } : {}),
};🤖 Prompt for AI Agents
In src/core/primitives/Primitive/index.tsx around line 36, mergedProps currently
spreads childProps which includes the child's own children and thus can override
the cloned element's children; destructure children out of childProps (e.g.
const { children: _omit, ...childPropsWithoutChildren } = childProps || {}) and
use that childPropsWithoutChildren when building mergedProps/ finalProps,
keeping the existing elementProps and ref/mergedRef handling so cloneElement
does not inadvertently replace children.
Summary
asChildin PrimitiveuseMergePropshookuseMergePropswith usage guidelines and common pitfallsuseMergePropstests to cover class name fallback, event handler precedence, and callback ref compositionTesting
npm test src/core/hooks/useMergeProps/useMergeProps.test.tsx src/components/ui/Button/tests/Button.test.tsx src/components/ui/Button/tests/Button.asChild.test.tsxSummary by CodeRabbit
New Features
Bug Fixes
Tests