fix: blur unique logic not trigger close #1184
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
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! 此拉取请求旨在解决Select组件中一个特定的失焦行为问题。它通过引入更精细的逻辑来判断何时应关闭组件，从而避免了当用户点击组件内部的非聚焦元素时，组件意外关闭的情况。这一改进提升了用户体验，确保了组件在内部交互时的预期行为。 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
|
演练此PR通过引入内部鼠标按下追踪机制（internalMouseDownRef）和macroTask来防止在内部交互期间意外关闭下拉菜单。模糊事件的关闭逻辑由此标志把守，同时添加测试以验证该行为。 变更
预计代码审查工作量🎯 3 (中等复杂度) | ⏱️ ~20 分钟
可能关联的PR
建议审查者
诗句
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 |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1184 +/- ##
=======================================
Coverage 99.42% 99.42%
=======================================
Files 31 31
Lines 1209 1213 +4
Branches 428 409 -19
=======================================
+ Hits 1202 1206 +4
Misses 7 7 ☔ 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
这个 PR 很好地解决了点击 Select 内部非可聚焦元素导致下拉框意外关闭的问题。通过 internalMouseDownRef 和 macroTask 来判断 blur 事件是否由内部点击触发，逻辑清晰且有效。
我注意到 tests/Combobox.test.tsx 中的修改，虽然能让测试通过，但 delay(100) 放在循环内部会显著拖慢测试速度。我提供了一个建议，将延迟移到循环外部，这样既能保证测试的正确性，又能维持测试效率。
除此之外，代码的其余部分看起来不错，新加的 focus.test.tsx 测试用例也很好地覆盖了核心场景。
| for (let i = 0; i < 10; i += 1) { | ||
| fireEvent.mouseDown(container.querySelector('input')!); | ||
| expectOpen(container); | ||
|
|
||
| await delay(100); | ||
| } | ||
|
|
||
| fireEvent.blur(container.querySelector('input')!); |
There was a problem hiding this comment.
将 await delay(100) 放在循环内部会导致测试运行时间不必要地增加（在这个例子中超过1秒）。这个 delay 的目的是为了确保在触发 blur 事件之前，由最后一次 mouseDown 事件触发的 macroTask 已经执行完毕，将 internalMouseDownRef 重置为 false。
因此，这个延迟只需要在循环之后、blur 事件之前执行一次即可。另外，使用 delay(0) 通常足以等待下一个宏任务，比 delay(100) 更高效，可以避免测试中的硬编码等待时间。
建议将 delay 移出循环，以提高测试效率。
| for (let i = 0; i < 10; i += 1) { | |
| fireEvent.mouseDown(container.querySelector('input')!); | |
| expectOpen(container); | |
| await delay(100); | |
| } | |
| fireEvent.blur(container.querySelector('input')!); | |
| for (let i = 0; i < 10; i += 1) { | |
| fireEvent.mouseDown(container.querySelector('input')!); | |
| expectOpen(container); | |
| } | |
| await delay(0); | |
| fireEvent.blur(container.querySelector('input')!); |
![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)
tests/Combobox.test.tsx (1)
500-505: 在循环中加入await delay(100)有助于稳定与宏任务相关的时序
- 每次
mouseDown后等待一个短暂的真实时间片，能让内部macroTask/blur 逻辑完整执行，避免因为事件过于紧凑导致的非预期状态。- 该测试未启用 fake timers，累计约 1 秒的延时对整体测试时长影响有限，但换来更稳的时序行为验证是值得的。
如后续发现整包测试时间偏长，可以考虑把间隔适当调小（例如 50ms）再评估一次稳定性。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/BaseSelect/index.tsx(4 hunks)tests/Combobox.test.tsx(1 hunks)tests/focus.test.tsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
src/BaseSelect/index.tsx (1)
src/hooks/useOpen.ts (1)
macroTask(10-19)
🔇 Additional comments (2)
tests/focus.test.tsx (1)
122-141: 新增用例很好地覆盖了“内部非可聚焦元素点击不应关闭弹层”的场景
- 用
popupRender渲染纯div.bamboo，再触发mouseDown+blur后断言onPopupVisibleChange没被调用，和 BaseSelect 新的 blur 处理逻辑相吻合。- 计时器配合
beforeEach的jest.useFakeTimers()与act(() => jest.runAllTimers())使用方式也与现有测试保持一致。这段用例整体设计合理，可读性也不错。
src/BaseSelect/index.tsx (1)
24-25: 利用internalMouseDownRef区分内部/外部 blur 的思路合理
- 新增的
internalMouseDownRef与macroTask组合，使得：
- 组件或弹层内部
onRootMouseDown时先将标记置为true，在一个宏任务后再重置为false；onRootBlur中只有在mergedOpen && !internalMouseDownRef.current时才尝试关闭，并继续通过cancelFun校验document.activeElement是否仍在getSelectElements()内。- 这样可以：
- 避免点击弹层内部（包括非可聚焦区域）时，由于
activeElement暂时指向body而误触发关闭；- 保留键盘 Tab 等纯 blur 场景以及真正外部点击时的关闭行为。
macroTask的异步重置也避免了 blur 与 mousedown 同步竞争条件，在现有单测（focus / combobox）下覆盖到了典型时序。整体改动与 PR 目标一致，目前看不到明显的回归风险。
Also applies to: 550-552, 567-572, 596-612
触发 blur 有两种情况：
case 1需要触发关闭，case 2仍应该认为是在元素内部不需要关闭。该 PR 额外做了一层监听，来处理
case 2的情况。TODO: 如果失去焦点，应该将焦点还原给 Select。但是这也是一个行为变化，暂时先不加。有人提 issue 再加。
Summary by CodeRabbit
发布说明
✏️ Tip: You can customize this high-level summary in your review settings.