Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because there are a few critical requirement and checklist violations that affect core functionality and expected data shape. The new todo id is generated with Date.now(), but the task explicitly requires id to be the current maximum id in todos plus 1; you should compute const newId = (todos.length ? Math.max(...todos.map(todo => todo.id)) : 0) + 1 and use that. Newly created todos currently only store userId and not the full user object, while the task requires each todo to contain an embedded user with id, name, username, and email; in App.tsx, find the selected user from usersFromServer before adding the todo and attach it as user so children receive fully prepared data. In TodoInfo.tsx, you import and look up users locally, which violates [CHECKLIST ITEM #5] about preparing data in the root; remove that import and instead rely on todo.user (or a user prop) passed from App via TodoList. The form is also missing required labels and a placeholder: add a visible <label htmlFor="title"> and placeholder to the title input, and a <label htmlFor="userSelect"> for the user <select>, while keeping the existing data-cy attributes. Finally, in App.tsx your event handlers use one-letter names like e, which breaks [CHECKLIST ITEM #1]; rename them to something descriptive like event or changeEvent. Once you address these points, your solution will align well with the specification and architectural guidelines, and the rest of your implementation (validation, data-cy usage, list rendering) is on the right track.
✨ 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
src/App.tsx
Outdated
| const newTodo: Todo = { | ||
| id: Date.now(), |
There was a problem hiding this comment.
This generates the new todo id using Date.now(). The description requires: "id is the largest id in the array + 1 (add data-id={todo.id} attribute to each .TodoInfo)." Compute the id as the largest existing id + 1 (handle empty array) instead of using Date.now().
src/App.tsx
Outdated
| const newTodo: Todo = { | ||
| id: Date.now(), | ||
| title, | ||
| completed: false, | ||
| userId, | ||
| }; |
There was a problem hiding this comment.
The created todo lacks the user object. The description requires: "and a user object containing: id, name, username, email". Find the selected user and include that user object on the todo before adding it to state.
| <input | ||
| type="text" | ||
| data-cy="titleInput" | ||
| value={title} | ||
| onChange={e => { | ||
| setTitle(e.target.value); | ||
| if (titleError) { | ||
| setTitleError(false); | ||
| } | ||
| }} | ||
| /> |
There was a problem hiding this comment.
Missing label and placeholder for the title input. The description requires: "- add labels and placeholders where they are needed;" Add a visible <label> for the title and a placeholder on the input (maintain data-cy="titleInput").
| <select | ||
| data-cy="userSelect" | ||
| value={userId} | ||
| onChange={e => { | ||
| setUserId(Number(+e.target.value)); | ||
| if (userError) { | ||
| setUserError(false); | ||
| } | ||
| }} | ||
| > |
There was a problem hiding this comment.
Missing label for the user select. The description requires: "- add labels and placeholders where they are needed;" Add a <label> for the select (keep data-cy="userSelect").
src/App.tsx
Outdated
| const newTodo: Todo = { | ||
| id: Date.now(), | ||
| title, | ||
| completed: false, | ||
| userId, | ||
| }; | ||
|
|
||
| setTodos([...todos, newTodo]); |
There was a problem hiding this comment.
This code creates and adds a todo without enriching it with the user object in App. That conflicts with the checklist item: "Prepare data in one place (App component) and pass it to child components. Don't use import keyword in non-root components" — prepare todos with their user in App so child components don't need to import or look up users.
src/App.tsx
Outdated
| const handleSubmit = (e: React.FormEvent<HTMLFormElement>) => { | ||
| e.preventDefault(); | ||
|
|
||
| const isTitleInvalid = !title.trim(); | ||
| const isUserInvalid = userId === 0; | ||
|
|
||
| if (isTitleInvalid || isUserInvalid) { | ||
| setTitleError(isTitleInvalid); | ||
| setUserError(isUserInvalid); | ||
|
|
||
| return; | ||
| } | ||
|
|
||
| const newTodo: Todo = { | ||
| id: Date.now(), | ||
| title, | ||
| completed: false, | ||
| userId, | ||
| }; | ||
|
|
||
| setTodos([...todos, newTodo]); | ||
| setTitle(''); | ||
| setUserId(0); | ||
| }; | ||
|
|
||
| return ( | ||
| <div className="App"> | ||
| <h1>Add todo form</h1> | ||
|
|
||
| <form action="/api/todos" method="POST"> | ||
| <form action="/api/todos" method="POST" onSubmit={handleSubmit}> | ||
| <div className="field"> | ||
| <input type="text" data-cy="titleInput" /> | ||
| <span className="error">Please enter a title</span> | ||
| <input | ||
| type="text" | ||
| data-cy="titleInput" | ||
| value={title} | ||
| onChange={e => { | ||
| setTitle(e.target.value); | ||
| if (titleError) { | ||
| setTitleError(false); | ||
| } | ||
| }} | ||
| /> | ||
| {titleError && <span className="error">Please enter a title</span>} | ||
| </div> | ||
|
|
||
| <div className="field"> | ||
| <select data-cy="userSelect"> | ||
| <select | ||
| data-cy="userSelect" | ||
| value={userId} | ||
| onChange={e => { |
There was a problem hiding this comment.
You use one-letter event parameter names (e) in handlers. This violates [CHECKLIST ITEM #1]: "1. [CODE STYLE] - Don't use one-letter variable naming. (It could be confusing for another developer, who reads your code. Also, it would really hard to find this variable in the code editor using search)" Use descriptive parameter names like event or changeEvent.
| </a> | ||
| </article> | ||
| </section> | ||
| <TodoList todos={todos} /> |
There was a problem hiding this comment.
You're passing todos to <TodoList /> but those todos do not include the user object required by the description. Per the description and 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" — enrich todos with their user before passing them down.
src/components/TodoInfo/TodoInfo.tsx
Outdated
| @@ -1 +1,27 @@ | |||
| export const TodoInfo = () => {}; | |||
| import { UserInfo } from '../UserInfo/UserInfo'; | |||
| import usersFromServer from '../../api/users'; | |||
There was a problem hiding this comment.
This file imports users from the API in a non-root component. This violates the checklist: "Prepare data in one place (App component) and pass it to child components. Don't use import keyword in non-root components". Remove this import and let App pass the resolved user (e.g. as todo.user or a separate user prop).
src/components/TodoInfo/TodoInfo.tsx
Outdated
| const foundUser: User | undefined = usersFromServer.find( | ||
| (user: User) => user.id === todo.userId, | ||
| ); |
There was a problem hiding this comment.
Finding the user inside this component creates a hidden dependency on external data and repeats data preparation logic in a child component. Per the same checklist item — "Prepare data in one place (App component) and pass it to child components" — move the usersFromServer.find(...) logic into App and receive the user via props (or on the todo object).
src/components/TodoInfo/TodoInfo.tsx
Outdated
| > | ||
| <h2 className="TodoInfo__title">{todo.title}</h2> | ||
|
|
||
| <UserInfo user={foundUser} /> |
There was a problem hiding this comment.
You pass foundUser to UserInfo here, but the task requires that each todo includes a user object: "and a user object containing: id, name, username, email". Instead of looking up the user here, ensure App adds the user object to each todo and then render <UserInfo user={todo.user} /> (or pass user prop directly).
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am requesting changes because two explicit form/accessibility requirements from the task description are still not met, which makes the solution incomplete. In src/App.tsx, the title field’s <label htmlFor="title"> is not actually associated with the corresponding <input> because the input is missing id="title", and the same input also lacks a placeholder text (e.g., placeholder="Enter title"); both of these are required by the “add labels and placeholders where they are needed” rule. Also in src/App.tsx, the user <select> does not have a visible <label> and the <select> has no id, so users and screen readers don’t get a clear description for that control, which again violates the same requirement.
To fix this, in the title input line, add id="title" and a meaningful placeholder string so the label is properly linked and the placeholder requirement is satisfied. For the user dropdown, add a visible label like <label htmlFor="userSelect">User</label> immediately before the <select>, and add id="userSelect" on the <select> so the association is correct. Everything else about your data preparation, todo rendering, validation, and component structure looks solid, so once these label/id/placeholder issues are corrected, your implementation will fully meet the specified requirements. Keep going—you’re very close and these are straightforward accessibility fixes that will strengthen your form overall.
✨ 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
| <label htmlFor="title">Title</label> | ||
| <input | ||
| type="text" | ||
| data-cy="titleInput" | ||
| value={title} | ||
| onChange={event => { | ||
| setTitle(event.target.value); | ||
| if (titleError) { | ||
| setTitleError(false); | ||
| } | ||
| }} | ||
| /> |
There was a problem hiding this comment.
This violates the requirement: "add labels and placeholders where they are needed;" (description). The title exists (htmlFor="title") but the corresponding has no id and no placeholder. Add id="title" to the input and a placeholder (for example placeholder="Enter title") so the label is properly associated and the placeholder requirement is satisfied.
| <div className="field"> | ||
| <select data-cy="userSelect"> | ||
| <select | ||
| data-cy="userSelect" | ||
| value={userId} | ||
| onChange={event => { | ||
| setUserId(Number(event.target.value)); | ||
| if (userError) { | ||
| setUserError(false); | ||
| } | ||
| }} | ||
| > | ||
| <option value="0" disabled> |
There was a problem hiding this comment.
This violates the requirement: "add labels and placeholders where they are needed;" (description). There is no visible <label> associated with the user <select> and the <select> has no id. Add a <label htmlFor="userSelect">User</label> (or similar) and add id="userSelect" to the <select> so the label is linked to the control and the form meets the requirement.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving your solution as an educational override so you can continue progressing while learning, even though there are a couple of small things to improve. You did a strong job in App.tsx, TodoList.tsx, TodoInfo.tsx, and UserInfo.tsx: data is prepared in App, the form uses onSubmit, validation messages are shown only after submit and cleared on change, IDs are stable, and each .TodoInfo correctly renders data-id={todo.id} and the user info. One area to polish is the select’s default option value in App.tsx (around the userSelect): make sure the default option uses the same type as userId (for example value={0} if userId is a number) to avoid subtle comparison or coercion issues. Another improvement is defensive rendering in TodoInfo.tsx: before rendering <UserInfo user={todo.user} />, check that todo.user exists and render nothing or a fallback if it’s missing, which better aligns with the requirement to handle missing user data safely [CHECKLIST ITEM #4]. As an optional enhancement, you could also sanitize the title input to allow only Ukrainian/English letters, digits, and spaces before saving the todo, and replace the as User cast in App.tsx with safer typing logic. Overall, your structure, naming, and adherence to the main checklist items are solid—these suggestions are refinements that will make your code more robust and in line with best practices.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
2 participants
DEMO LINK