fix: reset stack when unmount #31
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导出并暴露了 Changes
Sequence Diagram(s)（本次变更为导出变量与清理/测试验证，未引入跨多个组件的新控制流，故省略序列图。） Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 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 resolves an issue where components utilizing 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 #31 +/- ##
==========================================
+ Coverage 94.20% 94.24% +0.04%
==========================================
Files 7 7
Lines 138 139 +1
Branches 48 48
==========================================
+ Hits 130 131 +1
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 fixes a bug where a component's ID was not removed from the global stack on unmount. The change correctly addresses this by adding cleanup logic to the useEffect hook. However, this introduces code duplication, as similar logic exists in a useMemo hook. My review includes a comment pointing out this duplication and suggesting a refactor to improve maintainability by consolidating the related logic into a single useEffect hook, which is the standard practice for handling side effects in React.
![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)
18-24: 应使用 useEffect 而非 useMemo 来执行副作用。在
useMemo中修改全局stack违反了 React 的规则。useMemo设计用于记忆计算结果，不应包含副作用。在渲染期间修改外部状态会导致以下问题：
- 并发渲染问题：React 18+ 的并发特性可能会中断和重新开始渲染，导致 stack 状态不一致
- StrictMode 兼容性：开发模式下渲染可能执行两次，可能产生意外的副作用
- 可维护性：违反 React 最佳实践使代码更难理解和维护
🔎 建议的重构方案
将 stack 管理逻辑移至
useEffect中：- useMemo(() => { + useEffect(() => { if (open && !stack.includes(id)) { stack.push(id); - } else if (!open) { - stack = stack.filter(item => item !== id); } + return () => { + stack = stack.filter(item => item !== id); + }; }, [open, id]);这样可以：
- 遵循 React 的副作用处理规范
- 确保清理逻辑在正确的时机执行
- 提高代码在并发模式下的可靠性
注意：这将与现有的
useEffect（第 26-37 行）中的清理逻辑协同工作。
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/useEscKeyDown.ts
🔇 Additional comments (1)
src/useEscKeyDown.ts (1)
34-34: 修复有效，成功解决了内存泄漏问题。在
useEffect清理函数中添加stack过滤逻辑正确修复了以下场景：
- 当组件在
open=true状态下卸载时，确保id从全局 stack 中移除- 防止 stack 无限增长导致的内存泄漏
虽然这与
useMemo中的清理存在部分冗余（当open变为false时），但这种防御性编程是合理的，确保了所有卸载场景都能正确清理。
There was a problem hiding this comment.
Pull request overview
This PR fixes a memory leak in the Escape key handling functionality where the internal stack of portal IDs was not being cleaned up when a Portal component unmounted while in an open state.
Key Changes:
- Added stack cleanup logic in the useEffect cleanup function to remove the portal's ID when the component unmounts
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| window.addEventListener('keydown', handleEscKeyDown); | ||
|
|
||
| return () => { | ||
| stack = stack.filter(item => item !== id); |
There was a problem hiding this comment.
The stack cleanup on component unmount lacks test coverage. Consider adding a test that verifies the stack is properly cleaned up when a Portal component unmounts while still open (not just when open changes to false). This would ensure the fix works correctly and prevent regressions.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/useEscKeyDown.ts (1)
18-37: 逻辑重复且存在 React 反模式。当前实现存在以下问题：
逻辑重复：栈过滤操作出现在两处（第 22 行的
useMemo和第 34 行的useEffect清理函数），导致维护困难。React 反模式：
useMemo被用于执行副作用（修改模块级stack），这违反了 React 的设计原则。useMemo应该用于缓存计算值，而副作用应该放在useEffect中。这些问题在先前的评审中已被提及但未得到解决。
🔎 建议的修复方案
将所有栈管理逻辑合并到单个
useEffect中：- useMemo(() => { - if (open && !stack.includes(id)) { - stack.push(id); - } else if (!open) { - stack = stack.filter(item => item !== id); - } - }, [open, id]); - useEffect(() => { - if (!open) { - return; - } - - window.addEventListener('keydown', handleEscKeyDown); + if (open) { + // 添加到栈 + if (!stack.includes(id)) { + stack.push(id); + } + + // 注册事件监听器 + window.addEventListener('keydown', handleEscKeyDown); + } return () => { + // 清理：从栈中移除并移除事件监听器 stack = stack.filter(item => item !== id); window.removeEventListener('keydown', handleEscKeyDown); }; }, [open, id]);这样可以：
- 消除逻辑重复
- 遵循 React 最佳实践
- 使组件生命周期效果更易理解
- 确保栈的添加和移除逻辑集中在一处
基于先前评审中的反馈。
🧹 Nitpick comments (1)
src/useEscKeyDown.ts (1)
6-6: 导出可变模块状态存在风险。使用
export let导出可变的模块级状态会带来以下问题：
- 创建了与测试代码的紧耦合
- API 语义不清晰（是公共 API 还是仅用于测试？）
- 可能被外部代码意外修改
建议考虑以下替代方案：
🔎 推荐的替代方案
方案 1：提供测试专用的辅助函数
-export let stack: string[] = []; // export for testing +let stack: string[] = []; + +// 仅用于测试 +export const __TEST__ = { + getStack: () => stack, + resetStack: () => { stack = []; } +};方案 2：通过自定义事件通知栈变化
在测试中监听事件而不是直接访问内部状态。
📜 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)
tests/index.test.tsx (1)
360-370: 测试逻辑正确。新增的测试用例有效验证了组件卸载时
stack的清理行为，解决了先前评审中提到的测试覆盖不足的问题。
| import { render, fireEvent } from '@testing-library/react'; | ||
| import React from 'react'; | ||
| import Portal from '../src'; | ||
| import { stack } from '../src/useEscKeyDown'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
考虑测试隔离问题。
直接导入内部实现的 stack 用于测试会造成测试与实现的紧耦合。更重要的是，由于 stack 是模块级别的可变状态，如果没有在测试之间正确重置，可能会导致测试相互影响。
建议在 beforeEach 中添加清理逻辑：
beforeEach(() => {
global.isOverflow = true;
stack.length = 0; // 清空 stack 以确保测试隔离
});🤖 Prompt for AI Agents
In tests/index.test.tsx around line 4, the test currently imports the internal
mutable "stack" which tightly couples tests to implementation and can leak state
between tests; add a beforeEach block that resets global.isOverflow to true and
clears the stack (set its length to 0) to ensure test isolation, and preferably
update tests to use the public API rather than importing the internal stack
directly.
2 participants
Summary by CodeRabbit
发行说明
Bug 修复
测试
✏️ Tip: You can customize this high-level summary in your review settings.