added solution#1461
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are a few checklist-related issues that are considered critical for this task. Functionally your app is in very good shape: users, posts, comments, loaders, validation, and optimistic delete all work as required, and the structure is clear and idiomatic.
The critical problems to fix are:
- In
PostDetails.tsx(handleAddComment) you callclient.postwithouttry { ... } catch, so add‑comment errors are not handled and the user cannot retry as required by the spec and the optional error‑handling checklist. Wrap that call intry/catch, keep the comment in local state only on success, and on error expose some error flag/message so the user understands the add failed and can try again. - In
UserSelector.tsxyou usedocument.addEventListener('click', ...)to detect outside clicks, which violates the explicit “don’t interact with DOM directly” checklist item [CHECKLIST ITEM #2]; refactor to a React‑driven approach (for example, closing the dropdown on blur, or by handling clicks within React components rather than attaching a global document listener).
Less critical but worth addressing: you call multiple state setters in one handler in App.tsx (and similarly in NewCommentForm handlers), which goes against the optional “avoid multiple setState calls in one function” recommendation; consider grouping related flags into a single state object for clarity. Also, the checklist asks to describe objects in propTypes; you’ve done this via TypeScript interfaces only, so either add propTypes to your components or confirm with your mentor that TS types are an accepted replacement. Overall, you’re very close – fixing the add‑comment error handling and removing direct DOM manipulation will bring your solution fully in line with the requirements.
✨ 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
| const handleUserSelect = (user: User) => { | ||
| setSelectedUser(user); | ||
|
|
||
| setSelectedPost(null); | ||
| setShowCommentForm(false); | ||
|
|
||
| setPosts([]); | ||
| setPostsLoading(true); | ||
| setPostsError(false); |
There was a problem hiding this comment.
Checklist item "* don't use setState several times in one function call (method)" is not followed here: handleUserSelect calls setSelectedUser, setSelectedPost, setShowCommentForm, setPosts, setPostsLoading, and setPostsError in one handler. Consider grouping related state or restructuring to reduce multiple setState calls in a single function.
| const handlePostSelect = (post: Post | null) => { | ||
| if (!post) { | ||
| setSelectedPost(null); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| setSelectedPost(post); | ||
| setShowCommentForm(false); | ||
|
|
||
| setComments([]); | ||
| setCommentsLoading(true); | ||
| setCommentsError(false); |
There was a problem hiding this comment.
Similarly to the previous comment, handlePostSelect calls setSelectedPost, setShowCommentForm, setComments, setCommentsLoading, and setCommentsError in a single handler, which goes against the optional checklist recommendation about multiple setState calls per function.
|
|
||
| const newErrors: Errors = { | ||
| name: name.trim() === '', | ||
| email: email.trim() === '', | ||
| body: body.trim() === '', | ||
| }; | ||
|
|
||
| setErrors(newErrors); | ||
|
|
There was a problem hiding this comment.
handleUserSelect calls several state setters in sequence (setSelectedUser, setSelectedPost, setShowCommentForm, setPosts, setPostsLoading, setPostsError); this goes against optional checklist advice: * don't use setState several times in one function call (method) — consider grouping related state or using a single setter for cleaner updates.
| name: name.trim(), | ||
| email: email.trim(), | ||
| body: body.trim(), | ||
| }); | ||
|
|
||
| setBody(''); | ||
| } finally { | ||
| setIsLoading(false); | ||
| } | ||
| }; | ||
|
|
||
| const handleReset = () => { | ||
| setBody(''); |
There was a problem hiding this comment.
Similarly, handlePostSelect calls multiple state setters (setSelectedPost, setShowCommentForm, setComments, setCommentsLoading, setCommentsError) in one handler; this matches the optional "Bad" pattern from checklist item * don't use setState several times in one function call — think about combining related state to reduce the number of separate updates.
| className="delete is-small" | ||
| aria-label="delete" | ||
| onClick={() => onDeleteComment(comment.id)} | ||
| > | ||
| delete button | ||
| </button> | ||
| </div> | ||
|
|
||
| <div className="message-body" data-cy="CommentBody"> | ||
| {'Multi\nline\ncomment'} | ||
| </div> | ||
| </article> | ||
| <div className="message-body" data-cy="CommentBody"> |
There was a problem hiding this comment.
handleAddComment calls client.post without a surrounding try {..} catch, so any network/API error will be unhandled. This violates the optional checklist requirement "* use try {..} catch for error handling" and the description's "(*) Handle Add and Delete errors so the user can retry" for the Add case. Consider wrapping the call in try/catch and surfacing the error so the user can retry.
| export const PostDetails: React.FC<Props> = ({ | ||
| post, | ||
| comments, | ||
| commentsLoading, | ||
| commentsError, | ||
| showCommentForm, | ||
| onShowForm, | ||
| onAddComment, | ||
| onDeleteComment, | ||
| }) => { | ||
| return ( | ||
| <div className="content" data-cy="PostDetails"> | ||
| <div className="content" data-cy="PostDetails"> | ||
| <div className="block"> | ||
| <h2 data-cy="PostTitle"> | ||
| #18: voluptate et itaque vero tempora molestiae | ||
| </h2> | ||
|
|
||
| <p data-cy="PostBody"> | ||
| eveniet quo quis laborum totam consequatur non dolor ut et est | ||
| repudiandae est voluptatem vel debitis et magnam | ||
| </p> | ||
| </div> | ||
| <div className="block"> | ||
| <h2 data-cy="PostTitle"> |
There was a problem hiding this comment.
Using document.addEventListener('click', ...) is a direct DOM interaction, which contradicts checklist item #2: "don't interact with DOM directly, use React as much as possible;". Consider refactoring to a more React‑centric pattern (e.g. higher‑level event handling or a dedicated hook) that avoids manual DOM subscriptions.
| useEffect(() => { | ||
| const handleClickOutside = (event: MouseEvent) => { | ||
| if ( | ||
| dropdownRef.current && | ||
| !dropdownRef.current.contains(event.target as Node) | ||
| ) { | ||
| setIsOpen(false); | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener('click', handleClickOutside); |
There was a problem hiding this comment.
This document.addEventListener('click', ...) is a direct DOM interaction and violates checklist item #2: "don't interact with DOM directly, use React as much as possible". Consider refactoring to a React-based pattern (e.g., closing the dropdown on specific React events) rather than attaching global document listeners.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution because all core functional requirements are implemented and working: users and posts load correctly, the sidebar opens with post details and comments, loaders and error notices are shown, and comments can be added and deleted (with optimistic delete and proper error handling). The comment form behavior matches the specification: required fields with trim()-based validation, errors shown only after submit and cleared on change, name/email preserved while the text is cleared on success, a clear button that resets text and errors, and an is-loading state on submit. You correctly use fetchClient, classnames, stable keys, and avoid direct DOM manipulation, and previous critical issues (missing try/catch for add-comment and direct DOM listeners) have been fixed.
The only notable gap is that you rely solely on TypeScript interfaces instead of defining propTypes for components, which might conflict with checklist item about “describe objects in propTypes” [CHECKLIST ITEM #3]; if your mentor requires propTypes in addition to TS, you should add them (for example, using PostDetails.propTypes = { post: PropTypes.shape({ ... }) }). There are also a few optional style suggestions, like reducing multiple setState calls in a single handler by grouping related state into objects, but these are not blockers for this task. Overall, this is a solid, well-structured implementation that closely follows the task description—nice work.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.