Develop by BiCoNn · Pull Request #3573 · mate-academy/react_add-todo-form

BiCoNn · 2026-03-09T19:08:18Z

No description provided.

Feature/prettier

add Vite

Update README.md

mateacademy-ai-mentor

Review Summary

Great job implementing the core functionality! The app works well, and the tests you've written are comprehensive. However, some changes are needed to align with the project's architectural requirements before this can be approved.

The primary issue is the data flow. The App component should be the single source of truth and prepare all data for its children [CHECKLIST ITEM #5]. Currently, the TodoInfo component fetches the user list itself. To fix this, you should first update the ToDo type in src/types/ToDo.ts to include the user object. Then, in App.tsx, when creating a new todo, you should find the full selectedUser object and add it to the newTask before updating the state.

Additionally, the form in App.tsx is missing <label> elements for the input and select fields, which is an important accessibility requirement. Please also address the use of one-letter variable names in your find methods [CHECKLIST ITEM #1].

Addressing these points will bring the solution in line with all the project guidelines. 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-09T19:11:53Z

.github/workflows/test.yml

+        node-version: [20.x]

    steps:
      - uses: actions/checkout@v2


It's a good practice to use the latest versions of GitHub Actions. actions/checkout@v2 is outdated. Consider updating to a newer version like v4 for better performance and security.

mateacademy-ai-mentor · 2026-03-09T19:11:53Z

src/App.tsx

+  const [titleError, setTitleError] = useState(false);
+
+  const handleChangeUser = (event: React.ChangeEvent<HTMLSelectElement>) => {
+    const user = users.find(u => u.username === event.target.value);


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, for example, user instead of u.

mateacademy-ai-mentor · 2026-03-09T19:11:53Z

src/App.tsx

+    if (!titleTask.trim()) {
+      setTitleError(true);
+    }
+
+    setTitle(titleTask);
+    setTitleError(false);


The logic here is a bit confusing. The error state is set to true inside the if block, but then it's immediately set back to false on the next line. This makes the if block redundant. According to the requirements, the error should be hidden on any change, so calling setTitleError(false) is sufficient here.

mateacademy-ai-mentor · 2026-03-09T19:11:53Z

src/App.tsx

+    const newTask: ToDo = {
+      id: maxId,
+      title: title,
+      completed: false,
+      userId: selectedUser!.id,
+    };


The new task object is missing the user property as required by the task description ("each TODO item must have... and a user object..."). Currently, the TodoInfo component has to import the entire list of users to find the correct one. This violates checklist item #5, which states that data should be prepared in the root component and passed down. You should add the selectedUser object to this newTask and adjust the initial todos state to also include the user object for each todo.

mateacademy-ai-mentor · 2026-03-09T19:11:53Z

src/App.tsx

+      id: maxId,
+      title: title,
+      completed: false,
+      userId: selectedUser!.id,


While there's a check for selectedUser earlier in the function, using the non-null assertion operator (!) is generally considered a bad practice as it can hide potential bugs. Since you've already confirmed selectedUser is not null, you can safely access selectedUser.id without the assertion.

mateacademy-ai-mentor · 2026-03-09T19:11:53Z

src/App.tsx

+          <select
+            data-cy="userSelect"
+            value={selectedUser?.username ?? '0'}
+            onChange={handleChangeUser}
+          >


This select element is also missing an associated <label> element. All form controls should have labels for better accessibility.

mateacademy-ai-mentor · 2026-03-09T19:11:53Z

src/components/TodoInfo/TodoInfo.tsx

-export const TodoInfo = () => {};
+import { UserInfo } from '../UserInfo';
+import ToDo from '../../types/ToDo';
+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'. The TodoInfo component should receive a todo object that already includes the full user object, rather than looking it up here.

mateacademy-ai-mentor · 2026-03-09T19:11:53Z

src/components/TodoInfo/TodoInfo.tsx

+};
+
+export const TodoInfo = ({ todo }: Props) => {
+  const taskUser = users.find(u => u.id === todo.userId);


This logic for finding the user belongs in the parent App component, as stated in checklist item #5. Furthermore, this violates checklist item #1: 'Don't use one-letter variable naming.' The parameter u should be renamed to something more descriptive, like user.

mateacademy-ai-mentor · 2026-03-09T19:11:53Z

src/components/TodoInfo/TodoInfo.tsx

+      {taskUser ? (
+        <article
+          data-id={todo.id}
+          className={`TodoInfo ${todo.completed ? 'TodoInfo--completed' : ''}`}
+        >
+          <h2 className="TodoInfo__title">{todo.title}</h2>
+
+          <UserInfo name={taskUser.name} email={taskUser.email} />
+        </article>
+      ) : (
+        ''
+      )}


This conditional rendering violates the principle described in checklist item #4. The parent component should be responsible for ensuring that TodoInfo receives a valid todo object with a user. This component should not have to handle the case where taskUser might not be found. The check for a valid user should happen before this component is rendered.

mateacademy-ai-mentor · 2026-03-09T19:11:53Z

src/types/ToDo.ts

+type ToDo = {
+  id: number;
+  title: string;
+  completed: boolean;
+  userId: User['id'];
+};


The task description explicitly requires each ToDo item to have a user object containing the user's details. This type is missing the user: User; property. Adding it is a crucial first step to align with the application's data structure and fix the architectural issue in TodoInfo.tsx.

mateacademy-ai-mentor

Review Summary

Great work on the form logic and state management. However, this submission requires changes because several critical requirements from the task description and previous feedback have not yet been met.

The most important issue is the application's data flow. The App component must be the single source of truth, but the TodoInfo component is currently fetching its own data by importing the users list. This violates a core architectural guideline [CHECKLIST ITEM #5].

To resolve this, you'll need to:

Update the ToDo type in src/types/ToDo.ts to include the full user: User object.
In App.tsx, when creating a new task, find the complete selectedUser object and add it to the newTask.
Remove the data import and user-finding logic from TodoInfo.tsx so it only displays the data passed via props.

Additionally, please remember to add the required <label> elements to the form in App.tsx for accessibility.

✨ 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-09T19:36:55Z

src/App.tsx

+  const [titleError, setTitleError] = useState(false);
+
+  const handleChangeUser = (event: React.ChangeEvent<HTMLSelectElement>) => {
+    const user = users.find(u => u.username === event.target.value);


This violates checklist item #1: 'Don't use one-letter variable naming.' Please use a more descriptive variable name like user instead of u. This was also noted in the previous review and is important for code readability.

mateacademy-ai-mentor · 2026-03-09T19:36:55Z

src/App.tsx

+  const handleChangeTitle = (event: React.ChangeEvent<HTMLInputElement>) => {
+    const titleTask = event.target.value;
+
+    if (!titleTask.trim()) {
+      setTitleError(true);
+    }
+
+    setTitle(titleTask);
+    setTitleError(false);
+  };


This handler function works, but the logic is a bit confusing. The if block on lines 35-37 sets an error that is immediately cleared by setTitleError(false) on line 40. Since the requirement is to show errors only on submission and hide them on input change, you can simplify this function by removing lines 35-37.

mateacademy-ai-mentor · 2026-03-09T19:36:55Z

src/App.tsx

+    const newTask: ToDo = {
+      id: maxId,
+      title: title,
+      completed: false,
+      userId: selectedUser!.id,
+    };


This is the central part that needs adjustment based on the previous review. The task requires each new todo to have a complete user object.

Currently, you're only adding userId. To fix this:

First, update the ToDo type in src/types/ToDo.ts to include a user: User property.

Then, here in App.tsx, add the selectedUser object to newTask: user: selectedUser.

This will make the App component the single source of truth and will allow you to remove the user lookup logic from the TodoInfo component, satisfying checklist item #5.

mateacademy-ai-mentor · 2026-03-09T19:36:55Z

src/App.tsx

      <h1>Add todo form</h1>

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


The form is missing its corresponding <label> elements for the input and select fields. This is an important accessibility requirement and was part of the feedback from the previous review. Please add labels for both form controls.

mateacademy-ai-mentor · 2026-03-09T19:36:55Z

src/components/TodoInfo/TodoInfo.tsx

-export const TodoInfo = () => {};
+import { UserInfo } from '../UserInfo';
+import ToDo from '../../types/ToDo';
+import users from '../../api/users';


This violates checklist item #5. Non-root components like TodoInfo should not import data sources directly. All data should be prepared in the App component and passed down as props. This was a critical point in the previous review feedback and needs to be addressed.

mateacademy-ai-mentor · 2026-03-09T19:36:55Z

src/components/TodoInfo/TodoInfo.tsx

+};
+
+export const TodoInfo = ({ todo }: Props) => {
+  const taskUser = users.find(u => u.id === todo.userId);


This logic for finding the user should be in the App component. The todo object passed to this component should already contain the complete user object. Additionally, this violates checklist item #1 by using the one-letter variable u.

mateacademy-ai-mentor · 2026-03-09T19:36:55Z

src/types/ToDo.ts

+type ToDo = {
+  id: number;
+  title: string;
+  completed: boolean;
+  userId: User['id'];
+};


This ToDo type is missing the required user object property. The task description and previous review feedback specified that each todo should contain the full user object. Please add user: User; to this type definition.

mateacademy-ai-mentor

Review Summary

I am approving your solution as an educational override because you've done an excellent job refactoring the application's architecture! The data flow is now much cleaner with App as the single source of truth, which was the main goal of this iteration.

While your work is approved, here are a few key areas to focus on for future tasks:

Type Safety: In src/types/ToDo.ts, the user property on the ToDo type should be required (user: User) instead of optional. Since your app logic always includes a user, making the type stricter prevents potential bugs.
Code Clarity: The if block within the handleChangeTitle function in src/App.tsx is redundant and can be safely removed to simplify the code.
Naming Conventions: Please avoid one-letter variable names like u. Using descriptive names like user makes the code much easier to read and maintain. [CHECKLIST ITEM #1]

Keep up the great work, you're making fantastic progress!

✨ 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-11T04:21:51Z

.github/workflows/test.yml

      - name: Upload tests report(cypress mochaawesome merged HTML report)
        if: ${{ always() }}
-        uses: actions/upload-artifact@v2
+        uses: actions/upload-artifact@v4


There seems to be a mismatch in the artifact path. According to cypress.config.ts, the test reports are generated in the raw_reports directory. However, this step attempts to upload from a reports directory (defined on line 29), which likely doesn't exist. Please update the path to match your Cypress configuration.

mateacademy-ai-mentor · 2026-03-11T04:21:51Z

src/App.tsx

+  const [titleError, setTitleError] = useState(false);
+
+  const handleChangeUser = (event: React.ChangeEvent<HTMLSelectElement>) => {
+    const user = users.find(u => u.username === event.target.value);


This violates checklist item #1: "Don't use one-letter variable naming". Please rename u to a more descriptive name like user to improve code readability.

mateacademy-ai-mentor · 2026-03-11T04:21:51Z

src/App.tsx

+    if (!titleTask.trim()) {
+      setTitleError(true);
+    }


This if block is redundant. The call to setTitleError(true) on line 40 has no effect because setTitleError(false) is called unconditionally on line 44 right after it. You can safely remove this if statement, as the requirement is only to hide the error on change, which line 44 already accomplishes.

mateacademy-ai-mentor · 2026-03-11T04:21:51Z

src/App.tsx

      <h1>Add todo form</h1>

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


The action and method attributes are not necessary for a form handled entirely by React on the client side. The onSubmit handler in React (addTaskHandler) prevents the default browser form submission, so these attributes will never be used. You can remove them for cleaner code.

mateacademy-ai-mentor · 2026-03-11T04:21:52Z

src/App.tsx

+          <input
+            type="text"
+            data-cy="titleInput"
+            value={title}
+            onChange={handleChangeTitle}
+            placeholder="Enter title of task"
+          />


To improve accessibility and provide a better user experience, it would be beneficial to add an id attribute to this input that matches the htmlFor in the associated <label>.

mateacademy-ai-mentor · 2026-03-11T04:21:52Z

src/App.tsx

+          <select
+            data-cy="userSelect"
+            value={selectedUser?.username ?? '0'}
+            onChange={handleChangeUser}
+          >


To improve accessibility, please add an id attribute to this <select> element that matches the htmlFor attribute of its corresponding <label>.

mateacademy-ai-mentor · 2026-03-11T04:21:52Z

src/components/TodoInfo/TodoInfo.tsx

+};
+
+export const TodoInfo = ({ todo }: Props) => {
+  // const taskUser = users.find(u => u.id === todo.userId);


This line is no longer needed and can be removed. Great job on refactoring this component to rely on props instead of fetching its own data!

mateacademy-ai-mentor · 2026-03-11T04:21:52Z

src/types/ToDo.ts

+  title: string;
+  completed: boolean;
+  userId: User['id'];
+  user?: User;


According to the task description, every todo item must have a user object. The logic in your App component also ensures this is always the case. To align the type with the requirements and the actual data structure, please make the user property non-optional by removing the ?.

solaryasha and others added 22 commits January 3, 2023 17:42

Delete checklist.md

7e06e9a

Rename newChecklist.md to checklist.md

a11c9dc

small fixes

49b7280

fix linter and run all tests

88f815e

Update App.tsx

33e037a

Update README.md

d3a183a

Update package.json

c876e32

Update package.json

ed68295

fix deps

670edf3

feature/prettier added prettier, updated packages

3d8f0f2

feature/prettier updated readme.md

89c060d

Merge pull request mate-academy#2356 from mykhalenych/feature/prettier

015c29a

Feature/prettier

fix node14 deps

b7581e6

fix package versions

651e4d3

fix lock versions

6c9e7ef

add Vite

ba9d778

remove unused deps

f3a3537

Merge pull request mate-academy#2669 from mate-academy/vite-infra

515ab0f

add Vite

Update test.yml

8d91581

Update README.md

cc81ca9

Merge pull request mate-academy#2922 from mykhalenych/patch-1

c4d03d0

Update README.md

sol

4634d98

mateacademy-ai-mentor suggested changes Mar 9, 2026

View reviewed changes

sol2

b855d5d

BiCoNn requested a review from mateacademy-ai-mentor March 9, 2026 19:33

mateacademy-ai-mentor suggested changes Mar 9, 2026

View reviewed changes

BiCoNn added 3 commits March 9, 2026 21:26

sol3

4cfe53e

sol4

645652d

sol5

59e5cd5

BiCoNn requested a review from mateacademy-ai-mentor March 11, 2026 04:18

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

View reviewed changes

Conversation

BiCoNn commented Mar 9, 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!

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!

7 participants