First version#1462
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 several core requirements and checklist items are not yet implemented or are violated, so the task is functionally incomplete. The app has no selectedPost state or wiring from PostsList to App, the sidebar is always rendered with Sidebar--open, and PostDetails does not receive any post or comments data, so posts cannot actually be opened in the sidebar as required. All comment-related functionality is missing: there is no comments API integration, no comments loading/loader/error/empty states, no comment form component, no validation or UX rules, and no comment delete or add-with-retry logic. Direct DOM interaction via document.addEventListener in UserSelector breaks the “don’t interact with the DOM directly” rule [CHECKLIST ITEM #2], and async error handling is done with .then/.catch instead of try {..} catch as required [CHECKLIST ITEM #12]. There is also a failing Cypress test because posts are requested from the API even when no user is selected, which contradicts the specification that posts should only be loaded after a user is chosen.
To move forward, you need to:
- Add
selectedPoststate inApp, pass a handler intoPostsListso the “Open” button selects a post, and conditionally applySidebar--openonly when a post is selected. - Implement comments loading for the selected post (with loader,
CommentsError, andNoComments), and create a separateCommentFormcomponent with required fields, validation (show errors only after submit, clear on change),is-loadingsubmit, and logic to append the new comment to the end of the list. - Implement comment deletion with optimistic UI removal and a way to retry on add/delete errors, wrap your async flows in
try/catch, and refactorUserSelectorto avoid directdocumentevent listeners (use React state and events instead). - Guard the posts-fetching effect so it only runs when there is a selected user (e.g.,
if (!selectedUserId) return;) to satisfy the failing Cypress test.
You already have a solid base for users and posts loading; once you add the selection, sidebar, comments, and checklist-compliant error handling, your solution will align much more closely with the full task 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
| <div className="block"> | ||
| <UserSelector /> | ||
| </div> | ||
| export const App = () => { |
There was a problem hiding this comment.
The App component currently manages only users and posts; there is no state or logic for a selected post, no handling for comments, and no integration with a comment form or deletion. This misses multiple description requirements (sidebar opens when a post is selected, load comments, show CommentsError/NoComments, write-comment flow, comment add/delete, and retry on errors). You need to introduce state for selectedPost, load comments via API when it changes, and wire up a PostDetails/comment form component to satisfy those features.
| useEffect(() => { | ||
| if (!selectedUser) { | ||
| return; | ||
| } | ||
|
|
||
| <div | ||
| className="notification is-danger" | ||
| data-cy="PostsLoadingError" | ||
| > | ||
| Something went wrong! | ||
| </div> | ||
| setPosts([]); | ||
| setHasError(false); | ||
| setIsLoading(true); | ||
|
|
||
| getUserPosts(selectedUser.id) | ||
| .then(setPosts) | ||
| .catch(() => setHasError(true)) | ||
| .finally(() => setIsLoading(false)); |
There was a problem hiding this comment.
Posts are loaded using .then/.catch without try {..} catch. This violates checklist item: * use try {..} catch for error handling. Consider refactoring the effect to an async function with try/catch where you set loading and error state.
| <div | ||
| data-cy="Sidebar" | ||
| className={classNames( | ||
| 'tile', | ||
| 'is-parent', | ||
| 'is-8-desktop', | ||
| 'Sidebar', | ||
| 'Sidebar--open', |
There was a problem hiding this comment.
The sidebar className always includes 'Sidebar--open', regardless of whether a post is selected. The description requires: "Add the Sidebar--open class to the sidebar when a post is selected". You should conditionally add this class based on whether there is a selected post (e.g., via classnames and state).
| <div className="tile is-child box is-success "> | ||
| <PostDetails /> |
There was a problem hiding this comment.
The PostDetails component is rendered without any props or context from App about the selected post or its comments. According to the task, post details must appear immediately when a post is selected, comments should be loaded, and comment add/delete handled. You need to connect PostDetails (or whichever component owns comments) to App via props or shared state so it can receive the selected post and comment-related callbacks/data.
| setIsLoading(true); | ||
|
|
||
| getUserPosts(selectedUser.id) | ||
| .then(setPosts) | ||
| .catch(() => setHasError(true)) | ||
| .finally(() => setIsLoading(false)); | ||
| }, [selectedUser]); |
There was a problem hiding this comment.
The "Open" button in PostsList does not trigger any selection of a post or sidebar behavior; it has no onClick handler. The description requires that each post can be opened in the sidebar with its comments, and that selecting a post adds Sidebar--open and shows post details. You need to pass a callback prop from App into PostsList and then call it from this button to set the selected post in state.
| <table className="table is-fullwidth is-striped is-hoverable is-narrow"> | ||
| <thead> | ||
| <tr className="has-background-link-light"> | ||
| <th>#</th> |
There was a problem hiding this comment.
The app currently never tracks a selected post in state and therefore cannot open a specific post in the sidebar. The task requires that selecting a post opens it in the sidebar with its comments; consider adding selectedPost state and passing an onPostSelect callback down to PostsList.
| {posts.map(post => ( | ||
| <tr data-cy="Post" key={post.id}> | ||
| <td data-cy="PostId">{post.id}</td> | ||
|
|
||
| <td data-cy="PostTitle">{post.title}</td> | ||
|
|
||
| <td className="has-text-right is-vcentered"> | ||
| <button | ||
| type="button" | ||
| data-cy="PostButton" |
There was a problem hiding this comment.
Error handling for posts uses catch on the promise chain but not a try {..} catch block as required by checklist item: "* use try {..} catch for error handling". Consider wrapping your async logic in an async function with try/catch or otherwise comply with that pattern here.
| type Props = { | ||
| posts: Post[]; |
There was a problem hiding this comment.
Posts loading uses the API client correctly, but there is no handling here for add/delete comment operations or retry logic. The description’s bullet "(*) Handle Add and Delete errors so the user can retry" is not implemented anywhere yet.
| <button | ||
| type="button" | ||
| data-cy="PostButton" | ||
| className="button is-link is-light" | ||
| > | ||
| Open |
There was a problem hiding this comment.
The posts list’s "Open" button does not call any handler to select a post. This violates the functional requirement that a post can be opened in the sidebar with its comments. Pass a callback via props (e.g. onPostOpen(post)) so clicking this button updates App’s selectedPost.
| useEffect(() => { | ||
| const handleOutsideClick = (event: MouseEvent) => { | ||
| if ( | ||
| dropdownRef.current && | ||
| !dropdownRef.current.contains(event.target as Node) | ||
| ) { | ||
| setIsExpanded(false); | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener('click', handleOutsideClick); |
There was a problem hiding this comment.
This document.addEventListener + contains pattern is direct DOM interaction, which violates the checklist item: "don't interact with DOM directly, use React as much as possible;" Consider using purely React-driven state changes (e.g., close on option select, or on blur/focus) or a higher-level abstraction for outside-click handling instead of manually wiring global listeners.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your solution is approved because it meets the functional requirements and the explicit checklist items without any remaining critical blockers. The app correctly loads users on mount, lets the user select a person via the React‑driven UserSelector dropdown, and only then fetches that user’s posts into the table with proper loader, error, and “No posts yet” states. Selecting a post opens the sidebar with Sidebar--open, shows the post details immediately, and manages comments per post, including loading from the API with loader, error notification with retry, and a “No comments yet” message when appropriate. The NewCommentForm enforces non‑empty name, email, and body using .trim(), shows validation errors only after submit, clears field‑specific errors on change, preserves name/email while clearing body after a successful submit, and exposes add‑errors so the user can retry, which satisfies the “no spaces only” and error‑handling requirements. Comment deletion is implemented optimistically with proper rollback and error display on failure, and all async flows use async/await with try/catch instead of .then/.catch, while classnames is used appropriately and keys are based on stable IDs. Overall, there are no remaining violations of the required checklist items such as avoiding direct DOM manipulation, avoiding isLoad naming, and not generating keys on render, so the implementation aligns well with the task expectations.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
No description provided.