fix: fixed problem with update bookmarks #879
Conversation
|
@Maczi01 is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThe update involves enhancing the Changes
Poem
Recent Review DetailsConfiguration used: CodeRabbit UI Files selected for processing (1)
Files skipped from review as they are similar to previous changes (1)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
![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
Review Status
Configuration used: CodeRabbit UI
Files selected for processing (1)
- components/ArticlePreview/ArticlePreview.tsx (2 hunks)
Additional comments not posted (2)
components/ArticlePreview/ArticlePreview.tsx (2)
14-15: The import statements foruseQueryClientandtoastare correctly added and necessary for the bookmark update functionality and displaying success messages.
63-63: The initialization ofqueryClientusinguseQueryClientis correctly implemented for invalidating queries after bookmark updates.
| async onSuccess() { | ||
| await queryClient.invalidateQueries(); | ||
| toast.success("Bookmarks updated!"); |
There was a problem hiding this comment.
Refine the query invalidation to target specific queries related to bookmarks for improved efficiency.
- await queryClient.invalidateQueries();
+ await queryClient.invalidateQueries(['myBookmarks']);If targeting specific queries does not work as expected, ensure that the query keys used in invalidateQueries match exactly those used in your query definitions. Also, verify that the data structure returned by the queries is consistent.
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.
| async onSuccess() { | |
| await queryClient.invalidateQueries(); | |
| toast.success("Bookmarks updated!"); | |
| async onSuccess() { | |
| await queryClient.invalidateQueries(['myBookmarks']); | |
| toast.success("Bookmarks updated!"); |
There was a problem hiding this comment.
Thanks for this! ❤️
Aha, the problem here is we need an optimistic update. Basically, in local development it's hard to test because everything happens fast. But it's so the bookmark instantly is filled or unfilled when bookmarking or unmarking. In production you'll notice a half second delay which could cause the user to hit it multiple times. This will behave the exact same except we have a toast now too.
So the solution doesn't need a notification but it does need to look like the user has instantly bookmarked something (and then only alert them if something went wrong which should be rare).
|
Whooo, optimistic update can be perfect here. https://tanstack.com/query/v4/docs/framework/react/guides/optimistic-updates Maybe we can change approach a littie bit and use disabled property on button? Something like: I can get isLoading from useMutation. |
Sorry I'm slow getting back to you. Yeah something like that might be good! I'll also take a look and see what options there is |
|
@NiallJoeMaher can you check it now? button will be disable during action, it prevent sending another request. Should be helpful :-) If not, we can talk about better solution |
| const queryClient = useQueryClient(); | ||
|
|
||
| const { | ||
| mutate: bookmark, | ||
| status: bookmarkStatus, | ||
| isLoading, | ||
| } = api.post.bookmark.useMutation({ | ||
| onSettled() { | ||
| setIsBookmarked((isBookmarked) => !isBookmarked); | ||
| }, | ||
| async onSuccess() { | ||
| await queryClient.invalidateQueries(); | ||
| toast.success("Bookmarks updated!"); | ||
| }, |
There was a problem hiding this comment.
Refine the query invalidation to target specific queries related to bookmarks for improved efficiency.
- await queryClient.invalidateQueries();
+ await queryClient.invalidateQueries(['bookmarks']);If targeting specific queries does not work as expected, ensure that the query keys used in invalidateQueries match exactly those used in your query definitions. Also, verify that the data structure returned by the queries is consistent.
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.
| const queryClient = useQueryClient(); | |
| const { | |
| mutate: bookmark, | |
| status: bookmarkStatus, | |
| isLoading, | |
| } = api.post.bookmark.useMutation({ | |
| onSettled() { | |
| setIsBookmarked((isBookmarked) => !isBookmarked); | |
| }, | |
| async onSuccess() { | |
| await queryClient.invalidateQueries(); | |
| toast.success("Bookmarks updated!"); | |
| }, | |
| const queryClient = useQueryClient(); | |
| const { | |
| mutate: bookmark, | |
| status: bookmarkStatus, | |
| isLoading, | |
| } = api.post.bookmark.useMutation({ | |
| onSettled() { | |
| setIsBookmarked((isBookmarked) => !isBookmarked); | |
| }, | |
| async onSuccess() { | |
| await queryClient.invalidateQueries(['bookmarks']); | |
| toast.success("Bookmarks updated!"); | |
| }, |
There was a problem hiding this comment.
This is awesome, and would work very well. I'm going to suggest something more opinionated because I like how it looks 🙈
I was looking at the docs for "optimistic updates" and found this: https://tanstack.com/query/latest/docs/framework/react/guides/mutations#mutation-side-effects
onMutate: variables => {
// A mutation is about to happen!
// We can update right away
// Return a context containing data to use when for example rolling back
return { id: 1 }
},
onError: (error, variables, context) => {
// An error happened!
console.log(`rolling back optimistic update with id ${context.id}`)
},I'm not sure how the logic would work inside it but basically, on mutate, update the UI right away (as if the bookmark worked instantly) and then only on error roll back and present an error toast. I think a toast pop up for every bookmark success might get annoying especially on small screens.
Example:
https://tanstack.com/query/v3/docs/framework/react/examples/optimistic-updates-typescript
✨ Codu Pull Request 💻
Pull Request details
Fixed problems with update bookmarks - after click icon, article will be added to bookmarks + toast "Bookmark updated"
Any Breaking changes
Important. I am not sure it is most efficient solution.
In onSuccess I am invalidating query, with queryClient.invalidateQueries();
Imo problem is with invalidate all queries, not particular one. I tried with:
queryClient.invalidateQueries([myBookmarks), but it doesn't work
Associated Screenshots
Summary by CodeRabbit