feat(react): dynamic-list-of-posts#1465
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because all core functional requirements are met: users and posts load correctly, the sidebar opens and shows post details immediately, comments are fetched with proper loading/error/empty states, and the comment form behaves as specified (validation with trimming, error handling on submit, keeping name/email, clearing body, and optimistic deletion). There are no issues that would block task completion or break the main flows.
The main issues left are non-blocking checklist mismatches: UserSelector uses document.addEventListener('click', ...) for outside-click handling (violating the "don’t interact with the DOM directly" guideline [CHECKLIST ITEM #2]), and components rely only on TypeScript types without propTypes for object props [CHECKLIST ITEM #3]. You also have some lingering ESLint-disable comments and unused imports that could be cleaned up, and your error handling for comment add/delete relies on .catch without surfacing retry options, even though the description suggests optional retry handling.
Going forward, I recommend refactoring the outside-click behavior to a React-based approach (e.g., a higher-level click handler or an outside-click hook), adding propTypes definitions for components receiving objects, and introducing some visible error state for failed createComment/deleteComment calls so users can retry when needed. Despite these improvements being possible, your current implementation is functionally solid and aligns well with the task, so you can confidently move on while keeping these refinements in mind for future iterations.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| > | ||
| Something went wrong! | ||
| </div> | ||
| setPostsLoading(true); |
There was a problem hiding this comment.
You’re calling setPostsLoading, setPostsError, setPosts, and setSelectedPost sequentially in the same effect. While this works, checklist item #8 recommends not using setState several times in one function for clarity; consider consolidating related state changes or using a single state object if appropriate.
|
|
||
| <div className="notification is-warning" data-cy="NoPostsYet"> | ||
| No posts yet | ||
| getUserPosts(selectedUser.id) |
There was a problem hiding this comment.
The promise chain here handles errors via .catch, but checklist item #9 suggests using try {..} catch for error handling. Not mandatory, but you might refactor to async/await with try/catch for clearer error handling semantics.
| if (!selectedUser) { | ||
| return; | ||
| } | ||
|
|
||
| <div | ||
| className="notification is-danger" | ||
| data-cy="PostsLoadingError" | ||
| > | ||
| Something went wrong! | ||
| </div> | ||
| setPostsLoading(true); |
There was a problem hiding this comment.
Your validation trims name, email, and body before checking emptiness, which correctly satisfies checklist item #1 (prevent adding entries with spaces only). If you want to align with the optional recommendation in checklist item #8, you could reduce the number of setState calls here by grouping them, but it’s not required.
| selectedUser && !postsLoading && !postsError && posts.length > 0; | ||
|
|
||
| return ( | ||
| <main className="section"> |
There was a problem hiding this comment.
The submission handler doesn’t use try/catch; instead, it relies on .then/.finally. Checklist item #9 recommends try {..} catch for error handling (optional). To allow user retries and show add-error feedback (optional requirement in the description), consider capturing and surfacing errors from createComment.
| setPostsError(false); | ||
| setPosts([]); | ||
| setSelectedPost(null); |
There was a problem hiding this comment.
You optimistically remove the comment and then call deleteComment. This matches the requirement to delete immediately for UX, but you don’t handle deletion errors (the description’s optional "Handle Add and Delete errors so the user can retry"). Consider adding error handling so a failed delete can be retried or rolled back.
| createComment(commentData) | ||
| .then(newComment => { | ||
| onAddComment(newComment); | ||
| setBody(''); | ||
| }) | ||
| .finally(() => { | ||
| setIsSubmitting(false); | ||
| }); |
There was a problem hiding this comment.
In handleSubmit, you handle errors via promise chaining but do not expose any retry mechanism or user-visible error state for failed createComment calls, while the description mentions handling Add/Delete errors so the user can retry; consider tracking an error state and rendering a notification or retry UI.
| setHasEmailError(emailError); | ||
| setHasBodyError(bodyError); | ||
|
|
||
| if (nameError || emailError || bodyError) { |
There was a problem hiding this comment.
You optimistically delete comments and call deleteComment(commentId) but do not handle a failed delete or allow retry, whereas the description suggests handling Add/Delete errors so the user can retry; consider adding error state and UI to inform the user and possibly restore the comment on failure.
|
|
||
| const trimmedName = name.trim(); | ||
| const trimmedEmail = email.trim(); | ||
| const trimmedBody = body.trim(); |
There was a problem hiding this comment.
Checklist item recommends using try {..} catch for error handling; here you rely solely on .catch in chained promises for loading comments—while functionally sufficient, you might consider async/await with try/catch for clearer error handling, especially if you extend this logic.
|
|
||
| import { Comment } from '../types/Comment'; | ||
| import { Post } from '../types/Post'; | ||
| import { deleteComment, getPostComments } from '../api'; |
There was a problem hiding this comment.
There is an unused import User here that is never referenced in this file. This goes against checklist guidance about keeping code clean and can be safely removed.
| 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 direct use of document.addEventListener('click', ...) to handle outside clicks violates checklist item: "don't interact with DOM directly, use React as much as possible". Consider a React-based approach (e.g., handling clicks via a higher-level component or using a library/outside-click hook) instead of attaching a global document listener manually.
DEMO LINK