Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,7 @@ yarn.lock
package-lock.json
pnpm-lock.yaml
.doc
.vscode

# dumi
.dumi/tmp
Expand Down
12 changes: 9 additions & 3 deletions docs/examples/basic.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -41,11 +41,17 @@ export default () => {
/>
</div>

<Portal open={show} getContainer={getContainer} autoLock={lock}>
<Portal open={show} getContainer={getContainer} autoLock={lock} onEsc={({ top, event }) => {
console.log('root onEsc', { top, event });
}}>
<p className={clsx(contentCls, 'root')}>Hello Root</p>
<Portal open={show} getContainer={getContainer} autoLock={lock}>
<Portal open={show} getContainer={getContainer} autoLock={lock} onEsc={({ top, event }) => {
console.log('parent onEsc', { top, event });
}}>
<p className={clsx(contentCls, 'parent')}>Hello Parent</p>
<Portal open={show} getContainer={getContainer} autoLock={lock}>
<Portal open={show} getContainer={getContainer} autoLock={lock} onEsc={({ top, event }) => {
console.log('children onEsc', { top, event });
}}>
<p className={clsx(contentCls, 'children')}>Hello Children</p>
</Portal>
</Portal>
Expand Down
2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@
"devDependencies": {
"@rc-component/father-plugin": "^2.0.2",
"@rc-component/np": "^1.0.3",
"@testing-library/jest-dom": "^5.16.4",
"@testing-library/jest-dom": "^6.9.0",
"@testing-library/react": "^13.0.0",
"@types/jest": "^26.0.20",
"@types/node": "^20.9.0",
Expand Down
34 changes: 0 additions & 34 deletions script/update-content.js

This file was deleted.

14 changes: 14 additions & 0 deletions src/Portal.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@ import OrderContext from './Context';
import { inlineMock } from './mock';
import useDom from './useDom';
import useScrollLocker from './useScrollLocker';
import useEscKeyDown from './useEscKeyDown';

export type ContainerType = Element | DocumentFragment;

Expand All @@ -20,6 +21,14 @@ export type GetContainer =
| (() => ContainerType)
| false;

export type EscCallback = ({
top,
event,
}: {
top: boolean;
event: KeyboardEvent;
}) => void;

export interface PortalProps {
/** Customize container element. Default will create a div in document.body when `open` */
getContainer?: GetContainer;
Expand All @@ -30,6 +39,7 @@ export interface PortalProps {
autoDestroy?: boolean;
/** Lock screen scroll when open */
autoLock?: boolean;
onEsc?: EscCallback;
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

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

The new onEsc prop lacks documentation. Consider adding a JSDoc comment to describe its purpose, parameters, and usage. For example, explaining that the callback receives an object with isTop (boolean indicating if this portal is the topmost) and event (the keyboard event).

Copilot uses AI. Check for mistakes.

/** @private debug name. Do not use in prod */
debug?: string;
Expand Down Expand Up @@ -61,6 +71,7 @@ const Portal = React.forwardRef<any, PortalProps>((props, ref) => {
debug,
autoDestroy = true,
children,
onEsc,
} = props;

const [shouldRender, setShouldRender] = React.useState(open);
Expand Down Expand Up @@ -109,6 +120,9 @@ const Portal = React.forwardRef<any, PortalProps>((props, ref) => {
mergedContainer === document.body),
);

// ========================= Esc Keydown ==========================
useEscKeyDown(open, onEsc);

// =========================== Ref ===========================
let childRef: React.Ref<any> = null;

Expand Down
37 changes: 37 additions & 0 deletions src/useEscKeyDown.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
import { useEffect, useMemo } from 'react';
import { type EscCallback } from './Portal';
import useId from '@rc-component/util/lib/hooks/useId';
import { useEvent } from '@rc-component/util';

let stack: string[] = [];

export default function useEscKeyDown(open: boolean, onEsc?: EscCallback) {
const id = useId();

const handleEscKeyDown = useEvent((event: KeyboardEvent) => {
if (event.key === 'Escape') {
const top = stack[stack.length - 1] === id;
onEsc?.({ top, event });
}
});

useMemo(() => {
if (open && !stack.includes(id)) {
stack.push(id);
} else if (!open) {
stack = stack.filter(item => item !== id);
}
}, [open, id]);
Comment on lines +18 to +24
Copy link

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

严重问题:useMemo 用于副作用 + 缺少卸载清理

存在两个关键问题:

  1. 违反 React 原则useMemo 被设计用于记忆计算值,而非执行副作用。在 React 19 的并发模式下,渲染可能被中断或重复执行,导致 stack 状态不一致。

  2. 内存泄漏:当组件在 open=true 时卸载,useMemo 不会再次运行,导致 id 永久残留在 stack 中。

建议的修复方案:将 stack 操作移至 useEffect,确保在 cleanup 函数中移除 id。

🔎 推荐的修复方案
-  useMemo(() => {
-    if (open && !stack.includes(id)) {
-      stack.push(id);
-    } else if (!open) {
-      stack = stack.filter(item => item !== id);
-    }
-  }, [open, id]);
-
   useEffect(() => {
-    if (!open) {
-      return;
-    }
+    if (open && !stack.includes(id)) {
+      stack.push(id);
+    }
 
-    window.addEventListener('keydown', handleEscKeyDown);
+    if (open) {
+      window.addEventListener('keydown', handleEscKeyDown);
+    }
 
     return () => {
+      stack = stack.filter(item => item !== id);
       window.removeEventListener('keydown', handleEscKeyDown);
     };
   }, [open, id]);
🤖 Prompt for AI Agents
In src/useEscKeyDown.ts around lines 18 to 24, the code uses useMemo for side
effects which can cause inconsistent stack state and memory leaks; replace the
useMemo with a useEffect that pushes id onto stack when open is true and removes
id when open is false, and also return a cleanup function that always removes id
from stack (to handle component unmounts) so the stack remains consistent in
concurrent rendering.


useEffect(() => {
if (!open) {
return;
}

window.addEventListener('keydown', handleEscKeyDown);
Copy link

Copilot AI Dec 22, 2025

Choose a reason for hiding this comment

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

The event listener is added to window even when onEsc is undefined. This adds unnecessary overhead. Consider adding an early return or conditional check to avoid registering the event listener when onEsc is not provided.

Copilot uses AI. Check for mistakes.

return () => {
window.removeEventListener('keydown', handleEscKeyDown);
};
}, [open, id]);
}
68 changes: 67 additions & 1 deletion tests/index.test.tsx
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
import { render } from '@testing-library/react';
import { render, fireEvent } from '@testing-library/react';
import React from 'react';
import Portal from '../src';

Expand All @@ -18,6 +18,11 @@ jest.mock('@rc-component/util/lib/hooks/useLayoutEffect', () => {
return origin.useLayoutEffect;
});

jest.mock('@rc-component/util/lib/hooks/useId', () => {
const origin = jest.requireActual('react');
return origin.useId;
});

describe('Portal', () => {
beforeEach(() => {
global.isOverflow = true;
Expand Down Expand Up @@ -290,4 +295,65 @@ describe('Portal', () => {

expect(document.querySelector('.checker').textContent).toEqual('true');
});

describe('onEsc', () => {
it('only last opened portal is top', () => {
const onEscA = jest.fn();
const onEscB = jest.fn();

render(
<>
<Portal open onEsc={onEscA}>
<div />
</Portal>
<Portal open onEsc={onEscB}>
<div />
</Portal>
</>,
);

fireEvent.keyDown(window, { key: 'Escape' });

expect(onEscA).toHaveBeenCalledWith(
expect.objectContaining({ top: false }),
);
expect(onEscB).toHaveBeenCalledWith(
expect.objectContaining({ top: true }),
);
});

it('top changes after portal closes', () => {
const onEscA = jest.fn();
const onEscB = jest.fn();

const { rerender } = render(
<>
<Portal open onEsc={onEscA}>
<div />
</Portal>
<Portal open onEsc={onEscB}>
<div />
</Portal>
</>,
);

rerender(
<>
<Portal open onEsc={onEscA}>
<div />
</Portal>
<Portal open={false} onEsc={onEscB}>
<div />
</Portal>
</>,
);

fireEvent.keyDown(window, { key: 'Escape' });

expect(onEscA).toHaveBeenCalledWith(
expect.objectContaining({ top: true }),
);
expect(onEscB).not.toHaveBeenCalled();
});
});
});