fix: optimize ESC key handling in nested portal scenarios #494
Conversation
|
@aojunhao123 is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
|
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将图片预览的 ESC 键处理从组件内部移至 Portal 层，调整 Portal 渲染与生命周期（autoDestroy），新增嵌套示例与测试，并对样式、依赖、配置等做小幅更新（.gitignore、tsconfig、assets、docs、package.json）。 Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Component as Preview Component
participant Portal
participant Dialog as Outer Dialog
rect rgb(220,235,255)
Note right of Component: 打开预览流程
User->>Component: 点击图片打开预览
Component->>Portal: render open=true (portalRender && open)
Portal->>Portal: 成为顶层层级
end
rect rgb(255,235,220)
Note right of Portal: 按下 Escape 后的处理
User->>Portal: 按下 Escape
Portal->>Portal: isTopLayer()?
alt Portal 是顶层
Portal->>Component: 调用 onEsc -> onClose
Component->>Portal: 将 open 设为 false（关闭预览）
else Portal 不是顶层
Portal->>Dialog: 事件继续传递给 Dialog（Dialog 自行处理）
end
end
Estimated code review effort🎯 3 (中等复杂度) | ⏱️ ~20 分钟 Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 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 resolves a reported bug where image previews, when opened inside a modal, could suffer from incorrect layering and inconsistent Escape key behavior. The changes involve adjusting the styling to ensure the preview is always on top, and refactoring the Escape key logic to work seamlessly with nested components. A new demo and test case have been added to verify the fix and prevent regressions, enhancing the overall user experience for image previews in complex UI structures. 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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #494 +/- ##
=======================================
Coverage 99.41% 99.41%
=======================================
Files 17 17
Lines 509 511 +2
Branches 152 153 +1
=======================================
+ Hits 506 508 +2
Misses 3 3 ☔ 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 effectively resolves an issue where an image preview within a modal was obscured. The fix involves adjusting the z-index and refining the ESC key handling for nested portals, which is a solid approach. The changes are well-supported by a new test case and a demonstration example. My review includes a suggestion to refactor the new demo component to improve code clarity and maintainability by simplifying event handlers and removing duplication.
| const [show, setShow] = useState(false); | ||
| return ( | ||
| <> | ||
| <button | ||
| onClick={() => { | ||
| setShow(true); | ||
| }} | ||
| > | ||
| showModal | ||
| </button> | ||
| <Dialog | ||
| visible={show} | ||
| afterOpenChange={open => { | ||
| setShow(open); | ||
| }} | ||
| onClose={() => { | ||
| setShow(false); | ||
| }} | ||
| footer={ | ||
| <> | ||
| <button | ||
| onClick={() => { | ||
| setShow(false); | ||
| }} | ||
| > | ||
| Cancel | ||
| </button> | ||
| <button | ||
| onClick={() => { | ||
| setShow(false); | ||
| }} | ||
| > | ||
| OK | ||
| </button> | ||
| </> | ||
| } | ||
| > | ||
| <Image | ||
| width={200} | ||
| alt="svg image" | ||
| src="https://gw.alipayobjects.com/zos/rmsportal/KDpgvguMpGfqaHPjicRK.svg" | ||
| /> | ||
| <Image.PreviewGroup | ||
| preview={{ | ||
| onChange: (current, prev) => | ||
| console.log(`current index: ${current}, prev index: ${prev}`), | ||
| }} | ||
| > | ||
| <Image | ||
| width={200} | ||
| alt="svg image" | ||
| src="https://gw.alipayobjects.com/zos/rmsportal/KDpgvguMpGfqaHPjicRK.svg" | ||
| /> | ||
| <Image | ||
| width={200} | ||
| src="https://gw.alipayobjects.com/zos/antfincdn/aPkFc8Sj7n/method-draw-image.svg" | ||
| /> | ||
| </Image.PreviewGroup> | ||
| </Dialog> | ||
| </> | ||
| ); |
There was a problem hiding this comment.
The event handlers in this component can be simplified and deduplicated for better readability and maintainability. You can define separate functions for showing and hiding the modal and reuse them. Also, afterOpenChange can directly receive the setShow function.
const [show, setShow] = useState(false);
const showModal = () => setShow(true);
const hideModal = () => setShow(false);
return (
<>
<button
onClick={showModal}
>
showModal
</button>
<Dialog
visible={show}
afterOpenChange={setShow}
onClose={hideModal}
footer={
<>
<button
onClick={hideModal}
>
Cancel
</button>
<button
onClick={hideModal}
>
OK
</button>
</>
}
>
<Image
width={200}
alt="svg image"
src="https://gw.alipayobjects.com/zos/rmsportal/KDpgvguMpGfqaHPjicRK.svg"
/>
<Image.PreviewGroup
preview={{
onChange: (current, prev) =>
console.log(`current index: ${current}, prev index: ${prev}`),
}}
>
<Image
width={200}
alt="svg image"
src="https://gw.alipayobjects.com/zos/rmsportal/KDpgvguMpGfqaHPjicRK.svg"
/>
<Image
width={200}
src="https://gw.alipayobjects.com/zos/antfincdn/aPkFc8Sj7n/method-draw-image.svg"
/>
</Image.PreviewGroup>
</Dialog>
</>
);
![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)
docs/examples/nested.tsx (2)
18-25: 审查状态管理逻辑。
Dialog组件同时使用afterOpenChange和onClose来更新show状态可能导致状态管理问题：
afterOpenChange是在动画完成后触发的回调，通常不应直接用于控制可见性状态visible应由父组件完全控制，通过onClose更新即可建议移除
afterOpenChange中的状态更新，仅保留在onClose中：🔎 建议的重构
<Dialog visible={show} - afterOpenChange={open => { - setShow(open); - }} onClose={() => { setShow(false); }}
52-53: 考虑移除生产环境中的 console.log。
onChange回调中的console.log适合开发和演示，但在生产代码中应该移除或替换为适当的日志机制。tests/preview.test.tsx (1)
1056-1073: 增强测试覆盖：验证第一次 ESC 后模态框仍然打开。测试用例正确验证了 ESC 键的分层关闭行为，但可以增加一个断言来明确验证第一次按 ESC 后模态框（Dialog）仍然保持打开状态。
🔎 建议的测试增强
fireEvent.keyDown(window, { key: 'Escape' }); expect(baseElement.querySelector('.rc-image-preview')).toBeFalsy(); + // 验证 Dialog 仍然打开 + expect(baseElement.querySelector('.rc-dialog')).toBeTruthy(); fireEvent.keyDown(window, { key: 'Escape' }); expect(onClose).toHaveBeenCalled();
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
.gitignoreassets/preview.lessdocs/demo/nested.mddocs/examples/nested.tsxpackage.jsonsrc/Preview/index.tsxtests/preview.test.tsxtsconfig.json
🔇 Additional comments (9)
.gitignore (1)
47-48: LGTM!添加
.vscode到.gitignore是标准做法，避免将 IDE 特定配置提交到代码库。docs/demo/nested.md (1)
1-8: LGTM!标准的 dumi 演示文档格式，正确引用了示例代码。
src/Preview/index.tsx (3)
379-383: LGTM! ESC 处理逻辑正确。将 ESC 键处理从组件级别的
keydown事件迁移到 Portal 的onEsc回调是此修复的核心。通过检查top参数，确保只有当前 Portal 处于顶层时才关闭预览，完美解决了嵌套模态框的 ESC 键冲突问题。
331-343: 确认 ESC 关闭逻辑的移除是有意为之且实现正确。验证显示：
onKeyDown处理器正确地仅处理左右箭头键（第337-341行），ESC 逻辑已完全移除- Portal 的
onEsc处理器已正确实现（第379-382行），在顶部时触发onClose()回调- Portal 组件已正确配置
onEsc={onEsc}属性（第399行）左右箭头导航功能保持正常，Portal 的 ESC 键处理在所有场景下都能正确触发。
394-400: 确认 autoDestroy={false} 的设计意图，但需要验证 portalRender 状态管理的潜在问题。Portal 的
autoDestroy={false}确实是为了支持 CSSMotion 的退出动画：当open变为false时，Portal在视觉上关闭，但 DOM 保留以完成 CSSMotion 的motionLeave动画。然而，存在一个状态管理问题：
portalRender状态只在open=true时设置为true，但永远不会被重置为false。这意味着：
- 每次 Preview 打开时，
portalRender已为true（除首次）- 关闭后，Portal 的 DOM 节点在
getContainer中持久化，直到 Preview 组件卸载- 若 Preview 组件长期挂载且频繁打开/关闭，Portal DOM 节点会在容器中累积
建议检查：
- 在
onVisibleChanged中是否应在动画完成后重置portalRender为false- 或在
useLayoutEffect的清理函数中处理open=false的情况tsconfig.json (1)
14-15: LGTM!添加测试库的类型定义是标准做法，支持新增的测试文件中使用 Jest 和 Testing Library 的 API。
tests/preview.test.tsx (1)
10-11: LGTM!新增的导入支持测试用例中使用
useState和Dialog组件，这是测试嵌套场景所必需的。assets/preview.less (1)
8-8: z-index 值 1055 的选择已确认合理。搜索结果显示，该 z-index 值高于 Ant Design Modal 的默认值（1000），确保图片预览在嵌套场景中正确堆叠。代码库中无冲突：预览容器主层级为 1055，内部元素（关闭按钮、导航箭头、工具栏）使用相对 z-index: 1，层级结构清晰。此外，组件通过 props 支持 zIndex 自定义，满足不同场景需求。
package.json (1)
44-44: @rc-component/portal v2.1.2 已验证支持 onEsc 回调和 top 参数。代码在 src/Preview/index.tsx 第 379-383 行直接使用了这些功能：
onEsc回调接收{ top }参数，并通过检查top值来判断是否触发关闭操作。版本升级提供了所需的功能支持。
| return ( | ||
| <Portal open={portalRender} getContainer={getContainer} autoLock={lockScroll}> | ||
| <Portal | ||
| open={portalRender && open} |
There was a problem hiding this comment.
| <Portal open={portalRender} getContainer={getContainer} autoLock={lockScroll}> | ||
| <Portal | ||
| open={portalRender && open} | ||
| autoDestroy={false} |
There was a problem hiding this comment.
这里是为了确保行为与原逻辑一致
aojunhao123
changed the title
2 participants
Summary by CodeRabbit
发布说明
新功能
文档
改进
杂项
✏️ Tip: You can customize this high-level summary in your review settings.