-
Notifications
You must be signed in to change notification settings - Fork 42
refactor: Upgrade utils and replace useMergedState #74
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
refactor: Upgrade utils and replace useMergedState #74
Conversation
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. Walkthrough本次变更将 TextArea 与 ResizableTextArea 的状态管理从 useMergedState 迁移为 useControlledState(调用形式 useControlledState(defaultValue, value)),内部值以 internalValue ?? '' 作为渲染值;同时将 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor 用户
participant TextArea
participant Props as props (value/defaultValue)
participant State as useControlledState
rect rgba(200,230,255,0.25)
note over TextArea,State: 新:使用 useControlledState(defaultValue, value)
用户->>TextArea: 输入 / 触发 onChange
TextArea->>State: setMergedValue(internalValue)
State-->>TextArea: 返回 internalValue (受控或非受控值)
TextArea-->>用户: 渲染 value = internalValue ?? ''
alt 受控
Props-->>State: 外部 value 变化 -> hook 返回更新值
else 非受控
note right of State: 内部 state 驱动,外部不干预
end
end
sequenceDiagram
autonumber
participant RTA as ResizableTextArea
participant State as useControlledState
participant Auto as AutoSize/Resize
RTA->>State: 初始化 useControlledState(defaultValue, value)
RTA->>Auto: 计算高度 / 请求 resize
Auto-->>RTA: 返回高度调整
RTA->>State: onInternalChange -> setMergedValue(...)
RTA-->>Render: 使用 (internalValue ?? '') 渲染文本框
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Pre-merge checks (3 passed)✅ Passed checks (3 passed)
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
✨ Finishing Touches
🧪 Generate unit tests
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Summary of Changes
Hello @EmilyyyLiu, 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!
此 PR 主要通过将 TextArea
和 ResizableTextArea
组件中的状态管理钩子从 useMergedState
迁移到 useControlledState
来进行重构。同时,相关的 @rc-component/util
库也得到了升级。这一系列改动旨在统一受控/非受控组件的状态管理逻辑,并与关联的 issue 保持一致。
Highlights
- 依赖升级: 升级了
@rc-component/util
依赖库至^1.3.0
版本。 - 状态管理钩子替换: 在
src/ResizableTextArea.tsx
中,将useMergedState
替换为useControlledState
,并调整了默认值处理逻辑。 - 状态管理钩子替换: 在
src/TextArea.tsx
中,将useMergedState
替换为useControlledState
,并确保值始终为字符串类型。 - 快照更新: 更新了测试快照,为清除按钮添加了
tabindex="-1"
属性,这可能是一个辅助功能改进。
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command>
or @gemini-code-assist <command>
. Below is a summary of the supported commands.
Feature | Command | Description |
---|---|---|
Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
Help | /gemini help |
Displays a list of available commands. |
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 .gemini/
folder in the base of the repository. Detailed instructions can be found here.
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
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #74 +/- ##
==========================================
- Coverage 99.03% 99.00% -0.04%
==========================================
Files 3 3
Lines 208 201 -7
Branches 64 63 -1
==========================================
- Hits 206 199 -7
Misses 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
你好,感谢你的贡献。这次重构将 useMergedState
替换为 useControlledState
是一个很好的改进。
我注意到在两个文件中,新的实现方式可能会在处理 0
这样的 falsy 值时引入 bug,导致它们被错误地转换为空字符串。我已经针对这些问题提出了具体的代码修改建议。
请查看我的评论并应用建议的修复。除此之外,其他部分的修改看起来不错。
src/ResizableTextArea.tsx
Outdated
const [mergedValue, setMergedValue] = useControlledState( | ||
defaultValue || '', | ||
value || '', | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
你好,这里的 useControlledState
用法不正确。
defaultValue || ''
和 value || ''
会将 0
这样的falsy值转换为空字符串 ''
,从而导致bug。例如,当 value
为 0
时,文本框会显示为空。
为了正确地替换 useMergedState
并保留 postState: (val) => val ?? ''
的行为(即当值为 null
或 undefined
时回退到空字符串),建议进行如下修改:
const [rawMergedValue, setMergedValue] = useControlledState(defaultValue, value);
const mergedValue = rawMergedValue ?? '';
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/TextArea.tsx (1)
48-52
: 状态管理重构实现使用
useControlledState
替换之前的状态管理,并通过const value = internelValue || ''
确保值始终为字符串。这个实现是合理的,但变量名internelValue
似乎是internalValue
的拼写错误。考虑修正变量名的拼写:
- const [internelValue, setValue] = useControlledState( + const [internalValue, setValue] = useControlledState( defaultValue, customValue, ); - const value = internelValue || ''; + const value = internalValue || '';
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
tests/__snapshots__/allowClear.test.tsx.snap
is excluded by!**/*.snap
📒 Files selected for processing (3)
package.json
(1 hunks)src/ResizableTextArea.tsx
(2 hunks)src/TextArea.tsx
(2 hunks)
🔇 Additional comments (5)
package.json (1)
49-49
: 升级依赖版本以支持新的钩子函数将
@rc-component/util
从^1.2.0
升级到^1.3.0
是合理的,这个版本包含了代码中使用的useControlledState
钩子函数。src/ResizableTextArea.tsx (2)
2-2
: 引入新的状态管理钩子函数正确引入了
useControlledState
来替换useMergedState
。
5-5
: 导入语句调整
classNames
的导入位置调整是合理的,保持了代码的整洁性。src/TextArea.tsx (2)
5-5
: 引入新的状态管理钩子函数正确引入了
useControlledState
来替换之前的状态管理方式。
198-198
: 统一使用 clsx 进行类名处理在多个位置使用了
clsx
进行类名的条件拼接,这是一个好的实践,可以提供更好的性能和更简洁的语法。Also applies to: 226-226, 233-233, 257-257
src/ResizableTextArea.tsx
Outdated
const [mergedValue, setMergedValue] = useControlledState( | ||
defaultValue || '', | ||
value || '', | ||
); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
验证状态管理重构的正确性
从 useMergedState
迁移到 useControlledState
的实现看起来正确,使用 defaultValue || ''
和 value || ''
来确保空值时回退为空字符串。不过需要验证 useControlledState
是否与原来的 useMergedState
有相同的行为。
请运行以下脚本来验证 useControlledState
的 API 是否与 useMergedState
兼容:
🌐 Web query:
useControlledState hook from @rc-component/util API documentation
💡 Result:
Summary — useControlledState (from @rc-component/util)
Signature (TypeScript):
useControlledState<T, R = T>(
defaultStateValue: T | (() => T),
option?: {
defaultValue?: T | (() => T);
value?: T;
onChange?: (value: T, prevValue: T) => void;
postState?: (value: T) => T;
}
): [R, (value: T) => void]. [1][2]
Behavior:
- Returns [state, setState].
- state: current value (possibly post-processed by postState).
- setState: updater that calls onChange when controlled; otherwise updates internal state.
- Options let you provide controlled value, default value, onChange handler, and a postState transformer. [1][2]
Example usage pattern:
const [value, setValue] = useControlledState(defaultValue, { value: controlledValue, onChange, postState }); [1][3]
Sources:
[1] @rc-component/util (npm package API).
[2] rc-util type definitions (unpkg / useMergedState .d.ts).
[3] Demo / docs for useControlledState implementation pattern.
修复 useControlledState 调用签名
在 src/ResizableTextArea.tsx (35-38) 中,第二个参数必须为选项对象 { defaultValue?: T, value?: T, onChange?: (value, prev) => void, postState?: fn }
,当前传入字符串会导致类型不匹配与运行时异常 。请改为:
useControlledState(defaultValue || '', {
value,
onChange: /* 对应的更新回调 */,
});
🤖 Prompt for AI Agents
In src/ResizableTextArea.tsx around lines 35 to 38, the call to
useControlledState currently passes two strings which is an incorrect signature;
replace the second string argument with the options object expected by
useControlledState (e.g. { value, onChange: /* handler that updates state/prop
*/, defaultValue: /* if needed */, postState: /* optional transformer */ }) so
the hook receives { defaultValue?, value?, onChange?, postState? } and wire the
onChange to the component's update callback to keep controlled/uncontrolled
behavior correct.
import { resolveOnChange } from '@rc-component/input/lib/utils/commonUtils'; | ||
import useMergedState from '@rc-component/util/lib/hooks/useMergedState'; | ||
import useControlledState from '@rc-component/util/lib/hooks/useControlledState'; | ||
import clsx from 'classnames'; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Verification agent
🧩 Analysis chain
导入 clsx 而非 classnames
注意这里使用了 clsx
而不是 classnames
,但在 package.json
中看到的依赖是 classnames
。需要确认 clsx
是否可用或者是否应该保持使用 classnames
。
请运行以下脚本来检查项目中 clsx
的可用性:
🏁 Script executed:
#!/bin/bash
# 检查 clsx 是否在依赖中或者是否为 classnames 的别名
echo "检查 package.json 中的 clsx 依赖:"
cat package.json | jq '.dependencies.clsx // "not found"'
echo "检查 node_modules 中是否有 clsx:"
fd "clsx" node_modules --type d --max-depth 2 | head -5
Length of output: 66
🏁 Script executed:
#!/bin/bash
# 检查 package.json 中的 classnames 依赖:
echo "检查 package.json 中的 classnames 依赖:"
cat package.json | jq '.dependencies.classnames // "not found"'
# 检查 node_modules 中是否有 classnames:
echo "检查 node_modules 中是否有 classnames:"
fd "classnames" node_modules --type d --max-depth 2 | head -5
Length of output: 75
安装或重命名
依赖中没有 clsx
包,只有 classnames
。请确认是要安装并使用 clsx
(npm install clsx),还是将 import 改为 import classNames from 'classnames'
以消除命名混淆。
🤖 Prompt for AI Agents
In src/TextArea.tsx around line 6 the import uses `clsx` but the project has
only the `classnames` package; either install `clsx` (npm install clsx / yarn
add clsx) and keep the import, or change the import to `import classNames from
'classnames'` and update any subsequent usages (rename `clsx(...)` to
`classNames(...)`) to remove the naming mismatch.
src/TextArea.tsx
Outdated
}); | ||
customValue, | ||
); | ||
const value = internalValue || ''; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
这个看起来不需要
关联issue:ant-design/ant-design#54854
替换 useMergedState 为 useControlledState
Summary by CodeRabbit