refactor: forward refs in Tabs #1454
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 4 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 (4)
WalkthroughRef-forwarding was added to Tabs components (Root, List, Trigger, Content) and RovingFocusRoot. Props were broadened to extend native element props and support asChild composition. Trigger now handles click and focus-driven activation explicitly. Tests were added for ref forwarding, accessibility state (aria-hidden), and warning-free rendering. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Trigger as Tabs.Trigger (button)
participant Root as Tabs.Root (RovingFocusGroup.Root)
participant List as Tabs.List (div or asChild)
participant Content as Tabs.Content (div)
participant RFG as RovingFocusGroup
Note over Root,RFG: Refs forwarded to underlying elements
User->>Trigger: Click / Focus
alt Click
Trigger->>Root: handleTabChange(value)
else Focus (activationMode=automatic)
Trigger->>Root: handleTabChange(value)
end
Root->>Content: Update active tab value
Content-->>User: Render active content (aria-hidden=false)
Content-->>User: Inactive content (aria-hidden=true, if forceMount)
Trigger->>RFG: Participate in roving focus
RFG-->>Trigger: Manage focus movement
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/ui/Tabs/fragments/TabList.tsx (1)
3-3: Fix clsx import (named vs default export)
clsxexports a default function; the named import will be undefined at runtime.-import { clsx } from 'clsx'; +import clsx from 'clsx';src/components/ui/Tabs/fragments/TabRoot.tsx (1)
75-86: ForwardasChildand move...propsbefore core props onRovingFocusGroup.Root- <RovingFocusGroup.Root - ref={ref} - orientation={orientation} - loop - dir={dir} - className={clsx(rootClass, className)} - data-rad-ui-accent-color={color} - {...dataAttributes} - {...props} - > + <RovingFocusGroup.Root + ref={ref} + asChild={asChild} + {...props} + {...dataAttributes} + orientation={orientation} + loop + dir={dir} + className={clsx(rootClass, className)} + data-rad-ui-accent-color={color} + >
🧹 Nitpick comments (6)
src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx (2)
7-14: Update JSDoc to match the actual props (orientation 'both', dir, mode, disableTabIndexing)Docs are out of sync with the type surface.
Apply:
/** * Props for the RovingFocusRoot component * @property {React.ReactNode} children - Child components (should include RovingFocusGroup) - * @property {'horizontal'|'vertical'} [orientation='horizontal'] - Orientation of keyboard navigation - * @property {boolean} [loop=true] - Whether focus should loop from last to first item and vice versa - * @property {string} [aria-label] - Accessible label for the roving focus group - * @property {string} [aria-labelledby] - ID of an element that labels the roving focus group + * @property {'horizontal'|'vertical'|'both'} [orientation='horizontal'] - Orientation of keyboard navigation + * @property {'ltr'|'rtl'} [dir='ltr'] - Text direction, used for arrow-key handling + * @property {'default'|'tree'} [mode='default'] - Navigation mode + * @property {boolean} [loop=true] - Whether focus should loop from last to first item and vice versa + * @property {boolean} [disableTabIndexing=false] - Disable tabIndex management on items + * @property {string} [aria-label] - Accessible label for the roving focus group + * @property {string} [aria-labelledby] - ID of an element that labels the roving focus group */
60-66: Preserve controlled ARIA and add dir to DOMAvoid consumers overriding required ARIA accidentally; also reflect
diron the element for CSS/AT.- <Primitive.div - ref={ref} - aria-orientation={orientation === 'both' ? undefined : orientation} - aria-label={ariaLabel} - aria-labelledby={ariaLabelledBy} - {...props} - > + <Primitive.div + ref={ref} + {...props} + dir={dir} + aria-orientation={orientation === 'both' ? undefined : orientation} + aria-label={ariaLabel} + aria-labelledby={ariaLabelledBy} + >src/components/ui/Tabs/tests/Tabs.test.tsx (1)
555-575: Always restore console spies even on failureUse try/finally to avoid cross-test leakage.
- test('renders without console warnings', () => { - const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); - const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); - render( + test('renders without console warnings', () => { + const warnSpy = jest.spyOn(console, 'warn').mockImplementation(() => {}); + const errorSpy = jest.spyOn(console, 'error').mockImplementation(() => {}); + try { + render( <Tabs.Root defaultValue="tab1"> <Tabs.List> <Tabs.Trigger value="tab1">Tab 1</Tabs.Trigger> <Tabs.Trigger value="tab2">Tab 2</Tabs.Trigger> </Tabs.List> <Tabs.Content value="tab1">Content 1</Tabs.Content> <Tabs.Content value="tab2">Content 2</Tabs.Content> </Tabs.Root> - ); - expect(warnSpy).not.toHaveBeenCalled(); - expect(errorSpy).not.toHaveBeenCalled(); - warnSpy.mockRestore(); - errorSpy.mockRestore(); + ); + expect(warnSpy).not.toHaveBeenCalled(); + expect(errorSpy).not.toHaveBeenCalled(); + } finally { + warnSpy.mockRestore(); + errorSpy.mockRestore(); + } });src/components/ui/Tabs/fragments/TabList.tsx (1)
31-37: Preserve child ref when asChild=trueCloning overrides the child’s existing ref. Compose refs to avoid breaking consumers.
- ? React.cloneElement(children as React.ReactElement<any>, { - ...childProps, - className: clsx((children as React.ReactElement<any>).props.className, childProps.className) - }) + ? React.cloneElement(children as React.ReactElement<any>, { + ...childProps, + // compose child's ref with forwarded ref + ref: (node: TabListElement) => { + // @ts-ignore - accessing possible ref callback/object + const childRef = (children as any).ref; + if (typeof childRef === 'function') childRef(node); + else if (childRef && 'current' in childRef) childRef.current = node; + if (typeof ref === 'function') ref(node); + else if (ref && 'current' in (ref as any)) (ref as any).current = node; + }, + className: clsx((children as React.ReactElement<any>).props.className, childProps.className) + })Alternatively, use a small helper or
@radix-ui/react-compose-refs.src/components/ui/Tabs/fragments/TabContent.tsx (1)
17-17:asChildis declared but unused.Either wire it up (e.g., via a Slot) or drop it to avoid a dead prop.
src/components/ui/Tabs/fragments/TabTrigger.tsx (1)
11-13:asChildis unused.If
asChildis part of the public API, implement it (e.g., Slot-based) or remove to reduce surface area.Also applies to: 21-21
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (6)
src/components/ui/Tabs/fragments/TabContent.tsx(2 hunks)src/components/ui/Tabs/fragments/TabList.tsx(1 hunks)src/components/ui/Tabs/fragments/TabRoot.tsx(4 hunks)src/components/ui/Tabs/fragments/TabTrigger.tsx(2 hunks)src/components/ui/Tabs/tests/Tabs.test.tsx(1 hunks)src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx(5 hunks)
⏰ 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 (8)
src/core/utils/RovingFocusGroup/fragments/RovingFocusRoot.tsx (2)
40-50: Ref forwarding implementation looks goodForwarded ref is correctly attached to the DOM node and displayName is set.
Also applies to: 61-71
17-24: Confirm 'both' orientation is supported in downstream roving logicIf items rely on horizontal/vertical branching, passing 'both' into context may break key handling.
Would you like a quick repo scan to locate all consumers of
orientationin RovingFocus* and verify handling for'both'?src/components/ui/Tabs/tests/Tabs.test.tsx (2)
504-526: Solid ref-forwarding assertionsGood coverage on Root/List/Trigger/Content ref targets and element types.
529-551: A11y toggle test is accurateforceMount + aria-hidden checks validate the new behavior well.
src/components/ui/Tabs/fragments/TabList.tsx (1)
39-40: LGTM: grouping with RovingFocus is clearReturning the list element inside
RovingFocusGroup.Groupkeeps concerns separated.src/components/ui/Tabs/fragments/TabContent.tsx (1)
9-15: Types + forwardRef look solid.Element/props aliases are correct; children comes via div props.
src/components/ui/Tabs/fragments/TabRoot.tsx (2)
13-24: Type layering looks good.Omitting
orientationfrom inherited props and reintroducing it explicitly is correct.
26-39: Ref-forwarding and controllable state wiring look good.The forwardRef target and controlled/uncontrolled bridge are implemented cleanly.
| const TabTrigger = React.forwardRef<TabTriggerElement, TabTriggerProps>(({ | ||
| value, | ||
| children, | ||
| className = '', | ||
| disabled, | ||
| asChild = false, | ||
| ...props | ||
| }, forwardedRef) => { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Compose onClick, don’t override it; also set type="button" and lock attrs after spreads.
Currently, a consumer-provided onClick in {...props} will replace internal activation. Compose handlers and place {...props} before component-defined props so ARIA/disabled can’t be overridden. Add type="button" to avoid unintended form submits.
-const TabTrigger = React.forwardRef<TabTriggerElement, TabTriggerProps>({
- value,
- children,
- className = '',
- disabled,
- asChild = false,
- ...props
-}, forwardedRef) => {
+const TabTrigger = React.forwardRef<TabTriggerElement, TabTriggerProps>({
+ value,
+ children,
+ className = '',
+ disabled,
+ asChild = false,
+ onClick: userOnClick,
+ ...props
+}, forwardedRef) => {
@@
- <button
- ref={ref}
- onClick={handleClick}
+ <button
+ ref={ref}
+ {...props}
+ {...dataAttributes}
+ type="button"
+ onClick={(e) => {
+ userOnClick?.(e);
+ if (!e.defaultPrevented) handleClick(e);
+ }}
className={clsx(
`${rootClass}-trigger`,
isActive ? 'active' : '',
disabled ? 'disabled' : '',
className
)}
role="tab"
aria-selected={isActive}
aria-disabled={disabled}
disabled={disabled}
- {...dataAttributes}
- {...props}
>Please run: npx jest src/components/ui/Tabs/tests/Tabs.test.tsx --runInBand --silent
Also applies to: 66-81
🏁 Script executed:
#!/bin/bash
set -e
# Locate the TabTrigger component file
file=$(find src -type f -path '*/Tabs/fragments/TabTrigger.tsx' | head -n1)
echo "Inspecting $file"
# Show the component signature and prop destructuring
sed -n '1,50p' "$file"
# List all occurrences of onClick in the file
echo "------ onClick occurrences ------"
grep -n "onClick" "$file" || true
# Show the Button render snippet where spreads and attributes are applied
echo "------ Button render snippet ------"
sed -n '60,120p' "$file"Length of output: 3045
Extract onClick, compose with internal handler, reorder spreads before locked attrs, and add type="button
Destructure onClick from props to call the consumer’s handler before handleClick, move {...props} ahead of explicit props (disabled, role, ARIA, data-attributes) so they can’t be overridden, and add type="button" to the <button> to prevent implicit submits.
🤖 Prompt for AI Agents
In src/components/ui/Tabs/fragments/TabTrigger.tsx around lines 15 to 22,
destructure onClick from props, call the consumer onClick before calling the
component's internal handleClick, move the spread {...props} so it appears
before locked attributes (disabled, role, aria-*, data-*) to prevent consumers
from accidentally overriding them, and ensure the rendered button includes
type="button" to avoid implicit form submissions.
|
Closing as stale |
Summary
Testing
npx jest src/components/ui/Tabs/tests/Tabs.test.tsx --runInBand --silentnpm run build:rollup --silenthttps://chatgpt.com/codex/tasks/task_e_68ba4e158f748331af0892f0aa0e5aeb
Summary by CodeRabbit
New Features
Accessibility
Refactor
Tests