Conversation
📝 WalkthroughWalkthroughAdds a new orchestrator hook Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 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. 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: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/hooks/useResolveMotion/useResolveMotion.ts (1)
14-18: Doc mismatch: motion is not deep-copied.The hook returns
motionas-is, so the JSDoc claim about deep-copying is inaccurate and can mislead callers about mutability expectations.📝 Update docs (or implement copy)
- * If a `motion` object is provided, it will be deep-copied. + * If a `motion` object is provided, it will be returned as-is.
🤖 Fix all issues with AI agents
In `@src/hooks/useIntersectionObserver/useIntersectionObserver.ts`:
- Around line 29-34: When the ref callback (ref) receives a new node, reset the
intersection state so a previous true value doesn't carry over; modify the ref
defined with useCallback to call setElement(node) and also call
setIsIntersecting(false) (or explicitly set to a safe default) whenever the node
changes (including when node is null) so the observed element's visibility state
is cleared immediately before the observer attaches to the new node; update
references to element, setElement, isIntersecting, and setIsIntersecting
accordingly.
🧹 Nitpick comments (10)
src/hooks/useAnimateChildren/AnimatedSpan.tsx (1)
24-26: Remove commented-out newline branch to keep the component readable.Leaving dead code here makes the intended behavior unclear and adds noise for future maintainers. Consider deleting it or replacing it with a short comment that explicitly documents the new newline behavior.
src/hooks/useAnimateChildren/AnimatedSpan.spec.tsx (1)
34-38: Clean up the commented-out test to avoid ambiguity.Consider deleting this block or converting it to
it.skip(...)with a clear reason so future readers understand the intentional behavior shift.♻️ Possible cleanup
- // it('renders <br> when text is newline character', () => { - // const { container } = renderSpan('\n'); - - // expect(container.querySelector('br')).toBeInTheDocument(); - // }); + // it.skip('renders <br> when text is newline character', () => { + // // Intentionally disabled: newline is now rendered as text within <span>. + // const { container } = renderSpan('\n'); + // expect(container.querySelector('br')).toBeInTheDocument(); + // });src/hooks/useAnimateChildren/useAnimateChildren.tsx (1)
88-106: Consider extracting the mutable counter logic for clarity.The
sequenceIndex++side effect insidemapworks but mixes iteration with mutation, which can reduce predictability. This is a minor readability concern since the logic is contained within the function scope.If you want to improve explicitness, consider a
reduceor explicit loop, but this is acceptable for the current complexity level.src/hooks/useAnimateChildren/useAnimateChildren.spec.tsx (2)
105-110: Remove or address commented-out test code.Commented-out tests create noise and ambiguity about intended behavior. Based on the AI summary, the newline-to-br conversion was removed from AnimatedSpan. Either:
- Delete this commented test if the feature is permanently removed.
- Add a TODO comment explaining why it's disabled and when it should be revisited.
128-150: Test doesn't fully verify the callback update behavior.This test only asserts that the second callback hasn't been called after rerender, but doesn't verify that triggering the animation end event would actually invoke the updated callback. Consider completing the test:
♻️ Suggested test improvement
it('updates onAnimationEnd callback when it changes', () => { const firstCallback = jest.fn(); const secondCallback = jest.fn(); const { nodes } = splitReactNode('A', split); const { rerender } = renderHook( ({ onAnimationEnd }) => useAnimateChildren({ nodes, initialDelay: 0, animationOrder: 'first-to-last', motion, onAnimationEnd, }), { initialProps: { onAnimationEnd: firstCallback }, } ); rerender({ onAnimationEnd: secondCallback }); - expect(secondCallback).not.toHaveBeenCalled(); + // Render the animated nodes and trigger animation end + const { spans } = renderAnimatedHook(nodes, { onAnimationEnd: secondCallback }); + fireEvent.animationEnd(spans[0]); + + expect(firstCallback).not.toHaveBeenCalled(); + expect(secondCallback).toHaveBeenCalledTimes(1); });src/hooks/useValidation/validateProps.spec.tsx (1)
31-37: Consider usingit.eachfor parameterized tests.Using
it.eachinstead of.forEachinside a single test provides better test output - each split value would appear as a separate test case in the report, making failures easier to identify.♻️ Suggested refactor
- it('accepts all valid split values', () => { - (['character', 'word'] as const).forEach(split => { - const result = validateProps({ children: 'text', split }); - - expect(result.errors).toHaveLength(0); - }); - }); + it.each(['character', 'word'] as const)('accepts valid split value: %s', split => { + const result = validateProps({ children: 'text', split }); + + expect(result.errors).toHaveLength(0); + });src/components/TextMotion/TextMotion.spec.tsx (2)
60-74: Remove duplicate test case.Lines 60-74 and 87-101 test identical behavior: rendering spans when
canAnimateis true. The only difference is the comment in line 60. Consider removing one of these tests to reduce maintenance burden.Suggested fix
Keep the test at lines 60-74 (which has the clarifying comment about
trigger="on-load") and remove the duplicate at lines 87-101:- it('renders spans when canAnimate is true', () => { - const animatedChildren = Array.from(TEXT).map((ch, i) => ( - <span key={i} aria-hidden="true"> - {ch} - </span> - )); - - render(<MockTextMotion hookReturn={{ canAnimate: true, text: TEXT, animatedChildren }}>{TEXT}</MockTextMotion>); - - const container = screen.getByLabelText(TEXT); - const spans = container.querySelectorAll<HTMLSpanElement>('span[aria-hidden="true"]'); - - expect(spans.length).toBe(TEXT.length); - expect(container).toHaveClass('text-motion'); - });Also applies to: 87-101
138-148: Consider consolidating duplicate onAnimationStart tests.Lines 138-148 and 162-172 test the same scenario:
onAnimationStartis called whencanAnimateis true. The only difference is the parenthetical comment. Consider merging these into a single, well-named test.Suggested fix
Remove the duplicate test at lines 162-172:
- it('calls onAnimationStart when canAnimate is true (e.g., intersecting)', () => { - const onAnimationStart = jest.fn(); - - render( - <MockTextMotion hookReturn={{ canAnimate: true, text: TEXT }} onAnimationStart={onAnimationStart}> - {TEXT} - </MockTextMotion> - ); - - expect(onAnimationStart).toHaveBeenCalledTimes(1); - });Also applies to: 162-172
src/hooks/useValidation/validateProps.ts (1)
27-45: Inconsistent undefined checks may confuse future maintainers.Lines 27, 31, 43 use truthy checks (
if (props.split)) while lines 35, 39 use explicit undefined checks (if (props.X !== undefined)). While this works correctly (strings need truthy, booleans/numbers need explicit checks), the inconsistency reduces readability.Consider using explicit undefined checks consistently, or add a brief comment explaining why truthy checks are sufficient for string props.
Suggested fix for consistency
- if (props.split && !ALLOWED_SPLITS.includes(props.split)) { + if (props.split !== undefined && !ALLOWED_SPLITS.includes(props.split)) { errors.push(`split must be one of: ${ALLOWED_SPLITS.join(', ')}`); } - if (props.trigger && !ALLOWED_TRIGGERS.includes(props.trigger)) { + if (props.trigger !== undefined && !ALLOWED_TRIGGERS.includes(props.trigger)) { errors.push(`trigger must be one of: ${ALLOWED_TRIGGERS.join(', ')}`); }src/hooks/useController/useController.spec.tsx (1)
18-19: Variable names don't match the refactored hook names.The mock variables use past-tense naming (
mockUseResolvedMotion,mockUseAnimatedChildren) while the hooks use imperative naming (useResolveMotion,useAnimateChildren). This reduces predictability and could cause confusion.Suggested fix
- const mockUseResolvedMotion = useResolveMotion as jest.Mock; - const mockUseAnimatedChildren = useAnimateChildren as jest.Mock; + const mockUseResolveMotion = useResolveMotion as jest.Mock; + const mockUseAnimateChildren = useAnimateChildren as jest.Mock;Then update all usages in the file (lines 28, 62, 69, 74, 76, 86, 90, 100).
1 participant
Description
This PR refactors several custom hooks to improve readability, responsibility separation, and long-term maintainability.
Key improvements include:
Type of change
Checklist
Summary by CodeRabbit
New Features
Bug Fixes
Breaking Changes
Tests
✏️ Tip: You can customize this high-level summary in your review settings.