refactor: Upgrade utils and replace useMergedState #793
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将 Menu 的内部受控状态从 useMergedState 切换为 useControlledState，移除自定义 useUUID 并改用 @rc-component/util 的 useId；同时升级 package.json 中 @rc-component/util 版本，并在 IdContext.getMenuId 中移除对 undefined 的特判。对外签名未变更。 Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor User as 用户
participant Menu as Menu
participant C as useControlledState
participant I as useId (@rc-component/util)
participant CB as 回调 (onOpenChange/onSelect)
participant IdCtx as IdContext
User->>Menu: 交互（展开/选择/聚焦）
Menu->>C: 读/写 openKeys / activeKey / selectedKeys
alt props 提供受控值
C-->>Menu: 返回受控 props 值
else 非受控（内部状态）
C-->>Menu: 返回并更新内部值
end
Menu->>I: 请求或生成 uuid (useId)
I-->>Menu: 返回 uuid
Menu->>IdCtx: 计算 menuId via getMenuId(uuid, eventKey)
IdCtx-->>Menu: 返回 `${uuid}-${eventKey}` （注意：uuid 可为 undefined）
Menu->>CB: 触发 onOpenChange / onSelect（使用 mergedOpenKeys/mergedSelectKeys）
note right of Menu: 已删除本地 useUUID，改为 useId；状态管理已切换为 useControlledState
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
Pre-merge checks (2 passed, 1 warning)❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
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!
此拉取请求通过升级核心工具包并用 useControlledState 替换 useMergedState，重构了项目中的状态管理逻辑。此更改旨在标准化并可能简化受控和非受控状态的处理方式，使其与 React 组件开发的最佳实践保持一致。
Highlights
- 依赖升级: 将
@rc-component/util依赖从^1.0.0升级到^1.3.0。 - Hook 替换: 在
src/Menu.tsx和src/hooks/useUUID.ts文件中，将useMergedStateHook 替换为useControlledStateHook。 - 状态管理逻辑调整: 为适应
useControlledState的新签名，对openKeys、activeKey和selectedKeys等状态的后处理逻辑进行了调整和重构。
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. ↩
|
All alerts resolved. Learn more about Socket for GitHub. This PR previously contained dependency changes with security issues that have been resolved, removed, or ignored. |
There was a problem hiding this comment.
Code Review
This pull request successfully refactors the codebase to use the useControlledState hook, replacing useMergedState. The changes are generally well-implemented. I've added a couple of suggestions for the useUUID hook to improve clarity and efficiency. Overall, a good refactoring effort.
src/hooks/useUUID.ts
Outdated
|
|
||
| export default function useUUID(id?: string) { | ||
| const [uuid, setUUID] = useMergedState(id, { value: id }); | ||
| const [uuid, setUUID] = useControlledState(id, id); |
There was a problem hiding this comment.
Using id for both defaultValue and value in useControlledState can be confusing, as the first argument is only used when the component is uncontrolled (i.e., value is undefined). A clearer approach would be useControlledState(undefined, id) to explicitly state that there is no specific default value. This improves readability without changing the functionality.
| const [uuid, setUUID] = useControlledState(id, id); | |
| const [uuid, setUUID] = useControlledState(undefined, id); |
![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
🧹 Nitpick comments (4)
src/hooks/useUUID.ts (1)
9-9: 受控/非受控语义合理，但可避免无效 set 调用当前
useControlledState(id, id)在受控模式下（传入了 id）通常会让 setter 成为 no-op。为减少一次无意义的setUUID调用，建议在有id时跳过初始化写入（同时使副作用对 SSR 更友好）：可在现有 effect 中加保护（示意代码）：
React.useEffect(() => { if (id == null) { internalId += 1; const newId = process.env.NODE_ENV === 'test' ? 'test' : `${uniquePrefix}-${internalId}`; setUUID(`rc-menu-uuid-${newId}`); } // 若采纳此写法，依赖需补充 `id` }, [id]);请确认
useControlledState在受控场景下的实现确实为「setter no-op」，以保证不会覆写外部传入的 id。src/Menu.tsx (3)
276-277: 使用空值合并运算符以表达更准确的语义
||会把空字符串等假值也当作空处理，这里更期望仅在null/undefined时回退：- const mergedOpenKeys = innerOpenKeys || EMPTY_LIST; + const mergedOpenKeys = innerOpenKeys ?? EMPTY_LIST;
377-380: 避免把空字符串误判为未设置；同时收紧类型以去掉不必要断言当
activeKey === ''时，||会错误地回退到默认激活项；建议改为??。并为 hook 指定string | undefined类型，移除as string：- const [mergedActiveKey, setMergedActiveKey] = useControlledState( - activeKey || ((defaultActiveFirst && childList[0]?.key) as string), - activeKey, - ); + const [mergedActiveKey, setMergedActiveKey] = useControlledState<string | undefined>( + activeKey ?? (defaultActiveFirst ? (childList[0]?.key as string | undefined) : undefined), + activeKey, + );请回归验证：传入
activeKey=""时不应触发默认首项激活。
422-437: selectedKeys 初始化与归一化 OK；可用空值合并与常量进一步强调意图逻辑正确。出于可读性与一致性，可用
??并复用EMPTY_LIST：- const [internalSelectKeys, setMergedSelectKeys] = useControlledState( - defaultSelectedKeys || [], - selectedKeys, - ); + const [internalSelectKeys, setMergedSelectKeys] = useControlledState( + defaultSelectedKeys ?? EMPTY_LIST, + selectedKeys, + );请补一例用例覆盖「传入
selectedKeys为null/undefined/[]/['a']」四种情况，确保mergedSelectKeys归一化符合预期且不触发多余重渲染。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
package.json(1 hunks)src/Menu.tsx(4 hunks)src/hooks/useUUID.ts(1 hunks)
🔇 Additional comments (3)
package.json (1)
49-49: 保留 @rc-component/util@^1.3.0
npm 已显示 1.3.0 版本已发布，且unpkg.com/@rc-component/util@1.3.0/lib/hooks/useControlledState.js返回 HTTP 200，可正常安装和使用，无需降级或延后合并。Likely an incorrect or invalid review comment.
src/hooks/useUUID.ts (1)
2-2: 确认新 hook 的导出路径与所依赖版本一致
@rc-component/util/lib/hooks/useControlledState在 1.2.x 是否存在需确认；若仅 1.3.x 才提供，请与 package.json 的版本保持同步，否则会构建失败。可复用 package.json 评论中的脚本检查该文件是否存在于目标版本。
src/Menu.tsx (1)
4-4: 依赖与导出路径需与 util 版本锁定这里改为
useControlledState后，请确保对应 util 版本导出该路径；否则会在装包或构建阶段报错。建议在合并前先发布 util 对应版本或调整依赖范围。参见 package.json 评论中的版本与文件存在性检查脚本。
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #793 +/- ##
==========================================
- Coverage 99.72% 99.72% -0.01%
==========================================
Files 27 26 -1
Lines 736 726 -10
Branches 199 198 -1
==========================================
- Hits 734 724 -10
Misses 2 2 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/Menu.tsx (1)
422-437: selectedKeys 归一化分支可简化为恒定数组根据 props 类型，selectedKeys/defaultSelectedKeys 均为
string[]。在 useControlledState<T=string[]> 下，internalSelectKeys恒为数组（或 undefined），对“非数组/单值”的分支处理属于冗余。可将该段简化，减少一次 useMemo：
- const mergedSelectKeys = React.useMemo(() => { - if (Array.isArray(internalSelectKeys)) { - return internalSelectKeys; - } - - if (internalSelectKeys === null || internalSelectKeys === undefined) { - return EMPTY_LIST; - } - - return [internalSelectKeys]; - }, [internalSelectKeys]); + const mergedSelectKeys = internalSelectKeys || EMPTY_LIST;若未来要支持
string | string[]的 selectedKeys，再恢复归一化逻辑亦可。
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (6)
tests/__snapshots__/Keyboard.spec.tsx.snapis excluded by!**/*.snaptests/__snapshots__/Menu.spec.tsx.snapis excluded by!**/*.snaptests/__snapshots__/MenuItem.spec.tsx.snapis excluded by!**/*.snaptests/__snapshots__/Options.spec.tsx.snapis excluded by!**/*.snaptests/__snapshots__/Responsive.spec.tsx.snapis excluded by!**/*.snaptests/__snapshots__/SubMenu.spec.tsx.snapis excluded by!**/*.snap
📒 Files selected for processing (1)
src/Menu.tsx(5 hunks)
🔇 Additional comments (3)
src/Menu.tsx (3)
263-263: 基于 useId 的 uuid 生成 OK，但需注意实例间唯一性当外部传入相同 id 的多实例时，uuid 也会重复，可能导致 refreshElements 的作用域冲突。建议：
- 在文档/类型提示中强调 id 需全局唯一；
- 或评估是否需要对 useId 的返回值增加前缀/后缀来降低冲突（需确认 refreshElements/IdContext 是否假定与 DOM id 完全一致后再改）。
276-277: openKeys 受控/非受控整合方式合理useControlledState(defaultOpenKeys, openKeys) + 本地 EMPTY_LIST 兜底符合期望，行为与历史实现一致。
4-6: 确认 @rc-component/util 版本及导入路径、签名一致
- package.json 中 @rc-component/util 版本为 ^1.3.0，已包含 useControlledState 和 useId。
- 仓库内未检测到 useMergedState 的遗留导入。
- 请手动确认 useControlledState 的签名确为
(defaultValue, value)并与当前用法一致。可选：若后续 @rc-component/util 提供顶层导出，可统一从顶层导入以降低深路径变更风险。
| const [mergedActiveKey, setMergedActiveKey] = useControlledState( | ||
| activeKey || ((defaultActiveFirst && childList[0]?.key) as string), | ||
| { | ||
| value: activeKey, | ||
| }, | ||
| activeKey, | ||
| ); |
There was a problem hiding this comment.
默认 activeKey 计算存在逻辑/类型缺陷：defaultActiveFirst 为 false 时会将 false 作为默认值传入
表达式 activeKey || ((defaultActiveFirst && childList[0]?.key) as string) 在 defaultActiveFirst=false 时产生布尔值 false，导致 mergedActiveKey 可能为 false，影响聚焦与可达性逻辑。
建议改为显式的三元表达式，避免把布尔值灌入字符串状态：
- const [mergedActiveKey, setMergedActiveKey] = useControlledState(
- activeKey || ((defaultActiveFirst && childList[0]?.key) as string),
- activeKey,
- );
+ const [mergedActiveKey, setMergedActiveKey] = useControlledState<string | undefined>(
+ defaultActiveFirst ? (childList[0]?.key as string) : undefined,
+ activeKey,
+ );📝 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.
| const [mergedActiveKey, setMergedActiveKey] = useControlledState( | |
| activeKey || ((defaultActiveFirst && childList[0]?.key) as string), | |
| { | |
| value: activeKey, | |
| }, | |
| activeKey, | |
| ); | |
| const [mergedActiveKey, setMergedActiveKey] = useControlledState<string | undefined>( | |
| defaultActiveFirst ? (childList[0]?.key as string) : undefined, | |
| activeKey, | |
| ); |
🤖 Prompt for AI Agents
In src/Menu.tsx around lines 377-380, the default activeKey expression can pass
a boolean false into the state when defaultActiveFirst is false; replace the
current logical-AND/or expression with an explicit ternary: if activeKey is
provided use it; else if defaultActiveFirst is true use childList[0]?.key (cast
to string if needed); otherwise use undefined (or the hook's expected empty
value). Ensure the resulting type matches the controlled state type (string |
undefined) so boolean false is never assigned.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/Menu.tsx (1)
377-380: defaultActiveFirst=false 时将 boolean false 写入状态——请改为显式三元并收敛类型表达式
activeKey || ((defaultActiveFirst && childList[0]?.key) as string)在defaultActiveFirst=false且未受控时会把false当默认值写入，导致mergedActiveKey可能为false，进而影响聚焦与可达性。应使用显式三元并将受控类型统一为string | undefined。建议修复：
- const [mergedActiveKey, setMergedActiveKey] = useControlledState( - activeKey || ((defaultActiveFirst && childList[0]?.key) as string), - activeKey, - ); + const [mergedActiveKey, setMergedActiveKey] = useControlledState<string | undefined>( + defaultActiveFirst ? (childList[0]?.key as string | undefined) : undefined, + activeKey, + );
🧹 Nitpick comments (3)
src/Menu.tsx (3)
276-278: openKeys 状态可直接收敛为数组，减少后续空数组兜底与分支当前通过
innerOpenKeys || EMPTY_LIST兜底。可用泛型将默认值固定为数组，简化后续逻辑与类型判断。建议：
- const [innerOpenKeys, setMergedOpenKeys] = useControlledState(defaultOpenKeys, openKeys); - const mergedOpenKeys = innerOpenKeys || EMPTY_LIST; + const [innerOpenKeys, setMergedOpenKeys] = useControlledState<string[]>( + defaultOpenKeys ?? EMPTY_LIST, + openKeys, + ); + const mergedOpenKeys = innerOpenKeys;
422-425: selectedKeys 的状态类型可固定为 string[]，用泛型约束并以 EMPTY_LIST 兜底当前用
|| []兜底虽可行，但建议通过泛型将类型收紧为数组，避免后续不必要的判空与包装。建议：
- const [internalSelectKeys, setMergedSelectKeys] = useControlledState( - defaultSelectedKeys || [], - selectedKeys, - ); + const [internalSelectKeys, setMergedSelectKeys] = useControlledState<string[]>( + defaultSelectedKeys ?? EMPTY_LIST, + selectedKeys, + );
426-437: 合并选择键的 memo 逻辑多余，可直接使用数组状态在上面的泛型收紧后，
internalSelectKeys始终是string[]，无需再判断与包装，减少一次渲染期计算。建议：
- const mergedSelectKeys = React.useMemo(() => { - if (Array.isArray(internalSelectKeys)) { - return internalSelectKeys; - } - - if (internalSelectKeys === null || internalSelectKeys === undefined) { - return EMPTY_LIST; - } - - return [internalSelectKeys]; - }, [internalSelectKeys]); + const mergedSelectKeys = internalSelectKeys;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
src/Menu.tsx(5 hunks)
🔇 Additional comments (2)
src/Menu.tsx (2)
4-6: 迁移到 useControlledState / useId 的方向正确引入位置与现有 util 用法保持一致，变更本身无碍。
263-263: 确认 useId 参数语义及改用useId(id)
src/Menu.tsx 第263行：避免将'rc-menu-uuid'作为固定前缀直接传入useId，若 util 仅把入参当前缀拼接，可能导致 SSR hydration 或多实例冲突。请在node_modules/@rc-component/util/hooks/useId.ts（v1.3.x）源码或官方文档中，确认第一个参数是“固定 id”还是“前缀”，以及 React 18+ SSR 下的行为，确定后再决定是否将调用改为：- const uuid = useId(id ?? 'rc-menu-uuid'); + const uuid = useId(id);
EmilyyyLiu
force-pushed
the
useControlledState-use
branch
from
12c7ae2 to
2e327fd
Compare
2 participants
关联issue：ant-design/ant-design#54854
替换 useMergedState 为 useControlledState
Summary by CodeRabbit