fix: Resolve DOM residue issue on activate #525
Conversation
|
Someone 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 @cactuser-Lu, 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! This pull request implements a fix to prevent "DOM residue" issues in the Dialog component, specifically when a dialog is deactivated. It introduces a delayed closing mechanism using a timeout to ensure that the Highlights
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough在对话框关闭与内容渲染中分别加入延迟关闭计时与强制重挂载控制： Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 分钟
Possibly related PRs
Suggested reviewers
诗篇
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 aims to fix a DOM residue issue by introducing a fallback timer to ensure the dialog closes properly. The approach is generally sound, but I have a few suggestions for improvement. The closing logic has a minor flaw that introduces an unnecessary delay when animations are disabled, for which I've provided a corrected code snippet. Additionally, a magic number is used for the timeout duration, and I recommend extracting it into a named constant for better readability. Adding a test case to cover this specific scenario would also be valuable to prevent future regressions.
src/Dialog/index.tsx
Outdated
| if (hasMotion && !inMotion) { | ||
| doClose(); | ||
| } else { | ||
| closeTimerRef.current = setTimeout(() => { | ||
| if (!visible && animatedVisible) { | ||
| doClose(); | ||
| } | ||
| }, 500); | ||
| } |
There was a problem hiding this comment.
The current logic introduces an unnecessary 500ms delay for closing the dialog when motion is disabled (hasMotion is false). If there's no motion, doClose() should be called immediately. The condition should be updated to close the dialog if motion is disabled OR if it's not currently in motion.
| if (hasMotion && !inMotion) { | |
| doClose(); | |
| } else { | |
| closeTimerRef.current = setTimeout(() => { | |
| if (!visible && animatedVisible) { | |
| doClose(); | |
| } | |
| }, 500); | |
| } | |
| if (!hasMotion || !inMotion) { | |
| doClose(); | |
| } else { | |
| closeTimerRef.current = setTimeout(() => { | |
| if (!visible && animatedVisible) { | |
| doClose(); | |
| } | |
| }, 500); | |
| } |
src/Dialog/index.tsx
Outdated
| if (!visible && animatedVisible) { | ||
| doClose(); | ||
| } | ||
| }, 500); |
There was a problem hiding this comment.
The timeout duration 500 is a magic number. It's better to define it as a constant with a descriptive name (e.g., const DIALOG_CLOSE_FALLBACK_TIMEOUT = 500;) at the top of the component. This improves readability and makes the purpose of the timeout clearer and easier to modify in the future.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
src/Dialog/Content/index.tsx (1)
38-48: 优化逻辑避免初次渲染时的不必要重挂载，并澄清注释说明。当前实现存在以下问题：
初次渲染时的额外重挂载：当对话框初始状态为隐藏（
visible=false）时，prevVisibleRef.current初始化为false，useEffect 在首次渲染时会触发条件!visible && !prevVisibleRef.current，导致motionKey递增并强制 CSSMotion 重新挂载。这在普通场景下是不必要的性能开销。注释不够清晰：注释说"when visible changes from false to false"，但这个表述容易引起混淆。实际逻辑是"当 visible 在当前和上一次渲染/激活周期中都为 false 时触发"。建议更明确地说明这是为了处理 React.Activity 重新激活时的场景。
建议优化逻辑以避免初次渲染时的不必要重挂载：
// Force remount CSSMotion when visible changes from false to false // This handles React.Activity scenarios where component state is preserved const [motionKey, setMotionKey] = React.useState(0); const prevVisibleRef = useRef(visible); + const isMountedRef = useRef(false); React.useEffect(() => { - if (!visible && !prevVisibleRef.current) { + // Only trigger remount after initial mount, when visible is false in consecutive cycles + if (isMountedRef.current && !visible && !prevVisibleRef.current) { setMotionKey(k => k + 1); } + isMountedRef.current = true; prevVisibleRef.current = visible; }, [visible]);或者更新注释以更清晰地说明预期行为：
- // Force remount CSSMotion when visible changes from false to false - // This handles React.Activity scenarios where component state is preserved + // Force remount CSSMotion when visible remains false across render/activation cycles + // This handles React.Activity reactivation scenarios where component state is preserved + // but DOM may be in an inconsistent state, causing residual elements
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Dialog/Content/index.tsx(2 hunks)
🔇 Additional comments (1)
src/Dialog/Content/index.tsx (1)
78-78: Verify implementation impact of key-based remounting on Dialog animation cycles.Using
key={motionKey}to force CSSMotion remount is a standard React pattern for clearing component state. WhenmotionKeyincrements, the old CSSMotion instance unmounts and a new one mounts, clearing any lingering animation state.However, this needs verification to ensure:
- No animation anomalies occur during rapid dialog open/close sequences
- No performance degradation in scenarios without React.Activity
- The motionKey increment logic properly aligns with visibility state changes
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #525 +/- ##
==========================================
+ Coverage 98.46% 98.50% +0.04%
==========================================
Files 8 8
Lines 195 201 +6
Branches 68 69 +1
==========================================
+ Hits 192 198 +6
Misses 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
fix ant-design/ant-design#55970
Summary by CodeRabbit
Bug修复
性能改进
行为改进
✏️ Tip: You can customize this high-level summary in your review settings.