Conversation
| TasksQuery.eraseTask(context: dataContainer.mainContext, | ||
| task: task) | ||
|
|
||
| tasks = fetchData() |
There was a problem hiding this comment.
Ниже только по делу — что может быть улучшено и где есть потенциальные недопонимания после замены deleteTask на eraseTask.
-
Семантика и гайдлайны именования:
- В Swift/Apple-экосистеме для удаления принято delete/remove. Слово erase ассоциируется в Swift прежде всего с type erasure (eraseToAnyPublisher), что вводит в заблуждение.
- Если задуман иной смысл (например, «жёсткое удаление» vs «мягкое/архивация»), зафиксируйте это явно в имени: hardDeleteTask/purgeTask/softDeleteTask/trashTask. Если смысла менять терминологию нет — лучше оставить deleteTask.
-
Совместимость API:
- Если переименовали метод в продуктивном коде, оставьте устаревший с @available(*, deprecated, message: ...) и форвардингом на новый, чтобы не ломать внешний код/старые тесты.
-
Контракт метода удаления:
- Уточните поведение по сохранению контекста. Если eraseTask не делает context.save(), тесты могут «проходить» на том же контексте, но интеграционно данные не удалятся.
- Рекомендация: внутри eraseTask использовать performAndWait на нужном контексте и save, и пусть метод throws. В тестах тогда использовать XCTAssertNoThrow/try.
-
Контекст и потокобезопасность:
- Вызовы на mainContext из тестов могут быть не на главном потоке. Оберните операции в context.perform/performAndWait.
- Избегайте риска несоответствия контекстов: вместо передачи context снаружи можно брать task.managedObjectContext внутри метода. Или хотя бы ассертом проверять соответствие.
-
Консистентность:
- Комментарии остались “Delete …”, а метод — eraseTask. Приведите к одному термину, иначе это источник путаницы.
-
Полнота проверок в тестах:
- Сейчас проверяются только counts. Добавьте проверки конкретного удалённого объекта: например, попытка refetch по objectID должна возвращать nil/0, а у прежнего объекта isDeleted == true после удаления.
- Если удаление родителя должно каскадно удалять подзадачи — добавьте явный ассерт на отсутствие подзадачи, а не только на общий count.
- Для проектов: помимо общего fetch проверьте, что project.getTasks() больше не содержит ссылку на удалённую задачу (и нет висячих ссылок).
-
Шум в тестах:
- printInfo в тестах лучше убрать. Если нужен вывод при падении — используйте сообщения ассертов или XCTAttachment.
-
Улучшение читаемости тестов:
- Повторяющиеся фрагменты with fetchData() и одинаковые сообщения можно вынести в хелперы (например, assertTasksCount(_:line:file:)) с автоматическим включением фактического значения: “Expected 2 tasks, got (tasks.count)”.
-
Дизайн API:
- Статический “TasksQuery.eraseTask(context:task:)” выглядит «джависто». Ближе к свифтовской идиоме:
- метод на репозитории/сервисе, который инжектится в тесты;
- или расширение на NSManagedObjectContext (deleteCascade(_ task: Task));
- или метод, принимающий только task и использующий его managedObjectContext внутри.
- Это уменьшит шанс передать «не тот» контекст.
- Статический “TasksQuery.eraseTask(context:task:)” выглядит «джависто». Ближе к свифтовской идиоме:
-
Конфликты автодополнения:
- Имя erase* часто пересекается в подсказках с Combine (eraseToAnyPublisher). Если всё-таки оставлять “erase”, сделайте имя более специфичным: purgeTask(in:), deleteTaskCascade(in:), eraseTaskPermanently(in:).
Итог: если нет особого смыслового различия, стоит вернуться к deleteTask и обновить тесты/комментарии. Если различие есть — зафиксируйте его явно в названии, добавьте сохранение/throws внутри метода, расширьте тесты проверками конкретных объектов и каскадного поведения, и уберите печать из тестов.
| let dueDate: String | ||
| let groups: [GroupData] | ||
| let projects: [ProjectData] | ||
| } |
There was a problem hiding this comment.
Ниже — только те замечания, которые реально улучшают надежность и поддержку тестов.
- Регресс по точности локатора
- Было: тап по статическому тексту внутри кнопки (пусть и с мусорным кодом от Recorder), но всё же в контексте кнопки.
- Стало: app.staticTexts[localeData.dueDate].firstMatch.tap() — это:
- отвязка от кнопки (может тапнуть просто лейбл, который не tappable);
- повышенная неоднозначность (может быть несколько одинаковых лейблов на экране);
- зависимость от локализации (строка меняется — тесты ломаются).
Рекомендация:
- Использовать стабильный accessibilityIdentifier у самой кнопки и тапать по кнопке:
let dueDateButton = app.buttons["dueDateButton"]
XCTAssertTrue(dueDateButton.waitForExistence(timeout: 3))
dueDateButton.tap() - Если по какой-то причине нельзя добавить идентификатор — хотя бы искать кнопку, а не staticText:
let dueDateButton = app.buttons[localeData.dueDate].firstMatch
XCTAssertTrue(dueDateButton.waitForExistence(timeout: 3))
dueDateButton.tap()
Или связать по иерархии:
let dueDateButton = app.buttons.containing(.staticText, identifier: localeData.dueDate).element
- Устранить flaky: обязательно waitForExistence
Сейчас в обоих местах используется firstMatch.tap() без ожидания. Это источник флаков.
- Примените waitForExistence для dueDate и todayButton:
let todayButton = app.buttons["todayButton"]
XCTAssertTrue(todayButton.waitForExistence(timeout: 3))
todayButton.tap()
-
Локализация как ключ к элементу — хрупко
Даже при наличии localeData лучше использовать локализацию только для проверок текста, а не для поиска элементов. Для поиска — accessibilityIdentifier, одинаковый во всех локалях. Это значительно снизит стоимость сопровождения тестов. -
Изменение Codable-модели LocaleData — риск обратной несовместимости
Вы добавили non-optional dueDate. Если не обновлены все JSON-файлы для разных локалей, декодирование упадет.
Рекомендации:
- Сделать поле опциональным и подставлять дефолт (или самый частый):
let dueDate: String?
var dueDateTitle: String { dueDate ?? "Due Date" } - Или написать кастомный init(from:) с decodeIfPresent и дефолтом.
- Либо гарантированно обновить все фикстуры во всех локалях и добавить проверку на полноту данных в CI.
-
Очистить Recorder-мусор
В коде всё ещё остались START_MENU_TOKEN@/END_MENU_TOKEN комментарии у todayButton. Их стоит удалить для читаемости и чтобы не вводили в заблуждение насчет реального локатора. -
Сужение области поиска
Если элемент находится внутри конкретного контейнера (например, формы/таблицы/листа), лучше искать внутри него:
- например: app.tables.buttons["dueDateButton"] или app.sheets.buttons["todayButton"]
Это ускорит тест и снизит шанс совпадений с одноименными элементами на экране.
Итого по правке: возвратите таргетирование на кнопку (через идентификатор), добавьте waitForExistence, обеспечьте обратную совместимость LocaleData (или обновите все локали), удалите мусор от Recorder и по возможности сузьте область поиска. Это повысит стабильность и ремонтопригодность тестов.
| let finished = task.subtasks?.filter({ $0.deletionDate == nil && $0.completed }) { | ||
|
|
||
| CircularProgressView(progress: CGFloat(subtasksCount == finished.count ? 1.0 : 1.0 / Double(subtasksCount) * Double(finished.count)), | ||
| color: .gray, |
There was a problem hiding this comment.
Хорошее направление — исключать “мягко удалённые” подзадачи из расчётов. Ниже — конкретные улучшения по производительности, читабельности и поддерживаемости.
Что улучшить:
- Уберите двойную фильтрацию и создание лишних массивов. Сейчас task.subtasks фильтруется дважды. Достаточно один раз отфильтровать активные подзадачи, затем посчитать количество завершённых.
- Упростите формулу прогресса. Ветка subtasksCount == finished.count не нужна: Double(finished)/Double(total) и так даст 1.0 при равенстве.
- Не создавайте массив finished, если нужен только .count. Используйте lazy.filter(...).count или reduce.
- Проверку на пустоту делайте через !subtasks.isEmpty — читается лучше, чем count > 0.
- Инкапсулируйте логику в модель/вью-модель, чтобы не пересчитывать в body и переиспользовать правила “активности” подзадачи по всему коду. Это уменьшит шум в View и риск расхождений.
- При необходимости гарантируйте диапазон 0...1 (min(max(progress, 0), 1)) — полезно, если источник данных может схлопнуться или быть неконсистентным.
Вариант прямо во View (минимальные изменения, без лишних аллокаций):
if let subtasks = task.subtasks?.filter({ $0.deletionDate == nil }), !subtasks.isEmpty {
let finishedCount = subtasks.lazy.filter(.completed).count
let progress = CGFloat(finishedCount) / CGFloat(subtasks.count)
CircularProgressView(progress: progress, color: .gray, ...)
}
Ещё эффективнее — один проход без промежуточных массивов:
if let subtasks = task.subtasks {
let (total, finished) = subtasks.reduce(into: (0, 0)) { acc, s in
guard s.deletionDate == nil else { return }
acc.0 += 1
if s.completed { acc.1 += 1 }
}
if total > 0 {
let progress = CGFloat(finished) / CGFloat(total)
CircularProgressView(progress: progress, color: .gray, ...)
}
}
Структурное улучшение (рекомендую):
- Вынесите правила активности/прогресса в модель/вью-модель:
extension Task {
var activeSubtasks: [Subtask] {
(subtasks ?? []).filter { $0.deletionDate == nil }
}
var progress: Double {
let total = activeSubtasks.count
guard total > 0 else { return 0 }
let done = activeSubtasks.lazy.filter(.completed).count
return Double(done) / Double(total)
}
}
Тогда во View:
if !task.activeSubtasks.isEmpty {
CircularProgressView(progress: CGFloat(task.progress), color: .gray, ...)
}
Если это Core Data и подзадач много:
- Рассмотрите вынесение фильтрации в запрос (NSFetchRequest) с предикатом deletionDate == nil и parent == task, чтобы не гонять данные в памяти и не фильтровать в body.
| CURRENT_PROJECT_VERSION = 108; | ||
| DEVELOPMENT_TEAM = 9Z68336878; | ||
| GENERATE_INFOPLIST_FILE = YES; | ||
| MACOSX_DEPLOYMENT_TARGET = 26.0; |
There was a problem hiding this comment.
Коротко по делу — что стоит поправить в этом changelist:
-
Явная ошибка: MACOSX_DEPLOYMENT_TARGET = 26.0 в двух конфигурациях (~1802 и ~1822). Для macOS допустимы значения до 15.x (Sequoia). Если это не macOS-таргет, используйте корректный ключ:
- iOS: IPHONEOS_DEPLOYMENT_TARGET
- watchOS: WATCHOS_DEPLOYMENT_TARGET
- tvOS: TVOS_DEPLOYMENT_TARGET
- visionOS: XROS_DEPLOYMENT_TARGET
В противном случае сборка/линковка развалится или получите неподдерживаемое значение.
-
Safari App Extension: у расширения MACOSX_DEPLOYMENT_TARGET = 14.5, а у хост-приложения macOS = 14.0. На 14.0 расширение не загрузится. Либо поднимите у хоста до 14.5, либо опустите у расширения до 14.0 (если API позволяют). Лучше выровнять.
-
CODE_SIGN_IDENTITY = "Apple Development" принудительно прописана у Safari-расширения (и для macOS SDK). При включенном Automatic это лишнее и может сломать архивацию/загрузку в App Store (ожидается Distribution). Рекомендация: убрать явное CODE_SIGN_IDENTITY и доверить Automatic.
-
iOS-таргет: ENABLE_APP_SANDBOX = YES. Это макОС-специфичный флаг. На iOS он не нужен и может сбивать с толку (или привести к нежелательным entitlement’ам при ошибочной конфигурации). Уберите для iOS/watchOS таргетов.
-
Несогласованность версий: часть конфигураций всё ещё с MARKETING_VERSION = 1.0 (см. ~1195, ~1217 и далее). Если эти таргеты попадают в дистрибутив (extensions/plug-ins), выровняйте их на 25.12. Если это тестовые бандлы — ок.
-
Отсутствуют явные WATCHOS_DEPLOYMENT_TARGET у watchOS-виджетов/приложения (и, возможно, у части iOS/macOS таргетов тоже). Лучше проставить явные минимальные версии для всех deliverable-таргетов, чтобы не полагаться на дефолты проекта и избежать непредсказуемой миграции при обновлении Xcode.
-
Общая рекомендация по версиям: вы вынуждены менять CURRENT_PROJECT_VERSION и MARKETING_VERSION во множестве мест. Сведите версии в один источник:
- вынесите в project-level User-Defined Settings (например, APP_MARKETING_VERSION, APP_BUILD_NUMBER) и используйте их в таргетах;
- или перейдите на .xcconfig-файлы.
Это снизит риск пропустить таргет и упростит релизы.
-
Проверьте, не используется один и тот же .entitlements файл для iOS и watchOS-виджетов, если в нём есть платформенно-специфичные ключи (например, macOS sandbox). Для iOS/watchOS обычно достаточно application-groups и подобных общих entitlement’ов; при расхождениях — лучше разделить файлы.
-
Вы задали IPHONEOS_DEPLOYMENT_TARGET = 18.1. Если нет жёсткой привязки к API 18.1+, имеет смысл опустить до 18.0, чтобы не отсекать пользователей iOS 18.0. Если это осознанно — оставить как есть.
-
Проверьте наличие DEVELOPMENT_TEAM во всех таргетах, которые подписываются и публикуются (в части конфигураций есть, в части — не видно в дифе). Несогласованность может всплыть на CI/архивации.
-
Если macOS и iOS приложения не Catalyst, а нативные и оба публикуются, убедитесь, что стратегия с одинаковым bundle id действительно соответствует вашей модели (универсальная покупка). Если нет — лучше развести bundle id.
Итого: самое критичное — исправить MACOSX_DEPLOYMENT_TARGET = 26.0 и убрать принудительный CODE_SIGN_IDENTITY для Safari-расширения; далее — выровнять deployment target расширения с хостом и централизовать версии. Остальное — платформа- и процессные улучшения, чтобы избежать хрупкости при следующих релизах.
| .accessibility(identifier: "\(maintask.name)PlayButton") | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Ниже — только предметные замечания, где есть риск регрессий, неоднозначность или место для улучшения.
-
Идентичность элементов в OutlineGroup
- id: .self и .tag(maintask) + сравнение focus == maintask опираются на Hashable/Equatable сущности. Если Task — класс (особенно Core Data), это хрупко: хеш может зависеть от изменяемых полей, а == может сравнивать по содержимому, а не по стабильной идентичности.
- Рекомендации:
- Дайте Task стабильный идентификатор (например, id: .id где id — UUID), для Core Data — objectID.
- Сравнение фокуса делайте по стабильному id (focus.id == maintask.id), а не по == объекта.
- Точно так же .tag используйте с id, а не целым объектом.
-
Фильтрация удалённых задач (регрессия)
- В прежнем коде был guard по deletionDate (пусть и странной логики), сейчас он исчез. Вы переложили фильтрацию в visibleSubtasks, но корневой maintask всё равно будет отрисован даже если он удалён.
- Рекомендации:
- Гарантируйте, что исходный массив для OutlineGroup не содержит удалённых корней, либо оборачивайте HStack в условие, которое исключает удалённые корни. Лучше подготовить заранее “видимое дерево” и кормить OutlineGroup уже очищенными данными.
- Во visibleSubtasks возвращайте nil (а не пустой массив), если нет потомков — это корректнее для OutlineGroup.
-
Порядок и семантика команд таймера в кнопках
- В “Stop”: сначала timer.reset(), затем проверка timer.mode и timer.skip(). После reset состояние/режим обычно уже сброшены — условие и skip могут сработать не так, как ожидается.
- В “Play”: если таймер уже .running, вы ни reset, ни start/resume не делаете — переключение фокуса не синхронизировано с состоянием таймера (например, если таймер в режиме перерыва).
- Рекомендации:
- Инкапсулируйте сценарии в целевые методы уровня домена: timer.startFocus(on: task), timer.stopFocus(), timer.switchFocus(to: task). Внутри определите корректный порядок reset/start/resume/skip с учётом текущего state/mode. Это снимет дублирование и ошибки порядка вызовов.
- Если всё же оставаться на текущем API, выполните skip до reset (если это нужно), либо откажитесь от skip при stop. При смене фокуса во время работы определите явное поведение: сбрасывать и запускать заново или продолжать интервал.
-
viewMode = 1 — “магическое число”
- Используйте типобезопасный enum (например, enum ViewMode: Int { case list = 0, focus = 1 }) и храните именно ViewMode (или хотя бы константы), а не “1” в коде.
-
Размещение .listRowSeparator(.hidden)
- Сейчас модификатор стоит на TaskRowView, а не на корневой HStack строки. Это может не скрыть разделитель для всей строки. Перенесите .listRowSeparator(.hidden) на HStack (или на контейнер строки после HStack).
-
Кнопки в строке списка
- На iOS/macOS кнопки внутри List могут конфликтовать с выделением строки/наведением. Чтобы избежать побочных эффектов:
- Добавьте .buttonStyle(.borderless) или .buttonStyle(.plain) к кнопкам.
- Добавьте .contentShape(Rectangle()) к HStack, чтобы вся строка имела предсказуемую зону взаимодействия.
- По желанию — withAnimation при переключении иконок для плавности.
- На iOS/macOS кнопки внутри List могут конфликтовать с выделением строки/наведением. Чтобы избежать побочных эффектов:
-
Доступность и тестовые идентификаторы
- accessibility(identifier: "(maintask.name)PlayButton"): если name опционален или может менятьcя, это сделает идентификатор нестабильным. Лучшe формировать ID из стабильного task.id и дать читаемую accessibilityLabel/Hint для VoiceOver.
-
visibleSubtasks: безопасность и производительность
- Убедитесь, что visibleSubtasks:
- Не создаёт циклов (в случае испорченных данных) — при необходимости добавляйте защиту от повторных посещений (Set).
- Возвращает nil для листьев.
- Не строит заново тяжёлые массивы на каждый ререндер без надобности. Если это вычисляемое свойство, которое фильтрует Core Data фетчи — подумайте о мемоизации/кэше на слой модели.
- Убедитесь, что visibleSubtasks:
Итого: main fix — перейти на стабильные идентификаторы (id/objectID) в OutlineGroup, tag и сравнениях; вернуть/перенести корректную фильтрацию удалённых корней; упорядочить логику таймера в явные методы домена; убрать “магическое” viewMode; поправить место .listRowSeparator и поведение кнопок в List. Это улучшит предсказуемость, тестируемость и снизит риск регрессий.
| .listRowSeparator(.hidden) | ||
| } | ||
| .listRowSeparator(.hidden) | ||
| } else { |
There was a problem hiding this comment.
Ниже только содержательные замечания по диффу.
-
Потеря условия видимости корневой задачи.
Раньше был фильтр в теле OutlineGroup, теперь его нет. Переход на children: .visibleSubtasks скрывает только дочерние элементы, но не сам корень [task]. Если задача помечена как удалённая (deletionDate != nil) и её нужно скрывать вне корзины, корень всё равно будет показан. Решения:- Передавать в OutlineGroup уже отфильтрованные корни:
- let roots = isVisible(task, in: list) ? [task] : task.visibleSubtasks
- OutlineGroup(roots, id: .id, children: .visibleSubtasks) { ... }
- Либо оставить локальную проверку только для корня (если maintask == task), а для детей полагаться на visibleSubtasks.
- Передавать в OutlineGroup уже отфильтрованные корни:
-
Контекстно-зависимая фильтрация в KeyPath.
Логика видимости (например, режим «Корзина» vs обычный список) в прежнем коде учитывалась через условие (task.deletionDate != nil). Перенос в visibleSubtasks как KeyPath лишает возможности учитывать контекст (list), т.к. KeyPath не может захватить внешнее состояние. Если filtering зависит от list, есть два варианта:- Формировать roots и/или ветки снаружи OutlineGroup (как выше), оставив детям нейтральный KeyPath (.subtasks или .visibleSubtasks без знания о list).
- Отказаться от OutlineGroup и сделать свою рекурсивную вёрстку (DisclosureGroup/ForEach), где children формируются функцией, захватывающей list.
-
Идентичность элементов.
- id: .self нежелателен, если Task — класс или Hashable основан на изменяемых полях. Это чревато «data is not unique» и глюками при диффинге при изменении свойств. Рекомендация: использовать стабильный идентификатор (id: .id) и обеспечить уникальность.
- .tag(maintask) должен быть согласован с типом selection. Если selection хранит Set, то и тег должен быть .tag(maintask.id). Несогласованность id/tag часто приводит к неправильной работе выбора.
-
Дублирование listRowSeparator(.hidden).
Вы вызываете .listRowSeparator(.hidden) и на строке, и на OutlineGroup. Второй вызов лишний и не даёт добавочной ценности. Оставьте только на строке (внутри closure), либо только снаружи, но единообразно. -
Производительность/стабильность visibleSubtasks.
Если visibleSubtasks — вычисляемое свойство, не завязанное на стабильное кеширование, OutlineGroup будет вызывать его много раз по дереву. Убедитесь, что:- Оно не опирается на изменяемые поля, влияющие на Hashable/Equatable, иначе получите «скачущие» идентичности.
- Фильтрация не делает избыточной работы на каждом рендере. Иногда разумно подготовить отфильтрованное дерево заранее при смене list/фильтров.
-
Циклы/повторы в дереве.
Проверьте, что visibleSubtasks гарантированно не содержит parent и не создаёт циклов или дубликатов. OutlineGroup не защищает от зацикливания.
Рекомендуемая правка по сути (концептуально):
- Стабильные ID и согласованный tag.
- Явная фильтрация корней с учётом контекста list до передачи в OutlineGroup.
- Поддерживать children как нейтральный KeyPath, а контекстные фильтры применять до входа в OutlineGroup; если это невозможно — перейти на собственную рекурсивную верстку вместо KeyPath.
| let finished = task.subtasks?.filter({ $0.deletionDate == nil && $0.completed }) { | ||
|
|
||
| CircularProgressView(progress: CGFloat(subtasksCount == finished.count ? 1.0 : 1.0 / Double(subtasksCount) * Double(finished.count)), | ||
| color: .gray, |
There was a problem hiding this comment.
Ниже — только по делу, что можно улучшить в изменённом фрагменте.
-
Двойная фильтрация и лишние аллокации:
- Сейчас вы дважды фильтруете task.subtasks (для total и для finished), создавая два массива. Это лишняя работа и память.
- Улучшения:
- Либо один раз создайте activeSubtasks и переиспользуйте его:
- let active = task.subtasks?.filter { $0.deletionDate == nil } ?? []
- let total = active.count
- let done = active.lazy.filter(.completed).count // lazy — без промежуточных массивов
- Либо посчитайте за один проход без промежуточных коллекций:
- var total = 0, done = 0
- for s in subtasks where s.deletionDate == nil { total += 1; if s.completed { done += 1 } }
- Если это Core Data и task.subtasks — NSSet/Set, лучше один раз привести к Set и работать с ним, чтобы избежать неоднократного обращения к relationship.
- Либо один раз создайте activeSubtasks и переиспользуйте его:
-
Упростите формулу прогресса и добавьте ограничение диапазона:
- Текущий тернарный operator избыточен — Double(finished)/Double(total) уже даёт 1 при равенстве.
- На случай рассинхронизации данных (done > total) или отрицательных значений (логические ошибки), имеет смысл зажать значение:
- let progress = min(1, max(0, Double(done) / Double(total)))
- Затем один раз обернуть в CGFloat.
-
Избегайте расчётов в body при каждом ререндере:
- Если этот код в SwiftUI body, вычисления будут выполняться часто. Вынесите агрегацию в:
- вычисляемые свойства модели/вью-модели, или
- мемоизацию/кэш, если данные большие.
- Это особенно актуально для Core Data (faulting, обращение к relationship может быть не бесплатным).
- Если этот код в SwiftUI body, вычисления будут выполняться часто. Вынесите агрегацию в:
-
Именование и типы:
- finished как массив не нужен — используйте doneCount (Int) для ясности и чтобы не хранить лишний массив.
- Старайтесь работать с Int/Double и конвертировать в CGFloat только в месте передачи в UI.
-
Единый срез данных для согласованности:
- Вы дважды обращаетесь к task.subtasks?.filter(...). Между вызовами состояние relationship теоретически может измениться (особенно при асинхронных обновлениях), что даст несогласованные total и done.
- Возьмите один «снимок» набора в локальную переменную и работайте только с ним.
Пример более эффективной и простой версии (один проход + клампинг):
if let subtasks = task.subtasks {
var total = 0
var done = 0
for s in subtasks where s.deletionDate == nil {
total += 1
if s.completed { done += 1 }
}
if total > 0 {
let ratio = Double(done) / Double(total)
let progress = CGFloat(min(1, max(0, ratio)))
CircularProgressView(progress: progress, color: .gray, ...)
}
}
Или с одним фильтром и без лишних массивов:
if let subtasks = task.subtasks {
let active = subtasks.lazy.filter { $0.deletionDate == nil }
let total = active.count
if total > 0 {
let done = active.filter(.completed).count
let progress = CGFloat(min(1, max(0, Double(done) / Double(total))))
CircularProgressView(progress: progress, color: .gray, ...)
}
}
Эти изменения упростят код, устранят лишние аллокации/проходы, сделают результат устойчивее к аномалиям данных и снизят нагрузку при ререндеринге.
| } | ||
| } | ||
| .listRowSeparator(.hidden) | ||
| } else { |
There was a problem hiding this comment.
Ниже только по делу — где могут быть ошибки/несоответствия и как улучшить.
-
Сохранение прежней логики видимости. Раньше была проверка:
maintask.deletionDate == nil || task == maintask || task.deletionDate != nil
Переход на children: .visibleSubtasks корректен только если visibleSubtasks реализован так, чтобы:- всегда показывать корневой task;
- когда текущий узел удалён (deletionDate != nil), его дети не фильтруются (показываются все);
- когда текущий узел не удалён — дети фильтруются (показываем только не удалённые).
Если visibleSubtasks просто фильтрует по deletionDate == nil, вы потеряете кейс “корень удалён → показывать всё дерево”.
Рекомендация: реализовать visibleSubtasks примерно так: - если self.deletionDate != nil → return subtasks (без фильтра);
- иначе → return subtasks.filter { $0.deletionDate == nil }.
И убедиться, что для корня вы не фильтруете сам root (OutlineGroup всегда рендерит корень из seed [task]).
-
Идентичность/стабильные ID. id: .self и .tag(maintask):
- Если Hashable/Equatable зависят от изменяемых полей (например, deletionDate), вы получите «прыгающую» идентичность и проблемы с перерисовкой/selection.
- Рекомендовано иметь стабильный идентификатор (например, id: UUID) и использовать Identifiable:
- сделать Task: Identifiable с let id;
- OutlineGroup([task], children: .visibleSubtasks) без id-параметра;
- .tag(maintask.id) если selection привязан к идентификаторам.
Это уменьшит баги с выбором строк, свайпами и обновлениями.
-
Дублирование скрытия разделителей. У вас стоит .listRowSeparator(.hidden) и на каждом элементе, и на OutlineGroup. Достаточно одного уровня (чаще – на ряду). Уберите лишнее для предсказуемости.
-
hasSubtasks() vs реальная видимость. Вы выбираете OutlineGroup, только если hasSubtasks() == true. Но после фильтрации через visibleSubtasks у узла могут быть «логически» отсутствующие видимые дети. Два варианта улучшения:
- Всегда использовать один OutlineGroup (без ветвления) — он корректно ведёт себя, даже если детей нет;
- Либо проверять hasVisibleSubtasks вместо hasSubtasks(), чтобы не плодить пустые OutlineGroup.
-
Согласованность selection/tag с типом selection. .tag(maintask) подразумевает, что selection у List — того же типа (Task) и с устойчивым равенством. Если selection работает по id, приведите tag к .tag(maintask.id), чтобы исключить рассинхрон.
-
Производительность и стабильность порядка:
- Убедитесь, что visibleSubtasks возвращает стабильно отсортированный массив (если порядок важен). Скачущий порядок + id: .self → лишние перестройки.
- Избегайте тяжёлых вычислений внутри visibleSubtasks (фильтрации/сортировки на горячем пути). Если нужно — кэшируйте/предварительно готовьте.
-
Нейминг в замыкании OutlineGroup. maintask вводит в заблуждение — это не «главная» задача, а текущий узел. Лучше item/node/taskNode — снижает когнитивную нагрузку при чтении.
-
Тест кейсов видимости. Минимальный набор для регрессий:
- root не удалён: скрыть все удалённые дочерние;
- root удалён: показать всё дерево;
- удалён промежуточный узел при живом root: сам узел не виден, его поддерево — тоже не видно (если так задумано).
Итог: ключевые риски — потеря семантики старого условия из-за упрощённого visibleSubtasks и использование .self/объектов в tag, зависящих от изменяемого состояния. Рекомендую перейти на стабильный id и явно зафиксировать логику visibleSubtasks, после чего можно упростить ветвление и убрать дублирующий .listRowSeparator(.hidden).
| } | ||
|
|
||
| func hasSubtasks() -> Bool { | ||
| if let subtasks = self.subtasks, subtasks.count > 0 && self.deletionDate == nil { |
There was a problem hiding this comment.
Ниже — только те замечания, которые реально улучшат поведение и согласованность кода.
-
Несогласованность логики с hasSubtasks():
Сейчас visibleSubtasks скрывает удалённые подзадачи, а hasSubtasks() считает всех детей и ещё проверяет, что текущая задача не удалена. Это даст противоречия в UI (ветка может считаться «имеющей детей», но визуально — пустой).
Предложение:- Либо заменить реализацию hasSubtasks() на return visibleSubtasks != nil.
- Либо переименовать в hasVisibleSubtasks и использовать везде одну интерпретацию «есть дети».
- Если важно учитывать удалённость родителя, делайте это в одном месте (например, родитель вообще не должен попадать в список данных для OutlineGroup, а не дублировать проверку внутри hasSubtasks()).
-
Стабильный порядок детей:
Если порядок в subtasks не гарантирован (например, Core Data без сортировки), возможны «прыжки» строк/аномальные анимации при обновлениях. Лучше отсортировать детей по стабильному ключу (sortIndex, position, createdAt и т.д.).
Пример:
var visibleSubtasks: [Todo]? {
let filtered = (subtasks ?? []).filter { $0.deletionDate == nil }
let sorted = filtered.sorted { $0.sortIndex < $1.sortIndex } // или другой ключ
return sorted.isEmpty ? nil : sorted
} -
Производительность/повторы вычислений:
Фильтрация и сортировка будут вызываться часто при перерисовках OutlineGroup. Если дерево крупное, стоит:- Либо кешировать результат на время жизненного цикла узла с инвалидированием при изменении subtasks/deletionDate.
- Либо заранее формировать «живых» детей на уровне запроса/фетча (если это Core Data — через NSFetchedResultsController или отдельное отношение/предикат).
-
Нейминг и читаемость:
Если deletionDate — это soft-delete, имеет смысл ввести семантическую обёртку:
var isSoftDeleted: Bool { deletionDate != nil }
Тогда фильтр станет нагляднее: filter { !$0.isSoftDeleted }.
А visibleSubtasks лучше переименовать ближе к назначению, например outlineChildren или visibleChildren, если свойство используется именно для OutlineGroup. -
Совместимость с OutlineGroup:
Вы возвращаете Optional<[Todo]>. Это корректно для инициализаторов OutlineGroup, которые принимают KeyPath к Optional-детям. Убедитесь, что вы используете именно такой init. Если использовать вариант, ожидающий не-Optional коллекцию, код не скомпилируется. Если у вас уже стоит Optional-вариант — всё ок. -
Удалите дублирующуюся логику:
Если перейдёте на visibleSubtasks как «источник истины», то:
func hasSubtasks() -> Bool { visibleSubtasks != nil }
Или вообще сделайте вычисляемое свойство:
var hasVisibleSubtasks: Bool { visibleSubtasks != nil }
и переведите вызовы на него. -
Потоки (если это Core Data):
Если Todo — NSManagedObject, доступ к связям безопасен только на соответствующей очереди контекста. В SwiftUI это обычно MainActor. Имеет смысл документировать или аннотировать точку доступа. В крайнем случае можно объявить:
@mainactor var visibleSubtasks: [Todo]? { ... }
чтобы избежать случайного чтения с не той очереди.
Итоговое упрощение/унификация (одна из возможных версий):
- var isSoftDeleted: Bool { deletionDate != nil }
- var visibleSubtasks: [Todo]? {
let children = (subtasks ?? []).filter { !$0.isSoftDeleted }
// + опциональная сортировка:
// let children = children.sorted { $0.sortIndex < $1.sortIndex }
return children.isEmpty ? nil : children
} - func hasSubtasks() -> Bool { visibleSubtasks != nil } или var hasVisibleSubtasks: Bool { visibleSubtasks != nil }
Это устранит расхождения и улучшит предсказуемость поведения в OutlineGroup.
| "dueDate": "Дата завершения", | ||
| "groups": [ | ||
| { | ||
| "name": "🦝 Личное" |
There was a problem hiding this comment.
Наблюдения и предложения по улучшению:
-
Точность перевода RU: «Дата завершения» обычно понимается как фактическая дата, когда задача была завершена. Для due date корректнее: «Крайний срок», «Срок» или «Срок выполнения». Рекомендую «Крайний срок» (полная форма) или «Срок» (краткая, для компактных заголовков/лейблов).
-
Стиль регистров EN: если в интерфейсе вы используете sentence case (что видно по «Buy coffee», «Vertical scroll bar, 1 page»), то «Due Date» стоит привести к «Due date» для единообразия. Если у вас title case для лейблов — убедитесь, что остальные строки тоже соответствуют этому стилю.
-
Контекст и неоднозначность: ключ dueDate сейчас контекстно нейтрален. Если строка используется как:
- лейбл поля — dueDate.label / dueDate_title;
- плейсхолдер — dueDate.placeholder;
- состояние без значения — dueDate.none (например, «Без срока» / «No due date»).
Разделение уменьшит риск неправильных переводов и облегчит локализацию.
-
Согласованность терминологии: проверьте, нет ли в проекте уже «Deadline»/«Срок» в других местах. Лучше унифицировать термин везде (либо «Due date»/«Крайний срок», либо «Deadline»/«Дедлайн», избегая смешения).
-
Локализация во всех языках: вы добавили ключ только в en/ru. Если у вас есть другие локали — добавьте туда переводы или хотя бы временно продублируйте английский, чтобы избежать падений/фолбэков.
-
Комментарии для переводчиков: добавьте описательный комментарий к ключу (в .xcstrings или в вашем пайплайне локализации): «Label for the deadline (the date by which the task must be completed), not the completion date». Это снизит риск неверных переводов.
-
A11y/озвучивание: убедитесь, что в VoiceOver строка читается как «Срок: 24 декабря»/«Due date: December 24», а не дублируется или звучит неестественно. Для этого не добавляйте двоеточие в саму локализованную строку, а форматируйте его на уровне представления.
Резюме правок:
- EN: "dueDate": "Due date"
- RU: "dueDate": "Крайний срок" (или "Срок" — если нужно короче)
- По возможности — разнести на label/placeholder/none, добавить комментарий к ключу и синхронизировать во всех локалях.
No description provided.