refactor: extract animation logics into hooks#77
Conversation
📝 WalkthroughWalkthroughThe PR refactors the TextMotion component architecture by introducing a new Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
src/hooks/useAnimationEndCallback/useAnimationEndCallback.ts (1)
11-22: Implementation correctly guards against multiple invocations, but consider documenting the non-resetting behavior.The
calledRefis never reset, even when thecallbackprop changes. This means:
- First callback change + invocation → callback fires
- Subsequent invocations → no callback fires (even if callback identity changed)
This is likely correct for animation end handling where you want exactly one notification per component lifecycle, but consider adding a brief comment or updating the JSDoc to clarify this contract.
📝 Suggested documentation improvement
/** * @description * `useAnimationEndCallback` is a custom hook that calls a callback function when the animation ends. * It returns a function that should be passed to the `onAnimationEnd` prop of the animated component. + * The callback is invoked at most once per component mount, regardless of callback identity changes. * * @param {() => void} callback - The callback function to call when the animation ends. * @returns {() => void} A function that should be passed to the `onAnimationEnd` prop of the animated component. */src/components/TextMotion/TextMotion.spec.tsx (3)
65-106: Duplicate test cases forshouldAnimate: truerendering.Lines 65-79 and 92-106 test the same behavior with nearly identical implementations:
- Both set
shouldAnimate: true- Both create the same
animatedChildrenstructure- Both assert on span count and
text-motionclassConsider consolidating these into a single test or differentiating their purposes (e.g., one tests trigger="on-load", another tests intersection-based activation) by varying the mock setup more meaningfully.
143-177: Duplicate test cases foronAnimationStartcallback.Lines 143-153 and 167-177 are effectively identical tests:
- Both set
shouldAnimate: true- Both assert
onAnimationStartwas called onceThe comment on line 167 says "(e.g., intersecting)" but the mock setup is the same as line 147. If these are meant to test different scenarios, consider making the distinction explicit in the test setup or combining them.
220-240: Remove or address commented-out test.The commented-out test for line split with
<br>elements should either be:
- Removed if no longer relevant to the hook-based architecture
- Uncommented and fixed if it's still needed
- Replaced with a TODO comment explaining why it's disabled and when it will be addressed
Leaving commented-out tests without explanation creates maintenance burden and confusion about test coverage.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/components/AnimatedSpan/index.tssrc/components/TextMotion/TextMotion.spec.tsxsrc/components/TextMotion/TextMotion.tsxsrc/hooks/useAnimatedChildren/AnimatedSpan.spec.tsxsrc/hooks/useAnimatedChildren/AnimatedSpan.tsxsrc/hooks/useAnimatedChildren/index.tssrc/hooks/useAnimatedChildren/useAnimatedChildren.tsxsrc/hooks/useAnimationEndCallback/index.tssrc/hooks/useAnimationEndCallback/useAnimationEndCallback.spec.tsxsrc/hooks/useAnimationEndCallback/useAnimationEndCallback.tssrc/hooks/useTextMotionAnimation/index.tssrc/hooks/useTextMotionAnimation/useTextMotionAnimation.spec.tsxsrc/hooks/useTextMotionAnimation/useTextMotionAnimation.tssrc/utils/countNodes/countNodes.spec.tsx
💤 Files with no reviewable changes (1)
- src/components/AnimatedSpan/index.ts
🧰 Additional context used
📓 Path-based instructions (1)
**/*.{ts,tsx}
⚙️ CodeRabbit configuration file
**/*.{ts,tsx}: You are a senior frontend reviewer.
Review this code strictly based on the principles of "easy-to-change code"
from Frontend Fundamentals (Readability, Predictability, Cohesion, Low Coupling).================================
- Readability
================================
- Check if the code can be read top-to-bottom like a document.
- Flag places where the reader must jump around to understand logic.
- Suggest extracting logic when multiple contexts are mixed in one function or component.
- Identify magic numbers or strings and suggest meaningful named constants.
- Point out unclear or abbreviated variable/function names.
- Prefer simple control flow over nested or overly clever logic.
================================
2. Predictability
- Verify that function and variable names clearly imply their behavior.
- Ensure functions with similar responsibilities return consistent data shapes.
- Flag hidden side effects (global state mutation, implicit behavior).
- Warn when the same name is used with different meanings across the codebase.
- Suggest making implicit assumptions explicit.
================================
3. Cohesion
- Check whether code that changes together is located together.
- Flag cases where related logic is scattered across multiple files unnecessarily.
- Suggest grouping constants, helpers, and hooks near the domain they belong to.
- Identify components or functions that contain unrelated responsibilities.
- Prefer feature/domain-based structure over technical grouping when applicable.
================================
4. Coupling
- Identify code where a small change would cause wide ripple effects.
- Warn when abstractions increase coupling instead of reducing it.
- Prefer duplication over premature abstraction if it lowers change impact.
- Flag excessive prop drilling and suggest alternative patterns if appropriate.
- Ensure compo...
Files:
src/hooks/useAnimatedChildren/AnimatedSpan.tsxsrc/hooks/useAnimatedChildren/index.tssrc/hooks/useAnimationEndCallback/index.tssrc/hooks/useTextMotionAnimation/index.tssrc/hooks/useAnimationEndCallback/useAnimationEndCallback.tssrc/hooks/useAnimationEndCallback/useAnimationEndCallback.spec.tsxsrc/hooks/useTextMotionAnimation/useTextMotionAnimation.spec.tsxsrc/components/TextMotion/TextMotion.spec.tsxsrc/hooks/useTextMotionAnimation/useTextMotionAnimation.tssrc/hooks/useAnimatedChildren/useAnimatedChildren.tsxsrc/utils/countNodes/countNodes.spec.tsxsrc/components/TextMotion/TextMotion.tsx
🧬 Code graph analysis (5)
src/hooks/useTextMotionAnimation/useTextMotionAnimation.spec.tsx (3)
src/types/props.ts (1)
TextMotionProps(22-22)src/hooks/useTextMotionAnimation/useTextMotionAnimation.ts (1)
useTextMotionAnimation(17-51)src/types/common.ts (1)
Preset(67-83)
src/components/TextMotion/TextMotion.spec.tsx (3)
src/hooks/useTextMotionAnimation/useTextMotionAnimation.ts (1)
useTextMotionAnimation(17-51)src/components/TextMotion/TextMotion.tsx (1)
TextMotion(70-96)src/constants/defaults.ts (1)
DEFAULT_ARIA_LABEL(9-9)
src/hooks/useTextMotionAnimation/useTextMotionAnimation.ts (3)
src/hooks/useTextMotionAnimation/index.ts (1)
useTextMotionAnimation(1-1)src/types/props.ts (1)
TextMotionProps(22-22)src/hooks/useIntersectionObserver/useIntersectionObserver.spec.tsx (1)
trigger(26-43)
src/utils/countNodes/countNodes.spec.tsx (1)
src/utils/countNodes/countNodes.ts (1)
countNodes(13-29)
src/components/TextMotion/TextMotion.tsx (3)
src/hooks/useValidation/useValidation.ts (1)
useValidation(20-28)src/hooks/useTextMotionAnimation/useTextMotionAnimation.ts (1)
useTextMotionAnimation(17-51)src/constants/defaults.ts (1)
DEFAULT_ARIA_LABEL(9-9)
🔇 Additional comments (11)
src/utils/countNodes/countNodes.spec.tsx (1)
8-11: LGTM! Good edge case coverage.This test complements the existing test at lines 97-107 by testing direct
nullarray input without the JSX/getNodeswrapper. This ensurescountNodeshandles raw null values correctly at the array level, not just nulls filtered through React'sChildren.toArray.src/hooks/useAnimatedChildren/useAnimatedChildren.tsx (1)
9-9: LGTM! Good cohesion improvement.Moving
AnimatedSpancloser to its primary consumer (useAnimatedChildren) follows the principle of colocating code that changes together. This reduces cross-directory coupling and makes the module boundary clearer.src/hooks/useAnimationEndCallback/index.ts (1)
1-1: LGTM!Standard barrel export pattern consistent with other hook modules in the codebase.
src/hooks/useAnimatedChildren/index.ts (1)
1-2: LGTM! Reasonable colocation trade-off.Exporting a component (
AnimatedSpan) from a hooks directory is a slight naming inconsistency, but the cohesion benefit outweighs this concern sinceAnimatedSpanis tightly coupled touseAnimatedChildren. Keeping implementation details near their primary consumer improves maintainability.src/hooks/useTextMotionAnimation/index.ts (1)
1-1: LGTM!Standard barrel export following the established pattern for hook modules.
src/hooks/useAnimatedChildren/AnimatedSpan.tsx (1)
21-32: Clean refactor delegating animation end handling to the dedicated hook.The component now focuses solely on rendering while the
useAnimationEndCallbackhook encapsulates the single-invocation guard logic. This improves cohesion by keeping animation end behavior centralized and reusable.src/hooks/useAnimationEndCallback/useAnimationEndCallback.spec.tsx (1)
34-56: Test coverage is comprehensive, but verify the intended behavior on callback change.The test correctly validates that when the callback changes:
- The old callback is not invoked
- The new callback is invoked once
- Subsequent invocations do not call the new callback again
This means the "called once" guard persists across callback changes. If the intent is to allow each new callback to be invoked once (resetting the guard on change), the hook implementation would need adjustment.
Please confirm: Is the current behavior intentional where changing the callback does NOT reset the single-invocation guard? If a component re-renders with a new
onAnimationEndcallback mid-animation, it would still only fire once total—not once per unique callback.src/hooks/useTextMotionAnimation/useTextMotionAnimation.ts (1)
17-51: Well-structured orchestration hook that cleanly composes animation logic.The hook successfully centralizes the animation orchestration that was previously scattered in
TextMotion:
- Cohesion: Related animation state (
shouldAnimate,targetRef,animatedChildren,text) is computed and returned together- Predictability: Clear derivation of
shouldAnimatefromtriggerand intersection state- Low coupling: TextMotion now depends only on this hook's return value, not on multiple individual hooks
The conditional
splittedNodeon line 38 is a nice touch—it ensuresuseAnimatedChildrenalways receives valid input while respecting hook call order rules.src/hooks/useTextMotionAnimation/useTextMotionAnimation.spec.tsx (1)
16-108: Comprehensive test coverage for the orchestration hook.The test suite effectively validates:
- Correct argument propagation to dependencies (
splitNodeAndExtractText,useResolvedMotion,useAnimatedChildren)shouldAnimatederivation across trigger/intersection combinations- Conditional
splittedNodebehavior (split nodes when animating, original children when not)- Return value composition
The mocking strategy appropriately isolates the hook's orchestration logic from its dependencies.
src/components/TextMotion/TextMotion.spec.tsx (1)
18-33: Good pattern: MockTextMotion helper for driving hook-based test scenarios.The helper cleanly encapsulates the mock setup, making individual tests more readable and focused on the scenario being tested rather than mock boilerplate.
src/components/TextMotion/TextMotion.tsx (1)
70-95: No breaking change — TextMotionProps intentionally restricts accepted props via TypeScript type system.The component correctly implements its documented interface.
TextMotionPropsis explicitly defined in the type system and does not extend HTML element attributes. Consumers cannot passdata-testid,className,style, or other HTML attributes toTextMotion— TypeScript will error if they attempt to do so. This is intentional design, not a silent behavior change.
Description
This PR refactors focusing on improving component cohesion by clearly separating rendering responsibilities from animation-related logic.
TextMotion responsibility separation
TextMotioninto a dedicated hook,useTextMotionAnimation.TextMotionis now primarily responsible for rendering, while animation state and behavior are handled by the hook.AnimatedSpan side-effect isolation
useAnimationEndCallback.Type of change
Checklist
Summary by CodeRabbit
Release Notes
Tests
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.