add solution by GabrielSpelchuk · Pull Request #3576 · mate-academy/react_add-todo-form

GabrielSpelchuk · 2026-03-11T09:12:19Z

mateacademy-ai-mentor

Review Summary

Good job on implementing the form validation and state management. However, some changes are needed before this can be approved, primarily related to the data structure and data flow.

The most critical issue is that new todo objects are created without the required user object. You'll need to update the Todo type in src/types/Todo.ts to include the user property, and then in App.tsx, find the selected user and add their full object to the new todo upon creation.

This is related to the second main issue: data preparation should happen in the App component [CHECKLIST ITEM #5]. Instead of passing the entire users list down to child components for them to perform lookups, App.tsx should enrich each todo with its corresponding user data. This simplifies the child components significantly.

Lastly, please avoid using single-letter variable names like t or u in your map and find functions, as this violates our coding style guidelines [CHECKLIST ITEM #1]. Addressing these points will bring your solution in line with the requirements.

✨ 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-03-11T09:14:48Z

src/App.tsx

+    return false;
+  };
+
+  const nextId = todos.length > 0 ? Math.max(...todos.map(t => t.id)) + 1 : 1;


This violates checklist item #1: 'Don't use one-letter variable naming.' Please use a more descriptive name for the parameter in your map function, for example, todo instead of t.

mateacademy-ai-mentor · 2026-03-11T09:14:49Z

src/App.tsx

+          const todo = {
+            id: nextId,
+            title,
+            completed: false,
+            userId,
+          };


The new todo object is missing the required user object. The task description states: "each TODO item must have ... a user object containing: id, name, username, email". You need to find the selected user from usersFromServer and add it to this new todo object before updating the state.

mateacademy-ai-mentor · 2026-03-11T09:14:49Z

src/App.tsx

-            Patricia Lebsack
-          </a>
-        </article>
+        <TodoList users={usersFromServer} todoList={todos} />


Passing the entire usersFromServer list down to child components forces them to perform data lookups, which violates checklist item #5: 'Prepare data in one place (App component) and pass it to child components'.

To fix this, you should enrich each todo object with its corresponding user data within the App component. This should be done for both the initial todosFromServer and for any new todos that are created. The TodoList should then only need the prepared list of todos.

mateacademy-ai-mentor · 2026-03-11T09:14:49Z

src/components/TodoList/TodoList.tsx

+  return (
+    <>
+      {todoList.map((todo: Todo) => (
+        <TodoInfo users={users} todo={todo} key={todo.id} />


This violates checklist item #5: 'Prepare data in one place (App component) and pass it to child components'.

Instead of passing the entire users array to each TodoInfo component, you should find the corresponding user for each todo within this TodoList component. Then, you can pass only the specific user object that TodoInfo needs. This approach centralizes the data preparation logic and makes the child component simpler.

mateacademy-ai-mentor · 2026-03-11T09:14:49Z

src/components/TodoInfo/TodoInfo.tsx

+}
+
+export const TodoInfo = ({ users, todo }: TodoInterface) => {
+  const user = users.find(u => u.id === todo.userId);


This violates checklist item #1: "Don't use one-letter variable naming." Please use a more descriptive name for the parameter in the find method, such as user.

mateacademy-ai-mentor · 2026-03-11T09:14:49Z

src/components/TodoInfo/TodoInfo.tsx

+}
+
+export const TodoInfo = ({ users, todo }: TodoInterface) => {
+  const user = users.find(u => u.id === todo.userId);


This violates checklist item #5: "Prepare data in one place (App component) and pass it to child components." The logic for finding the correct user for a todo should be handled in a parent component (like App.tsx).

The App component should enrich the todo objects with their corresponding user objects before passing them down. This avoids passing the entire users list to every TodoInfo instance and keeps the data-related logic centralized.

mateacademy-ai-mentor · 2026-03-11T09:14:49Z

src/components/TodoInfo/TodoInfo.tsx

+    <article
+      data-id={todo.id}
+      className={cn('TodoInfo', { 'TodoInfo--completed': todo.completed })}
+      key={todo.id}


The key prop is used by React to uniquely identify elements in a list. It should be placed on the component when you are mapping over an array. This is already correctly done in the TodoList component where TodoInfo is rendered. The key here inside the TodoInfo component is redundant and has no effect, so it can be safely removed.

mateacademy-ai-mentor · 2026-03-11T09:14:49Z

src/types/Todo.ts

+export default interface Todo {
+  id: number;
+  title: string;
+  completed: boolean;
+  userId: number;
+}


According to the task description, "each TODO item must have ... a user object containing: id, name, username, email". This interface is missing the user property. You should import the User type from ./User and add the user property to this interface to match the requirements.

mateacademy-ai-mentor

Review Summary

Great job implementing the data enrichment in the App component and updating the Todo type! However, a couple of changes are needed before this can be approved.

The main issue is that while you've correctly added the user object to each todo, you are still passing the entire users array down to TodoList and TodoInfo. This leads to these components performing a data lookup that is now redundant. Please remove the users prop from TodoList and TodoInfo, and update TodoInfo to use the user object directly from its todo prop (todo.user). This will fully align with the requirement to prepare all data in the root component [CHECKLIST ITEM #5].

Additionally, in App.tsx, a single-letter variable u is used in a .find() callback. Please rename it to something more descriptive, like user, to adhere to the project's coding standards [CHECKLIST ITEM #1].

✨ 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-03-11T09:33:33Z

src/components/TodoList/TodoList.tsx

+  users: User[];
+  todoList: Todo[];
+}
+
+export const TodoList = ({ users, todoList }: TodoListInterface) => {
+  return (
+    <>
+      {todoList.map((todo: Todo) => (
+        <TodoInfo users={users} todo={todo} key={todo.id} />


Since the todo objects are now enriched with user data in the App component, you no longer need to pass the users array down to this component and then on to TodoInfo.

Please remove the users prop from TodoListInterface, the component's function signature, and from the props passed to <TodoInfo />. This will simplify your code and fully align with checklist item #5, which requires data to be prepared in the root component.

mateacademy-ai-mentor · 2026-03-11T09:33:33Z

src/App.tsx

+          }
+
+          const selectedUser: User | undefined = usersFromServer.find(
+            u => u.id === userId,


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

mateacademy-ai-mentor · 2026-03-11T09:33:33Z

src/App.tsx

-            Patricia Lebsack
-          </a>
-        </article>
+        <TodoList users={usersFromServer} todoList={todos} />


This violates checklist item #5. You have already enriched each todo object with its corresponding user data. Therefore, you don't need to pass the entire usersFromServer list down to child components. The TodoList and its children should rely on the user property within each todo object instead of performing the lookup again.

mateacademy-ai-mentor · 2026-03-11T09:33:33Z

src/components/TodoInfo/TodoInfo.tsx

+export const TodoInfo = ({ users, todo }: TodoInterface) => {
+  const user = users.find(findUser => findUser.id === todo.userId);


This component shouldn't perform data lookups. According to checklist item #5, all data preparation should be handled in the App component. The todo object passed as a prop already contains the necessary user information.

You should:

Remove the users prop from TodoInterface and from the component's function signature.

Remove this line where you find the user.

Use todo.user directly in your JSX.

mateacademy-ai-mentor

Review Summary

I am approving your solution to help you move forward, but please take a moment to review a few important points for future tasks. You've done an excellent job refactoring the data preparation logic into the App component, which has made the child components much cleaner!

For your next submission, please remember to:

Add <label> elements to all form inputs for accessibility. For example: <label>Title: <input ... /></label>.
Use descriptive variable names instead of single letters, like changing u to user in your .find() method in App.tsx [CHECKLIST ITEM #1].

One minor tip: the key prop inside TodoInfo.tsx is redundant because React only needs it on the list items where you use .map(), which you've already done correctly in TodoList.tsx. Keep up the great 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-03-11T09:44:30Z

src/components/TodoInfo/TodoInfo.tsx

+    <article
+      data-id={todo.id}
+      className={cn('TodoInfo', { 'TodoInfo--completed': todo.completed })}
+      key={todo.id}


The key prop is only used by React to identify components in a list. Since TodoList already provides a key when it maps over the todos to render TodoInfo, this key on the <article> element inside the component is redundant and has no effect. You can safely remove it.

mateacademy-ai-mentor · 2026-03-11T09:44:30Z

src/App.tsx

+          }
+
+          const selectedUser: User | undefined = usersFromServer.find(
+            u => u.id === userId,


This still violates checklist item #1: 'Don't use one-letter variable naming.' As mentioned in the previous review, please use a more descriptive name for the callback parameter, such as user.

mateacademy-ai-mentor · 2026-03-11T09:44:30Z

src/App.tsx

+          <input
+            type="text"
+            data-cy="titleInput"
+            value={title}
+            onChange={event => setTitle(event.target.value)}
+            required
+          />


To improve accessibility and fully meet the requirement to "add labels and placeholders where they are needed", please associate a <label> with the title input. For example: <label>Title: <input ... /></label>.

mateacademy-ai-mentor · 2026-03-11T09:44:30Z

src/App.tsx

+          <select
+            data-cy="userSelect"
+            value={userId}
+            onChange={value => setUserId(+value.target.value)}
+            required
+          >
            <option value="0" disabled>
              Choose a user
            </option>
+            {usersFromServer.map(user => (
+              <option value={user.id} key={user.id}>
+                {user.name}
+              </option>
+            ))}
          </select>


Similarly, this select element should have a <label> associated with it for better accessibility. For example: <label>User: <select ... /></label>.

add solution

cadbb18

mateacademy-ai-mentor suggested changes Mar 11, 2026

View reviewed changes

fixed version

d759a02

GabrielSpelchuk requested a review from mateacademy-ai-mentor March 11, 2026 09:31

mateacademy-ai-mentor suggested changes Mar 11, 2026

View reviewed changes

fixed all bugs

4ea586e

GabrielSpelchuk requested a review from mateacademy-ai-mentor March 11, 2026 09:42

mateacademy-ai-mentor approved these changes Mar 11, 2026

View reviewed changes

		export const TodoInfo = ({ users, todo }: TodoInterface) => {
		const user = users.find(findUser => findUser.id === todo.userId);

Conversation

GabrielSpelchuk commented Mar 11, 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!

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!

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