fix: compose existing child refs in CSSMotion#76
fix: compose existing child refs in CSSMotion#76zombieJ merged 8 commits intoreact-component:masterfrom
Conversation
|
@QDyanbing is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! 此拉取请求修复了 Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
|
Warning Rate limit exceeded
⌛ 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. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
Walkthrough重构 genCSSMotion 的子元素 ref 注入与调用时机：仅在子元素可接收 ref 且子元素原始 ref 与内部 nodeRef 不同时，使用 Changes
Sequence Diagram(s)（无） Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 enhances the CSSMotion component's ref handling by ensuring that its internal nodeRef is always applied, even when the child component already has its own ref. It achieves this by importing and utilizing composeRef to merge existing and new refs, preventing potential ref conflicts. A new test case has been added to tests/CSSMotion.spec.tsx to validate this improved ref composition behavior, particularly for motion end scenarios. There are no review comments to address.
![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/CSSMotion.tsx (1)
249-253: 注释与实现已不一致，建议同步更新。当前实现已变为“始终注入 ref（必要时合并）”，但注释仍描述“仅在 child 无 ref 时注入”。建议改成准确描述，避免后续误判。
建议修改
- // Auto inject ref if child node not have `ref` props + // Always inject motion ref; compose with child's existing ref when present🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/CSSMotion.tsx` around lines 249 - 253, The comment above the ref-injection logic in CSSMotion.tsx is outdated: the code now always injects a ref (merging when the child already has one) but the comment says it only injects when the child has no ref; update the comment to reflect the current behavior. Edit the comment near the block that checks React.isValidElement(motionChildren) and supportRef(motionChildren) (inside the CSSMotion component) to state that the component will always inject/merge a ref into the child element when supported, mentioning that it merges with an existing ref if present.
🤖 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/CSSMotion.tsx`:
- Around line 249-253: The comment above the ref-injection logic in
CSSMotion.tsx is outdated: the code now always injects a ref (merging when the
child already has one) but the comment says it only injects when the child has
no ref; update the comment to reflect the current behavior. Edit the comment
near the block that checks React.isValidElement(motionChildren) and
supportRef(motionChildren) (inside the CSSMotion component) to state that the
component will always inject/merge a ref into the child element when supported,
mentioning that it merges with an existing ref if present.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9b66e2ce-f2c3-4313-89e5-47608fa4e37a
📒 Files selected for processing (2)
src/CSSMotion.tsxtests/CSSMotion.spec.tsx
src/CSSMotion.tsx
Outdated
|
|
||
| const canHoldRef = | ||
| React.isValidElement(motionChildren) && supportRef(motionChildren); | ||
| const originNodeRef = canHoldRef ? getNodeRef(motionChildren) : null; |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/CSSMotionList.tsx`:
- Around line 177-179: The current logic in CSSMotionList.tsx that checks
children.length to decide whether to inject ref is unreliable and can drop refs
(see children usage and nodeRef propagation); change to use an explicit boolean
capability flag (e.g., shouldAutoInjectRef or a prop like forwardRef passed in
by the caller) instead of Function.length, update the rendering branch that
currently chooses between props => children({...}, undefined as any) and (props,
ref) => children({...}, ref) to read that explicit flag, and make the same
change in CSSMotion.tsx (the existing shouldAutoInjectRef logic around line 182)
so both components accept and use the explicit marker to decide ref injection
and ensure nodeRef is forwarded correctly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cecfb9c9-a01b-46cb-b0b1-ce22ee343990
📒 Files selected for processing (2)
src/CSSMotion.tsxsrc/CSSMotionList.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/CSSMotion.tsx
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #76 +/- ##
==========================================
- Coverage 98.56% 98.09% -0.47%
==========================================
Files 11 11
Lines 417 421 +4
Branches 120 119 -1
==========================================
+ Hits 411 413 +2
- Misses 6 7 +1
- Partials 0 1 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
背景
在 React 19 场景下，
CSSMotion的子节点如果本身已经持有ref，当前实现不会再挂载内部的 motion ref。这会导致
CSSMotion在某些场景下拿不到真实 DOM，进而无法正确监听离场动画结束事件。这个问题在
@rc-component/image/antd的 Image Preview 关闭流程里会被稳定触发：关闭后节点进入 leave 状态，但不会真正卸载，最终表现为透明遮罩残留、页面无法继续点击。
相关问题：
fix: ant-design/ant-design#57431
改动
CSSMotion中合并子节点已有的ref和内部nodeRef根因说明
当前逻辑在检测到 child 已有
ref时，只保留 child 自己的ref，导致 motion 内部拿不到 DOM 节点。这样一来，
transitionend/animationend无法正确绑定到目标元素，离场动画结束后也就无法进入最终清理流程。本次修复后，child 的原始
ref语义保持不变，同时 motion 也能拿到同一个 DOM 节点。影响评估
Summary by CodeRabbit
发布说明
Bug修复
测试