task complete #1066
task complete #1066
Conversation
There was a problem hiding this comment.
Great job on the code! 🎉 Despite a few areas for improvement, none of them significantly impact functionality, security, or fail to meet the core requirements of the task. Keep up the good work and consider the feedback for future improvements. Happy coding! 😊
✨ Please rate this review in the Platform's notification center. I will be grateful for your feedback! ✨
| getTodos() | ||
| .then(todosFromServer => { | ||
| dispatch(setTodos(todosFromServer)); | ||
| }) | ||
| .catch(error => { | ||
| throw new Error(error); | ||
| }) | ||
| .finally(() => { | ||
| setIsLoading(false); | ||
| dispatch(setStatus('all')); | ||
| }); |
There was a problem hiding this comment.
Error handling: You're throwing an error when the promise from getTodos() is rejected, but you're not catching it anywhere. This could potentially crash your application. Consider handling this error in a user-friendly way, for instance, by showing an error message to the user.
| const preparedTodos = todos | ||
| .filter(todo => todo.title.toLowerCase().includes(query.toLowerCase())) | ||
| .filter(todo => { | ||
| switch (status) { | ||
| case StatusType.Active: | ||
| return !todo.completed; | ||
|
|
||
| case StatusType.Completed: | ||
| return todo.completed; | ||
|
|
||
| default: | ||
| return true; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Performance issue: You're filtering your todos twice. This could be inefficient if your list of todos is very large. Consider combining these two filters into one to improve performance.
| useEffect(() => { | ||
| setIsLoading(true); | ||
|
|
||
| getTodos() | ||
| .then(todosFromServer => { | ||
| dispatch(setTodos(todosFromServer)); | ||
| }) | ||
| .catch(error => { | ||
| throw new Error(error); | ||
| }) | ||
| .finally(() => { | ||
| setIsLoading(false); | ||
| dispatch(setStatus('all')); | ||
| }); | ||
| }, [dispatch]); |
There was a problem hiding this comment.
Dependency array in useEffect: You've added dispatch to the dependency array of your useEffect. However, since dispatch does not change during the lifecycle of your component, it is not necessary to include it in the dependency array. You can safely remove it.
| const showOptions = Object.values(StatusType).map(stat => ( | ||
| <option key={stat} value={stat}> | ||
| {stat[0].toUpperCase() + stat.slice(1)} | ||
| </option> | ||
| )); |
There was a problem hiding this comment.
The showOptions constant is re-calculated on each render of the component. It doesn't depend on any props or state, so you could move it outside of the component to avoid unnecessary calculations. For example, you can define it in the module scope, just after imports.
| )); | ||
|
|
||
| const handleChangeStatus = (event: React.ChangeEvent<HTMLSelectElement>) => { | ||
| dispatch(setStatus(event.target.value as Status)); |
There was a problem hiding this comment.
You are casting the event target value to Status type. It would be safer to check if the value is actually a valid Status before dispatching the action. This will help prevent any unexpected values from being dispatched to the state.
| </td> | ||
|
|
||
| <td className="is-vcentered is-expanded"> | ||
| <p |
There was a problem hiding this comment.
The use of the 'cn' library here seems unnecessary. You could directly write the ternary operator inside the className property. It would make the code cleaner and easier to read.
| </tr> | ||
| </tbody> | ||
| </table> | ||
| const showTodos = todos.map(todo => ( |
There was a problem hiding this comment.
It's not a good practice to perform heavy computations, like mapping over an array, directly in the component body. This will cause the computation to run on every render, even if todos has not changed. Instead, you could use the useMemo hook to memoize the result. This way, the computation will only rerun when todos changes.
| </p> | ||
| getUser(userId) | ||
| .then(setUser) | ||
| .catch(error => { |
There was a problem hiding this comment.
You are not handling the error from the getUser function properly. Throwing an error inside a promise .catch() handler will not stop the execution of the program, but will simply generate an unhandled promise rejection. Instead, consider displaying an error message to the user or implementing a retry mechanism.
| <strong className="has-text-danger">Planned</strong> | ||
| )} | ||
| {' by '} | ||
| <a href={`mailto:${user?.email}`}>{user?.name}</a> |
There was a problem hiding this comment.
You are using the user state variable directly in your JSX without checking if it's null. This could lead to a runtime error if the user is null. Consider adding a null check before accessing the user properties.
| {/* For not completed */} | ||
| <strong className="has-text-danger">Planned</strong> | ||
| const handleCloseTodo = () => { | ||
| dispatch(setCurrentTodo(null)); |
There was a problem hiding this comment.
You are directly dispatching the setCurrentTodo action with null as payload. This could lead to potential bugs in your application if other parts of your code expect the current todo to always be an object. Consider adding a clearCurrentTodo action that doesn't require a payload.
DEMO LINK