fix: resolve react-number-format compatibility by refining compositionEnd handling#147
Conversation
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
Walkthrough修正 compositionEnd 分支以先更新内部值再返回；用 createNormalizedEvent 替换克隆事件以确保发出的 onChange.target/currentTarget 指向真实挂载的 input；新增测试验证事件目标为真实元素。 变更Input 事件处理与规范化
序列图sequenceDiagram
participant User
participant InputComponent
participant createNormalizedEvent
participant RealInputElement
participant onChange
User->>InputComponent: 点击清除 / 输入 / compositionEnd
InputComponent->>createNormalizedEvent: 需要发出标准化事件 (origEvent, RealInputElement, targetValue)
createNormalizedEvent->>RealInputElement: 设置 value 为 targetValue
createNormalizedEvent-->>InputComponent: 返回 normalizedEvent (target/currentTarget -> RealInputElement)
InputComponent->>onChange: 调用 onChange(normalizedEvent)
代码审查工作量估计🎯 3 (Moderate) | ⏱️ ~20 minutes 可能相关的 PRs
建议评审者
诗歌
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 📝 Customizable high-level summaries are now available in beta!You can now customize how CodeRabbit generates the high-level summary in your pull requests — including its content, structure, tone, and formatting.
Example instruction:
Note: This feature is currently in beta for Pro-tier users, and pricing will be announced later. 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 @oraziospada17, 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 resolves a regression where Highlights
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 aims to fix a regression with react-number-format by ensuring compositionEnd events are processed, while still preventing the double onChange firing for IME input. The approach is to always update the internal state but only trigger onChange if the value has changed.
However, the implementation of the value check (cutValue !== formatValue) has a potential flaw. Due to React's event batching, formatValue may be stale across both compositionend and change event handlers, leading to onChange being fired twice. This could affect both uncontrolled and controlled components, re-introducing the bug the change is meant to solve. I've left a critical comment with a more detailed explanation.
| if (inputRef.current && cutValue !== formatValue) { | ||
| resolveOnChange(inputRef.current, e, onChange, cutValue); | ||
| } |
There was a problem hiding this comment.
This condition may not reliably prevent double onChange events during IME composition. Both the compositionend event and the subsequent change event can trigger triggerChange before the component re-renders. In this scenario, formatValue will hold the stale value from the previous render for both calls, causing this condition to be true twice and onChange to be fired twice.
This could re-introduce the issue this PR is trying to solve for compositionend scenarios (#46587). A useRef to track the last reported value within the event cycle might be a more robust solution to prevent the double-firing.
|
Hello, Are there any updates on this PR? Will it be merged? 🙏 I really need this change to fix antd input and react-number-format. There aren't any other masking libraries that work well with antd input. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/Input.tsx (1)
104-127:⚠️ Potential issue | 🟠 Major | ⚡ Quick wincompositionEnd 在 exceedFormatter 命中时仍会触发 onChange
当前
else if结构会导致info.source === 'compositionEnd'且超长裁剪命中时跳过早返回，随后在 Line 132 继续触发resolveOnChange，会把双触发问题带回这个分支。建议修复
- if ( + if ( !compositionRef.current && countConfig.exceedFormatter && countConfig.max && countConfig.strategy(currentValue) > countConfig.max ) { cutValue = countConfig.exceedFormatter(currentValue, { max: countConfig.max, }); if (currentValue !== cutValue) { setSelection([ inputRef.current?.selectionStart || 0, inputRef.current?.selectionEnd || 0, ]); } - } else if (info.source === 'compositionEnd') { + } + + if (info.source === 'compositionEnd') { // Always update internal state on compositionEnd, but skip onChange. // The browser fires an input/change event after compositionEnd, which // will call onInternalChange → triggerChange again and trigger onChange. // Skipping here prevents double-firing (`#46587`). setValue(cutValue); return; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/Input.tsx` around lines 104 - 127, The compositionEnd branch still allows resolveOnChange to fire when exceedFormatter trimmed the value; update the logic so that when countConfig.exceedFormatter runs and info.source === 'compositionEnd' you apply the trimmed value and return early to avoid later resolveOnChange calling onChange. Concretely, in the block that runs when !compositionRef.current && countConfig.exceedFormatter … (where cutValue is computed and setSelection is called), detect info.source === 'compositionEnd', call setValue(cutValue) and return (same as the existing compositionEnd branch) so resolveOnChange is skipped; keep references to compositionRef, countConfig.exceedFormatter, cutValue, setSelection, setValue, and info.source to locate the code.
🧹 Nitpick comments (1)
tests/index.test.tsx (1)
401-458: ⚡ Quick win建议补一条 compositionEnd + exceedFormatter 的回归用例
现有新测试覆盖了 target 归一化，但还缺少
compositionEnd与exceedFormatter同时发生时“只更新内部值、不额外触发 onChange”的断言。这个用例能直接兜住本 PR 的关键契约。可参考的测试补充
describe('onChange event target', () => { + it('should not emit extra onChange on compositionEnd when exceedFormatter is active', () => { + const onChange = jest.fn(); + const { container } = render( + <Input + count={{ max: 3, exceedFormatter: (val) => val.slice(0, 3) }} + onChange={onChange} + />, + ); + const input = container.querySelector('input')!; + + fireEvent.compositionStart(input); + fireEvent.compositionEnd(input, { data: 'abcdef' }); + fireEvent.change(input, { target: { value: 'abcdef' } }); + + expect(onChange).toHaveBeenCalledTimes(1); + expect(onChange.mock.calls[0][0].target.value).toBe('abc'); + });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/index.test.tsx` around lines 401 - 458, Add a regression test that verifies compositionEnd combined with exceedFormatter only updates internal value and does not invoke onChange: in tests/index.test.tsx add an it block (e.g., "compositionEnd with exceedFormatter should not trigger onChange") that renders <Input count={{ max: 3, exceedFormatter: val => val.slice(0,3) }} onChange={jest.fn()} />, selects the input element, simulate compositionstart (optional), set the input value to a long string via fireEvent.change or by setting value then fireEvent.compositionEnd, then assert onChange was not called and the input.value is trimmed to 'abc' (i.e., internal value updated but no onChange fired) to cover the compositionEnd + exceedFormatter contract for the Input component.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/Input.tsx`:
- Around line 104-127: The compositionEnd branch still allows resolveOnChange to
fire when exceedFormatter trimmed the value; update the logic so that when
countConfig.exceedFormatter runs and info.source === 'compositionEnd' you apply
the trimmed value and return early to avoid later resolveOnChange calling
onChange. Concretely, in the block that runs when !compositionRef.current &&
countConfig.exceedFormatter … (where cutValue is computed and setSelection is
called), detect info.source === 'compositionEnd', call setValue(cutValue) and
return (same as the existing compositionEnd branch) so resolveOnChange is
skipped; keep references to compositionRef, countConfig.exceedFormatter,
cutValue, setSelection, setValue, and info.source to locate the code.
---
Nitpick comments:
In `@tests/index.test.tsx`:
- Around line 401-458: Add a regression test that verifies compositionEnd
combined with exceedFormatter only updates internal value and does not invoke
onChange: in tests/index.test.tsx add an it block (e.g., "compositionEnd with
exceedFormatter should not trigger onChange") that renders <Input count={{ max:
3, exceedFormatter: val => val.slice(0,3) }} onChange={jest.fn()} />, selects
the input element, simulate compositionstart (optional), set the input value to
a long string via fireEvent.change or by setting value then
fireEvent.compositionEnd, then assert onChange was not called and the
input.value is trimmed to 'abc' (i.e., internal value updated but no onChange
fired) to cover the compositionEnd + exceedFormatter contract for the Input
component.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d58026a4-3124-4a34-9fb3-993957f028f3
📒 Files selected for processing (3)
src/Input.tsxsrc/utils/commonUtils.tstests/index.test.tsx
afc163
force-pushed
the
fix/react-number-format-compat
branch
from
9904c82 to
96eb6b8
Compare
…ouble-firing (#46587) - Replace cloneEvent (which uses cloneNode creating detached DOM nodes) with createNormalizedEvent that sets target.value directly and points target/currentTarget at the real mounted input element. This fixes document.contains(e.target) returning false for third-party wrappers like react-number-format. - In triggerChange, use compositionEnd early-return with setValue to prevent double-firing of onChange. The browser's subsequent input event will handle the onChange call naturally. - Add regression tests for onChange event target being the real mounted element across clear, normal typing, and exceedFormatter scenarios.
afc163
force-pushed
the
fix/react-number-format-compat
branch
from
96eb6b8 to
428c817
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #147 +/- ##
==========================================
- Coverage 98.34% 98.30% -0.05%
==========================================
Files 11 11
Lines 424 413 -11
Branches 137 134 -3
==========================================
- Hits 417 406 -11
Misses 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
Fixes ant-design/ant-design#46999
Problem
The fix for #46587 (preventing double onChange firing with Chinese input) introduced a regression where
compositionEndevents were completely skipped. This broke compatibility with libraries likereact-number-formatwhich rely on receiving these events to function correctly when wrapping the Input component.Solution
Instead of early returning when
source === 'compositionEnd', this PR modifies triggerChange to:setValue).onChangecallback if the value has actually changed.This approach solves both issues:
react-number-formatto work (solving #46999) because it now receives the necessary events.Summary by CodeRabbit
发布说明
Bug Fixes
Tests