feat: support close popups by escape key #594
Conversation
|
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. 走读PR 引入了一个新的 变更
预估代码审查工作量🎯 3 (中等复杂度) | ⏱️ ~20 分钟 需要重点关注的区域：
可能相关的 PR
建议审查者
诗
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (2)
🧰 Additional context used🧬 Code graph analysis (3)src/UniqueProvider/index.tsx (1)
tests/unique.test.tsx (2)
src/hooks/useEscKeyDown.ts (1)
🔇 Additional comments (7)
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 @aojunhao123, 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 enhances the user experience by implementing the ability to close popups using the Escape key. It introduces a dedicated React hook to manage this behavior, ensuring proper handling of nested popups by closing them in a sequential, inside-out manner. The change is seamlessly integrated into the existing popup triggering mechanism and is accompanied by robust test coverage. 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 #594 +/- ##
==========================================
+ Coverage 96.40% 97.07% +0.66%
==========================================
Files 17 18 +1
Lines 947 992 +45
Branches 279 277 -2
==========================================
+ Hits 913 963 +50
+ Misses 34 29 -5 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![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 (3)
src/hooks/useEscCancel.ts (2)
19-38: Esc 处理函数可以加上更稳健的防御性判断当前
handler中直接从stackMap取值并访问stack[stack.length - 1]，如果以后有改动导致存在监听器但对应窗口没有栈或栈为空，按 Esc 会直接抛异常。removeEscListener也会在handler不存在时多做一次无意义的removeEventListener。建议轻量加一层判空，提升健壮性：const handler = (event: KeyboardEvent) => { if (event.key !== 'Escape') { return; } - const stack = stackMap.get(win); - - const top = stack[stack.length - 1]; - top.triggerOpen(false); + const stack = stackMap.get(win); + if (!stack?.length) { + return; + } + + const top = stack[stack.length - 1]; + top.triggerOpen(false); };以及：
function removeEscListener(win: Window) { - const handler = handlerMap.get(win); - win.removeEventListener('keydown', handler); - handlerMap.delete(win); + const handler = handlerMap.get(win); + if (!handler) { + return; + } + win.removeEventListener('keydown', handler); + handlerMap.delete(win); }这不会改变现有行为，只是避免未来改动时出现难排查的运行时错误。
65-89:useEscCancel的popupEle类型建议允许null以匹配调用方Hook 签名中
popupEle声明为HTMLElement，但在Trigger里传入的是可能为null的 state（初始值就是null），同时 effect 已经通过if (!open || !popupEle)做了运行时防御。为让类型与实际使用一致，可以小改一下：-export default function useEscCancel( - popupId: string, - open: boolean, - popupEle: HTMLElement, - triggerOpen: (open: boolean) => void, -) { +export default function useEscCancel( + popupId: string, + open: boolean, + popupEle: HTMLElement | null, + triggerOpen: (open: boolean) => void, +) {这样可以避免调用处/后续重构引入多余的类型断言。
tests/basic.test.jsx (1)
1220-1269: 嵌套 Esc 测试建议显式刷一次计时器以避免潜在时序问题
esc should close nested popup from inside out中，两次fireEvent.keyDown(window, { key: 'Escape' })后直接做同步断言。如果triggerOpen(false)经useDelay走的是setTimeout(..., 0)之类实现，这里可能会产生依赖实现细节的时序问题或偶现失败。建议在两次 Esc 之后分别调用
awaitFakeTimer()，与前一个 Esc 用例及其它依赖延时的测试保持一致，例如：fireEvent.keyDown(window, { key: 'Escape' }); - expect(isPopupClassHidden('.inner-popup')).toBeTruthy(); - expect(isPopupClassHidden('.outer-popup')).toBeFalsy(); + await awaitFakeTimer(); + expect(isPopupClassHidden('.inner-popup')).toBeTruthy(); + expect(isPopupClassHidden('.outer-popup')).toBeFalsy(); fireEvent.keyDown(window, { key: 'Escape' }); - expect(isPopupClassHidden('.outer-popup')).toBeTruthy(); + await awaitFakeTimer(); + expect(isPopupClassHidden('.outer-popup')).toBeTruthy();另外，使用
const useIdModule = require('@rc-component/util/lib/hooks/useId'); const useIdSpy = jest.spyOn(useIdModule, 'default').mockImplementation(...);的方式来稳定 id 顺序在当前 CJS/interop 流程下是可行的；如果后续构建切到纯 ESM，可以留意一下是否需要改成
jest.mock形式来继续保证这一用例的稳定性。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/hooks/useEscCancel.ts(1 hunks)src/index.tsx(2 hunks)tests/basic.test.jsx(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (3)
tests/basic.test.jsx (1)
tests/util.tsx (1)
awaitFakeTimer(97-104)
src/index.tsx (1)
src/hooks/useEscCancel.ts (1)
useEscCancel(65-89)
src/hooks/useEscCancel.ts (1)
src/util.ts (1)
getWin(39-41)
🔇 Additional comments (2)
src/index.tsx (1)
19-19: Esc 关闭逻辑在 Trigger 中的集成是合理的在
Trigger内引入并调用useEscCancel(id, mergedOpen, popupEle, triggerOpen)：
- 使用的是合并后的可见状态
mergedOpen，与现有逻辑保持一致；- 通过传入统一的
triggerOpen，Esc 关闭会走同一套受控/非受控、UniqueProvider 分支和回调链；- 调用位置固定且无条件，不会破坏 Hooks 调用顺序。
整体接入看起来没有明显问题。
Also applies to: 651-652
tests/basic.test.jsx (1)
1204-1218: 基础 Esc 关闭用例覆盖合理这里通过点击触发弹层，再触发
keyDown(Escape)并用awaitFakeTimer()刷新定时器后断言隐藏，验证了 Esc 走的是与其它关闭路径一致的异步流程，写法和文件里现有依赖计时器的用例风格一致，看起来没有问题。
![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 introduces a new feature to close popups by pressing the Escape key. A new hook useEscCancel is created to manage a stack of open popups and handle the keydown events. The implementation is clean and includes good test coverage for both single and nested popups. I've added a couple of suggestions in useEscCancel.ts to improve robustness by adding defensive checks against potential edge cases.
|
|
||
| const stack = stackMap.get(win); | ||
|
|
||
| const top = stack[stack.length - 1]; |
There was a problem hiding this comment.
stack 其实不靠谱，因为用户是可以通过 zIndex 来调整展示先后的。所以很有可能先出来的弹层反而在最前面
There was a problem hiding this comment.
这个倒是确实没考虑到这种情况。组件库层面要处理这种因为UI交互设计不规范造成的corner case吗
2 participants
Summary by CodeRabbit
新功能
测试
✏️ Tip: You can customize this high-level summary in your review settings.