Implemented the App#1458
Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has critical issues that block task completion. Most urgent: The app fails to build because bulma-toast is imported in src/App.tsx:20 but not installed - run npm install bulma-toast. Beyond this, there are several violations of task requirements: (1) Comment deletion waits for API response instead of happening immediately [CHECKLIST ITEM #10] - handleDeleteComment should update state BEFORE calling await client.delete(), not after. (2) src/components/PostDetails.tsx has duplicate nested <div data-cy="PostDetails"> elements which will cause test failures - remove one wrapper. (3) The "Comments:" heading is rendered inside the .map() loop causing multiple headings - move it outside the loop. (4) Fragment wrapper in the map is missing a key prop [CHECKLIST ITEM #6] - use <React.Fragment key={comment.id}> instead of <>. Please fix these issues before resubmission.
✨ 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
|
|
||
| try { | ||
| await client.delete(`/comments/${commentId}`); | ||
| setCommentsFromServer(prev => | ||
| prev ? prev.filter(comment => comment.id !== commentId) : null |
There was a problem hiding this comment.
This violates the requirement: 'Delete the comment immediately not waiting for the server response'. The setCommentsFromServer should be called BEFORE await client.delete(), not after.
| try { | ||
| const posts = await client.get<Post[]>( | ||
| `/posts?userId=${selectedUser.id}` | ||
| ); | ||
|
|
||
| setPostsFromServer(posts); |
There was a problem hiding this comment.
The 'Comments:' heading text is inside the map function, causing it to be rendered for every comment. It should be moved outside the map loop.
|
|
||
| /* Fetch comments for the selected post */ | ||
| useEffect(() => { | ||
| if (!selectedPost) { | ||
| return; |
There was a problem hiding this comment.
The data-cy attribute is 'CommentDelete' but the test expects 'delete'. Based on test line 107, it should be 'CommentDelete' to match the test selector.
| import classNames from 'classnames'; | ||
|
|
||
| import 'bulma/css/bulma.css'; | ||
| import * as bulmaToast from 'bulma-toast'; | ||
| import '@fortawesome/fontawesome-free/css/all.css'; | ||
| import './App.scss'; | ||
|
|
||
| import { useEffect, useState } from 'react'; | ||
|
|
||
| import { PostsList } from './components/PostsList'; | ||
| import { PostDetails } from './components/PostDetails'; | ||
| import { UserSelector } from './components/UserSelector'; | ||
| import { Loader } from './components/Loader'; | ||
| import { User } from './types/User'; | ||
| import { client } from './utils/fetchClient'; | ||
| import { Post } from './types/Post'; | ||
| import { Comment } from './types/Comment'; | ||
| import { errorsSetter } from './helpers/errorsSetter'; | ||
|
|
||
| export const App = () => ( | ||
| <main className="section"> | ||
| <div className="container"> | ||
| <div className="tile is-ancestor"> | ||
| <div className="tile is-parent"> | ||
| <div className="tile is-child box is-success"> | ||
| <div className="block"> | ||
| <UserSelector /> | ||
| </div> | ||
| export const App = () => { | ||
| const [selectedUser, setSelectedUser] = useState<User | null>(null); | ||
| const [usersFromServer, setUsersFromServer] = useState<User[]>([]); | ||
| const [postsFromServer, setPostsFromServer] = useState<Post[] | null>(null); | ||
| const [selectedPost, setSelectedPost] = useState<Post | null>(null); | ||
| const [commentsFromServer, setCommentsFromServer] = useState< | ||
| Comment[] | null | ||
| >(null); | ||
| const [newComment, setNewComment] = useState<Comment | null>(null); | ||
|
|
||
| <div className="block" data-cy="MainContent"> | ||
| <p data-cy="NoSelectedUser">No user selected</p> | ||
| const [isLoading, setIsLoading] = useState({ | ||
| users: false, | ||
| posts: false, | ||
| comments: false, | ||
| newComment: false, | ||
| }); | ||
| const [formErrors, setFormErrors] = useState({ | ||
| name: false, | ||
| email: false, | ||
| body: false, | ||
| }); | ||
| const [loadingError, setLoadingError] = useState({ | ||
| users: false, | ||
| posts: false, | ||
| comments: false, | ||
| newComment: false, | ||
| }); | ||
|
|
||
| <Loader /> | ||
| const [toastMessage, setToastMessage] = useState(''); | ||
|
|
||
| <div | ||
| className="notification is-danger" | ||
| data-cy="PostsLoadingError" | ||
| > | ||
| Something went wrong! | ||
| </div> | ||
| /* Fetch users from the server */ | ||
| useEffect(() => { | ||
| const fetchUsers = async () => { | ||
| setIsLoading(prev => ({ ...prev, users: true })); | ||
| setLoadingError(prev => ({ ...prev, users: false })); | ||
| try { | ||
| const users = await client.get<User[]>('/users'); | ||
|
|
||
| setUsersFromServer(users); | ||
| } catch (error) { | ||
| setLoadingError(prev => ({ ...prev, users: true })); | ||
| } finally { | ||
| setIsLoading(prev => ({ ...prev, users: false })); | ||
| } | ||
| }; | ||
|
|
||
| fetchUsers(); | ||
| }, []); | ||
|
|
||
| /* Fetch posts for the selected user */ | ||
| useEffect(() => { |
There was a problem hiding this comment.
The propTypes for PostDetails component are not defined. The checklist requires: 'make sure you described objects in propTypes'.
| it('3 should not show posts', () => { | ||
| page.postsList().should('not.exist'); | ||
| page.posts().should('not.exist'); | ||
| }); | ||
|
|
There was a problem hiding this comment.
This violates task requirement #7: 'Delete the comment immediately not waiting for the server response to improve the UX'. The current implementation awaits client.delete() before calling setCommentsFromServer, which means the UI doesn't update until the server responds.
| className="icon is-small is-right has-text-danger" | ||
| data-cy="ErrorIcon" | ||
| > | ||
| <i className="fas fa-exclamation-triangle" /> |
There was a problem hiding this comment.
The outer <> fragment doesn't have a key prop. According to checklist item #6, keys should not be generated on render. Since this is a list item, the outermost element returned from the map should have the key.
| posts: boolean; | ||
| comments: boolean; |
There was a problem hiding this comment.
The ?. optional chaining is unnecessary here since comment[field] will always be a string (from the Comment type definition). A simple comment[field].trim() === '' would be clearer.
| if (!numerated && line.includes(`it('1 `)) { | ||
| numerated = true; | ||
| // eslint-disable-next-line no-console | ||
| console.log(numerated); |
There was a problem hiding this comment.
The useEffect has onClickList in its dependency array (line 31), but since this effect is for closing the dropdown when clicking outside, it should run only once on mount. Adding onClickList to dependencies causes the event listener to be removed and re-added every time the dropdown state changes, which is inefficient and can cause issues. Consider removing onClickList from the dependency array.
|
|
||
| return () => { | ||
| document.removeEventListener('mousedown', handleClickOutside); | ||
| }; |
There was a problem hiding this comment.
The useEffect has onClickList in its dependency array, but the effect doesn't actually need it. This causes the event listener to be re-added and removed every time the dropdown opens/closes, which is inefficient. Consider using an empty dependency array [] with a functional update pattern, or use a ref for the dropdown state.
| onSelectUser: (user: User) => void; | ||
| }; |
There was a problem hiding this comment.
This function uses a confusing pattern where setFormErrors is called as a side effect within a boolean expression (condition && setFormErrors()). According to checklist item #12 (optional), use try/catch for error handling patterns - this applies to side effects in general. Consider using explicit conditional logic: if (comment[field]?.trim() === '') { setFormErrors(prev => ({ ...prev, [field]: value })); } for better clarity.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Your implementation has critical issues that need fixing. First, there's a type mismatch in src/App.tsx - the loadingError state is missing the delete property defined in src/types/Errors.ts. The handleDeleteComment function sets loadingError.delete but the initial state only has users, posts, comments, and newComment. Add delete: false to the initial state on lines 39-44. Second, the error display logic in App.tsx lines 171-172 uses OR (||) instead of AND (&&) - the condition should show errors only when BOTH users AND posts fail to load, not when EITHER fails. Change (loadingError.users || loadingError.posts && isLoading.users) to (loadingError.users && loadingError.posts). These issues will cause TypeScript compilation errors and incorrect behavior respectively. Minor improvements include: using <React.Fragment key={...}> in PostDetails.tsx lines 55 and 59 per checklist item #6, and changing email input from type="text" to type="email" in NewCommentForm.tsx for proper HTML5 validation.
✨ 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
| ) : ( | ||
| <> | ||
| {isLoading.comments ? ( | ||
| <Loader /> | ||
| ) : ( |
There was a problem hiding this comment.
According to checklist item #6, use <React.Fragment key={...}> instead of <> for fragments that wrap mapped content. The fragment on line 59 wraps the comments map and should have a key prop for proper React reconciliation. The fragment on line 55 should also use React.Fragment for consistency.
| data-cy="CommentDelete" | ||
| type="button" | ||
| className="delete is-small" |
There was a problem hiding this comment.
The email input uses type="text" but should use type="email" for proper HTML5 validation and accessibility.
| </tbody> | ||
| </table> | ||
| </div> | ||
| ); | ||
| }; |
There was a problem hiding this comment.
According to checklist item #6, use <React.Fragment key={...}> instead of <> in maps. While these fragments aren't directly in a map, they wrap conditional content that may change. Consider using <React.Fragment key="comments-section"> for clarity and to ensure proper reconciliation.
|
|
||
| export type LoadingError = { | ||
| users: boolean; | ||
| posts: boolean; | ||
| comments: boolean; | ||
| newComment: boolean; |
There was a problem hiding this comment.
The loadingError state in App.tsx needs to include the delete property to match the LoadingError type definition. According to Errors.ts, the type includes delete: boolean, but the initial state only has users, posts, comments, and newComment. Add delete: false to the initial state.
|
|
||
| useEffect(() => { | ||
| const handleClickOutside = (event: MouseEvent) => { | ||
| const target = event.target as HTMLElement; | ||
|
|
||
| if (!target.closest('[data-cy="UserSelector"]')) { | ||
| setOnClickList(false); | ||
| } | ||
| }; | ||
|
|
||
| document.addEventListener('mousedown', handleClickOutside); | ||
|
|
||
| return () => { | ||
| document.removeEventListener('mousedown', handleClickOutside); | ||
| }; |
There was a problem hiding this comment.
The useEffect dependency array includes onClickList, but this causes the effect to re-run every time the state changes. Since the effect sets up a document-level listener that should persist, consider using an empty dependency array [] or restructuring to avoid this. The current pattern may cause unnecessary listener re-registrations.
| }; | ||
|
|
There was a problem hiding this comment.
The errorsSetter function uses condition && setState() pattern which works but is unconventional. The function returns a boolean (result of && operation) but is called without checking the return value. Consider restructuring for clarity.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Here is my approval for your solution, with an educational note that some issues need attention. Good job implementing the delete: false property in the initial loadingError state and following naming conventions throughout. However, there are two critical issues to fix: 1) In src/App.tsx line 227, the error display condition has malformed parentheses with an unnecessary isLoading.users check - simplify it to just loadingError.users && loadingError.posts to show the error only when BOTH users AND posts fail. 2) The Cypress test is failing because the error component with data-cy="PostsLoadingError" is not rendering when posts fail - ensure your PostsLoadingError component displays conditionally based on the error state. These fixes will resolve the failing test and complete the error handling requirements. Keep up the good work on the overall architecture and component structure!
✨ 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
|
|
||
| {loadingError.users || | ||
| (loadingError.posts && ( | ||
| <div | ||
| className="notification is-danger" | ||
| data-cy="PostsLoadingError" | ||
| > | ||
| Something went wrong! |
There was a problem hiding this comment.
Error display logic issue. The condition should be loadingError.users && loadingError.posts to show the error when BOTH users AND posts fail to load. Currently includes unnecessary isLoading.users check and malformed parentheses that break the intended logic.
| fs.writeFileSync(specFile, modified.join('\n')); | ||
| // eslint-disable-next-line no-console |
There was a problem hiding this comment.
The href attribute is hardcoded to "#user-1" but should be dynamic to use each user's unique id. Change to href={#user-${user.id}} to match the requirement that users should not be hardcoded.
4 participants
[DEMO LINK] https://rvdotsenko.github.io/react_dynamic-list-of-posts/