Skip to content

add solution#3555

Open
0-nira-0 wants to merge 3 commits intomate-academy:masterfrom
0-nira-0:dev
Open

add solution#3555
0-nira-0 wants to merge 3 commits intomate-academy:masterfrom
0-nira-0:dev

Conversation

@0-nira-0
Copy link

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because there are still a few critical requirement violations that block this task from being complete.

Critical issues to fix before this can pass:

  1. In src/App.tsx, the ToDo type is missing a user field, even though the task requires every todo to have a full user object (id, name, username, email). Update the type (e.g. user: User) and, when creating a new todo, find the selected user in usersFromServer and include that user object in the todo you push into state. This will also let you remove the user lookup from child components.
  2. In src/App.tsx, your ID helper (findBiggestNumber) currently uses todos.sort(...), which mutates the array and assumes it’s non-empty. Replace this with a non-mutating, safe approach such as const maxId = todos.reduce((max, t) => Math.max(max, t.id), 0); so that it works even when the list starts empty and doesn’t mutate React state.
  3. In src/components/TodoInfo/TodoInfo.tsx, you import usersFromServer and resolve the user inside the component, which violates the checklist rule to prepare data only in App and not import data in non-root components [CHECKLIST ITEM #5]. After you attach the user in App, change TodoInfo to receive the already-prepared user via todo.user (or via props) and remove the usersFromServer import and lookup logic.

Once you fix these, your existing form logic, validation, and list rendering look solid, and the solution will align well with the project requirements. You’re very close — focus on centralizing data preparation in App, attaching the user object correctly, and making the ID generation helper safe and non-mutating.


✨ Please rate this review in the Platform's notification center1. 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 button2 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 tag3.
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.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am requesting changes because there is a critical mismatch with the task requirement that every TODO must include a full user object (id, name, username, email). In src/App.tsx, the ToDo type currently allows user: User | undefined, and todos are created using usersFromServer.find(...), which can return undefined; this means some todos in visibleToDos can end up with user === undefined. This breaks the requirement that each todo always has a user object and also violates the checklist rule about not passing null/undefined props into child components (e.g. passing such todos into <TodoList /> and then into TodoInfo / UserInfo).

The key fixes you need to make are:

  • Change the ToDo type so that user is non-optional, e.g. user: User, and update any related typings accordingly.
  • When building initial todos and when adding a new todo, ensure that you guarantee a matching user exists (e.g. call find, check the result, and only create the todo if a User was found; otherwise handle it as an error case) so that todos stored in state and passed to children always contain a real User object.
    Once you enforce this invariant, both the requirement about the user object and the checklist rule about non-null props will be satisfied. Aside from this data/typing issue, the overall structure (form handling, validation, TodoList/TodoInfo/UserInfo composition) looks solid, so you are very close to a complete solution.

✨ Please rate this review in the Platform's notification center1. 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 button2 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 tag3.
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.

Footnotes

  1. Rate AI review example

  2. Re-request AI review example

  3. Add AI review comments example

Copy link

@mateacademy-ai-mentor mateacademy-ai-mentor left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Review Summary

I am approving your solution as an educational override so you can keep progressing while you address a few remaining polish items.

You implemented the core requirements well: todos are added correctly with full user objects, IDs are computed as max+1, validation messages behave as specified, and the list is rendered cleanly via TodoListTodoInfoUserInfo.
There are a few checklist-related issues to fix next time: in src/components/TodoList/TodoList.tsx, the component is typed as React.FC<Props> but React (or the FC type) is not imported, which can cause type errors; in src/App.tsx, the initial state uses an unnecessary spread (useState([...usersToDos])) instead of useState(usersToDos) [CHECKLIST ITEM #3]; the reducer uses a one-letter variable t instead of a descriptive name like todo [CHECKLIST ITEM #1]; and the default select option uses a string value ("0") while userId is a number, so aligning them to the same type will avoid subtle bugs.
These are all relatively small, style/consistency issues rather than functional blockers, and fixing them will make your code more robust and professional.
Overall, your structure, data flow, and handling of validation show a solid understanding of React and TypeScript; keep up this level of clarity while tightening up the checklist details and you’ll be in great shape.


✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨

Footnotes

  1. Rate AI review example

todos: ToDo[];
};

export const TodoList: React.FC<Props> = ({ todos }) => {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Potential type/symbol error: the component is typed as React.FC<Props> but React is not imported in this file. Import React or the FC type to make the annotation explicit. For example: import React from 'react'; or import type { FC } from 'react'; and then use export const TodoList: FC<Props> = ({ todos }) => { ... }. This prevents issues in projects where React is not available as a global type.


const [title, setTitle] = useState('');
const [userId, setUserId] = useState(0);
const [visibleToDos, setVisibleToDos] = useState([...usersToDos]);

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates checklist item #3: "3. [CODE KNOWLEDGE] - If you are using a non-mutating array method, you don't need to create a copy of the array". You create an unnecessary copy when initializing state: useState([...usersToDos]). Use useState(usersToDos) instead to avoid an unneeded spread copy. Suggest changing the initialization at this line to const [visibleToDos, setVisibleToDos] = useState(usersToDos);

return [
...previousToDos,
{
id: previousToDos.reduce((max, t) => Math.max(max, t.id), 0) + 1,

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This violates checklist item #1: "1. [CODE STYLE] - Don't use one-letter variable naming." The reducer uses a one-letter variable t in previousToDos.reduce((max, t) => Math.max(max, t.id), 0). Use a descriptive name such as todo or item to satisfy the checklist and improve readability.

setUserIdError(false);
}}
>
<option value="0" disabled>

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Minor correctness issue: the default option uses a string value "0" while the select's value is the numeric userId (number). This mismatch may cause unexpected behavior. Use a numeric value for the default option (e.g. value={0}) so it consistently matches the numeric state, or make the controlled value a string consistently. Update the default option declaration on this line.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants