add solution by Evg3n22 · Pull Request #3565 · mate-academy/react_add-todo-form

Evg3n22 · 2026-02-28T08:22:14Z

Replace <your_account> with your Github username in the DEMO LINK and add it to the PR description.

mateacademy-ai-mentor

Review Summary

Great job implementing the core functionality for adding and displaying todos! The form validation logic is also working well. However, there are a few critical issues that need to be addressed before this can be approved.

Here are the main points to work on:

Data Structure & Logic: The Todo interface in src/services/todo.ts is missing the required user object. This has led to the data preparation logic being placed in TodoList.tsx. Please update the Todo type and move the user-finding logic into App.tsx to align with the project's architecture guidelines [CHECKLIST ITEM #5].
Runtime Safety: In TodoList.tsx, using the non-null assertion operator (!) is unsafe and could crash the app if a user isn't found. You should add a check to ensure the user exists before rendering [CHECKLIST ITEM #4].
ID Generation Bug: The logic for creating a new todo ID in App.tsx is getNextId(todos) + 1. Since getNextId already returns the correct next ID, you are adding 1 twice and skipping IDs. Please use the value from getNextId(todos) directly.
Missing Requirements: The form in App.tsx is missing the required <label> elements for accessibility, and the email link in UserInfo.tsx needs the mailto: prefix to function correctly.

Once these issues are resolved, your solution will be in great shape!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-02-28T08:25:00Z

src/components/UserInfo/UserInfo.tsx

+
+export const UserInfo = ({ user }: Props) => {
+  return (
+    <a className="UserInfo" href={user.email}>


The href attribute for an email link should start with mailto:. Without this prefix, clicking the link won't open the user's default email client.

mateacademy-ai-mentor · 2026-02-28T08:25:00Z

src/components/TodoInfo/TodoInfo.tsx

+type Props = {
+  todo: Todo;
+  user: User;
+};


This component's props signature (todo and user as separate props) indicates that the logic for associating a user with a todo is happening outside the root App component. This violates checklist item #5: 'Prepare data in one place (App component) and pass it to child components'.

The App component should be responsible for creating a complete data object for each todo, including its user. This component should then receive a single prop (e.g., todoWithUser) that contains all the necessary information, simplifying the data flow.

mateacademy-ai-mentor · 2026-02-28T08:25:00Z

src/components/TodoList/TodoList.tsx

+  const findUserById = (usersArr: User[], id: number): User | undefined => {
+    return usersArr.find(user => user.id === id);
+  };


This violates checklist item #5: 'Prepare data in one place (App component) and pass it to child components. Don't use import keyword in non-root components'. The logic to find the user associated with a todo should be handled in the App component. The App should prepare an array of 'todos with users' and pass that down to TodoList, making this component purely for presentation.

mateacademy-ai-mentor · 2026-02-28T08:25:00Z

src/components/TodoList/TodoList.tsx

+  return (
+    <section className="TodoList">
+      {todos.map(todo => {
+        const user: User = findUserById(users, todo.userId)!;


Using the non-null assertion operator ! is unsafe here. If findUserById returns undefined (for example, if a todo has a userId that doesn't exist in the users array), the application will crash. This violates checklist item #4. You should add a check to ensure user exists before rendering the TodoInfo component.

mateacademy-ai-mentor · 2026-02-28T08:25:01Z

src/components/TodoList/TodoList.tsx

+        const user: User = findUserById(users, todo.userId)!;
+
+        return (
+          <TodoInfo key={todo.id} user={user} todo={todo} data-id={todo.id} />


The data-id prop is redundant here. The requirement is for the rendered HTML element to have the data-id attribute. Your TodoInfo component already correctly sets this attribute on its root <article> element. You can safely remove this prop from the TodoInfo invocation.

mateacademy-ai-mentor · 2026-02-28T08:25:01Z

src/App.tsx

+    return Math.max(...arr.map(t => t.id)) + 1;
+  };
+
+  const selectedUser = users.find(u => u.id === Number(selectUser));


This violates checklist item #1: 'Don't use one-letter variable naming'. Please use a more descriptive name, for example, user.

mateacademy-ai-mentor · 2026-02-28T08:25:01Z

src/App.tsx

+    }
+
+    const newTodo: Todo = {
+      id: getNextId(todos) + 1,


The logic for generating the new id is incorrect. The getNextId function already calculates the next available ID by finding the maximum ID and adding 1. By adding 1 again here, you are skipping an ID every time a new todo is created. You should just use the value returned from getNextId(todos) directly.

mateacademy-ai-mentor · 2026-02-28T08:25:01Z

src/App.tsx

        <div className="field">
-          <input type="text" data-cy="titleInput" />
-          <span className="error">Please enter a title</span>
+          <input


The task requires adding labels where they are needed for form fields. This input is missing a corresponding <label> element for accessibility. Consider adding one. Also, a placeholder attribute could improve user experience.

mateacademy-ai-mentor · 2026-02-28T08:25:01Z

src/App.tsx


        <div className="field">
-          <select data-cy="userSelect">
+          <select


The task requires adding labels where they are needed for form fields. This select element is missing a corresponding <label> element for better accessibility and user guidance.

mateacademy-ai-mentor · 2026-02-28T08:25:01Z

src/services/todo.ts

+export interface Todo {
+  id: number;
+  title: string;
+  completed?: boolean;
+  userId: User['id'];
+}


The task description states: "each TODO item must have the following fields: ... and a user object containing: id, name, username, email". This interface is missing the user property.

Adding an optional user property here (e.g., user?: User;) would align this type with the requirements. It would also enable you to prepare the data correctly in the App component by combining todos and users in one place, which is a requirement from checklist item #5.

…ng requirements malto:

mateacademy-ai-mentor

Review Summary

Great work on refactoring the data handling! Moving the logic to the App component has really improved the structure. I am requesting a few changes to address some critical issues before this can be approved.

First, the form in App.tsx is missing <label> elements for the input and select fields. This is a key accessibility requirement from the task description that needs to be added.

Second, the use of the non-null assertion operator (!) in App.tsx when matching todos to users is unsafe and could cause the app to crash if a user isn't found. Please handle this case more safely, for example, by filtering out todos that don't have a matching user.

Once these two points are fixed, your solution will be in great shape!

✨ Please rate this review in the Platform's notification center¹. 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 button² 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 tag³.
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.

mateacademy-ai-mentor · 2026-02-28T09:01:55Z

src/components/TodoInfo/TodoInfo.tsx

+type Props = {
+  todo: Todo;
+  user: User;
+};


The user prop is redundant here because the user object is already a property of the todo object. To make the component's API cleaner, you can simplify the props to only accept the todo object.

For example:

// Props would be just: type Props = { todo: Todo; }; // Then, inside the component, you'd use `todo.user`: export const TodoInfo = ({ todo }: Props) => { // ... return ( // ... <UserInfo user={todo.user} /> // ... ); };

mateacademy-ai-mentor · 2026-02-28T09:01:55Z

src/App.tsx

+import { useState } from 'react';

 export const App = () => {
+  const users: User[] = [...usersFromServer];


This creates an unnecessary copy of the usersFromServer array. Since you are not mutating this array, you can directly assign it. This violates checklist item #3: 'If you are using a non-mutating array method, you don't need to create a copy of the array'.

mateacademy-ai-mentor · 2026-02-28T09:01:55Z

src/App.tsx

+  const [todos, setTodos] = useState<Todo[]>(() =>
+    todosFromServer.map(t => ({
+      ...t,
+      user: users.find(u => u.id === t.userId)!,


Using the non-null assertion operator (!) here is unsafe. If a todo from the server has a userId that doesn't exist in usersFromServer, the application will crash. It's better to handle this possibility, for example, by filtering out any todos that don't have a matching user.

mateacademy-ai-mentor · 2026-02-28T09:01:55Z

src/App.tsx

+      title: title,
+      completed: false,
+      userId: selectedUser!.id,
+      user: findUserById(users, selectedUser!.id)!,


This line re-finds the user, but you already have the complete user object stored in the selectedUser constant. You can simplify this and avoid repeating logic by using user: selectedUser!. This aligns with checklist item #2: 'Don't repeat yourself'.

mateacademy-ai-mentor · 2026-02-28T09:01:55Z

src/App.tsx

      <h1>Add todo form</h1>

-      <form action="/api/todos" method="POST">
+      <form action="/api/todos" method="POST" onSubmit={handleSubmit}>


The form is missing <label> elements for the title input and user select, which is a requirement from the task description (add labels and placeholders where they are needed). Labels are important for accessibility. Each form control should have an associated <label>. This was also pointed out in the previous review.

mateacademy-ai-mentor · 2026-02-28T09:01:55Z

src/components/TodoList/TodoList.tsx

+  // const findUserById = (usersArr: User[], id: number): User | undefined => {
+  //   return usersArr.find(user => user.id === id);
+  // };
+
+  // const todosWithUsers = todos.filter(todo =>
+  //   users.some(u => u.id === todo.userId),
+  // );


It's good practice to remove commented-out code that is no longer needed. This improves the readability and maintainability of the component.

mateacademy-ai-mentor · 2026-02-28T09:01:55Z

src/components/TodoList/TodoList.tsx

+            key={todo.id}
+            user={todo.user}
+            todo={todo}
+            data-id={todo.id}


The data-id attribute is already being set within the TodoInfo component itself (using the id from the todo object). Passing it as a prop here is unnecessary because the TodoInfo component doesn't expect or use a data-id prop. You can safely remove this line.

mateacademy-ai-mentor

Review Summary

Great job on this task and for addressing the previous feedback! I am approving this solution as an educational override to help you move forward, but there are a couple of important points to review that are causing the component tests to fail.

The main issue is in src/components/TodoList/TodoList.tsx, where you're passing an unnecessary data-id prop to the TodoInfo component. This component already sets its own data-id internally, and removing this prop from the parent will fix the failing tests. For even cleaner code, I also suggest removing the redundant user prop from TodoInfo (since it's already in the todo object) and using more descriptive variable names like todo instead of t in App.tsx [CHECKLIST ITEM #1].

You've made great progress, keep up the excellent work!

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

Rate AI review example ↩

mateacademy-ai-mentor · 2026-02-28T09:31:42Z

src/components/TodoList/TodoList.tsx

+            key={todo.id}
+            user={todo.user}
+            todo={todo}
+            data-id={todo.id}


This data-id prop is not needed here. The TodoInfo component already receives the entire todo object and is responsible for setting its own data-id attribute internally. Passing props that a component doesn't expect can lead to confusion.

mateacademy-ai-mentor · 2026-02-28T09:31:42Z

src/App.tsx

+import { useState } from 'react';

 export const App = () => {
+  const users: User[] = [...usersFromServer];


This copy of usersFromServer is not needed. Since the original array isn't being modified, you can assign it directly. This relates to checklist item #3 about avoiding unnecessary copies.

mateacademy-ai-mentor · 2026-02-28T09:31:42Z

src/App.tsx

+  const users: User[] = [...usersFromServer];
+  const [todos, setTodos] = useState<Todo[]>(() =>
+    todosFromServer
+      .map(t => {


This violates checklist item #1: 'Don't use one-letter variable naming.' Please use a more descriptive name like todo instead of t. This also applies to other single-letter variables in your array methods in this file.

mateacademy-ai-mentor · 2026-02-28T09:31:42Z

src/components/TodoInfo/TodoInfo.tsx

+
+type Props = {
+  todo: Todo;
+  user: User;


The user prop is redundant because the todo object already contains the full user object. You can simplify this component by removing this prop and using todo.user directly when passing it to the UserInfo component.

add solution

dd86d20

mateacademy-ai-mentor suggested changes Feb 28, 2026

View reviewed changes

fix: data structure & logic, runtime safety, id generation bug, missi…

d6a265c

…ng requirements malto:

Evg3n22 requested a review from mateacademy-ai-mentor February 28, 2026 08:59

mateacademy-ai-mentor suggested changes Feb 28, 2026

View reviewed changes

fix: add labels and rewrite app todos logic

ae1c6dc

Evg3n22 requested a review from mateacademy-ai-mentor February 28, 2026 09:28

mateacademy-ai-mentor approved these changes Feb 28, 2026

View reviewed changes

Conversation

Evg3n22 commented Feb 28, 2026

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

Review Summary

Footnotes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants