fix(select): merge raw trigger element event handlers#1215
Conversation
|
@QDyanbing is attempting to deploy a commit to the React Component Team on Vercel. A member of the Team first needs to authorize it. |
Summary of ChangesHello @QDyanbing, 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! 此拉取请求解决了 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
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
|
概述在 SelectInput 组件中添加了一个 RootComponent props 合并层，将原始 props 与 DOM props 合并，对于两端都是函数的 props 进行回调组合，使原始回调和 DOM 回调都能被调用。同时增加了测试覆盖以验证自定义输入元素的事件处理器不会被覆盖。 变更内容
可能相关的 PR
建议的审阅者
兔子之诗
🎯 2 (Simple) | ⏱️ ~12 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
| const originProps = (RootComponent as any).props || {}; | ||
| const mergedProps = { ...originProps, ...domProps }; | ||
|
|
||
| Object.keys(originProps).forEach((key) => { | ||
| const originVal = originProps[key]; | ||
| const domVal = domProps[key]; | ||
|
|
||
| if (typeof originVal === 'function' && typeof domVal === 'function') { | ||
| mergedProps[key] = (...args: any[]) => { | ||
| domVal(...args); | ||
| originVal(...args); | ||
| }; | ||
| } | ||
| }); | ||
|
|
||
| if (React.isValidElement<any>(RootComponent)) { | ||
| return React.cloneElement(RootComponent, { | ||
| ...domProps, | ||
| ...mergedProps, | ||
| ref: composeRef((RootComponent as any).ref, rootRef), | ||
| }); | ||
| } | ||
|
|
||
| return <RootComponent {...domProps} ref={rootRef} />; | ||
| return <RootComponent {...mergedProps} ref={rootRef} />; |
There was a problem hiding this comment.
处理 RootComponent 的逻辑可以进行重构以提高代码清晰度和类型安全。通过首先检查 RootComponent 是否为 ReactElement，我们可以将元素和组件类型的处理逻辑分开。这避免了在 RootComponent 是组件类型时不必要地执行属性合并逻辑，同时也移除了 any 类型断言，使代码更健壮且易于维护。
if (React.isValidElement(RootComponent)) {
const originProps = RootComponent.props || {};
const mergedProps = { ...originProps, ...domProps };
Object.keys(originProps).forEach(key => {
const originVal = originProps[key];
const domVal = domProps[key];
if (typeof originVal === 'function' && typeof domVal === 'function') {
mergedProps[key] = (...args: any[]) => {
domVal(...args);
originVal(...args);
};
}
});
return React.cloneElement(RootComponent, {
...mergedProps,
ref: composeRef(RootComponent.ref, rootRef),
});
}
return <RootComponent {...domProps} ref={rootRef} />;
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Custom.test.tsx (1)
48-49: 断言建议改为精确调用次数，防止重复触发漏检当前断言只校验“至少调用一次”，如果未来出现重复触发也会通过。建议改为
toHaveBeenCalledTimes(1)。✅ 建议修改
- expect(onFocus).toHaveBeenCalled(); - expect(onBlur).toHaveBeenCalled(); + expect(onFocus).toHaveBeenCalledTimes(1); + expect(onBlur).toHaveBeenCalledTimes(1);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/Custom.test.tsx` around lines 48 - 49, The assertions in Custom.test.tsx use toHaveBeenCalled() which only checks "at least once" and can miss duplicated triggers; update the expectations for the mocks onFocus and onBlur to assert exact invocation counts by replacing expect(onFocus).toHaveBeenCalled() and expect(onBlur).toHaveBeenCalled() with expect(onFocus).toHaveBeenCalledTimes(1) and expect(onBlur).toHaveBeenCalledTimes(1) so the test fails if handlers fire multiple times.
🤖 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/SelectInput/index.tsx`:
- Around line 225-235: The current merging loop in SelectInput (iterating
Object.keys(originProps) and combining any same-named functions into
mergedProps) wrongly combines non-event functions; change the condition to only
compose keys that are event handlers (e.g., match /^on[A-Z]/) and where both
originProps[key] and domProps[key] are functions, leaving other same-named
functions untouched; use the existing symbols originProps, domProps, mergedProps
and only create the combined wrapper for keys matching the event pattern so
non-event functions that rely on return values are not altered.
---
Nitpick comments:
In `@tests/Custom.test.tsx`:
- Around line 48-49: The assertions in Custom.test.tsx use toHaveBeenCalled()
which only checks "at least once" and can miss duplicated triggers; update the
expectations for the mocks onFocus and onBlur to assert exact invocation counts
by replacing expect(onFocus).toHaveBeenCalled() and
expect(onBlur).toHaveBeenCalled() with expect(onFocus).toHaveBeenCalledTimes(1)
and expect(onBlur).toHaveBeenCalledTimes(1) so the test fails if handlers fire
multiple times.
| Object.keys(originProps).forEach((key) => { | ||
| const originVal = originProps[key]; | ||
| const domVal = domProps[key]; | ||
|
|
||
| if (typeof originVal === 'function' && typeof domVal === 'function') { | ||
| mergedProps[key] = (...args: any[]) => { | ||
| domVal(...args); | ||
| originVal(...args); | ||
| }; | ||
| } | ||
| }); |
There was a problem hiding this comment.
仅合并事件处理函数，避免误合并普通函数属性
当前会合并所有同名函数属性，不仅是事件处理器。这样可能误伤非事件函数（例如依赖返回值的函数），与本次修复目标不一致。建议只对事件键（如 onFocus/onBlur）做组合调用。
🔧 建议修改
Object.keys(originProps).forEach((key) => {
const originVal = originProps[key];
const domVal = domProps[key];
- if (typeof originVal === 'function' && typeof domVal === 'function') {
+ const isEventHandler = /^on[A-Z]/.test(key);
+ if (isEventHandler && typeof originVal === 'function' && typeof domVal === 'function') {
mergedProps[key] = (...args: any[]) => {
domVal(...args);
originVal(...args);
};
}
});📝 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.
| Object.keys(originProps).forEach((key) => { | |
| const originVal = originProps[key]; | |
| const domVal = domProps[key]; | |
| if (typeof originVal === 'function' && typeof domVal === 'function') { | |
| mergedProps[key] = (...args: any[]) => { | |
| domVal(...args); | |
| originVal(...args); | |
| }; | |
| } | |
| }); | |
| Object.keys(originProps).forEach((key) => { | |
| const originVal = originProps[key]; | |
| const domVal = domProps[key]; | |
| const isEventHandler = /^on[A-Z]/.test(key); | |
| if (isEventHandler && typeof originVal === 'function' && typeof domVal === 'function') { | |
| mergedProps[key] = (...args: any[]) => { | |
| domVal(...args); | |
| originVal(...args); | |
| }; | |
| } | |
| }); |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/SelectInput/index.tsx` around lines 225 - 235, The current merging loop
in SelectInput (iterating Object.keys(originProps) and combining any same-named
functions into mergedProps) wrongly combines non-event functions; change the
condition to only compose keys that are event handlers (e.g., match /^on[A-Z]/)
and where both originProps[key] and domProps[key] are functions, leaving other
same-named functions untouched; use the existing symbols originProps, domProps,
mergedProps and only create the combined wrapper for keys matching the event
pattern so non-event functions that rely on return values are not altered.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #1215 +/- ##
=======================================
Coverage 99.43% 99.43%
=======================================
Files 31 31
Lines 1239 1248 +9
Branches 427 451 +24
=======================================
+ Hits 1232 1241 +9
Misses 7 7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
2 participants
🐞 Bug 修复
问题背景
在使用
getRawInputElement提供自定义触发元素时，rc-select 会对该元素进行clone并注入内部的domProps。当前实现中，如果自定义元素与
domProps中存在同名事件处理函数（如onFocus、onBlur等），内部函数会直接覆盖用户传入的函数，导致用户回调无法触发。修复内容
在克隆自定义触发元素时：
domProps为基础，保证内部逻辑正常执行；该修改不会影响现有功能逻辑，仅修复事件被覆盖的问题。
测试说明
新增测试用例，确保：
onFocus/onBlur等事件能够正常触发；Summary by CodeRabbit
发布说明
Bug Fixes
Tests