fix: add A → a replacement in dateFns localeParse for use12Hours#965
fix: add A → a replacement in dateFns localeParse for use12Hours#965daedalus28 wants to merge 2 commits intoreact-component:masterfrom
Conversation
When use12Hours is enabled, rc-picker internally injects uppercase A (moment.js AM/PM token) into the format string. date-fns uses lowercase a for AM/PM, so passing A through causes: RangeError: Format string contains an unescaped latin alphabet character `A` localeParse already converts other moment tokens (Y→y, D→d, etc.) but was missing A→a. This adds it. Fixes react-component#964
|
@daedalus28 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
Sequence Diagram(s)(本次变更属于小幅格式化修复与测试,交互组件不足以生成序列图,故省略。) Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Suggested reviewers
诗句
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
|
Could you add a test case? |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #965 +/- ##
=======================================
Coverage 98.81% 98.81%
=======================================
Files 65 65
Lines 2695 2695
Branches 724 749 +25
=======================================
Hits 2663 2663
Misses 29 29
Partials 3 3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Verifies that localeParse converts uppercase A (moment-style AM/PM) to lowercase a (date-fns) so that both format() and parse() work correctly with 12-hour time formats. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Done 👍 |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tests/generate.spec.tsx`:
- Around line 342-344: The comment incorrectly states the output will be
lowercase am/pm while the assertion expects uppercase "PM"; update the comment
in tests/generate.spec.tsx near the dateFnsGenerateConfig.locale.format('en_US',
date, 'YYYY-MM-DD hh:mm:ss A') test to explain that the test verifies token
mapping/format-token behavior (that the 'A' token is handled) rather than
asserting a case change of the am/pm output, so it doesn't claim lowercase
output.
🪄 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: 82e058b4-cb2d-4306-bedd-a042b8039e07
📒 Files selected for processing (1)
tests/generate.spec.tsx
| // Format with uppercase A (moment-style) should produce lowercase am/pm output | ||
| const formatted = dateFnsGenerateConfig.locale.format('en_US', date, 'YYYY-MM-DD hh:mm:ss A'); | ||
| expect(formatted).toEqual('2000-01-01 02:30:00 PM'); |
There was a problem hiding this comment.
注释里的预期大小写与断言不一致。
Line 342 说会输出小写 am/pm,但 Line 344 断言的是大写 PM。建议把注释改成说明转换的是 token,而不是输出大小写,避免后续维护时误读。
建议修改
- // Format with uppercase A (moment-style) should produce lowercase am/pm output
+ // Format with uppercase A (moment-style) should be normalized to date-fns `a`
const formatted = dateFnsGenerateConfig.locale.format('en_US', date, 'YYYY-MM-DD hh:mm:ss A');
expect(formatted).toEqual('2000-01-01 02:30:00 PM');📝 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.
| // Format with uppercase A (moment-style) should produce lowercase am/pm output | |
| const formatted = dateFnsGenerateConfig.locale.format('en_US', date, 'YYYY-MM-DD hh:mm:ss A'); | |
| expect(formatted).toEqual('2000-01-01 02:30:00 PM'); | |
| // Format with uppercase A (moment-style) should be normalized to date-fns `a` | |
| const formatted = dateFnsGenerateConfig.locale.format('en_US', date, 'YYYY-MM-DD hh:mm:ss A'); | |
| expect(formatted).toEqual('2000-01-01 02:30:00 PM'); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tests/generate.spec.tsx` around lines 342 - 344, The comment incorrectly
states the output will be lowercase am/pm while the assertion expects uppercase
"PM"; update the comment in tests/generate.spec.tsx near the
dateFnsGenerateConfig.locale.format('en_US', date, 'YYYY-MM-DD hh:mm:ss A') test
to explain that the test verifies token mapping/format-token behavior (that the
'A' token is handled) rather than asserting a case change of the am/pm output,
so it doesn't claim lowercase output.
Summary
.replace(/A/g, 'a')tolocaleParsein the date-fns generate configuse12Hourscrashing withRangeError: Format string contains an unescaped latin alphabet character AlocaleParsealready converts moment.js format tokens to date-fns equivalents (Y→y,D→d, etc.) but was missingA→a(AM/PM). Whenuse12Hoursis enabled, rc-picker injects uppercaseAinto the format string internally, which date-fns rejects.Fixes #964
Summary by CodeRabbit
Bug Fixes
Tests