Skip to content

react_redux_list-of-posts_TetSh#1107

Open
TetiankaSh wants to merge 3 commits intomate-academy:masterfrom
TetiankaSh:develop
Open

react_redux_list-of-posts_TetSh#1107
TetiankaSh wants to merge 3 commits intomate-academy:masterfrom
TetiankaSh:develop

Conversation

@TetiankaSh
Copy link

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

You've made a great start on refactoring the app to use Redux, and the users and author slices are well-implemented. However, this requires changes because several core requirements are not yet met. The state for posts and selectedPost in src/App.tsx, as well as comments in src/components/PostDetails.tsx, is still managed locally with useState instead of being moved into their own Redux slices. Additionally, the UsersContext needs to be removed as per the task description.

Please address the following:

  • Create Redux slices for posts, selectedPost, and comments to manage their state.
  • Add the new reducers to the store configuration in src/app/store.ts.
  • In src/api/posts.ts, add an async thunk to fetch posts for a specific user.
  • For consistency, rename the loading state property to loaded in the posts and users slices (src/api/posts.ts, src/api/users.ts) and adjust the logic accordingly.

✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

src/api/posts.ts Outdated
Comment on lines 13 to 15
export const initPosts = createAsyncThunk('posts/fetch', () => {
return getPosts();
});

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This async thunk fetches all posts, but the main application logic requires fetching posts for a specific user. The component App.tsx still contains logic for fetching posts using local state. A new async thunk should be created here to handle fetching posts by userId using the getUserPosts function.

src/api/posts.ts Outdated
});

type PostsState = {
loading: boolean;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requirement specifies that the state should have a loaded property, but you have named it loading. Please rename this property to loaded and adjust the logic in extraReducers. Keep in mind that the boolean logic for loaded will be the inverse of loading (e.g., when a request is pending, loaded should be false).

src/App.tsx Outdated
Comment on lines 32 to 34
const [posts, setPosts] = useState<Post[]>([]);
const [loaded, setLoaded] = useState(false);
const [hasError, setError] = useState(false);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the task description, the state for posts, including their loading and error statuses, should be managed in a Redux slice named posts. These useState calls should be replaced with selectors to get the data from the Redux store.

src/App.tsx Outdated
const [hasError, setError] = useState(false);

const [author, setAuthor] = useState<User | null>(null);
const [selectedPost, setSelectedPost] = useState<Post | null>(null);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requires creating a Redux slice for selectedPost. This useState hook should be removed and the selected post should be managed through the Redux store.

src/api/users.ts Outdated

type UsersState = {
items: User[];
loading: boolean;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The task requirements for other slices like posts and comments specify using a loaded flag. For consistency, it would be better to use loaded: boolean here instead of loading. This also aligns better with the existing logic in components that check if data has been loaded, rather than if it is currently loading.

Comment on lines 34 to 37
builder.addCase(initUsers.pending, state => ({
...state,
loading: true,
}));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a new request is initiated, it's good practice to reset the hasError state. This prevents displaying a stale error message from a previous failed request while new data is being fetched. You should add hasError: false here.

Comment on lines 8 to 12
reducer: {
counter: counterReducer,
users: usersSlice.reducer,
author: authorSlice.reducer,
},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The store configuration is incomplete. According to the task requirements, you need to add the reducers for the posts, selectedPost, and comments slices here after you create them.

Comment on lines 82 to 92
const deleteComment = async (commentId: number) => {
// we delete the comment immediately so as
// not to make the user wait long for the actual deletion
// eslint-disable-next-line max-len
setComments(currentComments =>
currentComments.filter(comment => comment.id !== commentId),
setComments(
currentComments => currentComments.filter(
comment => comment.id !== commentId,
),
);

await commentsApi.deleteComment(commentId);
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similarly, deleting a comment should be managed by an async thunk. The component should dispatch an action with the commentId to trigger the deletion process in Redux.

Comment on lines 7 to 20
export const UsersProvider: React.FC = ({ children }) => {
const [users, setUsers] = useState<User[]>([]);

useEffect(() => {
getUsers().then(setUsers);
getUsers()
.then(setUsers);
}, []);

return <UserContext.Provider value={users}>{children}</UserContext.Provider>;
return (
<UserContext.Provider value={users}>
{children}
</UserContext.Provider>
);
};

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the task requirements, UsersContext should be removed. The state for users should be managed by the users slice in Redux, and components should access this data from the Redux store.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

Great job on refactoring the application to use Redux! You've made significant progress. However, a few core requirements still need to be addressed before this can be approved.

Here are the changes required:

  1. selectedPost must be in its own slice: Currently, selectedPost state and its actions are part of features/postsSlice.ts. Please move this logic into a new, dedicated selectedPostSlice.ts as required by the task.
  2. PostDetails component needs to use Redux for comments: In src/components/PostDetails.tsx, the comments, loading, and error states are still managed by useState. This component should be refactored to use useAppSelector and useAppDispatch to interact with the comments slice.
  3. UsersContext should be removed: The file src/components/UsersContext.tsx must be deleted, as its functionality has been replaced by the users Redux slice.
  4. Inconsistent state property in users slice: In src/api/users.ts, the state property should be renamed from loading to loaded for consistency with the other slices, as mentioned in the previous review.

Please make these adjustments to fully meet the task requirements. Keep up the great work!


✨ 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment on lines +28 to +33
const {
items: posts,
loaded,
hasError,
selectedPost,
} = useAppSelector(state => state.posts);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the task requirements, selectedPost should be managed in its own Redux slice, separate from the posts slice. This selector retrieves it from state.posts, which needs to be adjusted. The posts slice should only contain loaded, hasError, and items.

// we clear the post when an author is changed
// not to confuse the user
setSelectedPost(null);
dispatch(setSelectedPost(null));

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This dispatch should use the action from the new selectedPost slice to clear the selected post, rather than the action from postsSlice.

posts={posts}
selectedPostId={selectedPost?.id}
onPostSelected={setSelectedPost}
onPostSelected={post => dispatch(setSelectedPost(post))}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The setSelectedPost action should be imported and dispatched from the dedicated selectedPost slice, not from postsSlice.

src/api/users.ts Outdated

type UsersState = {
items: User[];
loading: boolean;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This property should be named loaded to be consistent with the other slices (posts, comments) and to address the feedback from the previous review. After renaming it, please make sure to adjust the logic in the extraReducers: loaded should be false when the request is pending, and true when it is fulfilled or rejected.

loaded: boolean;
hasError: boolean;
items: Post[];
selectedPost: Post | null;

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The selectedPost property should not be part of the PostsState. According to the task requirements, selectedPost needs to be managed in its own separate Redux slice. This slice should only contain loaded, hasError, and items.

Comment on lines +31 to +34
setSelectedPost: (state, action: PayloadAction<Post | null>) => ({
...state,
selectedPost: action.payload,
}),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This reducer for managing selectedPost should be moved to its own dedicated slice as required by the task description.

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution as an educational override to help you progress, but there are a couple of key areas to correct.

Excellent work on refactoring the application to use Redux and creating dedicated slices! The structure is much cleaner now. The main issue is that the selectedPost state is not fully migrated. While you correctly created selectedPostSlice.ts, the old logic still exists in src/features/postsSlice.ts and is being used by src/App.tsx. Please remove the selectedPost logic from the posts slice and update App.tsx to use the new, dedicated slice.

Additionally, in src/api/users.ts, the loaded state logic is inverted. It should be false during the pending state and true for fulfilled and rejected to ensure loading indicators work correctly.

Addressing these points will finalize the refactoring. You're very close to a perfect implementation!


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

Comment on lines +25 to +30
const {
items: posts,
loaded,
hasError,
selectedPost,
} = useAppSelector(state => state.posts);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The selectedPost state is still being managed as part of the posts slice. According to the task requirements and previous feedback, it must be in its own dedicated slice.

You'll need to update this component to:

  1. Select the post from its own slice, for example: const selectedPost = useAppSelector(state => state.selectedPost);.
  2. Import and dispatch the correct action from selectedPostSlice when a post is selected or cleared.

Comment on lines +33 to +50
extraReducers(builder) {
builder.addCase(initUsers.pending, state => ({
...state,
loaded: true,
}));

builder.addCase(initUsers.rejected, state => ({
...state,
hasError: true,
loaded: false,
}));

builder.addCase(initUsers.fulfilled, (state, action) => ({
...state,
items: action.payload,
loaded: false,
}));
},

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic for managing the loaded state seems to be inverted here. For consistency with other slices and correct UI behavior, loaded should be false while data is being fetched (pending state) and true once fetching is complete—whether it was successful (fulfilled) or failed (rejected).

Comment on lines +13 to +34
selectedPost: Post | null;
};

const initialState: PostsState = {
loaded: false,
hasError: false,
items: [],
selectedPost: null,
};

export const postsSlice = createSlice({
name: 'posts',
initialState,
reducers: {
clearPosts: state => ({
...state,
items: [],
}),
setSelectedPost: (state, action: PayloadAction<Post | null>) => ({
...state,
selectedPost: action.payload,
}),

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to the task requirements and previous feedback, the selectedPost state and its related logic must be managed in its own dedicated slice. Please remove the selectedPost property from the PostsState type, the initial state, and the setSelectedPost reducer from this file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants