Conversation
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on implementing the form and its validation logic! The application works as expected, but there's one important issue that needs to be addressed before I can approve it.
In src/App.tsx, the findUser(todo.userId) function is called twice for every todo item inside your map callback. This is inefficient and violates a key principle from the project checklist: Don't Repeat Yourself [CHECKLIST ITEM #2]. Please refactor this by calling the function once per todo, storing the result in a variable, and then reusing that variable.
Once that's fixed, this will be good to go!
✨ 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
| onSubmit={event => { | ||
| onSubmit(event); | ||
| }} |
There was a problem hiding this comment.
This arrow function is a bit redundant. You can simplify the code by passing the onSubmit function reference directly, like this: onSubmit={onSubmit}.
src/App.tsx
Outdated
| href={`mailto:${findUser(todo.userId)?.email}`} | ||
| > | ||
| {findUser(todo.userId)?.name} |
There was a problem hiding this comment.
This violates checklist item #2: 'Don't repeat yourself'. The findUser(todo.userId) function is called here and again on line 144 with the same argument. It's more efficient to call this function once per todo inside the map callback, store the result in a variable, and then use that variable to get the user's email and name.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job addressing the feedback from the last review! The code is much more efficient now.
This requires one change before it can be approved. In src/App.tsx, a broken link is created if a todo's assigned user can't be found. Currently, the href attribute becomes mailto:undefined, which is not a functional link.
To fix this, please conditionally render the <a> tag only when a user object is available. This will prevent broken links and aligns with the best practice of not rendering components with undefined values [CHECKLIST ITEM #4].
Once that's fixed, this will be ready to go. Keep up the great work!
✨ 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
| <a className="UserInfo" href={`mailto:${user?.email}`}> | ||
| {user?.name} | ||
| </a> |
There was a problem hiding this comment.
While optional chaining (?.) prevents the app from crashing if user is undefined, it can lead to incorrect output. In this case, the href would become mailto:undefined, which is a broken link, and the link text would be empty. It's better to conditionally render the <a> tag only when a user is actually found.
This aligns with checklist item #4, which advises against rendering parts of a component with undefined values.
mateacademy-ai-mentor
left a comment
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
I am approving this solution as an educational override so you can move forward, but please take a moment to review this important feedback.
Excellent work fixing the broken link from the previous review! Your conditional rendering logic is now implemented correctly.
The final improvement needed relates to performance. In src/App.tsx, you're calling findUser inside the map function, which means it runs for every todo on every single re-render. To optimize this, you should prepare your data once when you initialize the state. You can .map() over the todosFromServer array to add the corresponding user object to each todo, just as you're already doing for new todos. This makes your data structure consistent and your component more efficient.
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
Footnotes
| </a> | ||
| </article> | ||
| {todos.map(todo => { | ||
| const user = findUser(todo.userId); |
There was a problem hiding this comment.
Calling findUser inside the map function used for rendering is inefficient because it has to find the user for every todo on every single re-render.
A better approach is to prepare the todos data with the associated user object only once. You are already doing this correctly for new todos on line 67. You should apply the same logic to the initial todosFromServer when you initialize the state.
This aligns with checklist item #5's principle of preparing data before rendering. After preparing the data, you can simply access todo.user here.
2 participants
DEMO LINK