Fix ref being undefined for measure functions#113
Fix ref being undefined for measure functions#113Choco-milk-for-u wants to merge 2 commits intoroninoss:mainfrom
Conversation
I decided to copy hook's behaviour by not calling ref callback and not passing updated values / ref itself.
I decided to add the line `if (typeof ref === 'function') return;` in each onPress function because it mimics the hook’s behavior, basicaly the same as when augmentedRef.current is null.
|
@Choco-milk-for-u is attempting to deploy a commit to the Ronin Technologies Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughRefactors ref augmentation across seven component packages, replacing the Changes
Sequence DiagramssequenceDiagram
participant User as User Code
participant Trigger as Trigger Component
participant augmentRef as augmentRef Utility
participant Ref as Forwarded Ref
User->>Trigger: onPress triggered
Trigger->>Trigger: check if ref is function<br/>(skip if true)
Trigger->>Trigger: measure via ref.current
Trigger->>Trigger: call methods.open()
activate Trigger
Trigger->>Trigger: update state & position
deactivate Trigger
Note over User,Ref: Ref augmentation during render:
Trigger->>augmentRef: augmentRef(userRef,<br/>componentRef, methods)
augmentRef->>Ref: attach methods to ref
augmentRef->>Ref: link userRef.current<br/>to componentRef
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/context-menu/src/context-menu.tsx (1)
136-188: Keep trigger measurement working for callback refsThe same regression appears here: returning when
typeof ref === 'function'prevents long-pressing from ever opening when consumers provide a callback ref, andref?.currentis stillundefinedif no object ref was passed. As a result,pressPositionnever updates and the menu cannot render. Please restore an internal ref (replacing the functionality previously handled byuseAugmentedRef) and drive measurement/methods through it so all ref shapes continue to work.@@ - if (relativeTo === 'trigger') { - if (typeof ref === 'function') return; - ref?.current?.measure((_x, _y, width, height, pageX, pageY) => { - setPressPosition({ width, pageX, pageY: pageY, height }); - }); - } + if (relativeTo === 'trigger') { + measureTrigger(); + } @@ - const methods = { - open: () => { - onOpenChange(true); - (ref as React.RefObject<TriggerRef>)?.current?.measure( - (_x, _y, width, height, pageX, pageY) => { - setPressPosition({ width, pageX, pageY: pageY, height }); - } - ); - }, - close: () => { - setPressPosition(null); - onOpenChange(false); - }, - }; + const triggerRef = React.useRef<TriggerRef>(null); + + const measureTrigger = React.useCallback(() => { + triggerRef.current?.measure((_x, _y, width, height, pageX, pageY) => { + setPressPosition({ width, pageX, pageY, height }); + }); + }, [setPressPosition]); + + const methods = React.useMemo( + () => ({ + open: () => { + onOpenChange(true); + measureTrigger(); + }, + close: () => { + setPressPosition(null); + onOpenChange(false); + }, + }), + [measureTrigger, onOpenChange, setPressPosition] + ); @@ - <Component - ref={(self) => augmentRef(ref, self, methods)} + <Component + ref={(self) => { + triggerRef.current = self; + augmentRef(ref, self, methods); + }}packages/select/src/select.tsx (1)
125-158: Avoid breaking select trigger when no mutable ref is providedHere too the trigger now refuses to open whenever the forwarded ref is a callback (or omitted), because we return before toggling
open, and measurement never runs sotriggerPositionstaysnull. This regresses the primary interaction for a large set of consumers. Reintroduce an internal ref that always points at the host element and route measurement plus the exposed methods through it instead of relying on the forwarded ref to be aRefObject.@@ - function onPress(ev: GestureResponderEvent) { - if (disabled) return; - if (typeof ref === 'function') return; - ref?.current?.measure((_x, _y, width, height, pageX, pageY) => { - setTriggerPosition({ width, pageX, pageY: pageY, height }); - }); - onOpenChange(!open); - onPressProp?.(ev); - } - - const methods = { - open: () => { - onOpenChange(true); - (ref as React.RefObject<TriggerRef>)?.current?.measure((_x, _y, width, height, pageX, pageY) => { - setTriggerPosition({ width, pageX, pageY: pageY, height }); - }); - }, - close: () => { - setTriggerPosition(null); - onOpenChange(false); - }, - }; + const triggerRef = React.useRef<TriggerRef>(null); + + const measureTrigger = React.useCallback(() => { + triggerRef.current?.measure((_x, _y, width, height, pageX, pageY) => { + setTriggerPosition({ width, pageX, pageY, height }); + }); + }, [setTriggerPosition]); + + function onPress(ev: GestureResponderEvent) { + if (disabled) return; + measureTrigger(); + onOpenChange(!open); + onPressProp?.(ev); + } + + const methods = React.useMemo( + () => ({ + open: () => { + onOpenChange(true); + measureTrigger(); + }, + close: () => { + setTriggerPosition(null); + onOpenChange(false); + }, + }), + [measureTrigger, onOpenChange, setTriggerPosition] + ); @@ - <Component - ref={(self) => augmentRef(ref, self, methods)} + <Component + ref={(self) => { + triggerRef.current = self; + augmentRef(ref, self, methods); + }}packages/hover-card/src/hover-card.tsx (1)
95-130: Do not regress hover-card trigger opening/measurementIdentical regression here: the guard on
typeof ref === 'function'stops the card from opening when consumers supply a callback ref, andref?.currentremains empty otherwise, sotriggerPositionnever updates. Please add a local ref that always captures the host node and reuse it for measurement and the imperative methods, matching the pattern we relied on previously.@@ - function onPress(ev: GestureResponderEvent) { - if (disabled) return; - if (typeof ref === 'function') return; - ref?.current?.measure((_x, _y, width, height, pageX, pageY) => { - setTriggerPosition({ width, pageX, pageY: pageY, height }); - }); - - onOpenChange(!open); - onPressProp?.(ev); - } - - const methods = { - open: () => { - onOpenChange(true); - (ref as React.RefObject<TriggerRef>)?.current?.measure( - (_x, _y, width, height, pageX, pageY) => { - setTriggerPosition({ width, pageX, pageY: pageY, height }); - } - ); - }, - close: () => { - setTriggerPosition(null); - onOpenChange(false); - }, - }; + const triggerRef = React.useRef<TriggerRef>(null); + + const measureTrigger = React.useCallback(() => { + triggerRef.current?.measure((_x, _y, width, height, pageX, pageY) => { + setTriggerPosition({ width, pageX, pageY, height }); + }); + }, [setTriggerPosition]); + + function onPress(ev: GestureResponderEvent) { + if (disabled) return; + measureTrigger(); + onOpenChange(!open); + onPressProp?.(ev); + } + + const methods = React.useMemo( + () => ({ + open: () => { + onOpenChange(true); + measureTrigger(); + }, + close: () => { + setTriggerPosition(null); + onOpenChange(false); + }, + }), + [measureTrigger, onOpenChange, setTriggerPosition] + ); @@ - <Component - ref={(self) => augmentRef(ref, self, methods)} + <Component + ref={(self) => { + triggerRef.current = self; + augmentRef(ref, self, methods); + }}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (12)
packages/context-menu/src/context-menu.tsx(3 hunks)packages/dropdown-menu/src/dropdown-menu.tsx(2 hunks)packages/hover-card/package.json(1 hunks)packages/hover-card/src/hover-card.tsx(2 hunks)packages/menubar/src/menubar.tsx(3 hunks)packages/popover/package.json(1 hunks)packages/popover/src/popover.tsx(2 hunks)packages/select/package.json(1 hunks)packages/select/src/select.tsx(2 hunks)packages/tooltip/package.json(1 hunks)packages/tooltip/src/tooltip.tsx(2 hunks)packages/utils/src/index.ts(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (9)
packages/popover/package.json (1)
packages/popover/src/popover.web.tsx (1)
onOpenChange(23-38)
packages/popover/src/popover.tsx (3)
packages/popover/src/popover.web.tsx (2)
open(60-62)augmentedRef(71-77)packages/popover/src/types.ts (1)
TriggerRef(58-58)packages/utils/src/index.ts (1)
augmentRef(72-72)
packages/utils/src/index.ts (6)
packages/menubar/src/menubar.web.tsx (1)
augmentedRef(87-92)packages/tooltip/src/tooltip.web.tsx (1)
augmentedRef(97-103)packages/checkbox/src/checkbox.web.tsx (2)
augmentedRef(90-95)augmentedRef(27-33)packages/collapsible/src/collapsible.web.tsx (4)
augmentedRef(139-144)augmentedRef(99-110)augmentedRef(92-97)augmentedRef(42-51)packages/navigation-menu/src/navigation-menu.web.tsx (1)
augmentedRef(71-76)packages/select/src/select.web.tsx (1)
augmentedRef(116-122)
packages/dropdown-menu/src/dropdown-menu.tsx (2)
packages/dropdown-menu/src/types.ts (1)
TriggerRef(125-125)packages/utils/src/index.ts (1)
augmentRef(72-72)
packages/context-menu/src/context-menu.tsx (3)
packages/context-menu/src/types.ts (1)
TriggerRef(140-140)packages/slot/src/slot.tsx (1)
Pressable(105-105)packages/utils/src/index.ts (1)
augmentRef(72-72)
packages/menubar/src/menubar.tsx (2)
packages/utils/src/index.ts (1)
augmentRef(72-72)packages/menubar/src/menubar.web.tsx (1)
augmentedRef(87-92)
packages/select/src/select.tsx (2)
packages/select/src/types.ts (1)
TriggerRef(145-145)packages/utils/src/index.ts (1)
augmentRef(72-72)
packages/hover-card/src/hover-card.tsx (3)
packages/hover-card/src/hover-card.web.tsx (1)
open(63-65)packages/hover-card/src/types.ts (1)
TriggerRef(69-69)packages/utils/src/index.ts (1)
augmentRef(72-72)
packages/tooltip/src/tooltip.tsx (2)
packages/tooltip/src/tooltip.web.tsx (1)
open(80-82)packages/utils/src/index.ts (1)
augmentRef(72-72)
🔇 Additional comments (9)
packages/tooltip/package.json (1)
37-37: LGTM!Dependency addition aligns with the ref augmentation refactor.
packages/hover-card/package.json (1)
39-39: LGTM!Dependency addition is consistent with the refactor pattern.
packages/select/package.json (1)
37-37: LGTM!Dependency addition is consistent with the refactor.
packages/popover/package.json (1)
37-37: LGTM!Dependency addition follows the established pattern.
packages/menubar/src/menubar.tsx (1)
132-135: Measurement guard is correctly implemented.The function ref check prevents calling
measureon function refs, which would fail. This pattern is consistent with the other components.packages/tooltip/src/tooltip.tsx (2)
95-98: Function ref guard correctly implemented.The guard prevents measurement failures when a function ref is provided.
120-120: Ref augmentation correctly implemented.The
augmentRefcall properly merges the local methods with the forwarded ref.packages/dropdown-menu/src/dropdown-menu.tsx (2)
114-121: Function ref guard and measurement logic correctly implemented.The guard and toggle logic work as expected.
141-141: Ref augmentation correctly implemented.The augmentRef integration properly exposes imperative methods via the forwarded ref.
| const methods = { | ||
| open: () => { | ||
| onOpenChange(true); | ||
| (ref as React.RefObject<TriggerRef>)?.current?.measure( | ||
| (_x, _y, width, height, pageX, pageY) => { | ||
| setTriggerPosition({ width, pageX, pageY: pageY, height }); | ||
| } | ||
| ); | ||
| }, | ||
| close: () => { | ||
| setTriggerPosition(null); | ||
| onOpenChange(false); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Inconsistent function ref handling in methods.open.
The same issue exists here as in tooltip.tsx: methods.open (line 126) calls measure without checking if ref is a function, while onPress (line 114) includes this guard.
Although augmentRef won't attach methods to function refs, the inconsistency makes the code harder to maintain.
const methods = {
open: () => {
+ if (typeof ref === 'function') return;
onOpenChange(true);
(ref as React.RefObject<TriggerRef>)?.current?.measure(
(_x, _y, width, height, pageX, pageY) => {
setTriggerPosition({ width, pageX, pageY: pageY, height });
}
);
},
close: () => {
setTriggerPosition(null);
onOpenChange(false);
},
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const methods = { | |
| open: () => { | |
| onOpenChange(true); | |
| (ref as React.RefObject<TriggerRef>)?.current?.measure( | |
| (_x, _y, width, height, pageX, pageY) => { | |
| setTriggerPosition({ width, pageX, pageY: pageY, height }); | |
| } | |
| ); | |
| }, | |
| close: () => { | |
| setTriggerPosition(null); | |
| onOpenChange(false); | |
| }, | |
| }; | |
| const methods = { | |
| open: () => { | |
| if (typeof ref === 'function') return; | |
| onOpenChange(true); | |
| (ref as React.RefObject<TriggerRef>)?.current?.measure( | |
| (_x, _y, width, height, pageX, pageY) => { | |
| setTriggerPosition({ width, pageX, pageY: pageY, height }); | |
| } | |
| ); | |
| }, | |
| close: () => { | |
| setTriggerPosition(null); | |
| onOpenChange(false); | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In packages/dropdown-menu/src/dropdown-menu.tsx around lines 123 to 136,
methods.open calls measure on ref without guarding against function refs
(inconsistent with onPress at ~line 114); update methods.open to check if ref is
a function before attempting to access ref.current (e.g. if (typeof ref ===
'function') return; or guard and only call (ref as
React.RefObject<TriggerRef>)?.current?.measure(...) when ref is not a function)
so function refs are skipped and behavior matches onPress.
| return ( | ||
| <Component | ||
| ref={triggerRef} | ||
| ref={(self) => augmentRef(ref, self)} // copy hook behaviour if not needed then pass 'ref' instead |
There was a problem hiding this comment.
Missing methods parameter for ref augmentation.
Unlike the other components in this PR (tooltip, dropdown-menu), this component calls augmentRef(ref, self) without providing the methods parameter containing open/close functions. This means:
- The augmented ref won't expose imperative
open/closemethods to consumers - The behavior is inconsistent with the pattern established in tooltip.tsx (lines 104-115) and dropdown-menu.tsx (lines 123-136)
If imperative control via ref is not needed for Menubar, the comment should be more explicit, or you should simply pass ref directly. Otherwise, add the methods object:
+ const methods = {
+ open: () => {
+ onValueChange(menuValue);
+ if (typeof ref === 'function') return;
+ ref?.current?.measure((_x, _y, width, height, pageX, pageY) => {
+ setTriggerPosition({ width, pageX, pageY, height });
+ });
+ },
+ close: () => {
+ setTriggerPosition(null);
+ onValueChange(undefined);
+ },
+ };
+
const Component = asChild ? Slot.Pressable : Pressable;
return (
<Component
- ref={(self) => augmentRef(ref, self)} // copy hook behaviour if not needed then pass 'ref' instead
+ ref={(self) => augmentRef(ref, self, methods)}Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/menubar/src/menubar.tsx around line 144, the call ref={(self) =>
augmentRef(ref, self)} omits the methods parameter (open/close) so the augmented
ref does not expose imperative control and is inconsistent with tooltip.tsx and
dropdown-menu.tsx; either pass the methods object (e.g., augmentRef(ref, self, {
open: () => setOpen(true), close: () => setOpen(false) }) matching the
component's open/close functions) so consumers get imperative open/close, or if
imperative control is intentionally not supported, replace the augmentRef call
with ref directly and update the inline comment to explicitly state that no
imperative methods are exposed.
| function onPress(ev: GestureResponderEvent) { | ||
| if (disabled) return; | ||
| augmentedRef.current?.measure((_x, _y, width, height, pageX, pageY) => { | ||
| if (typeof ref === 'function') return; | ||
| ref?.current?.measure((_x, _y, width, height, pageX, pageY) => { | ||
| setTriggerPosition({ width, pageX, pageY: pageY, height }); | ||
| }); | ||
| onOpenChange(!open); | ||
| onPressProp?.(ev); | ||
| } | ||
|
|
||
| const methods = { | ||
| open: () => { | ||
| onOpenChange(true); | ||
| (ref as React.RefObject<TriggerRef>)?.current?.measure( | ||
| (_x, _y, width, height, pageX, pageY) => { | ||
| setTriggerPosition({ width, pageX, pageY: pageY, height }); | ||
| } | ||
| ); | ||
| }, | ||
| close: () => { | ||
| setTriggerPosition(null); | ||
| onOpenChange(false); | ||
| }, | ||
| }; | ||
|
|
||
| const Component = asChild ? Slot.Pressable : Pressable; | ||
| return ( | ||
| <Component | ||
| ref={augmentedRef} | ||
| ref={(self) => augmentRef(ref, self, methods)} | ||
| aria-disabled={disabled ?? undefined} |
There was a problem hiding this comment.
Restore trigger measurement regardless of forwarded ref shape
Early-returning when ref is a function (or absent) skips both measurement and onOpenChange, so any consumer using a callback ref—or no ref at all—can no longer open the popover. Because we no longer keep an internal ref (the old useAugmentedRef handled this), ref?.current is undefined in those cases, so the trigger position never updates and the portal content stays null. This is a functional regression that completely breaks the component outside of the narrow “object ref” scenario. Please reinstate a local ref that is always wired to the host node and drive both measurement and the exposed methods through it instead of bailing on callback refs.
@@
- function onPress(ev: GestureResponderEvent) {
- if (disabled) return;
- if (typeof ref === 'function') return;
- ref?.current?.measure((_x, _y, width, height, pageX, pageY) => {
- setTriggerPosition({ width, pageX, pageY: pageY, height });
- });
- onOpenChange(!open);
- onPressProp?.(ev);
- }
-
- const methods = {
- open: () => {
- onOpenChange(true);
- (ref as React.RefObject<TriggerRef>)?.current?.measure(
- (_x, _y, width, height, pageX, pageY) => {
- setTriggerPosition({ width, pageX, pageY: pageY, height });
- }
- );
- },
- close: () => {
- setTriggerPosition(null);
- onOpenChange(false);
- },
- };
+ const triggerRef = React.useRef<TriggerRef>(null);
+
+ const measureTrigger = React.useCallback(() => {
+ triggerRef.current?.measure((_x, _y, width, height, pageX, pageY) => {
+ setTriggerPosition({ width, pageX, pageY, height });
+ });
+ }, [setTriggerPosition]);
+
+ function onPress(ev: GestureResponderEvent) {
+ if (disabled) return;
+ measureTrigger();
+ onOpenChange(!open);
+ onPressProp?.(ev);
+ }
+
+ const methods = React.useMemo(
+ () => ({
+ open: () => {
+ onOpenChange(true);
+ measureTrigger();
+ },
+ close: () => {
+ setTriggerPosition(null);
+ onOpenChange(false);
+ },
+ }),
+ [measureTrigger, onOpenChange, setTriggerPosition]
+ );
@@
- <Component
- ref={(self) => augmentRef(ref, self, methods)}
+ <Component
+ ref={(self) => {
+ triggerRef.current = self;
+ augmentRef(ref, self, methods);
+ }}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function onPress(ev: GestureResponderEvent) { | |
| if (disabled) return; | |
| augmentedRef.current?.measure((_x, _y, width, height, pageX, pageY) => { | |
| if (typeof ref === 'function') return; | |
| ref?.current?.measure((_x, _y, width, height, pageX, pageY) => { | |
| setTriggerPosition({ width, pageX, pageY: pageY, height }); | |
| }); | |
| onOpenChange(!open); | |
| onPressProp?.(ev); | |
| } | |
| const methods = { | |
| open: () => { | |
| onOpenChange(true); | |
| (ref as React.RefObject<TriggerRef>)?.current?.measure( | |
| (_x, _y, width, height, pageX, pageY) => { | |
| setTriggerPosition({ width, pageX, pageY: pageY, height }); | |
| } | |
| ); | |
| }, | |
| close: () => { | |
| setTriggerPosition(null); | |
| onOpenChange(false); | |
| }, | |
| }; | |
| const Component = asChild ? Slot.Pressable : Pressable; | |
| return ( | |
| <Component | |
| ref={augmentedRef} | |
| ref={(self) => augmentRef(ref, self, methods)} | |
| aria-disabled={disabled ?? undefined} | |
| const triggerRef = React.useRef<TriggerRef>(null); | |
| const measureTrigger = React.useCallback(() => { | |
| triggerRef.current?.measure((_x, _y, width, height, pageX, pageY) => { | |
| setTriggerPosition({ width, pageX, pageY, height }); | |
| }); | |
| }, [setTriggerPosition]); | |
| function onPress(ev: GestureResponderEvent) { | |
| if (disabled) return; | |
| measureTrigger(); | |
| onOpenChange(!open); | |
| onPressProp?.(ev); | |
| } | |
| const methods = React.useMemo( | |
| () => ({ | |
| open: () => { | |
| onOpenChange(true); | |
| measureTrigger(); | |
| }, | |
| close: () => { | |
| setTriggerPosition(null); | |
| onOpenChange(false); | |
| }, | |
| }), | |
| [measureTrigger, onOpenChange, setTriggerPosition] | |
| ); | |
| const Component = asChild ? Slot.Pressable : Pressable; | |
| return ( | |
| <Component | |
| ref={(self) => { | |
| triggerRef.current = self; | |
| augmentRef(ref, self, methods); | |
| }} | |
| aria-disabled={disabled ?? undefined} |
| const methods = { | ||
| open: () => { | ||
| onOpenChange(true); | ||
| (ref as React.RefObject<TriggerRef>)?.current?.measure((_x, _y, width, height, pageX, pageY) => { | ||
| setTriggerPosition({ width, pageX, pageY: pageY, height }); | ||
| }); | ||
| }, | ||
| close: () => { | ||
| setTriggerPosition(null); | ||
| onOpenChange(false); | ||
| }, | ||
| }; |
There was a problem hiding this comment.
Inconsistent function ref handling in methods.open.
The methods.open function (line 107) calls measure without checking if ref is a function, but the onPress handler (line 95) includes this guard. If a consumer passes a function ref and somehow calls the augmented methods, this will fail.
While augmentRef returns early for function refs (so the methods won't actually be attached), the code structure is inconsistent and confusing. Either:
- Add the same guard in
methods.open, or - Document that methods are only available for RefObject refs
const methods = {
open: () => {
+ if (typeof ref === 'function') return;
onOpenChange(true);
(ref as React.RefObject<TriggerRef>)?.current?.measure((_x, _y, width, height, pageX, pageY) => {
setTriggerPosition({ width, pageX, pageY: pageY, height });
});
},
close: () => {
setTriggerPosition(null);
onOpenChange(false);
},
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const methods = { | |
| open: () => { | |
| onOpenChange(true); | |
| (ref as React.RefObject<TriggerRef>)?.current?.measure((_x, _y, width, height, pageX, pageY) => { | |
| setTriggerPosition({ width, pageX, pageY: pageY, height }); | |
| }); | |
| }, | |
| close: () => { | |
| setTriggerPosition(null); | |
| onOpenChange(false); | |
| }, | |
| }; | |
| const methods = { | |
| open: () => { | |
| if (typeof ref === 'function') return; | |
| onOpenChange(true); | |
| (ref as React.RefObject<TriggerRef>)?.current?.measure((_x, _y, width, height, pageX, pageY) => { | |
| setTriggerPosition({ width, pageX, pageY: pageY, height }); | |
| }); | |
| }, | |
| close: () => { | |
| setTriggerPosition(null); | |
| onOpenChange(false); | |
| }, | |
| }; |
🤖 Prompt for AI Agents
In packages/tooltip/src/tooltip.tsx around lines 104 to 115, methods.open calls
ref.measure without handling the case where ref may be a function ref; add the
same guard used in the onPress handler so methods.open only attempts to call
measure when ref is an object ref with a current that exposes measure (i.e.,
check that typeof ref !== 'function' and (ref as
React.RefObject<TriggerRef>)?.current exists before calling measure), and bail
or no-op otherwise so function refs won't cause a runtime error.
| function augmentRef<T>( | ||
| userRef: React.Ref<T>, | ||
| componentRef: T, | ||
| methods?: Record<string, (...args: any[]) => any> | ||
| ) { | ||
| if (!userRef || !componentRef) return; | ||
| if (typeof userRef === 'function') return; // copy hook behaviour | ||
| Object.assign(componentRef, methods); | ||
| userRef.current = componentRef; | ||
| } |
There was a problem hiding this comment.
Review the early return logic and potential runtime issues.
Several concerns with the augmentRef implementation:
-
Line 67: Function refs are silently ignored. This means when consumers pass function refs, no augmentation occurs and they lose access to the
open/closemethods. Consider logging a warning or documenting this limitation clearly. -
Line 68:
Object.assign(componentRef, methods)is called even whenmethodsis undefined. While this works (undefined is handled gracefully), it's cleaner to guard this call:if (methods) Object.assign(componentRef, methods); -
Line 66: The early return when
!userRefmeans no augmentation happens when ref is null/undefined. However, in the component files (e.g., tooltip.tsx line 95, dropdown-menu.tsx line 114), there's a pattern wheretypeof ref === 'function'is checked before callingref?.current?.measure(). This suggests refs can be function refs, yetaugmentRefsilently skips them. This creates an inconsistency—measure calls check for function refs, but augmentation silently fails.
Consider this safer implementation:
function augmentRef<T>(
userRef: React.Ref<T>,
componentRef: T,
methods?: Record<string, (...args: any[]) => any>
) {
- if (!userRef || !componentRef) return;
- if (typeof userRef === 'function') return; // copy hook behaviour
- Object.assign(componentRef, methods);
+ if (!componentRef) return;
+ if (!userRef) return;
+ if (typeof userRef === 'function') {
+ // Function refs don't support augmentation
+ return;
+ }
+ if (methods) {
+ Object.assign(componentRef, methods);
+ }
userRef.current = componentRef;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| function augmentRef<T>( | |
| userRef: React.Ref<T>, | |
| componentRef: T, | |
| methods?: Record<string, (...args: any[]) => any> | |
| ) { | |
| if (!userRef || !componentRef) return; | |
| if (typeof userRef === 'function') return; // copy hook behaviour | |
| Object.assign(componentRef, methods); | |
| userRef.current = componentRef; | |
| } | |
| function augmentRef<T>( | |
| userRef: React.Ref<T>, | |
| componentRef: T, | |
| methods?: Record<string, (...args: any[]) => any> | |
| ) { | |
| if (!componentRef) return; | |
| if (!userRef) return; | |
| if (typeof userRef === 'function') { | |
| // Function refs don't support augmentation | |
| return; | |
| } | |
| if (methods) { | |
| Object.assign(componentRef, methods); | |
| } | |
| userRef.current = componentRef; | |
| } |
1 participant
Please see this issue
Updated a few component primitives AND the utils package.
I decided not to modify the .web components since the hook works on web, but honestly, i would just remove the hook at this point.
This should fixe all measure functions but might have a few other ones too.
Summary by CodeRabbit