[FIX] API calls for Recent Bookmarks Sidebar #1099
Conversation
WalkthroughThe pull request modifies the Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
@HarshitT00 is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
![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
🧹 Outside diff range and nitpick comments (2)
components/SideBar/SideBarSavedPosts.tsx (2)
15-15: Good addition oflimitedBookmarks, consider backend implementationThe introduction of
limitedBookmarksis a good improvement. It centralizes the logic for limiting the number of bookmarks and handles potential undefined values well.However, as noted in the TODO comment, consider implementing this slicing on the backend for better performance, especially as the number of bookmarks grows.
Would you like me to create a GitHub issue to track the implementation of backend slicing for bookmarks?
Line range hint
35-62: Improved success state rendering with a minor suggestionThe refactoring of the success state rendering is a significant improvement. The use of
limitedBookmarksand the simplified conditional rendering enhance code clarity and maintainability.However, there's a small improvement we can make:
In the
SideBarSavedArticlePreviewcomponent, consider using a more unique key instead of justid. Ifidis guaranteed to be unique across all bookmarks, it's fine. But if there's a chance it might not be, you could create a more unique key by combiningidandslug:<SideBarSavedArticlePreview key={`${id}-${slug}`} // ... other props />This ensures that each rendered component has a truly unique key, which is important for React's reconciliation process.
🧰 Tools
🪛 Biome
[error] 26-26: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
- components/SideBar/SideBarSavedPosts.tsx (2 hunks)
🧰 Additional context used
🪛 Biome
components/SideBar/SideBarSavedPosts.tsx
[error] 26-26: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
🔇 Additional comments (3)
components/SideBar/SideBarSavedPosts.tsx (3)
9-9: Improved data fetching logicThe consolidation of the query result destructuring is a good improvement. It simplifies the code and directly uses the
statusfrom the query, which provides more detailed information about the query state.
29-33: Improved error state handlingThe error state rendering using the
statusvariable is consistent with earlier changes and improves code readability. The error message is clear and provides actionable information to the user.
Line range hint
1-94: Overall assessment: Significant improvements with minor suggestionsThis PR successfully addresses its objectives by consolidating API calls and improving the efficiency of the
SideBarSavedPostscomponent. The changes enhance code readability, simplify logic, and improve error handling.Key improvements:
- Consolidated query result destructuring
- Introduction of
limitedBookmarksfor cleaner rendering logic- Consistent use of
statusfor conditional rendering- Simplified success state rendering
Minor suggestions for further improvement:
- Implement backend slicing for bookmarks
- Optimize loading state rendering
- Use a more unique key for
SideBarSavedArticlePreviewcomponentsThese changes align well with the PR objectives and the AI-generated summary. Great work on improving this component!
🧰 Tools
🪛 Biome
[error] 26-26: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
| {status === "loading" && | ||
| Children.toArray( | ||
| Array.from({ length: howManySavedToShow }, () => { | ||
| return <LoadingSkeleton />; | ||
| }), | ||
| )} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider optimizing the loading state rendering
The use of status for conditional rendering is good and consistent with earlier changes. However, the current implementation of loading skeletons can be optimized.
Instead of using Children.toArray(), consider using the index as a key, or better yet, create a simple array with a map function:
{status === "loading" &&
Array.from({ length: howManySavedToShow }).map((_, index) => (
<LoadingSkeleton key={index} />
))
}This approach is more efficient and doesn't require the use of Children.toArray().
🧰 Tools
🪛 Biome
[error] 26-26: Missing key property for this element in iterable.
The order of the items may change, and having a key can help React identify which item was moved.
Check the React documentation.(lint/correctness/useJsxKeyInIterable)
| @@ -6,45 +6,43 @@ import SideBarSavedArticlePreview from "./SideBarSavedArticlePreview"; | |||
| import Link from "next/link"; | |||
|
|
|||
| export default React.memo(function SideBarSavedPosts() { | |||
| let { data: bookmarks } = api.post.myBookmarks.useQuery(); | |||
| const { status: bookmarkStatus } = api.post.myBookmarks.useQuery(); | |||
| const { data: bookmarks, status } = api.post.myBookmarks.useQuery(); | |||
There was a problem hiding this comment.
I think this line alone should be good! Maybe we can pass the amount of bookmarks to the useQuery() so that we only get the right amount back rather than doing this on the frontend?
✨ Codu Pull Request 💻
Fixes #(issue)
Pull Request details
Getting data and status in same query which is previously done saperately.