Skip to content
Open
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
19 changes: 18 additions & 1 deletion src/Notice.tsx
Original file line number Diff line number Diff line change
@@ -1,9 +1,16 @@
import { clsx } from 'clsx';
import KeyCode from '@rc-component/util/lib/KeyCode';
import warning from '@rc-component/util/lib/warning';
import * as React from 'react';
import type { NoticeConfig } from './interface';
import pickAttrs from '@rc-component/util/lib/pickAttrs';

/**
* Maximum delay value for setTimeout in seconds (2^31 - 1 ms).
* @see https://developer.mozilla.org/en-US/docs/Web/API/Window/setTimeout#maximum_delay_value
*/
const MAX_DURATION = 2147483647 / 1000;

Choose a reason for hiding this comment

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

medium

To improve readability and avoid a magic number, it would be clearer to express 2147483647 as the calculation it represents. This makes the origin of the number apparent directly in the code, aligning with the explanation in the comment and the PR description.

Suggested change
const MAX_DURATION = 2147483647 / 1000;
const MAX_DURATION = (2 ** 31 - 1) / 1000;


export interface NoticeProps extends Omit<NoticeConfig, 'onClose'> {
prefixCls: string;
className?: string;
Expand Down Expand Up @@ -38,7 +45,9 @@ const Notify = React.forwardRef<HTMLDivElement, NoticeProps & { times?: number }
const [percent, setPercent] = React.useState(0);
const [spentTime, setSpentTime] = React.useState(0);
const mergedHovering = forcedHovering || hovering;
const mergedDuration: number = typeof duration === 'number' ? duration : 0;

const rawDuration: number = typeof duration === 'number' ? duration : 0;
const mergedDuration: number = Math.min(rawDuration, MAX_DURATION);
const mergedShowProgress = mergedDuration > 0 && showProgress;

// ======================== Close =========================
Expand All @@ -52,6 +61,14 @@ const Notify = React.forwardRef<HTMLDivElement, NoticeProps & { times?: number }
}
};

// ========================= Warn =========================
React.useEffect(() => {
warning(
rawDuration <= MAX_DURATION,
`\`duration\` exceeds the maximum supported value (${MAX_DURATION}s) and has been clamped.`,
);
}, [rawDuration]);

// ======================== Effect ========================
React.useEffect(() => {
if (!mergedHovering && mergedDuration > 0) {
Expand Down
47 changes: 47 additions & 0 deletions tests/index.test.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -1018,4 +1018,51 @@ describe('Notification.Basic', () => {
'xxx',
);
});

describe('MAX_DURATION', () => {
it('should not warn when duration is within limit', () => {
const { instance } = renderDemo();
const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {});

act(() => {
instance.open({
content: <p className="test">1</p>,
duration: 0.1,
});
});

expect(document.querySelector('.test')).toBeTruthy();
expect(errSpy).not.toHaveBeenCalled();
});

it('should warn and clamp when duration exceeds limit', () => {
const { instance } = renderDemo();
const errSpy = vi.spyOn(console, 'error').mockImplementation(() => {});

const MAX_DURATION = 2147483647 / 1000;
const EXCEEDED_DURATION = (2147483647 + 1) / 1000;
Comment on lines +1042 to +1043

Choose a reason for hiding this comment

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

medium

To improve maintainability and avoid repeating the magic number 2147483647, you could define the maximum timeout value in milliseconds as a constant and then derive MAX_DURATION and EXCEEDED_DURATION from it. This makes the code easier to read and ensures consistency if the value ever needs to be changed.

Suggested change
const MAX_DURATION = 2147483647 / 1000;
const EXCEEDED_DURATION = (2147483647 + 1) / 1000;
const MAX_TIMEOUT_MS = 2 ** 31 - 1;
const MAX_DURATION = MAX_TIMEOUT_MS / 1000;
const EXCEEDED_DURATION = (MAX_TIMEOUT_MS + 1) / 1000;


act(() => {
instance.open({
content: <p className="test">1</p>,
duration: EXCEEDED_DURATION,
});
});

expect(document.querySelector('.test')).toBeTruthy();
expect(errSpy).toHaveBeenCalledWith(
`Warning: \`duration\` exceeds the maximum supported value (${MAX_DURATION}s) and has been clamped.`,
);

act(() => {
vi.advanceTimersByTime(MAX_DURATION * 1000 - 1);
});
expect(document.querySelector('.test')).toBeTruthy();

act(() => {
vi.advanceTimersByTime(1);
});
expect(document.querySelector('.test')).toBeFalsy();
});
});
});