refactor: optimize animation hooks and improve conditional aria-label handling#78
Conversation
📝 WalkthroughWalkthroughRemoves the "line" split option across types, validation, docs, tests, and split logic; adds getAriaLabel accessibility util; and stabilizes onAnimationStart/onAnimationEnd via refs to avoid re-running memoized animation child computation. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant TextMotion
participant useAnimatedChildren
participant wrapWithAnimatedSpan
participant DOM
User->>TextMotion: render(children, split, onAnimationStart, onAnimationEnd)
TextMotion->>useAnimatedChildren: compute animated children (memoized)
useAnimatedChildren->>wrapWithAnimatedSpan: wrap nodes (passes onAnimationEndRef)
wrapWithAnimatedSpan->>DOM: create animated spans (attach end callback via ref)
TextMotion->>TextMotion: sync onAnimationStart -> onAnimationStartRef
DOM-->>TextMotion: animationstart event -> onAnimationStartRef.current()
DOM-->>useAnimatedChildren: animationend event -> onAnimationEndRef.current()
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
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: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/hooks/useAnimatedChildren/useAnimatedChildren.tsx (1)
44-64: Potential stale closure: ref is dereferenced at memo time, not invocation time.The optimization captures
onAnimationEndRef.currentwhen theuseMemoruns (line 60), not when the animation actually ends. If theonAnimationEndprop changes after memoization but before animation completion, the old callback executes.To fix, pass the ref object to
wrapWithAnimatedSpanand dereference.currentat the invocation site (line 85).Proposed fix
const animatedChildren = useMemo(() => { const totalNodes = countNodes(splittedNode); const { nodes } = wrapWithAnimatedSpan( splittedNode, 0, initialDelay, animationOrder, resolvedMotion, totalNodes, - onAnimationEndRef.current + onAnimationEndRef ); return nodes; }, [splittedNode, initialDelay, animationOrder, resolvedMotion]);Then update
wrapWithAnimatedSpansignature and usage:const wrapWithAnimatedSpan = ( splittedNode: ReactNode[], currentSequenceIndex: number, initialDelay: number, animationOrder: AnimationOrder, resolvedMotion: Motion, totalNodes: number, - onAnimationEnd?: () => void + onAnimationEndRef?: React.RefObject<(() => void) | undefined> ): WrapResult => { // ... - const handleAnimationEnd = isLast ? onAnimationEnd : undefined; + const handleAnimationEnd = isLast ? () => onAnimationEndRef?.current?.() : undefined;src/utils/splitText/splitText.ts (1)
8-8: Update JSDoc to reflect removal of 'line' split type.The documentation still lists
lineas a valid split type, but the implementation no longer supports it. This inconsistency will confuse API consumers.📝 Suggested fix
- * @param {Split} split - The split type (`character`, `word`, or `line`). + * @param {Split} split - The split type (`character` or `word`).
🧹 Nitpick comments (8)
src/utils/accessibility/accessibility.spec.ts (1)
8-10: Consider adding a whitespace-only test case.The current tests cover non-empty and empty strings, but a whitespace-only input (
' ') would validate thetrim()behavior—ensuring it returns{}rather than an aria-label with only spaces.Suggested additional test
it('should return empty object when text is empty', () => { expect(getAriaLabel('')).toEqual({}); }); + + it('should return empty object when text is whitespace only', () => { + expect(getAriaLabel(' ')).toEqual({}); + });src/components/TextMotion/TextMotion.spec.tsx (1)
265-280: Remove commented-out tests instead of leaving dead code.If the
split="line"feature is permanently removed, these commented-out tests should be deleted rather than left in place. Dead code increases cognitive load for future maintainers and contradicts the principle of keeping tests in sync with the codebase.The same applies to the commented-out test block at lines 220-240.
♻️ Suggested approach
Delete the entire commented block (lines 265-280) and the earlier commented block (lines 220-240) completely if the "line" split feature won't be restored.
If you anticipate potentially restoring this feature, consider tracking it in an issue or ADR instead of leaving commented code in the test suite.
src/hooks/useValidation/validation.spec.tsx (1)
30-38: Remove commented-out test instead of leaving dead code.Same concern as in
TextMotion.spec.tsx— if thesplit="line"feature is permanently removed, delete this test block entirely rather than commenting it out.src/utils/splitText/splitText.spec.ts (1)
7-11: Remove commented-out test case from array.Leaving a commented line inside an array literal is particularly confusing. Delete line 10 entirely to keep the test data clean.
♻️ Proposed fix
const testCases: [Split, string, string[]][] = [ ['character', 'Hello', ['H', 'e', 'l', 'l', 'o']], ['word', 'Hello World', ['Hello', ' ', 'World']], - // ['line', 'Hello\nWorld', ['Hello', '\n', 'World']], ];src/types/common.ts (1)
12-18: Type definition is clean, but clarify if this is an actual breaking change.The
Splittype definition and JSDoc accurately describe the'character'and'word'options. However, the claim that removing'line'is a breaking change needs validation:
- No usages of
split="line"or similar patterns exist in the codebase- The package is at version
0.0.9(pre-1.0), so API changes are typically expected- There is no CHANGELOG or documentation mentioning this removal
If
'line'was previously documented as a public feature, add a note to the PR or CHANGELOG. Otherwise, this is a safe internal refinement. Confirm which applies and update the PR description accordingly.src/utils/splitText/splitText.ts (1)
16-17: Remove commented-out code instead of leaving it in place.Commented-out code adds noise and creates confusion about whether this is intentionally disabled or pending future work. Since 'line' support is being removed across the codebase (types, validation, tests), this dead code should be deleted.
♻️ Suggested fix
case 'word': return text.split(/(\s+)/).filter(Boolean); - // case 'line': - // return text.split(/(\n)/).filter(Boolean); case 'character': default: return text.split('');src/hooks/useValidation/validation.ts (1)
25-27: Remove commented-out validation code.Same concern as in
splitText.ts— this dead code should be deleted rather than commented out. The 'line' split type is being fully removed, so this validation branch is no longer needed.♻️ Suggested fix
if (props.children === undefined || props.children === null) { warnings.push('children prop is empty'); } - // if (props.split === 'line' && typeof props.children !== 'string') { - // warnings.push('split="line" is only applicable when children is a string.'); - // } - const common = validateCommonProps(props);src/components/TextMotion/TextMotion.tsx (1)
99-107: Remove unnecessary IIFE wrapper.The IIFE (Immediately Invoked Function Expression) adds cognitive overhead without providing any benefit here. The
ariaPropsvariable can be computed directly before the return statement, matching the pattern used in the non-animating branch above (lines 90-96). This improves readability by keeping consistent patterns within the component.♻️ Suggested fix
- return (() => { - const ariaProps = getAriaLabel(text || DEFAULT_ARIA_LABEL); + const ariaProps = getAriaLabel(text || DEFAULT_ARIA_LABEL); - return ( - <Tag ref={targetRef} className="text-motion" {...ariaProps}> - {animatedChildren} - </Tag> - ); - })(); + return ( + <Tag ref={targetRef} className="text-motion" {...ariaProps}> + {animatedChildren} + </Tag> + );
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (13)
README.mdsrc/components/TextMotion/TextMotion.spec.tsxsrc/components/TextMotion/TextMotion.tsxsrc/hooks/useAnimatedChildren/useAnimatedChildren.tsxsrc/hooks/useValidation/useValidation.spec.tsxsrc/hooks/useValidation/validation.spec.tsxsrc/hooks/useValidation/validation.tssrc/types/common.tssrc/utils/accessibility/accessibility.spec.tssrc/utils/accessibility/accessibility.tssrc/utils/accessibility/index.tssrc/utils/splitText/splitText.spec.tssrc/utils/splitText/splitText.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/accessibility/index.tssrc/hooks/useValidation/useValidation.spec.tsxsrc/utils/accessibility/accessibility.tssrc/utils/splitText/splitText.spec.tssrc/utils/splitText/splitText.tssrc/utils/accessibility/accessibility.spec.tssrc/hooks/useAnimatedChildren/useAnimatedChildren.tsxsrc/hooks/useValidation/validation.tssrc/components/TextMotion/TextMotion.spec.tsxsrc/types/common.tssrc/components/TextMotion/TextMotion.tsxsrc/hooks/useValidation/validation.spec.tsx
🧬 Code graph analysis (3)
src/hooks/useValidation/useValidation.spec.tsx (1)
src/utils/splitNodeAndExtractText/splitNodeAndExtractText.spec.tsx (6)
splitNodeAndExtractText(51-60)splitNodeAndExtractText(23-28)splitNodeAndExtractText(37-42)splitNodeAndExtractText(76-84)symbolNode(62-74)splitNodeAndExtractText(44-49)
src/utils/splitText/splitText.ts (1)
src/utils/splitNodeAndExtractText/splitNodeAndExtractText.spec.tsx (5)
splitNodeAndExtractText(23-28)splitNodeAndExtractText(51-60)splitNodeAndExtractText(76-84)splitNodeAndExtractText(4-11)it(3-85)
src/utils/accessibility/accessibility.spec.ts (1)
src/utils/accessibility/accessibility.ts (1)
getAriaLabel(7-9)
🪛 LanguageTool
README.md
[style] ~98-~98: To form a complete sentence, be sure to include a subject or ‘there’.
Context: ... | Content to animate. Can be a string, a number, or any React ele...
(MISSING_IT_THERE)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Run Coverage
🔇 Additional comments (9)
src/utils/accessibility/accessibility.ts (1)
1-9: LGTM!Clean, focused utility with a clear single responsibility. The JSDoc is helpful, and the return type correctly models the optional property. The decision to trim only for the empty check while preserving original text in the label is reasonable for accessibility.
src/utils/accessibility/index.ts (1)
1-1: LGTM!Standard barrel export pattern that enables clean imports from the directory root.
README.md (1)
96-108: LGTM!The API reference table correctly reflects the removal of the
"line"split option. Documentation is clear and consistent with the code changes.src/hooks/useValidation/useValidation.spec.tsx (1)
23-26: LGTM!The updated error message expectation correctly reflects the removal of
'line'from valid split values, maintaining consistency with the updatedSplittype definition.src/hooks/useValidation/validation.spec.tsx (1)
40-47: LGTM!The valid splits array correctly reflects only
'character'and'word', consistent with the updatedSplittype.src/hooks/useValidation/validation.ts (1)
48-49: LGTM!The validation logic correctly narrows the allowed split values to
'character'and'word', with a clear error message. This is consistent with the type definition changes.src/components/TextMotion/TextMotion.tsx (3)
77-87: LGTM on the useRef callback pattern.This correctly stabilizes the
onAnimationStartcallback reference, preventing the effect from re-running when the callback identity changes while ensuring the latest callback is always invoked. The simplified[shouldAnimate]dependency is intentional and correct since the ref is stable.
89-96: LGTM on the ariaProps pattern.The dynamic
ariaPropsspread cleanly centralizes aria-label logic and only applies the attribute when meaningful.
10-10: No action needed—the getAriaLabel export is correctly structured.The barrel export at
src/utils/accessibility/index.tsproperly re-exports everything from./accessibility, andgetAriaLabelis correctly exported from the implementation file. The import path in TextMotion.tsx resolves without issue.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @src/hooks/useAnimatedChildren/useAnimatedChildren.tsx:
- Around line 44-48: The useMemo for AnimatedSpan is capturing
onAnimationEndRef.current at memoization time causing a stale callback; modify
the handler inside the useMemo so it reads onAnimationEndRef.current at
invocation time (e.g., replace direct reads of onAnimationEndRef.current within
the useMemo return with a closure that calls onAnimationEndRef.current() when
the animation actually ends), ensuring useAnimatedChildren's onAnimationEndRef
and the useEffect that updates it remain unchanged.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
src/hooks/useAnimatedChildren/useAnimatedChildren.tsxsrc/utils/splitText/splitText.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/utils/splitText/splitText.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/useAnimatedChildren.tsx
🔇 Additional comments (3)
src/hooks/useAnimatedChildren/useAnimatedChildren.tsx (3)
64-64: Dependency removal is correct (assuming the closure fix is applied).Removing
onAnimationEndfrom the dependency array is the intended optimization. Once the stale callback issue above is fixed with a closure, this correctly avoids re-computing animated children when only the callback reference changes.
69-77: LGTM on function signature and ref propagation.The updated signature clearly accepts the ref object, and propagating it through recursive calls (Line 100) maintains consistency. This is explicit and readable.
1-1: Imports are appropriate for the ref pattern.Adding
useEffectanduseRefaligns with the stabilization pattern. Per the coding guidelines on cohesion, the ref management logic stays localized within this hook.
1 participant
Description
This PR refactors focusing on performance optimizations and accessibility improvements in animation-related components and hooks.
Performance optimizations
useAnimatedChildrenby leveraginguseRefto avoid unnecessary recalculations.onAnimationStarthandling inTextMotionusinguseRef, preventing redundant re-creation of callback logic across renders.Accessibility improvements
aria-labellogic.aria-labelassignment to ensure accessibility attributes are only set when meaningful.Type of change
Checklist
Summary by CodeRabbit
Bug Fixes
New Features
Refactor
Documentation
Tests
Chores
✏️ Tip: You can customize this high-level summary in your review settings.