Solve the problem that toast cannot be cleared in strict mode #3413
Conversation
github-actions
bot
added
3.x
总体说明Toast 组件通知系统进行了类型安全加固和多实例管理改造。引入了 变更清单
估计代码审查工作量🎯 3 (中等复杂) | ⏱️ ~20 分钟 建议审查者
诗
Pre-merge checks and finishing touches❌ Failed checks (2 warnings)
✅ Passed checks (1 passed)
✨ Finishing touches
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/packages/toast/__test__/toast.spec.tsx (1)
32-36: 考虑用waitFor替代setTimeout以提高测试稳定性。根据项目的测试最佳实践,应避免在测试中使用固定延迟的
setTimeout。建议使用waitFor配合适当的断言,这样可以使测试更加稳定可靠。基于 learnings,在 React 组件测试中应该使用
act()包装异步状态更新,使用waitFor()进行断言。🔎 建议的重构方案
将使用
waitTimeout的地方改为使用waitFor:-const waitTimeout = (delay: number) => { - return new Promise((resolve) => { - setTimeout(resolve, delay) - }) -}在测试中使用
waitFor:-await waitTimeout(time) -expect(document.querySelector('.nut-toast-text')?.innerHTML).toBe(undefined) +await waitFor(() => { + expect(document.querySelector('.nut-toast-text')).toBe(null) +}, { timeout: time })
📜 Review details
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
src/packages/toast/Notification.tsxsrc/packages/toast/__test__/toast.spec.tsxsrc/packages/toast/toast.tsx
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-02-25T02:21:32.906Z
Learnt from: xiaoyatong
Repo: jdf2e/nutui-react PR: 2990
File: src/packages/pickerview/__test__/pickerview.spec.tsx:0-0
Timestamp: 2025-02-25T02:21:32.906Z
Learning: In React component tests, avoid using setTimeout with fixed delays. Instead, use act() to wrap async state updates and waitFor() for assertions, which makes tests more stable and reliable.
Applied to files:
src/packages/toast/__test__/toast.spec.tsx
🧬 Code graph analysis (3)
src/packages/toast/Notification.tsx (1)
src/packages/notify/Notification.tsx (1)
Notification(18-136)
src/packages/toast/__test__/toast.spec.tsx (2)
src/packages/toast/Notification.tsx (1)
render(91-147)src/packages/toast/toast.taro.tsx (1)
Toast(39-193)
src/packages/toast/toast.tsx (2)
src/packages/toast/Notification.tsx (1)
Notification(15-148)src/utils/merge.ts (1)
clone(23-41)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: test
- GitHub Check: build
🔇 Additional comments (11)
src/packages/toast/Notification.tsx (2)
159-159: 类型改进提升了类型安全性。将
ref回调参数类型从any改为Notification | null是一个很好的改进,提高了类型安全性。
166-166: 添加id字段以支持实例跟踪。在回调中添加
id字段使得toast.tsx能够通过 ID 跟踪和管理多个通知实例,这是修复严格模式下清除问题的关键部分。src/packages/toast/__test__/toast.spec.tsx (4)
146-168: 验证严格模式下清除多个 Toast 的功能。这个测试用例验证了在快速连续显示多个 Toast 后能够正确清除所有实例,这是修复严格模式问题的核心场景。测试逻辑清晰,覆盖了关键功能。
170-183: 增强了测试覆盖率。这些新增的测试用例覆盖了无内容、字符串选项和全局配置等场景,显著提高了 Toast 组件的测试覆盖率。
Also applies to: 185-199, 201-218
167-167: 断言逻辑不准确。Line 167 检查
.innerHTML是否为undefined,但当元素不存在时,document.querySelector返回null,访问null.innerHTML会抛出错误。应该直接检查元素是否为null。🔎 建议的修复
-expect(document.querySelector('.nut-toast-text')?.innerHTML).toBe(undefined) +expect(document.querySelector('.nut-toast-text')).toBe(null)Likely an incorrect or invalid review comment.
220-244: 验证onClose回调的执行时机。测试用例正确验证了 Toast 在持续时间结束后调用
onClose回调。不过与前面提到的问题类似,Line 242 的断言也应该检查元素是否为null而不是访问可能为null的元素的属性。🔎 建议的修复
-expect(document.querySelector('.nut-toast-text')).toBe(null)这一行是正确的,但请同时修复 Line 167 的类似问题。
⛔ Skipped due to learnings
Learnt from: xiaoyatong Repo: jdf2e/nutui-react PR: 2990 File: src/packages/pickerview/__test__/pickerview.spec.tsx:0-0 Timestamp: 2025-02-25T02:21:32.906Z Learning: In React component tests, avoid using setTimeout with fixed delays. Instead, use act() to wrap async state updates and waitFor() for assertions, which makes tests more stable and reliable.src/packages/toast/toast.tsx (5)
1-1: 导入必要的依赖。新增的
ReactNode和clone导入支持了类型改进和实例克隆功能。Also applies to: 5-5
7-11: 引入结构化的实例类型。
NotificationInstance类型定义清晰,包含component、id和destroy方法,为实例管理提供了良好的类型支持。
13-14: 使用 Set 跟踪多个实例。引入
messageInstanceSet来跟踪多个通知实例是解决严格模式清除问题的关键改进。这允许在调用clear()时销毁所有活动的 Toast 实例。
95-100: 增强clear()以销毁所有实例。
clear()方法现在正确地销毁了 Set 中跟踪的所有实例,这是修复严格模式下无法清除所有 Toast 的核心改进。实现逻辑正确。
38-38: 改进了类型注解。将回调和参数类型从
any更新为具体的类型(NotificationInstance、ToastNativeProps、ReactNode)显著提升了类型安全性。Also applies to: 44-44, 49-49, 67-67
| getInstance(opts2, (notification: NotificationInstance) => { | ||
| const oldInstance = messageInstance ? clone(messageInstance) : null | ||
| if (notification.id === oldInstance?.id) { | ||
| messageInstanceSet.add(oldInstance) | ||
| } | ||
| messageInstance = notification |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's find and examine the toast.tsx file structure
fd -t f "toast.tsx" | head -5Repository: jdf2e/nutui-react
Length of output: 89
🏁 Script executed:
# List files in the toast package to understand structure
find . -path "*/toast/*" -type f -name "*.tsx" -o -name "*.ts" | grep -v node_modules | head -20Repository: jdf2e/nutui-react
Length of output: 891
🏁 Script executed:
# Check if the file path exists
ls -la src/packages/toast/toast.tsx 2>/dev/null || find . -name "toast.tsx" -type f | grep -v node_modules | head -5Repository: jdf2e/nutui-react
Length of output: 138
🏁 Script executed:
# Read the toast.tsx file to understand the full context
cat -n src/packages/toast/toast.tsxRepository: jdf2e/nutui-react
Length of output: 3281
🏁 Script executed:
# Find Notification.tsx file
fd -t f "Notification.tsx" | grep -v node_modules | head -5Repository: jdf2e/nutui-react
Length of output: 133
🏁 Script executed:
# Search for ID generation logic using Date.now()
rg -n "Date\.now\(\)" src/packages/toast/ --type tsx --type tsRepository: jdf2e/nutui-react
Length of output: 88
🏁 Script executed:
# Search for Toast.show usage patterns
rg -n "Toast\.show" --type tsx --type ts -A 2 -B 2 | head -50Repository: jdf2e/nutui-react
Length of output: 88
🏁 Script executed:
# Read the Notification.tsx file to see ID generation logic
cat -n src/packages/toast/Notification.tsxRepository: jdf2e/nutui-react
Length of output: 5558
🏁 Script executed:
# Search for Toast.show usage patterns without file type flag
rg -n "Toast\.show" -A 2 -B 2 | head -80Repository: jdf2e/nutui-react
Length of output: 4039
🏁 Script executed:
# Search for Date.now() in the codebase
rg -n "Date\.now\(\)" -A 1 -B 1 | grep -E "(Notification|toast)" | head -20Repository: jdf2e/nutui-react
Length of output: 493
🏁 Script executed:
# Search for ID generation and id assignment patterns
rg -n "id\s*[:=]" src/packages/toast/ -A 1 | head -40Repository: jdf2e/nutui-react
Length of output: 2062
此实例跟踪逻辑存在无法到达的代码路径。
您的分析是正确的。当回调执行时，messageInstance 已在第 42 行被设置为 null，因此第 59 行的 oldInstance 始终为 null。这导致第 60 行的条件 notification.id === oldInstance?.id 始终为 false（比较 notification.id === undefined），使得 messageInstanceSet.add(oldInstance) 的代码路径无法到达。
即使用户显式提供相同的 ID，该条件也永远不会成立。clone() 调用同样是多余的，因为 oldInstance 总是 null。
这段代码看起来意图是为了追踪多个实例，但由于逻辑流程的缺陷，实例集合的管理机制无法正常工作。建议审查实例追踪的设计意图，确定是否需要修改 getInstance 的逻辑（例如在销毁前保存旧实例）或移除无效的追踪代码。
🤖 Prompt for AI Agents
In src/packages/toast/toast.tsx around lines 58-63, the code computes
oldInstance from messageInstance but messageInstance is set to null earlier, so
oldInstance is always null and the id comparison / clone is unreachable; fix by
capturing the previous instance before it is nulled (store a local prev =
messageInstance at the top of the function or move the nulling after this
check), then compare notification.id to prev?.id and add prev (not null) to
messageInstanceSet if they match, or if the design does not require tracking
previous instances, remove the dead clone/conditional entirely; ensure the logic
consistently maintains messageInstance and messageInstanceSet according to the
intended lifecycle.
1 participant
🤔 这个变动的性质是？
🔗 相关 Issue
💡 需求背景和解决方案
Solve the problem that toast cannot be cleared in strict mode
Add a ts type annotation
Add test cases to improve test coverage
Reproduction
☑️ 请求合并前的自查清单
Summary by CodeRabbit
发布说明
新功能
测试
✏️ Tip: You can customize this high-level summary in your review settings.