Conversation
📝 WalkthroughWalkthroughRenames and migrations: splitNodeAndExtractText → splitReactNode, updates callers and tests to use Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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! |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Fix all issues with AI agents
In `@src/hooks/useAnimatedChildren/useAnimatedChildren.spec.tsx`:
- Around line 195-225: The test named "triggers onAnimationEnd callback when
last node animation ends" currently only asserts that onAnimationEndMock is
defined; replace that check with a real invocation assertion (e.g.,
expect(onAnimationEndMock).toHaveBeenCalled() or
expect(onAnimationEndMock).toHaveBeenCalledTimes(1)) after firing the
animationEnd on lastSpan so the test verifies the onAnimationEnd callback was
actually invoked.
- Around line 138-153: The test only asserts the mock exists rather than that it
was invoked; update the test for useAnimatedChildren to render the generated
nodes into the DOM (from splitReactNode -> nodes), find the last animated
element (e.g., last span), dispatch/fire an 'animationend' event on that
element, and then assert onAnimationEndMock was called (e.g.,
toHaveBeenCalled()). Ensure you keep initialDelay: 0 and animationOrder:
'first-to-last' so the last node is the target, and use the same
onAnimationEndMock passed into useAnimatedChildren.
In `@src/hooks/useTextMotionAnimation/useTextMotionAnimation.spec.tsx`:
- Line 20: Rename the mock variable to match the imported function: change the
declaration to use mockSplitReactNode (e.g., const mockSplitReactNode =
splitReactNode as jest.Mock) and replace every usage of
mockSplitNodeAndExtractText in this test file with mockSplitReactNode so
references (mockReturnValue, mockImplementation, assertions) correctly reflect
the mocked splitReactNode function.
In `@src/utils/splitReactNode/splitReactNode.ts`:
- Around line 12-22: The JSDoc block references the wrong function name
(`splitNodeAndExtractText`) while the actual implementation is `splitReactNode`;
update the JSDoc to match the real function name and signatures (replace
occurrences of splitNodeAndExtractText with splitReactNode and ensure
param/return descriptions align with the function's parameters `node` and
`split` and return type `SplitResult`) so the doc comment accurately describes
`splitReactNode`.
In `@src/utils/splitText/splitText.spec.ts`:
- Around line 18-24: The test's name is inaccurate: splitText currently returns
the passed invalid value via the exhaustiveCheck path rather than defaulting to
a character split; update the test description to reflect that behavior (e.g.,
rename the it block to something like "should return the invalid split value
(exhaustiveCheck)" or similar) and keep the expectation that splitText(input,
invalidSplit) returns the invalid value; refer to the splitText function and the
exhaustiveCheck/default switch path when making the change.
🧹 Nitpick comments (8)
src/utils/generateAnimation/generateAnimation.ts (1)
32-48: Good refactor - Early return and explicit type guard improve readability.The early
returnafter handling custom animations (Line 35) eliminates implicit fall-through and makes the control flow clearer. The explicit type guard on Line 38 (typeof config === 'object' && config !== null && 'variant' in config) properly narrows the type before processing standard animations.One minor observation:
Object.assign(style, customProps)mutatesstylein-place, while the final return on Lines 51-56 creates a new object. This is fine sincestyleis locally scoped, but the mixed mutation/immutable patterns could be slightly confusing. Consider using spread for consistency if preferred:Object.entries(rest).forEach(([key, value]) => { if (value !== undefined && value !== null) { style[`--${name}-${key}`] = value as string | number; } });This is a minor style preference and not blocking.
src/utils/typeGuards/typeGuards.ts (1)
29-40: Consider renamingisElementWithChildrenfor predictability.The simplified implementation (Line 39) now accepts any valid React element, regardless of whether it has children. However, the function name
isElementWithChildrenand the JSDoc (Lines 31-32: "checks if a React node is a valid React element with children") imply that children must be present.The return type
ReactElement<{ children?: ReactNode }>correctly indicates children are optional, but the name creates a misleading expectation.Suggestion: Rename to
isReactElementorisElementNodeto match the actual behavior, or update the JSDoc to clarify the intent (e.g., "checks if a node is a React element that can contain children").As per coding guidelines on Predictability: "Verify that function and variable names clearly imply their behavior."
Option 1: Rename for clarity
-export const isElementWithChildren = (node: ReactNode): node is ReactElement<{ children?: ReactNode }> => { +export const isReactElement = (node: ReactNode): node is ReactElement<{ children?: ReactNode }> => { return isValidElement<{ children?: ReactNode }>(node); };Option 2: Update JSDoc to clarify intent
/** * `@description` - * Type guard function that checks if a React node is a valid React element with children. - * It returns `true` if the node is a valid React element with children, otherwise `false`. + * Type guard function that checks if a React node is a valid React element that may contain children. + * It returns `true` if the node is a valid React element, otherwise `false`. * * `@param` {ReactNode} node - The React node to check. * - * `@returns` {boolean} `true` if the node is a valid React element with children, otherwise `false`. + * `@returns` {boolean} `true` if the node is a valid React element, otherwise `false`. */src/utils/countNodes/countNodes.spec.tsx (2)
24-32: Verify the whitespace text node count.The test expects 3 nodes for
{'Hello'} {'World'}, which implies the whitespace between the two JSX expressions is counted as a separate text node. This is correct JSX behavior—the space between}and{becomes its own text node.Consider adding a brief comment to make this whitespace behavior explicit for future readers, as it's a subtle JSX detail that could confuse maintainers.
104-117: Clarify the custom component test behavior.This test validates that
countNodescounts children passed to a component, not the component's rendered output. The name "counts text nodes rendered by a functional component" could be misleading sincecountNodesoperates on the React element tree, not the DOM.Consider renaming to something like "counts children passed to a custom component" to better reflect the actual behavior being tested.
src/utils/splitReactNode/splitReactNode.spec.tsx (2)
62-74: Consider documenting the "unsupported type" fallback behavior.Testing Symbol and function inputs exercises the final fallback branch that returns
{ nodes: [node], text: '' }. While this is valid defensive testing, consider whether this fallback behavior is intentional API design or just a safety net.If intentional, a brief comment in the implementation explaining when this fallback applies would help future maintainers understand the design decision.
3-85: Consider grouping tests into describe blocks for consistency.The
countNodes.spec.tsxfile uses nesteddescribeblocks to organize tests by category (non-renderable nodes, text nodes, element nodes, etc.). For consistency across the codebase, consider applying the same structure here:describe('splitReactNode utility', () => { describe('text nodes', () => { /* string/number tests */ }); describe('element nodes', () => { /* React element tests */ }); describe('arrays', () => { /* array tests */ }); describe('non-renderable nodes', () => { /* null/boolean tests */ }); describe('split modes', () => { /* character vs word tests */ }); });This would improve cohesion with the related test file structure. As per coding guidelines, code that changes together should be located together—and consistent test organization aids maintainability.
src/utils/splitReactNode/splitReactNode.ts (1)
59-62: Consider movingmergeResultsbefore its usage.Per readability principles, code should read top-to-bottom. Currently, readers encounter
mergeResultson line 37 before its definition on line 59, requiring them to jump around. Moving helper functions above their first usage improves flow.src/hooks/useAnimatedChildren/useAnimatedChildren.spec.tsx (1)
227-249: Test should verify the updated callback is wired correctly.This test rerenders with a new callback but only checks it's defined. Consider firing
animationEndafter rerender and verifyingonAnimationEndMock2is called (notonAnimationEndMock1).
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
src/hooks/useAnimatedChildren/useAnimatedChildren.spec.tsxsrc/hooks/useAnimatedChildren/useAnimatedChildren.tsxsrc/hooks/useTextMotionAnimation/useTextMotionAnimation.spec.tsxsrc/hooks/useTextMotionAnimation/useTextMotionAnimation.tssrc/utils/accessibility/accessibility.tssrc/utils/countNodes/countNodes.spec.tsxsrc/utils/countNodes/countNodes.tssrc/utils/generateAnimation/generateAnimation.tssrc/utils/sequenceHelpers/sequenceHelpers.tssrc/utils/splitNodeAndExtractText/index.tssrc/utils/splitNodeAndExtractText/splitNodeAndExtractText.spec.tsxsrc/utils/splitNodeAndExtractText/splitNodeAndExtractText.tssrc/utils/splitReactNode/index.tssrc/utils/splitReactNode/splitReactNode.spec.tsxsrc/utils/splitReactNode/splitReactNode.tssrc/utils/splitText/splitText.spec.tssrc/utils/splitText/splitText.tssrc/utils/typeGuards/typeGuards.spec.tssrc/utils/typeGuards/typeGuards.ts
💤 Files with no reviewable changes (3)
- src/utils/splitNodeAndExtractText/splitNodeAndExtractText.spec.tsx
- src/utils/splitNodeAndExtractText/index.ts
- src/utils/splitNodeAndExtractText/splitNodeAndExtractText.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/utils/splitText/splitText.spec.tssrc/utils/accessibility/accessibility.tssrc/utils/generateAnimation/generateAnimation.tssrc/utils/countNodes/countNodes.tssrc/utils/typeGuards/typeGuards.spec.tssrc/hooks/useTextMotionAnimation/useTextMotionAnimation.tssrc/utils/splitReactNode/index.tssrc/hooks/useAnimatedChildren/useAnimatedChildren.spec.tsxsrc/utils/typeGuards/typeGuards.tssrc/utils/splitReactNode/splitReactNode.spec.tsxsrc/utils/sequenceHelpers/sequenceHelpers.tssrc/hooks/useTextMotionAnimation/useTextMotionAnimation.spec.tsxsrc/utils/countNodes/countNodes.spec.tsxsrc/utils/splitReactNode/splitReactNode.tssrc/hooks/useAnimatedChildren/useAnimatedChildren.tsxsrc/utils/splitText/splitText.ts
🧬 Code graph analysis (11)
src/utils/splitText/splitText.spec.ts (1)
src/utils/splitText/splitText.ts (1)
splitText(12-28)
src/utils/generateAnimation/generateAnimation.ts (2)
src/constants/animationDefaults.ts (1)
ANIMATION_DEFAULTS(19-22)src/types/animations.ts (2)
CustomAnimation(141-143)StandardAnimation(123-130)
src/utils/countNodes/countNodes.ts (1)
src/utils/typeGuards/typeGuards.ts (3)
isNonRenderableNode(25-27)isTextNode(12-14)isElementWithChildren(38-40)
src/utils/typeGuards/typeGuards.spec.ts (1)
src/utils/typeGuards/typeGuards.ts (2)
isNonRenderableNode(25-27)isElementWithChildren(38-40)
src/hooks/useTextMotionAnimation/useTextMotionAnimation.ts (1)
src/hooks/useAnimatedChildren/useAnimatedChildren.tsx (1)
useAnimatedChildren(37-67)
src/hooks/useAnimatedChildren/useAnimatedChildren.spec.tsx (2)
src/hooks/useAnimatedChildren/useAnimatedChildren.tsx (1)
useAnimatedChildren(37-67)src/hooks/useAnimatedChildren/index.ts (1)
useAnimatedChildren(2-2)
src/utils/splitReactNode/splitReactNode.spec.tsx (1)
src/utils/splitReactNode/splitReactNode.ts (1)
splitReactNode(23-57)
src/hooks/useTextMotionAnimation/useTextMotionAnimation.spec.tsx (1)
src/hooks/useAnimatedChildren/useAnimatedChildren.tsx (1)
useAnimatedChildren(37-67)
src/utils/countNodes/countNodes.spec.tsx (1)
src/utils/countNodes/countNodes.ts (1)
countNodes(14-34)
src/utils/splitReactNode/splitReactNode.ts (3)
src/utils/splitReactNode/index.ts (1)
splitReactNode(1-1)src/types/common.ts (1)
Split(18-18)src/utils/typeGuards/typeGuards.ts (3)
isNonRenderableNode(25-27)isTextNode(12-14)isElementWithChildren(38-40)
src/hooks/useAnimatedChildren/useAnimatedChildren.tsx (2)
src/utils/countNodes/countNodes.ts (1)
countNodes(14-34)src/types/common.ts (2)
AnimationOrder(34-34)Motion(50-59)
🔇 Additional comments (24)
src/utils/accessibility/accessibility.ts (1)
1-10: LGTM!The JSDoc formatting improvement adds visual separation between sections, enhancing documentation readability. The function itself is well-designed: single responsibility, clear naming, and type-safe return signature.
src/utils/sequenceHelpers/sequenceHelpers.ts (2)
3-20: LGTM!The JSDoc formatting improvement (blank line before
@returns) follows standard documentation conventions. The function itself is pure, well-named, and has a single clear responsibility.
22-34: LGTM!Consistent JSDoc formatting with
calculateSequenceIndex. The function is simple, pure, and the name clearly communicates its intent.src/utils/generateAnimation/generateAnimation.ts (2)
27-27: LGTM - Clean initialization.Initializing
styleas an empty object is cleaner and avoids unnecessary type assertions.
51-56: LGTM - Clear return structure.The return object explicitly shows the final shape with
animationat the top and custom properties spread in. This is readable and predictable.src/utils/splitText/splitText.ts (2)
14-15: Good choice using spread for character splitting.Using
[...text]instead oftext.split('')correctly handles multi-byte Unicode characters (e.g., emojis, accented characters). This is a subtle but important improvement for text processing utilities.
23-26: LGTM - Exhaustive switch enforcement.The
nevertype check in the default case ensures compile-time errors if newSplitvariants are added without updating this function. This is an excellent pattern for type-safe exhaustive handling.src/utils/typeGuards/typeGuards.spec.ts (3)
3-3: LGTM - Import updated for renamed function.The import correctly reflects the rename from
isNullishNodetoisNonRenderableNode.
38-67: LGTM - Tests properly updated for renamed type guard.The test suite correctly covers the
isNonRenderableNodebehavior. The new name is more semantically accurate since booleans are not "nullish" (they're truthy/falsy values) but they are non-renderable in React's rendering model.
81-83: Verify: Function name suggests "with children" but implementation accepts elements without children.The test now expects
isElementWithChildrento returntruefor elements created without a children prop. Per the implementation insrc/utils/typeGuards/typeGuards.ts(lines 37-39), this function now simply checksisValidElement()without verifying children exist.This creates a naming/behavior mismatch:
- Name implies: Element must have children
- Behavior: Any valid React element passes
If this is intentional (e.g., checking for elements that can have children rather than elements that do have children), consider renaming to
isReactElementorisElementNodefor predictability. Otherwise, the current name may confuse future maintainers. As per coding guidelines, function names should clearly imply their behavior.src/utils/typeGuards/typeGuards.ts (1)
16-27: LGTM - Rename improves semantic clarity.Renaming
isNullishNodetoisNonRenderableNodeis a good choice. The term "nullish" typically refers tonull | undefined, but this guard also includesboolean. "Non-renderable" accurately describes all three types that React skips during rendering.src/utils/countNodes/countNodes.ts (2)
3-3: LGTM - Import updated for renamed type guard.Correctly imports the renamed
isNonRenderableNodefunction.
14-34: LGTM - Clean refactor with improved readability.The refactored implementation is well-structured:
- Clear sequential checks - Early returns for non-renderable (→ 0) and text nodes (→ 1) eliminate nesting
- Consistent type guard usage - All three guards from
typeGuards.tsare used appropriately- Simplified API - Single
ReactNodeparameter is more intuitive than requiring an array wrapperThe edge case where
isElementWithChildrenreturns true butnode.props.childrenis undefined is safely handled:countNodes(undefined)will hit theisNonRenderableNodecheck and return 0.This aligns well with the PR objective of reducing complexity and improving maintainability.
src/utils/countNodes/countNodes.spec.tsx (1)
6-16: Well-structured test organization for non-renderable nodes.The grouping of null, undefined, boolean, and empty fragment tests clearly documents the expected behavior for edge cases. This aligns well with the
isNonRenderableNodeguard in the implementation.src/utils/splitReactNode/index.ts (1)
1-1: Clean public API export.The barrel export correctly exposes the renamed
splitReactNodeutility, aligning with the PR objective to improve naming clarity.src/hooks/useTextMotionAnimation/useTextMotionAnimation.ts (1)
33-43: Clean integration with renamed utility and improved naming.The refactor from
splittedNodetonodesimproves readability (fixing the grammatical issue). The conditionalnodes: shouldAnimate ? nodes : [children]correctly maintains a consistent array shape foruseAnimatedChildrenregardless of animation state.src/utils/splitReactNode/splitReactNode.spec.tsx (1)
3-85: Comprehensive test coverage for the new utility.The tests thoroughly cover the
splitReactNodebehavior across various input types. The coverage includes primitive types, React elements, arrays, and edge cases.src/hooks/useTextMotionAnimation/useTextMotionAnimation.spec.tsx (2)
37-40: LGTM!Test correctly verifies that
splitReactNodeis called with the expected arguments and aligns with the renamed function.
72-83: LGTM!The test properly validates that
useAnimatedChildrenreceives thenodesproperty (previouslysplittedNode), maintaining consistency with the API change.src/utils/splitReactNode/splitReactNode.ts (1)
23-57: Well-structured recursive implementation.The function handles all ReactNode variants with clear type guards, maintains consistent return shapes across branches, and properly preserves element structure through
cloneElement. The single-responsibility branches align well with the PR's goal of reducing complexity.src/hooks/useAnimatedChildren/useAnimatedChildren.spec.tsx (1)
288-307: LGTM!Good addition testing newline character handling. The test verifies that
<br>elements are rendered for newlines, which is important edge case coverage.src/hooks/useAnimatedChildren/useAnimatedChildren.tsx (3)
11-17: Good API improvement.Renaming
splittedNodetonodesbetter reflects the parameter's purpose (an array of React nodes) and aligns with standard React terminology. The addition of the optionalonAnimationEndcallback provides useful extensibility.
44-48: LGTM - correct stable callback pattern.Using a ref to hold
onAnimationEndand updating it via effect ensures the callback remains stable for memoization while always invoking the latest version. This prevents unnecessary re-renders when the callback identity changes.
69-115: LGTM!The internal
wrapWithAnimatedSpanfunction is updated consistently with the public API changes. The renaming fromsplittedNodetonodesand the return object structure{ nodes: animatedNodes, nextSequenceIndex }maintain clarity and predictability.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
src/hooks/useTextMotionAnimation/useTextMotionAnimation.spec.tsx
Outdated
Show resolved
Hide resolved
1 participant
Description
This PR refactors utility functions to improve readability, type safety, and maintainability.
Key changes include:
This refactor is focused on internal code quality and long-term maintainability.
Related Issue
Closes #79
Type of change
Checklist
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.