Fix creating project as a simple list on iOS#150
Conversation
Remove task from focus timer on completion or deletion Timer improvements UI improvements
Count tasks in sections
| OPENAI_API_ENDPOINT: https://api.proxyapi.ru/openai/v1 | ||
| MODEL: o1-mini | ||
| MODEL: gpt-5 | ||
| PROMPT: "You are an experienced Swift/SwigtUI developer and your job is to review pull requests. Please review the following code for any misunderstandings or violations. Don't spend time commenting on what is already working perfectly. I'm looking for constructive criticism and suggestions for improving the code, only useful and thorough notes" |
There was a problem hiding this comment.
Ниже — только вещи, которые потенциально проблемны или улучшаемы в контексте вашего изменения MODEL и окружения для вызова LLM в GitHub Actions.
-
Смена модели на gpt-5
- Проверьте, что ваш прокси https://api.proxyapi.ru/openai/v1 действительно поддерживает модель с именем gpt-5. Несоответствие имени часто приводит к 400/404. Рекомендую:
- Ввести в workflow_dispatch входной параметр model с дефолтом, и подставлять его в env. Так проще откатываться.
- Добавить ранний валидационный шаг (curl /models или пробный минимальный запрос) и фейлить джобу с понятной ошибкой, если модель не доступна.
- Имплементировать fallback на предыдущую модель (например, o1-mini) при недоступности основной.
- Учитывайте стоимость/латентность: gpt-5 (если доступна) может быть дороже/медленнее. Имеет смысл выставить temperature, max_tokens и, по возможности, seed для стабильности и предсказуемых расходов.
- Проверьте, что ваш прокси https://api.proxyapi.ru/openai/v1 действительно поддерживает модель с именем gpt-5. Несоответствие имени часто приводит к 400/404. Рекомендую:
-
Типографика/точность промпта
- Опечатка: “SwigtUI” → “SwiftUI”.
- LANGUAGE: Russian и текст PROMPT на английском — несогласованность. Если ваш экшен не использует переменную LANGUAGE явным образом, модель будет ориентироваться на PROMPT. Явно добавьте в PROMPT требование отвечать по-русски, либо убедитесь, что ваш рантайм реально читает LANGUAGE и формирует системный промпт.
- PROMPT сейчас длинной строкой — лучше оформить многострочным YAML-литералом (| или >-) ради читабельности и чтобы избежать проблем с кавычками/экранированием. Или вынести промпт в отдельный файл в репозитории.
-
Надёжность/детерминизм
- Добавьте управляемые параметры: TEMPERATURE, MAX_TOKENS, TOP_P, SEED (если поддерживается прокси). Хранить их в env с дефолтами — удобно для быстрой настройки без правки кода.
- Добавьте политику повторов при 429/5xx (retry с экспоненциальной паузой) и ограничение времени шага (timeout-minutes), чтобы джоба не “висела”.
-
Безопасность и конфиденциальность
- Вы используете сторонний прокси. Перенесите OPENAI_API_ENDPOINT в секрет (или хотя бы в переменные окружения репозитория), чтобы иметь возможность быстро переключать окружения и не светить URL в коде.
- Убедитесь, что workflow не запускается на небезопасных событиях с секретами (например, pull_request из форков). Для таких случаев используйте permissions минимум необходимые и избегайте передачи секретов в незнакомый код.
- Проследите, чтобы логи не печатали содержимое PROMPT и конфигурацию, которая может раскрывать детали.
-
Совместимость/контракт с клиентом
- Переменная OPENAI_API_ENDPOINT: многие клиенты ожидают OPENAI_BASE_URL или OPENAI_API_BASE. Убедитесь, что ваш шаг/скрипт читает именно это имя. Если используете сторонний экшен, сверяйте документацию.
- Переменная LANGUAGE: проверьте, что она действительно учитывается вызываемым инструментом. Если нет — удалите лишнее или интегрируйте в генерацию системного промпта.
-
Управление запуском и расходами
- Добавьте concurrency, чтобы не гонять параллельно несколько LLM-рецензий на одну и ту же ветку:
concurrency:
group: llm-review-${{ github.ref }}
cancel-in-progress: true - Подумайте о матрице/фиче-флагах для A/B между моделями, если вы ещё оцениваете качество gpt-5.
- Добавьте concurrency, чтобы не гонять параллельно несколько LLM-рецензий на одну и ту же ветку:
-
Качество рецензий
- Промпт сейчас общий. Добавьте явные критерии: стилистика Swift/SwiftUI, использование Concurrency/Actors, архитектура, UIKit/SwiftUI best practices, линтеры, тесты, перформанс, безопасность. Это повышает полезность ответа и снижает “водянистость”.
Итого: ключевые риски — валидность модели gpt-5 на вашем прокси, несогласованность языка, опечатка в PROMPT, потенциальная несуместимость имени переменной ENDPOINT с клиентом, отсутствие параметров для детерминизма и защиты от сбоев, а также вопросы безопасности при работе со сторонним прокси. Рекомендации выше адресуют эти моменты без лишних изменений пайплайна.
| .padding(10) | ||
| } | ||
|
|
||
| Spacer() |
There was a problem hiding this comment.
Ниже — только то, что реально можно улучшить или может быть проблемой.
-
Неверное имя SFSymbol: Image(systemName: "clear") почти наверняка не существует. Используйте, например, "xmark.circle", "xmark.circle.fill" или "xmark". Добавьте доступность:
Button {
withAnimation { focusTask.clearTask() }
} label: {
Image(systemName: "xmark.circle.fill")
}
.accessibilityLabel("Сбросить фокус") -
Логика вью против модели: focusTask.task = nil из View — запах. Лучше инкапсулировать в метод модели, который обработает побочные эффекты (остановка таймера, логирование, сохранение):
focusTask.clearTask()
И вызвать его в кнопке, желательно с withAnimation. -
Дублирование ответственности между TaskRowView и FocusTaskRowModifier:
Вы передаете task и в саму строку, и в модификатор. Это повышает риск рассинхронизации (кто отвечает за отображение состояния?).
Предложения:- Либо переместить «фокусную» стилизацию внутрь TaskRowView, управляя ей через Environment/Binding.
- Либо сделать модификатор независимым от task (только визуальный стиль), а данные оставить в TaskRowView.
- Либо завернуть в отдельный FocusTaskRow(task:viewMode:) и убрать модификатор.
-
ViewModifier с Binding на viewMode: модификатор, который читает/меняет состояние, зачастую менее прозрачен. Для поведения лучше использовать обертку-вью или расширение вида с говорящим именем:
extension View {
func focusTaskRowStyle(viewMode: Binding) -> some View { ... }
}
Так проще читать и легче тестировать. -
Макетирование и отступы: .padding(10) — «магическое число». Лучше:
- Использовать системные отступы .padding(.horizontal) или ваши дизайн-токены.
- Если это элемент списка — управлять через .listRowInsets, а не локальный padding.
- Убедитесь, что у кнопки есть достаточная зона тапа: добавить .padding(.horizontal, 8) к самой кнопке или .contentShape.
-
Расположение кнопки: сейчас она вплотную к содержимому. Если нужна кнопка справа, добавьте Spacer() между TaskRowView и Button. При необходимости .buttonStyle(.plain) или .tint для визуальной согласованности.
-
Анимация удаления строки: при сбросе задачи строка исчезает. Явно добавьте анимацию/транзишн, чтобы избежать «рывков»:
withAnimation(.default) { focusTask.clearTask() }
И/или .animation(.default, value: focusTask.task) -
Производительность/идентичность:
- Если task: Identifiable, укажите .id(task.id) на строке, чтобы SwiftUI корректно анимировал замену/удаление.
- Если TaskRowView может быть тяжелым, рассмотрите EquatableView или Equatable протокол для уменьшения перерисовок.
-
Доступность:
- Убедитесь, что TaskRowView объединяет свои подэлементы: .accessibilityElement(children: .combine)
- Кнопке очистки задайте .accessibilityLabel и .accessibilityHint, чтобы не было «кнопка, икс».
-
Соответствие функционалу, который был раньше: ранее томаты отображались только при tomatoesCount > 0. Проверьте, что TaskRowView сохраняет эту условность; иначе это регресс дизайна.
-
Нейминг: viewMode звучит обобщенно. Если это специфично для фокуса, дайте более точное имя (например, FocusViewMode), это улучшит читаемость.
Итого по изменениям: исправьте символ "clear", вынесите очистку в метод модели с анимацией, пересмотрите ответственность между TaskRowView и модификатором, уберите «магический» padding и аккуратно разрулите лейаут и доступность кнопки. Это повысит устойчивость, предсказуемость и качество UX.
| let status = Status(name: name.localizedString(), | ||
| order: order, | ||
| doCompletion: name.competion) | ||
| modelContext.insert(status) |
There was a problem hiding this comment.
Вот что бросается в глаза по диффу, с упором на потенциальные проблемы и улучшения.
-
Семантическая регрессия в логике createSimpleList
- Было: при createSimpleList true создавались все статусы, кроме .progress.
- Стало: при createSimpleList true статусы вообще не создаются.
- Это заметное изменение поведения. Уточните продуктовые требования. Если цель — «простая» доска без .progress, верните фильтрацию, а не полный отказ от статусов.
-
Порядок (order) и инкремент
- В прежней версии инкремент шёл до проверки и мог приводить к «дырам» в order при пропуске статуса.
- Рекомендация: сначала сформировать коллекцию статусов с учётом фильтра, затем использовать enumerated():
- let cases = DefaultProjectStatuses.allCases.filter { !createSimpleList || $0 != .progress }
- for (index, statusCase) in cases.enumerated() { let order = index + 1 ... }
- Это исключит ошибки с порядком и упростит чтение.
-
Локализация и персистентная модель
- Переход с name.rawValue на name.localizedString() для persist-имени может создать проблемы:
- Имя в модели начинает зависеть от языка устройства на момент создания.
- При смене языка существующие записи останутся «на старом» языке.
- Логические сравнения по имени (например, запросы/селекции) перестанут работать, если раньше вы сравнивали с rawValue.
- Рекомендация: хранить стабильный ключ (rawValue) в модели и отображать локализованную строку только в UI. Т.е. в модели поле для ключа, в представлениях — маппинг key -> localized. Если уже храните "name" как user-facing, подумайте о миграции или о добавлении отдельного поля key.
- Переход с name.rawValue на name.localizedString() для persist-имени может создать проблемы:
-
Нейминг и опечатки
- name.competion выглядит как опечатка. Вероятно, должно быть completion. Исправьте API — такие мелочи сильно влияют на читаемость и автодополнение.
- В цикле переменная name — это на самом деле case статуса. Лучше назвать statusCase или kind для ясности.
-
Связь статуса с проектом
- Вставляете project, затем статусы, но в сниппете не видно, привязываете ли статус к проекту (status.project = project или через init). Если связь требуется моделью — не забыть установить, иначе рискуете получить «сирот».
- Аналогично, проверьте, нет ли у Status инвариантов, которые надо заполнить до insert.
-
Транзакция/сохранение
- Если используете SwiftData, проверьте стратегию сохранения: нужна ли явная modelContext.save() здесь (в зависимости от конфигурации). Группировка вставок в транзакцию повысит предсказуемость.
-
Устойчивость порядка к изменениям enum
- Порядок берётся из allCases, который соответствует порядку объявления. Любое переставление кейсов в коде поменяет order по умолчанию. Если порядок критичен и должен быть стабильным — храните явные веса в enum или отдельной таблице.
Предлагаемая форма цикла (сохраняя намерения и устраняя риски):
- let cases = DefaultProjectStatuses.allCases.filter { !createSimpleList || $0 != .progress }
- for (index, statusCase) in cases.enumerated() {
let status = Status(
name: statusCase.rawValue, // стабильный ключ в модели
order: index + 1,
doCompletion: statusCase.completion
)
status.project = project
modelContext.insert(status)
} - Для UI: отображать статус через локализацию rawValue (или через statusCase.localizedString()), но не сохранять локализованную строку в базе.
И напоследок — тесты:
- Покройте оба сценария: createSimpleList = true/false.
- Проверяйте количество созданных статусов, их order (без дыр), и то, что в simple-режиме действительно отсутствует .progress, а не все статусы.
|
|
||
| return NewTaskView(isVisible: $isVisible, list: .inbox) | ||
| .environmentObject(refresher) | ||
| .modelContainer(previewer!.container) |
There was a problem hiding this comment.
Вот замечания по существу и предложения по улучшению.
Архитектура и API SwiftUI
- Дублирование механизма закрытия: сейчас и меняете isVisible = false, и вызываете presentationMode.wrappedValue.dismiss(). Это две конкурирующие модели управления. Выберите одну:
- Современный путь: @Environment(.dismiss) var dismiss и только dismiss().
- Либо полностью контролируйте показ через @binding isVisible и не вызывайте dismiss().
- @Environment(.presentationMode) устаревший. Замените на @Environment(.dismiss).
- NavigationView лучше заменить на NavigationStack (iOS 16+/SwiftData уже требует iOS 17).
- toolbar: сейчас обе кнопки находятся в ToolbarItemGroup(placement: .topBarTrailing) — это нормально. Если вы целитесь в iOS 17+, используйте .topBarLeading/.topBarTrailing. Если нужна совместимость шире — .navigationBarLeading/.navigationBarTrailing.
- @EnvironmentObject var refresher: Refresher не используется вью. Уберите зависимость, чтобы не плодить скрытые требования к окружению.
Состояние и модель данных
- Избыточный @State:
- getListName сейчас отдаёт status?.name раньше, чем project?.name. С UX точки зрения пользователю чаще понятнее видеть "Проект / Статус". Либо верните приоритет project, либо сформируйте строку вида "project.name — status.name". Если mainTask задана — приоритетно показывать её.
Поведение сохранения и SwiftData
- Возможная «двойная вставка» в SwiftData:
- Вы сначала добавляете task в mainTask.subtasks?, а потом вызываете modelContext.insert(task). В SwiftData добавление объекта в связь управляемой модели может автоматически вставить его в контекст. Повторная вставка способна привести к ошибке.
- Рекомендуемый порядок: сначала modelContext.insert(task), затем настраивайте связи (project, mainTask). Либо наоборот, но тогда не вызывайте insert вручную, полагаясь на авто‑инсерты через связь. Главное — один путь, без дублирования.
- mainTask.subtasks? — опциональная связь. Если она nil, append ничего не сделает. Обеспечьте ненулевую коллекцию или присваивайте: mainTask.subtasks = (mainTask.subtasks ?? []) + [task].
- status и project: проверьте, что status относится к project (иначе можно присвоить статус от другого проекта).
- Логика dueToday vs list:
- Сейчас dueToday имеет приоритет, а в иных случаях setDueDate зависит от списка, но только если project == nil и mainTask == nil. Проверьте, точно ли вы хотите игнорировать список, когда задача создаётся внутри проекта/подзадачи.
- Инициализируйте dueToday по умолчанию для list == .today (true) и, возможно, для .tomorrow (false, но сразу назначайте дату на завтра), чтобы UI отражал преднамеренную дату.
- Повторяющийся Calendar.current.startOfDay(for: Date()). Вынесите в локальные переменные (todayStart, tomorrowStart). Для tomorrow избегайте опционала: guard let tomorrowStart = ... else { return }.
- Валидация: предотвратите сохранение пустого имени задачи. Например, отключайте кнопки, пока taskName.trimmingCharacters(in: .whitespacesAndNewlines).isEmpty.
- После «Save and Add next» логично вернуть фокус на поле названия и очистить dueToday при необходимости (или явно решить, что сохраняем предыдущий выбор).
Фокус/клавиатура/UX
- .task { self.focusField = .taskName } может вызываться повторно при ререндерингах. Для простоты используйте .onAppear или контролируйте одноразовость.
- Текстовое поле Link:
- Добавьте .keyboardType(.URL), .textInputAutocapitalization(.never), .autocorrectionDisabled(true).
- Добавьте .submitLabel(.next/.done) и .onSubmit для перехода с имени на линк, а с линка — к сохранению.
- Назначьте accessibility identifier для кнопки «Save and Add next» (например, "SaveAndAddNext").
Текст/локализация
- Все строки («Add task to …», «Task name», «Link», «Due today», «Save and Add next», «OK», «Cancel») — кандидаты на локализацию через LocalizedStringKey/Strings.
Превью
- В превью: previewer!.container — возможный крэш при nil. Используйте guard let или предоставьте мок контейнер гарантированно.
- Если уберёте Refresher из вью, удалите и .environmentObject(refresher) из превью.
- Для iOS 17+ лучше #Preview вместо устаревшего PreviewProvider.
Мелкие улучшения кода
- getListName можно заменить на вычисляемое свойство listTitle или составной заголовок с проектом/статусом.
- Уберите явный .toggleStyle(.switch) — в iOS это и так дефолт.
- Если оставляете вставку даты по списку, вынесите setDueDate логику в более явную стратегию/функцию, чтобы облегчить тестирование.
Итого приоритетные правки:
- Перейти на @Environment(.dismiss) и исключить двойной механизм закрытия (isVisible vs dismiss).
- Убрать @State с list/project/mainTask/status и решить, как они поступают во вью.
- Исправить порядок/способ вставки в SwiftData, чтобы исключить двойной insert и корректно работать с опциональной связью subtasks.
- Ввести валидацию имени и улучшить UX текстовых полей (клавиатура URL, submit flow).
- Привести NavigationView -> NavigationStack.
|
|
||
| TasksQuery.deleteTask(context: modelContext, | ||
| task: task) | ||
| WidgetCenter.shared.reloadAllTimelines() |
There was a problem hiding this comment.
Ниже только то, что действительно стоит поправить.
-
Сравнение задач
- Вы сравниваете задачи через ==. Для SwiftData/Reference‑моделей это часто ненадежно (разные инстансы одной сущности). Лучше сравнивать по устойчивому идентификатору: task.persistentModelID или собственному id. Если хотите именно сравнение по ссылке — используйте ===. Иначе можно пропустить нужный дефокус.
- Упростите условие: if focusTask.task == task { … } без лишнего if let.
-
Повторяющаяся логика снятия фокуса
- Один и тот же паттерн в трех местах: complete, delete и отдельная кнопка. Вынесите в единый метод/сервис, чтобы не разъехались детали (порядок операций, работа с таймером, виджетом).
- Пример:
- FocusCoordinator.defocusIfNeeded(for task, timer: Timer, updateWidget: Bool)
- Внутри: проверка на совпадение, корректная остановка/сброс таймера, обнуление focus, перезагрузка виджетов, нужные сайд‑эффекты.
-
Непоследовательное поведение таймера
- При завершении/удалении фокусной задачи вы только очищаете focusTask.task, но не сбрасываете/останавливаете таймер. Это может оставить таймер в «подвешенном» состоянии без задачи. Приведите к единому сценарию, как в вашей «кнопке снятия фокуса»: reset, а при .pause/.longBreak — skip, либо введите отдельный метод timer.stopForFocusedTaskRemoval().
- Зафиксируйте семантику порядка: если reset/skip зависят от текущей focus‑задачи — сначала операции с таймером, потом focusTask.task = nil. Иначе наоборот. Лучше инкапсулировать, чтобы порядок не расходился в разных местах.
-
WidgetCenter.reloadAllTimelines
- Перезагрузка виджетов есть только при delete. Если виджет показывает фокус/прогресс, обновлять нужно и при complete/снятии фокуса. Вынесите в общий путь обновления состояния.
-
Порядок сайд‑эффектов и навигации
- В блоке с dismiss: обновляйте модель/состояние и останавливайте таймер до закрытия экрана. Это снижает риск гонок/мигающих состояний.
- Рассмотрите использование dismiss() из Environment, вместо устаревающего presentationMode.
-
Микроулучшения читаемости/надёжности
- Сведите дублирующиеся if к guard, чтобы уменьшить вложенность.
- Для enum mode используйте нейминг в стиле Swift: .longBreak, и добавьте вычислимое свойство isBreak, чтобы избежать дублирования условий.
- Вызовы TasksQuery.deleteTask и WidgetCenter вью лучше делегировать во ViewModel/координатор — тестируемость и единая точка бизнес‑логики.
-
Потенциальные проблемы с потоками
- Убедитесь, что операции с моделью и состоянием (SwiftUI/ObservableObject/SwiftData) выполняются на MainActor. Методы complete/reactivate/delete лучше пометить @mainactor или вызывать их с главной очереди.
-
Тесты сценариев
- Добавьте юнит/интеграционные тесты на кейсы:
- Удаление/завершение фокусной задачи: focus очищен, таймер корректно остановлен/сброшен, виджеты обновлены.
- Снятие фокуса кнопкой: порядок операций соблюдён, состояние консистентно.
- Добавьте юнит/интеграционные тесты на кейсы:
Пример возможной унификации (схематично):
- func defocusIfNeeded(for task: Task) {
guard let focused = focusTask.task else { return }
if focused.hasSameIdentity(as: task) { // сравнение по id/persistentModelID
timer.reset()
if timer.mode.isBreak { timer.skip() }
focusTask.task = nil
WidgetCenter.shared.reloadAllTimelines()
}
}
Такая централизация устранит дубли, зафиксирует порядок действий и снизит риск регрессий.
| } | ||
| task.complete(modelContext: modelContext) | ||
| } else { | ||
| task.reactivate() |
There was a problem hiding this comment.
Ниже — только замечания и предложения по улучшению, без повторения того, что уже ок:
-
Риск крэша из‑за EnvironmentObject. @EnvironmentObject требует обязательной инъекции. Если TaskCheckBoxView используется в местах, где FocusTask не прокидывается (включая превью/виджет/тесты), будет runtime crash. Рассмотрите:
- Явную передачу зависимости в инициализатор (например, @ObservedObject focusStore: FocusTask).
- Либо кастомный EnvironmentKey с опциональным значением, чтобы в отсутствие провайдера поведение было безопасным.
- Минимум — пометить свойство как private и убедиться, что все вызовы/превью обеспечивают .environmentObject.
-
Ненадежное сравнение задач: task == focus. Для моделей SwiftData сравнение может быть по ссылке и/или из разных ModelContext — тогда одна и та же сущность может не сравниваться как равная. Лучше сравнивать стабильные идентификаторы:
- Если используете SwiftData: task.persistentModelID.
- Если есть свой id (UUID), сравнивайте его.
Пример: if focusTask.task?.persistentModelID == task.persistentModelID { focusTask.task = nil }
-
Хранение модели внутри FocusTask. Если FocusTask хранит Todo напрямую (например, @published var task: Todo?), это связывает объект с конкретным ModelContext и жизненным циклом. Проблемы:
- «Застывшие» ссылки после удаления/реактивации в другом месте.
- Разные контексты в разных частях UI.
Рекомендация: хранить stable ID (persistentModelID/UUID) и при необходимости резолвить модель из текущего modelContext. Это избавит от утечек/крашей при инвалидных объектах и проблем межконтекстного сравнения.
-
Защита от удаления из другого места. Вы уже снимаете фокус перед complete для текущего сценария — это хорошо. Но если задача будет завершена/удалена извне, FocusTask может продолжать указывать на невалидный объект. Если перейдете на хранение ID, добавьте наблюдение за изменениями/удалениями (или проверку валидности при каждом доступе) и обнуляйте фокус при невозможности резолва.
-
Размещение бизнес-логики в View. Снятие фокуса при завершении — это бизнес-правило. Лучше инкапсулировать в слой действий/интентов/use case, чтобы View вызывал один метод вроде complete(task) и не знал про FocusTask. Это упрощает тестирование и уменьшает связность.
-
Упростить условие и сократить вложенность:
- Вместо if let focus = focusTask.task, task == focus можно if focusTask.task?.id == task.id.
- Структурировать через ранний выход:
Button {
if task.completed {
task.reactivate()
return
}
if focusTask.taskID == task.id { focusTask.taskID = nil }
task.complete(modelContext: modelContext)
}
-
Потокобезопасность. Убедитесь, что FocusTask помечен @mainactor, а его свойства @published. Метод task.complete(modelContext:) тоже должен выполняться на main, если трогает UI-контекст SwiftData.
-
Предпросмотры и тесты. Не забудьте в превью/тестах всегда инжектить FocusTask через .environmentObject, иначе получите падения.
-
Пользовательский опыт. При чекбоксе обычно ожидается анимация. Добавьте withAnimation вокруг изменений состояния, если нет причин избегать анимаций.
Итого ключевые действия:
- Перейти от сравнения моделей к сравнению стабильных идентификаторов.
- Хранить в FocusTask не модель, а ID (и резолвить по текущему modelContext).
- Сделать зависимость безопасной (избежать обязательного EnvironmentObject без гарантированной инъекции).
- По возможности вынести бизнес-правило снятия фокуса из View в слой действий/домена.
|
|
||
| TasksQuery.deleteTask(context: modelContext, | ||
| task: task) | ||
| } |
There was a problem hiding this comment.
Ниже только то, что, на мой взгляд, требует доработки или может привести к ошибкам/путанице.
Логика таймера и переключения фокуса
- Порядок вызовов при Stop. Сейчас: reset() -> if mode == .pause || .longbreak { skip() } -> focusTask = nil. После reset() режим, скорее всего, сбрасывается, и проверка mode теряет смысл; более того, skip() после reset() может увести в неправильное состояние. Если цель — “остановить фокус” и, находясь на брейке, перепрыгнуть его, порядок должен определяться исходным состоянием. Пример безопаснее:
- Сохранить текущий mode/state
- Если on break (.shortBreak/.longBreak) — сначала skip() (вернуться к фокусу), затем reset() или stop()
- Иначе просто stop() или reset()
- Снять фокус с задачи
- Обработка нажатия Start при разных состояниях. В ветке “Start focus” обрабатываются только .idle и .paused. Если таймер уже .running:
- Сейчас фокус на другую задачу просто переключится без воздействия на таймер. Это может быть ок, но тогда это явно задокументировать и, возможно, менять только focusedTask, не трогая reset/start.
- Если нежелательно “пересаживать” активную сессию на другую задачу, кнопку Start для неактивной задачи лучше дизейблить или показывать подтверждение/вариант “Переключить фокус с перезапуском”.
- Нейминг и семантика mode/state. Наличие mode == .pause выглядит подозрительно: “pause” обычно — состояние, а не режим. Для брейков лучше .shortBreak/.longBreak. Чёткое разделение timer.state (.idle/.running/.paused) и timer.mode (.focus/.shortBreak/.longBreak) уменьшит ветвления и ошибки.
- Дублирование логики таймера в View. Сейчас View знает про reset/start/resume/skip и про режимы. Лучше инкапсулировать сценарии в слой модели/сервиса:
- focusManager.startFocus(on: task)
- focusManager.stopFocus()
- focusManager.switchFocus(to: task)
Тогда View вызывает один метод, а сложная логика (включая порядок reset/skip) в одном месте.
Сравнение задач и идентичность
- Сравнение focus == task полагается на Equatable. Для моделей/объектов (Core Data/Observation) это ненадёжно. Лучше сравнивать устойчивые идентификаторы (id/objectID). Иначе можно “потерять” совпадение после фетча/обновления.
Удаление задач и активный фокус
- При удалении фокусной задачи вы обнуляете focusTask.task, это правильно. Но:
- Если таймер шёл на эту задачу, стоит также привести таймер в консистентное состояние (stop/reset/optional skip break). Иначе таймер продолжит тикать на несуществующую задачу.
- Дублирование кода в двух ветках удаления (множественная/одиночная). Вынесите в хелпер:
- defocusIfNeeded(task)
- deleteTasks(_ tasks)
- В множественном удалении используйте более безопасные имена переменных: for selectedTask in selectedTasksSet { ... } — чтобы не перекрывать “task” внешнего контекста.
Мелочи, влияющие на читаемость и надёжность
- if selectedTasksSet.count > 0 → if !selectedTasksSet.isEmpty.
- Единообразная терминология: Stop focus/Start focus vs “pause/longbreak”. Рекомендуется: “Start Focus” / “Stop Focus”, а режимы: Focus / Short Break / Long Break.
- Лейблы кнопок: вместо Image + Text лучше использовать Label("Start Focus", systemImage: "play.fill") — лучше для доступности.
- Повторяющийся код снятия фокуса перед удалением и в Stop-кнопке — вынести в один метод, чтобы не расходились сценарии.
- Обновления состояния, влияющие на UI, можно оборачивать в withAnimation { ... } там, где уместно, чтобы не было “дёрганья” интерфейса.
Предлагаемая структура сценариев (упрощённо)
- startFocus(on task):
- Если текущий focusedTask == task:
- Если state == .paused → resume
- Если state == .idle → reset + start
- Если state == .running → ничего не делать
- Если другой task в фокусе:
- Вариант A: просто focusedTask = task (не трогаем таймер)
- Вариант B: reset + start под новую задачу (чётко определить UX)
- Если mode.isBreak и хотим сразу перейти к фокусу — skip break перед стартом
- Если текущий focusedTask == task:
- stopFocus():
- Если mode.isBreak → сначала skip(), потом reset/stop
- Иначе reset/stop
- focusedTask = nil
- deleteTasks(tasks):
- Если среди tasks есть focusedTask → stopFocus() или минимум reset + focusedTask = nil
- Затем удалить
Итого: главные риски — порядок reset/skip, неполное покрытие состояний при Start/Stop/переключении задачи, сравнение задач по ссылке/Equatable, неполная синхронизация таймера при удалении активной задачи, дублирование и читаемость. Рекомендую инкапсулировать сценарии в отдельный “focus/timer manager” и сравнивать задачи по id.
|
|
||
| TasksQuery.deleteTask(context: modelContext, | ||
| task: task) | ||
| } |
There was a problem hiding this comment.
Ниже — только то, что, на мой взгляд, требует внимания или может быть улучшено.
-
Сравнение текущей задачи с фокусом
- Сейчас используется if let focus = focusTask.task, task == focus. Убедитесь, что вы действительно хотите сравнение по идентичности или по стабильному идентификатору модели.
- Если цель — сравнить одну и ту же сущность SwiftData, надежнее сравнивать по persistentModelID (или вашему стабильному id), а не по Equatable, который легко реализовать некорректно:
- По идентичности экземпляров: focus === task (только если это точно один и тот же объект в памяти; может не сработать при разных инстансах одной записи).
- По идентичности записи: focus.persistentModelID == task.persistentModelID (или focus.id == task.id, если есть стабильный id).
- Рекомендация: использовать сравнение по persistentModelID, чтобы не зависеть от особенностей загрузки/проксирования моделей SwiftData.
-
Локализация побочного эффекта «очистки фокуса»
- Сейчас очистка фокуса выполняется прямо в обработчике свайпа. Это легко забыть повторить в других сценариях удаления (контекстное меню, хоткей, массовое удаление и т.д.).
- Варианты улучшения:
- Инкапсулировать логику в helper на FocusTask: focusTask.clearIfMatches(task) — и вызывать его из всех мест.
- Или централизовать в уровне данных: TasksRepository.delete(task) внутри сам очищает FocusTask/Selection/Inspector (через делегаты/нотификаторы), чтобы любые вызовы delete были консистентны.
-
Координация с SelectedTasks/Inspector
- Если удаляемая задача может быть выбрана или открыта в инспекторе, стоит синхронизированно сбрасывать и эти состояния, иначе получите «висячие» ссылки/пустые представления.
- Если это уже делается в другом месте — ок. Если нет, добавьте: deselect удаляемой задачи и закрытие/очистку инспектора при совпадении.
-
Надежность внедрения FocusTask
- @EnvironmentObject крашится при отсутствии объекта (особенно в Preview). Если FocusTask критичен везде — оставляйте, но убедитесь, что он всегда инжектится.
- Альтернатива: свой EnvironmentKey и @Environment(.focusTask) var focusTask (можно сделать optional), либо использовать @Environment(FocusTask.self) при @Observable-модели в iOS 17+/macOS 14+.
- Если остаётесь на EnvironmentObject, для стабильности превью/тестов добавьте явные заглушки.
-
Область withAnimation
- Вы анимируете и очистку фокуса, и удаление. Возможно, вы хотите анимировать только исчезновение строки, а очистку фокуса — без анимации, чтобы избежать «скачков» в деталке/инспекторе.
- Пример: сначала очистить фокус без анимации, затем с анимацией удалить из списка:
- withTransaction(Transaction(animation: nil)) { focusTask.clearIfMatches(task) }
- withAnimation { TasksQuery.deleteTask(...) }
-
Обработка ошибок и поток выполнения
- Если TasksQuery.deleteTask может бросать/быть асинхронным, приведите к явной модели:
- Выполняйте изменение UI-состояния на MainActor.
- Если удаление async, оберните в Task { @mainactor in ... } и разделите анимацию UI и вызов удаления, чтобы не смешивать долгие операции с UI-анимацией.
- Если удаление синхронно — как минимум логируйте ошибки или учитывайте возможные сбои при сохранении контекста, чтобы не остаться с очищенным фокусом и не удалённой задачей.
- Если TasksQuery.deleteTask может бросать/быть асинхронным, приведите к явной модели:
-
UX аспекты свайпа на удаление
- Подумайте об отключении allowsFullSwipe или подтверждении удаления, если в продукте есть риск случайных свайпов.
- Возможно, стоит добавить Undo (Environment.undoManager) для удаления, чтобы пользователь мог восстановить.
-
Тестируемость
- Прямой вызов TasksQuery.deleteTask из ViewModifier затрудняет тесты. Рассмотрите внедрение протокола TaskRepository и инъекцию реализации через Environment, чтобы можно было подменять в тестах/превью.
-
Платформенная условность
- Ранее EnvObject’ы были под #if. Новый focusTask — вне условной компиляции. Убедитесь, что он действительно доступен и нужен на всех платформах, иначе добавьте соответствующие #if или обеспечьте инъекцию везде.
Короткий итог правок, которые можно внести сразу:
- Заменить сравнение на persistentModelID:
- if focusTask.task?.persistentModelID == task.persistentModelID { focusTask.task = nil }
- Вынести очистку фокуса в метод FocusTask.clearIfMatches(_:) и вызывать его из всех мест удаления.
- Разделить анимацию удаления и очистку фокуса, чтобы избежать лишних UI-дерганий.
- Проверить, что SelectedTasks/Inspector сбрасываются при удалении той же задачи.
- Обеспечить безопасную инъекцию FocusTask (особенно для превью) или перейти на @Environment с кастомным ключом.
|
|
||
| TasksQuery.deleteTask(context: modelContext, | ||
| task: task) | ||
| } |
There was a problem hiding this comment.
Ниже — только замечания и предложения по улучшению, без повторения очевидно рабочих мест.
-
EnvironmentObject FocusTask
- Убедитесь, что FocusTask прокинут во все родительские контейнеры и в Preview; иначе будет крэш в рантайме. Хорошая практика — добавить assert в инициализации верхнего контейнера или хотя бы не забыть про .environmentObject(FocusTask()) в превью.
-
Удаление и состояние (deleteItems/deleteTask)
- Сравнение task == focus потенциально хрупкое: для моделей SwiftData надежнее сравнивать по устойчивому идентификатору (например, persistentModelID/persistentIdentifier) вместо Equatable по ссылке/значению. Иначе можно не поймать совпадение при разных экземплярах одной и той же сущности.
- При пакетном удалении лучше делать snapshot выбора перед итерацией, чтобы не итерироваться по коллекции, которая может меняться при удалении:
- let toDelete = Array(selectedTasks.tasks)
- Затем очищать selectedTasks.tasks от удалённых, чтобы не держать «висячие» ссылки в selection после удаления.
- Оберните удаления в withAnimation { … } для корректной анимации исчезновения строк.
- Если удаление родителя каскадно удаляет подзадачи, и фокус стоит на подзадаче, текущая проверка это не поймает. Имеет смысл очистить фокус, если focusTask.task совпадает с удаляемым или является его потомком.
-
DisclosureGroup: label и подсчёт
- Подсчёт в label сейчас асимметричный: для .completed вы фильтруете только верхнеуровневые (parentTask == nil), а для остальных — все незавершённые, включая подзадачи. Итоговые цифры не будут соответствовать фактически выводимым элементам. Приведите логику к одному правилу (например, всегда считать только top-level для всех секций, если в списке показываете только их).
- searchResults.filter(...) вызывается в label дважды на каждый ререндер. Это O(n) на обновление секции. Вынесите подсчёт в мемоизированное свойство/кеш или хотя бы в локальные let-переменные до объявления List/DisclosureGroup.
- Текущий код считает «не завершённые» для всех секций, кроме .completed. Если enum CommonTaskListSections имеет больше двух секций, цифры точно неправильные. Сформируйте единый источник истины: функция count(for:), которая использует ту же фильтрацию, что и контент секции.
- Локализация/доступность: сейчас показывается просто число с пробелом: " 5". Лучше:
- Отрисовывать в скобках и/или использовать локализованный формат: String(localized: " (%d)", count).
- Для цвета используйте .foregroundStyle(.secondary), а не Color.gray — так цвета корректны в разных режимах/доступности.
- Добавьте .accessibilityLabel для числа, например “Количество задач: 5”.
-
.listRowSeparator(.hidden)
- Модификатор дублируется на нескольких уровнях (и у ForEach, и у строк). Оставьте его в одном месте. Если цель — скрыть разделители повсеместно, лучше применить:
- На List: .listStyle(.plain) и .listRowSeparator(.hidden) (и при необходимости .listSectionSeparator(.hidden) для iOS 16+/macOS 13+).
- Применение .listRowSeparator(.hidden) к DisclosureGroup часто не даёт ожидаемого эффекта. Проще централизованно управлять стилем списка.
- Модификатор дублируется на нескольких уровнях (и у ForEach, и у строк). Оставьте его в одном месте. Если цель — скрыть разделители повсеместно, лучше применить:
-
dropDestination(for: Todo.self)
- В замыкании параметр называется tasks и затеняет @query var tasks: [Todo]. Это ухудшает читаемость. Переименуйте параметр, например, в droppedTasks.
- Не забудьте вернуть Bool из closure (return true), иначе drop может не считаться принятым. В диффе возврата не видно.
- Если есть логика, зависящая от секции (куда дропнули), возможно, стоит использовать itemProviders или локальный контекст, чтобы корректно интерпретировать операцию.
-
Стиль и читаемость
- Избегайте $0.completed == false; короче и понятнее !$0.completed.
- Вынесите связку Binding(get:set:) для isExpanded в отдельный хелпер:
- private func isExpandedBinding(for section: CommonTaskListSections) -> Binding
- Это снимет визуальный шум из body и исключит дублирование.
- Ключ в groupsExpanded — section.rawValue. Если rawValue когда‑нибудь изменится, пользователи потеряют сохранённые состояния. Лучше хранить стабильный идентификатор (например, явно заданный case id) или сам enum (через Hashable) в Set, если он Codable/Hashable.
- Label DisclosureGroup сейчас — HStack из двух Text. Для лучшей кликабельности можно задать .contentShape(Rectangle). Это увеличивает область нажатия по всей строке.
-
Производительность
- Многократные filter по searchResults в разных местах (контент и label) дадут лишние аллокации. Сведите фильтрации к одному месту: сначала подготовьте коллекции для каждой секции (top-level completed, top-level pending и т.д.), а затем используйте их и для отображения, и для подсчёта.
- Если searchResults — @Query-результат SwiftData — попробуйте перенести часть фильтрации на уровень запроса, чтобы не делать это в UI-потоке на каждое обновление.
-
Прочее
- Убедитесь, что .tag(...) совпадает по типу с selection и используется последовательно для родительских и дочерних строк. Смешение тегов для разных уровней иерархии может приводить к странному поведению выбора.
- Проверьте превью: после добавления @EnvironmentObject var focusTask: FocusTask он нужен и там.
- Если при удалении задач есть побочные эффекты (например, сброс инспектора, закрытие деталей), синхронизируйте их с уже добавленным сбросом фокуса.
Итоговые рекомендации к рефакторингу:
- Вынести логику фильтрации и подсчёта в отдельные методы/свойства, чтобы и контент, и счетчики использовали единый источник данных.
- Централизовать скрытие разделителей на уровне List.
- Исправить сравнение focus с удаляемой задачей на сравнение по стабильному идентификатору.
- Очистка selection после удаления и обход по snapshot.
- Привести отображение счётчиков к локализуемому и доступному виду, убрать абсолютный Color.gray.
- Переименовать параметры dropDestination, вернуть Bool.
|
|
||
| }, | ||
| "Add task to Inbox" : { | ||
| "localizations" : { |
There was a problem hiding this comment.
Наблюдения и рекомендации по правке:
-
Похоже, это .xcstrings (Strings Catalog). Вы добавили ключ "Add task to current status" с пустым объектом. В каталоге строк такие записи должны содержать хотя бы "localizations" для языка разработки (и, желательно, "comment"). Пустая запись, как правило, игнорируется при сборке или порождает предупреждения, а в рантайме приведёт к падению поиска строки/фолбэку на ключ. Рекомендация: либо сразу заполнить локализации, либо не коммитить ключ-заглушку.
-
Синтаксис/скобки/запятые. В диффе присутствует вставка "}," перед добавлением нового ключа. Проверьте целостность JSON (или схемы .xcstrings) линтером/валидатором в CI: лишняя/недостающая запятая на этом уровне легко ломает сборку. Простая проверка: plutil -lint или xcodebuild с включёнными warnings по String Catalog.
-
Неполнота структуры относительно соседних ключей. Для единообразия с "Add task to Inbox" добавьте:
- localizations по всем поддерживаемым языкам;
- comment для переводчиков (контекст, где используется строка);
- при необходимости variations (pluralization/динамика), если это допускается схемой.
-
Семантика "current status". Если строка относится к App Intents/Shortcuts или фоновой логике, понятие «текущий» контекст UI недоступно. Интенты не знают «текущий экран» приложения. Это частое недопонимание:
- Сделайте статус явным параметром в интенте (со значением по умолчанию).
- Или храните «последний выбранный статус» в общих настройках и явно укажите этот fallback в описании/комментарии строки, чтобы не вводить пользователя в заблуждение.
- В UI-контекстах фраза «current» допустима, но для системных шорткатов лучше избегать двусмысленности.
-
Терминология/единообразие. Убедитесь, что во всём приложении вы используете один и тот же термин: status vs list/project/column. Если в соседних строках используется «Inbox» как список, возможно, корректнее «Add task to current list». Если всё-таки «status» — это доменная сущность (канбан-статус), то оставьте «status», но будьте последовательны в других строках.
-
Кейсы и стиль формулировок. Держите стиль в одном регистре и синтаксисе с соседними строками (у вас императив «Add task to …» — ок). Если у вас практика «ключ = исходная фраза», соблюдайте тот же шаблон. Если команда предпочитает стабильные ключи, лучше завести ключ вида task.add.currentStatus и значения для локализаций.
-
Тесты и качество. Добавьте в CI шаг проверки String Catalog:
- валидность схемы (plutil/JSON validator);
- наличие значения в языке разработки для каждой новой строки;
- отсутствие пустых записей;
- проверка консистентности локализаций.
Что бы я предложил поправить сразу:
- Либо удалить пустой блок до готовности локализаций, либо заполнить минимумом:
- comment с контекстом использования,
- localizations/en (и остальные базовые),
- при необходимости уточнить формулировку, если строка используется вне UI-контекста.
- Перепроверить запятые/скобки рядом с добавленным блоком автоматическим валидатором.
- Если строка для интента/шортката — переосмыслить «current» как параметр, а не скрытую зависимость от UI.
No description provided.