-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Add skeleton loader for articles #87
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
feat: Add skeleton loader for articles #87
Conversation
|
Caution Review failedThe pull request is closed. Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. WalkthroughAdds skeleton UI components and integrates them into list and post pages: new Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor U as User
participant List as ArticlesListPartial
participant API as Store/API
participant Skeleton as ArticleItemSkeletonPartial
U->>List: Trigger fetch (mount / search / refresh)
activate List
List->>List: lastRequestId = newId\nisLoading = true\nskeletonCount = skeletonCountFor(prevItems)
List->>Skeleton: Render N skeletons (skeletonCount)
List->>API: getCategories() (if needed)
List->>API: getPosts(reqId)
note right of List: Deduplicate via lastRequestId\nIgnore stale responses
API-->>List: posts / error
alt latest response
List->>List: Update items or restore previous on error\nisLoading = false
List->>List: Render ArticleItemPartial list
else stale response
List-->>List: Ignore result
end
deactivate List
sequenceDiagram
autonumber
actor U as User
participant Page as PostPage
participant API as Store/API
participant Skeleton as PostPageSkeletonPartial
U->>Page: Navigate to post
activate Page
Page->>Page: isLoading = true
Page->>Skeleton: Render skeleton
Page->>API: fetchPost(id)
API-->>Page: post / error
alt success
Page->>Page: render post\nisLoading = false
else error
Page->>Page: show "We couldn't load this post."\nisLoading = false
end
deactivate Page
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (2)
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 |
Summary of ChangesHello @gocanto, 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 significantly enhances the user experience of the article list by introducing a skeleton loader. When articles are being fetched, users will now see a visually consistent placeholder, preventing layout shifts and providing immediate feedback, making the application feel more responsive and polished. 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
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a skeleton loader for the articles list, which is a great enhancement for user experience. The implementation is solid, using a new ArticleItemSkeletonPartial component and updating ArticlesListPartial to manage the loading state. The logic to handle race conditions during data fetching is well-implemented. The tests have also been updated to cover the new loading behavior, which is excellent. I have one suggestion to refactor the fetchPosts function for better maintainability by reducing some code duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (2)
src/partials/ArticlesListPartial.vue (2)
65-92: LGTM!The
fetchPostsimplementation correctly handles:
- Request deduplication via
lastRequestIdto prevent race conditions- Skeleton count preservation based on previous results
- Error recovery by restoring previous items
- Proper cleanup in the finally block
The current structure with pre-fetch skeleton setup (line 69) and post-fetch update (line 88) is correct and necessary for the loading UX flow.
65-92: Previous review suggestion about consolidatingskeletonCountupdate.A prior reviewer suggested moving the
skeletonCountupdate to thefinallyblock to reduce duplication. This is a valid refactor that would consolidate the skeleton count logic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
src/pages/PostPage.vue(6 hunks)src/partials/ArticlesListPartial.vue(3 hunks)src/partials/PostPageSkeletonPartial.vue(1 hunks)tests/pages/PostPage.test.ts(2 hunks)tests/partials/ArticlesListPartial.test.ts(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
tests/pages/PostPage.test.ts (1)
src/stores/api/store.ts (1)
getPost(123-131)
tests/partials/ArticlesListPartial.test.ts (1)
src/stores/api/store.ts (2)
getPosts(114-122)getCategories(105-113)
🪛 GitHub Actions: Unit Tests
tests/partials/ArticlesListPartial.test.ts
[error] 187-187: AssertionError: expected [] to have a length of 3 but got +0
🪛 GitHub Check: vitest
tests/partials/ArticlesListPartial.test.ts
[failure] 187-187: tests/partials/ArticlesListPartial.test.ts > ArticlesListPartial > uses the previous result count while refreshing the list
AssertionError: expected [] to have a length of 3 but got +0
- Expected
- Received
- 3
- 0
❯ tests/partials/ArticlesListPartial.test.ts:187:21
🔇 Additional comments (14)
src/partials/PostPageSkeletonPartial.vue (2)
1-41: LGTM!The skeleton component is well-structured with appropriate accessibility attributes (
aria-hidden="true") and animation classes. The implementation correctly provides a loading placeholder that matches the post page layout.
1-41: LGTM! Well-structured skeleton component.The component correctly implements a loading placeholder with proper accessibility (
aria-hidden="true") and testability (data-testid). The skeleton structure mirrors the actual post layout, providing good visual continuity during loading.src/partials/ArticlesListPartial.vue (4)
24-29: LGTM!The conditional rendering correctly shows skeleton loaders during the loading state and displays items when available. The use of
aria-busy="true"on the loading container improves accessibility.
95-112: LGTM!The debounced fetch pattern is well-implemented:
- Correct lodash debounce configuration with
leading: true, trailing: true- Proper lifecycle cleanup via
onBeforeUnmountto cancel pending debounced calls- Prevents memory leaks and unnecessary API calls
61-92: Approve: Race condition prevention withlastRequestId.The request deduplication pattern using
lastRequestIdcorrectly prevents stale responses from updating state. The logic to restorepreviousItemson error and guard state updates withrequestId === lastRequestIdis sound.
95-112: Approve: Proper debounce configuration and cleanup.The debounced fetch with
leading: true, trailing: trueprovides immediate feedback while preventing excess requests. TheonBeforeUnmounthook correctly cancels pending debounced calls.tests/pages/PostPage.test.ts (4)
73-76: LGTM!The test correctly verifies the skeleton's presence during the initial loading state and its removal after the data is loaded. This aligns with the expected behavior of the loading flow in
PostPage.vue.
138-139: LGTM!The test correctly validates the error state behavior: skeleton is hidden and the fallback error message "We couldn't load this post." is displayed when loading fails.
60-78: Approve: Skeleton loading assertions.The test correctly verifies that the skeleton is present during initial load (line 73) and disappears after successful fetch (line 76), ensuring the loading UX works as intended.
120-140: Approve: Error handling with skeleton cleanup.The test properly asserts that the skeleton is removed (line 138) and the error message is displayed (line 139) when the fetch fails, confirming graceful error handling.
src/pages/PostPage.vue (4)
30-32: LGTM!The conditional rendering correctly manages the loading, success, and error states:
- Skeleton is shown during loading
- Post content is rendered when available
- Fallback error message appears when loading fails
Also applies to: 103-103
145-145: LGTM!The loading state is correctly managed: initialized to
trueand set tofalsein the finally block after the fetch attempt, ensuring the skeleton is hidden regardless of success or failure.Also applies to: 212-214
30-32: Approve: Clear loading and error states.The conditional rendering flow is well-structured:
- Skeleton shown during loading (line 30)
- Article content shown when post exists (line 32)
- Error message shown when load fails (line 103)
This provides good user feedback for all states.
Also applies to: 103-103
145-145: Approve: Proper loading state management.The
isLoadingref is correctly initialized totrue(line 145) and set tofalsein thefinallyblock (line 213), ensuring the loading state is cleared regardless of success or error.Also applies to: 205-214
Summary
Summary by CodeRabbit
New Features
Bug Fixes
Tests