Skip to content

Develop#1442

Open
SerhiyShimko wants to merge 3 commits into
mate-academy:masterfrom
SerhiyShimko:develop
Open

Develop#1442
SerhiyShimko wants to merge 3 commits into
mate-academy:masterfrom
SerhiyShimko:develop

Conversation

@SerhiyShimko

Copy link
Copy Markdown

No description provided.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

This solution requires several critical fixes before approval. The most serious issue is that comment IDs are being reassigned locally in PostDetails.tsx (lines 24-35) using a counter instead of preserving the original API IDs. This breaks the delete functionality since the server uses different IDs than the locally-generated ones. Additionally, there are multiple error state management bugs: showError in App.tsx and shoowErrorLoading/shoowErrorName in PostDetails.tsx are never reset when switching users or posts, causing stale errors to persist incorrectly. The form also violates the requirement by clearing name and email fields after successful submission when they should be preserved. Only the comment text should be cleared. Similarly, the NewCommentForm.tsx ignores the API response and constructs a local comment object with maxId + 1, which can cause ID mismatches with the server. To fix these issues: 1. Remove the ID remapping logic in PostDetails.tsx and use API IDs directly; 2. Reset all error states when the corresponding data loading is triggered (in the useEffect dependencies); 3. Preserve name and email state on successful form submission; 4. Use the server's returned comment data in NewCommentForm.tsx instead of constructing it locally; 5. Add proper try/catch error handling for add/delete operations as specified in the optional 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

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Comment thread src/App.tsx
Comment on lines +88 to +92
setChoosePost={setChoosePost}
choosePost={choosePost}
allPosts={allPosts}
/>
)}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The API response is ignored. The task requires using the comment from the API response: 'Add the new comment received as a response from the API to the end of the list'. Currently, a locally-generated ID and structure are used instead of the server response.

Comment thread src/App.tsx
Comment on lines +88 to +98
setChoosePost={setChoosePost}
choosePost={choosePost}
allPosts={allPosts}
/>
)}

{allPosts && allPosts.length === 0 && !showLoading && (
<div className="notification is-warning" data-cy="NoPostsYet">
No posts yet
</div>
)}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing error handling for the add comment API call. According to the checklist, use try {..} catch for error handling. Without this, if the API call fails, the loading state is never cleared and the user cannot retry.

Comment thread src/App.tsx Outdated
Comment on lines +24 to +35

<Loader />
// показувати при успіху або поилці
const [showLoading, setShowLoading] = useState(false);
const [showError, setShowError] = useState(false);

<div
className="notification is-danger"
data-cy="PostsLoadingError"
>
Something went wrong!
</div>
useEffect(() => {
FunctionCalls.getUsers()
.then(users => {
setAllUsers(users);
})
.catch(() => {})
.finally(() => {});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments are unnecessarily remapped with locally-generated IDs. The original comment IDs from the API should be used. The id variable is declared outside the map callback, which is a potential source of bugs. According to the checklist, don't generate keys on render.

Comment thread src/App.tsx
import { Loader } from './components/Loader';
import { useEffect, useState } from 'react';
import { User } from './types/User';
import * as FunctionCalls from './api/functionServerRequests';

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The showError state is set when comments fail to load, but it's never reset when switching to another post. This means the error will persist even after loading comments for a different post.

Comment thread src/App.tsx
Comment on lines +39 to +40
if (chooseUser) {
setShowLoading(true);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The showError state is set in the catch block but never reset when choosing a new post. The error from a previous post will incorrectly display for subsequent posts.

Comment on lines +88 to +93
className="message is-small"
data-cy="Comment"
>
<div className="message-header">
<a
href={`mailto:${comment.email}`}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addComment API call doesn't handle errors. Optional requirement (*) suggests implementing error handling so the user can retry if adding fails.

export const PostDetails: React.FC<Props> = ({ choosePost }) => {
const [shoowLoadingComments, setShoowLoadingComments] = useState(false);
const [shoowErrorLoading, setShoowErrorLoading] = useState(false);
const [shoowNewComment, setShoowNewComment] = useState(false);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PostDetails uses Comment type for allComments, but the API returns CommentData type. The addComment function also expects CommentData, but you're creating a Comment object. Check if the types are compatible or if there's a mismatch.

Comment thread src/components/NewCommentForm.tsx Outdated
Comment on lines +88 to +93
setShoowLoadingSubmit(true);
FunctionCalls.addComment(newComment)
.then(() => {
setAllComments(current =>
current ? [...current, newComment] : [newComment],
);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The addComment function returns CommentData[], but the .then() callback doesn't use the response. According to the task: 'Add the new comment received as a response from the API to the end of the list'. The server returns the created comment with its assigned ID, but the code discards this and creates a local object with maxId + 1. This could cause ID mismatches. Use the server's response instead: .then(newComment => setAllComments(current => current ? [...current, newComment] : [newComment])) and remove the locally constructed newComment object.

Comment on lines 41 to +47
<div className="dropdown-trigger">
<button
type="button"
className="button"
aria-haspopup="true"
aria-controls="dropdown-menu"
onClick={() => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The onClick handler calls setChoosePost twice when selecting a post (lines 45-46). Per the checklist, avoid using setState several times in one function call. Consider using a ternary directly: setChoosePost(choosePost?.id === post.id ? null : post)

Comment thread src/components/PostsList.tsx Outdated
Comment on lines +41 to +47
data-cy="PostButton"
onClick={() => {
if (choosePost?.id === post.id) {
setChoosePost(null);
} else {
setChoosePost(null);
setChoosePost(post);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lines 45-46 call setChoosePost twice in succession - first to null, then to the post. The checklist recommends not using setState several times in one function call. This could be simplified to a single call.

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

The request meets the required criteria for approval.


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

Footnotes

  1. Rate AI review example

Comment on lines +17 to +41
// input name
const [name, setName] = useState('');
const [shoowErrorName, setShoowErrorName] = useState(false);

// input Email
const [email, setEmail] = useState('');
const [shoowErrorEmail, setShoowErrorEmail] = useState(false);

// input Text
const [text, setText] = useState('');
const [shoowErrorText, setShoowErrorText] = useState(false);

const [shoowLoadingSubmit, setShoowLoadingSubmit] = useState(false);

function clearInput() {
setName('');
setShoowErrorName(false);
setEmail('');
setShoowErrorText(false);
setText('');
setShoowErrorEmail(false);
}

function detectionName() {
if (name === '') {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates checklist item #7: error states should be reset when the data loading is triggered. The shoowErrorLoading state is never reset when switching posts, so an error from one post will incorrectly persist when viewing another post's comments. Add setShoowErrorLoading(false) to the useEffect that depends on choosePost.

Comment on lines +30 to +37

function clearInput() {
setName('');
setShoowErrorName(false);
setEmail('');
setShoowErrorText(false);
setText('');
setShoowErrorEmail(false);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The clearInput function resets name and email but fails to reset shoowErrorEmail (it resets shoowErrorText instead of shoowErrorEmail). This means clicking the Clear button leaves the email error visible. Should reset shoowErrorEmail to false.

Comment on lines +43 to +48

return false;
}

return true;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Following the optional checklist item #9, the delete operation should use try/catch for proper error handling. Currently, the .then(() => {}) is an empty handler that silently ignores any errors, leaving the user unaware if the deletion failed.

Comment on lines +17 to +41

useEffect(() => {
setShoowNewComment(false);
setAllComments(null);
if (choosePost) {
setShoowLoadingComments(true);
FunctionCalls.getComments(choosePost.id)
.then(comments => {
const newComments = comments.map(com => ({
id: com.id,
postId: choosePost.id,
name: com.name,
email: com.email,
body: com.body,
}));

setAllComments(newComments);
})
.catch(() => {
setShoowErrorLoading(true);
})
.finally(() => {
setShoowLoadingComments(false);
});
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shoowErrorLoading state is never reset when switching posts. Add setShoowErrorLoading(false) to the useEffect to clear the error when choosePost changes.

Comment on lines +92 to +96
</a>
<button
data-cy="CommentDelete"
type="button"
className="delete is-small"

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After successful form submission, shoowErrorName and shoowErrorText are not cleared in the finally block. The Clear button also doesn't reset all error states consistently (line 34-36 clears shoowErrorText instead of shoowErrorName).

Comment on lines +43 to +48

function deleteComment(id: number) {
setAllComments(current =>
current ? current.filter(com => com.id !== id) : [],
);
FunctionCalls.deleteComment(id).then(() => {});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The deleteComment function ignores errors from the API call. The optional checklist requirement specifies handling Add and Delete errors so the user can retry. Consider adding try/catch handling.

return client.get<Post[]>(`/posts?userId=${userId}`);
};

export const getPost = (id: number) => {

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shoowErrorLoading state is never reset when switching posts. This means if loading comments for one post fails, the error will incorrectly persist when selecting another post. Reset it alongside other state changes in the useEffect.

Comment thread src/App.tsx
Comment on lines +17 to +19
// тут зберігаються обраний юсер і всі юсери з серверу
const [chooseUser, setChooseUser] = useState<User | null>(null);
const [allUsers, setAllUsers] = useState<User[] | null>(null);

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The shoowErrorLoading state is not reset when the post changes. Add setShoowErrorLoading(false); to the beginning of the useEffect to clear previous errors when a new post is selected.

Comment thread src/App.tsx
Comment on lines +43 to +48
.catch(() => {
setShowError(true);
})
.finally(() => {
setShowLoading(false);
});

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The delete function doesn't handle errors. If deleteComment fails, the comment has already been removed from the UI (line 45) with no error notification. Consider adding try/catch or at least handling the error so the user knows the deletion failed.

@brespect brespect left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm 🔥

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants