Conversation
dosubot
bot
added
the
size:M
|
|
Summary of ChangesHello @lzm0x219, 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. 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)sequenceDiagram
participant User
participant SelectedDemo
participant Picker
User->>SelectedDemo: 点击打开 Picker
SelectedDemo->>Picker: render with props(selectValue, onSelect, onConfirm)
Picker->>SelectedDemo: onSelect(tempVal)
SelectedDemo->>SelectedDemo: 更新 selectValue（临时显示）
User->>Picker: 确认选择
Picker->>SelectedDemo: onConfirm(finalVal)
SelectedDemo->>SelectedDemo: 将 finalVal 设为 value（持久）
Estimated code review effort🎯 3 (中等) | ⏱️ ~20 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}
There was a problem hiding this comment.
Code Review
本次 PR 旨在为 Picker 组件添加 selectValue 属性以实现对临时选中值的手动控制，这是一个很好的功能增强。代码实现上通过 usePropsValue 来统一处理受控与非受控逻辑，思路是正确的。但在目前的实现中，我发现了一些问题：
- 用于重置选择器状态的
useEffect钩子没有考虑到selectValue受控的情况，这会导致外部传入的selectValue值被意外覆盖，属于比较严重的逻辑缺陷。 onSelect回调会在每次选择时被触发两次，这可能导致不符合预期的行为和性能问题。
我在代码中留下了具体的修改建议，希望能帮助你完善这个功能。
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/components/picker/picker.tsx (2)
39-77:selectValue属性未在PickerProps类型中声明。代码在第 134 行使用了
props.selectValue，但PickerProps类型定义中缺少该属性声明，这会导致 TypeScript 类型错误。🐛 建议修复
export type PickerProps = { columns: PickerColumn[] | ((value: PickerValue[]) => PickerColumn[]) value?: PickerValue[] defaultValue?: PickerValue[] + selectValue?: PickerValue[] loading?: boolean loadingContent?: ReactNode onSelect?: (value: PickerValue[], extend: PickerValueExtend) => void
133-158:onSelect被重复调用两次。存在逻辑冲突：
usePropsValue的onChange回调（第 136-139 行）会在setInnerValue时触发props.onSelectonChange函数（第 156 行）在visible为 true 时也会调用props.onSelect这导致每次选择变更时
onSelect被调用两次。🐛 建议修复：移除 onChange 中的重复调用
const [innerValue, setInnerValue] = usePropsValue({ value: props.selectValue, defaultValue: value, onChange: val => { const extend = generateColumnsExtend(props.columns, val) props.onSelect?.(val, extend) }, }) useEffect(() => { if (innerValue !== value) { setInnerValue(value) } }, [visible]) useEffect(() => { if (!visible) { setInnerValue(value) } }, [value]) const onChange = useMemoizedFn((val, ext) => { setInnerValue(val) - if (visible) { - props.onSelect?.(val, ext) - } })或者，如果需要保留原有的
onChange调用逻辑，则应移除usePropsValue中的onChange回调：🐛 备选方案：移除 usePropsValue 中的 onChange
const [innerValue, setInnerValue] = usePropsValue({ value: props.selectValue, defaultValue: value, - onChange: val => { - const extend = generateColumnsExtend(props.columns, val) - props.onSelect?.(val, extend) - }, })
🧹 Nitpick comments (1)
src/components/picker/picker.tsx (1)
142-146: 数组比较使用引用相等性，需确认是否符合预期。第 143 行
innerValue !== value使用引用相等性比较两个数组。如果数组内容相同但引用不同，条件仍为true。这在 React 中是常见做法，但请确认这是预期行为。
lzm0x219
force-pushed
the
feature/picker-selectvalue
branch
from
48a6f5f to
a15adf3
Compare
|
@gemini-code-assist /gemini review |
| const [innerValue, setInnerValue] = usePropsValue({ | ||
| value: props.selectValue, | ||
| defaultValue: value, | ||
| }) |
There was a problem hiding this comment.
将 useState 改为 usePropsValue 是实现 selectValue 受控功能的正确方向。但这个改动与后面（133-137行）的 useEffect 产生了冲突。
原有的 useEffect 会在 visible 状态改变时，尝试将 innerValue 重置为 value。当 selectValue 属性被使用时，innerValue 由外部控制，这个 useEffect 会错误地覆盖外部传入的值，尤其是在取消后重新打开选择器时，会导致 selectValue 的值被忽略，并可能引起界面闪烁。
为了修复这个问题，需要修改 133-137 行的 useEffect，使其在 props.selectValue 被定义（即受控模式）时不执行重置逻辑。例如：
useEffect(() => {
// 仅在非受控模式下，当 picker 打开时，重置 innerValue
if (props.selectValue === undefined && visible) {
if (innerValue !== value) {
setInnerValue(value);
}
}
}, [visible, value, innerValue, props.selectValue]);由于这个 useEffect 不在本次 diff 的修改范围内，我无法直接提供代码建议，但这是一个关键问题，需要在此处指出。
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/picker/picker.tsx (1)
142-146: 与上述相同的依赖数组问题。此处
[value]依赖数组未包含visible和setInnerValue，存在同样的 ESLint 规则警告风险。建议一并添加注释说明意图。
🤖 Fix all issues with AI agents
In `@src/components/picker/picker.tsx`:
- Around line 133-140: The useEffect that resets innerValue when the picker
opens reads innerValue and value but its dependency array only lists [visible,
props.selectValue], which triggers react-hooks/exhaustive-deps warnings and
risks stale closures; update the effect to either include the missing
dependencies (innerValue and value and setInnerValue) or explicitly document the
intent with an eslint-disable-next-line comment, or switch to a ref pattern
(e.g., valueRef.current) and reference valueRef in the effect; adjust the effect
around useEffect / props.selectValue / visible / innerValue / value /
setInnerValue accordingly so the linter is satisfied and closures are safe.
| useEffect(() => { | ||
| if (innerValue !== value) { | ||
| setInnerValue(value) | ||
| // 仅在非受控模式下，当 picker 打开时，重置 innerValue | ||
| if (props.selectValue === undefined && visible) { | ||
| if (innerValue !== value) { | ||
| setInnerValue(value) | ||
| } | ||
| } | ||
| }, [visible]) | ||
| }, [visible, props.selectValue]) |
There was a problem hiding this comment.
依赖数组不完整，可能引发 ESLint 警告或闭包陷阱。
useEffect 内部读取了 innerValue 和 value，但依赖数组仅包含 [visible, props.selectValue]。虽然当前逻辑意图明确（仅在 visible 或 selectValue 变化时触发），但这种写法：
- 会触发
react-hooks/exhaustive-deps规则警告 - 后续代码修改时可能产生意外的闭包问题
建议添加 eslint-disable 注释明确意图，或考虑使用 ref 来获取最新值：
🔧 建议的修复方案
方案 1：添加注释说明意图
useEffect(() => {
// 仅在非受控模式下，当 picker 打开时，重置 innerValue
if (props.selectValue === undefined && visible) {
if (innerValue !== value) {
setInnerValue(value)
}
}
+ // eslint-disable-next-line react-hooks/exhaustive-deps -- 仅在 visible 或 selectValue 变化时触发，intentionally 读取最新的 innerValue 和 value
}, [visible, props.selectValue])方案 2：使用 ref 保存最新值（如果项目有此模式）
const valueRef = useRef(value)
valueRef.current = value
useEffect(() => {
if (props.selectValue === undefined && visible) {
setInnerValue(valueRef.current)
}
}, [visible, props.selectValue, setInnerValue])📝 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.
| useEffect(() => { | |
| if (innerValue !== value) { | |
| setInnerValue(value) | |
| // 仅在非受控模式下，当 picker 打开时，重置 innerValue | |
| if (props.selectValue === undefined && visible) { | |
| if (innerValue !== value) { | |
| setInnerValue(value) | |
| } | |
| } | |
| }, [visible]) | |
| }, [visible, props.selectValue]) | |
| useEffect(() => { | |
| // 仅在非受控模式下，当 picker 打开时，重置 innerValue | |
| if (props.selectValue === undefined && visible) { | |
| if (innerValue !== value) { | |
| setInnerValue(value) | |
| } | |
| } | |
| // eslint-disable-next-line react-hooks/exhaustive-deps -- 仅在 visible 或 selectValue 变化时触发，intentionally 读取最新的 innerValue 和 value | |
| }, [visible, props.selectValue]) |
🤖 Prompt for AI Agents
In `@src/components/picker/picker.tsx` around lines 133 - 140, The useEffect that
resets innerValue when the picker opens reads innerValue and value but its
dependency array only lists [visible, props.selectValue], which triggers
react-hooks/exhaustive-deps warnings and risks stale closures; update the effect
to either include the missing dependencies (innerValue and value and
setInnerValue) or explicitly document the intent with an
eslint-disable-next-line comment, or switch to a ref pattern (e.g.,
valueRef.current) and reference valueRef in the effect; adjust the effect around
useEffect / props.selectValue / visible / innerValue / value / setInnerValue
accordingly so the linter is satisfied and closures are safe.
commit: |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7024 +/- ##
==========================================
+ Coverage 92.94% 92.96% +0.01%
==========================================
Files 337 337
Lines 7375 7376 +1
Branches 1879 1844 -35
==========================================
+ Hits 6855 6857 +2
+ Misses 512 511 -1
Partials 8 8 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
| columns: PickerColumn[] | ((value: PickerValue[]) => PickerColumn[]) | ||
| value?: PickerValue[] | ||
| defaultValue?: PickerValue[] | ||
| selectValue?: PickerValue[] |
There was a problem hiding this comment.
我的场景是打开 picker 组件后，需要动态指定 selected 值，目前组件无法满足的
There was a problem hiding this comment.
那和 antd 一样叫 defaultPickerValue 好了
There was a problem hiding this comment.
哦，我get 了。的确是 Picker 和 antd 的 DatePicker 不一样。我明白你说 selectValue 的意思。不过即便这样，selectValue 也应该是受控属性，需要 onSelect 改了才能改 selectValue
2 participants
功能描述
为
Picker组件添加了手动控制选中值的功能，通过新的selectValue属性允许用户以编程方式控制 Picker 组件中的临时选中值。改动说明
核心功能
selectValue属性：允许开发者通过传入此属性来手动控制 Picker 中的选中值，与value属性配合使用innerValue改为使用usePropsValuehook，更好地处理受控组件逻辑修改的文件
picker.tsx
selectValue到 PickerProps 类型定义usePropsValue替代useState来管理内部选中值demo1.tsx
RenderChildrenDemo2演示组件，展示如何通过外部操作（点击标题）来设置选中值onSelect回调中更新selectValue的用法使用示例
类型兼容性
✅ 向后兼容 -
selectValue属性为可选，不影响现有代码Summary by CodeRabbit
发布说明
✏️ Tip: You can customize this high-level summary in your review settings.