Conversation
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great work on implementing the core features of the todo application! The component structure is solid, and you've correctly handled the state management for individual todos, including loading and error states.
However, this submission requires changes because a core requirement was not met. Several callback functions pass the entire todo object from child components to the parent, which violates the guideline to only pass prepared data. [CHECKLIST ITEM #2]
For example, in TodoItem.tsx, the onToggle and onUpdate props should be refactored to pass only the necessary data (e.g., (id: number, data: { completed: boolean })) instead of the whole todo object. This pattern should be applied across the component tree to improve decoupling.
Additionally, please consider optimizing the bulk operations in App.tsx (onDeleteCompletedTodos and onToggleAll). Currently, they create multiple API requests and re-renders inside a loop. Using Promise.all to handle these requests concurrently would be much more efficient.
Please address these points to finalize your 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
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job refactoring the onToggle callback! However, this submission requires changes because a critical requirement from the previous review has not been fully addressed. The onUpdate callback, used for editing a todo's title, still passes the entire todo object from TodoItem.tsx up to App.tsx.
This violates a core requirement to only pass prepared data from child to parent components [CHECKLIST ITEM #2]. Please refactor the handleUpdateTodo function in App.tsx to accept only the todo's ID and the new data, like (id: number, data: { title: string }), and update the props in TodoList.tsx and TodoItem.tsx accordingly.
Additionally, the functions for deleting completed todos and toggling all todos are inefficient. Please refactor them to use Promise.all to send API requests concurrently and update the state only once after all requests are complete. Addressing these points will finalize your 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
mateacademy-ai-mentor
left a comment
There was a problem hiding this comment.
Review Summary
Great job on this submission! I am approving your solution as an educational override so you can keep moving forward, but please take a moment to review the feedback on the failing test. You've done excellent work implementing concurrent API requests with Promise.all for bulk operations, which is a fantastic improvement! However, the tests uncovered an edge case: if one API call in the batch fails, Promise.all rejects entirely, preventing the UI from updating for the successful operations. A great way to solve this is by using Promise.allSettled, which allows you to handle the results of each promise individually, even when some fail. Keep up the great work!
✨ Please rate this review in the Platform's notification center1. I will be grateful for your feedback! ✨
DEMO LINK