feat: add escape event handling for better a11y #30
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为 Portal 添加全局 Escape 键处理：新增 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User
participant Portal as Portal 组件
participant Hook as useEscKeyDown Hook
participant Stack as ID 栈
participant Window as 全局 window
participant Callback as onEsc 回调
User->>Portal: open=true（打开 Portal）
Portal->>Hook: useEscKeyDown(open, onEsc)
Hook->>Hook: 生成唯一 id (useId)
Hook->>Stack: id 入栈（若 open）
Hook->>Window: 注册 keydown 监听（Escape）
User->>Window: 按下 Escape
Window->>Hook: 触发 keydown 事件
Hook->>Stack: 检查 id 是否栈顶
alt 是栈顶
Hook->>Callback: onEsc({ top: true, event })
else 不是栈顶
Hook->>Callback: onEsc({ top: false, event })
end
User->>Portal: open=false（关闭 Portal）
Portal->>Hook: 清理副作用
Hook->>Stack: id 出栈
Hook->>Window: 移除 keydown 监听（如无需要）
估算代码审查工作量🎯 3 (中等) | ⏱️ ~20-30 分钟 诗歌
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 @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 significantly improves the accessibility of the Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. 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
|
|
Review the following changes in direct dependencies. Learn more about Socket for GitHub.
|
![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 an onEsc prop to the Portal component for handling 'Escape' key presses, which is a valuable accessibility enhancement. A new useEscKeyDown hook manages a stack of open portals to correctly identify the topmost one. While the implementation is generally sound, it uses a module-level variable for the portal stack. This can lead to subtle issues in complex applications with multiple library versions or in testing environments. I've provided a detailed comment on this with a suggestion for a more robust, context-based approach. The other changes, including dependency updates, test additions, and example usage, are well-implemented.
| export default function useEscKeyDown(open: boolean, onEsc?: EscCallback) { | ||
| const id = useId(); | ||
|
|
||
| const queueCreate = useContext(OrderContext); | ||
|
|
||
| const handleEscKeyDown = useEvent((event: KeyboardEvent) => { | ||
| if (event.key === 'Escape') { | ||
| const isTop = stack[stack.length - 1] === id; | ||
| onEsc?.({ isTop, event }); | ||
| } | ||
| }); | ||
|
|
||
| useEffect(() => { | ||
| if (!open) { | ||
| return; | ||
| } | ||
|
|
||
| const pushToStack = () => { | ||
| stack.push(id); | ||
| }; | ||
|
|
||
| if (queueCreate) { | ||
| queueCreate(pushToStack); | ||
| } else { | ||
| pushToStack(); | ||
| } | ||
|
|
||
| window.addEventListener('keydown', handleEscKeyDown); | ||
|
|
||
| return () => { | ||
| stack = stack.filter(item => item !== id); | ||
| window.removeEventListener('keydown', handleEscKeyDown); | ||
| }; | ||
| }, [open, id]); | ||
| } |
There was a problem hiding this comment.
The use of a module-level stack variable introduces a global state that can lead to subtle bugs and testing difficulties.
Potential Issues:
-
Multiple Library Instances: If an application bundles multiple versions of this library (e.g., in a micro-frontend setup or due to transitive dependencies), each instance will have its own
stack. This will cause theisToplogic to be incorrect, as portals from one library instance won't be aware of portals from another. -
Test Flakiness: State can leak between tests. Although
react-testing-library's automatic cleanup should unmount components and trigger theuseEffectcleanup, relying on this for module-level state is fragile. If a test fails before cleanup, subsequent tests will run with a dirtystack, potentially causing them to fail unpredictably.
Recommendation:
Consider managing the stack within a React Context. This would scope the stack to a React tree, making it more robust, predictable, and easier to test. You could potentially extend the existing OrderContext to manage this stack, or create a new one. This would align better with React's component model and avoid the pitfalls of module-level global state.
aojunhao123
changed the title
![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/useEscKeyDown.ts (2)
1-5: 存在循环依赖风险。
useEscKeyDown.ts从Portal.tsx导入EscCallback类型，而Portal.tsx又导入useEscKeyDown。虽然 TypeScript 对纯类型导入通常能正确处理，但建议将EscCallback类型提取到单独的类型文件（如types.ts）中以避免潜在问题。🔎 建议的重构方案
创建
src/types.ts:export type EscCallback = ({ isTop, event, }: { isTop: boolean; event: KeyboardEvent; }) => void;然后在
Portal.tsx和useEscKeyDown.ts中从types.ts导入。
7-7: 模块级状态可能在特定环境下导致问题。模块级的
stack变量在以下场景可能出现问题：
- SSR 环境：状态会在多个请求之间持久化
- 测试环境：如果测试未正确清理，状态会在测试间泄漏
- 多 React 根节点：不同的 React 根节点会共享同一个 stack
建议考虑在 SSR 环境下提供保护措施，或者提供一个重置 stack 的方法用于测试。
🔎 建议添加 SSR 保护和测试重置方法
+import canUseDom from '@rc-component/util/lib/Dom/canUseDom'; + let stack: string[] = []; + +// For testing purposes +export function resetStack() { + stack = []; +}并在 hook 中添加 SSR 检查：
useEffect(() => { - if (!open) { + if (!open || !canUseDom()) { return; }docs/examples/basic.tsx (1)
44-46: 可选建议：提取 onEsc 处理函数以提高可读性。当前的内联箭头函数实现是正确的，但将处理函数提取到组件顶层可以提高代码可读性。此外，对于可访问性演示，建议展示更真实的用例——按 ESC 键时关闭 Portal（例如调用
setShow(false)），而不仅仅是记录日志。🔎 可选的重构方案
export default () => { const [show, setShow] = React.useState(true); const [customizeContainer, setCustomizeContainer] = React.useState(false); const [lock, setLock] = React.useState(true); const divRef = React.useRef<HTMLDivElement>(null); + const handleRootEsc = ({ isTop, event }) => { + console.log('root onEsc', { isTop, event }); + if (isTop) { + setShow(false); // 演示真实的可访问性用例 + } + }; + + const handleParentEsc = ({ isTop, event }) => { + console.log('parent onEsc', { isTop, event }); + }; + + const handleChildrenEsc = ({ isTop, event }) => { + console.log('children onEsc', { isTop, event }); + }; + React.useEffect( () => () => { console.log('Demo unmount!!'); }, [], ); const getContainer = customizeContainer ? () => divRef.current : undefined; const contentCls = customizeContainer ? '' : 'abs'; return ( <React.StrictMode> <div style={{ height: '200vh' }}> <div style={{ border: '2px solid red' }}> Real Version: {version} <button onClick={() => setShow(!show)}> show: {show.toString()} </button> <button onClick={() => setCustomizeContainer(!customizeContainer)}> customize container: {customizeContainer.toString()} </button> <button onClick={() => setLock(!lock)}> lock scroll: {lock.toString()} </button> <div id="customize" ref={divRef} style={{ border: '1px solid green', minHeight: 10 }} /> </div> - <Portal open={show} getContainer={getContainer} autoLock={lock} onEsc={({ isTop, event }) => { - console.log('root onEsc', { isTop, event }); - }}> + <Portal open={show} getContainer={getContainer} autoLock={lock} onEsc={handleRootEsc}> <p className={clsx(contentCls, 'root')}>Hello Root</p> - <Portal open={show} getContainer={getContainer} autoLock={lock} onEsc={({ isTop, event }) => { - console.log('parent onEsc', { isTop, event }); - }}> + <Portal open={show} getContainer={getContainer} autoLock={lock} onEsc={handleParentEsc}> <p className={clsx(contentCls, 'parent')}>Hello Parent</p> - <Portal open={show} getContainer={getContainer} autoLock={lock} onEsc={({ isTop, event }) => { - console.log('children onEsc', { isTop, event }); - }}> + <Portal open={show} getContainer={getContainer} autoLock={lock} onEsc={handleChildrenEsc}> <p className={clsx(contentCls, 'children')}>Hello Children</p> </Portal> </Portal> </Portal> </div> </React.StrictMode> ); };Also applies to: 48-50, 52-54
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
.gitignore(1 hunks)docs/examples/basic.tsx(1 hunks)package.json(1 hunks)script/update-content.js(0 hunks)src/Portal.tsx(5 hunks)src/useEscKeyDown.ts(1 hunks)tests/index.test.tsx(2 hunks)
💤 Files with no reviewable changes (1)
- script/update-content.js
🧰 Additional context used
🧬 Code graph analysis (2)
src/useEscKeyDown.ts (1)
src/Portal.tsx (1)
EscCallback(24-30)
src/Portal.tsx (1)
src/useEscKeyDown.ts (1)
useEscKeyDown(9-43)
🔇 Additional comments (11)
.gitignore (1)
31-31: LGTM!添加
.vscode到忽略列表是一个合理的做法，可以防止提交用户特定的 IDE 配置。tests/index.test.tsx (4)
1-1: LGTM!正确导入了
fireEvent用于模拟键盘事件。
294-305: 测试设置正确。mock 设置能够为测试提供确定性的 ID 生成。
afterEach中调用jest.restoreAllMocks()确保测试隔离。
307-330: LGTM!测试正确验证了堆栈式 Portal 的
isTop行为：最后打开的 Portal 应该是顶层。
332-364: LGTM!测试正确验证了 Portal 关闭后
isTop状态的变化逻辑。src/Portal.tsx (3)
14-14: LGTM!正确导入了新的
useEscKeyDownhook。
24-30: LGTM!
EscCallback类型设计良好，提供了isTop标志和原始event对象，让消费者可以灵活决定如何处理 Escape 事件。
123-124: LGTM!使用
open而非mergedRender是正确的选择——只有当 Portal 明确打开时才应该响应 Escape 键，即使内容因autoDestroy=false仍然渲染。src/useEscKeyDown.ts (2)
14-19: LGTM!使用
useEvent创建handleEscKeyDown确保了稳定的引用，同时能够访问最新的onEsc回调。isTop的计算逻辑正确地检查当前 Portal 是否在堆栈顶部。
21-42: LGTM!
useEffect的实现正确：
- 当
open为 false 时提前返回- 通过
queueCreate确保与OrderContext的正确协作- 清理函数正确地从 stack 中移除并解绑事件监听器
package.json (1)
53-53: 确认 v6 升级中的实际破坏性变更。v6.0.0 移除了 extend-expect 脚本，这是主要的破坏性变更。此外，v6.0 中移除了 extend-expect，因此使用 v6 之前的版本需要导入不同的路径。
升级后，TypeScript 无法再找到其类型定义。v6 需要在设置文件中导入 @testing-library/jest-dom，并且不同的测试框架有不同的导入路径（jest-globals 用于 @jest/globals，vitest 用于 vitest）。
验证导入路径和 TypeScript 类型配置是否已正确更新以适应 v6。
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #30 +/- ##
==========================================
+ Coverage 93.27% 94.20% +0.92%
==========================================
Files 6 7 +1
Lines 119 138 +19
Branches 44 48 +4
==========================================
+ Hits 111 130 +19
Misses 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR adds Escape key event handling to the Portal component to improve accessibility. Users can now provide an onEsc callback that will be triggered when the Escape key is pressed, with information about whether the portal is the topmost one in the stack.
Key changes:
- New
useEscKeyDownhook manages a stack of open portals and handles Escape key events - Added
onEsccallback prop to Portal component withisTopandeventparameters - Comprehensive test coverage for escape key interactions and portal stacking behavior
Reviewed changes
Copilot reviewed 6 out of 7 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/useEscKeyDown.ts | New hook that manages global escape key handling with portal stack tracking |
| src/Portal.tsx | Integrates the new escape key handler and adds EscCallback type definition |
| tests/index.test.tsx | Adds test suite for escape key behavior, including stack management and top portal detection |
| docs/examples/basic.tsx | Updates example to demonstrate the new onEsc callback functionality |
| package.json | Updates @testing-library/jest-dom dependency from 5.16.4 to 6.9.0 |
| .gitignore | Adds .vscode directory to ignored files |
| script/update-content.js | Removes unused utility script |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| pushToStack(); | ||
| } | ||
|
|
||
| window.addEventListener('keydown', handleEscKeyDown); |
There was a problem hiding this comment.
The event listener is added to window even when onEsc is undefined. This adds unnecessary overhead. Consider adding an early return or conditional check to avoid registering the event listener when onEsc is not provided.
| autoDestroy?: boolean; | ||
| /** Lock screen scroll when open */ | ||
| autoLock?: boolean; | ||
| onEsc?: EscCallback; |
There was a problem hiding this comment.
The new onEsc prop lacks documentation. Consider adding a JSDoc comment to describe its purpose, parameters, and usage. For example, explaining that the callback receives an object with isTop (boolean indicating if this portal is the topmost) and event (the keyboard event).
tests/index.test.tsx
Outdated
| }); | ||
|
|
||
| afterEach(() => { | ||
| jest.restoreAllMocks(); |
There was a problem hiding this comment.
The tests mock useId but don't clean up the module-level stack variable in useEscKeyDown.ts between tests. This could lead to test pollution where the stack state from one test affects another. Consider adding cleanup to reset the stack, or better yet, export a test-only function to reset the stack state.
| jest.restoreAllMocks(); | |
| jest.restoreAllMocks(); | |
| jest.resetModules(); |
tests/testUtils.ts
Outdated
There was a problem hiding this comment.
目前 useId为了确保测试环境的快照稳定，因此返回值恒为 test-id，但对于 portal 这种需要唯一id来注册栈的情况下，测试环境就需要模拟真实useId行为。感觉这个工具函数是不是可以放rc-util里，trigger那边把portal版本抬上来之后，测试用例里也需要做mock useId的操作的
There was a problem hiding this comment.
emmm没必要做额外的mock逻辑，这里useId不用于快照，直接jest.mock还原成react的useId就行
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/useEscKeyDown.ts (2)
31-36: 性能优化：避免在无回调时注册事件监听器即使
onEsc为undefined，事件监听器仍然会被注册到window上，这会带来不必要的性能开销。🔎 建议的优化方案
useEffect(() => { - if (!open) { + if (!open || !onEsc) { return; } window.addEventListener('keydown', handleEscKeyDown); return () => { if (inStackRef.current) { stack = stack.filter(item => item !== id); inStackRef.current = false; } window.removeEventListener('keydown', handleEscKeyDown); }; -}, [open, id]); +}, [open, id, onEsc]);
6-6: 模块级别的stack变量仍存在风险此问题已在之前的评审中被标记。模块级别的全局状态可能导致：
- 多个库实例之间的冲突（微前端场景）
- 测试之间的状态泄漏
建议考虑使用 React Context 来管理堆栈，这样可以将状态限定在 React 树的作用域内。
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
docs/examples/basic.tsxsrc/Portal.tsxsrc/useEscKeyDown.tstests/index.test.tsxtests/testUtils.ts
🚧 Files skipped from review as they are similar to previous changes (3)
- src/Portal.tsx
- tests/index.test.tsx
- docs/examples/basic.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
src/useEscKeyDown.ts (1)
src/Portal.tsx (1)
EscCallback(24-30)
🔇 Additional comments (2)
tests/testUtils.ts (1)
1-27: 测试工具实现合理这个模拟
useId的工具函数实现得当，能够为测试提供确定性的 ID 生成。使用React.useRef缓存每个组件实例的 ID，并通过setup重置计数器，可以确保测试的可预测性。src/useEscKeyDown.ts (1)
8-17: 事件处理逻辑正确使用
useEvent包装回调函数是正确的做法，确保了回调的稳定引用。堆栈顶部判断逻辑清晰准确。
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
src/useEscKeyDown.ts (2)
6-6: 模块级 stack 变量的架构问题此问题在之前的评审中已被标记：模块级的可变状态可能导致多实例场景（如微前端）下的逻辑错误，以及测试间的状态泄漏。虽然维护者似乎已知晓，但建议重新考虑使用 React Context 来管理 stack，以提高可预测性和可测试性。
26-36: 性能优化：onEsc 为 undefined 时应避免添加事件监听器当
onEsc未提供时，仍然会添加全局的keydown事件监听器，造成不必要的性能开销。建议添加提前返回检查。🔎 优化建议
useEffect(() => { - if (!open) { + if (!open || !onEsc) { return; } window.addEventListener('keydown', handleEscKeyDown); return () => { window.removeEventListener('keydown', handleEscKeyDown); }; - }, [open, id]); + }, [open, onEsc, handleEscKeyDown]);
🧹 Nitpick comments (1)
src/useEscKeyDown.ts (1)
36-36: 可选优化：依赖数组包含不必要的id
useId()返回的id在组件生命周期内是稳定的，无需包含在依赖数组中。移除它可以使代码意图更清晰。- }, [open, id]); + }, [open]);
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/useEscKeyDown.ts
🧰 Additional context used
🧬 Code graph analysis (1)
src/useEscKeyDown.ts (1)
src/Portal.tsx (1)
EscCallback(24-30)
🔇 Additional comments (1)
src/useEscKeyDown.ts (1)
11-16: LGTM：Escape 键处理逻辑正确使用
useEvent确保了回调的稳定引用，top 状态的计算逻辑清晰准确。
| useMemo(() => { | ||
| if (open && !stack.includes(id)) { | ||
| stack.push(id); | ||
| } else if (!open) { | ||
| stack = stack.filter(item => item !== id); | ||
| } | ||
| }, [open, id]); |
There was a problem hiding this comment.
严重问题：useMemo 用于副作用 + 缺少卸载清理
存在两个关键问题：
-
违反 React 原则：
useMemo被设计用于记忆计算值，而非执行副作用。在 React 19 的并发模式下，渲染可能被中断或重复执行，导致 stack 状态不一致。 -
内存泄漏：当组件在
open=true时卸载，useMemo不会再次运行，导致id永久残留在stack中。
建议的修复方案：将 stack 操作移至 useEffect，确保在 cleanup 函数中移除 id。
🔎 推荐的修复方案
- useMemo(() => {
- if (open && !stack.includes(id)) {
- stack.push(id);
- } else if (!open) {
- stack = stack.filter(item => item !== id);
- }
- }, [open, id]);
-
useEffect(() => {
- if (!open) {
- return;
- }
+ if (open && !stack.includes(id)) {
+ stack.push(id);
+ }
- window.addEventListener('keydown', handleEscKeyDown);
+ if (open) {
+ window.addEventListener('keydown', handleEscKeyDown);
+ }
return () => {
+ stack = stack.filter(item => item !== id);
window.removeEventListener('keydown', handleEscKeyDown);
};
}, [open, id]);🤖 Prompt for AI Agents
In src/useEscKeyDown.ts around lines 18 to 24, the code uses useMemo for side
effects which can cause inconsistent stack state and memory leaks; replace the
useMemo with a useEffect that pushes id onto stack when open is true and removes
id when open is false, and also return a cleanup function that always removes id
from stack (to handle component unmounts) so the stack remains consistent in
concurrent rendering.
2 participants
Summary by CodeRabbit
新功能
测试
杂项
✏️ Tip: You can customize this high-level summary in your review settings.