Develop #1219

DmytroHoncharuk · 2025-01-13T20:15:04Z

No description provided.

mateacademy-ai-mentor

Thank you for your hard work on this project! While your code is well-structured and meets many of the task requirements, there are a few critical issues that need to be addressed before we can approve the pull request:

Error Handling: The fetchUsers function lacks error handling, which is crucial for robustness. Please ensure that errors are properly managed, possibly by setting an error state or logging them.
Unused State: The postError state is initialized but not utilized. Consider integrating it into your error handling logic or removing it if it's unnecessary.
Prop Type Handling: The users prop is typed as User[] | null, but the component assumes it can always be mapped over. Please handle the null case to prevent potential runtime errors.

These issues impact the functionality and robustness of the application. Once addressed, your code will be in great shape! Keep up the great work, and don't hesitate to reach out if you need any help. 😊

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

🔄 Fix the issues, commit the changes, and then re-request my review using the Re-request review button in the Reviewers tab.

mateacademy-ai-mentor · 2025-01-13T20:17:28Z

src/App.tsx

-              <p data-cy="NoSelectedUser">No user selected</p>
+  const [chosenUser, setChosenUser] = React.useState<User | null>(null);
+  // eslint-disable-next-line @typescript-eslint/no-unused-vars
+  const [postError, setPostError] = React.useState(false);


The postError state is initialized but not used in the component logic. Consider using it to handle errors related to fetching posts or remove it if it's unnecessary.

mateacademy-ai-mentor · 2025-01-13T20:17:28Z

src/App.tsx

+      } catch (err) {
+      } finally {
+      }


The error handling block in the fetchUsers function is empty. Consider adding logic to handle errors, such as setting an error state or logging the error, to improve the robustness of the application.

mateacademy-ai-mentor · 2025-01-13T20:17:28Z

src/components/NewCommentForm.tsx

+    }
+
+    if (!email.trim() || !/^[\w.%+-]+@[\w.-]+\.[a-zA-Z]{2,}$/.test(email)) {
+      newErrors.email = 'Email is required';


The error message for email validation states 'Email is required', but the validation logic also checks for a valid email format. Consider updating the message to reflect both conditions, such as 'Valid email is required'.

mateacademy-ai-mentor · 2025-01-13T20:17:29Z

src/components/NewCommentForm.tsx

+      newErrors.name = 'Name is required';
+    }
+
+    if (!email.trim() || !/^[\w.%+-]+@[\w.-]+\.[a-zA-Z]{2,}$/.test(email)) {


The email validation regex is quite basic and might not cover all valid email formats. Consider using a more comprehensive regex for email validation to ensure better accuracy.

mateacademy-ai-mentor · 2025-01-13T20:17:29Z

src/components/PostDetails.tsx

+  const [error, setError] = useState<string | null>(null);
+  const [commentsFromServer, setCommentsFromServer] = useState<Comment[]>([]);
+  const [isFormVisible, setIsFormVisible] = useState<boolean>(false);
+  const [currentPostId, setCurrentPostId] = useState<number | null>(null);


The currentPostId state appears to be redundant since it mirrors the post?.id prop. Consider removing it and directly using post?.id to simplify the logic.

mateacademy-ai-mentor · 2025-01-13T20:17:29Z

src/components/PostDetails.tsx

+    try {
+      await deleteComment(commentId);
+    } catch {
+      setError('Failed to delete comment. Please try again.');


The error message for failed comment deletion is generic. Consider including more specific information, such as the comment ID or reason for failure, to provide better feedback to the user.

mateacademy-ai-mentor · 2025-01-13T20:17:29Z

src/components/PostsList.tsx

+  function handleClickToChoosePost(post: Post) {
+    if (!activePost) {
+      choosePost(post);
+    }

-    <table className="table is-fullwidth is-striped is-hoverable is-narrow">
-      <thead>
-        <tr className="has-background-link-light">
-          <th>#</th>
-          <th>Title</th>
-          {/* eslint-disable-next-line jsx-a11y/control-has-associated-label */}
-          <th> </th>
-        </tr>
-      </thead>
+    if (activePost?.id === post.id) {
+      choosePost(null);
+    }

-      <tbody>
-        <tr data-cy="Post">
-          <td data-cy="PostId">17</td>
+    if (activePost?.id !== post.id) {
+      choosePost(post);
+    }


The handleClickToChoosePost function can be simplified. The current logic checks three conditions, but it can be reduced to two: if activePost?.id === post.id, set choosePost(null); otherwise, set choosePost(post). This will make the function more concise and easier to read.

mateacademy-ai-mentor · 2025-01-13T20:17:29Z

src/components/UserSelector.tsx

+          {users?.map(user => (
+            <a
+              key={user.id}
+              href={`#${user.id}`}
+              className={classNames('dropdown-item', {
+                'is-active': user.id === chosenUser?.id,
+              })}
+              onClick={() => {
+                setIsActive(false);
+                chooseUser(user);
+              }}
+            >
+              {user.name}
+            </a>
+          ))}


The users prop is typed as User[] | null, but the component maps over users without checking for null. Consider providing a default value or handling the null case to prevent potential runtime errors.

mateacademy-ai-mentor

Thank you for your hard work on this Pull Request! 🎉 The code has improved significantly, addressing many of the previous concerns. However, there are still a few areas to enhance:

Unused State: The postError state should be utilized effectively to display an error message or take necessary actions when a post fetch error occurs.
Email Validation Message: Update the email validation message to 'Valid email is required' to cover both missing and invalid formats.
Error Message Specificity: Provide a more specific error message for failed comment loads, such as 'Failed to load comments. Please try again later.'
Prop Type Handling: Ensure the users prop is always an array to prevent runtime errors.
Redundant Conditional Checks: Simplify the logic in the handleClickToChoosePost function for better readability.

These improvements will enhance the robustness and user experience of your application. Keep up the great work, and continue learning from these insights! 🚀

✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨

mateacademy-ai-mentor · 2025-01-13T20:29:54Z

src/App.tsx

-            <div className="block" data-cy="MainContent">
-              <p data-cy="NoSelectedUser">No user selected</p>
+  const [chosenUser, setChosenUser] = React.useState<User | null>(null);
+  const [postError, setPostError] = React.useState(false);


The postError state is set to true when an error occurs while fetching posts, but it is not utilized effectively in the component's logic. Ensure that this state is used to display an appropriate error message or take necessary actions when a post fetch error occurs.

mateacademy-ai-mentor · 2025-01-13T20:29:54Z

src/components/NewCommentForm.tsx

+    }
+
+    if (!email.trim() || !/^[\w.%+-]+@[\w.-]+\.[a-zA-Z]{2,}$/.test(email)) {
+      newErrors.email = 'Email is required';


The error message for email validation is misleading. It currently states 'Email is required' even when the email format is incorrect. Consider updating the message to something like 'Valid email is required' to cover both missing and invalid email formats.

mateacademy-ai-mentor · 2025-01-13T20:29:54Z

src/components/PostDetails.tsx

+          <Loader />
+        ) : error ? (
+          <div className="notification is-danger" data-cy="CommentsError">
+            Something went wrong


The error message 'Something went wrong' is too generic. Consider providing a more specific message, such as 'Failed to load comments. Please try again later.' to give users clearer feedback.

mateacademy-ai-mentor · 2025-01-13T20:29:54Z

src/components/PostsList.tsx

+  function handleClickToChoosePost(post: Post) {
+    if (!activePost) {
+      choosePost(post);
+    }

-    <table className="table is-fullwidth is-striped is-hoverable is-narrow">
-      <thead>
-        <tr className="has-background-link-light">
-          <th>#</th>
-          <th>Title</th>
-          {/* eslint-disable-next-line jsx-a11y/control-has-associated-label */}
-          <th> </th>
-        </tr>
-      </thead>
+    if (activePost?.id === post.id) {
+      choosePost(null);
+    }

-      <tbody>
-        <tr data-cy="Post">
-          <td data-cy="PostId">17</td>
+    if (activePost?.id !== post.id) {
+      choosePost(post);
+    }


The handleClickToChoosePost function contains redundant conditional checks. You can simplify the logic by combining the conditions into a single check. For example, you can directly toggle the post selection based on whether the clicked post is the active one or not.

mateacademy-ai-mentor · 2025-01-13T20:29:54Z

src/components/UserSelector.tsx

+import classNames from 'classnames';
+
+type Props = {
+  users: User[] | null;


The users prop is typed as User[] | null, but the component assumes it can always be mapped over. Consider providing an empty array instead of null to prevent potential runtime errors. This can be done by ensuring the parent component passes an empty array when there are no users.

DmytroHoncharuk added 3 commits January 13, 2025 19:48

almost done

929aa09

implemented add and delete of comments

50d2048

solution

3d5c58e

mateacademy-ai-mentor suggested changes Jan 13, 2025

View reviewed changes

fix bugs

06fba62

DmytroHoncharuk requested a review from mateacademy-ai-mentor January 13, 2025 20:27

mateacademy-ai-mentor approved these changes Jan 13, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Develop #1219

Develop #1219

DmytroHoncharuk commented Jan 13, 2025

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Jan 13, 2025

mateacademy-ai-mentor Jan 13, 2025

mateacademy-ai-mentor Jan 13, 2025

mateacademy-ai-mentor Jan 13, 2025

mateacademy-ai-mentor Jan 13, 2025

mateacademy-ai-mentor Jan 13, 2025

mateacademy-ai-mentor Jan 13, 2025

mateacademy-ai-mentor Jan 13, 2025

mateacademy-ai-mentor left a comment

mateacademy-ai-mentor Jan 13, 2025

mateacademy-ai-mentor Jan 13, 2025

mateacademy-ai-mentor Jan 13, 2025

mateacademy-ai-mentor Jan 13, 2025

mateacademy-ai-mentor Jan 13, 2025

Develop #1219

Are you sure you want to change the base?

Develop #1219

Conversation

DmytroHoncharuk commented Jan 13, 2025

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment