Conversation
🦋 Changeset detectedLatest commit: 6077f60 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello @dieguezguille, 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! This pull request undertakes a significant refactoring of the application's carousel components, specifically within the authentication and benefits sections. By migrating from a custom Highlights
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
|
WalkthroughReplaces FlatList-based carousels with a Carousel component across auth and benefits screens; swaps index/x-driven animations for reanimated SharedValue-driven scrollOffset and distance-based pagination, updates ListItem props and pagination signatures, and adds progress animation control and responsive sizing. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Carousel
participant Reanimated as ReanimatedEngine
participant Auth
participant ListItem
participant Pagination
User->>Carousel: swipe / gesture
Carousel->>ReanimatedEngine: emit scrollOffset updates
ReanimatedEngine->>ListItem: provide animationValue (derived from scrollOffset)
ReanimatedEngine->>Pagination: update dot distances / progress
Carousel->>Auth: notify snap/end
Auth->>ReanimatedEngine: start/reset/cancel progress animation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 4✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
![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
This pull request is a great improvement, refactoring the custom carousel implementation to use react-native-reanimated-carousel. This simplifies the code significantly by removing complex manual animation and scrolling logic, and leverages a specialized library for the task. The changes in Auth.tsx, ListItem.tsx, and Pagination.tsx are well-executed. The same pattern has been applied to the benefits carousel, which is great for consistency. I've found a couple of opportunities for improvement regarding code duplication, which I've detailed in my comments.
src/components/auth/ListItem.tsx
Outdated
| function ListItem({ item, animationValue }: ListItemProperties) { | ||
| const aspectRatio = useAspectRatio(); | ||
| const { width, height } = useWindowDimensions(); | ||
| const itemWidth = Platform.OS === "web" ? height * aspectRatio : width; | ||
|
|
||
| // animationValue: -1 (left), 0 (center/active), 1 (right) | ||
| /* istanbul ignore next */ | ||
| const rBackgroundStyle = useAnimatedStyle(() => { | ||
| const animatedScale = interpolate( | ||
| x.value, | ||
| [(index - 1) * itemWidth, index * itemWidth, (index + 1) * itemWidth], | ||
| [0, 1, 0], | ||
| Extrapolation.CLAMP, | ||
| ); | ||
| const interpolatedOpacity = interpolate( | ||
| x.value, | ||
| [(index - 1) * itemWidth, index * itemWidth, (index + 1) * itemWidth], | ||
| [0, 1, 0], | ||
| Extrapolation.CLAMP, | ||
| ); | ||
| const animatedScale = interpolate(animationValue.value, [-1, 0, 1], [0.5, 1, 0.5], Extrapolation.CLAMP); | ||
| const interpolatedOpacity = interpolate(animationValue.value, [-1, 0, 1], [0.3, 1, 0.3], Extrapolation.CLAMP); | ||
| return { transform: [{ scale: animatedScale }], opacity: interpolatedOpacity }; | ||
| }, [index, x]); | ||
| }, [animationValue]); | ||
|
|
||
| /* istanbul ignore next */ | ||
| const rImageStyle = useAnimatedStyle(() => { | ||
| const animatedScale = interpolate( | ||
| x.value, | ||
| [(index - 1) * itemWidth, index * itemWidth, (index + 1) * itemWidth], | ||
| [0.5, 1, 0.5], | ||
| Extrapolation.CLAMP, | ||
| ); | ||
| const animatedScale = interpolate(animationValue.value, [-1, 0, 1], [0.7, 1, 0.7], Extrapolation.CLAMP); | ||
| return { transform: [{ scale: animatedScale }] }; | ||
| }, [index, x]); | ||
| }, [animationValue]); | ||
|
|
||
| return ( | ||
| <View width={itemWidth} aspectRatio={aspectRatio} justifyContent="center" alignItems="center"> |
There was a problem hiding this comment.
The itemWidth is calculated here and also in the parent Auth component. This duplication can lead to inconsistencies. In fact, the calculation in Auth.tsx includes Math.max(..., 250) which is missing here.
Since the Carousel component already sets the width for each item's container, this component should just fill that container. You can remove the itemWidth calculation and set width="100%" on the root View.
function ListItem({ item, animationValue }: ListItemProperties) {
const aspectRatio = useAspectRatio();
// animationValue: -1 (left), 0 (center/active), 1 (right)
/* istanbul ignore next */
const rBackgroundStyle = useAnimatedStyle(() => {
const animatedScale = interpolate(animationValue.value, [-1, 0, 1], [0.5, 1, 0.5], Extrapolation.CLAMP);
const interpolatedOpacity = interpolate(animationValue.value, [-1, 0, 1], [0.3, 1, 0.3], Extrapolation.CLAMP);
return { transform: [{ scale: animatedScale }], opacity: interpolatedOpacity };
}, [animationValue]);
/* istanbul ignore next */
const rImageStyle = useAnimatedStyle(() => {
const animatedScale = interpolate(animationValue.value, [-1, 0, 1], [0.7, 1, 0.7], Extrapolation.CLAMP);
return { transform: [{ scale: animatedScale }] };
}, [animationValue]);
return (
<View width="100%" aspectRatio={aspectRatio} justifyContent="center" alignItems="center">
| function calculateDistance(scrollOffset: number, index: number, length: number) { | ||
| "worklet"; | ||
| const normalizedOffset = ((scrollOffset % length) + length) % length; | ||
| let distance = Math.abs(normalizedOffset - index); | ||
| if (distance > length / 2) { | ||
| distance = length - distance; | ||
| } | ||
| return distance; | ||
| } |
There was a problem hiding this comment.
This calculateDistance worklet is identical to the one in src/components/auth/Pagination.tsx. To avoid code duplication and improve maintainability, you should extract this function into a shared utility file (e.g., in src/utils/animation.ts) and import it in both places.
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #701 +/- ##
==========================================
+ Coverage 64.54% 64.64% +0.10%
==========================================
Files 173 173
Lines 5378 5385 +7
Branches 1509 1505 -4
==========================================
+ Hits 3471 3481 +10
+ Misses 1727 1723 -4
- Partials 180 181 +1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/components/benefits/BenefitsSection.tsx (1)
153-160:⚠️ Potential issue | 🟠 MajorAvoid accidental taps in the carousel by using TapGestureHandler.
BenefitCardis triggered viaonPressinside the carousel; ensure the card usesTapGestureHandler(or wrap it here) so swipe gestures don't fire taps.#!/bin/bash # Verify BenefitCard uses TapGestureHandler instead of plain onPress rg -n "TapGestureHandler|onPress" -g 'BenefitCard.tsx' -C2Based on learnings, "In benefit card components (e.g., src/components/benefits/BenefitCard.tsx) that use a carousel (react-native-reanimated-carousel) with Tamagui, prefer using TapGestureHandler from react-native-gesture-handler instead of Tamagui's built-in onPress for card items."
🤖 Fix all issues with AI agents
In `@src/components/auth/Pagination.tsx`:
- Around line 10-19: The duplicate circular-scroll logic in calculateDistance
should be extracted to a shared helper: create a single exported function (e.g.,
calculateDistance) in a new utility module and replace the inline
implementations in both Pagination component and BenefitsSection by importing
that helper; update the components to call the shared calculateDistance
(preserving the same signature scrollOffset, index, length) and remove the
duplicated function bodies to avoid drift, and update any relevant
imports/exports or tests that referenced the old local implementations.
- Around line 85-96: The row wrapper uses React Native's View but per guidelines
should use Tamagui's XStack; replace the outer <View flexDirection="row" ...>
with <XStack ...> while preserving props (alignItems/justifyContent/gap) and
passing the same children into PaginationComponent (key, index, length,
scrollOffset, progress, isScrolling, activeColor using
theme.interactiveBaseBrandDefault.val); ensure you import XStack at top and
remove the now-unnecessary View import.
| /* istanbul ignore next */ | ||
| function calculateDistance(scrollOffset: number, index: number, length: number) { | ||
| "worklet"; | ||
| const normalizedOffset = ((scrollOffset % length) + length) % length; | ||
| let distance = Math.abs(normalizedOffset - index); | ||
| if (distance > length / 2) { | ||
| distance = length - distance; | ||
| } | ||
| return distance; | ||
| } |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Extract calculateDistance into a shared helper to avoid drift.
The same logic exists in src/components/benefits/BenefitsSection.tsx; a shared utility reduces divergence.
🤖 Prompt for AI Agents
In `@src/components/auth/Pagination.tsx` around lines 10 - 19, The duplicate
circular-scroll logic in calculateDistance should be extracted to a shared
helper: create a single exported function (e.g., calculateDistance) in a new
utility module and replace the inline implementations in both Pagination
component and BenefitsSection by importing that helper; update the components to
call the shared calculateDistance (preserving the same signature scrollOffset,
index, length) and remove the duplicated function bodies to avoid drift, and
update any relevant imports/exports or tests that referenced the old local
implementations.
dieguezguille
force-pushed
the
carousel
branch
2 times, most recently
from
6ddca82 to
dfece93
Compare
![sentry[bot]](https://avatars.githubusercontent.com/in/12637?s=60&v=4){kind=small}
| const timer = setInterval(() => { | ||
| /* istanbul ignore next */ | ||
| progress.value = withTiming(progress.value + 0.2, { duration: 1000, easing: Easing.linear }, () => { |
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
dieguezguille
force-pushed
the
carousel
branch
from
dfece93 to
6077f60
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/components/auth/ListItem.tsx`:
- Line 6: The component uses Tamagui's View as a pure layout wrapper—replace the
import and usages with YStack: change the import from "View" to "YStack" and
replace each <View ...> wrapper (including the occurrences around lines 32-39)
with <YStack ...>, preserving the same layout props but using YStack's
primitives and closing tags (update any View-specific props if needed to Tamagui
stack equivalents); ensure the component export/function ListItem remains
unchanged.
| import { Extrapolation, interpolate, useAnimatedStyle } from "react-native-reanimated"; | ||
|
|
||
| import { useWindowDimensions, View } from "tamagui"; | ||
| import { View } from "tamagui"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
🧩 Analysis chain
🏁 Script executed:
cat -n src/components/auth/ListItem.tsxRepository: exactly/exa
Length of output: 2007
Replace View with YStack for layout.
The wrapper is a pure layout container applying layout properties and should use Tamagui's stack primitives.
♻️ Suggested refactor
-import { View } from "tamagui";
+import { YStack } from "tamagui";
@@
- <View width="100%" height="100%" justifyContent="center" alignItems="center">
+ <YStack width="100%" height="100%" justifyContent="center" alignItems="center">
@@
- </View>
+ </YStack>Also applies to: lines 32-39
🤖 Prompt for AI Agents
In `@src/components/auth/ListItem.tsx` at line 6, The component uses Tamagui's
View as a pure layout wrapper—replace the import and usages with YStack: change
the import from "View" to "YStack" and replace each <View ...> wrapper
(including the occurrences around lines 32-39) with <YStack ...>, preserving the
same layout props but using YStack's primitives and closing tags (update any
View-specific props if needed to Tamagui stack equivalents); ensure the
component export/function ListItem remains unchanged.
2 participants
closes #91
Summary by CodeRabbit
New Features
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.