Skip to content

Solution#2130

Open
HiBlurryface wants to merge 9 commits intomate-academy:masterfrom
HiBlurryface:develop
Open

Solution#2130
HiBlurryface wants to merge 9 commits intomate-academy:masterfrom
HiBlurryface:develop

Conversation

@HiBlurryface
Copy link

@HiBlurryface HiBlurryface commented Feb 26, 2026

src/App.tsx Outdated
}, []);

const filteredTodos = () => {
let copyTodos = [...todos];
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Array.filter() już zwraca nową tablicę. Spread [...todos] jest niepotrzebny.

src/App.tsx Outdated
Comment on lines +72 to +75
<TodoContext.Provider
value={{ todos, setTodos, loadingIds, setLoadingIds }}
>
<ErrorContext.Provider value={{ ...error, showError, closeError }}>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here ytou have some good tips for context

https://kentcdodds.com/blog/how-to-use-react-context-effectively

src/App.tsx Outdated
Comment on lines +46 to +61
const filteredTodos = () => {
let copyTodos = [...todos];

switch (filter) {
case 'Active':
copyTodos = copyTodos.filter(item => !item.completed);
break;
case 'Completed':
copyTodos = copyTodos.filter(item => item.completed);
break;
default:
break;
}

return copyTodos;
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

use useMemo for this

src/App.tsx Outdated
Comment on lines +72 to +75
<TodoContext.Provider
value={{ todos, setTodos, loadingIds, setLoadingIds }}
>
<ErrorContext.Provider value={{ ...error, showError, closeError }}>
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Both contexts create new object on each render

@@ -0,0 +1,170 @@
import classNames from 'classnames';
import { useContext } from 'react';
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are you impolrting in each line from the same module it can be squashed into one line

import { useRef, useEffect, useContext, useState } from 'react';

type="checkbox"
className="todo__status"
checked={todo.completed}
onClick={todoCompleteButton}
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here it should listen for onChange event. onClick z checked will create React warning about controlled component

try {
if (trimmed.length === 0) {
inputRef.current?.focus();
await deleteTodoFromServer();
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In editTodo, you add todo.id to loadingIds and then (if the title is empty) call deleteTodoFromServer(), which also adds the same todo.id to loadingIds. So the same ID gets added twice. Then editTodo removes the ID in its finally, but deleteTodoFromServer also tries to remove it in its own finally- this creates a race condition where one finally can clear the loading state while the other operation is still in progress.

return;
}

await toggleTodo(todo.id, { title: trimmed });
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Better name fro this could be patchTodo

Comment on lines +19 to +22
const completed = todos.filter(todo => todo.completed);
const completedCount = completed.length;
const active = todos.filter(todo => !todo.completed);
const activeCount = active.length;
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
const completed = todos.filter(todo => todo.completed);
const completedCount = completed.length;
const active = todos.filter(todo => !todo.completed);
const activeCount = active.length;
const activeCount = todos.filter(t => !t.completed).length;
const completedCount = todos.length - activeCount;

Comment on lines +68 to +76
results.forEach((result, index) => {
if (result.status === 'fulfilled' && result.value === 1) {
const todoId = idsToDelete[index];

setTodos(prev => prev.filter(todo => todo.id !== todoId));
} else {
showError('Unable to delete a todo');
}
});
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each setTodos call inside the loop is a separate state update. In React 18, batching usually covers updates in event handlers, but in async code (after an await) it isn’t guaranteed. It’s better to collect the results first and then call setTodos once:

const deletedIds = results
  .map((r, i) => (r.status === "fulfilled" ? idsToDelete[i] : null))
  .filter(Boolean);

setTodos((prev) => prev.filter((t) => !deletedIds.includes(t.id)));

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants