feat: add isNumber function - #256
Conversation
|
🎊 PR Preview 814e198 has been successfully built and deployed to https://ant-design-cssinjs-preview-pr-256.surge.sh 🕐 Build time: 107.331s
🤖 By surge-preview |
Summary of ChangesHello, 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 introduces a dedicated 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. Changelog
Activity
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
|
📝 WalkthroughWalkthrough引入并导出新的类型守卫函数 Changes
Sequence Diagram(s)（无） Estimated code review effort🎯 2 (简洁) | ⏱️ ~12 分钟 Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 introduces a helpful isNumber utility function to differentiate between numbers and NaN. While this is a good improvement, its application has inadvertently created a critical issue in the CSSCalculator constructor where NaN values are no longer handled, leaving this.result uninitialized. Additionally, there are a couple of medium-severity type-safety concerns in the updated unit functions, which can now return a number type (NaN) where a string is expected. My review includes specific suggestions to address these points and ensure the code is robust and type-safe.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/theme/calc/CSSCalculator.ts`:
- Line 1: The import "import { isNumber } from '@/util';" in CSSCalculator.ts
uses the alias "@/util" which isn't resolvable in CI; replace that alias with a
repository-resolvable relative import that points to the actual util module
(i.e., update the import for the isNumber symbol to the correct relative path
within the repo so the bundler/TS resolver can find it).
- Around line 34-35: The constructor currently only assigns this.result when
isNumber(num) is true, so NaN (typeof number but not isNumber) falls through and
leaves this.result unset; update the branch so numeric-typed inputs are handled
explicitly (e.g. add an else if (typeof num === 'number') branch) and assign
this.result = unit(num) there (referencing isNumber, unit, and this.result) to
make the constructor behavior consistent with add/sub paths.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: ec7e0144-9e6b-4348-8f63-0f304c6c0bc3
📒 Files selected for processing (2)
src/theme/calc/CSSCalculator.tssrc/util/index.ts
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #256 +/- ##
=======================================
Coverage 94.74% 94.75%
=======================================
Files 32 32
Lines 2873 2878 +5
Branches 450 452 +2
=======================================
+ Hits 2722 2727 +5
Misses 151 151 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/theme/calc/CSSCalculator.ts (1)
34-35:⚠️ Potential issue | 🟠 Major
NaN在构造路径仍会落空，导致行为与add/sub不一致。
isNumber(num)会排除NaN，因此构造函数不会给this.result赋值；但add/sub对number分支仍会接收NaN并拼接成字符串，前后路径不一致，可能产生空结果或异常表达式。建议补一个numType === 'number'的兜底分支。🔧 建议修复
if (num instanceof CSSCalculator) { this.result = `(${num.result})`; } else if (isNumber(num)) { this.result = unit(num); + } else if (numType === 'number') { + this.result = `${num}`; } else if (numType === 'string') { this.result = num as string; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/theme/calc/CSSCalculator.ts` around lines 34 - 35, The constructor in CSSCalculator leaves this.result unset when num is NaN because isNumber(num) excludes NaN, causing inconsistent behavior with add/sub which accept NaN; update the constructor of class CSSCalculator to include a fallback branch checking numType === 'number' (or otherwise detecting numeric inputs including NaN) and assign this.result = unit(num) there so NaN follows the same string-path logic as add/sub; reference the constructor, isNumber, numType, unit, add, sub and this.result when making the change so the numeric (including NaN) path is handled consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/theme/calc/CSSCalculator.ts`:
- Around line 34-35: The constructor in CSSCalculator leaves this.result unset
when num is NaN because isNumber(num) excludes NaN, causing inconsistent
behavior with add/sub which accept NaN; update the constructor of class
CSSCalculator to include a fallback branch checking numType === 'number' (or
otherwise detecting numeric inputs including NaN) and assign this.result =
unit(num) there so NaN follows the same string-path logic as add/sub; reference
the constructor, isNumber, numType, unit, add, sub and this.result when making
the change so the numeric (including NaN) path is handled consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 910ff932-e2a8-40b4-b653-081ee87bf36f
📒 Files selected for processing (1)
src/theme/calc/CSSCalculator.ts
2 participants
Summary by CodeRabbit
发行说明