react_dynamic-list-of-posts#1448
Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
This implementation requires changes due to three critical blockers. First, whitespace-only input bypasses validation across multiple files (NewCommentForm.tsx, App.tsx, PostDetails.tsx, PostsList.tsx) — form validation checks authorName.trim() but error states use !authorName (without trim), allowing users to submit comments with only spaces while error messages never appear. Fix by using !authorName.trim() consistently for all error state checks [CHECKLIST ITEM #1]. Second, delete comment operation has no error handling — if the API call fails after the optimistic UI update removes the comment, the user has no indication of failure and no way to retry. Wrap deleteComment in try/catch and restore the comment on failure [TASK #27/28]. Third, the WriteCommentButton is covered by another element (z-index/positioning bug) — the Cypress test should hide WriteCommentButton after clicking it fails because <main class="section"> covers the button, preventing interaction. This is a legitimate UI bug blocking core functionality. Additionally, fix naming convention violations in UserSelector.tsx where handleselectUser and handledropdownToggle should be handleSelectUser and handleDropdownToggle [CHECKLIST ITEM #6], and remove the unused commented import on line 16 of App.tsx [CHECKLIST ITEM #5].
✨ 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
|
|
||
| export const deleteComment = (id: number) => { | ||
| return client.delete(`/comments/${id}`); | ||
| }; |
There was a problem hiding this comment.
Unnecessary commented import should be removed - checklist item #5 requires removing unused comments
| @media (min-width: 769px) { | ||
| max-width: 50%; | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove this unused commented import. Checklist item #5 requires removing unused comments from code.
| const hasSelectedUser = selectedUserId !== null; | ||
| const isReady = hasSelectedUser && !postsLoading && !error; | ||
|
|
||
| <div className="notification is-warning" data-cy="NoPostsYet"> | ||
| No posts yet | ||
| const hasNoPosts = isReady && posts.length === 0; | ||
|
|
||
| const shouldShowPost = isReady && posts.length > 0; | ||
|
|
||
| return ( | ||
| <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 |
There was a problem hiding this comment.
Checklist item #1 violation: The validation checks authorName.trim() on line 57 to determine if the form is valid, but sets error state using !authorName (without trim) on lines 60, 63, 65. This allows users to add comments with whitespace-only values while error messages won't appear. Use the same trimmed check for setting error states.
| <UserSelector /> | ||
| </div> | ||
| import { getUsers } from './api/users'; | ||
| //import { getPosts } from './api/posts';// |
There was a problem hiding this comment.
Checklist item #5 violation: Remove unused code. This commented import is not being used in the file.
| const isFormValid = | ||
| authorName.trim() && authorEmail.trim() && commentBody.trim(); | ||
|
|
||
| setHasNameError(!authorName); | ||
| setHasEmailError(!authorEmail); |
There was a problem hiding this comment.
Checklist item #1 violation: Form validation checks authorName.trim() on line 48 for isFormValid, but error states are set using !authorName (without trim) on lines 50-52. This means whitespace-only input like ' ' will return early without showing error messages. Use the same trimmed check for setting error states: setHasNameError(!authorName.trim()).
|
|
||
| setComments([]); | ||
| setCommentsError(''); | ||
| setCommentsLoading(true); |
There was a problem hiding this comment.
Multiple setState calls in one function body. Lines 23-26 each call setState separately. Consider batching these into a single state update or using a reducer to improve code clarity (checklist item #10).
| </td> | ||
| </tr> | ||
| ); | ||
| })} | ||
| </tbody> |
There was a problem hiding this comment.
Checklist item #1 violation: The form uses trimmed values (authorName.trim()) for validation on line 48, but sets error states using untrimmed values (!authorName, !authorEmail, !commentBody) on lines 50-52. This allows users to submit whitespace-only input like ' ' without seeing error messages. Use consistent trimmed checks for both validation and error states.
| data-cy="PostButton" | ||
| className={classNames('button is-link', { | ||
| 'is-light': !isSelected, | ||
| })} | ||
| onClick={() => onPostSelected(post)} |
There was a problem hiding this comment.
Multiple setState calls in one function body (lines 40-44). Consider batching these into a single state update or using a reducer for better code clarity per checklist item #10.
| const [isDropdownActive, setIsDropdownActive] = useState(false); | ||
| const selectedUser = users.find(user => user.id === selectedUserId); | ||
| const dropdownRef = useRef<HTMLDivElement>(null); | ||
|
|
There was a problem hiding this comment.
Checklist item #6 violation: Function name should be handleSelectUser (camelCase with uppercase first letter for event handlers, following React naming conventions).
| onUserSelected(user.id); | ||
| setIsDropdownActive(false); | ||
| }; | ||
|
|
There was a problem hiding this comment.
Checklist item #6 violation: Function name should be handleDropdownToggle (camelCase with uppercase first letter for event handlers, following React naming conventions).
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Changes are needed due to a critical CSS stacking context issue that's blocking user interaction. The .Sidebar class in App.scss uses overflow: hidden but lacks position: relative to establish a proper stacking context, causing the WriteCommentButton to be covered by the <main class="section"> element. This is confirmed by the Cypress test failure at line 760. To fix this, add position: relative; z-index: 10; to .Sidebar and z-index: 20; to .Sidebar--open to ensure the sidebar content appears above the main section element. This is a blocker since the button cannot be clicked in the UI.
✨ 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
| import classNames from 'classnames'; | ||
| import React, { useState } from 'react'; | ||
| import { CommentData } from '../types/Comment'; | ||
|
|
There was a problem hiding this comment.
The .Sidebar class needs position: relative to establish a proper stacking context. Without it, the overflow: hidden won't create a reliable stacking context in all browsers, and z-index won't work correctly. Add position: relative; z-index: 10; to .Sidebar and z-index: 20; to .Sidebar--open to ensure the sidebar content appears above the <main class="section"> element.
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| import { client } from '../utils/fetchClient'; | ||
|
|
||
| export const getUsers = () => { | ||
| return client.get<User[]>('/users'); |
There was a problem hiding this comment.
Missing App.scss file - The critical z-index stacking fix mentioned in the previous review cannot be verified. The .Sidebar needs 'position: relative; z-index: 10;' and .Sidebar--open needs 'z-index: 20;' to prevent the sidebar content from being covered by the main section element.
| import React, { useState, useEffect, useRef } from 'react'; | ||
| import { User } from '../types/User'; |
There was a problem hiding this comment.
Unused import: NewComment is imported on line 2 but never used. The addComment function in this file receives a NewComment parameter typed in its signature, so there's no need for this import.
|
|
||
| getComments(selectedPost.id) | ||
| .then(newComments => { | ||
| setComments(newComments); | ||
| }) | ||
| .catch(() => setCommentsError('Something went wrong')) | ||
| .finally(() => { | ||
| setCommentsLoading(false); | ||
| }); | ||
| }, [selectedPost]); | ||
|
|
||
| if (!selectedPost) { | ||
| return null; | ||
| } | ||
|
|
||
| const handleAddComment = async (data: CommentData) => { |
There was a problem hiding this comment.
This useEffect is missing a dependency array. Without [] as the second argument, the effect runs after every render, causing the document click listener to be repeatedly attached and removed. Add [] to run only once on mount, which matches the typical click-outside pattern.
No description provided.