Conversation
#Conflicts: # PomPadDo.mobile/Activities/ContentView.swift # PomPadDo.watch Watch App/Activities/Settings/SettingsView.swift # PomPadDo.watch Watch App/SectionsListView.swift # PomPadDo.xcodeproj/project.pbxproj
Turn off lists view with Settings
#Conflicts: # PomPadDo.xcodeproj/project.pbxproj
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
|
|
||
| return maxHeight | ||
| } | ||
| } |
There was a problem hiding this comment.
Ниже — только по делу, что может стать проблемой и как улучшить.
-
Риск регрессии из‑за смены базовой высоты: базу снизили с 300 до 150. Если где‑то рассчитывали на прежний минимум, получите обрезку/скачки лейаута. Проверьте сценарии, когда все секции скрыты: раньше было минимум 300, теперь 150.
-
Магические числа и дублирование: 150 и 50 «зашиты» и одинаковое условие повторяется для каждой секции. Это хрупко при добавлении новых секций.
Предложение:- вынести константы,
- считать количество видимых секций,
- сделать вычисляемое свойство вместо функции (чистая зависимость от @AppStorage).
Пример:
private let baseHeight: CGFloat = 150
private let sectionHeight: CGFloat = 50private var maxSectionsHeight: CGFloat {
let visibleCount = [showDeadlinesSection, showAllSection, showReviewSection, showTomorrowSection]
.filter { $0 }
.count
return baseHeight + CGFloat(visibleCount) * sectionHeight
} -
Адаптивность/доступность: фиксированные высоты не учитывают Dynamic Type, размер экрана, платформу (iPhone/iPad/macOS). Если высота ограничивает контейнер с контентом, при крупном шрифте/локализации заголовков произойдёт обрезание.
Предложения:- если возможно, измерять фактическую высоту секций (GeometryReader/PreferenceKey) или отказаться от «макс. высоты» в пользу естественного лейаута/ScrollView,
- либо вычислять высоту на основе метрик (UIFontMetrics/Environment(.sizeCategory)) с коэффициентами.
-
Ключи @AppStorage: сейчас строковые литералы без неймспейса — легко схлопотать конфликт/опечатку, особенно в многоэкранном приложении.
Предложение — централизовать:
private enum StorageKey {
static let showDeadlines = "sections.showDeadlines"
static let showAll = "sections.showAll"
static let showReview = "sections.showReview"
static let showTomorrow = "sections.showTomorrow"
}
@AppStorage(StorageKey.showAll) var showAllSection = true
… -
Группировка настроек: четыре @AppStorage вью «шумят». Если таких станет больше — вынести в SettingsViewModel (ObservableObject) с синхронизацией в UserDefaults, или хотя бы сгруппировать под один неймспейс ключей. Плюс это упростит переиспользование и тестирование.
-
Уровень персистентности: @AppStorage хранит глобально для всего приложения/всех окон. Если поведение должно быть «на окно/сцену» (актуально на iPad/macOS), лучше @SceneStorage. Если всё‑таки глобально — ок.
-
Тестопригодность: из‑за смены базы с 300 → 150 добавьте снапшот‑тесты или хотя бы UI‑чек‑листы для кейсов «0/1/N секций включено» и разных size classes.
Итого: основная проблема — жёсткие и дублированные расчёты высоты с изменением базовой константы. Вынесите константы, посчитайте количество видимых секций, сделайте вычисляемое свойство и подумайте над адаптивной моделью высоты или измерением фактического контента. Ключи @AppStorage — зафиксировать и неймспейсить.
| } | ||
| } | ||
| case 1: | ||
| Form { |
There was a problem hiding this comment.
Ниже только существенные замечания и улучшения по коду.
-
Несогласованность имен:
- Ключ "showReviewBadge" и свойство showReviewProjectsBadge — неочевидная пара. Приведите к единообразию (или ключ, или свойство), чтобы избежать путаницы.
- В UI вы перешли на термин «list», а ключи остались в стиле "...Section" (например, showDeadlinesSection). Либо переименуйте ключи в ...List, либо осознанно сохраните старые имена и добавьте комментарий о backward compatibility.
-
Локализация:
- Все строки ("Lists", "Show … list") — в LocalizedStringKey или String(localized:), и вынести в локализуемые ресурсы. Желательно синхронизировать текст с реальными названиями списков в приложении, чтобы не дублировать и не рассинхронизировать.
-
Единообразие области видимости:
- Часть @AppStorage помечена private, часть — нет. Если эти значения не используются вне SettingsView, делайте их private для инкапсуляции.
-
Стиль переключателей:
- .toggleStyle(.switch) в Form на iOS избыточен — это и так дефолт. На macOS это ухудшает «нативность» (там чаще уместен чекбокс). Либо уберите, либо применяйте условно:
- #if os(iOS) .toggleStyle(.switch) #endif
- .toggleStyle(.switch) в Form на iOS избыточен — это и так дефолт. На macOS это ухудшает «нативность» (там чаще уместен чекбокс). Либо уберите, либо применяйте условно:
-
Централизация ключей:
- Разнесенные по коду строковые ключи — источник опечаток. Рекомендую вынести их в единое место:
- enum/struct UserDefaultsKeys { static let showAllSection = "showAllSection" … }
- Затем @AppStorage(UserDefaultsKeys.showAllSection) var showAllSection = true
- Разнесенные по коду строковые ключи — источник опечаток. Рекомендую вынести их в единое место:
-
Миграция ключей (если решите переименовать под «list»):
- Сохраните backward compatibility: при старте приложения считайте старое значение и при наличии — запишите в новый ключ, затем удалите старый. Иначе пользователи потеряют свои настройки.
-
UX-уголки:
- Подумайте, что произойдет, если пользователь выключит все списки. Нужен ли safeguard (минимум один список обязан быть включен), или авто-редирект, если активный экран скрывается?
- Добавьте footer к Section("Lists") с кратким пояснением влияния настроек.
-
Удаление повторов/масштабируемость:
- Если число списков растёт, текущий подход быстро зарастёт дублями. Можно инкапсулировать строку + ключ в переиспользуемый элемент:
- Пример:
- struct AppStorageToggleRow: View {
let title: LocalizedStringKey
@AppStorage var value: Bool
init(_ title: LocalizedStringKey, key: String, defaultValue: Bool) {
self.title = title
_value = AppStorage(wrappedValue: defaultValue, key)
}
var body: some View { Toggle(title, isOn: $value) }
} - И использовать в секции без объявления отдельных свойств на каждый ключ.
- struct AppStorageToggleRow: View {
- Либо хотя бы массив метаданных и несколько AppStorage-свойств рядом — чтобы упростить добавление новых пунктов.
- Пример:
- Если число списков растёт, текущий подход быстро зарастёт дублями. Можно инкапсулировать строку + ключ в переиспользуемый элемент:
-
Тестируемость/идентификаторы:
- Добавьте .accessibilityIdentifier к переключателям — облегчит UI-тесты и проверку конкретных ключей.
Итого: приведи к единообразию имена (ключи/переменные/тексты), централизуй ключи, убери навязанный .toggleStyle(.switch) на платформах где это неуместно, продумай миграцию при переименовании и UX-кейсы отключения всех списков. Для масштабируемости — вынеси повторяющийся шаблон переключателя в отдельный View с @AppStorage внутри.
| .toggleStyle(.switch) | ||
| } | ||
|
|
||
| Section("Focus Timer") { |
There was a problem hiding this comment.
Ниже только предметные замечания и улучшения.
-
Единообразие терминов. Сейчас переменные — showDeadlinesSection / showAllSection / showTomorrowSection, а подписи — “Show Deadlines section” vs “Show All list/Tomorrow list”. Выберите один термин (section или list) и используйте его везде (и в именах, и в UI-тексте). Например: showAllSection + “Show ‘All’ section”.
-
Локализация. Жёстко заданные строки в Toggle/Section затрудняют перевод. Используйте LocalizedStringKey и строки из .strings:
- Section("settings.general")
- Toggle("settings.show_deadlines_section", …)
- Toggle("settings.show_all_section", …)
- Toggle("settings.show_tomorrow_section", …)
-
Неймспейс и константы для ключей @AppStorage. Строковые ключи лучше вынести в константы и, по возможности, неймспейсить (например, "settings.showAllSection"), чтобы избежать опечаток и коллизий:
struct SettingsKeys {
static let showDeadlinesSection = "settings.showDeadlinesSection"
static let showAllSection = "settings.showAllSection"
static let showTomorrowSection = "settings.showTomorrowSection"
} -
Объявление @AppStorage. Для булевых удобно использовать wrappedValue, снижает шум типов и подчёркивает дефолт:
@AppStorage(SettingsKeys.showDeadlinesSection) var showDeadlinesSection: Bool = true
@AppStorage(SettingsKeys.showAllSection) var showAllSection: Bool = true
@AppStorage(SettingsKeys.showTomorrowSection) var showTomorrowSection: Bool = true -
Последовательность UI. Раз вы показываете переключатели видимости списков, возможно, секцию стоит назвать точнее (например, “Lists”/“Sections visibility”), чтобы лучше отражать смысл.
-
Избыточность toggleStyle(.switch). На iOS это стиль по умолчанию; можно убрать для лаконичности. Оставьте только если явно нужно на всех платформах.
-
UX-страховка. Если пользователь выключит все секции, не останется контента. Подумайте о валидации (нельзя отключить всё) или минимум об информировании/автокоррекции.
-
Автотесты/доступность. Добавьте accessibilityIdentifier к переключателям для UI-тестов и стабильной привязки:
.accessibilityIdentifier("settings.toggle.showAllSection") -
Регистрация дефолтов при старте. Если значения читаются где-то вне SwiftUI, зарегистрируйте дефолты однажды в AppDelegate/SceneDelegate:
UserDefaults.standard.register(defaults: [
SettingsKeys.showDeadlinesSection: true,
SettingsKeys.showAllSection: true,
SettingsKeys.showTomorrowSection: true
])
Пример корректировки фрагмента:
struct SettingsKeys {
static let showDeadlinesSection = "settings.showDeadlinesSection"
static let showAllSection = "settings.showAllSection"
static let showTomorrowSection = "settings.showTomorrowSection"
}
struct SettingsView: View {
@AppStorage(SettingsKeys.showDeadlinesSection) var showDeadlinesSection: Bool = true
@AppStorage(SettingsKeys.showAllSection) var showAllSection: Bool = true
@AppStorage(SettingsKeys.showTomorrowSection) var showTomorrowSection: Bool = true
var body: some View {
Form {
Section("settings.general") {
Toggle("settings.show_deadlines_section", isOn: $showDeadlinesSection)
.accessibilityIdentifier("settings.toggle.showDeadlinesSection")
Toggle("settings.show_all_section", isOn: $showAllSection)
.accessibilityIdentifier("settings.toggle.showAllSection")
Toggle("settings.show_tomorrow_section", isOn: $showTomorrowSection)
.accessibilityIdentifier("settings.toggle.showTomorrowSection")
}
// ...
}
}
}
Эти правки улучшают консистентность, локализацию, тестируемость и устойчивость к опечаткам в ключах.
| EmptyView() | ||
| } | ||
| case .focus: | ||
| NavigationLink(value: item) { |
There was a problem hiding this comment.
Ниже только предметные замечания по улучшению и предотвращению проблем.
-
Не возвращайте EmptyView в else. В ViewBuilder if без else уже эквивалентен “ничего не рендерить”. Лишний шум:
// было
if showTomorrowSection { ... } else { EmptyView() }
// лучше
if showTomorrowSection { ... } -
Лучше фильтровать источник данных, а не возвращать пустые вью внутри списка. Это уменьшит “мёртвые” ноды в дереве, снимет риски с selection и упростит логику.
Пример:
private var visibleItems: [SideBarItem] {
SideBarItem.allCases.filter {
switch $0 {
case .tomorrow: return showTomorrowSection
case .alltasks: return showAllSection
default: return true
}
}
}
var body: some View {
List(visibleItems, selection: $selectedSideBarItem) { item in
row(for: item)
}
} -
Обработайте selectedSideBarItem при скрытии секции. Сейчас можно попасть в состояние, когда selection указывает на скрытый элемент (особенно с NavigationSplitView / NavigationStack value:). При выключении флага — сбросьте selection на другой валидный элемент.
Пример:
@State private var fallbackItem: SideBarItem = .today
.onChange(of: showTomorrowSection) { newValue in
if !newValue, selectedSideBarItem == .tomorrow { selectedSideBarItem = fallbackItem }
}
.onChange(of: showAllSection) { newValue in
if !newValue, selectedSideBarItem == .alltasks { selectedSideBarItem = fallbackItem }
} -
Избавьтесь от дублирования разметки для строк списка. Вынесите построение ряда в отдельный метод/вью или свойства enum.
Пример:
private func row(for item: SideBarItem) -> some View {
NavigationLink(value: item) {
HStack {
Image(systemName: item.iconName)
Text(item.titleKey) // LocalizedStringKey
}
}
.listItemTint(item.tintColor) // см. следующий пункт
}
// В SideBarItem:
var iconName: String { ... }
var titleKey: LocalizedStringKey { ... } // для локализации
var tintColor: Color { ... } -
Вместо .foregroundStyle на весь HStack в списке рассмотрите .listItemTint (iOS 17+) или .tint для consistent-семантики, чтобы не конфликтовать с цветами выделения и состояниями (disabled/selected). Это обычно лучше “садится” на sidebar/лист.
Пример:
NavigationLink(value: item) { ... }
.listItemTint(Color(...)) // iOS 17+
// или .tint(Color(...)) на соответствующей платформе -
Цвета: не используйте Color(#colorLiteral(...)) в продакшене. Перенесите в Asset Catalog с light/dark вариантами или заведите семантические константы. Это улучшит доступность и поддержку тёмной темы.
Пример:
enum Palette {
static let tomorrow = Color("TomorrowTint")
static let all = Color("AllTint")
} -
Именование и ключи @AppStorage. Приведите к единому стилю и вынесите ключи в константы, чтобы исключить опечатки и упростить рефакторинг.
Пример:
enum SettingsKeys {
static let showAllSection = "showAllSection"
static let showTomorrowSection = "showTomorrowSection"
}
@AppStorage(SettingsKeys.showAllSection) var showAllSection = true -
Локализация строк. "Tomorrow" и "All" лучше хранить как LocalizedStringKey (или в Localizable.strings), а не как raw-строки в коде.
-
Доступность и UI-тесты. Если на Today есть accessibilityIdentifier, добавьте для Tomorrow/All для согласованности:
.accessibilityIdentifier("TomorrowNavButton")
.accessibilityIdentifier("AllNavButton") -
Консистентность нейминга. У вас case .alltasks и текст "All". Приведите enum к allTasks (camelCase) и/или унифицируйте отображаемое имя с кейсом, чтобы позже не путаться.
Итог: основной технический долг здесь — условный рендеринг внутри полного перечисления кейсов. Перенесите условность в фильтрацию данных и обработайте selection при скрытии, плюс вычистите дублирование разметки и используйте семантические цвета/локализацию. Это сделает код короче, устойчивее и предсказуемее.
| CURRENT_PROJECT_VERSION = 102; | ||
| DEVELOPMENT_TEAM = 9Z68336878; | ||
| GENERATE_INFOPLIST_FILE = YES; | ||
| MACOSX_DEPLOYMENT_TARGET = 26.0; |
There was a problem hiding this comment.
Ниже только по делу — что может сломать сборку/релиз или что стоит улучшить.
Критично
- MACOSX_DEPLOYMENT_TARGET = 26.0 — это некорректно для macOS (должно быть что‑то вроде 14.0/15.0). Если хотели выставить целевую для watchOS/visionOS — используйте соответствующие ключи (WATCHOS_DEPLOYMENT_TARGET / VISIONOS_DEPLOYMENT_TARGET), а не MACOSX_*. Исправьте, иначе получите ошибки линковки/архивации.
- IPHONEOS_DEPLOYMENT_TARGET = 18.1: убедитесь, что это действительно нужно. Это отсекает 18.0. Если нет фич 18.1+, лучше 18.0, чтобы не сужать аудиторию.
Релизная согласованность версий
- CURRENT_PROJECT_VERSION поднят до 102 почти везде — это хорошо. Проверьте, что НИ один таргет, включающийся в релизный бандл (host app, app extensions, watch app/extension, widgets), не остался на 100/101. Несовпадение CFBundleVersion между host и embedded — частая причина отказа в App Store Connect.
- MARKETING_VERSION указан не у всех таргетов. Для App Store рекомендуется, чтобы CFBundleShortVersionString совпадал у host и всех embedded таргетов. Приведите к единому значению везде, где таргет попадает в .ipa/.pkg.
Конфигурация Info.plist
- У многих таргетов одновременно стоят GENERATE_INFOPLIST_FILE = YES и INFOPLIST_FILE = ... Это двусмысленно. Либо генерируйте plist (уберите INFOPLIST_FILE и лишние файлы из репо), либо используйте ручной plist (поставьте GENERATE_INFOPLIST_FILE = NO). Сейчас есть риск рассинхронизации ключей.
Поддержка и сопровождение
- Централизуйте версии: вынесите CURRENT_PROJECT_VERSION и MARKETING_VERSION в общий .xcconfig или задайте на уровне проекта и уберите переопределения в таргетах. Это избавит от массовых правок при каждом бампе.
- Автоинкремент: добавьте шаг в CI/fastlane (agvtool new-version -all 102 или fastlane increment_build_number) и скрипт‑проверку, что у всех embedded бандлов версии совпадают.
- Тестовые таргеты: поднимать им CFBundleVersion не обязательно. Лучше пусть наследуют значение с уровня проекта, чтобы не плодить шум в диффах.
Быстрые проверки
- Поиск оставшихся старых значений: grep -R "CURRENT_PROJECT_VERSION = 10[01]" -n .
- Проверка отсутствующих MARKETING_VERSION в релевантных таргетах (app + все extensions + watch).
| } | ||
|
|
||
| return maxHeight | ||
| } |
There was a problem hiding this comment.
Ниже только предметные замечания по изменённому коду и связанные с ним улучшения.
-
Опечатка и стиль именования: getMaxSectionsHeigth → getMaxSectionsHeight или, лучше по Swift-гайдам, computed property без префикса get, например maxSectionsHeight.
-
Магические числа: 84.0 и 28.0 надо вынести в именованные константы (baseHeight, rowHeight) с пояснящими комментариями. Сейчас неочевидно, почему базовая высота 84 даже когда все секции скрыты.
-
Логика подсчёта:
- Если базовая высота — это некая постоянная часть интерфейса (например, отступы/заголовок), назовите её соответствующе и отделите от высоты секций. Если нет — базу лучше сделать 0 и считать только количество видимых секций.
- Уберите повторяющиеся if’ы. Подсчитайте число включённых секций и умножьте на rowHeight:
- visibleSectionCount = [showDeadlinesSection, showAllSection, showReviewSection, showTomorrowSection].filter { $0 }.count
- return baseHeight + CGFloat(visibleSectionCount) * rowHeight
-
Адаптивность/доступность: жёсткий шаг 28.0 ломает адаптацию под Dynamic Type/аксессибилити. Если это высота строки, её стоит вычислять на основе метрик шрифта (Font/UIFontMetrics) или отказаться от ручного подсчёта в пользу авто-лейаута SwiftUI. Фиксированные пиксели — источник будущих багов на iPad, в SplitView и при разных размерах текста.
-
@AppStorage ключи:
- Строковые ключи вынесите в неймспейс (static let внутри ContentView или отдельного типа), чтобы исключить опечатки и коллизии: "ContentView.showAllSection" и т.п.
- Явно задайте wrappedValue: @AppStorage(wrappedValue: true, ...) — читабельнее и не даёт сомнений о дефолте.
- Учтите миграцию: изменение дефолта не повлияет на пользователей, у кого значение уже сохранено. Если нужно принудительно включить новые секции для существующих пользователей — добавьте миграционную логику.
-
Расширяемость: при добавлении новых секций текущий подход требует менять код в нескольких местах. Рассмотрите перечисление Section с isVisible, где видимость хранится в @AppStorage с составным ключом, и сведение подсчёта высоты к итерации по коллекции секций.
-
Нейминг: showAllSection/… — нормально, но лучше унифицировать булевы имена как isAllSectionVisible, isReviewSectionVisible, isTomorrowSectionVisible, isDeadlinesSectionVisible.
Итоговая идея в коде (схематично):
- частные константы rowHeight/baseHeight;
- коллекция булевых флагов;
- maxSectionsHeight как computed property, считающий base + count(true) * rowHeight;
- исправленное имя и ключи @AppStorage, вынесенные в статические константы;
- по возможности — отказ от жёстких высот в пользу авто-лейаута или вычисления на основе метрик шрифта.
| } | ||
| } | ||
| .tabItem { | ||
| Label("General", systemImage: "gear") |
There was a problem hiding this comment.
Вот конкретные и полезные замечания по диффу:
-
Единообразие области видимости. Сейчас showReviewProjectsBadge — private, остальные @AppStorage — нет. Если эти значения не используются вне SettingsView, сделайте их тоже private. Если используются в разных местах — лучше вынести в единый Settings (ObservableObject) и инъектить через EnvironmentObject/Environment.
-
Именование: «Section» vs «List». В интерфейсе вы показываете «Show … list», а переменные называются show…Section. Это может путать. Минимально — переименуйте свойства для согласованности, оставив существующие ключи UserDefaults, чтобы не потерять данные:
@AppStorage("showDeadlinesSection") private var showDeadlinesList = true -
Стиль чекбоксов: уберите дублирование .toggleStyle(.checkbox). Достаточно задать стиль на контейнере, он пробрасывается вниз по иерархии:
Form {
Section("Lists") {
Toggle("Show Deadlines list", isOn: $showDeadlinesList)
Toggle("Show All list", isOn: $showAllList)
Toggle("Show Review list", isOn: $showReviewList)
Toggle("Show Tomorrow list", isOn: $showTomorrowList)
}
}
.toggleStyle(.checkbox) -
Ключи @AppStorage. Сейчас голые строковые ключи легко опечатать и сложно переиспользовать. Вынесите их в единое место с неймспейсом, чтобы избежать коллизий и упростить миграции:
enum SettingsKeys {
enum lists {
static let deadlines = "settings.lists.deadlines"
static let all = "settings.lists.all"
static let review = "settings.lists.review"
static let tomorrow = "settings.lists.tomorrow"
}
static let reviewBadge = "settings.badge.review"
}
@AppStorage(SettingsKeys.lists.deadlines) private var showDeadlinesList = true -
Кроссплатформенность стиля .checkbox. Если проект не чисто macOS, проверьте доступность. На iOS старше определенной версии чекбокс может быть недоступен. Защититесь #if os(macOS) или проверкой версии, иначе упадете на сборке/рантайме:
#if os(macOS)
.toggleStyle(.checkbox)
#endif -
Локализация. Тексты сейчас ок (SwiftUI использует LocalizedStringKey), но лучше перейти на ключи, чтобы можно было менять копирайт без миграции ключей:
Toggle(String(localized: "settings.lists.deadlines"), isOn: $showDeadlinesList)
И не забудьте локализовать заголовок вкладки "General". -
Консистентность нейминга. showReviewProjectsBadge (переменная) и "showReviewBadge" (ключ) расходятся по смыслу. Приведите к одному термину, например reviewBadge / showReviewBadge, чтобы не плодить неоднозначности.
-
Тестируемость и реактивность. Если другие экраны скрывают/показывают списки по этим флагам, убедитесь, что они наблюдают изменения (через @AppStorage в тех вью или через общий Settings-объект). Иначе потребуется перезапуск/реинициализация. Для UI-тестов добавьте .accessibilityIdentifier к тумблерам.
-
Дефолты. Вы задаете дефолт true через = true — это ок. Старайтесь придерживаться одного стиля во всех @AppStorage (либо через = true, либо через wrappedValue:), чтобы не смешивать подходы.
Опционально (для масштабирования):
- Свести дублирование через небольшой вспомогательный вью-компонент, если тумблеров станет больше (и в нем задать идентификаторы, локализацию, стиль). Это упростит сопровождение и тесты.
| .listRowSeparator(.hidden) | ||
| } | ||
| } | ||
| .listStyle(SidebarListStyle()) |
There was a problem hiding this comment.
Ниже — только конструктивные замечания и улучшения по вашему диффу.
-
Фильтрация элементов вместо EmptyView
Сейчас вы строите List на основе SideBarItem.allCases, но для скрываемых секций возвращаете EmptyView. Это:- создаёт «пустые» строки и артефакты поведения;
- ломает логику selection: в selection может остаться пункт, которого больше нет визуально.
Рекомендуется заранее отфильтровать массив элементов, а не возвращать EmptyView.
Пример: - вычисляем видимые элементы:
var visibleItems: [SideBarItem] {
SideBarItem.allCases.filter { isVisible($0) }
}
private func isVisible(_ item: SideBarItem) -> Bool {
switch item {
case .tomorrow: return showTomorrowSection
case .review: return showReviewSection
case .deadlines: return showDeadlinesSection
case .alltasks: return showAllSection
case .projects: return false // или логика видимости
}
} - и используем их в List:
List(visibleItems, selection: $selectedSideBarItem) { item in
// switch как раньше, но без else { EmptyView() }
} - Почистите selection при изменении флагов видимости, если текущий выбранный элемент стал невидим:
private func sanitizeSelection() {
if let sel = selectedSideBarItem, !visibleItems.contains(sel) {
selectedSideBarItem = visibleItems.first
}
}
.onChange(of: showTomorrowSection) { _ in sanitizeSelection() }
.onChange(of: showReviewSection) { _ in sanitizeSelection() }
.onChange(of: showDeadlinesSection) { _ in sanitizeSelection() }
.onChange(of: showAllSection) { _ in sanitizeSelection() }
-
Уберите лишние else { EmptyView() }
В ViewBuilder можно просто if show { ... } без else — это читается лучше и не плодит пустые вью. -
.listRowSeparator(.hidden)
Вы дублируете модификатор для каждой строки. Если хотите отключить разделители везде — повесьте один раз на List:
List(visibleItems, selection: ...) { ... }
.listRowSeparator(.hidden) -
Повторяющийся UI-код
Строки секций одинаковы по структуре (иконка, текст, цвет, .badge, опционально dropDestination). Вынесите фабрику ряда/Label:- Используйте Label вместо ручного HStack:
NavigationLink(value: item) {
Label("Tomorrow", systemImage: "sunrise")
} - Cконструируйте вспомогательную функцию/вью для единообразия и чтобы не дублировать .foregroundStyle/.badge/дроп.
- Используйте Label вместо ручного HStack:
-
Цвета через Color literal
Color(#colorLiteral(...)) не адаптивны к темной теме/контрасту. Лучше:- вынести цвета в Asset Catalog и использовать Color("TomorrowColor");
- либо использовать .tint(color) на NavigationLink, чтобы получить семантически корректную окраску и лучшее поведение при disabled/selection, вместо .foregroundStyle.
-
Производительность бэйджей
projects.filter({ TasksQuery.filterProjectToReview($0) }).count и tasksAllActive.count/ tasksTomorrowActive.count считаются при каждом redraw. Если это «тяжёлые» вычисления/фетчи — кешируйте/мемоизируйте или переведите в наблюдаемые источники данных (например, @fetchrequest / @query / @observableobject) и деривативные свойства. -
DropDestination: не вычисляйте дату в цикле
- Вынесите target дату вне for:
.dropDestination(for: Todo.self) { tasks, _ in
let cal = Calendar.current
let startOfToday = cal.startOfDay(for: Date())
let target = cal.date(byAdding: .day, value: 1, to: startOfToday)
tasks.forEach { $0.setDueDate(dueDate: target) }
return true
} - Проверьте, нужен ли здесь @MainActor/контекст Core Data при записи (если Todo — NSManagedObject), чтобы не писать в контекст из фонового потока.
- Вынесите target дату вне for:
-
Именование и ключи @AppStorage
- showAllSection звучит расплывчато; точнее — showAllTasksSection.
- Ключи @AppStorage лучше вынести в статические константы/enum для единообразия и исключения опечаток. При необходимости — общий suiteName.
-
Локализация
Строки "Tomorrow", "Review", "All" — лучше сделать LocalizedStringKey или использовать String(localized:). -
Совместимость/платформы
- .badge доступен не везде (например, macOS < 13). Если таргетируете несколько платформ/версий — оберните в #available.
- .listStyle(SidebarListStyle()) можно заменить на .listStyle(.sidebar) на новых платформах, ради единообразия.
-
Поведение при скрытии «Projects»
В кейсе .projects возвращается EmptyView — если элемент присутствует в allCases, но ряд пуст, будет нелогично. Либо убирайте его фильтрацией из списка (см. пункт про visibleItems), либо отображайте валидную строку.
Итого: главное — не рендерить пустые строки, корректно вести selection при скрытии секции, вынести повторяющиеся части и сделать цвета/тексты семантически корректными и адаптивными. Это улучшит стабильность, поддержку и внешний вид.
| #else | ||
| .swipeActions(edge: .leading, allowsFullSwipe: false) { | ||
| if let subtasks = task.subtasks { | ||
| NavigationLink { |
There was a problem hiding this comment.
Ниже — только те замечания, которые реально могут улучшить поведение/стабильность и читаемость.
-
Две .swipeActions на одном и том же edge (.trailing)
- Сейчас у вас два вызова .swipeActions(edge: .trailing, ...): первый с allowsFullSwipe: true, второй — false. SwiftUI объединяет экшены, но флаг берётся по последнему модификатору, из‑за чего «полный свайп» фактически выключен. Объедините trailing‑кнопки в один блок и явно зафиксируйте порядок (чтобы full swipe срабатывал на «Complete»):
.swipeActions(edge: .trailing, allowsFullSwipe: true) {
completeButton
deleteButton
} - Это также упростит поддержку и сделает поведение предсказуемым.
- Сейчас у вас два вызова .swipeActions(edge: .trailing, ...): первый с allowsFullSwipe: true, второй — false. SwiftUI объединяет экшены, но флаг берётся по последнему модификатору, из‑за чего «полный свайп» фактически выключен. Объедините trailing‑кнопки в один блок и явно зафиксируйте порядок (чтобы full swipe срабатывал на «Complete»):
-
@EnvironmentObject без платформенной отсечки
- Вы сняли #if os(iOS) с showInspector и selectedTasks, но ниже для leading‑действий оставили #if !os(watchOS). На watchOS (и потенциально на macOS) эти EnvironmentObject могут вообще не быть инжектированы и приложение упадёт при построении body. Верните условную компиляцию для самих свойств или инкапсулируйте функциональность так, чтобы эти зависимости появлялись только на нужных платформах, например:
#if !os(watchOS)
@EnvironmentObject var showInspector: InspectorToggler
@EnvironmentObject var selectedTasks: SelectedTasks
#endif - Альтернатива: не тянуть их через EnvironmentObject в модификатор, а передавать в инициализатор модификатора явно на платформах, где они нужны.
- Вы сняли #if os(iOS) с showInspector и selectedTasks, но ниже для leading‑действий оставили #if !os(watchOS). На watchOS (и потенциально на macOS) эти EnvironmentObject могут вообще не быть инжектированы и приложение упадёт при построении body. Верните условную компиляцию для самих свойств или инкапсулируйте функциональность так, чтобы эти зависимости появлялись только на нужных платформах, например:
-
Порядок и логика работы таймера при завершении задачи
- Сейчас вы делаете timer.reset() до проверки режимов и проверяете только .pause и .longbreak. Если цель — «если сейчас перерыв/пауза, пропустить», то reset может уже изменить mode и условие не сработает. Снимайте состояние ДО reset и включите .shortbreak, если он есть:
let wasBreakOrPause = timer.mode == .pause || timer.mode == .longbreak || timer.mode == .shortbreak
timer.reset()
if wasBreakOrPause { timer.skip() } - Подумайте, не нужно ли вызывать skip до reset (в зависимости от того, что именно делают ваши методы).
- Сейчас вы делаете timer.reset() до проверки режимов и проверяете только .pause и .longbreak. Если цель — «если сейчас перерыв/пауза, пропустить», то reset может уже изменить mode и условие не сработает. Снимайте состояние ДО reset и включите .shortbreak, если он есть:
-
NavigationLink в swipeActions
- Использование NavigationLink внутри .swipeActions работает нестабильно на разных платформах/версиях. Рекомендация — использовать Button, который триггерит изменение состояния навигации (selection/path), и уже это состояние ведёт к переходу. Это уменьшит вероятность «странных» переходов при свайпе.
-
Нейминг/локализация и иконки для Complete/Uncomplete
- "Uncomplete" — неудачный текст; лучше «Mark Incomplete»/«Reopen» (и локализовать). Иконки "square" vs "checkmark.square.fill" выглядят разностилево; подберите согласованную пару (например, "checkmark.square" / "checkmark.square.fill") или настройте tint.
-
Согласованность сайд‑эффектов complete/reactivate
- task.complete(modelContext:) принимает контекст, а task.reactivate() — нет. Это может запутывать и приводить к несогласованным апдейтам/сохранениям. Приведите к единому стилю (либо оба через контекст, либо оба без; сохранение — в одном месте).
-
Сравнение task == focus
- Если Todo — SwiftData @model, равенство по ссылке/идентификатору может вести себя неожиданно при разных контекстах. Более надёжно сравнивать по persistentIdentifier или id, если у вас есть стабильный идентификатор модели.
-
Атомарность анимаций и побочных эффектов
- Сейчас withAnimation применён только к удалению. Если completion/uncompletion влияет на видимый список, оберните и их в анимацию для консистентности UX:
withAnimation {
// toggle complete
}
- Сейчас withAnimation применён только к удалению. Если completion/uncompletion влияет на видимый список, оберните и их в анимацию для консистентности UX:
-
Дублирование кода для watchOS/не watchOS в leading‑actions
- Логика в блоках очень похожа. Имеет смысл вынести общие элементы в приватные вью‑билдеры и различать только то, что реально отличается (например, способ навигации).
-
Потенциальный мёртвый инжект
- В этом модификаторе видно @EnvironmentObject var selectedTasks, но в показанном фрагменте он не используется. Если фактически не нужен — лучше убрать зависимость (особенно с учётом кроссплатформенности).
-
Устойчивость к отсутствию focusTask
- В нескольких местах однотипная проверка if let focus = focusTask.task, task == focus { ... }. Вынесите в помощник, например isFocused(task), чтобы не дублировать и не ошибиться в одном из мест.
Итоговые приоритеты исправлений:
- Объединить trailing .swipeActions и восстановить ожидаемое allowsFullSwipe.
- Вернуть платформенные #if для EnvironmentObject (или убрать эти зависимости из модификатора).
- Исправить порядок/условия работы таймера при завершении задачи (.shortbreak и проверка до reset).
- Заменить NavigationLink в свайпах на Button с управлением состоянием навигации.
- Причесать нейминг/локализацию и симметрию API complete/reactivate.
|
|
||
| }, | ||
| "View Mode" : { | ||
| "localizations" : { |
There was a problem hiding this comment.
Ниже — только по делу, по изменениям в диффе.
Критичные проблемы
-
Нарушение структуры данных. Новые узлы ("Lists", "Show All list", "Show Deadlines list", "Show Review list", "Show Tomorrow list", "Uncomplete") добавлены пустыми объектами без поля localizations. Если парсер/рендер ожидает наличие localizations (как у остальных записей), это приведет к:
- падениям (force unwrap/assumptions),
- пропаданию текста в UI,
- несогласованному фолбэку.
Рекомендация: всегда добавлять localizations хотя бы с базовым языком, например:
{
"localizations": {
"en": "...",
"ru": "..."
}
}
Либо гарантировать безопасный фолбэк в коде доступа:
title = node.localizations[locale] ?? node.localizations["en"] ?? node.key
-
Семантика/дублирование:
- Уже есть "Show Deadlines section", вы добавляете "Show Deadlines list". Это потенциальная путаница для пользователя и источника строк. Четко разделите и переименуйте:
- "Show Deadlines Section (Today)"
- "Show Deadlines List (Sidebar)"
или объедините, если поведение дублируется.
- Уже есть "Show Deadlines section", вы добавляете "Show Deadlines list". Это потенциальная путаница для пользователя и источника строк. Четко разделите и переименуйте:
Качество строк и консистентность
- "Uncomplete" — некорректный английский. Правильно: "Incomplete" или, если это действие, "Mark as Incomplete".
- Непоследовательная капитализация:
- "Show All list", "Show Review list", "Show Tomorrow list" — лучше унифицировать в "Show ... List" (оба слова с заглавной) или привести ко внутреннему гайду по капитализации.
- "Lists" выглядит как заголовок секции — ему тоже нужны локализации, иначе будет выпадать в дефолт.
Поддерживаемость/ключи
- Судя по файлу, вы используете человекочитаемые ключи как идентификаторы. Это усложняет рефакторинг и проверку. Рекомендация: перейти на стабильные ключи без пробелов и с неймспейсом (например, settings.lists.showAll), а тексты держать только в localizations. Это:
- упрощает поиск/замену,
- снижает риск опечаток,
- делает сортировку и валидацию предсказуемой.
Валидация и процесс
- Добавьте pre-commit/CI-проверки:
- JSON-схема: узел обязан содержать объект localizations.
- Покрытие обязательных локалей (например, en и ru).
- Запрет пустых объектов и пустых строк перевода.
- Линт капитализации по гайду.
- Юнит-тест: парсинг всего файла и проверка фолбэка локализаций, чтобы пустые/отсутствующие переводы не падали.
Мелкие замечания
- Поддерживайте порядок (алфавитный или логический). Сейчас вставки выглядят «как получилось», что усложняет ревью и делает диффы шумными.
- Уберите пустые строки внутри пустых объектов — это визуальный шум.
- Проверьте отсутствие «висячих» запятых у последнего элемента секции (JSON их не допускает).
Предлагаемые правки (образец для одного кейса)
- Было:
"Show Review list" : {
}, - Стало:
"Show Review List" : {
"localizations" : {
"en" : "Show Review List",
"ru" : "Показывать список «На проверку»"
}
}
И аналогично для остальных:
- "Lists" -> добавить локализации.
- "Show All list" -> "Show All List" (+ локализации).
- "Show Deadlines list" — переименовать, чтобы различалось с section, и добавить локализации.
- "Show Tomorrow list" -> "Show Tomorrow List" (+ локализации).
- "Uncomplete" -> "Incomplete" или "Mark as Incomplete" (+ локализации).
| CURRENT_PROJECT_VERSION = 103; | ||
| DEVELOPMENT_TEAM = 9Z68336878; | ||
| GENERATE_INFOPLIST_FILE = YES; | ||
| MACOSX_DEPLOYMENT_TARGET = 26.0; |
There was a problem hiding this comment.
Ниже — только то, что реально может укусить в сборке/релизе или что стоит улучшить.
-
Подозрительная настройка: MACOSX_DEPLOYMENT_TARGET = 26.0
- Это выглядит как опечатка/ошибка. Для macOS платформы допустимы значения вида 10.15, 11.0, 12.0, 13.0, 14.0, 15.0 и т.д. 26.0 невалидно и приведёт к варнингу/ошибке сборки или неожиданному поведению в Xcode.
- Проверьте оба встречающихся места с 26.0 в diff и замените на корректную целевую версию macOS для данного таргета.
-
Централизация версии
- Вы вручную обновляете CURRENT_PROJECT_VERSION во многих конфигурациях/таргетах — это очень легко рассинхронизировать в будущем.
- Рекомендации:
- Вынесите версионирование в xcconfig (например, Version.xcconfig) и подключите ко всем конфигурациям/таргетам.
- Либо задайте CURRENT_PROJECT_VERSION на уровне проекта (Project) и удалите дубли на уровне таргетов, оставив только где необходимо переопределение.
- Если используете agvtool, держите Apple Generic Versioning включённым и используйте agvtool new-version -all 103, чтобы исключить ручные правки .pbxproj.
-
Консистентность версий между контейнером и расширениями
- Для App Store Connect безопаснее иметь одинаковый CFBundleVersion у всех вложенных бандлов (приложение, виджеты, watch-приложение/расширения). Судя по diff, вы синхронизировали до 103 — это правильно. Проверьте, что нигде не остался таргет с иным значением (особенно там, где GENERATE_INFOPLIST_FILE = NO и CFBundleVersion прописан в кастомном Info.plist).
- Тестовые таргеты: указывать для них CURRENT_PROJECT_VERSION не обязательно; можно убрать, чтобы снизить шум и риск рассинхронизации.
-
MARKETING_VERSION
- У части таргетов задано MARKETING_VERSION = 1.0, у части — не видно (вероятно, наследуется или отсутствует). Убедитесь, что у всех дистрибутируемых бандлов выставлен единый MARKETING_VERSION, чтобы не получить неожиданные значения в итоговых Info.plist.
-
Целевые версии платформ
- IPHONEOS_DEPLOYMENT_TARGET = 18.1 — очень высокий минимум. Если это не осознанное решение, пересмотрите: вы обрежете совместимость. Аналогично проверьте WATCHOS_DEPLOYMENT_TARGET/TVOS_DEPLOYMENT_TARGET для watch/widget таргетов на предмет реалистичности и согласованности с основной app-частью.
- Для watch-приложения/виджета явно задайте WATCHOS_DEPLOYMENT_TARGET (сейчас в diff не видно). Без явного значения Xcode может подставить неожиданное.
-
Автоматизация в CI
- Чтобы не управлять номером сборки руками, можно использовать номер билда из CI:
- Пример: CURRENT_PROJECT_VERSION = $(GITHUB_RUN_NUMBER) через xcconfig, а локально — fallback значение.
- Или скрипт в CI, который вызывает agvtool new-version -all перед сборкой.
- Чтобы не управлять номером сборки руками, можно использовать номер билда из CI:
-
Верификация перед релизом
- Добавьте шаг проверки в CI, который собирает build settings и убеждается, что все продукты имеют одинаковый CURRENT_PROJECT_VERSION:
- xcodebuild -showBuildSettings -alltargets | grep -E 'TARGET_NAME|CURRENT_PROJECT_VERSION'
- И отдельную проверку итоговых .app/.appex/.watchkitapp Info.plist (plutil -extract CFBundleVersion …), если где-то GENERATE_INFOPLIST_FILE = NO.
- Добавьте шаг проверки в CI, который собирает build settings и убеждается, что все продукты имеют одинаковый CURRENT_PROJECT_VERSION:
Если исправить MACOSX_DEPLOYMENT_TARGET и централизовать версионирование, остальное выглядит логичным для повышения build number до 103.
| #else | ||
| .swipeActions(edge: .leading, allowsFullSwipe: false) { | ||
| if let subtasks = task.subtasks { | ||
| NavigationLink { |
There was a problem hiding this comment.
Ниже только предметные замечания и улучшения, без очевидностей.
-
Платформозависимые EnvironmentObject:
- Вы убрали #if os(iOS) вокруг showInspector и selectedTasks, но оставили их как @EnvironmentObject для всех платформ. На watchOS это высокий риск краша при отсутствии инъекции (даже если логика «не используется», доступ к body или дочерним представлениям может всё равно попытаться резолвить объект).
- Рекомендации:
- Вернуть #if вокруг этих @EnvironmentObject, либо
- Инкапсулировать их через @Environment с кастомными ключами с дефолтами (если допустимо), либо
- Явно прокидывать зависимости только туда, где они реально нужны.
- Аналогично, проверьте FocusTimer/FocusTask на watchOS: если они не используются — оградите объявления условной компиляцией; если используются — убедитесь, что они действительно инжектятся на всех платформах.
-
Сравнение моделей SwiftData:
- Сравнение if let focus = focusTask.task, task == focus хрупко, если экземпляры одной модели происходят из разных контекстов/снепшотов. Лучше сравнивать по стабильному идентификатору модели (например, persistentModelID/objectID) вместо оператора ==.
-
Анимации и UX:
- Для completion/uncompletion нет withAnimation, в отличие от удаления. Перемещение/скрытие строки при изменении completed часто сопровождается сортировкой/фильтрацией — добавьте withAnimation для согласованности UI.
- Подумайте, действительно ли full swipe должен быть «Complete», а не «Delete» (зависит от вашей UX-логики, но стоит проверить ожидания пользователей).
-
Локализация и текст:
- "Uncomplete" — некорректный английский. Обычно используют "Mark Incomplete" или "Incomplete". Вынесите строки в локализацию (LocalizedStringKey) и приведите к понятным ключам. Иконки подобраны ок.
-
Инкапсуляция логики таймера:
- Вью-слой выполняет timer.reset()/skip() при завершении текущей задачи. Лучше инкапсулировать это в методе таймера/фокуса, например focusTask.endFocusIfMatches(taskID:) или timer.handleTaskCompletionIfFocused(taskID:), чтобы не дублировать бизнес-правила и удерживать инварианты в одном месте.
- Проверьте, действительно ли skip() уместен только в .pause/.longbreak. Если есть другие режимы (work/shortbreak), опишите ожидаемое поведение явно в одном месте.
-
Поток исполнения:
- Все мутации модели и ObservableObject желательно гарантировать на главном потоке. Пометьте методы FocusTimer/FocusTask/@Model-операции как @mainactor или оберните вызовы в MainActor.run { ... }, чтобы избежать редких гонок при свайпах.
-
Сохранение в SwiftData:
- task.complete(modelContext:) вероятно делает save, но task.reactivate() — уже без контекста. Добейтесь симметрии: либо обе операции принимают context и сохраняют, либо управление сохранением снаружи. Явная обработка ошибок сохранения будет плюсом.
-
Неиспользуемые зависимости:
- refresher, list: SideBarItem? в этом фрагменте не применяются. Если они не нужны в модификаторе, удалите, чтобы снизить связность и риски инжекции «в никуда».
-
Доступность:
- Добавьте явные accessibilityLabel/Hint для кнопок свайпа и динамически меняйте их при completed/не completed. Это поможет VoiceOver и сделает интерфейс понятнее.
-
Платформенные нюансы:
- NavigationLink внутри .swipeActions для некоторых платформ выглядит спорно (особенно на macOS). Проверьте UX и поведение: иногда лучше открывать сущности через .sheet/.popover из действия, чем вкладывать NavigationLink прямо в swipe.
- Условная компиляция .swipeActions(edge: .leading) заменена на #if !os(watchOS). Убедитесь, что trailing-действия действительно поддерживаются на watchOS версии, которую вы таргетируете, иначе оградите и trailing аналогично.
-
Нейминг/единообразие API:
- Enum case .longbreak лучше именовать .longBreak для читаемости (Swift API Design Guidelines).
- Приведите названия методов модели (complete/reactivate) к консистентности: либо обе принимают контекст, либо оба — нет; предусмотреть возврат Result/throws.
Если коротко, приоритетно: вернуть платформенные гардовки вокруг EnvironmentObject, сравнивать модели по стабильному ID, добавить анимацию для complete/uncomplete и вынести таймерную логику из вью в слой модели/сервиса. Это снимет основные риски крашей и улучшит сопровождаемость.
1 participant
No description provided.