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. Walkthrough为 Trigger/UniqueProvider/Popup 引入并透传 onEsc，Popup/Portal 增加对 Esc 键处理并触发关闭；新增键盘相关测试（包含嵌套弹窗 Esc 行为）并稳定化测试 ID；同时更新 tsconfig 类型与 Changes
Sequence Diagram(s)sequenceDiagram
participant User as 用户
participant Trigger as Trigger
participant Unique as UniqueProvider
participant Popup as Popup
participant Portal as Portal
Note over User,Trigger: 用户按下键盘 (Escape)
User->>Trigger: keydown(Escape)
activate Trigger
Trigger->>Unique: 触发/传递 onEsc 处理
activate Unique
Unique->>Popup: 将 onEsc 透传给 Popup
activate Popup
Popup->>Portal: 将 onEsc 透传给 Portal
activate Portal
Portal->>Portal: 判断是否为顶层弹窗
alt 是顶层弹窗
Portal->>Trigger: 通知关闭/更新可见性
else 非顶层弹窗或非 Esc
Portal-->>Trigger: 保持打开
end
deactivate Portal
deactivate Popup
deactivate Unique
Trigger-->>User: 弹窗状态更新（关闭或保持）
deactivate Trigger
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
庆祝诗
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization 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 |
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.42% 97.06% +0.64%
==========================================
Files 17 17
Lines 950 953 +3
Branches 279 270 -9
==========================================
+ Hits 916 925 +9
+ Misses 34 28 -6 ☔ 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.
aojunhao123
force-pushed
the
feat/esc-close
branch
from
881eacc to
4591871
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/hooks/useEscKeyDown.ts (1)
24-28: 在访问 stack 前需要添加空值检查
stackMap.get(win)可能返回undefined，直接访问stack[stack.length - 1]会导致TypeError。虽然当前逻辑中registerEscEntry会先于 handler 执行填充 stack，但添加防御性检查更为稳健。🔎 建议的修复方案
const handler = (event: KeyboardEvent) => { if (event.key !== 'Escape') { return; } const stack = stackMap.get(win); + if (!stack?.length) { + return; + } - const top = stack[stack.length - 1]; - top.triggerOpen(false); + const top = stack[stack.length - 1]; + top?.triggerOpen(false); };
🧹 Nitpick comments (2)
src/hooks/useEscKeyDown.ts (1)
34-38: 移除事件监听器前应检查 handler 是否存在
handlerMap.get(win)可能返回undefined，应在移除监听器前进行检查，避免无效操作。🔎 建议的修复方案
function removeEscListener(win: Window) { const handler = handlerMap.get(win); - win.removeEventListener('keydown', handler); - handlerMap.delete(win); + if (handler) { + win.removeEventListener('keydown', handler); + handlerMap.delete(win); + } }tests/unique.test.tsx (1)
379-379: 格式问题：解构赋值缺少空格
{ container,baseElement }中逗号后缺少空格，应为{ container, baseElement }。🔎 建议的修复
- const { container,baseElement } = render( + const { container, baseElement } = render(
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
src/UniqueProvider/index.tsx(2 hunks)src/hooks/useEscKeyDown.ts(1 hunks)src/index.tsx(2 hunks)tests/basic.test.jsx(1 hunks)tests/unique.test.tsx(1 hunks)tsconfig.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- src/index.tsx
- tsconfig.json
🧰 Additional context used
🧬 Code graph analysis (4)
tests/unique.test.tsx (2)
src/index.tsx (1)
UniqueProvider(39-39)tests/util.tsx (1)
awaitFakeTimer(97-104)
src/UniqueProvider/index.tsx (1)
src/hooks/useEscKeyDown.ts (1)
useEscKeyDown(65-89)
tests/basic.test.jsx (1)
tests/util.tsx (1)
awaitFakeTimer(97-104)
src/hooks/useEscKeyDown.ts (1)
src/util.ts (1)
getWin(38-40)
🔇 Additional comments (8)
src/hooks/useEscKeyDown.ts (3)
40-54: LGTM!
unregisterEscEntry正确处理了 stack 的空值检查，并在 stack 为空时清理监听器。
56-63: LGTM!注册逻辑正确处理了重复注册的情况，先过滤掉相同 id 的旧条目再添加新条目。
65-89: Hook 实现正确，但需注意 zIndex 场景的局限性Hook 逻辑清晰，正确处理了注册/注销生命周期。但如过往评论所述，基于栈的关闭顺序可能与用户通过
zIndex自定义的视觉层级不一致。如果用户手动调整了弹层的zIndex，后打开的弹层可能视觉上在下层，但按 Escape 仍会先关闭它。这是一个已知的设计限制，如果需要支持 zIndex 感知的关闭顺序，可能需要额外的实现复杂度。
src/UniqueProvider/index.tsx (1)
18-18: LGTM!
useEscKeyDown的集成正确。使用mergedOptions?.id进行可选链访问，并正确传递关闭回调() => trigger(false)。Also applies to: 95-96
tests/unique.test.tsx (1)
378-393: 测试用例逻辑正确测试正确验证了 UniqueProvider 中 Escape 键关闭弹层的功能，包括打开弹层、按下 Escape、验证关闭状态的完整流程。
tests/basic.test.jsx (3)
1205-1218: LGTM!基础 Escape 键关闭测试用例正确，覆盖了打开、按 Escape、验证关闭的完整流程。
1220-1232: LGTM!负面测试用例正确验证了非 Escape 键不会关闭弹层。
1234-1283: LGTM!嵌套弹层测试用例设计良好：
- 使用 mock 确保 ID 的可预测性
- 正确验证了由内向外的关闭顺序
- 使用
try/finally确保 mock 正确恢复
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
tests/basic.test.jsx (2)
1241-1253: 建议添加await以保持测试一致性在第一个测试（第 1237 行）中，在
fireEvent.keyDown后使用了await awaitFakeTimer()，但此测试中未使用。虽然此测试期望不发生变化，但为了测试一致性和可靠性，建议也添加await awaitFakeTimer()。🔎 建议的修改
fireEvent.keyDown(window, { key: 'Enter' }); + await awaitFakeTimer(); expect(isPopupHidden()).toBeFalsy();
1255-1295: 嵌套弹窗测试逻辑正确，建议添加异步等待此测试正确验证了嵌套弹窗的"由内而外"关闭行为。但是，与第一个测试不同，在
fireEvent.keyDown后没有使用await awaitFakeTimer()（第 1288、1292 行）。建议在每次
keyDown事件后添加await awaitFakeTimer()，以确保状态更新完成并保持测试一致性。🔎 建议的修改
fireEvent.keyDown(window, { key: 'Escape' }); + await awaitFakeTimer(); expect(isPopupClassHidden('.inner-popup')).toBeTruthy(); expect(isPopupClassHidden('.outer-popup')).toBeFalsy(); fireEvent.keyDown(window, { key: 'Escape' }); + await awaitFakeTimer(); expect(isPopupClassHidden('.outer-popup')).toBeTruthy();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/Popup/index.tsxsrc/UniqueProvider/index.tsxsrc/index.tsxtests/basic.test.jsx
🚧 Files skipped from review as they are similar to previous changes (2)
- src/UniqueProvider/index.tsx
- src/index.tsx
🧰 Additional context used
🪛 GitHub Actions: ✅ test
src/Popup/index.tsx
[error] 28-28: Property 'onEsc' does not exist on type 'PortalProps'.
🔇 Additional comments (1)
tests/basic.test.jsx (1)
1204-1224: 测试辅助工具实现良好使用
useIdmock 来稳定测试中生成的 ID 是一个很好的做法。实现正确地使用了React.useRef来维护 ID 的稳定性，并且正确地在afterEach中清理 mock。
| import PopupContent from './PopupContent'; | ||
| import useOffsetStyle from '../hooks/useOffsetStyle'; | ||
| import { useEvent } from '@rc-component/util'; | ||
| import type { PortalProps } from '@rc-component/portal'; |
There was a problem hiding this comment.
TypeError：PortalProps 类型缺少 onEsc 属性
根据 @rc-component/portal 的类型定义，PortalProps 接口中不存在 onEsc 属性。尝试使用 onEsc?: PortalProps['onEsc'] 会导致 TypeScript 编译失败。
当前版本 @rc-component/portal@2.0.0 不支持该属性。解决方案：
- 如果需要 Escape 键关闭功能，需要在 PopupProps 中直接定义
onEsc类型，而不是从 PortalProps 继承 - 或者考虑为 Portal 组件的业务包装层单独处理 ESC 键事件
🤖 Prompt for AI Agents
In src/Popup/index.tsx around line 17, the code attempts to reference
PortalProps.onEsc but @rc-component/portal v2.0.0's PortalProps has no onEsc,
causing a TypeScript error; to fix, stop trying to inherit onEsc from
PortalProps and instead add an explicit onEsc?: (e: KeyboardEvent) => void (or
the appropriate signature) to your PopupProps interface, or handle ESC key logic
inside the Popup wrapper (attach a keydown listener that calls a local onEsc
handler) so types align with the external library.
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
| "dependencies": { | ||
| "@rc-component/motion": "^1.1.4", | ||
| "@rc-component/portal": "^2.0.0", | ||
| "@rc-component/portal": "^2.1.0", |
There was a problem hiding this comment.
版本不存在：@rc-component/portal 2.1.0 未在 npm 注册表中发布
@rc-component/portal 的最新版本是 2.0.0，而非 2.1.0。package.json 中指定的版本 ^2.1.0 无法正确安装，因为此版本在 npm 上不存在。请将依赖版本改为 ^2.0.0，或等待 2.1.0 版本正式发布。
🤖 Prompt for AI Agents
In package.json around line 45, the dependency "@rc-component/portal": "^2.1.0"
refers to a non‑existent npm release; change the version specifier to a valid
published release such as "^2.0.0" (or to an exact published version) and then
run your package manager to update lockfiles (npm install / yarn install) so the
lockfile reflects the corrected dependency.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
tests/basic.test.jsxtests/unique.test.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- tests/basic.test.jsx
🧰 Additional context used
🧬 Code graph analysis (1)
tests/unique.test.tsx (1)
src/index.tsx (1)
UniqueProvider(40-40)
src/UniqueProvider/index.tsx
Outdated
| onTargetVisibleChanged(visible); | ||
| }); | ||
|
|
||
| const onEsc: PortalProps['onEsc'] = ({ top, event }) => { |
There was a problem hiding this comment.
这个 ESC 应该是不需要 UniqueProvider 关注的，无论与没有套这个 Provider 应该都可以通过 ESC 来关闭的
2 participants
Summary by CodeRabbit
发布说明
新功能
测试
维护
✏️ Tip: You can customize this high-level summary in your review settings.