fix: forwardRef render functions not using ref

[winchesHe]

Closes #4167

📝 Description

⛳️ Current behavior (updates)

🚀 New behavior

💣 Is this a breaking change (Yes/No):

📝 Additional Information

Summary by CodeRabbit

• New Features
  • Enhanced functionality for dropdown, listbox, menu, modal, and popover components.
• Bug Fixes
  • Resolved issues with ref forwarding in multiple UI components, improving integration with React components.
• Refactor
  • Simplified component definitions by removing forwardRef wrappers, streamlining prop handling and internal logic across various components.

[changeset-bot]

🦋 Changeset detected

Latest commit: 11a263a

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 10 packages

Name	Type
@nextui-org/dropdown	Patch
@nextui-org/listbox	Patch
@nextui-org/menu	Patch
@nextui-org/modal	Patch
@nextui-org/popover	Patch
@nextui-org/react	Patch
@nextui-org/autocomplete	Patch
@nextui-org/select	Patch
@nextui-org/drawer	Patch
@nextui-org/date-picker	Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

[vercel]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
nextui-docs-v2	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 1, 2024 8:21am
nextui-storybook-v2	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Dec 1, 2024 8:21am

[coderabbitai]

Walkthrough

This pull request introduces patches to several components from the @nextui-org library, specifically the dropdown, listbox, menu, modal, and popover components. The primary change involves the removal of the forwardRef functionality from these components, altering their definitions to standard functional components. This change affects how refs are handled, potentially impacting the integration of these components with parent components that rely on ref forwarding for DOM manipulation.

Changes

File Path	Change Summary
packages/components/dropdown/src/dropdown-trigger.tsx	Removed forwardRef from DropdownTrigger, now a standard functional component.
packages/components/listbox/src/listbox-item.tsx	Removed forwardRef from ListboxItem, now a standard functional component.
packages/components/menu/src/menu-item.tsx	Removed forwardRef from MenuItem, now a standard functional component.
packages/components/modal/src/modal-content.tsx	Removed forwardRef from ModalContent, now a standard functional component.
packages/components/popover/src/popover-content.tsx	Removed forwardRef from PopoverContent, now a standard functional component.
packages/components/popover/src/popover-trigger.tsx	Removed forwardRef from PopoverTrigger, now a standard functional component.

Possibly related PRs

• fix: interactions with popover & focus issues #3137: Addresses interactions with the Popover component, relevant to the changes in the main PR involving ref forwarding.
• fix: RA dependencies bump #3240: Includes updates to the Popover component, aligning with the main PR's focus on enhancing the popover component's behavior.
• fix(dropdown): _a2.find is not a function #3762: Fixes an issue related to the Dropdown component, directly relevant to the changes in the main PR affecting the dropdown component.
• fix: missing li tag when href is specified #4168: Ensures list items are correctly wrapped in <li> tags when using the href property, relating to improvements in the dropdown and listbox components.

Suggested reviewers

• jrgarciadev
• wingkwong

---

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?

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

• Review comments: Directly reply to a review comment made by CodeRabbit. Example:
  • I pushed a fix in commit <commit_id>, please review it.
  • Generate unit testing code for this file.
  • Open a follow-up GitHub issue for this discussion.
• Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
  • @coderabbitai generate unit testing code for this file.
  • @coderabbitai modularize this function.
• PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
  • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
  • @coderabbitai read src/utils.ts and generate unit testing code.
  • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
  • @coderabbitai help me debug CodeRabbit configuration file.

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)

• @coderabbitai pause to pause the reviews on a PR.
• @coderabbitai resume to resume the paused reviews.
• @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
• @coderabbitai full review to do a full review from scratch and review all the files again.
• @coderabbitai summary to regenerate the summary of the PR.
• @coderabbitai resolve resolve all the CodeRabbit review comments.
• @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
• @coderabbitai help to get help.

Other keywords and placeholders

• Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
• Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
• Add @coderabbitai anywhere in the PR title to generate the title automatically.

Documentation and Community

• Visit our Documentation for detailed information on how to use CodeRabbit.
• Join our Discord Community to get help, request features, and share feedback.
• Follow us on X/Twitter for updates and announcements.

[winchesHe]

#4099 updated in here @wingkwong

[coderabbitai]

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (5)

packages/components/menu/src/menu-item.tsx (1)

Line range hint 11-70: Consider documenting the removal of ref forwarding

While the component's internal logic remains sound, the removal of ref forwarding should be documented in the component's JSDoc comments to inform internal developers about this architectural decision.

Apply this diff to add documentation:

 /**
  * @internal
+ * Note: This component previously supported ref forwarding. As of version X.X.X,
+ * ref forwarding has been removed to simplify the component's architecture.
  */

packages/components/listbox/src/listbox-item.tsx (1)

Line range hint 11-66: Consider architectural implications of removing ref forwarding

While the internal implementation remains clean and well-organized, removing ref forwarding might limit component flexibility. Consider these alternatives:

1. Keep ref forwarding but improve its typing
2. Document alternative approaches for DOM access

Would you like help with:

1. Implementing a type-safe ref forwarding solution?
2. Creating documentation for alternative DOM access patterns?

packages/components/modal/src/modal-content.tsx (1)

Line range hint 25-141: Documentation update needed for the new implementation

While the core functionality remains intact, the component's API has changed significantly. Consider adding or updating the following documentation:

1. Migration guide for users upgrading from the previous version
2. Updated examples showing the recommended patterns without ref usage
3. Changelog entry detailing this breaking change

Would you like me to help draft the documentation updates or create a GitHub issue to track this task?

packages/components/popover/src/popover-trigger.tsx (1)

Line range hint 35-40: Consider memoizing getTriggerProps separately

The useMemo dependency array includes child.props which could cause unnecessary re-renders if the child has inline object props.

Consider this optimization:

-  const {onPress, isDisabled, ...restProps} = useMemo(() => {
-    return getTriggerProps(mergeProps(otherProps, child.props), childRef);
-  }, [getTriggerProps, child.props, otherProps, childRef]);
+  const mergedProps = useMemo(() => {
+    return mergeProps(otherProps, child.props);
+  }, [otherProps, child.props]);
+  
+  const {onPress, isDisabled, ...restProps} = useMemo(() => {
+    return getTriggerProps(mergedProps, childRef);
+  }, [getTriggerProps, mergedProps, childRef]);

packages/components/dropdown/src/dropdown-trigger.tsx (1)

13-19: Consider adding proper type annotations for event handlers

The component could benefit from explicit type annotations for common button event handlers in the props interface.

 export interface DropdownTriggerProps {
   children?: React.ReactNode;
+  onClick?: (event: React.MouseEvent<HTMLButtonElement>) => void;
+  onKeyDown?: (event: React.KeyboardEvent<HTMLButtonElement>) => void;
 }

📜 Review details

Configuration used: .coderabbit.yaml
Review profile: CHILL

📥 Commits

Reviewing files that changed from the base of the PR and between 1c46061 and 11a263a.

📒 Files selected for processing (7)

• .changeset/sharp-years-raise.md (1 hunks)
• packages/components/dropdown/src/dropdown-trigger.tsx (1 hunks)
• packages/components/listbox/src/listbox-item.tsx (2 hunks)
• packages/components/menu/src/menu-item.tsx (2 hunks)
• packages/components/modal/src/modal-content.tsx (2 hunks)
• packages/components/popover/src/popover-content.tsx (2 hunks)
• packages/components/popover/src/popover-trigger.tsx (2 hunks)

🔇 Additional comments (11)

packages/components/menu/src/menu-item.tsx (1)

11-11: ⚠️ Potential issue

Breaking Change: Removal of ref forwarding capability

The change from forwardRef to a regular functional component removes the ability to forward refs to the underlying DOM element. Although this is an internal component, this change could impact parent components that rely on ref forwarding for DOM manipulation.

Let's verify the usage of refs with this component:

packages/components/listbox/src/listbox-item.tsx (1)

11-11: ⚠️ Potential issue

⚠️ Breaking Change: Removal of ref forwarding capability

The change from forwardRef to a standard functional component is a breaking change that will affect consumers who rely on ref forwarding to access the underlying DOM element.

Let's verify the impact of this change:

packages/components/popover/src/popover-content.tsx (2)

23-23: ⚠️ Potential issue

Critical: Removing forwardRef is a breaking change

Converting PopoverContent from a forwardRef to a regular functional component is a breaking change that will affect consumers who need to access the underlying DOM element. This change breaks React's component composition pattern and limits the component's reusability.

Consider keeping the forwardRef implementation:

-const PopoverContent = (props: PopoverContentProps) => {
+import {forwardRef} from "react";
+
+const PopoverContent = forwardRef<HTMLDivElement, PopoverContentProps>((props, ref) => {
  const {as, children, className, ...otherProps} = props;
  
  // Pass the forwarded ref to the dialog
-  const dialogRef = useRef(null);
+  const dialogRef = useRef(ref);

Let's verify the impact of this change:

---

Line range hint 23-115: Verify accessibility implications

The component maintains accessibility through React Aria's useDialog, but we should verify that removing ref forwarding doesn't impact advanced accessibility patterns.

✅ Verification successful

Accessibility implementation is properly maintained

The verification shows that the accessibility implementation is robust and safe:

• The component correctly uses React Aria's useDialog hook with proper dialog ref handling (dialogRef)
• ARIA props are properly forwarded through the composition of dialogProps from useDialog and getDialogProps
• The ref forwarding concern is addressed as the component maintains proper ref handling through the dialogRef which is used both for React Aria's dialog management and prop forwarding

The current implementation maintains all necessary accessibility patterns and the removal of ref forwarding doesn't impact accessibility since the component internally manages refs appropriately through React Aria's dialog management system.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Search for aria-* attributes used with PopoverContent
rg -A 2 "PopoverContent.*aria-"

# Look for custom keyboard handling that might rely on refs
rg -A 5 "onKeyDown.*PopoverContent"

Length of output: 71

---

Script:

#!/bin/bash
# Search for useDialog usage and its aria-related configurations
ast-grep --pattern 'useDialog($$$)'

# Look for aria-related props in tests or stories
rg "PopoverContent.*props" -A 5

# Check if there are any accessibility tests
fd ".*test.*" --exec rg -l "PopoverContent.*accessibility|aria"

# Look for ref usage patterns with PopoverContent
rg "PopoverContent.*ref"

Length of output: 4854

packages/components/modal/src/modal-content.tsx (1)

25-25: ⚠️ Potential issue

Breaking Change: Removal of ref forwarding capability

The removal of forwardRef is a breaking change that affects consumers who rely on accessing the modal's DOM element via refs. This change needs to be properly documented and communicated to users.

Let's verify the impact of this change:

.changeset/sharp-years-raise.md (1)

1-9: ⚠️ Potential issue

Critical: Version bump and description inconsistency

There appears to be a significant mismatch between the changeset and the actual code changes:

1. The description suggests "fixing forwardRef render functions", but the code changes appear to be removing forwardRef functionality entirely from components.
2. Removing forwardRef support is a breaking change as it affects how components can be referenced by parent components.
3. A patch version bump suggests backward-compatible fixes, but removing ref forwarding capabilities would break existing usage patterns.

Please:

1. Clarify whether the intent is to fix or remove forwardRef
2. If removing forwardRef, this should be a major version bump
3. Update the changeset description to accurately reflect the changes

Let's verify the actual changes to these components:

packages/components/popover/src/popover-trigger.tsx (3)

Line range hint 42-52: LGTM: Button validation and rendering logic

The Button component detection and conditional props merging logic is well-implemented. The use of useMemo for hasNextUIButton is a good optimization.

---

Line range hint 29-31: Verify ref fallback behavior

The comment references a React PR (#28348) for ref handling. While the fallback logic is sound, we should ensure it works consistently across different React versions.

✅ Verification successful

Let me check the implementation of the ref handling to ensure it aligns with React 18's behavior.

---

Ref handling implementation is compatible with React 18+

The implementation correctly handles refs by:

1. Using the modern ref handling pattern mentioned in React PR #28348
2. Supporting both prop-based refs and element refs via child.props.ref ?? (child as any).ref
3. Working within React 18's constraints as verified by package.json requirements ("react": ">=18")

The fallback behavior is properly implemented and will work consistently across React 18+ versions.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check package.json for React version compatibility
rg '"react":\s*"([^"]+)"' -g 'package.json'

Length of output: 11833

---

Script:

#!/bin/bash
# Check the ref handling implementation in popover-trigger
ast-grep --pattern 'const $_ = $_ref || $_' packages/components/popover/src/popover-trigger.tsx

Length of output: 97

---

Script:

#!/bin/bash
# Let's see the actual implementation of the popover trigger component
cat packages/components/popover/src/popover-trigger.tsx

Length of output: 1869

---

17-17: ⚠️ Potential issue

Breaking Change: Removal of forwardRef

The removal of forwardRef is a breaking change that could affect consumers who rely on ref forwarding to access the underlying DOM element.

Let's verify the impact:

packages/components/dropdown/src/dropdown-trigger.tsx (2)

13-19: ⚠️ Potential issue

Breaking Change: Removal of ref forwarding capability

The removal of forwardRef is a breaking change that could impact existing implementations. This change:

• Prevents parent components from accessing the underlying DOM element
• May affect accessibility features that rely on ref forwarding
• Could break existing code that depends on ref forwarding for DOM manipulation

Consider keeping the forwardRef wrapper to maintain backward compatibility and accessibility support.

Suggested fix:

-const DropdownTrigger = (props: DropdownTriggerProps) => {
+import {ForwardedRef, forwardRef} from "react";
+
+const DropdownTrigger = forwardRef<HTMLButtonElement, DropdownTriggerProps>((props, ref) => {
  const {getMenuTriggerProps} = useDropdownContext();

  const {children, ...otherProps} = props;

-  return <PopoverTrigger {...getMenuTriggerProps(otherProps)}>{children}</PopoverTrigger>;
+  return <PopoverTrigger {...getMenuTriggerProps({...otherProps, ref})}>{children}</PopoverTrigger>;
});

---

13-19: Verify PopoverTrigger compatibility

Ensure that PopoverTrigger's props handling aligns with this change, especially regarding ref propagation.

✅ Verification successful

Let me gather more information about the context and props handling.

---

Let me check one more thing about the dropdown context implementation.

---

Let me try one last search with a different pattern.

---

Let me check one final thing about the UseDropdownReturn type.

---

PopoverTrigger ref handling is compatible with the implementation

The implementation is safe because:

• PopoverTrigger properly handles refs through its internal triggerRef and getTriggerProps
• DropdownTrigger's getMenuTriggerProps correctly merges refs and props before passing them to PopoverTrigger
• The ref propagation is handled through mergeRefs utility in the dropdown implementation

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Check PopoverTrigger implementation and props interface
ast-grep --pattern 'interface PopoverTriggerProps'
ast-grep --pattern 'const PopoverTrigger = $$$'

Length of output: 3299

---

Script:

#!/bin/bash
# Check useDropdownContext implementation to understand what getMenuTriggerProps returns
ast-grep --pattern 'const useDropdownContext'

# Check for PopoverTriggerProps interface
rg -A 5 'interface PopoverTriggerProps'

Length of output: 558

---

Script:

#!/bin/bash
# Check dropdown context implementation
rg -A 10 'export const useDropdownContext'

# Also check the dropdown provider to understand the context value
ast-grep --pattern 'const DropdownProvider = $$$'

Length of output: 96

---

Script:

#!/bin/bash
# Search for dropdown context in a different way
rg -A 10 'createContext.*Dropdown'

# Also check the dropdown types
rg -A 5 'type DropdownContextValue'

Length of output: 596

---

Script:

#!/bin/bash
# Check the UseDropdownReturn type and getMenuTriggerProps implementation
rg -A 10 'type UseDropdownReturn'
rg -A 10 'getMenuTriggerProps'

Length of output: 6171