test: cover Primitive asChild and forwarding #1521
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 6 minutes and 3 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 (2)
WalkthroughIntroduces a runtime guard in Primitive’s asChild path to detect React.Fragment as the sole child, warn, and render the host element directly. Adds a dedicated test suite covering prop/ref forwarding, fragment and invalid-children fallbacks, controlled/uncontrolled inputs, and accessibility checks. No public APIs or types changed. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant P as Primitive (asChild)
participant C as Child
participant H as Host Element (elementType)
note over P: Entry: render with asChild
U->>P: render({ asChild, children, elementType })
P->>P: validate children count
alt single valid child AND child is NOT Fragment
P->>C: cloneElement(child, mergedProps, ref)
C-->>U: rendered child with forwarded props/ref
else single valid child AND child IS React.Fragment
note over P,H: Warn: "Primitive.{elementType}: asChild does not support React.Fragment"
P->>H: createElement(elementType, props, children)
H-->>U: rendered host element
else invalid (null/multiple)
note over P,H: Fallback to host element
P->>H: createElement(elementType, props)
H-->>U: rendered host element
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
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 (3)
src/core/primitives/Primitive/index.tsx (2)
51-58: Prop merge drops parent className and handlers when child defines them; compose instead of overrideToday child.props wins entirely. At minimum, merge className; consider composing common event handlers to avoid surprising overrides.
Apply:
return React.cloneElement(child, { - // Start with all the elementProps - ...elementProps, - // Override with the child's own props to preserve them - ...child.props, + // Start with elementProps, then child's props take precedence + ...elementProps, + ...child.props, + // Merge className + className: [elementProps.className, child.props.className].filter(Boolean).join(' '), // Only forward ref if it exists ...(ref ? { ref: mergedRef } : {}) });Optional follow-up: introduce a small composeEventHandlers utility (with defaultPrevented guard) for onClick, onKeyDown, etc.
8-15: Tighten typings per intrinsic element to preserve attribute/ref specificityUsing a single PrimitiveProps union with HTMLElement ref loses specificity (e.g., img alt, button-specific attrs, precise ref types). Consider a generic factory typed by intrinsic element name so Primitive.button exposes button props and ref HTMLButtonElement, Primitive.img exposes ImgHTMLAttributes, etc.
I can draft a minimal polymorphic typing scaffold based on JSX.IntrinsicElements if helpful.
Also applies to: 68-75
src/core/primitives/Primitive/tests/Primitive.asChild.test.tsx (1)
123-129: Optional: switch to jest-axe for clearer a11y assertionsaxe-core works, but jest-axe provides idiomatic matchers (toHaveNoViolations) and better failure messages in Jest.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/core/primitives/Primitive/index.tsx(1 hunks)src/core/primitives/Primitive/tests/Primitive.asChild.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/core/primitives/Primitive/tests/Primitive.asChild.test.tsx (1)
src/setupTests.ts (1)
ACCESSIBILITY_TEST_TAGS(3-3)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (3)
src/core/primitives/Primitive/tests/Primitive.asChild.test.tsx (3)
15-39: LGTM: solid coverage for prop/data/ref forwarding with user interactionsThis verifies the core asChild happy-path behavior and ref wiring. Looks good.
69-82: Add a dedicated test for “single Fragment” child to validate the new guardCurrent tests cover null and multiple-children-in-Fragment, but not a sole Fragment. This misses the exact scenario the runtime guard targets.
Add:
+ test('warns and renders host element when sole child is a Fragment', () => { + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); + const { container } = render( + <Primitive.div asChild> + <React.Fragment> + <span data-testid="inside" /> + </React.Fragment> + </Primitive.div> + ); + expect(warnSpy).toHaveBeenCalledTimes(1); + expect(container.firstElementChild?.tagName).toBe('DIV'); + expect(screen.getByTestId('inside')).toBeInTheDocument(); + warnSpy.mockRestore(); + });
84-121: LGTM: controlled/uncontrolled propagation through asChildGood end-to-end check that value/onChange and defaultValue flow correctly.
Summary
asChildPrimitiveprop/attribute forwarding, ref behavior, and invalid casesTesting
npm test src/core/primitives/Primitive/tests/Primitive.asChild.test.tsxnpm test src/core/primitives/Primitive/tests/Primitive.test.jsSummary by CodeRabbit
Bug Fixes
Tests