React-add-todo-form by FRANCISCOPELEGRINI · Pull Request #3567 · mate-academy/react_add-todo-form

FRANCISCOPELEGRINI · 2026-03-01T18:03:44Z

No description provided.

mateacademy-ai-mentor

Review Summary

Good job setting up the form and state management for adding new todos! 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:

Todo Data Structure: The task requires each todo item to contain a full user object. Currently, you're looking up user details on every render. Instead, when a new todo is created, you should find the corresponding user from usersFromServer and embed the entire user object into the new todo item you add to the state.
New Todo ID Generation: The logic for creating a new todo ID in App.tsx is based on the initial todosFromServer list. This will cause duplicate IDs if you add more than one todo. Please update this to calculate the new ID based on the current list of todos in your state.
Error Handling: The error messages for the title and user select fields should disappear as soon as the user starts typing or selects a different option. Currently, they only disappear on the next successful submission.
Component Structure: The logic for rendering a single todo item should be moved from App.tsx into the TodoInfo component, which is currently empty. Also, the TodoList component is being used to render <option> elements, which is not its intended purpose.

Addressing these points will bring your solution in line with the project 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-01T18:05:48Z

src/components/TodoList/TodoList.tsx

@@ -1 +1,14 @@
-export const TodoList = () => {};
+interface RInterface {


Interface names should be descriptive. RInterface doesn't explain what this interface represents. A name like User would be more appropriate.

mateacademy-ai-mentor · 2026-03-01T18:05:48Z

src/components/TodoList/TodoList.tsx

+  email: string;
+}
+
+export const TodoList = (r: RInterface) => {


This violates checklist item #1: 'Don't use one-letter variable naming.' The parameter r should be replaced with a more descriptive name like user.

Also, the component is named TodoList, but its function is to render a single <option> for a user. This is misleading; a name like UserOption would be more accurate. The task requires a TodoList component to display the list of todos.

mateacademy-ai-mentor · 2026-03-01T18:05:49Z

src/App.tsx

+interface TodoVar {
+  id: number;
+  title: string;
+  completed: boolean;
+  userId: number;
+}


The task description requires each TODO item to have a user object. This interface should be updated to include a user property. This will allow you to prepare the data correctly and avoid looking up user information on every render.

mateacademy-ai-mentor · 2026-03-01T18:05:49Z

src/App.tsx


 export const App = () => {
+  const [selectedUser, setSelectedUser] = useState(0);
+  const [users, setUsers] = useState<TodoVar[]>(todosFromServer);


The state variable users is storing the list of todos, which can be confusing. It would be clearer to rename it to todos to accurately reflect its content.

mateacademy-ai-mentor · 2026-03-01T18:05:49Z

src/App.tsx

+  const [users, setUsers] = useState<TodoVar[]>(todosFromServer);
+  const [aparecerTxt, SetAparecerTxt] = useState<boolean[]>([false, false]);
+  const [todoVar, setTodoVar] = useState<TodoVar>({
+    id: todosFromServer[todosFromServer.length - 1].id + 1,


The ID for a new todo is calculated based on the initial todosFromServer list. This means if you add multiple todos, they will have the same ID. The new ID should be calculated from the current list of todos in your state (users). This logic is also repeated in the cleanTodo function, which is a violation of the DRY principle (Checklist item #2).

mateacademy-ai-mentor · 2026-03-01T18:05:49Z

src/App.tsx

+  const onChangeTodo = (
+    event: ChangeEvent<HTMLInputElement | HTMLSelectElement>,
+    type: string,
+  ) => {
+    if (type === 'title' && 'target' in event) {
+      setTodoVar(prev => ({
+        ...prev,
+        title: (event as ChangeEvent<HTMLInputElement>).target.value,
+      }));
+
+      return;
+    }
+
+    if (type === 'user') {
+      const value =
+        typeof event === 'string'
+          ? event
+          : (event as ChangeEvent<HTMLSelectElement>).target.value;
+
+      setTodoVar(prev => ({ ...prev, userId: Number(value) }));
+    }
+  };


The requirements state that an error message should be hidden immediately after any change to the field that has the error. You should add logic here to update the error state (aparecerTxt) and clear the corresponding error when the user types in the input or selects a user.

mateacademy-ai-mentor · 2026-03-01T18:05:49Z

src/App.tsx

+          <input
+            type="text"
+            data-cy="titleInput"
+            value={todoVar.title}
+            onChange={e => onChangeTodo(e, 'title')}
+          />


The task requires adding labels and placeholders where they are needed. This input is missing a <label> and a placeholder attribute, which are important for accessibility and user experience. The same applies to the <select> element below.

mateacademy-ai-mentor · 2026-03-01T18:05:49Z

src/App.tsx

+            <option disabled={isOpen} value="0">
              Choose a user
            </option>
+            {usersFromServer.map(r => TodoList(r))}


In React, it's a best practice to render components using JSX syntax (e.g., <TodoList user={r} />) rather than calling them as functions. Calling components as functions can lead to unexpected behavior with hooks and performance issues. Also, consider renaming the TodoList component to something more descriptive like UserOption, as it renders a single <option>.

mateacademy-ai-mentor · 2026-03-01T18:05:49Z

src/App.tsx

-            Leanne Graham
-          </a>
-        </article>
+        {users.map((a: TodoVar, b: number) => {


This violates checklist item #1: "Don't use one-letter variable naming." Using names like a and b makes the code difficult to understand. Please use more descriptive names, such as todo and index.

mateacademy-ai-mentor · 2026-03-01T18:05:49Z

src/components/TodoInfo/TodoInfo.tsx

+// interface User {
+//   id: number;
+//   name: string;
+//   username: string;
+//   email: string;
+// }
+
+// interface RInterface {
+//   id: number;
+//   title: string;
+//   completed: boolean;
+//   userId: number;
+// }
+
+// interface Props {
+//   a: RInterface;
+//   b: number;
+//   user: User;
+// }
+
+// export const TodoInfo: React.FC<Props> = ({ a, b, user }) => {
+//   return (
+//     <article>
+//       <h2>{a.title}</h2>
+//       <h2>{b}</h2>
+//       <h2>{user.name}</h2>
+//     </article>
+//   );
+// };


This component is not implemented. The rendering logic for individual todo items should be placed here instead of directly inside the App component. Creating a dedicated TodoInfo component will make your code more modular and easier to maintain.

mateacademy-ai-mentor

Review Summary

I am requesting changes because the application currently has several critical build and test failures that need to be resolved before it can be approved.

The most important issue is the application's structure. All state management (like the list of todos) and form logic should be in the App.tsx component, with data passed down to child components as props. Currently, this logic is incorrectly placed in TodoInfo.tsx, which violates a core requirement. [CHECKLIST ITEM #5]

Additionally, there are specific errors causing tests to fail:

Components are being called as functions (e.g., Generator()) instead of being rendered with JSX (<Generator />), which is an anti-pattern in React.
The TodoList component expects props named a and b, but the tests pass todo, causing a runtime error. Please use descriptive prop names. [CHECKLIST ITEM #1]

Finally, remember that when creating a new todo, you must embed the entire user object, not just the userId. Addressing these points will get your project on the right track.

✨ 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.

src/App.tsx

mateacademy-ai-mentor · 2026-03-02T22:41:43Z

src/App.tsx

-      </section>
-    </div>
-  );
+  return Generator();


This is not the correct way to render a React component. Components should be rendered using JSX syntax (e.g., <Generator />), not called directly as functions. Calling a component as a function bypasses React's lifecycle and hook management, which can lead to bugs and unpredictable behavior.

src/components/TodoList/TodoList.tsx

src/components/TodoInfo/TodoInfo.tsx

src/components/UserInfo/UserInfo.tsx

mateacademy-ai-mentor

Review Summary

I am approving this submission as an educational override to help you progress, but there are a few critical points to address for future tasks. You've done a great job implementing the form logic for adding new todos!

The most important issue is that all state management (useState) and application logic must be located in the App.tsx component, not in child components like TodoInfo.tsx [CHECKLIST ITEM #5]. Data and functions should then be passed down as props. Additionally, please make sure to render components using JSX syntax (e.g., <UserInfo {...user} />) instead of calling them as functions, which is a React anti-pattern. The tests are also failing because the component in TodoInfo.tsx is named Generator but should be TodoInfo.

Keep up the great work, and focus on these architectural patterns going forward!

✨ 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-03T22:24:45Z

src/components/UserInfo/UserInfo.tsx

@@ -1 +1,14 @@
-export const UserInfo = () => {};
+interface RInterface {


The interface name RInterface is not very descriptive. A name like User or UserInfoProps would better describe the data it represents.

mateacademy-ai-mentor · 2026-03-03T22:24:45Z

src/components/UserInfo/UserInfo.tsx

+  email: string;
+}
+
+export const UserInfo = (r: RInterface) => {


This component is being called as a function from its parent, which is a React anti-pattern. Components should always be rendered using JSX syntax (<UserInfo {...props} />).

To fix this, you should change the signature to accept a props object. For example: export const UserInfo = ({ id, name }: UserInfoProps) => { ... }.

Also, the parameter r violates checklist item #1, which advises against one-letter variable names. Using a more descriptive name like user would improve readability.

mateacademy-ai-mentor · 2026-03-03T22:24:45Z

src/components/UserInfo/UserInfo.tsx

+
+export const UserInfo = (r: RInterface) => {
+  return (
+    <option value={r.id} key={r.id}>


The key prop should be applied to the component instance inside the .map() loop in the parent component, not on the <option> element here. React uses the key on the list items themselves to track them.

For example, in the parent component: {users.map(user => <UserInfo key={user.id} {...user} />)}

mateacademy-ai-mentor · 2026-03-03T22:24:45Z

src/components/TodoList/TodoList.tsx

+  setUsers: React.Dispatch<React.SetStateAction<TodoVar[]>>;
+}
+
+export const TodoList: React.FC<TodoListProps> = ({
+  todo,
+  index,
+  setUsers,


Passing the setUsers state setter and the index prop directly to a child component creates tight coupling, making this component less reusable and dependent on the parent's implementation. A better approach is to pass a specific callback function (e.g., onComplete(id: number)) that the parent defines. This would also allow you to use the unique todo.id for updates, which is more reliable than an array index.

mateacademy-ai-mentor · 2026-03-03T22:24:45Z

src/components/TodoList/TodoList.tsx

+  index,
+  setUsers,
+}) => {
+  const setTrue = (e: React.MouseEvent<HTMLAnchorElement>) => {


This violates checklist item #1: 'Don't use one-letter variable naming.' Please use a more descriptive name for the event object, such as event.

mateacademy-ai-mentor · 2026-03-03T22:24:45Z

src/components/TodoInfo/TodoInfo.tsx

+export const Generator = () => {
+  const [users, setUsers] = useState<TodoVar[]>(
+    todosFromServer.map(todo => {
+      const fullUser = usersFromServer.find(u => u.id === todo.userId)!;


This violates checklist item #1: 'Don't use one-letter variable naming.' Please use a more descriptive name like user instead of u. This suggestion also applies to e for events and r in the .map() on line 141.

mateacademy-ai-mentor · 2026-03-03T22:24:45Z

src/components/TodoInfo/TodoInfo.tsx

+  const [isOpen, setIsOpen] = useState<boolean>(false);
+
+  const [todoVar, setTodoVar] = useState<TodoVar>({
+    id: users[users.length - 1].id + 1,


The requirement is to use the "largest id in the array + 1". This implementation assumes the users array (which actually contains todos) is always sorted by ID. This is not a safe assumption. A more robust way to find the largest ID would be to iterate through all todos, for example: Math.max(0, ...users.map(todo => todo.id)) + 1.

mateacademy-ai-mentor · 2026-03-03T22:24:45Z

src/components/TodoInfo/TodoInfo.tsx

+    id: users[users.length - 1].id + 1,
+    title: '',
+    completed: false,
+    user: usersFromServer[0],


The initial user for a new todo should not be a valid user from the list. This breaks the validation logic because todoVar.user.id will not be 0, allowing the form to be submitted without the user making a selection. The initial state should represent that no user has been chosen, for instance, by using a placeholder user object with id: 0.

mateacademy-ai-mentor · 2026-03-03T22:24:45Z

src/components/TodoInfo/TodoInfo.tsx

+    setTodoVar({
+      id: users[users.length - 1].id + 1,
+      title: '',
+      completed: false,
+      user: usersFromServer[0],
+    });


The object being created here is a repetition of the initial state defined for todoVar on lines 40-45. This violates the DRY (Don't Repeat Yourself) principle from checklist #2. You should extract this object creation logic into a reusable function to avoid duplication.

mateacademy-ai-mentor · 2026-03-03T22:24:45Z

src/components/TodoInfo/TodoInfo.tsx

+              Choose a user
+            </option>
+
+            {usersFromServer.map(r => UserInfo(r))}


In React, components must be rendered using JSX syntax (<Component />), not called as plain functions (Component()). This was also mentioned in the previous review. Calling components as functions can lead to issues with hooks and performance.

This should be changed to:

{usersFromServer.map(user => ( <UserInfo key={user.id} id={user.id} name={user.name} /> ))}

You will also need to update the UserInfo component to receive props correctly instead of a single argument.

React-add-todo-form

fa00f12

mateacademy-ai-mentor suggested changes Mar 1, 2026

View reviewed changes

React_add-todo-form 2.0

d6939a8

FRANCISCOPELEGRINI requested a review from mateacademy-ai-mentor March 2, 2026 22:39

mateacademy-ai-mentor suggested changes Mar 2, 2026

View reviewed changes

mateacademy-ai-mentor reviewed Mar 2, 2026

View reviewed changes

src/components/UserInfo/UserInfo.tsx Show resolved Hide resolved

React-add-todo-form 3.0

1aa1570

FRANCISCOPELEGRINI requested a review from mateacademy-ai-mentor March 3, 2026 22:22

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

View reviewed changes

		@@ -1 +1,14 @@
		export const TodoList = () => {};
		interface RInterface {

		@@ -1 +1,14 @@
		export const UserInfo = () => {};
		interface RInterface {

Conversation

FRANCISCOPELEGRINI commented Mar 1, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

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!