fix: prevent error when nativeElement is not available #537
Conversation
|
@cncolder is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
Walkthrough在 Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 分钟
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Demohttps://stackblitz.com/edit/mngzgsqm?file=src%2Fdemo.tsx Dependencies
|
![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
🧹 Nitpick comments (1)
src/Dialog/Content/index.tsx (1)
40-44: 考虑为dialogRef.current添加空值检查以保持一致性。与
onPrepare中新增的保护措施类似，这里访问dialogRef.current.inMotion和dialogRef.current.enableMotion时也可能遇到dialogRef.current为空的情况。建议添加空值检查以提高代码的健壮性。可以考虑使用以下方式：
React.useImperativeHandle(ref, () => ({ ...panelRef.current, - inMotion: dialogRef.current.inMotion, - enableMotion: dialogRef.current.enableMotion, + inMotion: dialogRef.current?.inMotion, + enableMotion: dialogRef.current?.enableMotion, }));
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Dialog/Content/index.tsx(1 hunks)
🔇 Additional comments (1)
src/Dialog/Content/index.tsx (1)
55-56: 良好的防御性编程实践。此保护措施有效防止了当
nativeElement不可用时访问空引用的错误，逻辑正确。
src/Dialog/Content/index.tsx
Outdated
| } | ||
|
|
||
| function onPrepare() { | ||
| if (!dialogRef.current?.nativeElement) return; |
There was a problem hiding this comment.
| if (!dialogRef.current?.nativeElement) return; | |
| if (!dialogRef.current?.nativeElement) { | |
| return; | |
| } |
什么情况下会没有，补充这个情况的测试用例，尝试跑到这一行。
There was a problem hiding this comment.
@afc163 跨 iframe render Modal 时会遇到. https://stackblitz.com/edit/mngzgsqm?file=src%2Fdemo.tsx
There was a problem hiding this comment.
jsdom 环境下可能没有办法测试 iframe, 我只能添加测试用例覆盖到这一行.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #537 +/- ##
==========================================
+ Coverage 98.41% 98.42% +0.01%
==========================================
Files 8 8
Lines 189 191 +2
Branches 66 68 +2
==========================================
+ Hits 186 188 +2
Misses 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
tests/index.spec.tsx (1)
282-292: 新增用例方向正确，但建议减少对其他用例副作用的依赖这个测试很好地抓住了“nativeElement 不可用时调用 onAppearPrepare 不应抛错”的核心行为，整体思路没问题。但当前有两点可以稍微收紧一下：
- 第一个
expect(() => { global.onAppearPrepare?.(); }).not.toThrow();在本用例内部没有任何render，它隐式依赖于前面其它用例已经给global.onAppearPrepare赋值；如果这个用例单独执行或用例顺序调整，这一段要么什么都不做（onAppearPrepare为undefined），要么行为取决于上一个用例的状态，可读性和稳健性都稍弱。- 第二次调用前虽然有一次
render(<Dialog visible />)，但可以考虑在这里也显式构造和本 bug 更接近的场景（例如带mousePosition的 Dialog），让测试意图更直接。可以参考下面两种小改法择一：
- 如果只关心“当前实现下不抛错”，建议去掉对前一个用例状态的依赖：
- it('should not throw error when nativeElement is not available', () => { - expect(() => { - global.onAppearPrepare?.(); - }).not.toThrow(); - - render(<Dialog visible />); - - expect(() => { - global.onAppearPrepare?.(); - }).not.toThrow(); - }); + it('should not throw error when nativeElement is not available', () => { + render(<Dialog visible />); + + expect(() => { + global.onAppearPrepare?.(); + }).not.toThrow(); + });
- 如果希望覆盖“前一次动画留下的 onAppearPrepare + 当前没有 nativeElement”的组合场景，可以在本用例内先显式渲染一次、再触发关闭或卸载，再做第一次调用，这样不用依赖其它测试的执行顺序，同时场景更自说明。
整体来说，用例已经能保护这次修复，以上只是为了让测试更加自洽、抗变更性更好一些，可按需要取舍。
2 participants
Summary by CodeRabbit
修复
测试
✏️ Tip: You can customize this high-level summary in your review settings.