fix(Anchor): resolve scroll animation conflict when clicking links ra…#55814
Conversation
|
|
👁 Visual Regression Report for PR #55814 Passed ✅
🎊 Congrats! No visual-regression diff found. 
|
|
|
Summary of ChangesHello @tuzixiangs, 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 resolves an issue in 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
|
📝 WalkthroughSummary by CodeRabbit发行说明
✏️ Tip: You can customize this high-level summary in your review settings. Walkthrough为平滑滚动引入可取消的动画机制： Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Anchor
participant ScrollUtil as scrollTo
participant RAF as requestAnimationFrame
User->>Anchor: 点击链接 A
Anchor->>ScrollUtil: scrollTo(targetA)
Note right of ScrollUtil: 创建并保存 rafId，开始帧循环
ScrollUtil-->>Anchor: 返回取消函数 cancelA
Anchor->>Anchor: 保存 cancelA 到 scrollRequestId.current
Note over Anchor: 滚动进行中...
User->>Anchor: 快速点击链接 B
Anchor->>Anchor: 调用 scrollRequestId.current() （取消 cancelA）
Note right of ScrollUtil: 使用保存的 rafId 取消 RAF 回调
Anchor->>ScrollUtil: scrollTo(targetB)
Note right of ScrollUtil: 保存新 rafId 并开始新帧循环
ScrollUtil-->>Anchor: 返回取消函数 cancelB
Anchor->>Anchor: 更新 scrollRequestId.current = cancelB
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 分钟 需要额外关注：
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 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)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
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 |
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/anchor/Anchor.tsx (1)
295-300: 组件卸载时需要取消进行中的动画。当前代码缺少在组件卸载时取消正在进行的滚动动画的清理逻辑。如果组件在动画进行期间卸载，可能会导致尝试操作已销毁的 DOM 元素，引发错误。
建议添加清理 Effect：
React.useEffect(() => { return () => { // Cancel any ongoing scroll animation on unmount scrollRequestId.current?.(); }; }, []);可以将此 Effect 添加在其他 useEffect 调用附近（例如第 367-374 行之后）。
🧹 Nitpick comments (1)
components/_util/scrollTo.ts (1)
21-22: 建议明确 rafId 的初始类型。
rafId声明为number类型但未初始化。虽然 TypeScript 允许这样做，但更明确的类型声明会提高代码可读性。可以考虑以下改进：
- let rafId: number; + let rafId: number | undefined;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/_util/scrollTo.ts(2 hunks)components/anchor/Anchor.tsx(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
components/anchor/Anchor.tsx (1)
components/_util/scrollTo.ts (1)
scrollTo(15-45)
🔇 Additional comments (2)
components/_util/scrollTo.ts (2)
23-39: 代码逻辑正确！动画帧循环正确地更新了
rafId，确保取消函数始终引用最新的帧 ID。当动画被中断时不调用callback是符合预期的行为。
40-44: 取消机制实现正确！通过闭包捕获
rafId引用，返回的清理函数能够正确取消最新的动画帧。这个实现有效解决了动画冲突问题。
More templates
commit: |
Bundle ReportChanges will increase total bundle size by 200 bytes (0.01%) ⬆️. This is within the configured threshold ✅ Detailed changes
Affected Assets, Files, and Routes:view changes for bundle: antd.min-array-pushAssets Changed:
|
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #55814 +/- ##
===========================================
- Coverage 100.00% 99.99% -0.01%
===========================================
Files 803 803
Lines 14804 14809 +5
Branches 3911 3912 +1
===========================================
+ Hits 14804 14808 +4
- Misses 0 1 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
test 覆盖率掉了, 需要看一下 |
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
components/_util/__tests__/scrollTo.test.ts (2)
73-85: 验证滚动动画是否真正被取消。当前测试仅验证
scrollToSpy被调用过，但没有验证取消后滚动是否真的停止了。应该验证最终滚动位置没有达到目标位置 1000。建议应用此修改：
const cancel = scrollTo(1000); jest.advanceTimersByTime(50); cancel(); await waitFakeTimer(); expect(scrollToSpy).toHaveBeenCalled(); + // Verify that scrolling did not reach the target position + expect(window.pageYOffset).toBeLessThan(1000); scrollToSpy.mockRestore();
99-113: 考虑增强断言以验证第一个动画被取消。测试验证了第二个滚动完成到 2000，但可以额外验证第一个滚动没有达到其目标位置 1000。另外，Line 111 的
cancel2()调用是在动画完成后，虽然无害但不必要。建议应用此修改：
const cancel1 = scrollTo(1000); jest.advanceTimersByTime(50); + const positionAfterCancel = window.pageYOffset; + expect(positionAfterCancel).toBeLessThan(1000); cancel1(); const cancel2 = scrollTo(2000); await waitFakeTimer(); expect(window.pageYOffset).toBe(2000); - cancel2(); scrollToSpy.mockRestore();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
components/_util/__tests__/scrollTo.test.ts(1 hunks)components/anchor/__tests__/Anchor.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
components/anchor/__tests__/Anchor.test.tsx (1)
tests/utils.tsx (1)
waitFakeTimer(77-89)
components/_util/__tests__/scrollTo.test.ts (2)
components/_util/scrollTo.ts (1)
scrollTo(15-45)tests/utils.tsx (1)
waitFakeTimer(77-89)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (16)
- GitHub Check: size
- GitHub Check: build preview
- GitHub Check: test lib/es module (es, 1/2)
- GitHub Check: test lib/es module (lib, 1/2)
- GitHub Check: test lib/es module (es, 2/2)
- GitHub Check: test-react-latest (dom, 2/2)
- GitHub Check: build
- GitHub Check: test lib/es module (lib, 2/2)
- GitHub Check: test-react-latest (dom, 1/2)
- GitHub Check: test-node
- GitHub Check: test-react-legacy (18, 1/2)
- GitHub Check: lint
- GitHub Check: test-react-legacy (18, 2/2)
- GitHub Check: build
- GitHub Check: visual-diff snapshot (2/2)
- GitHub Check: visual-diff snapshot (1/2)
🔇 Additional comments (2)
components/_util/__tests__/scrollTo.test.ts (2)
87-97: LGTM!此测试正确验证了取消滚动后回调函数不会被调用。逻辑清晰，断言准确。
115-120: LGTM!此测试正确验证了取消函数可以多次调用而不抛出错误，并且返回
undefined。这是良好的防御性编程验证。
Co-authored-by: afc163 <afc163@gmail.com> Signed-off-by: 遇见同学 <1875694521@qq.com>
|
感谢 @meet-student 和 @afc163 的帮助!学到了很多 🙏 |
|
🎉 Thank you for your contribution! If you have not yet joined our DingTalk community group, please feel free to join us (when joining, please provide the link to this PR). 🎉 感谢您的贡献！如果您还没有加入钉钉社区群，请扫描下方二维码加入我们（加群时请提供此 PR 链接）。 
|
English Template / 英文模板
🤔 这个变动的性质是？
🔗 相关 Issue
Fixes #55813
💡 需求背景和解决方案
问题背景: 目前
Anchor组件在快速点击不同链接时，由于没有取消上一次的滚动动画，导致多个 raf 循环同时运行，引发页面滚动抖动。解决方案:
components/_util/scrollTo.ts:2.
components/anchor/Anchor.tsx:修复后效果:
Jietu20251122-153658-HD.mp4
📝 更新日志