fix: Style not sync with step status #71
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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此 PR 新增调试演示（文档、示例组件与样式），并在 CSSMotion 中引入 styleReady 管控与 memoized 渲染流程，同时扩展 useStatus 的返回值以包含 styleReady。测试中将定时器推进包装在 React act 中以修正时序断言。 Changes
Sequence Diagram(s)sequenceDiagram
participant App as Demo App / Consumer
participant CSSMotion as CSSMotion
participant useStatus as useStatus Hook
participant Child as Child Element / DOM
App->>CSSMotion: render props (visible, motionName, handlers)
CSSMotion->>useStatus: get status, step, style, visible, styleReady
alt styleReady false
CSSMotion--xChild: 延迟渲染（跳过渲染）
note right of CSSMotion `#f9f9c5`: 等待 styleReady 为真
else styleReady true
CSSMotion->>CSSMotion: 更新 idRef，进入 memoized 渲染
CSSMotion->>Child: 注入 ref / 克隆并传入 class/style
Child->>CSSMotion: onAppear/onEnter callbacks (开始/激活)
CSSMotion->>useStatus: 通知步骤变化（prepare -> active -> end）
CSSMotion->>Child: 动画结束触发 transitionEnd
end
Estimated code review effort🎯 3 (中等) | ⏱️ ~25 分钟 Possibly related PRs
诗
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Organization UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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 |
Summary of ChangesHello @zombieJ, 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! 此拉取请求解决了 Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe 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 by creating a comment using either
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 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
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
你好，感谢你的贡献。这个 PR 旨在修复一个因 reflow 导致动画样式与状态不同步的边界情况。核心改动位于 useStatus hook，通过将 style 与 step 状态绑定来确保同步，并引入 styleReady 标志。CSSMotion 组件则利用此标志来控制渲染更新。整体解决方案的思路是正确的，并且添加了用于调试的 demo 和相应的测试，这很棒。
我在 src/CSSMotion.tsx 中发现了一个潜在问题。当前使用 React.useMemo 和 useRef 的实现方式虽然巧妙，但存在一些风险，可能导致组件对 props 的变化不响应。我在代码中留下了具体的反馈和建议，希望能帮助你完善这个修复。请查看我的评论。
src/CSSMotion.tsx
Outdated
| 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]); |
There was a problem hiding this comment.
这部分代码通过 idRef 和 useMemo 来解决样式和状态不同步时渲染旧内容的问题，想法很巧妙，但存在一些问题：
-
不完整的依赖项数组:
React.useMemo的依赖数组只有[idRef.current]，这是不完整的。当children,visible等外部 props 发生变化时，如果styleReady恰好为false，useMemo不会重新计算，从而返回了旧的、缓存的motionChildren，导致渲染结果与新的 props 不一致。这会破坏组件的响应性，是一个潜在的 bug。 -
渲染期间的副作用: 在渲染函数中直接修改
ref的current属性 (idRef.current += 1;) 是一个副作用。在 React 的严格模式（Strict Mode）下，渲染函数可能会被调用多次，这会导致idRef.current被意外地增加多次，可能引发非预期的行为。
建议重构这部分逻辑以提高代码的健壮性和可预测性。一个更符合 React 模式的做法是避免在渲染期间产生副作用。例如，可以考虑使用 useState 和 useLayoutEffect 来缓存并同步渲染内容，确保只在 styleReady 为 true 时才更新输出。虽然这可能引入额外的渲染周期，但代码会更清晰和安全。
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/CSSMotion.tsx (1)
185-189: 建议重新考虑使用 ref 作为 useMemo 依赖的模式。使用
idRef.current作为useMemo的依赖项是一种非常规的模式。虽然它能在styleReady变为 true 时触发更新，但 ref 的变化本身不会触发重新渲染，依赖于先前的渲染来增加计数器值。这个模式能工作是因为
styleReady的变化会触发useStatus内部的状态更新，从而导致重新渲染。但代码的意图不够明确，建议添加注释说明这种模式的工作原理。
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
docs/demo/debug.mddocs/examples/debug.lessdocs/examples/debug.tsxsrc/CSSMotion.tsxsrc/hooks/useStatus.tstests/CSSMotion.spec.tsxtests/StrictMode.spec.tsx
🧰 Additional context used
🧬 Code graph analysis (3)
src/hooks/useStatus.ts (2)
src/hooks/useStepQueue.ts (1)
status(33-89)src/interface.ts (7)
MotionStatus(6-10)StepStatus(23-30)STATUS_NONE(1-1)STEP_ACTIVE(15-15)STEP_PREPARED(21-21)STEP_PREPARE(13-13)STEP_START(14-14)
src/CSSMotion.tsx (4)
src/hooks/useStatus.ts (1)
useStatus(27-316)src/hooks/useStepQueue.ts (2)
status(33-89)isActive(29-31)src/interface.ts (3)
STATUS_NONE(1-1)STEP_PREPARE(13-13)STEP_START(14-14)src/util/motion.ts (1)
getTransitionName(87-101)
docs/examples/debug.tsx (1)
src/CSSMotion.tsx (1)
CSSMotionProps(42-99)
🪛 GitHub Actions: ✅ test
src/CSSMotion.tsx
[error] 125-125: Type '(props: CSSMotionProps, ref: ForwardedRef) => string | number | boolean | ReactElement<any, string | JSXElementConstructor> | Iterable<...>' is not assignable to parameter of type 'ForwardRefRenderFunction<CSSMotionRef, CSSMotionProps>'. Type 'string | number | boolean | ReactElement<any, string | JSXElementConstructor> | Iterable' is not assignable to type 'ReactElement<any, string | JSXElementConstructor>'.
🔇 Additional comments (13)
docs/examples/debug.less (1)
1-17: LGTM!调试演示样式正确实现了动画状态的可视化效果。
start状态设置opacity: 0，active状态过渡到opacity: 1，符合 PR 描述中验证 style 与 step status 同步的目的。docs/demo/debug.md (1)
1-3: LGTM!文档结构清晰，正确引用了调试示例文件。
tests/StrictMode.spec.tsx (2)
5-5: LGTM!从
@testing-library/react导入act是正确的做法，react-dom/test-utils中的act在 React 18+ 中已被弃用。
54-56: LGTM!在
transitionEnd后使用act()包装jest.runAllTimers()确保状态更新被正确刷新，与新增的styleReady门控逻辑配合良好。tests/CSSMotion.spec.tsx (2)
197-199: LGTM!与
StrictMode.spec.tsx的修改保持一致，确保transitionEnd后的状态更新被正确处理。
811-813: LGTM!正确地在
transitionEnd后使用act()包装定时器刷新，与新的渲染门控逻辑兼容。docs/examples/debug.tsx (3)
6-14: LGTM!回调函数正确实现了折叠/展开动画的起始和激活状态样式，
console.log语句有助于调试动画阶段。
29-56: LGTM!
CSSMotion的使用方式正确，通过key属性变化来触发重新挂载和动画，能够有效验证 PR 中修复的 style 与 step status 同步问题。
61-65: LGTM!使用
React.StrictMode包装是良好的实践，有助于在开发环境中发现潜在问题。src/hooks/useStatus.ts (4)
51-57: LGTM!返回类型扩展为 5 元组，新增的
styleReady字段清晰地表达了样式是否与当前步骤状态同步的语义。
61-63: LGTM!将
style状态从单一值改为[style, step]元组是解决 PR 描述中问题的关键。这样可以追踪样式是为哪个步骤计算的，确保渲染时使用的样式与当前步骤一致。
171-175: LGTM!在设置样式时同时记录对应的步骤，确保后续可以验证样式与步骤的同步状态。
307-315: 核心修复逻辑正确。
styleReady的计算逻辑解决了 PR 描述中的问题：
- 当
step为STEP_START或STEP_ACTIVE时，只有在styleStep === step时才返回true- 其他情况（如
STEP_NONE、STEP_PREPARE等）直接返回true这确保了在
start→active过渡期间，只有当样式确实为当前步骤计算时才会渲染，避免了使用前一步骤样式导致动画被跳过的问题。
src/CSSMotion.tsx
Outdated
| 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]); |
There was a problem hiding this comment.
流水线类型错误：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.
| 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.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #71 +/- ##
==========================================
+ Coverage 98.48% 98.51% +0.03%
==========================================
Files 11 11
Lines 395 405 +10
Branches 108 112 +4
==========================================
+ Hits 389 399 +10
Misses 6 6 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
在非常罕见的情况下，会出现 React 的 layoutEffect update 中强行触发一次 reflow 导致页面被渲染出来。
由于 stepStyle 是通过 layoutEffect 监听到 stepStatus 更新而触发 setState 会导致因为 reflow 而使得 stepStyle 使用的是上一个 stepStatus 的 style 进行渲染，从而导致了 start -> active 状态使用了相同的 style 而跳过动画。
Summary by CodeRabbit
发布说明
✏️ Tip: You can customize this high-level summary in your review settings.