fix: handle StrictMode esc stack cleanup #33
Conversation
Walkthrough在 Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 分钟 Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 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 addresses and resolves an issue where 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
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #33 +/- ##
==========================================
+ Coverage 94.28% 94.36% +0.08%
==========================================
Files 7 7
Lines 140 142 +2
Branches 48 49 +1
==========================================
+ Hits 132 134 +2
Misses 8 8 ☔ 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 addresses an issue where the escape key handling stack was not being correctly managed in React's StrictMode, which could lead to incorrect onEsc behavior. The fix involves using a useRef to cache the portal's index in the stack and restoring it during the useEffect hook if it was removed by StrictMode's mount-unmount-remount cycle. A new test case is added to verify this behavior in StrictMode.
The fix appears correct and effectively solves the problem. My only concern is the continued use of useMemo for side effects, which is an anti-pattern in React. While changing this would require a larger refactoring, it's a point of fragility in the current implementation. Overall, the change is a good, targeted fix.
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/useEscKeyDown.ts (1)
19-26: 严重问题：useMemo 中存在副作用在
useMemo中直接修改模块级stack变量违反了 React 的规则。useMemo仅应用于计算和记忆值，不应包含副作用。React 可能会在任意时刻调用useMemo，或者跳过调用，这会导致不可预测的行为。根据 React 文档，副作用应该放在
useEffect或useLayoutEffect中。当前实现可能在某些边缘情况下失败，特别是在并发渲染模式下。🔎 建议的修复方案
将栈管理逻辑移到
useEffect中，并使用useLayoutEffect来确保同步执行：- useMemo(() => { - if (open && !stack.includes(id)) { - stack.push(id); - } else if (!open) { - stack = stack.filter(item => item !== id); - } - indexRef.current = stack.indexOf(id); - }, [open, id]); - useEffect(() => { if (!open) { + stack = stack.filter(item => item !== id); + indexRef.current = -1; return; } + if (!stack.includes(id)) { const index = indexRef.current; const safeIndex = index < 0 ? stack.length : Math.min(index, stack.length); stack.splice(safeIndex, 0, id); + } else { + indexRef.current = stack.indexOf(id); } window.addEventListener('keydown', handleEscKeyDown); return () => { stack = stack.filter(item => item !== id); window.removeEventListener('keydown', handleEscKeyDown); }; }, [open, id]);注意：这个修复方案将所有栈操作统一到
useEffect中，避免了useMemo的副作用问题。
🧹 Nitpick comments (1)
tests/index.test.tsx (1)
385-402: 建议增强测试覆盖率当前测试仅验证了在 StrictMode 下首次挂载 Portal 时的场景。为了确保 StrictMode 下的栈清理逻辑完全正确，建议添加以下测试场景：
- 测试在 StrictMode 下切换
visible属性（从 true 到 false 再到 true）- 测试在 StrictMode 下多个 Portal 的挂载和卸载顺序
- 验证组件卸载后栈是否正确清空
🔎 建议的额外测试用例
+ it('onEsc should handle portal toggle in StrictMode', () => { + const onEsc = jest.fn(); + + const Demo = ({ visible }: { visible: boolean }) => + visible ? ( + <Portal open onEsc={onEsc}> + <div /> + </Portal> + ) : null; + + const { rerender } = render(<Demo visible />, { wrapper: React.StrictMode }); + expect(stack).toHaveLength(1); + + rerender(<Demo visible={false} />); + expect(stack).toHaveLength(0); + + rerender(<Demo visible />); + expect(stack).toHaveLength(1); + + fireEvent.keyDown(window, { key: 'Escape' }); + expect(onEsc).toHaveBeenCalledWith(expect.objectContaining({ top: true })); + });
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/useEscKeyDown.tstests/index.test.tsx
🧰 Additional context used
🧬 Code graph analysis (1)
tests/index.test.tsx (1)
src/useEscKeyDown.ts (1)
stack(6-6)
🔇 Additional comments (1)
src/useEscKeyDown.ts (1)
32-37: 在 StrictMode 下的栈管理逻辑已正确实现，无需修复经验证，代码在 StrictMode 下的行为已通过测试用例
onEsc should treat first mounted portal as top in StrictMode（第 385-402 行）验证，该测试正确验证了以下场景：
- Portal 在 StrictMode 中正确挂载并保持栈长度为 1
- onEsc 回调正确接收
top: trueGit 历史记录显示存在针对 "handle StrictMode esc stack cleanup" 的提交，说明这一逻辑是有意设计的。虽然代码逻辑复杂，但
safeIndex的计算和indexRef的使用方式能够正确处理所有场景，包括 StrictMode 的双重挂载流程。
src/useEscKeyDown.ts
Outdated
| if (!stack.includes(id)) { | ||
| const index = indexRef.current; | ||
| const safeIndex = | ||
| index < 0 ? stack.length : Math.min(index, stack.length); | ||
| stack.splice(safeIndex, 0, id); | ||
| } |
There was a problem hiding this comment.
这里用回 orderContext 的话代码似乎更简洁，可读性也会好一些
| it('nested portals should trigger in correct order', () => { | ||
| const onEsc = jest.fn(); | ||
| const onEsc2 = jest.fn(); | ||
| const onEsc3 = jest.fn(); | ||
|
|
||
| render( | ||
| <Portal open onEsc={onEsc}> | ||
| <div /> | ||
| <Portal open onEsc={onEsc2}> | ||
| <div /> | ||
| <Portal open onEsc={onEsc3}> | ||
| <div /> | ||
| </Portal> | ||
| </Portal> | ||
| </Portal> | ||
| ); | ||
|
|
||
| fireEvent.keyDown(window, { key: 'Escape' }); | ||
|
|
||
| expect(onEsc).toHaveBeenCalledWith(expect.objectContaining({ top: false })); | ||
| expect(onEsc2).toHaveBeenCalledWith(expect.objectContaining({ top: false })); | ||
| expect(onEsc3).toHaveBeenCalledWith(expect.objectContaining({ top: true })); | ||
| }); |
There was a problem hiding this comment.
顺带补了一下嵌套Portal下的case
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/useEscKeyDown.ts (1)
18-24: 不应在 useMemo 中执行副作用操作在
useMemo中直接修改共享的stack状态是一个反模式：
useMemo应该是纯函数，用于计算开销较大的值，而不是用于副作用- React 可能会在任何时候丢弃或重新计算 memoized 值，执行时机不可预测
- 现在
useEffect中也有相同的栈管理逻辑（27-28 行），造成逻辑重复且难以维护- 在 StrictMode 下，
useMemo和useEffect的执行次数不同，可能导致不一致🔎 建议的重构方案
将所有栈管理逻辑整合到
useEffect中，移除useMemo中的副作用：- useMemo(() => { - if (open && !stack.includes(id)) { - stack.push(id); - } else if (!open) { - stack = stack.filter(item => item !== id); - } - }, [open, id]); - useEffect(() => { if (open && !stack.includes(id)) { stack.push(id); } else if (!open) { return; } window.addEventListener('keydown', handleEscKeyDown); return () => { stack = stack.filter(item => item !== id); window.removeEventListener('keydown', handleEscKeyDown); }; }, [open, id]);这样可以确保栈管理逻辑只在一个地方维护，并且符合 React 的最佳实践。
src/useEscKeyDown.ts
Outdated
| if (open && !stack.includes(id)) { | ||
| stack.push(id); | ||
| } else if (!open) { |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
栈管理逻辑重复
这段代码与 useMemo 中的逻辑（19-20 行）完全重复。虽然在 useEffect 中处理副作用是正确的做法，但现在同样的逻辑存在于两个地方，违反了 DRY 原则。
建议：将所有栈管理逻辑整合到 useEffect 中，移除 useMemo 中的副作用操作（见上一条评论的建议）。
🤖 Prompt for AI Agents
In src/useEscKeyDown.ts around lines 19-20 and 27-29, the stack push/remove
logic is duplicated between the useMemo and the useEffect; remove side effects
from the useMemo so it remains pure and consolidate all stack management (push
when open becomes true and id not present, and removal on close/unmount) inside
the useEffect cleanup and effect body; ensure the effect depends on open/id and
correctly adds the id when open and removes it when open becomes false or on
unmount, and delete the duplicated code in useMemo.
aojunhao123
force-pushed
the
fix/strict-mode
branch
from
4c8899a to
831863b
Compare
2 participants
Summary by CodeRabbit
发布说明
Bug 修复
测试
兼容性
✏️ Tip: You can customize this high-level summary in your review settings.