Avoid rc deep imports#509
Conversation
|
@QDyanbing is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough将项目内对 变更内容依赖升级与导入统一
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![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 updates the @rc-component/util dependency and refactors imports across the codebase to use named exports from the package root instead of deep internal paths. Feedback was provided to include the __esModule: true property in the Jest mock for @rc-component/util to ensure consistent behavior and compatibility with ES module syntax.
| jest.mock('@rc-component/util', () => { | ||
| const util = jest.requireActual('@rc-component/util'); | ||
| const origin = jest.requireActual('react'); | ||
| return origin.useId; | ||
| return { | ||
| ...util, | ||
| useId: origin.useId, | ||
| }; | ||
| }); |
There was a problem hiding this comment.
When mocking a module that is intended to be used as an ES module (with named imports), it is a best practice to explicitly include the __esModule: true property. While spreading ...util might copy it if it is enumerable in the actual module, explicitly setting it ensures consistent behavior across different environments and Jest configurations, especially when the module is imported using ESM syntax.
| jest.mock('@rc-component/util', () => { | |
| const util = jest.requireActual('@rc-component/util'); | |
| const origin = jest.requireActual('react'); | |
| return origin.useId; | |
| return { | |
| ...util, | |
| useId: origin.useId, | |
| }; | |
| }); | |
| jest.mock('@rc-component/util', () => { | |
| const util = jest.requireActual('@rc-component/util'); | |
| const origin = jest.requireActual('react'); | |
| return { | |
| __esModule: true, | |
| ...util, | |
| useId: origin.useId, | |
| }; | |
| }); |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/preview.test.tsx (1)
1133-1140:⚠️ Potential issue | 🟠 Major | ⚡ Quick win补齐 Escape 事件的 keyCode 字段，修复测试不稳定问题
Line 1134 和 Line 1139 的
fireEvent.keyDown()调用缺少keyCode字段。相比之下，Line 1104 的相同事件已包含keyCode: 27且工作正常。缺少keyCode导致 Dialog 组件的keyboard属性无法正确识别 Escape 按键，致使 Preview 无法关闭（CI 报错 Line 1137 中.rc-image-preview仍存在）。建议为两处
keyDown事件补齐完整的键盘事件属性：建议修改
await act(async () => { - fireEvent.keyDown(window, { key: 'Escape' }); + fireEvent.keyDown(window, { key: 'Escape', code: 'Escape', keyCode: 27, which: 27 }); jest.runAllTimers(); }); expect(baseElement.querySelector('.rc-image-preview')).toBeFalsy(); - fireEvent.keyDown(window, { key: 'Escape' }); + fireEvent.keyDown(window, { key: 'Escape', code: 'Escape', keyCode: 27, which: 27 }); expect(onClose).toHaveBeenCalled();🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/preview.test.tsx` around lines 1133 - 1140, The Escape key events fired in the test are missing the numeric key code, causing the Dialog keyboard handling to miss the Escape; update the two fireEvent.keyDown(...) calls (the one inside act with jest.runAllTimers and the subsequent one that should trigger onClose) to include the proper numeric fields (e.g., keyCode: 27 and which: 27) alongside key: 'Escape' so the Preview closes and onClose is called.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@tests/preview.test.tsx`:
- Around line 1133-1140: The Escape key events fired in the test are missing the
numeric key code, causing the Dialog keyboard handling to miss the Escape;
update the two fireEvent.keyDown(...) calls (the one inside act with
jest.runAllTimers and the subsequent one that should trigger onClose) to include
the proper numeric fields (e.g., keyCode: 27 and which: 27) alongside key:
'Escape' so the Preview closes and onClose is called.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 92d20569-5c65-4597-ba74-22b0366ad982
📒 Files selected for processing (2)
package.jsontests/preview.test.tsx
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #509 +/- ##
=======================================
Coverage 99.44% 99.44%
=======================================
Files 17 17
Lines 541 541
Branches 165 165
=======================================
Hits 538 538
Misses 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
QDyanbing
force-pushed
the
avoid-deep-imports
branch
from
69a81d4 to
1656419
Compare
1 participant
调整内容
@rc-component/util/lib/*的引用调整为从@rc-component/util根入口导入。@rc-component/util到^1.11.1，升级@rc-component/father-plugin到^2.2.0。@rc-component/util/lib/test/domHook和@rc-component/util/lib/hooks/useId，改为根入口 mock 并仅覆盖useId。背景
为了避免 rc 包之间依赖
es/lib构建产物内部路径，统一改为依赖公开根入口，后续 lint 规则由@rc-component/father-plugin统一处理。验证
npm test -- preview.test.tsx --runInBandSummary by CodeRabbit
发布说明
Chores
Tests