Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on refactoring the application to use Redux Toolkit! You've successfully met all the core requirements for this task, and the overall structure is solid. I am approving your solution. As you continue to develop your skills, I have a couple of suggestions for improvement:
- In
src/features/commentsSlice.ts, theloadedstate is modified during thependingstate for creating and deleting comments. It's better to only manage this flag during the initial data fetch (loadPostComments) to avoid inconsistent UI loading states. - To make your code more robust and leverage TypeScript's full potential, consider adding explicit types to function and async thunk arguments, for example, for the
postparameter inApp.tsxand thepostIdincommentsSlice.ts.
These are not blockers, but applying these practices will enhance the quality and maintainability of your code. Keep up the excellent work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| // We disable the spinner in any case | ||
| .finally(() => setLoaded(true)); | ||
| } | ||
| const setSelectedPost = post => { |
There was a problem hiding this comment.
It's a good practice to provide an explicit type for function parameters. In this case, based on how setSelectedPost is used, the post parameter should be of type Post | null. You'll need to import the Post type as well.
|
|
||
| export const loadPostComments = createAsyncThunk( | ||
| 'comments/getPostComments', | ||
| async postId => { |
There was a problem hiding this comment.
It's a good practice to add an explicit type for the postId parameter to leverage TypeScript's type safety. Based on its usage, it should be a number.
| ); | ||
| export const createCommentAsync = createAsyncThunk( | ||
| 'comments/createComment', | ||
| async data => { |
There was a problem hiding this comment.
The data parameter should have an explicit type. You could define a type for the new comment data, for example Omit<Comment, 'id'>, to ensure type safety when creating a comment.
|
|
||
| export const deleteCommentAsync = createAsyncThunk( | ||
| 'comments/deleteComment', | ||
| async id => { |
There was a problem hiding this comment.
The id parameter should have an explicit type, which should be number in this case.
| .addCase(createCommentAsync.pending, state => { | ||
| state.loaded = true; |
There was a problem hiding this comment.
Setting state.loaded = true during the pending state for creating a comment is inconsistent with the loadPostComments.pending case which sets it to false. The loaded flag should typically indicate whether the list of comments has been fetched. During a creation or deletion, the list is already loaded, so this state probably shouldn't be modified here.
| .addCase(deleteCommentAsync.pending, state => { | ||
| state.loaded = true; |
There was a problem hiding this comment.
Similar to the createCommentAsync.pending case, setting state.loaded = true here is inconsistent. The list of comments is still considered loaded while one is being deleted, so modifying the loaded state is likely unnecessary.
| const users = useAppSelector(selectUsers); | ||
| const author = useAppSelector(selectAuthor); | ||
|
|
||
| const onChange = a => { |
There was a problem hiding this comment.
To leverage the full power of TypeScript, it's a good practice to provide an explicit type for the a parameter. Based on its usage, what type should it be? You might need to import the User type.
| hasError: false, | ||
| }; | ||
|
|
||
| export const loadUserPosts = createAsyncThunk('posts/getPosts', async id => { |
There was a problem hiding this comment.
To improve type safety, it's a good practice to explicitly type the arguments of your async thunks. Based on the getUserPosts function, what type should the id parameter be?
| .addCase(loadUserPosts.pending, state => { | ||
| state.loaded = false; |
There was a problem hiding this comment.
When a new data loading process starts, it's a good idea to also reset any previous error state. Consider setting hasError to false here to ensure the UI doesn't show a stale error message from a previous failed request.
2 participants
No description provided.