Conversation
There was a problem hiding this comment.
Pull request overview
Implements a full React TodoMVC-style app backed by the Mate Academy students API, including CRUD operations, filtering, and UI state for loading/errors.
Changes:
- Added API layer (
fetchClient,api/todos) and shared types/enums for todos, filters, and error messages. - Implemented main app logic in
App.tsx(load, add, delete, update, toggle-all, filtering, error notification). - Added UI components for header/input, todo list/items, and footer/filtering/clear-completed.
Reviewed changes
Copilot reviewed 10 out of 11 changed files in this pull request and generated 10 comments.
Show a summary per file
| File | Description |
|---|---|
| src/utils/fetchClient.ts | Adds a small fetch wrapper used by the todos API client. |
| src/api/todos.ts | Implements todos API methods (get/add/delete/update) and USER_ID constant. |
| src/types/Todo.ts | Introduces the Todo type used across the app. |
| src/types/enums.ts | Defines filter states and user-facing error messages. |
| src/App.tsx | Main app state management, filtering, loading indicators, and error notification UI. |
| src/components/Header.tsx | Handles creating todos and toggle-all button rendering. |
| src/components/TodoList.tsx | Renders the visible todo collection with per-item loading state. |
| src/components/TodoItem.tsx | Implements per-todo UI (toggle, rename, delete) and loader overlay. |
| src/components/Footer.tsx | Implements filter links, items-left counter, and clear-completed. |
| package.json | Updates @mate-academy/scripts dev dependency version. |
| package-lock.json | Locks updated @mate-academy/scripts version and metadata. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| interface Props { | ||
| USER_ID: number; | ||
| addTodo: (title: string) => Promise<Todo>; | ||
| setTodos: React.Dispatch<React.SetStateAction<Todo[]>>; | ||
| setShowError: React.Dispatch<React.SetStateAction<'' | ErrorMessage>>; | ||
| setTempTodo: React.Dispatch<React.SetStateAction<Todo | null>>; | ||
| todoFieldRef: React.RefObject<HTMLInputElement>; | ||
| handleToggleAll: () => void; | ||
| todos: Todo[]; |
There was a problem hiding this comment.
Using an ALL_CAPS name for a prop (USER_ID) is easily confused with a module constant and is inconsistent with typical prop naming. Consider renaming the prop to userId (and updating its usage) while keeping the module-level constant as USER_ID if desired.
| /* eslint-disable jsx-a11y/label-has-associated-control */ | ||
| /* eslint-disable jsx-a11y/control-has-associated-label */ |
There was a problem hiding this comment.
These eslint disables appear unused in this file (there are no <label> elements here). Consider removing them to avoid blanket rule suppression and keep lint configuration focused on the files that need it.
| /* eslint-disable jsx-a11y/label-has-associated-control */ | |
| /* eslint-disable jsx-a11y/control-has-associated-label */ |
| const title = input.value.trim(); | ||
|
|
||
| if (title) { | ||
| const newTempTodo: Todo = { | ||
| id: 0, | ||
| userId: USER_ID, | ||
| title, | ||
| completed: false, | ||
| }; | ||
|
|
||
| addTodo(title) | ||
| .then(newTodo => { | ||
| setTodos(prevTodos => [...prevTodos, newTodo]); | ||
| input.value = ''; | ||
| }) | ||
| .catch(() => { | ||
| setShowError(ErrorMessage.Add); | ||
| }) | ||
| .finally(() => { | ||
| setTempTodo(null); | ||
| input.disabled = false; | ||
| input.focus(); | ||
| }); |
There was a problem hiding this comment.
When starting a new create request, the previous error message is not cleared, so the error stays visible until the request resolves/rejects. Cypress expects the error notification to be hidden immediately on a new request. Consider calling setShowError('') right before triggering addTodo(title) (and optionally also before showing EmptyTitle to reset the timer on repeated submits).
| .then(() => fetch(BASE_URL + url, options)) | ||
| .then(response => { | ||
| if (!response.ok) { | ||
| throw new Error(); |
There was a problem hiding this comment.
throw new Error() without any message (or status info) makes debugging and logging harder, and prevents callers from distinguishing between different HTTP failure cases. Consider including at least the HTTP status/statusText (and maybe the response body text) in the thrown error.
| throw new Error(); | |
| throw new Error( | |
| `Request to ${url} failed with status ${response.status} ${response.statusText}`, | |
| ); |
| const focusField = () => { | ||
| if (todoFieldRef.current) { | ||
| todoFieldRef.current.focus(); | ||
| } | ||
| }; | ||
|
|
||
| const handleDelete = useCallback((todoId: number) => { | ||
| setLoadingIds(prev => [...prev, todoId]); | ||
| deleteTodo(todoId) | ||
| .then(() => { | ||
| setTodos(prevTodos => prevTodos.filter(t => t.id !== todoId)); | ||
| }) | ||
| .catch(() => { | ||
| setShowError(ErrorMessage.Delete); | ||
| }) | ||
| .finally(() => { | ||
| setLoadingIds(prev => prev.filter(id => id !== todoId)); | ||
| focusField(); | ||
| }); | ||
| }, []); |
There was a problem hiding this comment.
These useCallback hooks are declared with empty dependency arrays, but they close over focusField (and todos in other callbacks). With plugin:react-hooks/recommended, this will trigger react-hooks/exhaustive-deps and can also lead to stale closures if focusField changes. Consider wrapping focusField in useCallback and including it in the dependency array (or inlining the focus logic inside the callback).
| const handleUpdate = useCallback( | ||
| (todoId: number, title: string, completed: boolean) => { | ||
| setLoadingIds(prev => [...prev, todoId]); | ||
|
|
||
| return updateTodo(todoId, title, completed) | ||
| .then(updatedTodo => { | ||
| setTodos(prevTodos => | ||
| prevTodos.map(t => (t.id === todoId ? updatedTodo : t)), | ||
| ); | ||
| }) | ||
| .catch(error => { | ||
| setShowError(ErrorMessage.Update); | ||
| throw error; | ||
| }) | ||
| .finally(() => { | ||
| setLoadingIds(prev => prev.filter(id => id !== todoId)); | ||
| focusField(); | ||
| }); | ||
| }, | ||
| [], | ||
| ); |
There was a problem hiding this comment.
handleUpdate is memoized with [] but closes over focusField. This will be flagged by react-hooks/exhaustive-deps and can become a stale closure if the referenced function changes. Include focusField in deps (and memoize focusField), or remove useCallback if memoization isn’t needed.
| todo={tempTodo} | ||
| deleting={true} | ||
| handleDelete={() => {}} | ||
| handleUpdate={handleUpdate} |
There was a problem hiding this comment.
The temporary todo is rendered with a real TodoItem, but TodoItem doesn’t disable interactions when deleting is true. This allows users (or tests) to click the checkbox or delete button on the temp item (id=0), which can trigger invalid API calls (e.g. PATCH /todos/0) or do nothing silently. Consider rendering a non-interactive placeholder for tempTodo, or ensure TodoItem disables/ignores actions when deleting is true.
| handleUpdate={handleUpdate} | |
| handleUpdate={() => {}} |
| interface Props { | ||
| todos: Todo[]; | ||
| deletingIds: number[]; | ||
| handleDelete: (todoId: number) => void; | ||
| handleUpdate: ( | ||
| todoId: number, | ||
| title: string, | ||
| completed: boolean, | ||
| ) => Promise<void>; |
There was a problem hiding this comment.
Prop name deletingIds suggests it only tracks deletions, but App passes loadingIds used for both delete and update operations. Renaming this prop to something like loadingIds/processingIds (and deleting to isLoading) would make the component intent clearer.
| readOnly | ||
| onClick={() => { |
There was a problem hiding this comment.
When deleting is true, the checkbox remains clickable and still calls handleUpdate. This can trigger updates while a todo is “loading” (and for the temp todo, it can call the API with id=0). Consider disabling the input (or early-returning from the handler) when deleting is true.
| readOnly | |
| onClick={() => { | |
| readOnly | |
| disabled={deleting} | |
| onClick={() => { | |
| if (deleting) { | |
| return; | |
| } |
| .then(() => { | ||
| setIsEditing(false); | ||
| }) | ||
| .catch(() => {}); |
There was a problem hiding this comment.
handleRename swallows errors with an empty .catch(() => {}), which makes failures harder to reason about and can hide unexpected exceptions. If the error is intentionally ignored because the parent shows a global error, consider adding a short comment, or remove the catch and let the returned promise rejection be handled upstream.
| .catch(() => {}); | |
| .catch(error => { | |
| // Log the error instead of swallowing it to aid debugging | |
| console.error('Failed to update todo', error); | |
| }); |
DEMO LINK