all tests passed by AntonNimachuk · Pull Request #3579 · mate-academy/react_add-todo-form

AntonNimachuk · 2026-03-14T01:27:43Z

https://antonnimachuk.github.io/react_add-todo-form/

mateacademy-ai-mentor

Review Summary

You've done a great job building the form and implementing the validation logic. The app is mostly working as expected! However, I'm requesting a few changes to align the solution with the project's core requirements. The main issue is that new todo objects are missing the required user object. Please update your logic to find the full user based on the selected userId and include it when creating a new todo. This will also require updating the Todo type in src/types/Todo.ts. Additionally, the UserInfo component should not import data directly. This violates the rule that data should be prepared in the App component and passed down as props [CHECKLIST ITEM #5]. Please refactor this to receive the user object as a prop. While you're there, you can also fix the repeated users.find() call to avoid repetition [CHECKLIST ITEM #2]. Fixing these data flow issues will make your application much more robust. Keep up the great work!

✨ 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-14T01:30:24Z

src/components/TodoForm/TodoForm.tsx

+    if (!title) {
+      setTitleError(true);
+    } if (!userId) {
+      setUserIdError(true);
+    } if (!title || !userId) {
+      return;
+    }


This validation logic is a bit repetitive. The conditions !title and !userId are checked once to set the error states, and then checked again in the final if statement. You can refactor this to avoid the duplicate checks, for example, by setting boolean flags and checking them only once to decide whether to proceed.

mateacademy-ai-mentor · 2026-03-14T01:30:24Z

src/components/TodoForm/TodoForm.tsx

+    const newTodo : Todo = {
+      id: maxId,
+      title:title,
+      completed:false,
+      userId:userId,
+    };


The task description requires that each new todo item must contain a user object with the selected user's details (id, name, username, email). This newTodo object is missing that user property. You should find the full user object from the users prop based on the selected userId and include it here. Note that this might require you to adjust the Todo type definition to match the requirement.

mateacademy-ai-mentor · 2026-03-14T01:30:24Z

src/App.tsx


 export const App = () => {
+  const users = usersFromServer;
+  const [todos, setTodos] = useState(todosFromServer);


According to checklist item #5, data should be prepared in the root component. It would be better to enrich the todos here by adding the corresponding user object to each todo from todosFromServer. This would centralize your data logic and prevent child components from having to import and search the users list themselves.

mateacademy-ai-mentor · 2026-03-14T01:30:24Z

src/App.tsx

+  const handleTodos = (newTodo : Todo) => {
+    setTodos([...todos, newTodo]);
+  }


The App component should be responsible for creating new todo items since it owns the todos state. This handler should accept the form data (e.g., title and userId) and construct the complete newTodo object. This process should include calculating the new id and adding the required user object, simplifying the TodoForm component and centralizing state logic.

mateacademy-ai-mentor · 2026-03-14T01:30:25Z

src/App.tsx

-          </a>
-        </article>
-      </section>
+        <TodoForm onAdd={handleTodos} users={users} todos={todos}/>


The TodoForm component shouldn't need the entire todos list. This prop is currently used to calculate the new id, but that logic should be moved into the App component's handleTodos function.

mateacademy-ai-mentor · 2026-03-14T01:30:25Z

src/components/TodoInfo/TodoInfo.tsx

+    className={`TodoInfo ${todo.completed ?'TodoInfo--completed' : ''}`}
+  >
+    <h2 className="TodoInfo__title">{todo.title}</h2>
+    <UserInfo userId={todo.userId}/>


According to the task description, each todo item should contain a full user object. The UserInfo component should receive this user object as a prop, rather than just the userId. This will help to avoid data fetching in child components, as per checklist item #5.

mateacademy-ai-mentor · 2026-03-14T01:30:25Z

src/components/UserInfo/UserInfo.tsx

-export const UserInfo = () => {};
+/* eslint-disable */
+
+import users from '../../api/users';


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". This component should not import users directly. Instead, it should receive the relevant user object as a prop from its parent.

mateacademy-ai-mentor · 2026-03-14T01:30:25Z

src/components/UserInfo/UserInfo.tsx

+    <a className="UserInfo" href={`mailto:${users.find(u => u.id===userId)?.email}`}>
+      {`${users.find(u => u.id===userId)?.name}`}


This violates checklist item #2: "Don't repeat yourself". The users.find(...) method is called twice to get the same user. You should find the user once, store the result in a variable, and then use that variable to access the email and name properties. This also makes the code more readable and efficient.

mateacademy-ai-mentor · 2026-03-14T01:30:25Z

src/types/Todo.ts

+export type Todo = {
+  id: number;
+  title: string;
+  completed: boolean;
+  userId: number;
+};


The task description specifies that each Todo item must include a user object containing fields like id, name, username, and email. This type is missing the user property. You'll need to import the User type and add it here.

mateacademy-ai-mentor

Review Summary

Excellent progress on this pull request! You've successfully addressed the major feedback from the previous review. However, I'm requesting a few changes to fix some critical issues that could lead to runtime errors.

New Todo ID Calculation: In src/components/TodoForm/TodoForm.tsx, the calculation Math.max(...todos.map(todo => todo.id)) will fail if the todos array is empty, as Math.max() with no arguments returns -Infinity. Please add a check to handle this edge case, for example, by setting the initial ID to 1.
Unsafe Non-Null Assertions: You are using the non-null assertion operator (!) in TodoForm.tsx and TodoInfo.tsx. This is risky because if users.find() returns undefined or todo.user is not present, the app will crash. Please add proper checks to ensure the user object exists before you use it. This also addresses [CHECKLIST ITEM #4].
Variable Naming: In App.tsx and TodoForm.tsx, you've used the single-letter variable u in a callback. Please use a more descriptive name like user to comply with [CHECKLIST ITEM #1].

These changes will make your code more robust and prevent potential bugs. Keep up the great work!

✨ 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-15T00:55:19Z

src/components/TodoForm/TodoForm.tsx

+  const [titleError, setTitleError] = useState(false);
+  const [userIdError, setUserIdError] = useState(false);
+
+  const maxId = Math.max(...todos.map(todo => todo.id)) + 1;


This calculation for maxId can lead to an error if the todos array is empty. Math.max() called without arguments (which happens with an empty array) returns -Infinity. You should add a check to handle this edge case, for example, by setting the ID to 1 if the array is empty.

mateacademy-ai-mentor · 2026-03-15T00:55:19Z

src/components/TodoForm/TodoForm.tsx

+      return;
+    }
+
+    const user = users.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 instead of u.

mateacademy-ai-mentor · 2026-03-15T00:55:19Z

src/components/TodoForm/TodoForm.tsx

+      title:title,
+      completed:false,
+      userId:userId,
+      user:user!


Using the non-null assertion operator (!) is risky here because users.find could return undefined. This would cause a runtime error, and it bypasses the type safety that TypeScript provides. A safer approach is to verify that user is not undefined before proceeding to create the newTodo.

mateacademy-ai-mentor · 2026-03-15T00:55:19Z

src/App.tsx

-      </form>
+  const todosWithUsers = todosFromServer.map(todo => ({
+    ...todo,
+    user: 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 for the parameter, for example, user.

mateacademy-ai-mentor · 2026-03-15T00:55:19Z

src/components/TodoInfo/TodoInfo.tsx

+    className={`TodoInfo ${todo.completed ?'TodoInfo--completed' : ''}`}
+  >
+    <h2 className="TodoInfo__title">{todo.title}</h2>
+    <UserInfo user={todo.user!}/>


Using the non-null assertion operator (!) is unsafe here because todo.user can be undefined according to its type definition. If a todo without a user is passed, this will cause a runtime error. You should conditionally render the UserInfo component only when todo.user exists. This aligns with the principle in checklist item #4, which advises against rendering a component with undefined props.

mateacademy-ai-mentor

Review Summary

Great job on this iteration! You've successfully addressed almost all the feedback from the previous review, especially the logic for handling new todo IDs and improving variable names.

I am approving this solution to help you progress, but there is one small but important point to remember for the future. In src/components/TodoForm/TodoForm.tsx, you are still using the non-null assertion operator (user: user!). While your guard clause makes this safe, it's a best practice to let TypeScript's type inference do the work. Since you already check if (!user), TypeScript knows user cannot be null afterwards, so you can safely remove the !. [CHECKLIST ITEM #4]

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-03-15T01:18:28Z

src/components/TodoForm/TodoForm.tsx

+      title:title,
+      completed:false,
+      userId:userId,
+      user:user!


While the guard clause on line 39 makes this operation safe at runtime, the non-null assertion operator (!) should be avoided. TypeScript's control flow analysis is smart enough to know that user cannot be undefined after that check, so you can safely remove the ! here.

all tests passed

53cba40

mateacademy-ai-mentor suggested changes Mar 14, 2026

View reviewed changes

the required changes was completed

6b83ec2

AntonNimachuk requested a review from mateacademy-ai-mentor March 15, 2026 00:52

mateacademy-ai-mentor suggested changes Mar 15, 2026

View reviewed changes

the new required changes include with meaningless was completed

43b1b84

AntonNimachuk requested a review from mateacademy-ai-mentor March 15, 2026 01:16

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

View reviewed changes

		<a className="UserInfo" href={`mailto:${users.find(u => u.id===userId)?.email}`}>
		{`${users.find(u => u.id===userId)?.name}`}

Conversation

AntonNimachuk commented Mar 14, 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!

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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants