[jules] enhance: Add skeleton loading for mobile home screen#290
[jules] enhance: Add skeleton loading for mobile home screen#290
Conversation
Co-authored-by: Devasy23 <110348311+Devasy23@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.
|
WalkthroughAdds a reusable mobile Skeleton component and a GroupListSkeleton placeholder, replaces the HomeScreen ActivityIndicator with the GroupListSkeleton, and updates related documentation and task metadata. 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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.Jules/knowledge.md:
- Around line 313-321: Add a blank line immediately before the fenced code block
that begins with ```jsx so the example block is separated from the preceding
text; update the section containing the Card/Card.Title/Skeleton example to
insert one empty line before the opening ```jsx fence to satisfy markdownlint
MD031.
In `@mobile/components/skeletons/GroupListSkeleton.js`:
- Line 9: The View in GroupListSkeleton that sets accessibilityLabel="Loading
groups" must also include accessible={true} so screen readers reliably announce
the container as a single element; update the View in the GroupListSkeleton
component to add accessible={true} alongside the existing accessibilityLabel
prop so assistive tech skips its children and announces "Loading groups".
- Around line 14-16: The render-prop parameter named props in the left prop of
Card.Title is unused and should be prefixed to avoid linter warnings; rename
props to _props (or _) in the left={(props) => (...) } arrow function so it
becomes left={(_props) => (<Skeleton width={40} height={40} borderRadius={20}
/>)} to signal intentional non-use while keeping the Skeleton usage unchanged in
GroupListSkeleton.js.
- Around line 12-17: The Card.Title usage in GroupListSkeleton.js is causing a
runtime crash because title expects text-based content but we're passing the
View-based Skeleton; replace the Card.Title block (the Card.Title call that
currently has title={<Skeleton .../>} and left={(props) => <Skeleton .../>})
with a plain View-based header layout: create a container View with
flexDirection: 'row' and alignItems: 'center', render the left avatar Skeleton
(width 40 height 40 borderRadius 20) as the first child, and render a right-side
column View containing the title Skeleton (width 150 height 20) and optional
subtitle Skeleton (e.g. width 100 height 14) stacked vertically; this keeps all
Skeletons as Views and avoids nesting Views inside Text while visually matching
the Card.Title header.
In `@mobile/components/ui/Skeleton.js`:
- Around line 30-41: The Animated.View used in the Skeleton component should be
hidden from screen readers: update the Animated.View (in
mobile/components/ui/Skeleton.js) to include accessible={false} and
importantForAccessibility="no-hide-descendants" so assistive tech ignores the
placeholder (importantForAccessibility applies on Android for full suppression).
- Line 35: In the Skeleton component replace the fallback for borderRadius so it
doesn't override explicit zero values: update the style assignment that
currently uses "borderRadius: borderRadius || 4" to use nullish coalescing
(borderRadius ?? 4) so only null/undefined fall back to 4; locate this in the
Skeleton component's render/style object where borderRadius is referenced.
| **Example:** | ||
| ```jsx | ||
| <Card> | ||
| <Card.Title | ||
| title={<Skeleton width={150} height={20} />} | ||
| left={(props) => <Skeleton width={40} height={40} borderRadius={20} />} | ||
| /> | ||
| </Card> | ||
| ``` |
There was a problem hiding this comment.
Add a blank line before the fenced code block to fix the MD031 lint warning.
markdownlint requires blank lines surrounding fenced code blocks.
📝 Proposed fix
**Example:**
+
```jsx
<Card>📝 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.
| **Example:** | |
| ```jsx | |
| <Card> | |
| <Card.Title | |
| title={<Skeleton width={150} height={20} />} | |
| left={(props) => <Skeleton width={40} height={40} borderRadius={20} />} | |
| /> | |
| </Card> | |
| ``` | |
| **Example:** | |
🧰 Tools
🪛 markdownlint-cli2 (0.21.0)
[warning] 314-314: Fenced code blocks should be surrounded by blank lines
(MD031, blanks-around-fences)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.Jules/knowledge.md around lines 313 - 321, Add a blank line immediately
before the fenced code block that begins with ```jsx so the example block is
separated from the preceding text; update the section containing the
Card/Card.Title/Skeleton example to insert one empty line before the opening
```jsx fence to satisfy markdownlint MD031.
| const GroupListSkeleton = () => { | ||
| // Render 4 skeleton items to fill the screen | ||
| return ( | ||
| <View style={{ padding: 16 }} accessibilityLabel="Loading groups"> |
There was a problem hiding this comment.
accessibilityLabel on a plain View requires accessible={true} to be reliably announced.
Without accessible={true}, React Native may not treat this View as a focusable accessibility element and the label may be ignored on both iOS and Android. Add accessible={true} so the container is announced as a single element and screen readers skip traversal into children.
♿ Proposed fix
- <View style={{ padding: 16 }} accessibilityLabel="Loading groups">
+ <View style={{ padding: 16 }} accessible={true} accessibilityLabel="Loading groups">📝 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.
| <View style={{ padding: 16 }} accessibilityLabel="Loading groups"> | |
| <View style={{ padding: 16 }} accessible={true} accessibilityLabel="Loading groups"> |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/components/skeletons/GroupListSkeleton.js` at line 9, The View in
GroupListSkeleton that sets accessibilityLabel="Loading groups" must also
include accessible={true} so screen readers reliably announce the container as a
single element; update the View in the GroupListSkeleton component to add
accessible={true} alongside the existing accessibilityLabel prop so assistive
tech skips its children and announces "Loading groups".
| left={(props) => ( | ||
| <Skeleton width={40} height={40} borderRadius={20} /> | ||
| )} |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Prefix the unused props parameter with _ to signal intentional non-use.
The left render prop receives { size: number } from Card.Title, but Skeleton uses fixed dimensions. Renaming to _props (or just _) prevents lint warnings and communicates intent clearly.
♻️ Proposed fix
- left={(props) => (
+ left={(_props) => (
<Skeleton width={40} height={40} borderRadius={20} />
)}📝 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.
| left={(props) => ( | |
| <Skeleton width={40} height={40} borderRadius={20} /> | |
| )} | |
| left={(_props) => ( | |
| <Skeleton width={40} height={40} borderRadius={20} /> | |
| )} |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/components/skeletons/GroupListSkeleton.js` around lines 14 - 16, The
render-prop parameter named props in the left prop of Card.Title is unused and
should be prefixed to avoid linter warnings; rename props to _props (or _) in
the left={(props) => (...) } arrow function so it becomes left={(_props) =>
(<Skeleton width={40} height={40} borderRadius={20} />)} to signal intentional
non-use while keeping the Skeleton usage unchanged in GroupListSkeleton.js.
| <Animated.View | ||
| style={[ | ||
| { | ||
| width, | ||
| height, | ||
| borderRadius: borderRadius || 4, | ||
| backgroundColor: theme.colors.surfaceVariant, | ||
| opacity, | ||
| }, | ||
| style, | ||
| ]} | ||
| /> |
There was a problem hiding this comment.
Hide the skeleton placeholder from screen readers.
Without accessible={false}, screen readers may traverse into this animated view and announce nothing useful (or confuse users). Add accessible={false} to suppress it, and importantForAccessibility="no-hide-descendants" on Android for full suppression.
♿ Proposed fix
<Animated.View
+ accessible={false}
+ importantForAccessibility="no-hide-descendants"
style={[🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/components/ui/Skeleton.js` around lines 30 - 41, The Animated.View
used in the Skeleton component should be hidden from screen readers: update the
Animated.View (in mobile/components/ui/Skeleton.js) to include
accessible={false} and importantForAccessibility="no-hide-descendants" so
assistive tech ignores the placeholder (importantForAccessibility applies on
Android for full suppression).
| { | ||
| width, | ||
| height, | ||
| borderRadius: borderRadius || 4, |
There was a problem hiding this comment.
Use nullish coalescing ?? instead of || for the borderRadius default.
borderRadius || 4 evaluates to 4 when borderRadius={0} is explicitly passed (e.g., sharp corners), silently ignoring the caller's intent. Use ?? to only fall back when the value is null/undefined.
🐛 Proposed fix
- borderRadius: borderRadius || 4,
+ borderRadius: borderRadius ?? 4,📝 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.
| borderRadius: borderRadius || 4, | |
| borderRadius: borderRadius ?? 4, |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@mobile/components/ui/Skeleton.js` at line 35, In the Skeleton component
replace the fallback for borderRadius so it doesn't override explicit zero
values: update the style assignment that currently uses "borderRadius:
borderRadius || 4" to use nullish coalescing (borderRadius ?? 4) so only
null/undefined fall back to 4; locate this in the Skeleton component's
render/style object where borderRadius is referenced.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #290 +/- ##
=======================================
Coverage ? 63.55%
=======================================
Files ? 21
Lines ? 2456
Branches ? 254
=======================================
Hits ? 1561
Misses ? 831
Partials ? 64
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
mobile/components/skeletons/GroupListSkeleton.js (1)
9-9: Addaccessible={true}to make the label reliably announced.
accessibilityLabelon a plainViewmay be ignored unless the view is marked accessible. This was flagged previously and still applies.♿ Proposed fix
- <View style={{ padding: 16 }} accessibilityLabel="Loading groups"> + <View style={{ padding: 16 }} accessible={true} accessibilityLabel="Loading groups">🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@mobile/components/skeletons/GroupListSkeleton.js` at line 9, The View in the GroupListSkeleton component uses accessibilityLabel but isn't marked accessible, so screen readers may ignore it; update the root View in GroupListSkeleton to include accessible={true} (e.g., the View that currently has style={{ padding: 16 }} and accessibilityLabel="Loading groups") so the label is reliably announced.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@mobile/components/skeletons/GroupListSkeleton.js`:
- Line 9: The View in the GroupListSkeleton component uses accessibilityLabel
but isn't marked accessible, so screen readers may ignore it; update the root
View in GroupListSkeleton to include accessible={true} (e.g., the View that
currently has style={{ padding: 16 }} and accessibilityLabel="Loading groups")
so the label is reliably announced.
1 participant
Implemented a complete skeleton loading system for the mobile Home screen.
Skeletoncomponent (mobile/components/ui/Skeleton.js) usingAnimatedfor pulsing effect.GroupListSkeletoncomponent (mobile/components/skeletons/GroupListSkeleton.js) to mimic the group list layout.mobile/screens/HomeScreen.jsto replace the genericActivityIndicatorwith the new skeleton loader.todo.md,changelog.md, andknowledge.mdto reflect the changes.PR created automatically by Jules for task 2058336560218393763 started by @Devasy23
Summary by CodeRabbit
New Features
Documentation