Skip to content

react_dynamic-list-of-posts_solution#1445

Open
Banderos14 wants to merge 1 commit into
mate-academy:masterfrom
Banderos14:develop
Open

react_dynamic-list-of-posts_solution#1445
Banderos14 wants to merge 1 commit into
mate-academy:masterfrom
Banderos14:develop

Conversation

@Banderos14

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

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

.then(() => {
setBody('');
setErrors({});
})

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 catch block is empty - users cannot retry when adding a comment fails. According to item 8 of the description, handle Add errors so the user can retry.

Comment on lines +83 to 89
onChange={event => {
setName(event.target.value);
removeError('name');
}}
/>

<span className="icon is-small is-left">

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 calls client.delete() but has no error handling. If deletion fails, the user cannot retry. According to item 8 of the description, handle Delete errors so the user can retry.

const [body, setBody] = useState('');
const [errors, setErrors] = useState<FormErrors>({});
const [isSubmitting, setIsSubmitting] = 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.

State is initialized with props (useState<User | null>(null) uses null directly, but the pattern check applies to derived state from props). This is acceptable here as null is an explicit default, not a derived value from props.

Comment thread src/App.tsx
Comment on lines +83 to +89

const deleteComment = (commentId: number) => {
setComments(currentComments =>
currentComments.filter(comment => comment.id !== commentId),
);

<div className="notification is-warning" data-cy="NoPostsYet">
No posts yet
client.delete(`/comments/${commentId}`);

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 the checklist requirement (*) to use try {..} catch for error handling. The API call doesn't handle errors, so users cannot retry when deletion fails.

Comment thread src/App.tsx
})
.finally(() => {
setIsPostsLoading(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.

This empty catch block silently swallows errors. Users won't know if their comment failed to add. Consider setting an error state so users can see and retry failed submissions.

</button>
</td>
</tr>
);

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 .catch(() => {}) silently swallows errors. The task requirement states: 'Handle Add and Delete errors so the user can retry'. Users have no way of knowing if their comment failed to add, and cannot retry the operation.

{canShowCommentsContent && comments.length === 0 && (
<p className="title is-4" data-cy="NoCommentsMessage">
No comments yet
</p>

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 .catch(() => {}) silently ignores API errors. The task requires handling Add and Delete errors so the user can retry. Consider storing error state and showing a notification to the user.

Comment on lines 88 to 89
<button
data-cy="WriteCommentButton"

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 client.delete call doesn't handle errors. If deletion fails, the UI will have already removed the comment, leaving the app in an inconsistent state. Consider adding error handling with retry capability.

Comment on lines +33 to 40
<h2 data-cy="PostTitle">{`#${post.id}: ${post.title}`}</h2>

<p data-cy="PostBody">
eveniet quo quis laborum totam consequatur non dolor ut et est
repudiandae est voluptatem vel debitis et magnam
</p>
</div>
<p data-cy="PostBody">{post.body}</p>
</div>

<div className="block">
<Loader />
<div className="block">
{isLoading && <Loader />}

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 function calls setState 7 times in sequence. While functionally correct, the checklist recommends avoiding multiple setState calls in one function for code clarity. Consider batching related state updates or using a single state object.

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.

2 participants