A1daros:react_dynamic-list-of-posts#1464
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are still explicit checklist requirements that are not met, even though your core functionality works well. The most important issues are missing propTypes for components like App, UserSelector, PostsList, PostDetails, and NewCommentForm [CHECKLIST ITEM #3]; even with TypeScript, the checklist explicitly asks you to describe object props via propTypes, so you should add definitions such as UserSelector.propTypes = { users: PropTypes.array.isRequired, ... } in each relevant file. Error handling for delete-comment and several fetch flows currently relies on .catch chained to promises (for example in src/App.tsx and src/hooks/usePosts.ts), which conflicts with the requirement to use try { ... } catch for errors [CHECKLIST ITEM #8]; refactor these handlers into async functions and wrap the client.get/post/delete calls in try/catch blocks, updating your error state inside the catch. There is also direct DOM interaction via document.addEventListener('mousedown', ...) in components like UserSelector and logic in usePosts/PostDetails that attaches document-level listeners, which violates the "don't interact with DOM directly" guideline [CHECKLIST ITEM #2]; remove these global listeners and manage open/close purely with React state and component-level events.
A secondary checklist point is that some handlers call multiple setState functions in a single function (for example clearForm and handleChange in NewCommentForm, and handleSelectUser setting both selectedUser and selectedPost) [CHECKLIST ITEM #9]; while marked optional, you can improve clarity by consolidating related updates into one state object or combining them into a single setState per handler. Functionally, your user selection, posts table, sidebar, comments loading, optimistic deletion, and form validation (including trimming and preventing whitespace-only submissions) are all implemented very well. Once you add the required propTypes, switch remaining error handling to async/await with try/catch, and remove direct document interaction, your solution will fully comply with the task and checklist. Keep up the good work—these changes are mostly about aligning with the course’s technical expectations rather than fixing broken behavior.
✨ 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
| ); | ||
| </main> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
This violates checklist item #3: "make sure you described objects in propTypes". Even though you’re using TypeScript types, the checklist explicitly requires describing objects in propTypes, so consider adding propTypes definitions for this component’s props.
| <UserSelector | ||
| users={users} | ||
| selectedUser={selectedUser} | ||
| selectUser={handleSelectUser} | ||
| /> | ||
| </div> | ||
|
|
||
| <Loader /> | ||
| <div className="block" data-cy="MainContent"> | ||
| {!selectedUser && ( | ||
| <p data-cy="NoSelectedUser">No user selected</p> | ||
| )} | ||
|
|
||
| <div | ||
| className="notification is-danger" | ||
| data-cy="PostsLoadingError" | ||
| > | ||
| Something went wrong! | ||
| </div> | ||
| {isPostsLoading && <Loader />} | ||
|
|
||
| <div className="notification is-warning" data-cy="NoPostsYet"> | ||
| No posts yet | ||
| </div> | ||
| {postsError && ( |
There was a problem hiding this comment.
This violates checklist item #8: "* use try {..} catch for error handling". Here you handle the delete-comment error using .catch on the promise instead of a try/catch block. To align with the checklist and keep error handling consistent with your add-comment logic, consider wrapping the async delete in try/catch (e.g., in an async function) and managing commentsError there.
| ) => { | ||
| const { name: field, value } = event.target; | ||
|
|
||
| setValues(prev => ({ ...prev, [field]: value })); | ||
| setErrors(prev => ({ ...prev, [field]: false })); |
There was a problem hiding this comment.
This component accepts object props (users, selectedUser, selectUser) but does not declare any propTypes. This violates checklist item #3: "make sure you described objects in propTypes" – you should add a UserSelector.propTypes definition describing these props, or otherwise ensure the project satisfies this requirement.
| type Props = { | ||
| onSubmit: (data: CommentData) => Promise<void>; | ||
| }; | ||
|
|
||
| export const NewCommentForm: React.FC<Props> = ({ onSubmit }) => { | ||
| const [submitting, setSubmitting] = useState(false); | ||
|
|
||
| const [values, setValues] = useState({ name: '', email: '', body: '' }); | ||
| const [errors, setErrors] = useState({ | ||
| name: false, | ||
| email: false, | ||
| body: false, | ||
| }); | ||
|
|
||
| const { name, email, body } = values; | ||
|
|
||
| const clearForm = () => { | ||
| setValues({ | ||
| name: '', | ||
| email: '', | ||
| body: '', | ||
| }); | ||
|
|
||
| setErrors({ | ||
| name: false, | ||
| email: false, | ||
| body: false, | ||
| }); | ||
| }; | ||
|
|
||
| const handleChange = ( | ||
| event: React.ChangeEvent<HTMLInputElement | HTMLTextAreaElement>, | ||
| ) => { | ||
| const { name: field, value } = event.target; | ||
|
|
||
| setValues(prev => ({ ...prev, [field]: value })); | ||
| setErrors(prev => ({ ...prev, [field]: false })); | ||
| }; | ||
|
|
||
| const handleSubmit = async (event: React.FormEvent) => { | ||
| event.preventDefault(); | ||
|
|
||
| const nextErrors = { | ||
| name: !name.trim(), | ||
| email: !email.trim(), | ||
| body: !body.trim(), | ||
| }; | ||
|
|
||
| setErrors(nextErrors); | ||
|
|
||
| if (nextErrors.name || nextErrors.email || nextErrors.body) { | ||
| return; | ||
| } | ||
|
|
||
| try { |
There was a problem hiding this comment.
PostsList receives object-like props (posts, selectedPostId, onPostClick) but has no propTypes definition. This violates checklist item #3: "make sure you described objects in propTypes". Consider adding PostsList.propTypes to describe these props explicitly.
|
|
||
| export const NewCommentForm: React.FC<Props> = ({ onSubmit }) => { | ||
| const [submitting, setSubmitting] = useState(false); | ||
|
|
||
| const [values, setValues] = useState({ name: '', email: '', body: '' }); | ||
| const [errors, setErrors] = useState({ | ||
| name: false, | ||
| email: false, | ||
| body: false, | ||
| }); | ||
|
|
||
| const { name, email, body } = values; | ||
|
|
||
| const clearForm = () => { | ||
| setValues({ | ||
| name: '', |
There was a problem hiding this comment.
PostDetails also uses structured props (post, comments, error, loading, onDeleteComment, onAddComment) but does not define propTypes. This violates checklist item #3: "make sure you described objects in propTypes" – add a PostDetails.propTypes declaration to satisfy the checklist.
|
|
||
| const handleDeleteComment = (commentId: number) => { | ||
| const commentToDelete = comments.find(comment => comment.id === commentId); | ||
|
|
||
| if (!commentToDelete) { | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
This also violates checklist item #9: "* don't use setState several times in one function call (method)". handleChange calls setValues and setErrors sequentially within the same handler; refactoring to minimize multiple state updates per event will better match the checklist recommendation.
| const handleDeleteComment = (commentId: number) => { | ||
| const commentToDelete = comments.find(comment => comment.id === commentId); | ||
|
|
||
| if (!commentToDelete) { | ||
| return; | ||
| } | ||
|
|
||
| setCommentsError(''); | ||
|
|
||
| setComments(prev => prev.filter(comment => comment.id !== commentId)); | ||
|
|
||
| client.delete(`/comments/${commentId}`).catch(() => { | ||
| setComments(prev => [...prev, commentToDelete]); | ||
| setCommentsError('Failed to delete comment. Please try again.'); | ||
| }); |
There was a problem hiding this comment.
This violates checklist item #8: "* use try {..} catch for error handling". Here you handle the delete-comment error using .catch on client.delete instead of wrapping the async logic in a try/catch block. For consistency with the add-comment handler and to follow the checklist, consider making handleDeleteComment async and using try/catch around the API call.
| import { Comment } from '../types/Comment'; | ||
|
|
||
| export const usePosts = () => { | ||
| const [users, setUsers] = useState<User[]>([]); | ||
| const [posts, setPosts] = useState<Post[]>([]); | ||
| const [comments, setComments] = useState<Comment[]>([]); | ||
|
|
||
| const [selectedUser, setSelectedUser] = useState<User | null>(null); | ||
| const [selectedPost, setSelectedPost] = useState<Post | null>(null); | ||
|
|
||
| const [isPostsLoading, setIsPostsLoading] = useState(false); | ||
| const [postsError, setPostsError] = useState(''); | ||
|
|
||
| const [isCommentsLoading, setIsCommentsLoading] = useState(false); | ||
| const [commentsError, setCommentsError] = useState(''); | ||
|
|
||
| const handlePostClick = (post: Post) => { | ||
| setSelectedPost(currentPost => { | ||
| if (currentPost?.id === post.id) { | ||
| return null; | ||
| } | ||
|
|
||
| return post; | ||
| }); | ||
| }; | ||
|
|
||
| const handleSelectUser = (user: User) => { | ||
| setSelectedUser(user); | ||
| setSelectedPost(null); | ||
| }; | ||
|
|
||
| const handleDeleteComment = (commentId: number) => { | ||
| const commentToDelete = comments.find(comment => comment.id === commentId); | ||
|
|
||
| if (!commentToDelete) { | ||
| return; | ||
| } | ||
|
|
||
| setCommentsError(''); | ||
|
|
||
| setComments(prev => prev.filter(comment => comment.id !== commentId)); | ||
|
|
||
| client.delete(`/comments/${commentId}`).catch(() => { | ||
| setComments(prev => [...prev, commentToDelete]); | ||
| setCommentsError('Failed to delete comment. Please try again.'); | ||
| }); | ||
| }; | ||
|
|
||
| const handleAddComment = (newComment: Comment) => { | ||
| setComments(prev => [...prev, newComment]); | ||
| }; | ||
|
|
||
| useEffect(() => { | ||
| setIsPostsLoading(true); | ||
| setPostsError(''); | ||
|
|
||
| client | ||
| .get<User[]>('/users') | ||
| .then(data => setUsers(data)) | ||
| .catch(() => setPostsError('Failed to load users')) | ||
| .finally(() => { | ||
| setIsPostsLoading(false); | ||
| }); | ||
| }, []); | ||
|
|
||
| useEffect(() => { | ||
| if (!selectedUser) { | ||
| setPosts([]); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| setIsPostsLoading(true); | ||
| setPostsError(''); | ||
|
|
||
| client | ||
| .get<Post[]>(`/posts?userId=${selectedUser.id}`) | ||
| .then(data => setPosts(data)) | ||
| .catch(() => setPostsError('Failed to load posts')) |
There was a problem hiding this comment.
This violates checklist item #3: "make sure you described objects in propTypes". The UserSelector component uses a Props TypeScript type but does not define UserSelector.propTypes. To satisfy the checklist, add a propTypes declaration describing users, selectedUser, and selectUser.
| import { Comment } from '../types/Comment'; | ||
|
|
||
| export const usePosts = () => { | ||
| const [users, setUsers] = useState<User[]>([]); | ||
| const [posts, setPosts] = useState<Post[]>([]); | ||
| const [comments, setComments] = useState<Comment[]>([]); | ||
|
|
||
| const [selectedUser, setSelectedUser] = useState<User | null>(null); | ||
| const [selectedPost, setSelectedPost] = useState<Post | null>(null); | ||
|
|
||
| const [isPostsLoading, setIsPostsLoading] = useState(false); | ||
| const [postsError, setPostsError] = useState(''); | ||
|
|
||
| const [isCommentsLoading, setIsCommentsLoading] = useState(false); | ||
| const [commentsError, setCommentsError] = useState(''); | ||
|
|
||
| const handlePostClick = (post: Post) => { | ||
| setSelectedPost(currentPost => { | ||
| if (currentPost?.id === post.id) { | ||
| return null; | ||
| } | ||
|
|
||
| return post; | ||
| }); | ||
| }; | ||
|
|
||
| const handleSelectUser = (user: User) => { | ||
| setSelectedUser(user); | ||
| setSelectedPost(null); | ||
| }; | ||
|
|
||
| const handleDeleteComment = (commentId: number) => { | ||
| const commentToDelete = comments.find(comment => comment.id === commentId); | ||
|
|
||
| if (!commentToDelete) { | ||
| return; | ||
| } | ||
|
|
||
| setCommentsError(''); | ||
|
|
||
| setComments(prev => prev.filter(comment => comment.id !== commentId)); | ||
|
|
||
| client.delete(`/comments/${commentId}`).catch(() => { | ||
| setComments(prev => [...prev, commentToDelete]); | ||
| setCommentsError('Failed to delete comment. Please try again.'); | ||
| }); | ||
| }; | ||
|
|
||
| const handleAddComment = (newComment: Comment) => { | ||
| setComments(prev => [...prev, newComment]); | ||
| }; | ||
|
|
||
| useEffect(() => { | ||
| setIsPostsLoading(true); | ||
| setPostsError(''); |
There was a problem hiding this comment.
This violates checklist item #3: "make sure you described objects in propTypes". PostsList uses a Props type but does not declare any propTypes. You should add PostsList.propTypes that describes posts, selectedPostId, and onPostClick to comply with the checklist.
| const [users, setUsers] = useState<User[]>([]); | ||
| const [posts, setPosts] = useState<Post[]>([]); | ||
| const [comments, setComments] = useState<Comment[]>([]); | ||
|
|
||
| const [selectedUser, setSelectedUser] = useState<User | null>(null); | ||
| const [selectedPost, setSelectedPost] = useState<Post | null>(null); | ||
|
|
||
| const [isPostsLoading, setIsPostsLoading] = useState(false); | ||
| const [postsError, setPostsError] = useState(''); | ||
|
|
||
| const [isCommentsLoading, setIsCommentsLoading] = useState(false); | ||
| const [commentsError, setCommentsError] = useState(''); | ||
|
|
||
| const handlePostClick = (post: Post) => { | ||
| setSelectedPost(currentPost => { | ||
| if (currentPost?.id === post.id) { |
There was a problem hiding this comment.
This violates checklist item #3: "make sure you described objects in propTypes". PostDetails defines Props but does not provide propTypes; adding PostDetails.propTypes that describes post, comments, error, loading, onDeleteComment, and onAddComment will align with the checklist.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because it now satisfies the functional requirements and aligns with the checklist constraints. The app correctly loads users, filters posts by the selected user, opens the sidebar with the chosen post, and handles comments with proper loading, error, and empty states. The NewCommentForm enforces validation (including trimming whitespace-only input), maintains the required behaviors (keeping name/email, clearing body, is-loading on submit, clear button), and integrates well with optimistic comment addition and deletion. You’ve removed direct DOM manipulation, are using async/await with try/catch for all API flows, and your use of classnames, stable keys, and propTypes (including object shapes) matches the checklist items. Any remaining ideas (like further UX polish or refactoring) are purely optional and not required for this task. Overall, this is a solid implementation that demonstrates good attention to both functionality and the specified coding standards.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
npm t:
Spec Tests Passing Failing Pending Skipped
┌────────────────────────────────────────────────────────────────────────────────────────────────┐
│ ✔ page.spec.js 04:21 124 124 - - - │
└────────────────────────────────────────────────────────────────────────────────────────────────┘
✔ All specs passed! 04:21 124 124 - - -