fix(motion): attach motionRef to correct DOM element for CSSMotion lifecycle#591
fix(motion): attach motionRef to correct DOM element for CSSMotion lifecycle#591JeremiahGeronimo wants to merge 2 commits intoreact-component:masterfrom
Conversation
|
@JeremiahGeronimo is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
Walkthrough对 Changes
代码审查工作量估计🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates DrawerPopup.tsx to improve ref management by introducing a callback that merges wrapperRef and motionRef using the fillRef utility. This change replaces the direct passing of motionRef to DrawerPanel. The review feedback suggests renaming the new callback to mergeRefs to better reflect its functionality of combining multiple refs, which enhances code clarity.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/DrawerPopup.tsx (1)
307-313: 建议让组合后的ref保持稳定。Line 355 每次渲染都会重新执行
fillWrapperRef(motionRef)并创建新的 callback ref。React 会先用null清理旧 ref，再把同一节点设置给新 ref；这里又把这两个值继续转发给motionRef。拖拽 resize 会频繁重渲染，这会让 CSSMotion 的节点跟踪比必要的更脆弱。建议保留一个稳定的 wrapper ref callback，并通过中间ref保存最新的motionRef；同时把node类型改成HTMLDivElement | null会更符合 React 的 ref 调用约定。♻️ 可参考的调整
+ const motionWrapperRef = React.useRef<React.Ref<HTMLDivElement>>(); + - const fillWrapperRef = React.useCallback( - (motionRef: React.Ref<HTMLDivElement>) => (node: HTMLDivElement) => { - wrapperRef.current = node; - fillRef(motionRef, node); - }, - [], - ); + const fillWrapperRef = useEvent((node: HTMLDivElement | null) => { + wrapperRef.current = node; + fillRef(motionWrapperRef.current, node); + });{({ className: motionClassName, style: motionStyle }, motionRef) => { + motionWrapperRef.current = motionRef; const content = ( <DrawerPanel id={id} prefixCls={prefixCls} className={clsx(className, drawerClassNames?.section)} style={{ ...style, ...styles?.section }} {...pickAttrs(props, { aria: true })} {...eventHandlers} > {children} </DrawerPanel> ); return ( <div - ref={fillWrapperRef(motionRef)} + ref={fillWrapperRef}Also applies to: 355-355
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/DrawerPopup.tsx` around lines 307 - 313, The current fillWrapperRef recreates a callback ref on each call which causes React to clear and re-set refs; make the callback stable by storing the latest motionRef in a ref and returning a single stable node callback. Concretely: add a ref like motionRefLatest = useRef<React.Ref<HTMLDivElement> | null>(null), create a stable node callback (e.g., stableWrapperNodeRef = useCallback((node: HTMLDivElement | null) => { wrapperRef.current = node; fillRef(motionRefLatest.current, node); }, [])), and change fillWrapperRef to set motionRefLatest.current = motionRef and then return stableWrapperNodeRef (ensure node param type is HTMLDivElement | null). Update references to fillWrapperRef, wrapperRef, and fillRef accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/DrawerPopup.tsx`:
- Around line 307-313: The current fillWrapperRef recreates a callback ref on
each call which causes React to clear and re-set refs; make the callback stable
by storing the latest motionRef in a ref and returning a single stable node
callback. Concretely: add a ref like motionRefLatest =
useRef<React.Ref<HTMLDivElement> | null>(null), create a stable node callback
(e.g., stableWrapperNodeRef = useCallback((node: HTMLDivElement | null) => {
wrapperRef.current = node; fillRef(motionRefLatest.current, node); }, [])), and
change fillWrapperRef to set motionRefLatest.current = motionRef and then return
stableWrapperNodeRef (ensure node param type is HTMLDivElement | null). Update
references to fillWrapperRef, wrapperRef, and fillRef accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0e6dbd91-aa6d-4880-a844-95f2dcadbeda
📒 Files selected for processing (1)
src/DrawerPopup.tsx
1 participant
Summary
After the resize feature was added,
motionRefwas passed toDrawerPanelwhile motion classes/styles were applied to the content-wrapper element. This mismatch causedCSSMotionto lose track of the animated DOM node, breaking lifecycle callbacks (onLeaveEnd,onEnterEnd, etc.).The fix binds
motionRefto the content-wrapper — the same element that receives motion classes — by merging it with wrapperRef via a callback ref (fillWrapperRef). The stale ref passthrough toDrawerPanelhas been removed.What changed
fillWrapperRefcallback that mergeswrapperRef(used by resize/drag) andmotionRef(used byCSSMotion) on the content-wrapper divcontainerRef={motionRef}fromDrawerPanel,which was the source of the ref mismatchSummary by CodeRabbit
发布说明