Skip to content

Conversation

amikhaylin
Copy link
Owner

No description provided.

Copy link

@github-actions github-actions bot left a 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"
Copy link

Choose a reason for hiding this comment

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

Ниже — пара замечаний по вашему изменению, которые могут помочь сделать GitHub Action чуть более надёжным и удобным:

  1. Правильность названия модели
    Вы меняете 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».

  2. Язык взаимодействия лучше параметризовать более описательно
    Переменная LANGUAGE вполне рабочая, но неочевидна — легче было бы назвать её, например, REVIEW_LANGUAGE или LOCALE. Тогда сразу понятно, зачем она служит.

  3. Многострочный 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.

    Это не повлияет на работу, но облегчит поддержку.

  4. Проверка наличия обязательных переменных
    Если какой-то секрет не прокинулся, 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

    Это даст понятный лог.

  5. Унифицируйте версию действия и 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)
Copy link

Choose a reason for hiding this comment

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

Ниже — несколько замечаний и рекомендаций по вашему PR. Я не комментирую то, что уже работает, а концентрируюсь на потенциальных проблемах и возможностях улучшения.

  1. Дублирование кода
    Вы трижды подряд вставляете один и тот же кусок:

    .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)
  2. Мгновенное «замалчивание» ошибок (try?)
    Использование try? может заглушить реальные проблемы при сохранении. Вам стоит:
    – Ловить ошибку в do-catch.
    – Либо хотя бы залогировать её (os_log, print или собственный Logger), чтобы в случае падения Core Data/SwiftData вы понимали, что пошло не так.

  3. Логика сохранения в .refreshable
    .refreshable задумывался для «pull to refresh» (получения актуальных данных из сети, БД и т.д.), а не для сохранения изменений в контексте.
    – Если ваша цель — запушить локальные изменения в базу, лучше это делать в момент, когда пользователи действительно изменяют данные (в .onChange, при закрытии редактора, нажатии «Сохранить» и т.п.).
    – «Сохранение на рефреш» может быть неожиданным для пользователя и неочевидным для архитектуры.

  4. Хак с .id(refresher.refresh)
    Использование меняющегося id для того, чтобы «перезагрузить» SwiftUI-вью — тоже скорее workaround. Если вы используете @fetchrequest (или @query в SwiftData), SwiftUI будет автоматически обновлять UI при изменении данных. Поищите более декларативный способ:
    @fetchrequest для CoreData или @Query/Observable<…> для SwiftData.
    – При изменении @published поля в ViewModel SwiftUI перерисует интерфейс.

  5. Принудительная распаковка Optional
    selectedSideBarItem!.name потенциально может привести к краху, если item = nil. Лучше безопасно развернуть:

    if let item = selectedSideBarItem {
      DetailView(title: item.name)
        // …
    } else {
      Text("Нет выбора")
    }
  6. Распределение environmentObject
    Вы три раза прокидываете environmentObject(showInspector). Если дочерние вью все используют один и тот же объект, достаточно прокинуть его однажды в верхнем уровне ContentView.

  7. UX и асинхронность
    Если сохранение контекста может занимать время или быть асинхронным, подумайте о:
    – Показывать индикатор загрузки.
    – Делать await modelContext.save() в async-контексте (для SwiftData).

  8. Совместимость по версиям 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:
Copy link

Choose a reason for hiding this comment

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

Ниже – несколько замечаний и предложений по предлагаемым правкам. Основная мысль: текущее решение немного «замусоривает» View лишней бизнес-логикой и избыточно прокидывает ModelContext по методам, тогда как SwiftData уже умеет автоматически отслеживать и сохранять изменения.

  1. Избыточная передача modelContext в методы расширения

    • Почему это плохо?
      • В SwiftData экземпляр модели уже связан с контекстом (при вставке через modelContext.insert). Дополнительная прокрутка modelContext в каждый метод усложняет сигнатуру и связывает View с реализацией модели.
    • Что делать?
      • Сделать методы-расширения на стороне модели «чистыми» по API и свободными от контекста. Например:
      extension Todo {
        mutating func setDueDate(_ date: Date) {
          self.dueDate = date
        }
      }
      А уже внутри View (или, еще лучше, ViewModel) – единожды вызвать modelContext.insert(task) и затем чисто task.setDueDate(date).
  2. Разделение ответственности: вынос бизнес-логики из View

    • Сейчас в NewTaskView у вас склеены сразу:
      • расчёт даты (today/tomorrow/…)
      • вставка в контекст
      • «переподключение» через reconnect
      • добавление в локальный массив tasks
    • Рекомендация:
      • Завести ViewModel (например, NewTaskViewModel), инжектировать в неё ModelContext и уже внутри неё:
      – принимать параметры (имя, ссылка, секция карточки и т.п.),
      – считать нужную dueDate,
      – делать modelContext.insert(task),
      – вызывать task.reconnect() (без передачи контекста, если метод реализовать на модели).
      • View остается максимально декларативной: только биндинг полей и кнопка «Сохранить → viewModel.createTask()».
  3. Централизация расчёта даты

    • Сейчас вы в трёх местах повторяете 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))
  4. task.reconnect() vs task.reconnect(modelContext:)

    • Аналогично сеттеру даты, метод reconnect лучше позаботиться о контексте внутри модели (или вообще реализовать через @PostInsert-хук SwiftData, если нужно), а View не должен передавать контекст вручную.
  5. Локальный массив tasks и отражение изменений

    • Вы сами вставляете task в tasks.append(task). Но если у вас есть @Query на все задачи из SwiftData, то этот массив дублирует источник правды.
    • Либо полностью перейти на @Query, либо убрать дублирующие коллекции и работать только через контекст.
  6. Проверка на 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 {
Copy link

Choose a reason for hiding this comment

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

Ниже — несколько замечаний и рекомендаций по участку, где вы добавили сохранение контекста в реакцию на изменение scenePhase. Постарался не касаться того, что уже «работает», а именно сконцентрироваться на потенциальных проблемах и улучшениях.

  1. Не игнорируйте возможные ошибки при сохранении
    Сейчас вы используете

    try? modelContext.save()

    Такой подход «молча» заглатывает любые сбои и усложняет отладку. Лучше явно обрабатывать ошибки, хотя бы в лог:

    do {
        if modelContext.hasChanges {
            try modelContext.save()
        }
    } catch {
        // Здесь можно использовать ваш логгер или просто print
        print("Failed to save modelContext on .active: \(error)")
    }
  2. Проверка изменений перед сохранением
    Вызывая save() без проверки, каждый раз будете пытаться сохранить, даже если ничего не менялось. Это дополнительная нагрузка. Проверяйте:

    if modelContext.hasChanges {
        try modelContext.save()
    }
  3. Поток выполнения и изоляция данных
    SwiftData (и CoreData) ожидают, что вы будите работать с контекстом в том же потоке/акторе, где он создан. Если modelContext — это обычный MainActor-контейнер, то всё ок. Но если вы вдруг передали контекст в фон, то save может падать или блокировать UI. Убедитесь, что вызываете save в нужном контексте (главном акторе), либо явно оборачиваете в Task { @MainActor in … }.

  4. Совместимость .onChange(old,new)
    Подпись

    .onChange(of: scenePhase) { oldPhase, newPhase in  }

    доступна начиная с iOS 17 / macOS 14. Если таргетите более ранние версии платформ, это не скомпилируется. Либо:

    • Поменять на классический вариант .onChange(of: scenePhase) { newPhase in … } и вручную хранить старое состояние в @State.
    • Либо увеличить минимальную версию SDK.
  5. Разделение ответственности (чистота кода)
    Чем более «громоздким» становится ваш .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()
        }
    }
  6. Убедитесь, что у вас правильно прокинута модель
    Добавили вы @Environment(\.modelContext) в MainView, но не забудьте, чтобы в корне приложения или в нужном контейнере был установлен .modelContainer(...) (SwiftData). Иначе при запуске получите runtime-краш.

  7. Не забывайте про 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)
Copy link

Choose a reason for hiding this comment

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

Ниже — несколько замечаний и предложений, которые, на мой взгляд, могут сделать код более надёжным и читаемым. Если какие-то пункты для вас уже не актуальны — смело пропускайте.

  1. Порядок вставки и модификации объекта в 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-контекста объекта.

  2. Вынесение бизнес-логики из 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.

  3. Упрощение установки dueDate
    Если dueDate — всего лишь свойство сущности, не обязательно писать для этого отдельный метод с транзакцией. Можно:

    • либо вставить объект в контекст до изменения и просто присвоить task.dueDate = …
    • либо обернуть сразу несколько изменений в один modelContext.transaction { … }, если в будущем добавится больше полей.

    К примеру:

    modelContext.insert(task)
    modelContext.transaction {
      if dueToday {
        task.dueDate = .startOfToday
      }
      // в будущем можно добавить другие поля
    }
    

    Это чище, чем создавать метод расширения на каждое свойство.

  4. Утилита для «начала дня»
    Повторяющийся Calendar.current.startOfDay(for: Date()) можно вынести в static-свойство, чтобы назвать его понятнее:

    extension Date {
      static var startOfToday: Date {
        Calendar.current.startOfDay(for: Date())
      }
    }
    

    Тогда в коде будет выглядеть лаконичнее: Date.startOfToday.

  5. Обработка ошибок и валидация
    Сейчас создаётся Todo(name: taskName) без проверки на пустоту строки. Можно:

    • блокировать кнопку «Сохранить», пока строка пустая
    • показывать ошибку
    • или в save() проверять guard !name.isEmpty else { return }
  6. Тестируемость
    Если спокойно вынести работу со 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)
Copy link

Choose a reason for hiding this comment

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

Ниже — несколько замечаний и рекомендаций по фрагменту с .refreshable, где вы добавили try? modelContext.save():

  1. Сохранение в pull-to-refresh выглядит не по назначению
    Обычно .refreshable используют для «обновления» данных из внешнего источника (сети, базы, файлов и т. д.), а не для сохранения локальных изменений. В вашем случае непонятно, что вы «сохраняете» при каждом жесте pull-to-refresh и зачем это делать именно здесь. Если задача — периодически сбрасывать накопленные правки, перенесите save() в более семантичное место (кнопка «Сохранить», onDisappear, контроллер состояний и т. п.).

  2. Молчаливое подавление ошибок
    Вы используете try?, и если сохранение отвалится — потеряете информацию об ошибке. Рекомендую минимум логировать её или показывать пользователю:

    .refreshable {
      do {
        try modelContext.save()
      } catch {
        print("Ошибка сохранения контекста: \(error)")
        // можно показывать Alert
      }
      refresher.refresh.toggle()
    }
  3. Асинхронность и блокировка UI
    .refreshable — асинхронный замыкатель, и хотя modelContext.save() обычно работает быстро, при большом количестве изменений может заблокировать главный поток и подвесить анимацию «тянущейся» пробы. Если вы используете SwiftData (где save() синхронный), возможно, лучше выносить тяжёлые операции в фон:

    .refreshable {
      await Task.detached(priority: .userInitiated) {
        do {
          try modelContext.save()
        } catch {
          // логика обработки
        }
      }.value
      refresher.refresh.toggle()
    }
  4. Хак с .id(refresher.refresh)
    Привязка ID к булеву флагу — распространённый приём «перерисовать всю вьюшку», но это костыль и может приводить к потере состояния вложенных вью.

    • Лучше отделить логику обновления данных от самого представления: использовать @StateObject viewModel и менять @Published var items, чтобы SwiftUI сам понимал, какие строки поменялись.
    • Если вам действительно нужно форсить полный ре-рендер — документируйте в комментариях причины, иначе следующий разработчик удивится «откуда взялся» id-хак.
  5. MVVM и ответственностное разделение
    Сейчас вы смешиваете логику сохранения (ModelContext) прямо во ViewModifier’е. Рекомендую:

    • Завести отдельный ViewModel (или ObservableObject) с методом refresh(), который внутри делает save() и/или новый fetch.
    • Внутри .refreshable просто вызывать await viewModel.refresh().
  6. Тесты и предсказуемость
    После всех этих изменений протестируйте:

    • Что происходит на реальном устройстве/симуляторе при 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:
Copy link

Choose a reason for hiding this comment

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

Ниже — несколько наблюдений и предложений по улучшению, на что стоит обратить внимание:

  1. Группировка сохранений

    • Сейчас вы делаете «try? modelContext.save()» только в ветке реактивации и вообще вызываете save один раз на задачу. Если при дропе переносите несколько тасков в цикле, логичнее накопить все изменения и сохранить один раз после завершения цикла, а не внутри каждой итерации. Это уменьшит накладные расходы на транзакции и снизит риск частично сохранённых изменений при сбое.
  2. Обработка ошибок при сохранении

    • «try? modelContext.save()» глушит все исключения. Лучше хотя бы залогировать ошибку или показать пользователю, иначе вы никак не узнаете, что сохранение провалилось:
      • Перейти на «do { try modelContext.save() } catch { print("Save error:", error) }»
      • Или сделать пользовательский алерт.
  3. Последовательность вызова методов

    • Вы в одном месте вызываете «disconnectFromParentTask(modelContext:)» и тут же вручную обнуляете «task.parentTask = nil». Возможно, внутри вашего метода «disconnectFromParentTask» уже идёт сброс parentTask и save, поэтому этот прямой сет может быть избыточен. Проверьте, не дублируете ли вы логику.
  4. Вынос логики настройки dueDate

    • У вас два идентичных кейса «today» и «tomorrow», которые только отличаются параметром даты. Можно сделать утилитарный метод, принимающий смещение в днях, например:
      func applyDueDate(offsetDays: Int) { … }
      Это снизит дублирование.
  5. Безопасная работа с опционалом

    • «Calendar.current.date(byAdding:…)» возвращает Optional. Пока вы передаёте его в «setDueDate», метод, скорее всего, принимает Optional и попросту ничего не сделает при nil, но лучше всё-таки явно обрабатывать «guard let date = … else { return }». Так вы заранее защититесь от странных невычисляемых дат.
  6. Правильный return в dropDestination

    • Ваша замыкалка всегда возвращает true, даже если что-то пошло не так. Правильно было бы возвращать булеан успеха: если запилились все операции без ошибок, вернуть true, иначе — false, чтобы система понимала, стоит ли считать drop удачным.
  7. Инъекция modelContext

    • Вместо того, чтобы передавать явно modelContext во все методы модели, можно заинжектить его через @Environment(.modelContext) прямо в модельный extension (с помощью static var context = Environment… или подобной обёртки), либо через ViewModel. Тогда ваши модели будут чище (без длинных сигнатур) и не придётся постоянно тянуть контекст аргументом.
  8. Декомпозиция большого замыкания

    • Код в dropDestination довольно объёмный и содержит сразу несколько задач: дисконнект, рестарт/завершение, установка даты, сохранение. Стоит вынести эти шаги в методы-врапперы, например:
      handleDropped(tasks: [Todo], in: modelContext)
      Это повысит читаемость и упростит тестирование.
  9. Стиль именования

    • Сейчас у вас одновременно есть метод «setDueDate(modelContext:dueDate:)» и старое прямое присвоение «task.dueDate =». Лучше определиться: либо вы всегда пользовались set-методом, либо вы даёте доступ к свойству напрямую. Консистентность поможет новым разработчикам быстрее сориентироваться.
  10. Лишние break в switch

  • В Swift в конце кейса «.inbox», «.projects» и т.д. не нужен «break» — он там implicit. Это небольшая стилистическая правка, но уменьшает визуальный шум.

В целом идея перехода от прямых присвоений к методам, принимающим modelContext, правильная: вы централизуете логику мутаций и сохранения. Только стоит ещё позаботиться о едином месте сохранения, обработке ошибок и выносить повторяющийся код в helper-функции.

}

if status.clearDueDate {
self.dueDate = nil
}

self.status = status
try? context.save()
}
}
Copy link

Choose a reason for hiding this comment

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

Ниже собраны основные замечания и предложения по улучшению вашего расширения для Todo. Я постарался не комментировать то, что «просто работает», а сфокусировался на потенциальных проблемах и улучшениях.

  1. Частые вызовы try? context.save()

    • Сейчас вы сохраняете контекст после практически каждой операции.
    • Это неэффективно: постоянные обращения к диску и блокировки МК (ModelContext) могут серьёзно снизить производительность.
    • Предлагаю «собирать» все связанные мутации и делать один единый save() в конце бизнес-операции (например, в сервисном слое или ViewModel), а не в каждом методе модели.
    • Если действительно нужен моментальный контроль ошибок — используйте do { try context.save() } catch { … }, а не try?, чтобы хотя бы логировать возникшие проблемы.
  2. Подавление ошибок (try?)

    • Во всех случаях вы «глушите» ошибки, что затруднит отладку, если что-то пойдёт не так.
    • Лучше либо пробросить ошибку вверх, либо хотя бы залогировать её через print/os_log/Sentry, чтобы знать, что пошло не так.
  3. Разрыв инверсии контроля:

    • Методы экземпляра Todo теперь требуют ModelContext в качестве параметра.
    • Это наводит на мысль, что бизнес-логику проще держать во ViewModel/сервисном слое, а не в NSManagedObject-расширении.
    • Вы можете оставить в расширении чистые «мутационные» методы без сохранения, а сохранение делать централизованно:
      todo.setDueDate(Date())
      todo.reconnect()
      // … другие шаги …
      try context.save()
  4. Дублирование кода

    • В каждом методе вы пишете try? context.save().
    • Если всё же решите сохранять «на месте», можно вынести в приватный хелпер:
      private func save(_ context: ModelContext) {
        do { try context.save() }
        catch { /* логирование */ }
      }
    • И вызывать save(modelContext) вместо try? …
  5. Нейминг и ответственность методов

    • Метод reconnect делает сразу две вещи: «приклеивает» задачу к проекту и к родительской задаче. Предлагаю разделить или переименовать:
      • attachToProject(_:)
      • attachToParent(_:)
    • Сейчас большую часть методов можно лучше описать:
      • completecompleteAndScheduleNextIfRepeating
      • skipscheduleNextOccurrence
  6. Логика расчёта дат

    • В skip(modelContext:) и nextWeek(modelContext:) вы используете Calendar.current и firstWeekday. Это может работать не так, как ожидает пользователь в разных локалях.
    • Проверьте, что для вас первая неделя действительно Monday (или любая другая), и что смещения по дням работают корректно на границе месяцев/лет.
  7. Потенциальные утечки связей

    • В disconnectFromAll вы обнуляете project и status, но не удаляете задачу из коллекций проекта (или справляетесь только частично). Убедитесь, что при переносе между проектами старая коллекция «чистится» правильно.
  8. Порядок операций в changeStatus

    • Вы сначала отсоединяете от родителя и меняете проект, потом при необходимости complete(modelContext:) или reactivate(modelContext:), а потом устанавливаете новый status и ещё раз сохраняете.
    • Лучшая последовательность (и меньше лишних сохранений) — сгруппировать всё в одну транзакцию:
      context.perform {
        if isCompleting { self.completeWithoutSave() }
        else              { self.reactivateWithoutSave() }
        self.status = newStatus
        try context.save()
      }
  9. Потокобезопасность

    • Убедитесь, что все эти методы вызываются в том же потоке/качестве актёра (например, в @MainActor), где обрабатывается ModelContext.
    • Если вы сделаете @MainActor extension Todo { … }, то компилятор предупредит о неправильном доступе из фоновых потоков.
  10. Тестируемость

  • Разделение мутации и сохранения позволит вам проще подставлять «мок» контекста при юнит-тестах и проверять бизнес-логику без реального сохранения.

Итогово: вынесите сохранение из модели на уровень бизнес-логики, централизуйте обработку ошибок при 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()
Copy link

Choose a reason for hiding this comment

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

Вот несколько замечаний и рекомендаций по представленному коду (все нацелено на улучшение, без упоминания того, что уже «работает»):

  1. Сохранение контекста
    – Вы в одном месте делаете try? sharedModelContainer.mainContext.save(), в другом — нет. Лучше вывести сохранение за пределы отдельных кнопок и вызывать его один раз, после того как вы изменили все задачи.
    – Использование try? «глотает» ошибки. Предлагаю заменить на полноценный do { try … } catch { /* показать alert или лог */ } — так вы не пропустите сбой сохранения.

  2. Множественные циклы и вызов 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(…) }, а дублирования кода станет меньше.

  3. Работа со SwiftData и реактивностью
    – Если вы используете SwiftData, то не обязательно вручную «тогглить» флаг refresher — достаточно быть уверенным, что ваши сущности подхватываются через @Query или @Observable. Тогда любые изменения контекста подхватятся автоматически в UI.
    – Также вместо «глобального» sharedModelContainer.mainContext можно инжектить контекст через @Environment(\.modelContext) — это более SwiftUI-style и упростит тестирование.

  4. Вспомогательные методы на модели
    – Сейчас вы добавили setDueDate(modelContext:dueDate:) и nextWeek(modelContext:), а из оригинального кода можно вообще убрать передачу контекста внутрь моделей. По идее, если модель — это @Observable или @Model SwiftData, ей достаточно просто присвоить свойство dueDate = … в том же потоке контекста, и затем сохранить контекст снаружи.
    – Если же в этих методах вы выполняете какие-то дополнительные операции (например, перестраиваете связи), то стоит чётче разделить:

    • методы модели меняют только своё состояние,
    • методы сервиса/контейнера — сохраняют и управляют связями.
  5. Консистентность сохранений
    – В блоке приоритетов вы вызываете try? save() внутри цикла, а по факту достаточно выполнить один save() после того, как все приоритеты назначены. Это будет более оптимально.

  6. Абстрагирование логики вычисления дат
    – У вас повторяются вычисления 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) }
    
  7. 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)
}
Copy link

Choose a reason for hiding this comment

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

Ниже — несколько конкретных замечаний и предложений по улучшению вашей функции удаления:

  1. Использование ручного «отсоединения» и «удаления»
    Похоже, вы вручную «отсоединяете» задачу от всех связей и отдельно удаляете субзадачи. Если вы пользуетесь SwiftData и на свойство-связь (@Relationship) повесили правило .cascade, всё это произойдёт автоматически при вызове context.delete(task). Проверьте, нельзя ли упростить модель и убрать ручные disconnectFromAll/deleteSubtasks, положившись на контейнер данных.

  2. Переименование параметра modelContext
    Сейчас у вас:

    task.disconnectFromAll(modelContext: context)

    но в других методах вы называете параметр просто context. Ради единообразия лучше:

    func disconnectFromAll(context: ModelContext)

    — или вообще сделать метод без внешнего имени:

    func disconnectFromAll(_ context: ModelContext)
  3. Объединить логику в расширение
    Если подобный каскад удаления вам нужен в разных местах, вынесите его в ModelContext-extension. Пример:

    extension ModelContext {
      func deleteCascade(_ task: Todo) {
        task.disconnectFromAll(context: self)
        task.deleteSubtasks(in: self)
        delete(task)
      }
    }

    Тогда в коде вызов будет предельно лаконичным:

    context.deleteCascade(task)
  4. Безопасность потоков и @mainactor
    Работа с ModelContext, как правило, должна происходить на основном потоке. Если ваш метод может вызываться из фонового, оберните в @MainActor:

    @MainActor
    static func deleteTask(...) {  }
  5. Проверки на наличие субзадач
    Можно добавить пред-условие, чтобы не гонять лишний запрос/исправление, если у task.subtasks уже пусто:

    if !task.subtasks.isEmpty {
      task.deleteSubtasks(context: context)
    }
  6. Транзакции и откат
    В случае каких-то сбоев полезно оборачивать серию изменений в транзакцию и давать возможность отмены. SwiftData напрямую этого не поддерживает, но можно держать «черновой» контекст или копию данных до удаления, если важно откатить.

Итого: основная рекомендация — максимально использовать декларативные возможности SwiftData (deleteRule, cascade) и не дублировать везде однотипные операции, а свести их к переиспользуемым extension или встроенным правилам модели.

@amikhaylin amikhaylin merged commit 9e2700c into master Jul 5, 2025
1 check passed
@amikhaylin amikhaylin deleted the manual-sync branch July 5, 2025 16:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant