[UX] Mobile HomeScreen Skeleton Loading #265
Conversation
Co-authored-by: Devasy <110348311+Devasy@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
✅ Deploy Preview for split-but-wiser canceled.
|
WalkthroughThe PR adds a GroupSkeleton component to Changes
Suggested reviewers
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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 |
![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)
mobile/screens/HomeScreen.js (1)
259-263: Remove unusedloaderContainerstyle.This style appears to be leftover from the previous
ActivityIndicatorimplementation and is no longer used.🧹 Proposed cleanup
const styles = StyleSheet.create({ container: { flex: 1, }, - loaderContainer: { - flex: 1, - justifyContent: "center", - alignItems: "center", - }, list: { padding: 16, },
🤖 Fix all issues with AI agents
In `@mobile/components/GroupSkeleton.js`:
- Around line 12-27: The Animated.loop started inside the useEffect (wrapping
Animated.sequence of Animated.timing on opacity) never gets stopped; fix by
capturing the looped animation (e.g., const anim = Animated.loop(...)), call
anim.start() in the effect, and return a cleanup function that calls anim.stop()
to halt the animation when the component unmounts (update the useEffect that
references opacity to return () => anim.stop()).
| useEffect(() => { | ||
| Animated.loop( | ||
| Animated.sequence([ | ||
| Animated.timing(opacity, { | ||
| toValue: 0.7, | ||
| duration: 800, | ||
| useNativeDriver: true, | ||
| }), | ||
| Animated.timing(opacity, { | ||
| toValue: 0.3, | ||
| duration: 800, | ||
| useNativeDriver: true, | ||
| }), | ||
| ]) | ||
| ).start(); | ||
| }, [opacity]); |
There was a problem hiding this comment.
Add cleanup for the animation loop to prevent memory leaks.
The animation loop is started but never stopped when the component unmounts. This could cause warnings or memory leaks if the skeleton is removed while the animation is running (e.g., when data finishes loading).
🛠️ Proposed fix
useEffect(() => {
- Animated.loop(
+ const animation = Animated.loop(
Animated.sequence([
Animated.timing(opacity, {
toValue: 0.7,
duration: 800,
useNativeDriver: true,
}),
Animated.timing(opacity, {
toValue: 0.3,
duration: 800,
useNativeDriver: true,
}),
])
- ).start();
+ );
+ animation.start();
+ return () => animation.stop();
}, [opacity]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| Animated.loop( | |
| Animated.sequence([ | |
| Animated.timing(opacity, { | |
| toValue: 0.7, | |
| duration: 800, | |
| useNativeDriver: true, | |
| }), | |
| Animated.timing(opacity, { | |
| toValue: 0.3, | |
| duration: 800, | |
| useNativeDriver: true, | |
| }), | |
| ]) | |
| ).start(); | |
| }, [opacity]); | |
| useEffect(() => { | |
| const animation = Animated.loop( | |
| Animated.sequence([ | |
| Animated.timing(opacity, { | |
| toValue: 0.7, | |
| duration: 800, | |
| useNativeDriver: true, | |
| }), | |
| Animated.timing(opacity, { | |
| toValue: 0.3, | |
| duration: 800, | |
| useNativeDriver: true, | |
| }), | |
| ]) | |
| ); | |
| animation.start(); | |
| return () => animation.stop(); | |
| }, [opacity]); |
🤖 Prompt for AI Agents
In `@mobile/components/GroupSkeleton.js` around lines 12 - 27, The Animated.loop
started inside the useEffect (wrapping Animated.sequence of Animated.timing on
opacity) never gets stopped; fix by capturing the looped animation (e.g., const
anim = Animated.loop(...)), call anim.start() in the effect, and return a
cleanup function that calls anim.stop() to halt the animation when the component
unmounts (update the useEffect that references opacity to return () =>
anim.stop()).
|
@jules mobile/screens/HomeScreen.js (1) 259-263: Remove unused loaderContainer style. This style appears to be leftover from the previous ActivityIndicator implementation and is no longer used. 🧹 Proposed cleanup const styles = StyleSheet.create({ container: { flex: 1, }, - loaderContainer: { - flex: 1, - justifyContent: "center", - alignItems: "center", - }, list: { padding: 16, }, 🤖 Fix all issues with AI agents In |
Implemented a skeleton loading state for the mobile HomeScreen to replace the generic spinner.
mobile/components/GroupSkeleton.jswith pulsing animation usingAnimatedAPI.mobile/screens/HomeScreen.js.useThemefromreact-native-paper.HomeScreen.js.PR created automatically by Jules for task 5052525611379640803 started by @Devasy23
Summary by CodeRabbit
✏️ Tip: You can customize this high-level summary in your review settings.