Fixes Refetch of SideBar Posts on Bookmarked #1112
Conversation
|
@revanthnetha is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe changes primarily involve updates to the Changes
Assessment against linked issues
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🔇 Additional comments (4)
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 (
|
|
Hello @{{ contributor }}, thanks for opening your first Pull Request. The maintainers will review this Pull Request and provide feedback as soon as possible. Keep up the great work! |
revanthnetha
temporarily deployed
to
production
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (4)
app/(app)/saved/_client.tsx (1)
15-15: Good addition of fallback for bookmarks data.The introduction of
const bookmarks = bookmarksData?.bookmarks || []is a solid improvement. It ensures thatbookmarksis always an array, even ifbookmarksDatais undefined or lacks abookmarksproperty. This enhances the robustness of the code and prevents potential runtime errors.For even better type safety and clarity, consider using a type assertion:
const bookmarks = (bookmarksData?.bookmarks ?? []) as Array<BookmarkType>;Replace
BookmarkTypewith the actual type of your bookmark objects. This approach maintains the fallback behavior while providing clearer type information to TypeScript.components/SideBar/SideBarSavedPosts.tsx (2)
9-11: Approved: Query optimization with limit parameterThe addition of the
limitparameter to themyBookmarksquery aligns well with the PR objective of optimizing bookmark fetching. This change should improve performance by reducing the amount of data transferred.A minor suggestion for improvement:
Consider extracting the limit value to a constant at the top of the component for easier maintenance:
const BOOKMARKS_LIMIT = 3; // Then use it in the query const { data: bookmarksData } = api.post.myBookmarks.useQuery({ limit: BOOKMARKS_LIMIT, });This makes it easier to adjust the limit in the future if needed.
15-16: Approved: Improved data structure handlingThe updates to
totalNumberSavedandbookmarksreflect the new data structure returned by the query. The use of optional chaining and fallback values improves the robustness of the code.For consistency, consider using the
BOOKMARKS_LIMITconstant (suggested earlier) instead of the hardcoded value:const totalNumberSaved = bookmarksData?.totalCount || 0; const bookmarks = bookmarksData?.bookmarks || []; const howManySavedToShow = BOOKMARKS_LIMIT;This ensures that the number of bookmarks shown is always consistent with the query limit.
components/ArticlePreview/ArticlePreview.tsx (1)
62-62: Remove unused variablebookmarksThe
bookmarksvariable is declared but not used within this component. Removing it will clean up the code and prevent potential confusion.Apply this change to remove the unused variable:
- const bookmarks = bookmarksData?.bookmarks;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
- app/(app)/saved/_client.tsx (1 hunks)
- components/ArticlePreview/ArticlePreview.tsx (2 hunks)
- components/SideBar/SideBarSavedPosts.tsx (1 hunks)
- schema/post.ts (1 hunks)
- server/api/router/post.ts (2 hunks)
🧰 Additional context used
🔇 Additional comments (7)
schema/post.ts (1)
80-82: Approve new schema with suggestions for improvementThe addition of
GetLimitSidePostsschema is a good approach for handling limited sidebar posts queries. However, consider the following improvements:
Add a maximum value for the
limitto prevent excessive data fetching:limit: z.number().optional().max(50) // or any other appropriate maximumConsider renaming the schema to be more specific, e.g.,
GetLimitedSidebarPostsSchema.Let's verify the usage of this new schema in the codebase:
✅ Verification successful
Verified usage of
GetLimitSidePostsschemaThe
GetLimitSidePostsschema is correctly utilized inserver/api/router/post.tsas intended for handling procedure inputs. No additional usages were found, ensuring that the schema's scope is appropriately confined.🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Check for usage of GetLimitSidePosts schema # Search for import statements echo "Checking import statements:" rg --type typescript "import.*GetLimitSidePosts.*from.*schema/post" # Search for direct usage of the schema echo "\nChecking direct usage:" rg --type typescript "GetLimitSidePosts" # Search for potential trpc procedure definitions using this schema echo "\nChecking potential trpc procedure definitions:" rg --type typescript "input:\s*GetLimitSidePosts"Length of output: 498
Script:
#!/bin/bash # Description: Check for usage of GetLimitSidePosts schema # Search for import statements echo "Checking import statements:" rg --type ts "import.*GetLimitSidePosts.*from.*schema/post" # Search for direct usage of the schema echo "\nChecking direct usage:" rg --type ts "GetLimitSidePosts" # Search for potential trpc procedure definitions using this schema echo "\nChecking potential trpc procedure definitions:" rg --type ts "input:\s*GetLimitSidePosts"Length of output: 531
app/(app)/saved/_client.tsx (3)
11-11: Improved variable naming enhances code clarity.The change from
bookmarkstobookmarksDatafor the destructured query result is a good improvement. It clearly distinguishes between the raw query data and the processed bookmark information, making the code more readable and maintainable.
14-14: Clarify the purpose of the empty object in the query call.An empty object
{}has been added as an argument toapi.post.myBookmarks.useQuery(). While this suggests that the query might now accept parameters, no actual parameters are being passed. This could be related to the mentioned change in handling bookmark limits, but the implementation seems incomplete.Could you please clarify:
- The purpose of this empty object?
- Are there plans to add specific parameters here, such as a limit for the number of bookmarks to fetch?
If parameters are intended to be used, consider adding them explicitly:
api.post.myBookmarks.useQuery({ limit: 10 }); // Example with a limitIf no parameters are needed, it might be clearer to remove the empty object:
api.post.myBookmarks.useQuery();
Line range hint
1-93: Overall improvements align with PR objectives, but some goals remain unaddressed.The changes in this file contribute to better code organization and lay groundwork for potential enhancements in bookmark handling. The variable renaming and introduction of a fallback for bookmark data improve code clarity and robustness.
However, the primary PR objective of automatically updating bookmarks when added or removed is not directly addressed in this file. Consider the following suggestions:
Implement real-time updates: Use React Query's
invalidateQueriesor similar mechanism to trigger a refetch of bookmarks when a bookmark is added or removed in other components.Add error handling: Implement toast notifications for failed bookmark actions as mentioned in the PR objectives.
Optimize performance: If not implemented elsewhere, consider adding pagination or limiting the number of bookmarks fetched, as mentioned in the PR summary.
To ensure these objectives are met, please provide information on how the automatic updating of bookmarks is implemented across components. Are there any other files that handle this functionality?
components/SideBar/SideBarSavedPosts.tsx (1)
Line range hint
1-85: Summary of changes and suggestionsThe changes in this file successfully implement the PR objectives of optimizing bookmark fetching and updating the UI. The new data structure is handled correctly, and the code is more robust with the use of optional chaining and fallback values.
Key points:
- The query optimization with the
limitparameter is a good performance improvement.- The handling of the new data structure is implemented correctly.
- There's a potential issue with an additional query that needs clarification (line 12).
Suggestions for improvement:
- Extract the
limitvalue to a constant for better maintainability.- Clarify or optimize the additional query on line 12.
- Ensure consistency in using the limit value throughout the component.
Overall, the changes are well-implemented, with room for minor improvements in code maintainability and consistency.
To ensure these changes don't introduce any regressions, consider running the following tests:
- Verify that the sidebar displays the correct number of bookmarks (should be 3 or less).
- Check that the total count of bookmarks is displayed correctly.
- Ensure that adding or removing a bookmark updates the sidebar correctly without a page refresh.
#!/bin/bash # Search for test files related to bookmarks or the sidebar rg --type typescript --type tsx -g "*test*" -g "*spec*" "bookmark|sidebar" -lThis script will help locate relevant test files that should be updated or run to verify the changes.
server/api/router/post.ts (1)
14-14: LGTM: New import added forGetLimitSidePosts.The addition of
GetLimitSidePoststo the import statement is appropriate for the changes made to themyBookmarksprocedure.components/ArticlePreview/ArticlePreview.tsx (1)
75-77: Ensure cache invalidation after bookmark mutationBy invalidating the
myBookmarksquery cache usingqueryClient.invalidateQueries()after a successful bookmark mutation, you ensure that all components relying on this data will reflect the updated bookmarks list. This is the recommended approach for cache management in React Query.
| const { data: bookmarksData } = api.post.myBookmarks.useQuery({ | ||
| limit: 3, | ||
| }); | ||
| const { status: bookmarkStatus } = api.post.myBookmarks.useQuery({}); |
There was a problem hiding this comment.
💡 Codebase verification
Redundant myBookmarks.useQuery Calls Detected
Multiple instances of myBookmarks.useQuery are found, some of which may be redundant and could impact performance negatively:
-
components/SideBar/SideBarSavedPosts.tsx
const { data: bookmarksData } = api.post.myBookmarks.useQuery({ limit: 3 });const { status: bookmarkStatus } = api.post.myBookmarks.useQuery({});
-
app/(app)/saved/_client.tsx
const { data: bookmarksData } = api.post.myBookmarks.useQuery({});
Consider consolidating these queries or utilizing the enabled option to prevent unnecessary executions.
🔗 Analysis chain
Clarify the purpose of the additional query
There's an additional myBookmarks query without parameters, from which only the status is used. This might be redundant and could potentially impact performance negatively.
Could you clarify the purpose of this second query? If it's necessary, consider the following suggestions:
- Combine both queries into a single one if possible.
- If separate queries are required, use the
enabledoption in the second query to prevent it from running unnecessarily.
Here's a script to check for other usages of myBookmarks query in the codebase:
This will help us understand if there are other places where this query is used differently.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other usages of myBookmarks query
rg --type typescript --type tsx "myBookmarks\.useQuery" -C 5
Length of output: 114
Script:
#!/bin/bash
# Search for other usages of myBookmarks query in .ts and .tsx files
rg "myBookmarks\.useQuery" --glob "*.ts" --glob "*.tsx" -C 5
Length of output: 2612
| myBookmarks: protectedProcedure | ||
| .input(GetLimitSidePosts) | ||
| .query(async ({ ctx, input }) => { | ||
| const limit = input?.limit ?? undefined; | ||
|
|
||
| const response = await ctx.db.query.bookmark.findMany({ | ||
| columns: { | ||
| id: true, | ||
| }, | ||
| where: (bookmarks, { eq }) => eq(bookmarks.userId, ctx.session.user.id), | ||
| with: { | ||
| post: { | ||
| columns: { | ||
| id: true, | ||
| title: true, | ||
| excerpt: true, | ||
| updatedAt: true, | ||
| published: true, | ||
| readTimeMins: true, | ||
| slug: true, | ||
| }, | ||
| with: { | ||
| user: { | ||
| columns: { | ||
| name: true, | ||
| username: true, | ||
| image: true, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| }, | ||
| orderBy: (bookmarks, { desc }) => [desc(bookmarks.id)], | ||
| }); | ||
| orderBy: (bookmarks, { desc }) => [desc(bookmarks.id)], | ||
| }); | ||
|
|
||
| return response.map(({ id, post }) => ({ bookmarkId: id, ...post })); | ||
| }), | ||
| const totalCount = response.length; | ||
|
|
||
| const bookmarksResponse = response.slice(0, limit || response.length); | ||
|
|
||
| return { | ||
| totalCount, | ||
| bookmarks: bookmarksResponse.map(({ id, post }) => ({ | ||
| bookmarkId: id, | ||
| ...post, | ||
| })), | ||
| }; | ||
| }), |
There was a problem hiding this comment.
LGTM: Improved myBookmarks procedure with pagination support.
The changes to the myBookmarks procedure align well with the PR objectives. The new structure allows for pagination and provides more detailed information about bookmarks, which should improve the sidebar's bookmark display functionality.
Consider optimizing for large datasets.
While the current implementation works, using slice to limit results after fetching all records may not be efficient for large datasets. Consider using database-level pagination for better performance.
Here's a suggestion to optimize the query:
const bookmarksResponse = await ctx.db.query.bookmark.findMany({
// ... existing query options ...
limit: limit,
});
const totalCount = await ctx.db.query.bookmark.count({
where: (bookmarks, { eq }) => eq(bookmarks.userId, ctx.session.user.id),
});
return {
totalCount,
bookmarks: bookmarksResponse.map(({ id, post }) => ({
bookmarkId: id,
...post,
})),
};Add input validation for the limit parameter.
It's important to validate the limit parameter to ensure it's a positive number. This will prevent potential issues with negative or zero limits.
Consider adding input validation:
const limit = input?.limit && input.limit > 0 ? input.limit : undefined;| const { data: bookmarksData, refetch } = api.post.myBookmarks.useQuery({ | ||
| limit: 3, | ||
| }); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider invalidating queries instead of refetching bookmarksData
The bookmarksData and bookmarks variables are not used within this component. Fetching them here solely to access the refetch function may be unnecessary. Instead, you can use the queryClient.invalidateQueries() method provided by React Query to invalidate the myBookmarks query after a successful mutation. This approach avoids fetching unused data and ensures that any component depending on myBookmarks will refetch the updated data.
Apply this change to implement the suggestion:
+import { useQueryClient } from "@tanstack/react-query";
+// Add the import statement for useQueryClient
const ArticlePreview: NextPage<Props> = ({
// props
}) => {
+ const queryClient = useQueryClient();
const [bookmarked, setIsBookmarked] = useState(bookmarkedInitialState);
- const { data: bookmarksData, refetch } = api.post.myBookmarks.useQuery({
- limit: 3,
- });
const { data: session } = useSession();
- const bookmarks = bookmarksData?.bookmarks;
const dateTime = Temporal.Instant.from(new Date(date).toISOString());
const { mutate: bookmark, status: bookmarkStatus } =
api.post.bookmark.useMutation({
onSettled() {
setIsBookmarked((isBookmarked) => !isBookmarked);
},
onSuccess: () => {
- refetch();
+ queryClient.invalidateQueries(
+ api.post.myBookmarks.getQueryKey({ limit: 3 })
+ );
+ // Invalidate the 'myBookmarks' query to ensure up-to-date data
},
});
// rest of the componentAlso applies to: 62-62
| let { data: bookmarks } = api.post.myBookmarks.useQuery(); | ||
| const { status: bookmarkStatus } = api.post.myBookmarks.useQuery(); | ||
| const { data: bookmarksData } = api.post.myBookmarks.useQuery({ | ||
| limit: 3, |
There was a problem hiding this comment.
Instead of the magic number 3 for the limit, you should use the howManySavedToShow const
|
Hey @revanthnetha |
|
Got it @John-Paul-Larkin . I'll make the changes as necessary. |
2.Fixed minor Changes
…ha/codu into revanth/sidebar-posts
revanthnetha
temporarily deployed
to
production
GitHub Actions
Inactive
|
Review the changes @John-Paul-Larkin . Happy to do if any changes needed :) Could you please add the labels hacktober-fest and hactober-fest-accepted tags and add my name in assigness. It really means a lot. |
revanthnetha
temporarily deployed
to
production
GitHub Actions
Inactive
|
Hey @revanthnetha @NiallJoeMaher Do you think we remove the toast on error? |
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
✨ Codu Pull Request 💻
Fixes #834
Approach:
Todo :
Happy to do changes if needed :)
Any Breaking changes
Associated Screenshots