Skip to content

refactor: Upgrade utils and replace useMergedState #793

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.

Sign up for GitHub

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

Merged
merged 3 commits into from
Sep 10, 2025
Merged

refactor: Upgrade utils and replace useMergedState #793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5e66f74
refactor: Upgrade utils and replace useMergedState
Sep 8, 2025
2e327fd
refactor: useUUID replace useId
Sep 9, 2025
28ddcf9
fix: coverage
Sep 9, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -46,7 +46,7 @@
"dependencies": {
"@rc-component/motion": "^1.1.4",
"@rc-component/trigger": "^3.0.0",
"@rc-component/util": "^1.0.0",
"@rc-component/util": "^1.3.0",
"classnames": "2.x",
"rc-overflow": "^1.3.1"
},
Expand Down
45 changes: 20 additions & 25 deletions src/Menu.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,8 @@
import classNames from 'classnames';
import type { CSSMotionProps } from '@rc-component/motion';
import Overflow from 'rc-overflow';
import useMergedState from '@rc-component/util/lib/hooks/useMergedState';
import useControlledState from '@rc-component/util/lib/hooks/useControlledState';
import useId from '@rc-component/util/lib/hooks/useId';
import isEqual from '@rc-component/util/lib/isEqual';
import warning from '@rc-component/util/lib/warning';
import * as React from 'react';
Expand All @@ -14,7 +15,6 @@ import PrivateContext from './context/PrivateContext';
import { getFocusableElements, refreshElements, useAccessibility } from './hooks/useAccessibility';
import useKeyRecords, { OVERFLOW_KEY } from './hooks/useKeyRecords';
import useMemoCallback from './hooks/useMemoCallback';
import useUUID from './hooks/useUUID';
import type {
BuiltinPlacements,
Components,
Expand Down Expand Up @@ -260,7 +260,7 @@ const Menu = React.forwardRef<MenuRef, MenuProps>((props, ref) => {

const containerRef = React.useRef<HTMLUListElement>();

const uuid = useUUID(id);
const uuid = useId(id ? `rc-menu-uuid-${id}` : 'rc-menu-uuid');

const isRtl = direction === 'rtl';

Expand All @@ -273,10 +273,8 @@ const Menu = React.forwardRef<MenuRef, MenuProps>((props, ref) => {
}

// ========================= Open =========================
const [mergedOpenKeys, setMergedOpenKeys] = useMergedState(defaultOpenKeys, {
value: openKeys,
postState: keys => keys || EMPTY_LIST,
});
const [innerOpenKeys, setMergedOpenKeys] = useControlledState(defaultOpenKeys, openKeys);
const mergedOpenKeys = innerOpenKeys || EMPTY_LIST;

// React 18 will merge mouse event which means we open key will not sync
// ref: https://github.com/ant-design/ant-design/issues/38818
Expand Down Expand Up @@ -376,11 +374,9 @@ const Menu = React.forwardRef<MenuRef, MenuProps>((props, ref) => {
}, [lastVisibleIndex, allVisible]);

// ======================== Active ========================
const [mergedActiveKey, setMergedActiveKey] = useMergedState(
const [mergedActiveKey, setMergedActiveKey] = useControlledState(
activeKey || ((defaultActiveFirst && childList[0]?.key) as string),
{
value: activeKey,
},
activeKey,
);
Comment on lines +377 to 380
Copy link

@coderabbitai coderabbitai bot Sep 9, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue

默认 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.

Suggested change
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.


const onActive = useMemoCallback((key: string) => {
Expand Down Expand Up @@ -423,22 +419,21 @@ const Menu = React.forwardRef<MenuRef, MenuProps>((props, ref) => {

// ======================== Select ========================
// >>>>> Select keys
const [mergedSelectKeys, setMergedSelectKeys] = useMergedState(defaultSelectedKeys || [], {
value: selectedKeys,

// Legacy convert key to array
postState: keys => {
if (Array.isArray(keys)) {
return keys;
}
const [internalSelectKeys, setMergedSelectKeys] = useControlledState(
defaultSelectedKeys || [],
selectedKeys,
);
const mergedSelectKeys = React.useMemo(() => {
if (Array.isArray(internalSelectKeys)) {
return internalSelectKeys;
}

if (keys === null || keys === undefined) {
return EMPTY_LIST;
}
if (internalSelectKeys === null || internalSelectKeys === undefined) {
return EMPTY_LIST;
}

return [keys];
},
});
return [internalSelectKeys];
}, [internalSelectKeys]);

// >>>>> Trigger select
const triggerSelection = (info: MenuInfo) => {
Expand Down
3 changes: 0 additions & 3 deletions src/context/IdContext.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,6 @@ import * as React from 'react';
export const IdContext = React.createContext<string>(null);

export function getMenuId(uuid: string, eventKey: string) {
if (uuid === undefined) {
return null;
}
return `${uuid}-${eventKey}`;
}

Expand Down
18 changes: 0 additions & 18 deletions src/hooks/useUUID.ts
View file
Open in desktop

This file was deleted.

8 changes: 4 additions & 4 deletions tests/__snapshots__/Keyboard.spec.tsx.snap
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,11 +13,11 @@ HTMLCollection [
role="none"
>
<div
aria-controls="rc-menu-uuid-test-light-popup"
aria-controls="rc-menu-uuid-light-popup"
aria-expanded="true"
aria-haspopup="true"
class="rc-menu-submenu-title"
data-menu-id="rc-menu-uuid-test-light"
data-menu-id="rc-menu-uuid-light"
role="menuitem"
style="padding-left: 24px;"
tabindex="-1"
Expand All @@ -31,12 +31,12 @@ HTMLCollection [
<ul
class="rc-menu rc-menu-sub rc-menu-inline"
data-menu-list="true"
id="rc-menu-uuid-test-light-popup"
id="rc-menu-uuid-light-popup"
role="menu"
>
<li
class="rc-menu-item"
data-menu-id="rc-menu-uuid-test-bamboo"
data-menu-id="rc-menu-uuid-bamboo"
role="menuitem"
style="padding-left: 48px;"
tabindex="-1"
Expand Down
Loading
Loading