Develop by AngelaStrechen · Pull Request #3569 · mate-academy/react_add-todo-form

AngelaStrechen · 2026-03-05T12:46:12Z

Feature/prettier

add Vite

Update README.md

mateacademy-ai-mentor

Review Summary

I am requesting changes as there are a few critical issues that need to be addressed. While the core functionality works well, the solution misses several key requirements.

First, in src/App.tsx, you are duplicating the rendering logic instead of using the TodoList component as required. Please delegate rendering the list to <TodoList todos={todos} />. Second, the form inputs are missing their required <label> elements, which is an important accessibility feature. Additionally, several checklist items were violated, such as redefining types instead of importing them [CHECKLIST ITEM #2] and using single-letter variable names [CHECKLIST ITEM #1]. Finally, the CI workflow in .github/workflows/test.yml is misconfigured and will fail to upload test reports due to an incorrect directory path.

Please fix these issues and resubmit. You're 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.

mateacademy-ai-mentor · 2026-03-05T12:49:25Z

.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
        with:
          name: report
          path: reports


This step is likely to fail. Your cypress.config.ts specifies the reportDir as raw_reports, but this action attempts to upload artifacts from a directory named reports (on line 29). Please ensure the path here matches the output directory from your test reporter.

mateacademy-ai-mentor · 2026-03-05T12:49:25Z

src/App.tsx

+  const [titleError, setTitleError] = useState(false);
+  const [userError, setUserError] = useState(false);
+
+  const handleSubmit = (e: React.FormEvent) => {


This violates checklist item #1: 'Don't use one-letter variable naming.' Please use a more descriptive name for the event object, like event. This also applies to the use of u as a variable name on lines 37 and 100, and e on lines 64 and 77.

mateacademy-ai-mentor · 2026-03-05T12:49:25Z

src/App.tsx

+          <input
+            type="text"
+            data-cy="titleInput"
+            placeholder="Enter a title"
+            value={title}
+            onChange={e => {
+              setTitle(e.target.value);
+              setTitleError(false);
+            }}
+          />


This input field is missing a corresponding <label> element. The task description requires adding labels, which are important for accessibility and a better user experience.

mateacademy-ai-mentor · 2026-03-05T12:49:25Z

src/App.tsx

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


This select field is missing a corresponding <label> element. The task requires adding labels where needed for better accessibility.

mateacademy-ai-mentor · 2026-03-05T12:49:25Z

src/App.tsx

      <section className="TodoList">
-        <article data-id="1" className="TodoInfo TodoInfo--completed">
-          <h2 className="TodoInfo__title">
-            delectus aut autem
-          </h2>
-
-          <a className="UserInfo" href="mailto:Sincere@april.biz">
-            Leanne Graham
-          </a>
-        </article>
-
-        <article data-id="15" className="TodoInfo TodoInfo--completed">
-          <h2 className="TodoInfo__title">delectus aut autem</h2>
-
-          <a className="UserInfo" href="mailto:Sincere@april.biz">
-            Leanne Graham
-          </a>
-        </article>
-
-        <article data-id="2" className="TodoInfo">
-          <h2 className="TodoInfo__title">
-            quis ut nam facilis et officia qui
-          </h2>
-
-          <a className="UserInfo" href="mailto:Julianne.OConner@kory.org">
-            Patricia Lebsack
-          </a>
-        </article>
+        {todos.map(todo => {
+          const user = usersFromServer.find(u => u.id === todo.userId);
+
+          return (
+            <article
+              key={todo.id}
+              data-id={todo.id}
+              className={`TodoInfo ${
+                todo.completed ? 'TodoInfo--completed' : ''
+              }`}
+            >
+              <h2 className="TodoInfo__title">{todo.title}</h2>
+
+              {user && (
+                <a className="UserInfo" href={`mailto:${user.email}`}>
+                  {user.name}
+                </a>
+              )}
+            </article>
+          );
+        })}
      </section>


This section duplicates the rendering logic that should be handled by the TodoList component. The task requires creating an App component that displays the todos using the TodoList component. You should remove this logic and instead render <TodoList todos={todos} />.

mateacademy-ai-mentor · 2026-03-05T12:49:25Z

src/App.tsx

-          </a>
-        </article>
+        {todos.map(todo => {
+          const user = usersFromServer.find(u => u.id === todo.userId);


This find operation is executed on every render for every single todo item, which is inefficient. This violates checklist item #5, which states that data should be prepared in the root component. You should enrich the todosFromServer data with the full user object once when you initialize your state, rather than looking it up during rendering.

mateacademy-ai-mentor · 2026-03-05T12:49:25Z

src/components/TodoInfo/TodoInfo.tsx

+type User = {
+  id: number;
+  name: string;
+  username: string;
+  email: string;
+};
+
+type Todo = {
+  id: number;
+  title: string;
+  completed: boolean;
+  userId: number;
+  user: User;
+};


This violates checklist item #2: "Don't repeat yourself." The User and Todo types are already defined in src/types/Todo.ts. Instead of redefining them here, you should import them from that file to ensure consistency and avoid code duplication.

mateacademy-ai-mentor · 2026-03-05T12:49:25Z

src/components/UserInfo/UserInfo.tsx

+type User = {
+  id: number;
+  name: string;
+  username: string;
+  email: string;
+};


This violates checklist item #2: "Don't repeat yourself." The User type is already defined in src/types/Todo.ts. Instead of redefining it here, you should import it from the shared types file to maintain a single source of truth for your data structures.

mateacademy-ai-mentor

Review Summary

Excellent work on implementing the required features! The application is fully functional, and you've addressed most of the previous feedback. However, a few changes are needed before this can be merged.

High Priority: The CI workflow will fail to upload test reports. The path in cypress.config.ts is set to raw_reports, but the workflow in .github/workflows/test.yml looks for reports. Please make these paths consistent to fix the build.
Code Style: In src/components/TodoInfo/TodoInfo.spec.tsx and src/components/UserInfo/UserInfo.spec.tsx, you are repeating test data. Please define it once and reuse it to follow the DRY principle [CHECKLIST ITEM #2].
Code Style: The test file cypress/integration/page.spec.js uses a single-letter variable e. Please rename it to something more descriptive like error or err [CHECKLIST ITEM #1].

Once you've addressed these points, this will be ready for approval. Great job so far!

✨ 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-06T14:13:44Z

.github/workflows/test.yml

+        uses: actions/upload-artifact@v4
        with:
          name: report
          path: reports


This artifact upload step seems to have an incorrect path. The path is set to reports, but your cypress.config.ts specifies raw_reports as the output directory. Please verify where your final test report is generated and update the path accordingly.

mateacademy-ai-mentor · 2026-03-06T14:13:44Z

cypress.config.ts

+  screenshotOnRunFailure: true,
+  reporter: 'mochawesome',
+  reporterOptions: {
+    reportDir: 'raw_reports',


The test reports are configured to be generated in the raw_reports directory. However, your CI workflow in .github/workflows/test.yml is configured to look for reports in a directory named reports. This mismatch will prevent the test reports from being uploaded correctly. Please make sure the paths are consistent.

mateacademy-ai-mentor

Review Summary

Great job on fixing the critical CI issue and improving your test code! Your application is working well, and I'm happy to approve this submission to help you move forward.

For future reference, there are a couple of areas for improvement:

Don't Repeat Yourself (DRY) [CHECKLIST ITEM #2]: The test data in src/components/TodoInfo/TodoInfo.spec.tsx and src/components/UserInfo/UserInfo.spec.tsx is duplicated. A best practice is to move this mock data into a shared file and import it into your tests.
Performance: In src/App.tsx, the preparedTodos variable is recalculated on every render. Since the data doesn't change, you can move this calculation outside the component so it only runs once.

Keep up the excellent work and focus on these points in your next tasks!

✨ 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-06T16:10:32Z

src/App.tsx

+  const preparedTodos: Todo[] = todosFromServer.map(todo => {
+    const user = usersFromServer.find(userItem => userItem.id === todo.userId)!;
+
+    return {
+      ...todo,
+      user,
+    };
+  });


This preparedTodos variable is recalculated on every render of the App component, which is unnecessary since the source data (todosFromServer and usersFromServer) is static. For better performance, you should compute this value only once by moving this logic outside the component body.

mateacademy-ai-mentor · 2026-03-06T16:10:32Z

src/components/TodoInfo/TodoInfo.spec.tsx

+const user1 = {
+  id: 1,
+  name: 'Leanne Graham',
+  username: 'Bret',
+  email: 'Sincere@april.biz',
+};
+
+const user2 = {
+  id: 2,
+  name: 'Ervin Howell',
+  username: 'Antonette',
+  email: 'Shanna@melissa.tv',
+};
+
+const user3 = {
+  id: 3,
+  name: 'Clementine Bauch',
+  username: 'Samantha',
+  email: 'Nathan@yesenia.net',
+};


While it's good that you've defined the test data here, this same data (like user1 and user2) is also defined in src/components/UserInfo/UserInfo.spec.tsx. This violates the DRY principle mentioned in checklist item #2.

To fix this, consider creating a single file for your mock test data (e.g., in a fixtures or test-data directory) and import it into both test files.

mateacademy-ai-mentor · 2026-03-06T16:10:32Z

src/components/UserInfo/UserInfo.spec.tsx

+const user1 = {
+  id: 1,
+  name: 'Leanne Graham',
+  username: 'Bret',
+  email: 'Sincere@april.biz',
+};
+
+const user2 = {
+  id: 2,
+  name: 'Ervin Howell',
+  username: 'Antonette',
+  email: 'Shanna@melissa.tv',
+};


This test data (user1 and user2) is also defined in src/components/TodoInfo/TodoInfo.spec.tsx. This repetition violates checklist item #2 (DRY principle).

To fix this, please create a shared file for this mock data (e.g., in a fixtures or test-data directory) and import it into all test files where it's needed.

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

solution

7c3c08e

mateacademy-ai-mentor suggested changes Mar 5, 2026

View reviewed changes

fix: use TodoList component and remove duplicated types

484e877

AngelaStrechen requested a review from mateacademy-ai-mentor March 6, 2026 14:10

mateacademy-ai-mentor suggested changes Mar 6, 2026

View reviewed changes

fix: CI report path and improve tests code style

ebfde20

AngelaStrechen requested a review from mateacademy-ai-mentor March 6, 2026 16:07

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

View reviewed changes

Conversation

AngelaStrechen commented Mar 5, 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!

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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants