refactor: use props check instead of version check #593
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
|
Warning Rate limit exceeded@zombieJ has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 12 minutes and 30 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Walkthrough本次更改涉及多个文件的更新，主要包括在 Changes
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #593 +/- ##
==========================================
+ Coverage 89.47% 89.84% +0.36%
==========================================
Files 40 40
Lines 979 975 -4
Branches 328 325 -3
==========================================
Hits 876 876
+ Misses 100 96 -4
Partials 3 3 ☔ View full report in Codecov by Sentry. |
|
New and removed dependencies detected. Learn more about Socket for GitHub ↗︎
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (3)
src/ref.ts (1)
88-90: 建议添加实现说明注释虽然添加了源代码链接，但建议补充说明这种实现方式的原理和优势，以便其他开发者理解。
🧰 Tools
🪛 Biome (1.9.4)
[error] 90-90: Do not access Object.prototype method 'propertyIsEnumerable' from target object.
(lint/suspicious/noPrototypeBuiltins)
tests/ref-19.test.tsx (1)
5-13: 建议优化 mock 实现当前的 mock 实现可能会在测试环境中造成一些副作用。建议：
- 在
afterAll中清理 mock- 确保只 mock 必要的方法而不是整个模块
tests/ref.test.tsx (1)
216-217: 建议增强错误监控的测试覆盖率虽然基本的错误监控已经实现，但建议添加以下改进：
- 添加正向测试用例，验证在预期的错误情况下确实触发了 console.error
- 为不同的 props 场景添加专门的测试用例，确保 props 检查的准确性
建议参考以下示例添加测试用例：
it('should log error for invalid props', () => { const invalidNode = <div prop={null} />; getNodeRef(invalidNode); expect(errSpy).toHaveBeenCalled(); }); it('should handle different prop scenarios', () => { const validNode = <div ref={React.createRef()} />; getNodeRef(validNode); expect(errSpy).not.toHaveBeenCalled(); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
package.json(1 hunks)src/ref.ts(1 hunks)tests/ref-19.test.tsx(1 hunks)tests/ref.test.tsx(2 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
src/ref.ts
[error] 90-90: Do not access Object.prototype method 'propertyIsEnumerable' from target object.
(lint/suspicious/noPrototypeBuiltins)
🪛 GitHub Check: codecov/patch
src/ref.ts
[warning] 92-92: src/ref.ts#L92
Added line #L92 was not covered by tests
🔇 Additional comments (2)
package.json (1)
70-71: 验证 React 19 RC 版本的稳定性
新增的 React 19 RC 版本依赖使用了特定的提交哈希值，这对测试来说是好的做法。但是需要注意：
- RC 版本可能存在不稳定性
- 后续需要及时更新到正式发布版本
tests/ref.test.tsx (1)
15-19: 控制台错误监控设置正确！
错误监控的实现遵循了 Jest 的最佳实践，通过 beforeEach 确保每个测试用例都有一个干净的监控状态。
| ) => React.Ref<T> | null = node => { | ||
| if (node && isReactElement(node)) { | ||
| const ele = node as any; | ||
|
|
||
| // Source from: | ||
| // https://github.com/mui/material-ui/blob/master/packages/mui-utils/src/getReactNodeRef/getReactNodeRef.ts | ||
| return ele.props.propertyIsEnumerable('ref') ? ele.props.ref : ele.ref; | ||
| } | ||
| return null; | ||
| }; |
There was a problem hiding this comment.
建议优化 ref 获取逻辑的实现
当前实现存在以下问题：
- 直接调用目标对象的
propertyIsEnumerable方法可能不安全 - 新增的返回
null分支缺少测试覆盖
建议进行如下优化：
export const getNodeRef: <T = any>(
node: React.ReactNode,
) => React.Ref<T> | null = node => {
if (node && isReactElement(node)) {
const ele = node as any;
- return ele.props.propertyIsEnumerable('ref') ? ele.props.ref : ele.ref;
+ return Object.prototype.propertyIsEnumerable.call(ele.props, 'ref')
+ ? ele.props.ref
+ : ele.ref;
}
return null;
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ) => React.Ref<T> | null = node => { | |
| if (node && isReactElement(node)) { | |
| const ele = node as any; | |
| // Source from: | |
| // https://github.com/mui/material-ui/blob/master/packages/mui-utils/src/getReactNodeRef/getReactNodeRef.ts | |
| return ele.props.propertyIsEnumerable('ref') ? ele.props.ref : ele.ref; | |
| } | |
| return null; | |
| }; | |
| ) => React.Ref<T> | null = node => { | |
| if (node && isReactElement(node)) { | |
| const ele = node as any; | |
| // Source from: | |
| // https://github.com/mui/material-ui/blob/master/packages/mui-utils/src/getReactNodeRef/getReactNodeRef.ts | |
| return Object.prototype.propertyIsEnumerable.call(ele.props, 'ref') | |
| ? ele.props.ref | |
| : ele.ref; | |
| } | |
| return null; | |
| }; |
🧰 Tools
🪛 Biome (1.9.4)
[error] 90-90: Do not access Object.prototype method 'propertyIsEnumerable' from target object.
(lint/suspicious/noPrototypeBuiltins)
🪛 GitHub Check: codecov/patch
[warning] 92-92: src/ref.ts#L92
Added line #L92 was not covered by tests
| it('getNodeRef', () => { | ||
| const ref = React.createRef<HTMLDivElement>(); | ||
| const node = <div ref={ref} />; | ||
|
|
||
| expect(getNodeRef(node)).toBe(ref); | ||
|
|
||
| expect(errSpy).not.toHaveBeenCalled(); | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
建议增加更多测试用例
当前测试只覆盖了基本场景，建议添加以下测试用例：
- 传入 null/undefined 节点的情况
- 传入非 React 元素的情况
- 不同类型的 ref（函数式、对象式）
示例测试用例：
it('should handle null/undefined', () => {
expect(getNodeRef(null)).toBe(null);
expect(getNodeRef(undefined)).toBe(null);
});
it('should handle non-React elements', () => {
expect(getNodeRef('string')).toBe(null);
expect(getNodeRef(123)).toBe(null);
});
it('should work with function ref', () => {
const fnRef = jest.fn();
const node = <div ref={fnRef} />;
expect(getNodeRef(node)).toBe(fnRef);
});
replaced with MUI resolution. It's more flexible:
https://github.com/mui/material-ui/blob/master/packages/mui-utils/src/getReactNodeRef/getReactNodeRef.ts
Summary by CodeRabbit
新特性
测试
getNodeRef函数添加了针对 React 19 的测试套件，确保功能正常并监控控制台错误。