fix: mock useId to fix ESC test in nested portal scenarios#502
fix: mock useId to fix ESC test in nested portal scenarios#502zombieJ merged 1 commit intoreact-component:masterfrom
Conversation
Made-with: Cursor
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 focuses on improving the stability of existing tests by addressing a specific interaction issue related to 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. 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
|
|
@aojunhao123 is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
概述修改测试文件以优化模块导入和添加useId钩子的模拟实现，确保在测试环境中实现一致的ID生成。单个文件受影响，改动精准专注。 变更
代码审查工作量评估🎯 2 (简单) | ⏱️ ~8 分钟 建议审查者
诗歌
🚥 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 |
![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 aims to fix a test for nested portals by mocking useId. The change involves removing an unused useState import and adding a mock for @rc-component/util/lib/hooks/useId. My review includes a suggestion to make the mock more robust and align with Jest's documentation for mocking ES modules with default exports.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #502 +/- ##
=======================================
Coverage 99.41% 99.41%
=======================================
Files 17 17
Lines 511 511
Branches 153 153
=======================================
Hits 508 508
Misses 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/preview.test.tsx (1)
26-29: Mock 实现正确，已修复嵌套 Portal 场景下的 ESC 测试问题。使用 React 18 原生的
useId来 mock@rc-component/util/lib/hooks/useId，确保测试中 ID 生成的一致性。这个方案有效地解决了嵌套 Portal 场景下 ESC 键处理的问题。小建议：变量名
origin可以改为更具描述性的名称，如React或actualReact，提升代码可读性。♻️ 可选的命名改进
jest.mock('@rc-component/util/lib/hooks/useId', () => { - const origin = jest.requireActual('react'); - return origin.useId; + const React = jest.requireActual('react'); + return React.useId; });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/preview.test.tsx` around lines 26 - 29, The mock for '@rc-component/util/lib/hooks/useId' uses a non-descriptive variable name "origin"; rename it to a clearer identifier (e.g., React or actualReact) inside the jest.mock callback so the line that assigns const origin = jest.requireActual('react') becomes const React = jest.requireActual('react') (or const actualReact = ...), and then return React.useId (or actualReact.useId) to improve readability while keeping the existing mock behavior in the jest.mock(...) block that targets useId.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/preview.test.tsx`:
- Around line 26-29: The mock for '@rc-component/util/lib/hooks/useId' uses a
non-descriptive variable name "origin"; rename it to a clearer identifier (e.g.,
React or actualReact) inside the jest.mock callback so the line that assigns
const origin = jest.requireActual('react') becomes const React =
jest.requireActual('react') (or const actualReact = ...), and then return
React.useId (or actualReact.useId) to improve readability while keeping the
existing mock behavior in the jest.mock(...) block that targets useId.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 21ebe1c9-25ee-4b65-a945-1aadb73ad141
📒 Files selected for processing (1)
tests/preview.test.tsx
2 participants
useId 为了确保测试环境的 snapshot 稳定，返回值写死为 'test-id'，导致了这个 test case 挂掉，reset 回去就行
那么为什么我在 #494 里写下这个 case 的时候没出现这个问题呢？那就要说到这个 pr 了：
在这个 pr 之前，Portal 处理 keydown 时是每个 Portal 实例各自注册独立的 listener，即使多个 Portal 的 useId() 返回了相同的 'test-id'，每个 handler 仍然会独立触发，Preview 的 onEsc 依然能收到 Escape 事件。
而这个 pr 将实现重构为单一全局 listener + 内部 stack 遍历的方式，stack 的入栈逻辑通过 id 做去重。当 Dialog 的 Portal 先入栈（id='test-id'），Preview 的 Portal 再尝试入栈时发现已存在相同 id 的条目，就直接跳过了。导致 Escape 事件永远只通知到 Dialog，Preview 的 onClose 从未被调用。
顺带一提，这个问题其实不复杂但是涉及上下文比较多，仅把报错信息提供给 AI 的话目前它还是很难推理出完整上下文来给出一个根本性的解法的