fix: Clamp duration to setTimeout max value #381
Conversation
|
@jynxio is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
|
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 概述该 PR 在 Notice 组件中引入了最大持续时间限制，使用 MAX_DURATION 常数对提供的 duration 值进行约束，当超出限制时会触发警告。同时添加了测试用例来验证这一约束行为及相关警告机制。 变更内容
代码审查工作量评估🎯 2 (Simple) | ⏱️ ~10 分钟 可能相关的 PR
建议审查者
庆祝诗
Pre-merge checks and finishing touches✅ Passed checks (3 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 |
Summary of ChangesHello @jynxio, 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 addresses a subtle but critical bug in how Ant Design's 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. 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
|
![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 addresses an issue with setTimeout overflowing for large duration values. The chosen solution is to clamp the duration to the maximum supported value, which is a sensible approach for this edge case. The implementation is correct and includes a helpful warning for developers when the duration is clamped. The added tests are thorough and cover the new logic well. I've provided a couple of suggestions to improve code readability and maintainability by replacing magic numbers with their calculated forms, making the code's intent clearer.
| * Maximum delay value for setTimeout in seconds (2^31 - 1 ms). | ||
| * @see https://developer.mozilla.org/en-US/docs/Web/API/Window/setTimeout#maximum_delay_value | ||
| */ | ||
| const MAX_DURATION = 2147483647 / 1000; |
There was a problem hiding this comment.
To improve readability and avoid a magic number, it would be clearer to express 2147483647 as the calculation it represents. This makes the origin of the number apparent directly in the code, aligning with the explanation in the comment and the PR description.
| const MAX_DURATION = 2147483647 / 1000; | |
| const MAX_DURATION = (2 ** 31 - 1) / 1000; |
| const MAX_DURATION = 2147483647 / 1000; | ||
| const EXCEEDED_DURATION = (2147483647 + 1) / 1000; |
There was a problem hiding this comment.
To improve maintainability and avoid repeating the magic number 2147483647, you could define the maximum timeout value in milliseconds as a constant and then derive MAX_DURATION and EXCEEDED_DURATION from it. This makes the code easier to read and ensures consistency if the value ever needs to be changed.
| const MAX_DURATION = 2147483647 / 1000; | |
| const EXCEEDED_DURATION = (2147483647 + 1) / 1000; | |
| const MAX_TIMEOUT_MS = 2 ** 31 - 1; | |
| const MAX_DURATION = MAX_TIMEOUT_MS / 1000; | |
| const EXCEEDED_DURATION = (MAX_TIMEOUT_MS + 1) / 1000; |
![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)
tests/index.test.tsx (1)
1038-1067: 测试全面验证了限制行为，建议考虑复用常量。该测试正确验证了超限时长的警告和限制行为，包括边界条件的检查。测试逻辑准确且覆盖充分。
一个可选的改进：测试中硬编码了
MAX_DURATION常量（第 1042 行），可以考虑从src/Notice.tsx导入该常量以提高可维护性。不过由于该值基于 JavaScript 规范不太可能变化，当前实现也是可接受的。🔎 可选的改进方案
可以导出 MAX_DURATION 常量并在测试中导入：
在
src/Notice.tsx中：/** * Maximum delay value for setTimeout in seconds (2^31 - 1 ms). * @see https://developer.mozilla.org/en-US/docs/Web/API/Window/setTimeout#maximum_delay_value */ -const MAX_DURATION = 2147483647 / 1000; +export const MAX_DURATION = 2147483647 / 1000;在
tests/index.test.tsx中：+import { MAX_DURATION } from '../src/Notice'; + describe('MAX_DURATION', () => { // ... it('should warn and clamp when duration exceeds limit', () => { const { instance } = renderDemo(); const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); - const MAX_DURATION = 2147483647 / 1000; const EXCEEDED_DURATION = (2147483647 + 1) / 1000;
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/Notice.tsxtests/index.test.tsx
🔇 Additional comments (5)
src/Notice.tsx (4)
8-12: 常量定义正确且文档清晰。MAX_DURATION 的计算和注释都很准确，将 setTimeout 的最大毫秒值转换为秒以匹配 duration 属性的单位。MDN 链接为开发者提供了有用的参考。
49-50: 限制逻辑实现正确。
rawDuration正确处理了非数字类型的 duration（如false），mergedDuration通过Math.min将时长限制在最大值内。这个实现简洁且正确。
64-70: 警告机制设计合理。使用
useEffect配合rawDuration依赖项来触发警告，确保当时长超限时开发者能够收到提示。警告消息清晰地说明了限制值和处理方式。
51-51: 现有逻辑与限制机制集成正确。所有使用时长的地方（进度条、超时定时器）都统一使用了
mergedDuration，确保了限制行为的一致性。通知将在限制后的时长结束时正确关闭。Also applies to: 74-91, 93-119
tests/index.test.tsx (1)
1023-1036: 测试用例正确验证了正常情况。该测试确认了当 duration 在限制范围内时不会触发警告，测试值（0.1s）选择合理，代表了典型的使用场景。
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #381 +/- ##
=======================================
Coverage 99.74% 99.74%
=======================================
Files 7 7
Lines 776 793 +17
Branches 127 128 +1
=======================================
+ Hits 774 791 +17
Misses 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Problem Description
The
setTimeoutused in this component has a maximum delay limit. If thedurationexceeds this limit, it overflows and triggers immediately (treated as0ms).This causes issues for Ant Design's
MessageandNotificationcomponents (CodeSandbox):Proposed Solutions
I considered three approaches. This PR implements Option 2. Please let me know if you prefer a different approach.
0: The component will never close (duration = 0)setTimeoutlimit (closes after ~25 days)setTimeoutChanges
Impact
If the
durationfor aMessageorNotificationexceeds the limit:Summary by CodeRabbit
Release Notes
Bug Fixes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.