-
Notifications
You must be signed in to change notification settings - Fork 2
sync experiments #132
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
sync experiments #132
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code review by ChatGPT
OPENAI_API_ENDPOINT: https://api.proxyapi.ru/openai/v1 | ||
MODEL: o1-mini | ||
MODEL: o4-mini | ||
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ниже — пара замечаний по вашему изменению, которые могут помочь сделать GitHub Action чуть более надёжным и удобным:
-
Правильность названия модели
Вы меняетеMODEL
сo1-mini
наo4-mini
, но в официальном API OpenAI нет моделей с такими именами. Если вы проксируете запросы через «proxyapi.ru» и у них действительно есть кастомные «o1-mini»/«o4-mini», то ок, но если же вы рассчитываете на официальные GPT-4-mini-подмодели — их идентификатор будет простоgpt-4
(евентуально с точной версией, вродеgpt-4-0613
). Рекомендую уточнить документацию вашего прокси и поставить корректный model_id, иначе Action будет падать с ошибкой «model not found». -
Язык взаимодействия лучше параметризовать более описательно
ПеременнаяLANGUAGE
вполне рабочая, но неочевидна — легче было бы назвать её, например,REVIEW_LANGUAGE
илиLOCALE
. Тогда сразу понятно, зачем она служит. -
Многострочный PROMPT для удобства чтения
Сейчас вы вставляете довольно длинный текст в одну строку:
PROMPT: "You are an experienced Swift/SwigtUI developer …"
При добавлении новых условий будет неудобно править. Можно оформить так:PROMPT: > You are an experienced Swift/SwiftUI 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.
Это не повлияет на работу, но облегчит поддержку.
-
Проверка наличия обязательных переменных
Если какой-то секрет не прокинулся, Action упадёт с неочевидной ошибкой. Можно прямо в шаге перед обращением к API добавить bash-скрипт с проверкой:- name: Validate env run: | for var in GITHUB_TOKEN OPENAI_API_KEY REVIEW_LANGUAGE MODEL; do if [ -z "${!var}" ]; then echo "ERROR: $var is not set" >&2 exit 1 fi done
Это даст понятный лог.
-
Унифицируйте версию действия и endpoint
Похоже, вы используете кастомный проксиhttps://api.proxyapi.ru/openai/v1
. Убедитесь, что он точно поддерживает те же параметры, что и OpenAI, — особенно если придут новые поля (stream, functions и т. п.).
В остальном сама правка (смена языка на русский и модели) выглядит корректно. Если вы проверили, что модель действительно существует, и прокси адекватно её отдаёт — можно смержить.
@@ -62,6 +64,7 @@ struct ContentView: View { | |||
title: selectedSideBarItem!.name) | |||
.id(refresher.refresh) | |||
.refreshable { | |||
try? modelContext.save() | |||
refresher.refresh.toggle() | |||
} | |||
.environmentObject(showInspector) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ниже — несколько замечаний и рекомендаций по вашему PR. Я не комментирую то, что уже работает, а концентрируюсь на потенциальных проблемах и возможностях улучшения.
-
Дублирование кода
Вы трижды подряд вставляете один и тот же кусок:.refreshable { try? modelContext.save() refresher.refresh.toggle() }
Это нарушение принципа DRY (Don’t Repeat Yourself). Лучше вынести это в отдельный вью-модификатор или функцию:
extension View { func saveOnRefresh(modelContext: ModelContext, refresher: RefreshTrigger) -> some View { self.refreshable { do { try modelContext.save() } catch { // логируем или показываем ошибку } refresher.refresh.toggle() } } }
И в теле:
content .id(refresher.refresh) .saveOnRefresh(modelContext: modelContext, refresher: refresher) .environmentObject(showInspector)
-
Мгновенное «замалчивание» ошибок (try?)
Использованиеtry?
может заглушить реальные проблемы при сохранении. Вам стоит:
– Ловить ошибку вdo-catch
.
– Либо хотя бы залогировать её (os_log, print или собственный Logger), чтобы в случае падения Core Data/SwiftData вы понимали, что пошло не так. -
Логика сохранения в .refreshable
.refreshable задумывался для «pull to refresh» (получения актуальных данных из сети, БД и т.д.), а не для сохранения изменений в контексте.
– Если ваша цель — запушить локальные изменения в базу, лучше это делать в момент, когда пользователи действительно изменяют данные (в .onChange, при закрытии редактора, нажатии «Сохранить» и т.п.).
– «Сохранение на рефреш» может быть неожиданным для пользователя и неочевидным для архитектуры. -
Хак с .id(refresher.refresh)
Использование меняющегося id для того, чтобы «перезагрузить» SwiftUI-вью — тоже скорее workaround. Если вы используете @fetchrequest (или @query в SwiftData), SwiftUI будет автоматически обновлять UI при изменении данных. Поищите более декларативный способ:
– @fetchrequest для CoreData или @Query/Observable<…> для SwiftData.
– При изменении @published поля в ViewModel SwiftUI перерисует интерфейс. -
Принудительная распаковка Optional
selectedSideBarItem!.name
потенциально может привести к краху, если item = nil. Лучше безопасно развернуть:if let item = selectedSideBarItem { DetailView(title: item.name) // … } else { Text("Нет выбора") }
-
Распределение environmentObject
Вы три раза прокидываетеenvironmentObject(showInspector)
. Если дочерние вью все используют один и тот же объект, достаточно прокинуть его однажды в верхнем уровне ContentView. -
UX и асинхронность
Если сохранение контекста может занимать время или быть асинхронным, подумайте о:
– Показывать индикатор загрузки.
– Делатьawait modelContext.save()
в async-контексте (для SwiftData). -
Совместимость по версиям iOS
.refreshable доступен с iOS 15+. Если таргетите и более старые версии, нужно оборачивать в условный модификатор.
Итоговые рекомендации:
– Уберите дублирование, вынесите refreshable
в своё расширение.
– Обработайте ошибки сохранения.
– Пересмотрите логику: «save on pull-to-refresh» звучит необычно. Возможно стоит сохранять сразу после редактирования.
– Откажитесь от id-хаков, используйте @fetchrequest / @query / ObservableObject для автоматического обновления.
– Уберите принудительные ! и прокидывайте environmentObject один раз в корень.
Это сделает код более надёжным, декларативным и легче поддерживаемым.
case .tomorrow: | ||
task.dueDate = Calendar.current.date(byAdding: .day, value: 1, to: Calendar.current.startOfDay(for: Date())) | ||
task.setDueDate(modelContext: modelContext, dueDate: Calendar.current.date(byAdding: .day, value: 1, to: Calendar.current.startOfDay(for: Date()))) | ||
case .projects: | ||
break | ||
case .review: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ниже – несколько замечаний и предложений по предлагаемым правкам. Основная мысль: текущее решение немного «замусоривает» View лишней бизнес-логикой и избыточно прокидывает ModelContext
по методам, тогда как SwiftData уже умеет автоматически отслеживать и сохранять изменения.
-
Избыточная передача
modelContext
в методы расширения- Почему это плохо?
• В SwiftData экземпляр модели уже связан с контекстом (при вставке черезmodelContext.insert
). Дополнительная прокруткаmodelContext
в каждый метод усложняет сигнатуру и связывает View с реализацией модели. - Что делать?
• Сделать методы-расширения на стороне модели «чистыми» по API и свободными от контекста. Например:А уже внутри View (или, еще лучше, ViewModel) – единожды вызватьextension Todo { mutating func setDueDate(_ date: Date) { self.dueDate = date } }
modelContext.insert(task)
и затем чистоtask.setDueDate(date)
.
- Почему это плохо?
-
Разделение ответственности: вынос бизнес-логики из View
- Сейчас в
NewTaskView
у вас склеены сразу:
• расчёт даты (today/tomorrow/…)
• вставка в контекст
• «переподключение» черезreconnect
• добавление в локальный массивtasks
- Рекомендация:
• Завести ViewModel (например,NewTaskViewModel
), инжектировать в неёModelContext
и уже внутри неё:
– принимать параметры (имя, ссылка, секция карточки и т.п.),
– считать нужную dueDate,
– делатьmodelContext.insert(task)
,
– вызыватьtask.reconnect()
(без передачи контекста, если метод реализовать на модели).
• View остается максимально декларативной: только биндинг полей и кнопка «Сохранить → viewModel.createTask()».
- Сейчас в
-
Централизация расчёта даты
- Сейчас вы в трёх местах повторяете
Calendar.current.startOfDay(for: Date())
+ смещение на 1 день. - Предложение: вынести в extension на Date:
extension Date { var startOfDay: Date { Calendar.current.startOfDay(for: self) } func addingDays(_ days: Int) -> Date? { Calendar.current.date(byAdding: .day, value: days, to: startOfDay) } }
- Тогда код упрощается до:
task.setDueDate(.now.startOfDay) // или task.setDueDate(.now.addingDays(1))
- Сейчас вы в трёх местах повторяете
-
task.reconnect()
vstask.reconnect(modelContext:)
- Аналогично сеттеру даты, метод
reconnect
лучше позаботиться о контексте внутри модели (или вообще реализовать через@PostInsert
-хук SwiftData, если нужно), а View не должен передавать контекст вручную.
- Аналогично сеттеру даты, метод
-
Локальный массив
tasks
и отражение изменений- Вы сами вставляете
task
вtasks.append(task)
. Но если у вас есть@Query
на все задачи из SwiftData, то этот массив дублирует источник правды. - Либо полностью перейти на
@Query
, либо убрать дублирующие коллекции и работать только через контекст.
- Вы сами вставляете
-
Проверка на
project == nil && mainTask == nil
- Сейчас логика «если не привязан к проекту и не вложен в задачу» дублируется вместе с установкой dueDate.
- Можно в модели сделать фабричный конструктор:
– внутри него целиком строить задачу, устанавливать
static func makeNew(name: String, link: URL?, context: ModelContext, section: Section) -> Todo { … }
dueDate
и сразу вставлять в контекст. - Тогда View просто вызовет:
let task = Todo.makeNew( name: taskName, link: link, context: modelContext, section: selectedSection )
Итог:
• Уберите лишнюю передачу modelContext
в методы setDueDate
и reconnect
.
• Перенесите всю логику создания и конфигурации задачи в отдельный слой (ViewModel или фабрику в модели).
• В View оставьте только биндинги и вызов финального метода создания/сохранения.
• Вынесите утилитарные расчёты дат в Date
-extension.
Это сделает код чище, легче тестируемым и более соответствующим MVVM-подходу.
@@ -149,6 +150,7 @@ struct MainView: View { | |||
} | |||
.onChange(of: scenePhase) { oldPhase, newPhase in | |||
if newPhase == .active && (oldPhase == .background || oldPhase == .inactive) { | |||
try? modelContext.save() | |||
refresher.refresh.toggle() | |||
timer.removeNotification() | |||
} else if newPhase == .background && timer.state == .running { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ниже — несколько замечаний и рекомендаций по участку, где вы добавили сохранение контекста в реакцию на изменение scenePhase
. Постарался не касаться того, что уже «работает», а именно сконцентрироваться на потенциальных проблемах и улучшениях.
-
Не игнорируйте возможные ошибки при сохранении
Сейчас вы используетеtry? modelContext.save()
Такой подход «молча» заглатывает любые сбои и усложняет отладку. Лучше явно обрабатывать ошибки, хотя бы в лог:
do { if modelContext.hasChanges { try modelContext.save() } } catch { // Здесь можно использовать ваш логгер или просто print print("Failed to save modelContext on .active: \(error)") }
-
Проверка изменений перед сохранением
Вызываяsave()
без проверки, каждый раз будете пытаться сохранить, даже если ничего не менялось. Это дополнительная нагрузка. Проверяйте:if modelContext.hasChanges { try modelContext.save() }
-
Поток выполнения и изоляция данных
SwiftData (и CoreData) ожидают, что вы будите работать с контекстом в том же потоке/акторе, где он создан. ЕслиmodelContext
— это обычный MainActor-контейнер, то всё ок. Но если вы вдруг передали контекст в фон, то save может падать или блокировать UI. Убедитесь, что вызываете save в нужном контексте (главном акторе), либо явно оборачиваете вTask { @MainActor in … }
. -
Совместимость .onChange(old,new)
Подпись.onChange(of: scenePhase) { oldPhase, newPhase in … }
доступна начиная с iOS 17 / macOS 14. Если таргетите более ранние версии платформ, это не скомпилируется. Либо:
- Поменять на классический вариант
.onChange(of: scenePhase) { newPhase in … }
и вручную хранить старое состояние в@State
. - Либо увеличить минимальную версию SDK.
- Поменять на классический вариант
-
Разделение ответственности (чистота кода)
Чем более «громоздким» становится ваш.onChange
, тем хуже читаемость. Я бы вынес логику в приватный метод:.onChange(of: scenePhase) { old, new in handleScenePhaseChange(from: old, to: new) } private func handleScenePhaseChange(from old: ScenePhase, to new: ScenePhase) { if new == .active, old != .active { saveContextIfNeeded() refresher.refresh.toggle() timer.removeNotification() } else if new == .background, timer.state == .running { timer.scheduleNotification() } }
-
Убедитесь, что у вас правильно прокинута модель
Добавили вы@Environment(\.modelContext)
вMainView
, но не забудьте, чтобы в корне приложения или в нужном контейнере был установлен.modelContainer(...)
(SwiftData). Иначе при запуске получите runtime-краш. -
Не забывайте про unit-тесты
Если вам важно корректное сохранение при сменеscenePhase
, имеет смысл замокатьmodelContext
и протестировать, что при разных комбинациях фаз вызывается именно ваш метод сохранения.
В целом идея с сохранением на рестарт приложения (переход из background → active) правильная. Лишь рекомендую сделать это более надёжно (обработка ошибок, проверка hasChanges
, вынесение кода в методы) и проследить за совместимостью .onChange(old,new)
.
@@ -22,7 +22,7 @@ struct NewTaskView: View { | |||
let task = Todo(name: taskName) | |||
|
|||
if dueToday { | |||
task.dueDate = Calendar.current.startOfDay(for: Date()) | |||
task.setDueDate(modelContext: modelContext, dueDate: Calendar.current.startOfDay(for: Date())) | |||
} | |||
|
|||
modelContext.insert(task) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ниже — несколько замечаний и предложений, которые, на мой взгляд, могут сделать код более надёжным и читаемым. Если какие-то пункты для вас уже не актуальны — смело пропускайте.
-
Порядок вставки и модификации объекта в SwiftData
Сейчас у вас получается так:let task = Todo(name: …) if dueToday { task.setDueDate(modelContext: modelContext, dueDate: …) } modelContext.insert(task)
Но по факту
setDueDate
запускает транзакцию черезmodelContext
. Поскольку объект ещё не вставлен в контекст, попытка внутри транзакции что-то поменять может привести к неожиданному поведению (или даже runtime-ошибке). Я бы переставил вызовы местами:let task = Todo(name: taskName) modelContext.insert(task) if dueToday { task.setDueDate(modelContext: modelContext, dueDate: Calendar.current.startOfDay(for: Date())) }
Тогда вы гарантированно изменяете атрибут уже «знакомого» SwiftData-контекста объекта.
-
Вынесение бизнес-логики из View
СозданиеTodo
, вставку и установкуdueDate
лучше вынести в отдельный ViewModel или в какой-либо сервис-слой.
Сейчас вьюха напрямую оперируетmodelContext
и знает про транзакции — в крупном проекте это затруднит тестирование и переиспользование.
Пример упрощённого подхода:class NewTaskViewModel: ObservableObject { @Published var name = "" @Published var dueToday = false private let modelContext: ModelContext init(context: ModelContext) { self.modelContext = context } func save() { let todo = Todo(name: name) modelContext.insert(todo) if dueToday { todo.setDueDate(modelContext: modelContext, dueDate: .startOfToday) } } } struct NewTaskView: View { @StateObject private var vm: NewTaskViewModel @Environment(\.modelContext) private var context init() { _vm = StateObject(wrappedValue: NewTaskViewModel(context: context)) } var body: some View { … } }
Такой подход отделяет UI от логики работы со SwiftData.
-
Упрощение установки
dueDate
ЕслиdueDate
— всего лишь свойство сущности, не обязательно писать для этого отдельный метод с транзакцией. Можно:- либо вставить объект в контекст до изменения и просто присвоить
task.dueDate = …
- либо обернуть сразу несколько изменений в один
modelContext.transaction { … }
, если в будущем добавится больше полей.
К примеру:
modelContext.insert(task) modelContext.transaction { if dueToday { task.dueDate = .startOfToday } // в будущем можно добавить другие поля }
Это чище, чем создавать метод расширения на каждое свойство.
- либо вставить объект в контекст до изменения и просто присвоить
-
Утилита для «начала дня»
ПовторяющийсяCalendar.current.startOfDay(for: Date())
можно вынести в static-свойство, чтобы назвать его понятнее:extension Date { static var startOfToday: Date { Calendar.current.startOfDay(for: Date()) } }
Тогда в коде будет выглядеть лаконичнее:
Date.startOfToday
. -
Обработка ошибок и валидация
Сейчас создаётсяTodo(name: taskName)
без проверки на пустоту строки. Можно:- блокировать кнопку «Сохранить», пока строка пустая
- показывать ошибку
- или в
save()
проверятьguard !name.isEmpty else { return }
-
Тестируемость
Если спокойно вынести работу со SwiftData в ViewModel или сервис, вы получите возможность писать unit-тесты без UI:- мокать
ModelContext
(или заворачивать операции в протокол). - проверять, что при
dueToday == true
у создаваемого объекта выставилсяdueDate
.
- мокать
Итого: основной анти-паттерн здесь — это присвоение через отдельный метод до вставки в modelContext
. Второе — смешение UI и бизнес-логики. В остальном концепция с транзакцией в методе расширения вполне жива, но часто избыточна для простого поля.
@@ -45,6 +45,7 @@ struct TaskSwipeModifier: ViewModifier { | |||
mainTask: task) | |||
.id(refresher.refresh) | |||
.refreshable { | |||
try? modelContext.save() | |||
refresher.refresh.toggle() | |||
} | |||
.environmentObject(showInspector) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ниже — несколько замечаний и рекомендаций по фрагменту с .refreshable
, где вы добавили try? modelContext.save()
:
-
Сохранение в pull-to-refresh выглядит не по назначению
Обычно.refreshable
используют для «обновления» данных из внешнего источника (сети, базы, файлов и т. д.), а не для сохранения локальных изменений. В вашем случае непонятно, что вы «сохраняете» при каждом жесте pull-to-refresh и зачем это делать именно здесь. Если задача — периодически сбрасывать накопленные правки, перенеситеsave()
в более семантичное место (кнопка «Сохранить»,onDisappear
, контроллер состояний и т. п.). -
Молчаливое подавление ошибок
Вы используетеtry?
, и если сохранение отвалится — потеряете информацию об ошибке. Рекомендую минимум логировать её или показывать пользователю:.refreshable { do { try modelContext.save() } catch { print("Ошибка сохранения контекста: \(error)") // можно показывать Alert } refresher.refresh.toggle() }
-
Асинхронность и блокировка UI
.refreshable
— асинхронный замыкатель, и хотяmodelContext.save()
обычно работает быстро, при большом количестве изменений может заблокировать главный поток и подвесить анимацию «тянущейся» пробы. Если вы используете SwiftData (гдеsave()
синхронный), возможно, лучше выносить тяжёлые операции в фон:.refreshable { await Task.detached(priority: .userInitiated) { do { try modelContext.save() } catch { // логика обработки } }.value refresher.refresh.toggle() }
-
Хак с
.id(refresher.refresh)
Привязка ID к булеву флагу — распространённый приём «перерисовать всю вьюшку», но это костыль и может приводить к потере состояния вложенных вью.- Лучше отделить логику обновления данных от самого представления: использовать
@StateObject viewModel
и менять@Published var items
, чтобы SwiftUI сам понимал, какие строки поменялись. - Если вам действительно нужно форсить полный ре-рендер — документируйте в комментариях причины, иначе следующий разработчик удивится «откуда взялся» id-хак.
- Лучше отделить логику обновления данных от самого представления: использовать
-
MVVM и ответственностное разделение
Сейчас вы смешиваете логику сохранения (ModelContext) прямо во ViewModifier’е. Рекомендую:- Завести отдельный ViewModel (или ObservableObject) с методом
refresh()
, который внутри делаетsave()
и/или новый fetch. - Внутри
.refreshable
просто вызыватьawait viewModel.refresh()
.
- Завести отдельный ViewModel (или ObservableObject) с методом
-
Тесты и предсказуемость
После всех этих изменений протестируйте:- Что происходит на реальном устройстве/симуляторе при pull-to-refresh (анимация не «зависает»).
- Обрабатываются ли ошибки, и не приводят ли они к «повисанию» UI.
В общем, основная рекомендация — вынести сохранение из зоны UI‐дескриптора, грамотно обрабатывать ошибки и не использовать «force-refresh» через id, а полагаться на реактивные обновления состояния.
case .tomorrow: | ||
task.dueDate = Calendar.current.date(byAdding: .day, value: 1, to: Calendar.current.startOfDay(for: Date())) | ||
task.setDueDate(modelContext: modelContext, dueDate: Calendar.current.date(byAdding: .day, value: 1, to: Calendar.current.startOfDay(for: Date()))) | ||
case .projects: | ||
break | ||
case .review: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ниже — несколько наблюдений и предложений по улучшению, на что стоит обратить внимание:
-
Группировка сохранений
- Сейчас вы делаете «try? modelContext.save()» только в ветке реактивации и вообще вызываете save один раз на задачу. Если при дропе переносите несколько тасков в цикле, логичнее накопить все изменения и сохранить один раз после завершения цикла, а не внутри каждой итерации. Это уменьшит накладные расходы на транзакции и снизит риск частично сохранённых изменений при сбое.
-
Обработка ошибок при сохранении
- «try? modelContext.save()» глушит все исключения. Лучше хотя бы залогировать ошибку или показать пользователю, иначе вы никак не узнаете, что сохранение провалилось:
• Перейти на «do { try modelContext.save() } catch { print("Save error:", error) }»
• Или сделать пользовательский алерт.
- «try? modelContext.save()» глушит все исключения. Лучше хотя бы залогировать ошибку или показать пользователю, иначе вы никак не узнаете, что сохранение провалилось:
-
Последовательность вызова методов
- Вы в одном месте вызываете «disconnectFromParentTask(modelContext:)» и тут же вручную обнуляете «task.parentTask = nil». Возможно, внутри вашего метода «disconnectFromParentTask» уже идёт сброс parentTask и save, поэтому этот прямой сет может быть избыточен. Проверьте, не дублируете ли вы логику.
-
Вынос логики настройки dueDate
- У вас два идентичных кейса «today» и «tomorrow», которые только отличаются параметром даты. Можно сделать утилитарный метод, принимающий смещение в днях, например:
func applyDueDate(offsetDays: Int) { … }
Это снизит дублирование.
- У вас два идентичных кейса «today» и «tomorrow», которые только отличаются параметром даты. Можно сделать утилитарный метод, принимающий смещение в днях, например:
-
Безопасная работа с опционалом
- «Calendar.current.date(byAdding:…)» возвращает Optional. Пока вы передаёте его в «setDueDate», метод, скорее всего, принимает Optional и попросту ничего не сделает при nil, но лучше всё-таки явно обрабатывать «guard let date = … else { return }». Так вы заранее защититесь от странных невычисляемых дат.
-
Правильный return в dropDestination
- Ваша замыкалка всегда возвращает true, даже если что-то пошло не так. Правильно было бы возвращать булеан успеха: если запилились все операции без ошибок, вернуть true, иначе — false, чтобы система понимала, стоит ли считать drop удачным.
-
Инъекция modelContext
- Вместо того, чтобы передавать явно modelContext во все методы модели, можно заинжектить его через @Environment(.modelContext) прямо в модельный extension (с помощью static var context = Environment… или подобной обёртки), либо через ViewModel. Тогда ваши модели будут чище (без длинных сигнатур) и не придётся постоянно тянуть контекст аргументом.
-
Декомпозиция большого замыкания
- Код в dropDestination довольно объёмный и содержит сразу несколько задач: дисконнект, рестарт/завершение, установка даты, сохранение. Стоит вынести эти шаги в методы-врапперы, например:
handleDropped(tasks: [Todo], in: modelContext)
Это повысит читаемость и упростит тестирование.
- Код в dropDestination довольно объёмный и содержит сразу несколько задач: дисконнект, рестарт/завершение, установка даты, сохранение. Стоит вынести эти шаги в методы-врапперы, например:
-
Стиль именования
- Сейчас у вас одновременно есть метод «setDueDate(modelContext:dueDate:)» и старое прямое присвоение «task.dueDate =». Лучше определиться: либо вы всегда пользовались set-методом, либо вы даёте доступ к свойству напрямую. Консистентность поможет новым разработчикам быстрее сориентироваться.
-
Лишние break в switch
- В Swift в конце кейса «.inbox», «.projects» и т.д. не нужен «break» — он там implicit. Это небольшая стилистическая правка, но уменьшает визуальный шум.
В целом идея перехода от прямых присвоений к методам, принимающим modelContext, правильная: вы централизуете логику мутаций и сохранения. Только стоит ещё позаботиться о едином месте сохранения, обработке ошибок и выносить повторяющийся код в helper-функции.
} | ||
|
||
if status.clearDueDate { | ||
self.dueDate = nil | ||
} | ||
|
||
self.status = status | ||
try? context.save() | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ниже собраны основные замечания и предложения по улучшению вашего расширения для Todo
. Я постарался не комментировать то, что «просто работает», а сфокусировался на потенциальных проблемах и улучшениях.
-
Частые вызовы
try? context.save()
- Сейчас вы сохраняете контекст после практически каждой операции.
- Это неэффективно: постоянные обращения к диску и блокировки МК (ModelContext) могут серьёзно снизить производительность.
- Предлагаю «собирать» все связанные мутации и делать один единый
save()
в конце бизнес-операции (например, в сервисном слое или ViewModel), а не в каждом методе модели. - Если действительно нужен моментальный контроль ошибок — используйте
do { try context.save() } catch { … }
, а неtry?
, чтобы хотя бы логировать возникшие проблемы.
-
Подавление ошибок (
try?
)- Во всех случаях вы «глушите» ошибки, что затруднит отладку, если что-то пойдёт не так.
- Лучше либо пробросить ошибку вверх, либо хотя бы залогировать её через
print
/os_log
/Sentry, чтобы знать, что пошло не так.
-
Разрыв инверсии контроля:
- Методы экземпляра
Todo
теперь требуютModelContext
в качестве параметра. - Это наводит на мысль, что бизнес-логику проще держать во ViewModel/сервисном слое, а не в
NSManagedObject
-расширении. - Вы можете оставить в расширении чистые «мутационные» методы без сохранения, а сохранение делать централизованно:
todo.setDueDate(Date()) todo.reconnect() // … другие шаги … try context.save()
- Методы экземпляра
-
Дублирование кода
- В каждом методе вы пишете
try? context.save()
. - Если всё же решите сохранять «на месте», можно вынести в приватный хелпер:
private func save(_ context: ModelContext) { do { try context.save() } catch { /* логирование */ } }
- И вызывать
save(modelContext)
вместоtry? …
- В каждом методе вы пишете
-
Нейминг и ответственность методов
- Метод
reconnect
делает сразу две вещи: «приклеивает» задачу к проекту и к родительской задаче. Предлагаю разделить или переименовать:attachToProject(_:)
attachToParent(_:)
- Сейчас большую часть методов можно лучше описать:
complete
→completeAndScheduleNextIfRepeating
skip
→scheduleNextOccurrence
- Метод
-
Логика расчёта дат
- В
skip(modelContext:)
иnextWeek(modelContext:)
вы используетеCalendar.current
иfirstWeekday
. Это может работать не так, как ожидает пользователь в разных локалях. - Проверьте, что для вас первая неделя действительно Monday (или любая другая), и что смещения по дням работают корректно на границе месяцев/лет.
- В
-
Потенциальные утечки связей
- В
disconnectFromAll
вы обнуляетеproject
иstatus
, но не удаляете задачу из коллекций проекта (или справляетесь только частично). Убедитесь, что при переносе между проектами старая коллекция «чистится» правильно.
- В
-
Порядок операций в
changeStatus
- Вы сначала отсоединяете от родителя и меняете проект, потом при необходимости
complete(modelContext:)
илиreactivate(modelContext:)
, а потом устанавливаете новыйstatus
и ещё раз сохраняете. - Лучшая последовательность (и меньше лишних сохранений) — сгруппировать всё в одну транзакцию:
context.perform { if isCompleting { self.completeWithoutSave() } else { self.reactivateWithoutSave() } self.status = newStatus try context.save() }
- Вы сначала отсоединяете от родителя и меняете проект, потом при необходимости
-
Потокобезопасность
- Убедитесь, что все эти методы вызываются в том же потоке/качестве актёра (например, в
@MainActor
), где обрабатываетсяModelContext
. - Если вы сделаете
@MainActor extension Todo { … }
, то компилятор предупредит о неправильном доступе из фоновых потоков.
- Убедитесь, что все эти методы вызываются в том же потоке/качестве актёра (например, в
-
Тестируемость
- Разделение мутации и сохранения позволит вам проще подставлять «мок» контекста при юнит-тестах и проверять бизнес-логику без реального сохранения.
Итогово: вынесите сохранение из модели на уровень бизнес-логики, централизуйте обработку ошибок при save()
, оптимизируйте логику расчёта дат и уточните ответственность каждого метода (переименование + разбиение на меньшие по смыслу). Это улучшит производительность, читаемость и тестируемость кода.
@@ -286,7 +288,7 @@ struct PomPadDoMacApp: App { | |||
let newTask = task.copy(modelContext: sharedModelContainer.mainContext) | |||
|
|||
sharedModelContainer.mainContext.insert(newTask) | |||
newTask.reconnect() | |||
newTask.reconnect(modelContext: sharedModelContainer.mainContext) | |||
} | |||
|
|||
refresher.refresh.toggle() |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Вот несколько замечаний и рекомендаций по представленному коду (все нацелено на улучшение, без упоминания того, что уже «работает»):
-
Сохранение контекста
– Вы в одном месте делаетеtry? sharedModelContainer.mainContext.save()
, в другом — нет. Лучше вывести сохранение за пределы отдельных кнопок и вызывать его один раз, после того как вы изменили все задачи.
– Использованиеtry?
«глотает» ошибки. Предлагаю заменить на полноценныйdo { try … } catch { /* показать alert или лог */ }
— так вы не пропустите сбой сохранения. -
Множественные циклы и вызов
refresh.toggle()
– Сейчас у каждой кнопки вы пробегаете поselectedTasks.tasks
, меняете данные и сразу дергаетеrefresher.refresh.toggle()
. Можно вынести общий код в метод, например:func updateTasks(_ updater: (Task) -> Void) { for t in selectedTasks.tasks { updater(t) } saveContext() refresher.refresh.toggle() }
Тогда в каждой кнопке останется только вызов
updateTasks { $0.setDueDate(…) }
, а дублирования кода станет меньше. -
Работа со SwiftData и реактивностью
– Если вы используете SwiftData, то не обязательно вручную «тогглить» флагrefresher
— достаточно быть уверенным, что ваши сущности подхватываются через@Query
или@Observable
. Тогда любые изменения контекста подхватятся автоматически в UI.
– Также вместо «глобального»sharedModelContainer.mainContext
можно инжектить контекст через@Environment(\.modelContext)
— это более SwiftUI-style и упростит тестирование. -
Вспомогательные методы на модели
– Сейчас вы добавилиsetDueDate(modelContext:dueDate:)
иnextWeek(modelContext:)
, а из оригинального кода можно вообще убрать передачу контекста внутрь моделей. По идее, если модель — это@Observable
или@Model
SwiftData, ей достаточно просто присвоить свойствоdueDate = …
в том же потоке контекста, и затем сохранить контекст снаружи.
– Если же в этих методах вы выполняете какие-то дополнительные операции (например, перестраиваете связи), то стоит чётче разделить:- методы модели меняют только своё состояние,
- методы сервиса/контейнера — сохраняют и управляют связями.
-
Консистентность сохранений
– В блоке приоритетов вы вызываетеtry? save()
внутри цикла, а по факту достаточно выполнить одинsave()
после того, как все приоритеты назначены. Это будет более оптимально. -
Абстрагирование логики вычисления дат
– У вас повторяются вычисленияCalendar.current.startOfDay(for: Date())
и смещение на день.
– Можно вынести в расширение:extension Date { static var startOfToday: Date { Calendar.current.startOfDay(for: .now) } func addingDays(_ n: Int) -> Date? { Calendar.current.date(byAdding: .day, value: n, to: self) } }
Тогда кнопки упростятся:
updateTasks { $0.dueDate = .startOfToday.addingDays(1) }
-
UI/UX: выбор даты
– Вместо набора кнопок «ни дня», «сегодня», «завтра», «след. неделя» можно подумать об универсальномDatePicker
или календаре — так пользователь сам выберет любую дату, а вы не придумываете фиксированные варианты.
Итого:
– Сведите к минимуму дублирование (единственный метод обновления + единый save()
+ единое переключение refresh
).
– Перейдите на @Environment(\.modelContext)
и реактивные запросы (@Query
).
– Не «глотайте» ошибки try?
— обрабатывайте с UI-обратной связью.
– Вынесите логику работы с датами и обновления задач в переиспользуемые утилиты.
@@ -154,7 +154,7 @@ struct TasksQuery { | |||
} | |||
|
|||
static func deleteTask(context: ModelContext, task: Todo) { | |||
task.disconnectFromAll() | |||
task.disconnectFromAll(modelContext: context) | |||
task.deleteSubtasks(context: context) | |||
context.delete(task) | |||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ниже — несколько конкретных замечаний и предложений по улучшению вашей функции удаления:
-
Использование ручного «отсоединения» и «удаления»
Похоже, вы вручную «отсоединяете» задачу от всех связей и отдельно удаляете субзадачи. Если вы пользуетесь SwiftData и на свойство-связь (@Relationship
) повесили правило.cascade
, всё это произойдёт автоматически при вызовеcontext.delete(task)
. Проверьте, нельзя ли упростить модель и убрать ручныеdisconnectFromAll
/deleteSubtasks
, положившись на контейнер данных. -
Переименование параметра
modelContext
Сейчас у вас:task.disconnectFromAll(modelContext: context)
но в других методах вы называете параметр просто
context
. Ради единообразия лучше:func disconnectFromAll(context: ModelContext)
— или вообще сделать метод без внешнего имени:
func disconnectFromAll(_ context: ModelContext)
-
Объединить логику в расширение
Если подобный каскад удаления вам нужен в разных местах, вынесите его вModelContext
-extension. Пример:extension ModelContext { func deleteCascade(_ task: Todo) { task.disconnectFromAll(context: self) task.deleteSubtasks(in: self) delete(task) } }
Тогда в коде вызов будет предельно лаконичным:
context.deleteCascade(task)
-
Безопасность потоков и @mainactor
Работа сModelContext
, как правило, должна происходить на основном потоке. Если ваш метод может вызываться из фонового, оберните в@MainActor
:@MainActor static func deleteTask(...) { … }
-
Проверки на наличие субзадач
Можно добавить пред-условие, чтобы не гонять лишний запрос/исправление, если уtask.subtasks
уже пусто:if !task.subtasks.isEmpty { task.deleteSubtasks(context: context) }
-
Транзакции и откат
В случае каких-то сбоев полезно оборачивать серию изменений в транзакцию и давать возможность отмены. SwiftData напрямую этого не поддерживает, но можно держать «черновой» контекст или копию данных до удаления, если важно откатить.
Итого: основная рекомендация — максимально использовать декларативные возможности SwiftData (deleteRule, cascade) и не дублировать везде однотипные операции, а свести их к переиспользуемым extension или встроенным правилам модели.
No description provided.