Skip to content

fix: Style not sync with step status #71

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
zombieJ merged 5 commits into from
Dec 29, 2025
Merged

fix: Style not sync with step status #71

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
6ace63c
chore: tmp of it
zombieJ Dec 29, 2025
917b3e8
Merge branch 'master' into fix-sync
zombieJ Dec 29, 2025
fd217a9
test: fix test case
zombieJ Dec 29, 2025
bc1ee18
test: fix test case
zombieJ Dec 29, 2025
930cee3
fix: cast motion children hook result to ReactElement
zombieJ Dec 29, 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
3 changes: 3 additions & 0 deletions docs/demo/debug.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
# Debug Demo

<code src="../examples/debug.tsx"></code>
18 changes: 18 additions & 0 deletions docs/examples/debug.less
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
.debug-demo-block {
overflow: hidden;
box-shadow: 0 0 0 3px red;
}

.debug-motion {
&-appear,
&-enter {
&-start {
opacity: 0;
}

&-active {
opacity: 1;
transition: background 0.3s, height 1.3s, opacity 1.3s;
}
}
}
65 changes: 65 additions & 0 deletions docs/examples/debug.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
import { clsx } from 'clsx';
import CSSMotion, { type CSSMotionProps } from 'rc-motion';
import React, { useState } from 'react';
import './debug.less';

const onCollapse = () => {
console.log('🔥 Collapse');
return { height: 0 };
};

const onExpand: CSSMotionProps['onAppearActive'] = node => {
console.log('🔥 Expand');
return { height: node.scrollHeight };
};

function DebugDemo() {
const [key, setKey] = useState(0);

return (
<div>
<button
onClick={() => {
setKey(prev => prev + 1);
}}
>
Start
</button>

<CSSMotion
visible
motionName="debug-motion"
motionAppear
onAppearStart={onCollapse}
onAppearActive={onExpand}
key={key}
>
{({ style, className }, ref) => {
console.log('render', className, style);

return (
<div
ref={ref}
className={clsx('debug-demo-block', className)}
style={style}
>
<div
style={{
height: 100,
width: 100,
background: 'blue',
}}
/>
</div>
);
}}
</CSSMotion>
</div>
);
}

export default () => (
<React.StrictMode>
<DebugDemo />
</React.StrictMode>
);
138 changes: 73 additions & 65 deletions src/CSSMotion.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -146,12 +146,8 @@ export function genCSSMotion(config: CSSMotionConfig) {
return getDOM(nodeRef.current) as HTMLElement;
}

const [getStatus, statusStep, statusStyle, mergedVisible] = useStatus(
supportMotion,
visible,
getDomElement,
props,
);
const [getStatus, statusStep, statusStyle, mergedVisible, styleReady] =
useStatus(supportMotion, visible, getDomElement, props);
const status = getStatus();

// Record whether content has rendered
Expand Down Expand Up @@ -186,73 +182,85 @@ export function genCSSMotion(config: CSSMotionConfig) {
React.useImperativeHandle(ref, () => refObj, []);

// ===================== Render =====================
let motionChildren: React.ReactNode;
const mergedProps = { ...eventProps, visible };

if (!children) {
// No children
motionChildren = null;
} else if (status === STATUS_NONE) {
// Stable children
if (mergedVisible) {
motionChildren = children({ ...mergedProps }, nodeRef);
} else if (!removeOnLeave && renderedRef.current && leavedClassName) {
motionChildren = children(
{ ...mergedProps, className: leavedClassName },
nodeRef,
);
} else if (forceRender || (!removeOnLeave && !leavedClassName)) {
motionChildren = children(
{ ...mergedProps, style: { display: 'none' } },
nodeRef,
);
} else {
motionChildren = null;
}
} else {
// In motion
let statusSuffix: string;
if (statusStep === STEP_PREPARE) {
statusSuffix = 'prepare';
} else if (isActive(statusStep)) {
statusSuffix = 'active';
} else if (statusStep === STEP_START) {
statusSuffix = 'start';
}

const motionCls = getTransitionName(
motionName,
`${status}-${statusSuffix}`,
);

motionChildren = children(
{
...mergedProps,
className: clsx(getTransitionName(motionName, status), {
[motionCls]: motionCls && statusSuffix,
[motionName as string]: typeof motionName === 'string',
}),
style: statusStyle,
},
nodeRef,
);
// return motionChildren as React.ReactElement;
const idRef = React.useRef(0);
if (styleReady) {
idRef.current += 1;
}

// Auto inject ref if child node not have `ref` props
if (React.isValidElement(motionChildren) && supportRef(motionChildren)) {
const originNodeRef = getNodeRef(motionChildren);
// We should render children when motionStyle is sync with stepStatus
return React.useMemo(() => {
let motionChildren: React.ReactNode;
const mergedProps = { ...eventProps, visible };

if (!children) {
// No children
motionChildren = null;
} else if (status === STATUS_NONE) {
// Stable children
if (mergedVisible) {
motionChildren = children({ ...mergedProps }, nodeRef);
} else if (!removeOnLeave && renderedRef.current && leavedClassName) {
motionChildren = children(
{ ...mergedProps, className: leavedClassName },
nodeRef,
);
} else if (forceRender || (!removeOnLeave && !leavedClassName)) {
motionChildren = children(
{ ...mergedProps, style: { display: 'none' } },
nodeRef,
);
} else {
motionChildren = null;
}
} else {
// In motion
let statusSuffix: string;
if (statusStep === STEP_PREPARE) {
statusSuffix = 'prepare';
} else if (isActive(statusStep)) {
statusSuffix = 'active';
} else if (statusStep === STEP_START) {
statusSuffix = 'start';
}

const motionCls = getTransitionName(
motionName,
`${status}-${statusSuffix}`,
);

if (!originNodeRef) {
motionChildren = React.cloneElement(
motionChildren as React.ReactElement,
motionChildren = children(
{
ref: nodeRef,
...mergedProps,
className: clsx(getTransitionName(motionName, status), {
[motionCls]: motionCls && statusSuffix,
[motionName as string]: typeof motionName === 'string',
}),
style: statusStyle,
},
nodeRef,
);
}
}

return motionChildren as React.ReactElement;
// Auto inject ref if child node not have `ref` props
if (
React.isValidElement(motionChildren) &&
supportRef(motionChildren)
) {
const originNodeRef = getNodeRef(motionChildren);

if (!originNodeRef) {
motionChildren = React.cloneElement(
motionChildren as React.ReactElement,
{
ref: nodeRef,
},
);
}
}

return motionChildren;
}, [idRef.current]);
Copy link

@gemini-code-assist gemini-code-assist bot Dec 29, 2025

Choose a reason for hiding this comment

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

high

这部分代码通过 idRef 和 useMemo 来解决样式和状态不同步时渲染旧内容的问题，想法很巧妙，但存在一些问题：

  1. 不完整的依赖项数组: React.useMemo 的依赖数组只有 [idRef.current]，这是不完整的。当 children, visible 等外部 props 发生变化时，如果 styleReady 恰好为 false，useMemo 不会重新计算，从而返回了旧的、缓存的 motionChildren，导致渲染结果与新的 props 不一致。这会破坏组件的响应性，是一个潜在的 bug。

  2. 渲染期间的副作用: 在渲染函数中直接修改 ref 的 current 属性 (idRef.current += 1;) 是一个副作用。在 React 的严格模式（Strict Mode）下，渲染函数可能会被调用多次，这会导致 idRef.current 被意外地增加多次，可能引发非预期的行为。

建议重构这部分逻辑以提高代码的健壮性和可预测性。一个更符合 React 模式的做法是避免在渲染期间产生副作用。例如，可以考虑使用 useState 和 useLayoutEffect 来缓存并同步渲染内容，确保只在 styleReady 为 true 时才更新输出。虽然这可能引入额外的渲染周期，但代码会更清晰和安全。

Copy link

@coderabbitai coderabbitai bot Dec 29, 2025

Choose a reason for hiding this comment

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

⚠️ Potential issue | 🔴 Critical

流水线类型错误：useMemo 返回类型与 ForwardRefRenderFunction 不兼容。

流水线报错指出第 125 行的类型不匹配：useMemo 返回 ReactNode（可能是 null、undefined、string 等），但 ForwardRefRenderFunction 期望返回 ReactElement。

需要确保返回值符合 ReactElement 类型，或者使用类型断言。

🔎 建议的修复方案 
-      return React.useMemo(() => {
+      const motionChildren = React.useMemo(() => {
         let motionChildren: React.ReactNode;
         // ... existing logic ...
         return motionChildren;
       }, [idRef.current]);
+
+      return motionChildren as React.ReactElement;

或者更安全的做法：

       return React.useMemo(() => {
         // ... existing logic ...
-        return motionChildren;
+        return motionChildren as React.ReactElement;
       }, [idRef.current]);
📝 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
return React.useMemo(() => {
let motionChildren: React.ReactNode;
const mergedProps = { ...eventProps, visible };
if (!children) {
// No children
motionChildren = null;
} else if (status === STATUS_NONE) {
// Stable children
if (mergedVisible) {
motionChildren = children({ ...mergedProps }, nodeRef);
} else if (!removeOnLeave && renderedRef.current && leavedClassName) {
motionChildren = children(
{ ...mergedProps, className: leavedClassName },
nodeRef,
);
} else if (forceRender || (!removeOnLeave && !leavedClassName)) {
motionChildren = children(
{ ...mergedProps, style: { display: 'none' } },
nodeRef,
);
} else {
motionChildren = null;
}
} else {
// In motion
let statusSuffix: string;
if (statusStep === STEP_PREPARE) {
statusSuffix = 'prepare';
} else if (isActive(statusStep)) {
statusSuffix = 'active';
} else if (statusStep === STEP_START) {
statusSuffix = 'start';
}
const motionCls = getTransitionName(
motionName,
`${status}-${statusSuffix}`,
);
if (!originNodeRef) {
motionChildren = React.cloneElement(
motionChildren as React.ReactElement,
motionChildren = children(
{
ref: nodeRef,
...mergedProps,
className: clsx(getTransitionName(motionName, status), {
[motionCls]: motionCls && statusSuffix,
[motionName as string]: typeof motionName === 'string',
}),
style: statusStyle,
},
nodeRef,
);
}
}
return motionChildren as React.ReactElement;
// Auto inject ref if child node not have `ref` props
if (
React.isValidElement(motionChildren) &&
supportRef(motionChildren)
) {
const originNodeRef = getNodeRef(motionChildren);
if (!originNodeRef) {
motionChildren = React.cloneElement(
motionChildren as React.ReactElement,
{
ref: nodeRef,
},
);
}
}
return motionChildren;
}, [idRef.current]);
return React.useMemo(() => {
let motionChildren: React.ReactNode;
const mergedProps = { ...eventProps, visible };
if (!children) {
// No children
motionChildren = null;
} else if (status === STATUS_NONE) {
// Stable children
if (mergedVisible) {
motionChildren = children({ ...mergedProps }, nodeRef);
} else if (!removeOnLeave && renderedRef.current && leavedClassName) {
motionChildren = children(
{ ...mergedProps, className: leavedClassName },
nodeRef,
);
} else if (forceRender || (!removeOnLeave && !leavedClassName)) {
motionChildren = children(
{ ...mergedProps, style: { display: 'none' } },
nodeRef,
);
} else {
motionChildren = null;
}
} else {
// In motion
let statusSuffix: string;
if (statusStep === STEP_PREPARE) {
statusSuffix = 'prepare';
} else if (isActive(statusStep)) {
statusSuffix = 'active';
} else if (statusStep === STEP_START) {
statusSuffix = 'start';
}
const motionCls = getTransitionName(
motionName,
`${status}-${statusSuffix}`,
);
motionChildren = children(
{
...mergedProps,
className: clsx(getTransitionName(motionName, status), {
[motionCls]: motionCls && statusSuffix,
[motionName as string]: typeof motionName === 'string',
}),
style: statusStyle,
},
nodeRef,
);
}
// Auto inject ref if child node not have `ref` props
if (
React.isValidElement(motionChildren) &&
supportRef(motionChildren)
) {
const originNodeRef = getNodeRef(motionChildren);
if (!originNodeRef) {
motionChildren = React.cloneElement(
motionChildren as React.ReactElement,
{
ref: nodeRef,
},
);
}
}
return motionChildren as React.ReactElement;
}, [idRef.current]);
🤖 Prompt for AI Agents 
In src/CSSMotion.tsx around lines 192 to 263, useMemo is inferred to return
React.ReactNode (null/string/etc.) but the ForwardRefRenderFunction expects a
ReactElement; change the memo return type to a ReactElement|null and ensure
motionChildren is normalized to an element before returning (e.g., if
motionChildren is null keep null, otherwise wrap non-element nodes in a React
fragment or cast the final value to ReactElement|null); update the useMemo
generic/type annotation to React.ReactElement | null (or add an explicit type
assertion on the returned value) so the types align with the
ForwardRefRenderFunction contract.

},
);

Expand Down
40 changes: 29 additions & 11 deletions src/hooks/useStatus.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
import { useEvent } from '@rc-component/util';
import useState from '@rc-component/util/lib/hooks/useState';
import useSyncState from '@rc-component/util/lib/hooks/useSyncState';
import * as React from 'react';
import { useEffect, useRef } from 'react';
Expand Down Expand Up @@ -49,11 +48,19 @@ export default function useStatus(
onLeaveEnd,
onVisibleChanged,
}: CSSMotionProps,
): [() => MotionStatus, StepStatus, React.CSSProperties, boolean] {
): [
status: () => MotionStatus,
stepStatus: StepStatus,
style: React.CSSProperties,
visible: boolean,
styleReady: boolean,
] {
// Used for outer render usage to avoid `visible: false & status: none` to render nothing
const [asyncVisible, setAsyncVisible] = useState<boolean>();
const [asyncVisible, setAsyncVisible] = React.useState<boolean>();
const [getStatus, setStatus] = useSyncState<MotionStatus>(STATUS_NONE);
const [style, setStyle] = useState<React.CSSProperties | undefined>(null);
const [style, setStyle] = React.useState<
[style: React.CSSProperties | undefined, step: StepStatus]
>([null, null]);

const currentStatus = getStatus();

Expand All @@ -73,7 +80,7 @@ export default function useStatus(
*/
function updateMotionEndStatus() {
setStatus(STATUS_NONE);
setStyle(null, true);
setStyle([null, null]);
}

const onInternalMotionEnd = useEvent((event: MotionEvent) => {
Expand Down Expand Up @@ -161,11 +168,14 @@ export default function useStatus(
}

// Rest step is sync update
if (step in eventHandlers) {
setStyle(eventHandlers[step]?.(getDomElement(), null) || null);
if (newStep in eventHandlers) {
setStyle([
eventHandlers[newStep]?.(getDomElement(), null) || null,
newStep,
]);
}

if (step === STEP_ACTIVE && currentStatus !== STATUS_NONE) {
if (newStep === STEP_ACTIVE && currentStatus !== STATUS_NONE) {
// Patch events when motion needed
patchMotionEvents(getDomElement());

Expand All @@ -179,7 +189,7 @@ export default function useStatus(
}
}

if (step === STEP_PREPARED) {
if (newStep === STEP_PREPARED) {
updateMotionEndStatus();
}

Expand Down Expand Up @@ -286,13 +296,21 @@ export default function useStatus(
}, [asyncVisible, currentStatus]);

// ============================ Styles ============================
let mergedStyle = style;
let mergedStyle = style[0];
if (eventHandlers[STEP_PREPARE] && step === STEP_START) {
mergedStyle = {
transition: 'none',
...mergedStyle,
};
}

return [getStatus, step, mergedStyle, asyncVisible ?? visible];
const styleStep = style[1];

return [
getStatus,
step,
mergedStyle,
asyncVisible ?? visible,
step === STEP_START || step === STEP_ACTIVE ? styleStep === step : true,
];
}
6 changes: 6 additions & 0 deletions tests/CSSMotion.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -194,6 +194,9 @@ describe('CSSMotion', () => {
});

fireEvent.transitionEnd(container.querySelector('.motion-box'));
act(() => {
jest.runAllTimers();
});

expect(container.querySelector('.motion-box')).toHaveClass('removed');

Expand Down Expand Up @@ -805,6 +808,9 @@ describe('CSSMotion', () => {
});

fireEvent.transitionEnd(container.querySelector('.motion-box'));
act(() => {
jest.runAllTimers();
});

expect(container.querySelector('.motion-box')).toBeTruthy();
expect(container.querySelector('.motion-box')).toHaveClass('removed');
Expand Down
6 changes: 4 additions & 2 deletions tests/StrictMode.spec.tsx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@
react/no-render-return-value, max-classes-per-file,
react/prefer-stateless-function, react/no-multi-comp
*/
import { fireEvent, render } from '@testing-library/react';
import { act, fireEvent, render } from '@testing-library/react';
import { clsx } from 'clsx';
import React from 'react';
import { act } from 'react-dom/test-utils';
import { genCSSMotion, type CSSMotionRef } from '../src/CSSMotion';

describe('StrictMode', () => {
Expand Down Expand Up @@ -52,6 +51,9 @@ describe('StrictMode', () => {

// Trigger End
fireEvent.transitionEnd(node);
act(() => {
jest.runAllTimers();
});
expect(node).not.toHaveClass('transition-appear');

expect(ref.current.inMotion()).toBeFalsy();
Expand Down
Loading