Develop by rekverr · Pull Request #3558 · mate-academy/react_add-todo-form

rekverr · 2026-02-21T02:08:17Z

mateacademy-ai-mentor

Review Summary

I am requesting changes because there are still a couple of critical requirement violations that affect core functionality and checklist compliance. First, when you create a new todo in src/App.tsx, the object you push into todos only contains id, title, completed, and userId, but the task explicitly requires each todo to also contain a full user object (id, name, username, email); you should find the selected user in usersFromServer by userId and include it in the new todo. Second, validation errors are currently triggered on every input/select change (validateInput and validateSelect), which makes errors appear before the Add button is clicked, violating the requirement that errors show only after submit. To fix this, add a submit-attempt flag like const [isSubmitted, setIsSubmitted] = useState(false) in App.tsx, set it to true in the submit handler, and render title/select errors only when isSubmitted is true, while change handlers simply update values and clear their specific error flags. In TodoList.tsx, also add a guard at the top such as if (!todos || !users) return null; to satisfy the checklist about not rendering when props can be null/undefined, and ensure that when you normalize todos with users you filter out any null entries before mapping to JSX. Your TodoInfo and UserInfo components are implemented well, especially the data-id={todo.id} requirement, so once you address these blockers your solution should 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-21T02:14:27Z

src/App.tsx

+      setTodos(prevTodos => [
+        ...prevTodos,
+        {
+          id: Math.max(...prevTodos.map(todo => todo.id), 0) + 1,
+          title,
+          userId,
+          completed: false,
+        },
+      ]);


This block adds a new todo but omits the required user object. The task description requires: "each TODO item must have the following fields: ... and a user object containing: id, name, username, email". Find the user by userId (from usersFromServer) and include it in the object you push to todos.

mateacademy-ai-mentor · 2026-02-21T02:14:27Z

src/App.tsx

+  const handleInputChange = (event: React.ChangeEvent<HTMLInputElement>) => {
+    const value = event.target.value;
+
+    setTitle(value);
+    validateInput(value);


Calling validateInput on each input change causes validation errors to appear before the Add button is clicked, which violates the requirement: "Errors should appear only after clicking the Add button". Instead, track a submit-attempt flag (e.g., isSubmitted) and render the title error only when that flag is true; on change just update the value and clear the title error.

mateacademy-ai-mentor · 2026-02-21T02:14:27Z

src/App.tsx

+  const handleUserChange = (event: React.ChangeEvent<HTMLSelectElement>) => {
+    const value = Number(event.target.value);
+
+    setUserId(value);
+    validateSelect(value);
+  };


Similarly, calling validateSelect on each select change can show/clear the select error prematurely. The spec states: "Errors should appear only after clicking the Add button". Use the submit-attempt flag to control when the select error is shown; the change handler should only update userId and clear the select error.

mateacademy-ai-mentor · 2026-02-21T02:14:27Z

src/App.tsx

+  const handleSubmit = (event: React.FormEvent<HTMLFormElement>) => {
+    event.preventDefault();
+
+    const isTitleEmpty = !title;
+    const isUserNotSelected = userId === 0;
+
+    setHasErrorInput(isTitleEmpty);
+    setHasErrorSelect(isUserNotSelected);
+


The submit handler sets per-field error flags but there is no submit-attempt state to ensure errors appear only after clicking Add. Per requirement "Errors should appear only after clicking the Add button", add a state like const [isSubmitted, setIsSubmitted] = useState(false) and set it to true in this handler; then render errors only when isSubmitted is true and clear the specific error on field change.

mateacademy-ai-mentor · 2026-02-21T02:14:27Z

src/components/TodoList/TodoList.tsx

+};
+
+export const TodoList = ({ todos, users }: TodoListProps) => {
+  const normalizedTodos = todos.map(todo => {


Mapping todos here can produce null values (see the return of null when a user is not found). Consider filtering out null entries or using reduce so the normalized array contains only valid TodoWithUser items before rendering. This avoids rendering an array with null elements and keeps the render clean.

mateacademy-ai-mentor · 2026-02-21T02:14:27Z

src/components/TodoList/TodoList.tsx

+    const user = (users || []).find(item => item.id === (todo as Todos).userId);
+
+    if (!user) {
+      return null;


When a matching user is not found you return null. That causes the corresponding todo to be dropped silently. If the users prop can be undefined/missing, this will hide todos unexpectedly. Either ensure users is provided by the parent or filter/handle missing users explicitly (for example, skip the TodoList render when users is not available).

mateacademy-ai-mentor · 2026-02-21T02:14:27Z

src/components/TodoList/TodoList.tsx

+  return (
+    <section className="TodoList">


The component immediately renders the section regardless of whether users or todos exist. This violates checklist item #4: "Don't render the component if the property that you pass to the component has null or undefined value; instead, conditionally render the component only when the data exists." Add a guard such as if (!todos || !users) return null; (or check the specific prop you expect) before creating normalizedTodos.

mateacademy-ai-mentor

Review Summary

Great job addressing the previous feedback! The form validation and new todo creation logic are working perfectly. The only remaining issue is an architectural one that needs to be fixed before this can be approved.

Currently, the data normalization logic that combines todos and users is located in src/components/TodoList/TodoList.tsx. According to the project guidelines [CHECKLIST ITEM #5], this data preparation should happen in the root App component. This ensures that child components remain simple and are only responsible for presentation.

Please move this logic to App.tsx, create the list of todos with their associated user data there, and pass the fully prepared array as a prop to TodoList. This is the last change needed to meet all 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-02-26T00:34:57Z

src/components/TodoList/TodoList.tsx

+  const normalizedTodos = todos
+    .map(todo => {
+      if ('user' in todo && todo.user) {
+        return todo as TodoWithUser;
+      }
+
+      const user = (users || []).find(
+        item => item.id === (todo as Todos).userId,
+      );
+
+      if (!user) {
+        return null;
+      }
+
+      return {
+        ...(todo as Todos),
+        user,
+      } as TodoWithUser;
+    })
+    .filter((todo): todo is TodoWithUser => Boolean(todo));


This data normalization logic is well-implemented, but it violates the principle from checklist item #5, which states that data should be prepared in the App component.

To fix this, you should move this logic for combining todos and users into the App.tsx component. You can do this when you initialize your todos state. This will simplify TodoList into a purely presentational component that just receives a list of todos that already have the user information attached.

mateacademy-ai-mentor

Review Summary

Excellent work on this task! You've done a fantastic job moving the data preparation logic into the App component, which is a great architectural improvement. I am approving your solution so you can move forward, but I have a couple of small suggestions for you to consider for future projects.

To make the code even cleaner, you could define the TodoWithUser type in a single shared file (like in src/types) to avoid duplication, which follows the "Don't Repeat Yourself" principle [CHECKLIST ITEM #2]. Also, since the todos now contain all the necessary user info, the users prop is no longer needed in the TodoList component and can be removed.

These are minor refinements to an already solid solution. 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-02-26T00:44:17Z

src/components/TodoList/TodoList.tsx

+type TodoWithUser = Todos & {
+  user: Users;
+};


This TodoWithUser type is also defined in App.tsx and TodoInfo.tsx. To avoid code duplication and ensure consistency, it's better to define this type once in a shared location, like a new file in the src/types directory, and then import it wherever it's needed. This follows the "Don't repeat yourself" principle from checklist item #2.

mateacademy-ai-mentor · 2026-02-26T00:44:17Z

src/components/TodoList/TodoList.tsx

+
+type TodoListProps = {
+  todos: TodoWithUser[];
+  users?: Users[];


The users prop is not used within this component. Since all the necessary user information is already included in the todos array (as TodoWithUser), this prop is redundant and can be removed to simplify the component's interface. This will also require updating the props destructuring on line 14 and the conditional check on line 15.

mateacademy-ai-mentor · 2026-02-26T00:44:17Z

src/App.tsx

+  const normalizedTodos = todos
+    .map(todo => {
+      if ('user' in todo && todo.user) {
+        return todo as TodoWithUser;
+      }
+
+      const user = usersFromServer.find(
+        item => item.id === (todo as Todos).userId,
+      );
+
+      if (!user) {
+        return null;
+      }
+
+      return {
+        ...(todo as Todos),
+        user,
+      } as TodoWithUser;
+    })
+    .filter((todo): todo is TodoWithUser => Boolean(todo));


Great job moving the data normalization logic into the App component! This correctly follows the principle of preparing data in the root component.

To make this even better, you could perform this normalization just once when you initialize the state, instead of on every render. You could map over todosFromServer to add the user object to each todo initially, and then set that as the default state.

This would mean your todos state always contains the complete TodoWithUser data, simplifying the rest of your component. You could then remove this normalizedTodos calculation entirely and just pass the todos state directly to the TodoList component.

mateacademy-ai-mentor · 2026-02-26T00:44:17Z

src/App.tsx

-          </a>
-        </article>
-      </section>
+      <TodoList todos={normalizedTodos} users={usersFromServer} />


Since the logic to find and attach users to todos has been moved into this App component, the TodoList component no longer needs the users prop. You can safely remove it from here and from the TodoList component's definition to clean up the code.

rekverr added 2 commits February 20, 2026 03:50

finish TodoInfo, page

9054b58

finish task

bd6486d

mateacademy-ai-mentor suggested changes Feb 21, 2026

View reviewed changes

redid

a87504f

rekverr requested a review from mateacademy-ai-mentor February 26, 2026 00:33

mateacademy-ai-mentor suggested changes Feb 26, 2026

View reviewed changes

redid

162617f

rekverr requested a review from mateacademy-ai-mentor February 26, 2026 00:42

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

View reviewed changes

Conversation

rekverr commented Feb 21, 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!

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!

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