Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on migrating the application to Redux! The core functionality is in place and you've successfully set up all the required slices.
I'm requesting a few changes to improve consistency across your data-fetching slices, which is a key aspect of building a predictable and maintainable Redux store. Currently, postsSlice and usersSlice handle state differently than commentsSlice.
Please standardize them by making the following adjustments:
- In
usersSliceandpostsSlice, ensure theloadedflag is set totruewhen the async thunk isfulfilledorrejected, notpending. This will make its meaning consistent across the app. - In
usersSlice, please rename theusersproperty toitemsfor consistency with the other slices. - In both
postsSliceandusersSlice, change the error property tohasError: booleaninstead oferror: stringto match the state shape incommentsSlice.
Lastly, please remove the unused counter slice and its import from src/app/store.ts as it's leftover from the template. After these adjustments, your solution will be excellent!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Important
Fix the issues, commit the changes, and then re-request my review using the Re-request review button2 in the Reviewers tab.
Tip
If you have any comments about this review, please leave a message in the Add a comment section below and mention me with the @mateacademy-ai-mentor tag3.
I will read your message during my next review of your pull request. Please do not reply directly to my comments, as that feature is not supported yet.
Caution
Do not close the pull request until the review is complete. Closing it early can lead to unexpected behavior and disrupt the review process.
Footnotes
|
|
||
| export const store = configureStore({ | ||
| reducer: { | ||
| counter: counterReducer, |
There was a problem hiding this comment.
The counter slice is not part of the task requirements. It seems to be a leftover from the template. Please remove it from the store, along with its import on line 3.
| type PostsState = { | ||
| items: Post[]; | ||
| loaded: boolean; | ||
| hasError: string; |
There was a problem hiding this comment.
The task requires a hasError property, but its type is inconsistent between slices. Here it's a string, while in the commentsSlice it is a boolean. For consistency across the application's state, it would be better to use a boolean type here as well.
| .addCase(fetchPosts.pending, state => { | ||
| state.loaded = true; | ||
| state.hasError = ''; | ||
| }) | ||
| .addCase(fetchPosts.fulfilled, (state, action) => { | ||
| state.loaded = false; | ||
| state.items = action.payload; | ||
| }) | ||
| .addCase(fetchPosts.rejected, (state, action) => { | ||
| state.loaded = false; | ||
| state.hasError = action.error.message || 'Something went wrong'; | ||
| }); |
There was a problem hiding this comment.
The logic for the loaded state is the reverse of how it's implemented in the commentsSlice. Here, loaded is set to true during the pending state, effectively acting as an isLoading flag. In commentsSlice, loaded is false during pending and true on fulfilled or rejected. To maintain consistency, please update this logic to match the implementation in commentsSlice.
| type UsersState = { | ||
| users: User[]; | ||
| error: string; | ||
| loaded: boolean; |
There was a problem hiding this comment.
For consistency with other slices (like commentsSlice), it's better to use common property names. Consider renaming users to items and changing error: string to hasError: boolean to make the state shape uniform across the application.
| .addCase(fetchUsers.pending, state => { | ||
| state.loaded = true; | ||
| state.error = ''; | ||
| }) | ||
| .addCase(fetchUsers.fulfilled, (state, action) => { | ||
| state.loaded = false; | ||
| state.users = Array.isArray(action.payload) ? action.payload : []; | ||
| }) | ||
| .addCase(fetchUsers.rejected, (state, action) => { | ||
| state.loaded = false; | ||
| state.error = action.error.message ?? 'Unknown error'; | ||
| }); |
There was a problem hiding this comment.
The logic for the loaded flag is inconsistent with its implementation in commentsSlice. Here, loaded: true is used to signify a loading state (pending), whereas in commentsSlice it signifies that the loading process has completed (either successfully or not). To maintain consistency, please adjust the logic here to match the one in commentsSlice.
Anton-Kuchmasov
left a comment
Anton-Kuchmasov
left a comment
There was a problem hiding this comment.
Great job!
don't forget to erase all unused inline comments
| // }); | ||
|
|
||
| // setComments(currentComments => [...currentComments, newComment]); | ||
|
|
There was a problem hiding this comment.
Don't forget to erase all unused inline comments in further tasks
4 participants
DEMO LINK