fix: handle Escape in preview keydown for nested portal CI#499
fix: handle Escape in preview keydown for nested portal CI#499
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello, 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 addresses a critical issue where the preview component failed to close reliably with the Escape key in nested portal contexts, particularly in CI environments. The changes centralize the Escape key event handling within the component's 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. Changelog
Activity
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
|
Walkthrough将 Preview 组件的 Escape 键关闭流程改为：在组件内维护 Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Window
participant Preview
participant Portal
User->>Window: 按下 Escape（keydown 捕获阶段）
Window->>Preview: 触发 Preview.onKeyDown (capture)
Note over Preview: 设置 escClosingRef = true\n设置 openRef.current = false\n阻止默认并调用 onClose
Preview->>Preview: 调用 onClose (关闭逻辑)
Window->>Portal: 捕获/冒泡到 Portal.onEsc 回退
Portal->>Portal: 读取 escClosingRef（guard）
alt escClosingRef == true
Note right of Portal: 不再调用 onClose（去重）
else
Portal->>Preview: 调用 onClose（通过 Portal 回退）
end
Preview->>Preview: 在微任务中重置 escClosingRef = false
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 #499 +/- ##
==========================================
+ Coverage 99.41% 99.42% +0.01%
==========================================
Files 17 17
Lines 511 523 +12
Branches 153 155 +2
==========================================
+ Hits 508 520 +12
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 refactors the Escape key handling for the Preview component to better manage nested overlays. The approach moves the logic into a keydown event listener on the window, using event propagation control to ensure overlays close in the correct order. This also involves cleaning up the previous onEsc prop implementation. While this is a good direction, there's a small improvement to be made in the event handling to make it more robust, as detailed in my comment.
src/Preview/index.tsx
Outdated
|
|
||
| if (keyCode === KeyCode.ESC || key === 'Escape') { | ||
| event.preventDefault(); | ||
| event.stopPropagation(); |
There was a problem hiding this comment.
Using event.stopPropagation() here will not prevent other event listeners attached directly to the window object from firing. It only stops the event from bubbling up the DOM tree. Since both the Preview and a parent Dialog component might register their own keydown listeners on window, this could lead to unexpected behavior where both close simultaneously if the event listener registration order is not as expected.
To ensure that only the topmost overlay's event handler runs and prevents any others on window from executing, you should use event.stopImmediatePropagation(). This will stop the invocation of any subsequent listeners on the same element (window in this case).
| event.stopPropagation(); | |
| event.stopImmediatePropagation(); |
There was a problem hiding this comment.
Pull request overview
This PR updates the image preview overlay’s Escape-key handling to be more reliable across environments (notably JSDOM in CI) and to avoid duplicate close paths when used in nested portal/overlay scenarios.
Changes:
- Handle
Escapedirectly in the preview’swindowkeydownhandler using bothkeyCodeandkey. - Remove
Portal’sonEschandler to avoid double-close callbacks / duplicated Escape handling. - Stop/prevent default on Escape to influence nested overlay close ordering.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
| if (keyCode === KeyCode.ESC || key === 'Escape') { | ||
| event.preventDefault(); | ||
| event.stopPropagation(); | ||
| onClose?.(); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (2)
src/Preview/index.tsx (2)
342-348: LEFT/RIGHT 键处理未调用stopPropagationESC键处理调用了
stopPropagation()，但LEFT/RIGHT键切换图片时未做相同处理。如果父级组件也监听了方向键，可能会产生意外行为。请确认这是否为预期设计。🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Preview/index.tsx` around lines 342 - 348, 在处理左右方向键切换的分支（包含 showLeftOrRightSwitches 的代码块，比较 KeyCode.LEFT / KeyCode.RIGHT 并调用 onActive）中加入对事件的 stopPropagation() 调用；定位到处理键盘事件的函数（包含 showLeftOrRightSwitches、KeyCode.LEFT/RIGHT 和 onActive 的代码），在执行 onActive(-1) 或 onActive(1) 之前调用 event.stopPropagation()（若使用的是 KeyboardEvent 对象名非 event，请用实际参数名），以和 ESC 分支对齐并避免父组件接收到这些方向键事件。
352-360: 建议使用捕获阶段以应对可能的嵌套 Portal 场景在嵌套 Portal 环境中，使用捕获阶段可以确保 Preview 组件的 keydown 事件处理器优先执行。虽然当前使用的
stopPropagation()在简单场景中有效，但对于复杂的 Portal 嵌套，捕获阶段能提供更可靠的事件处理优先级。♻️ 建议修改
useEffect(() => { if (open) { - window.addEventListener('keydown', onKeyDown); + window.addEventListener('keydown', onKeyDown, true); return () => { - window.removeEventListener('keydown', onKeyDown); + window.removeEventListener('keydown', onKeyDown, true); }; } }, [open]);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/Preview/index.tsx` around lines 352 - 360, In the useEffect block that installs the keydown listener for the Preview component, switch the listener to the capture phase so the onKeyDown handler runs before nested Portals; call window.addEventListener('keydown', onKeyDown, { capture: true }) (or third arg true) and ensure the cleanup calls window.removeEventListener('keydown', onKeyDown, { capture: true }) to match; update the useEffect surrounding code referencing useEffect and onKeyDown accordingly so the handler is added/removed in capture mode.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/Preview/index.tsx`:
- Around line 342-348: 在处理左右方向键切换的分支（包含 showLeftOrRightSwitches 的代码块，比较
KeyCode.LEFT / KeyCode.RIGHT 并调用 onActive）中加入对事件的 stopPropagation()
调用；定位到处理键盘事件的函数（包含 showLeftOrRightSwitches、KeyCode.LEFT/RIGHT 和 onActive
的代码），在执行 onActive(-1) 或 onActive(1) 之前调用 event.stopPropagation()（若使用的是
KeyboardEvent 对象名非 event，请用实际参数名），以和 ESC 分支对齐并避免父组件接收到这些方向键事件。
- Around line 352-360: In the useEffect block that installs the keydown listener
for the Preview component, switch the listener to the capture phase so the
onKeyDown handler runs before nested Portals; call
window.addEventListener('keydown', onKeyDown, { capture: true }) (or third arg
true) and ensure the cleanup calls window.removeEventListener('keydown',
onKeyDown, { capture: true }) to match; update the useEffect surrounding code
referencing useEffect and onKeyDown accordingly so the handler is added/removed
in capture mode.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 6de5bab6-02cf-4e7e-bbc8-5af85bd83bbe
📒 Files selected for processing (1)
src/Preview/index.tsx
- Register keydown listener in capture phase (true) so Preview handler fires before any bubble-phase listeners (e.g. parent Dialog) - Replace stopPropagation with stopImmediatePropagation to also block other capture-phase listeners registered after this one Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…Dialog ESC Portal's onGlobalKeyDown uses a stack with top-check to prevent Dialog from closing when Preview is on top. stopImmediatePropagation was blocking this mechanism entirely, causing Dialog's onClose to never fire on second ESC. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
| } | ||
| }; | ||
|
|
||
| const escClosingRef = useRef(false); |
|
奇怪，啥原因被改坏的 |
#494 这个改法把关闭 preview 的成功条件，绑定到了 Portal onEsc 的内部判定（top、栈顺序、容器上下文）上。
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Preview/index.tsx`:
- Around line 333-345: The capture-phase global keydown listener
(window.addEventListener(..., true)) in onKeyDown currently forces Preview to
close before child Portals/Dropdowns can handle ESC; change the global listener
to the bubble phase (remove the third-arg true / use false) so descendant
handlers get priority, and in onKeyDown first check event.defaultPrevented and
return early (do not call onClose) to treat child-handled ESC as authoritative;
when you do call onClose, keep escClosingRef handling but also call
event.stopPropagation() in addition to event.preventDefault() to avoid
parent/native handlers receiving the event; update tests to cover "Preview with
nested Dropdown/Portal ESC priority" to ensure child handlers can prevent
Preview closing.
In `@tests/preview.portal.test.tsx`:
- Around line 60-73: Split the current single test into two separate cases to
validate handling when only key or only keyCode is provided: keep the existing
setup using Preview and the simulated portal escape via (global as
any).__portalProps.onEsc({ top: true }), but in one test
fireEvent.keyDown(window, { key: 'Escape' }) (no keyCode) and in the other
fireEvent.keyDown(window, { keyCode: 27 }) (no key), and assert onClose was
called exactly once in each; update the test names to reflect "key only" and
"keyCode only" and reuse the same onClose mock and render logic from the
original test to avoid duplicate-close regressions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 120b5fdb-45f8-4051-8c40-a12480093802
📒 Files selected for processing (2)
src/Preview/index.tsxtests/preview.portal.test.tsx
| const onKeyDown = useEvent((event: KeyboardEvent) => { | ||
| if (open) { | ||
| const { keyCode } = event; | ||
| const { keyCode, key } = event; | ||
|
|
||
| if (keyCode === KeyCode.ESC || key === 'Escape') { | ||
| escClosingRef.current = true; | ||
| event.preventDefault(); | ||
| onClose?.(); | ||
|
|
||
| Promise.resolve().then(() => { | ||
| escClosingRef.current = false; | ||
| }); | ||
| return; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's examine the Preview component file and the specific lines mentioned
cd /repo && head -c 500000 src/Preview/index.tsx | wc -lRepository: react-component/image
Length of output: 120
🏁 Script executed:
# Get the relevant code sections
sed -n '333,345p' src/Preview/index.tsxRepository: react-component/image
Length of output: 433
🏁 Script executed:
# Check the other mentioned line ranges
sed -n '358,364p' src/Preview/index.tsx
sed -n '392,395p' src/Preview/index.tsxRepository: react-component/image
Length of output: 373
🏁 Script executed:
# Search for window.addEventListener in the file to see capture phase usage
rg -n "addEventListener.*keydown" src/Preview/index.tsxRepository: react-component/image
Length of output: 127
🏁 Script executed:
# Look for escClosingRef definition and usage
rg -n "escClosingRef" src/Preview/index.tsxRepository: react-component/image
Length of output: 239
🏁 Script executed:
# Find Portal component and understand its onEsc handling
fd Portal src/ -type fRepository: react-component/image
Length of output: 235
🏁 Script executed:
# Check if there's a Portal component and how it handles ESC
rg -n "onEsc" src/ -A 3Repository: react-component/image
Length of output: 469
🏁 Script executed:
find . -name "*.ts" -o -name "*.tsx" | xargs grep -l "onEsc.*top"Repository: react-component/image
Length of output: 247
🏁 Script executed:
# Look for Portal component definition
rg -n "interface PortalProps|type PortalProps" -A 10Repository: react-component/image
Length of output: 962
🏁 Script executed:
# Check how Portal uses the onEsc callback and what 'top' means
rg -n "onEsc\(\{.*top" -B 5 -A 5Repository: react-component/image
Length of output: 1260
🏁 Script executed:
# Check the test file to understand expected behavior
cat tests/preview.portal.test.tsxRepository: react-component/image
Length of output: 2045
🏁 Script executed:
# Search for any nested Portal or child component ESC handling tests
rg -n "imageRender|actionsRender" tests/ -A 5 -B 5Repository: react-component/image
Length of output: 2796
🏁 Script executed:
# Look for any tests with nested Dropdown or Portal scenarios
rg -n "Dropdown|nested" tests/Repository: react-component/image
Length of output: 47
window capture 阶段的 ESC 处理会直接关闭 Preview，无法让内部子浮层优先响应。
window.addEventListener('keydown', onKeyDown, true) 在捕获阶段执行，早于任何子层 keydown 或 Portal.onEsc 回调。这里直接调用 onClose()，设置 escClosingRef.current = true 后，虽然能防止 Portal.onEsc 重复调用 onClose，但无法让子浮层（如 imageRender/actionsRender 中的 Dropdown 或嵌套 Portal）优先处理 ESC。按 ESC 时 Preview 会先关闭，打破了 Portal 的 top 语义。此外只有 preventDefault() 而无 stopPropagation()，父层的原生 keydown 监听仍会收到事件。
建议：
- 把直接
onClose的路径收窄为真正的 fallback（检查子层是否已处理） - 补充"Preview 内嵌 Dropdown/子 Portal + ESC 优先权"的回归用例
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Preview/index.tsx` around lines 333 - 345, The capture-phase global
keydown listener (window.addEventListener(..., true)) in onKeyDown currently
forces Preview to close before child Portals/Dropdowns can handle ESC; change
the global listener to the bubble phase (remove the third-arg true / use false)
so descendant handlers get priority, and in onKeyDown first check
event.defaultPrevented and return early (do not call onClose) to treat
child-handled ESC as authoritative; when you do call onClose, keep escClosingRef
handling but also call event.stopPropagation() in addition to
event.preventDefault() to avoid parent/native handlers receiving the event;
update tests to cover "Preview with nested Dropdown/Portal ESC priority" to
ensure child handlers can prevent Preview closing.
当时那个 pr 就是为了修嵌套场景下的 esc 关闭顺序问题，当时修完是没问题的，现在挂了然后也不太清楚根因是啥 btw 官网嵌套例子我刚刚试了一下是好的 https://ant.design/components/image-cn#image-demo-nested |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/Preview/index.tsx`:
- Around line 347-349: The ESC handling only calls event.stopPropagation() in
the keyCode branch (if (keyCode === KeyCode.ESC)), but not when the browser
supplies event.key ('Escape') only; update the Preview keyboard handler to
ensure event.stopPropagation() is always invoked for Escape regardless of which
property is present — either by adding a matching branch that checks event.key
=== 'Escape' and calls event.stopPropagation(), or by normalizing the key check
(e.g., derive isEscape from keyCode or key) before the existing logic so the
same stopPropagation call runs for both cases.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7196aa28-0541-4562-9f07-473b72e1f88f
📒 Files selected for processing (2)
src/Preview/index.tsxtests/preview.portal.test.tsx
| if (keyCode === KeyCode.ESC) { | ||
| event.stopPropagation(); | ||
| } |
There was a problem hiding this comment.
stopPropagation 仅在 keyCode 分支调用，key 分支缺失。
当浏览器仅提供 key: 'Escape' 而无 keyCode 时（现代浏览器趋势），事件仍会传播到父级监听器，可能导致父层对话框同时关闭，与 PR 目标"防止事件冒泡以确保嵌套层按顺序关闭"不一致。
🐛 建议修复：统一调用 stopPropagation
if (keyCode === KeyCode.ESC || key === 'Escape') {
if (escClosingRef.current) {
return;
}
escClosingRef.current = true;
openRef.current = false;
event.preventDefault();
- if (keyCode === KeyCode.ESC) {
- event.stopPropagation();
- }
+ event.stopPropagation();
onClose?.();
setTimeout(() => {
escClosingRef.current = false;
}, 0);
return;
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/Preview/index.tsx` around lines 347 - 349, The ESC handling only calls
event.stopPropagation() in the keyCode branch (if (keyCode === KeyCode.ESC)),
but not when the browser supplies event.key ('Escape') only; update the Preview
keyboard handler to ensure event.stopPropagation() is always invoked for Escape
regardless of which property is present — either by adding a matching branch
that checks event.key === 'Escape' and calls event.stopPropagation(), or by
normalizing the key check (e.g., derive isEscape from keyCode or key) before the
existing logic so the same stopPropagation call runs for both cases.
|
定位到根因了，歇一会儿晚点发个 pr 修一下 |
Summary
Escapedirectly in the preview'swindowkeydown handlerevent.keyCodeandevent.keyfor better environment compatibilityPortal onEschandling to avoid double-close callbacksWhy
masterCI has been failing ontests/preview.test.tsx(Esc closes preview then modal). In nested portal contexts, the preview escape close path was not reliably triggered, leaving preview open after first Esc.Validation
npm test -- --runInBand(all suites pass locally)Summary by CodeRabbit
发布说明