fix: keep onChange target mounted when value does not need cloning#175
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
Walkthrough在 Changes事件目标归一化处理
🎯 2 (Simple) | ⏱️ ~12 分钟
🚥 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)
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.
🧹 Nitpick comments (2)
src/utils/commonUtils.ts (1)
48-56: ⚡ Quick win建议添加注释说明函数用途
新增的
cloneEventWithTarget函数与现有的cloneEvent函数在命名和签名上相似，但用途不同。建议添加注释说明：
- 该函数仅替换事件的 target/currentTarget 引用，不克隆元素也不修改值
- 适用场景：当 targetValue 与当前值相同时，避免不必要的元素克隆，保持事件目标指向已挂载的 DOM 元素
这有助于未来维护者理解两个函数的使用场景差异。
📝 建议添加的注释
+/** + * Clone event with target reference only (without cloning the element or modifying value). + * Use when the target value is already correct and we just need to preserve the mounted element reference. + */ function cloneEventWithTarget< EventType extends React.SyntheticEvent<any, any>, Element extends HTMLInputElement | HTMLTextAreaElement, >(event: EventType, target: Element): EventType {🤖 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/utils/commonUtils.ts` around lines 48 - 56, Add a clear JSDoc comment above cloneEventWithTarget explaining that it only replaces the event.target and event.currentTarget references (does not deep-clone the element or change its value), and describe intended use: prefer this when the targetValue equals the current value to avoid unnecessary DOM element cloning and to keep the event targeting an already-mounted element; also note the distinction versus the existing cloneEvent function so maintainers know when to use each. Ensure the comment mentions the function signature (cloneEventWithTarget<EventType, Element>) and the specific behavior of returning a new event object with target/currentTarget replaced.tests/index.test.tsx (1)
442-453: ⚡ Quick win建议增强测试以覆盖新增的代码路径
当前测试验证了基础 onChange 场景（无 formatter 或 targetValue），但未覆盖此 PR 新增的核心逻辑路径（
commonUtils.ts第103行）。新逻辑在resolveOnChange被调用且targetValue等于当前值时才会执行。建议增加测试用例直接调用
resolveOnChange并传入与当前值相同的targetValue，以验证在该场景下事件目标仍指向已挂载元素。可以参考第528-539行已有的resolveOnChange单元测试模式。💡 建议添加的测试用例
it('resolveOnChange should preserve mounted target when targetValue equals current value', () => { const onChange = jest.fn(); const { container } = render(<textarea defaultValue="test" />); const textarea = container.querySelector('textarea')!; const mockEvent = { type: 'compositionEnd', target: textarea, currentTarget: textarea, } as React.CompositionEvent<HTMLTextAreaElement>; resolveOnChange(textarea, mockEvent, onChange, 'test'); expect(onChange).toHaveBeenCalled(); const event = onChange.mock.calls[0][0]; expect(event.target).toBe(textarea); expect(event.currentTarget).toBe(textarea); expect(document.contains(event.target)).toBe(true); });🤖 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 442 - 453, Add a unit test that calls resolveOnChange directly to exercise the new branch where targetValue equals the element's current value (the logic added around commonUtils.ts line 103): render a mounted input/textarea, create a mock CompositionEvent-like object with target/currentTarget set to that element, invoke resolveOnChange(element, mockEvent, onChange, '<same-value>'), and assert onChange was called and the emitted event's target and currentTarget are the mounted element and still present in document (document.contains(event.target) is true).
🤖 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.
Nitpick comments:
In `@src/utils/commonUtils.ts`:
- Around line 48-56: Add a clear JSDoc comment above cloneEventWithTarget
explaining that it only replaces the event.target and event.currentTarget
references (does not deep-clone the element or change its value), and describe
intended use: prefer this when the targetValue equals the current value to avoid
unnecessary DOM element cloning and to keep the event targeting an
already-mounted element; also note the distinction versus the existing
cloneEvent function so maintainers know when to use each. Ensure the comment
mentions the function signature (cloneEventWithTarget<EventType, Element>) and
the specific behavior of returning a new event object with target/currentTarget
replaced.
In `@tests/index.test.tsx`:
- Around line 442-453: Add a unit test that calls resolveOnChange directly to
exercise the new branch where targetValue equals the element's current value
(the logic added around commonUtils.ts line 103): render a mounted
input/textarea, create a mock CompositionEvent-like object with
target/currentTarget set to that element, invoke resolveOnChange(element,
mockEvent, onChange, '<same-value>'), and assert onChange was called and the
emitted event's target and currentTarget are the mounted element and still
present in document (document.contains(event.target) is true).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 0054ef43-e3cc-45c5-98e6-2a8f3b3350fb
📒 Files selected for processing (2)
src/utils/commonUtils.tstests/index.test.tsx
![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 new utility function cloneEventWithTarget and updates resolveOnChange to ensure the event target is correctly preserved when an input value is unchanged by a formatter. A new test case has been added to verify this behavior. The review feedback suggests refactoring the logic in resolveOnChange to improve readability and avoid duplicating the onChange call.
Co-authored-by: gemini-code-assist[bot] <176961590+gemini-code-assist[bot]@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #175 +/- ##
==========================================
- Coverage 98.34% 98.13% -0.22%
==========================================
Files 11 11
Lines 424 428 +4
Branches 137 133 -4
==========================================
+ Hits 417 420 +3
- Misses 7 8 +1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This PR is being reverted in #176. The original issue (ant-design/ant-design#46999) has already been fixed in antd v6 — See ant-design/ant-design#46999 (comment) for details. |
2 participants
Problem
resolveOnChangecloned the input element whenevertargetValuewas provided, even when the live input already had that value. This makes ordinaryonChangeevents expose a detachedevent.target.Some custom input integrations, including the
react-number-formatcase linked from ant-design/ant-design#46999, expectevent.targetto be the mounted input element.Solution
Only use the existing cloned-event path when the reported
targetValuediffers from the live input value. When the values already match, preserve a stable synthetic event wrapper but pointtargetandcurrentTargetat the mounted input.This keeps the existing synthetic-value behavior for clear/composition/formatter paths while avoiding detached targets for ordinary input changes.
Verification
onChangetargets.pnpm test -- --runInBand tests/index.test.tsxpnpm lintpnpm exec prettier --check src/utils/commonUtils.ts tests/index.test.tsxFixes ant-design/ant-design#46999
Summary by CodeRabbit
发布说明
Bug Fixes
Tests