Fix permission guard to forward ref#2053
Conversation
|
There was a problem hiding this comment.
Pull request overview
Updates PermissionGuard to support ref forwarding (for “asChild”/Radix-style composition) by switching it from a simple children passthrough to a forwardRef component built on Radix Slot.
Changes:
- Added
@radix-ui/react-slotand wrapped rendered output in<Slot>to forward refs. - Converted
PermissionGuardtoReact.forwardRefand setdisplayName.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| >(({ children, ...rest }, ref) => { | ||
| return ( | ||
| <Slot ref={ref} {...rest}> | ||
| {children} | ||
| </Slot> |
There was a problem hiding this comment.
...rest here includes permission, tooltipClassName, and tooltipLocation from PermissionGuardProps, and spreading them onto Slot will forward them to the child (often down to a DOM element). That can produce React "unknown prop" warnings and potentially leak permission into the rendered markup. Suggestion: don’t spread the guard-only props onto Slot (explicitly pick/omit these props before spreading, or remove the spread entirely if no passthrough props are intended).
| return ( | ||
| <Slot ref={ref} {...rest}> | ||
| {children} | ||
| </Slot> | ||
| ); |
There was a problem hiding this comment.
@radix-ui/react-slot expects a single valid React element child (it clones/merges props & refs). With children typed as React.ReactNode, callers can legally pass strings/fragments/arrays, which will throw at runtime when Slot tries to operate on it. Suggestion: narrow children to React.ReactElement (or React.ReactElement | null) or add a runtime React.isValidElement guard that falls back to returning children unmodified when it isn’t a single element.
| ); | ||
| }); | ||
|
|
||
| PermissionGuard.displayName = 'PermissionGuard'; |
There was a problem hiding this comment.
The PR description still contains placeholders (e.g., Fixes #<issue_number>) and the testing checklist isn’t filled out. Please update the PR metadata so reviewers can trace the change to an issue and understand what was validated.
Fixes OPS-3833