fix: open trigger force scroll #304
Conversation
Walkthrough本 PR 为 DropdownMenu 组件添加了 Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant KT as KeywordTrigger
participant DM as DropdownMenu
participant RC as RC Menu
User->>KT: 触发下拉菜单
KT->>RC: 打开菜单
RC->>KT: afterOpenChange(true)
KT->>KT: 设置 opened=true
KT->>DM: 传递 opened=true
User->>KT: 键盘导航 (Down)
KT->>DM: activeIndex 变化
DM->>DM: 检查：opened=true 且 menuRef 存在
DM->>RC: scrollIntoView({block, inline})
User->>KT: 关闭菜单
RC->>KT: afterOpenChange(false)
KT->>KT: 设置 opened=false
KT->>DM: 传递 opened=false
User->>KT: 键盘导航 (Down)
KT->>DM: activeIndex 变化
DM->>DM: 检查：opened=false，跳过滚动
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 需要关注的具体区域：
Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
Summary of ChangesHello @zombieJ, 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 where the mentions dropdown component would inadvertently trigger a scroll-into-view action upon its initial opening. By implementing a new 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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #304 +/- ##
==========================================
+ Coverage 98.09% 98.48% +0.38%
==========================================
Files 8 8
Lines 263 264 +1
Branches 64 64
==========================================
+ Hits 258 260 +2
+ Misses 5 4 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 an issue with forced scrolling when the mentions dropdown is triggered. The changes introduce an opened state that is synchronized with the dropdown's visibility, and use this state to delay the scrollIntoView call until after the dropdown has finished its opening animation. This seems like a reasonable approach to prevent scrolling issues during the animation. My main feedback is on the test file tests/DropdownMenu.spec.tsx, where the test logic has become confusing and less comprehensive. I've left a comment with suggestions to improve the test's clarity and coverage.
| for (let i = 0; i < 10; i++) { | ||
| await act(async () => { | ||
| jest.advanceTimersByTime(1000); | ||
| await Promise.resolve(); | ||
| }); | ||
| } |
There was a problem hiding this comment.
This test has a few issues that could be improved:
- Confusing loop: The
forloop advancing timers seems incorrect. To wait for the dropdown to open, a singleactblock that runs all timers is sufficient and much clearer. - Misleading test name: The test is named
...navigating with keyboardbut no longer simulates keyboard navigation. It now only tests the scroll behavior on open. Consider renaming the test to something likeshould scroll into view after dropdown opens. - Reduced coverage: The part of the test that verified scrolling up with
ArrowUphas been removed. It would be beneficial to have a test that covers keyboard navigation in both directions, perhaps in a separate test case.
Here's a suggestion to simplify the timer logic:
| for (let i = 0; i < 10; i++) { | |
| await act(async () => { | |
| jest.advanceTimersByTime(1000); | |
| await Promise.resolve(); | |
| }); | |
| } | |
| // Wait for the dropdown to fully open by running all timers. | |
| // This will trigger `afterOpenChange` and any subsequent effects. | |
| await act(async () => { | |
| jest.runAllTimers(); | |
| await Promise.resolve(); | |
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
src/DropdownMenu.tsx (1)
38-38: 移除多余的空行。第 38 行的空行是多余的，建议移除以保持代码整洁。
应用此差异来移除空行：
const activeItem = menuRef.current?.findItem?.({ key: activeOption.key }); - if (activeItem) {tests/DropdownMenu.spec.tsx (2)
39-44: 定时器推进逻辑过于脆弱。使用循环推进 10 秒（10 次迭代 × 1000ms）的定时器看起来很随意，并且使测试变得脆弱。如果动画或
afterOpenChange的调用时机发生变化，这个测试可能会失败或产生误报。建议使用更明确的方式来等待下拉菜单的打开状态，例如：
- 使用
waitFor等待特定的 DOM 状态- 或者直接测试在不同
opened状态下的滚动行为，而不依赖定时器考虑以下更稳健的测试方法：
// 等待下拉菜单实际打开并完成动画 await waitFor(() => { // 检查下拉菜单是否可见或者检查特定的 DOM 状态 expect(container.querySelector('.rc-mentions-dropdown-menu')).toBeVisible(); }); // 然后验证滚动行为 expect(scrollIntoViewMock).toHaveBeenCalledWith({ block: 'nearest', inline: 'nearest', });
39-50: 增加键盘导航的测试覆盖。虽然当前测试验证了下拉菜单打开后的滚动行为，但根据测试名称，应该也测试键盘导航（例如按 Down/Up 箭头键）时的滚动行为。建议添加一个独立的测试来验证当
opened=true且activeIndex通过键盘导航改变时，scrollIntoView会被正确调用。示例测试代码：
it('should scroll when activeIndex changes via keyboard navigation', async () => { const { container } = render( <UnstableContext.Provider value={{ open: true }}> <Mentions defaultValue="@" options={generateOptions} /> </UnstableContext.Provider>, ); const scrollIntoViewMock = jest .spyOn(HTMLElement.prototype, 'scrollIntoView') .mockImplementation(jest.fn()); simulateInput(container, '@'); // 等待下拉菜单打开 await act(async () => { jest.runAllTimers(); await Promise.resolve(); }); scrollIntoViewMock.mockClear(); // 模拟按下 Down 箭头键 fireEvent.keyDown(container.querySelector('textarea'), { key: 'ArrowDown', keyCode: KeyCode.DOWN, }); // 验证滚动被触发 expect(scrollIntoViewMock).toHaveBeenCalled(); scrollIntoViewMock.mockRestore(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/DropdownMenu.tsx(2 hunks)src/KeywordTrigger.tsx(2 hunks)tests/DropdownMenu.spec.tsx(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/DropdownMenu.spec.tsx (1)
tests/util.ts (1)
simulateInput(3-24)
src/DropdownMenu.tsx (1)
src/Mentions.tsx (1)
DataDrivenOptionProps(43-45)
⏰ 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). (1)
- GitHub Check: test / react component workflow
🔇 Additional comments (8)
src/KeywordTrigger.tsx (3)
72-72: 状态初始化正确。使用
useState(false)初始化opened状态是合理的，确保在下拉菜单首次打开前不会触发滚动。
75-79: 正确传递 opened 状态到 DropdownMenu。将
opened属性传递给 DropdownMenu 组件使其能够根据下拉菜单的实际打开状态来控制滚动行为。
103-103:afterOpenChange的行为已验证，实现方式正确，无需修改。根据 @rc-component/trigger v3.0.0 的文档，
afterOpenChange回调在内部 visible 状态变化时被调用，而非在动画完成后调用。这是该库的标准行为。当前代码实现（
afterOpenChange={setOpened}）正确地追踪了弹窗的可见性变化，opened状态随后被传递给DropdownMenu使用。这种模式符合库的 API 设计，不存在状态不一致或不必要的滚动行为问题。src/DropdownMenu.tsx (4)
1-1: 正确使用 type 导入。将
MenuRef改为 type 导入是良好的 TypeScript 实践，有助于减少运行时包大小。
9-9: opened 属性应该是必需的。
opened: boolean被定义为必需属性，这确保了调用者必须明确提供下拉菜单的打开状态。这是正确的设计决策。
33-34: 滚动防护逻辑正确。通过检查
!opened来防止在下拉菜单未打开时执行滚动操作，这正确地解决了强制滚动的问题。
45-45: 依赖数组正确包含 opened。将
opened添加到依赖数组确保当下拉菜单状态改变时，effect 会重新执行。这对于在下拉菜单打开后触发滚动行为至关重要。tests/DropdownMenu.spec.tsx (1)
35-37: 良好的防护验证。验证在输入触发后
scrollIntoView不会立即被调用，这正确地测试了修复的核心行为——防止在下拉菜单打开时强制滚动。
| }); | ||
|
|
||
| it('should scroll into view when navigating with keyboard', () => { | ||
| it('should scroll into view when navigating with keyboard', async () => { |
There was a problem hiding this comment.
测试描述与实际行为不匹配。
测试名称为 "should scroll into view when navigating with keyboard"，但实际测试中并未模拟键盘导航（例如按下 Down/Up 箭头键）。测试只是模拟输入并等待定时器推进。建议将测试名称修改为更准确地反映其实际验证的行为，例如 "should scroll into view after dropdown opens"。
应用此差异来更新测试名称：
- it('should scroll into view when navigating with keyboard', async () => {
+ it('should scroll into view after dropdown opens', async () => {📝 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.
| it('should scroll into view when navigating with keyboard', async () => { | |
| it('should scroll into view after dropdown opens', async () => { |
🤖 Prompt for AI Agents
In tests/DropdownMenu.spec.tsx around line 22, the test title "should scroll
into view when navigating with keyboard" is inaccurate because the test does not
simulate keyboard navigation; rename the test to reflect actual behavior, e.g.,
change the it(...) description to "should scroll into view after dropdown opens"
so the test name matches what is being verified.
ref ant-design/ant-design#55618
Summary by CodeRabbit
发布说明
Bug Fixes
Tests