From c38d4075be2a34a00529e8d7f5ade4cd6679166a Mon Sep 17 00:00:00 2001 From: Hendrik Hogertz Date: Sun, 15 Feb 2026 06:04:41 +0100 Subject: [PATCH] Fix grocery sync deletions not persisting and Reminders race condition Stop cascading syncs by adding an isReconciling flag so that reconcileFromServer no longer triggers scheduleSync via addItem/deleteItem. Make Reminders write-only by removing the diff/sync logic from the onDataChanged callback. Fetch fresh server state in RecipeView reconcile instead of using stale local cache. Track pending removal recipe IDs via DataStore so performInitialSync can push deletions for recipes whose grocery keys have already been removed from groceryDict. Fix a race condition in RemindersGroceryStore where EKEventStoreChanged notifications triggered load() before saveMappings() finished writing to disk, causing the correct in-memory state to be overwritten with stale data. Add ignoreNextExternalChange flag to skip self-triggered reloads. Restyle the add/remove all grocery button to match the Plan recipe button style using Label, subheadline font, and rounded rectangle background. Co-Authored-By: Claude Sonnet 4.5 --- .../Data/GroceryListManager.swift | 20 ++-- .../Data/GroceryStateSyncManager.swift | 91 ++++++++++++++++--- .../Data/RemindersGroceryStore.swift | 19 ++++ .../Localizable.xcstrings | 4 +- .../Views/Recipes/RecipeView.swift | 6 +- .../RecipeIngredientSection.swift | 18 ++-- 6 files changed, 123 insertions(+), 35 deletions(-) diff --git a/Nextcloud Cookbook iOS Client/Data/GroceryListManager.swift b/Nextcloud Cookbook iOS Client/Data/GroceryListManager.swift index 52358db..bf657b0 100644 --- a/Nextcloud Cookbook iOS Client/Data/GroceryListManager.swift +++ b/Nextcloud Cookbook iOS Client/Data/GroceryListManager.swift @@ -28,20 +28,7 @@ class GroceryListManager: ObservableObject { remindersStore.onDataChanged = { [weak self] in guard let self else { return } if self.mode == .appleReminders { - let previousDict = self.groceryDict self.groceryDict = self.remindersStore.groceryDict - - // Only sync recipes that changed externally (e.g. checked off in Reminders app), - // not ones we just modified ourselves. - for recipeId in self.remindersStore.groceryDict.keys { - guard !self.recentlyModifiedByUs.contains(recipeId) else { continue } - // Detect if item count changed (external add/remove/complete) - let oldCount = previousDict[recipeId]?.items.count ?? 0 - let newCount = self.remindersStore.groceryDict[recipeId]?.items.count ?? 0 - if oldCount != newCount { - self.syncManager?.scheduleSync(forRecipeId: recipeId) - } - } self.recentlyModifiedByUs.removeAll() } } @@ -63,6 +50,7 @@ class GroceryListManager: ObservableObject { remindersStore.addItem(itemName, toRecipe: recipeId, recipeName: recipeName) groceryDict = remindersStore.groceryDict } + syncManager?.clearPendingRemoval(recipeId: recipeId) syncManager?.scheduleSync(forRecipeId: recipeId) } @@ -76,6 +64,7 @@ class GroceryListManager: ObservableObject { remindersStore.addItems(items, toRecipe: recipeId, recipeName: recipeName) groceryDict = remindersStore.groceryDict } + syncManager?.clearPendingRemoval(recipeId: recipeId) syncManager?.scheduleSync(forRecipeId: recipeId) } @@ -88,6 +77,9 @@ class GroceryListManager: ObservableObject { recentlyModifiedByUs.insert(recipeId) remindersStore.deleteItem(itemName, fromRecipe: recipeId) } + if groceryDict[recipeId] == nil { + syncManager?.trackPendingRemoval(recipeId: recipeId) + } syncManager?.scheduleSync(forRecipeId: recipeId) } @@ -100,6 +92,7 @@ class GroceryListManager: ObservableObject { recentlyModifiedByUs.insert(recipeId) remindersStore.deleteGroceryRecipe(recipeId) } + syncManager?.trackPendingRemoval(recipeId: recipeId) syncManager?.scheduleSync(forRecipeId: recipeId) } @@ -114,6 +107,7 @@ class GroceryListManager: ObservableObject { remindersStore.deleteAll() } for recipeId in recipeIds { + syncManager?.trackPendingRemoval(recipeId: recipeId) syncManager?.scheduleSync(forRecipeId: recipeId) } } diff --git a/Nextcloud Cookbook iOS Client/Data/GroceryStateSyncManager.swift b/Nextcloud Cookbook iOS Client/Data/GroceryStateSyncManager.swift index 682e3ce..06325cd 100644 --- a/Nextcloud Cookbook iOS Client/Data/GroceryStateSyncManager.swift +++ b/Nextcloud Cookbook iOS Client/Data/GroceryStateSyncManager.swift @@ -13,6 +13,11 @@ class GroceryStateSyncManager { private var debounceTimers: [String: Task] = [:] private let debounceInterval: TimeInterval = 2.0 + private var isReconciling = false + + private let dataStore = DataStore() + private let pendingRemovalPath = "grocery_pending_removals.data" + private(set) var pendingRemovalRecipeIds: Set = [] init(appState: AppState, groceryManager: GroceryListManager) { self.appState = appState @@ -24,6 +29,7 @@ class GroceryStateSyncManager { /// Debounced sync trigger. Waits `debounceInterval` seconds then pushes state for the recipe. func scheduleSync(forRecipeId recipeId: String) { guard UserSettings.shared.grocerySyncEnabled else { return } + guard !isReconciling else { return } debounceTimers[recipeId]?.cancel() debounceTimers[recipeId] = Task { [weak self] in @@ -38,17 +44,18 @@ class GroceryStateSyncManager { guard let appState, let groceryManager else { return } guard let recipeIdInt = Int(recipeId) else { return } - // Build local state from current grocery data - let localState = buildLocalState(forRecipeId: recipeId, groceryManager: groceryManager) - - // Fetch latest recipe from server + // Fetch latest recipe from server first so we can detect deletions guard let serverRecipe = await appState.getRecipe(id: recipeIdInt, fetchMode: .onlyServer) else { Logger.data.error("Grocery sync: failed to fetch recipe \(recipeId) from server") return } - // Merge local state with server state let serverState = serverRecipe.groceryState + + // Build local state, passing server state so deleted items can be marked .removed + let localState = buildLocalState(forRecipeId: recipeId, groceryManager: groceryManager, serverState: serverState) + + // Merge local state with server state let merged = mergeStates(local: localState, server: serverState) // Upload merged state @@ -67,6 +74,9 @@ class GroceryStateSyncManager { guard let groceryManager else { return } guard let serverState, !serverState.items.isEmpty else { return } + isReconciling = true + defer { isReconciling = false } + let localItems = Set( groceryManager.groceryDict[recipeId]?.items.map(\.name) ?? [] ) @@ -94,6 +104,8 @@ class GroceryStateSyncManager { func performInitialSync() async { guard let appState, let groceryManager else { return } + await loadPendingRemovals() + let recipeIds = Array(groceryManager.groceryDict.keys) for recipeId in recipeIds { guard let recipeIdInt = Int(recipeId) else { continue } @@ -111,6 +123,18 @@ class GroceryStateSyncManager { ) } } + + // Push deletion state for recipes whose items were fully removed + for recipeId in pendingRemovalRecipeIds { + guard !recipeIds.contains(recipeId) else { + // Recipe was re-added locally since removal was tracked; clear it + pendingRemovalRecipeIds.remove(recipeId) + continue + } + await pushGroceryState(forRecipeId: recipeId) + pendingRemovalRecipeIds.remove(recipeId) + } + savePendingRemovals() } // MARK: - Merge Logic @@ -151,18 +175,59 @@ class GroceryStateSyncManager { // MARK: - Build Local State /// Builds a `GroceryState` from the current local grocery data for a recipe. - private func buildLocalState(forRecipeId recipeId: String, groceryManager: GroceryListManager) -> GroceryState { - guard let groceryRecipe = groceryManager.groceryDict[recipeId] else { - return GroceryState() - } - + /// When `serverState` is provided, any server item with `.added` status that is + /// absent locally is emitted as `.removed` so the deletion propagates to the server. + private func buildLocalState(forRecipeId recipeId: String, groceryManager: GroceryListManager, serverState: GroceryState?) -> GroceryState { var items: [String: GroceryItemState] = [:] let now = GroceryStateDate.now() - for item in groceryRecipe.items { - let status: GroceryItemState.Status = item.isChecked ? .completed : .added - items[item.name] = GroceryItemState(status: status, addedAt: now, modifiedAt: now) + + // Existing local items + if let groceryRecipe = groceryManager.groceryDict[recipeId] { + for item in groceryRecipe.items { + let status: GroceryItemState.Status = item.isChecked ? .completed : .added + items[item.name] = GroceryItemState(status: status, addedAt: now, modifiedAt: now) + } + } + + // Mark items that exist on server as .added but are absent locally as .removed + if let serverState { + for (itemName, serverItem) in serverState.items { + if items[itemName] == nil && serverItem.status == .added { + items[itemName] = GroceryItemState( + status: .removed, + addedAt: serverItem.addedAt, + modifiedAt: now + ) + } + } } return GroceryState(lastModified: now, items: items) } + + // MARK: - Pending Removal Tracking + + /// Records a recipe ID whose grocery items were fully removed, so that + /// `performInitialSync` can push the deletion even after the key disappears + /// from `groceryDict`. + func trackPendingRemoval(recipeId: String) { + pendingRemovalRecipeIds.insert(recipeId) + savePendingRemovals() + } + + func clearPendingRemoval(recipeId: String) { + guard pendingRemovalRecipeIds.remove(recipeId) != nil else { return } + savePendingRemovals() + } + + private func loadPendingRemovals() async { + guard let loaded: Set = try? await dataStore.load(fromPath: pendingRemovalPath) else { return } + pendingRemovalRecipeIds = loaded + } + + private func savePendingRemovals() { + Task { + await dataStore.save(data: pendingRemovalRecipeIds, toPath: pendingRemovalPath) + } + } } diff --git a/Nextcloud Cookbook iOS Client/Data/RemindersGroceryStore.swift b/Nextcloud Cookbook iOS Client/Data/RemindersGroceryStore.swift index 1827679..7726ff2 100644 --- a/Nextcloud Cookbook iOS Client/Data/RemindersGroceryStore.swift +++ b/Nextcloud Cookbook iOS Client/Data/RemindersGroceryStore.swift @@ -35,6 +35,11 @@ class RemindersGroceryStore { private let mappingPath = "reminder_mappings.data" private var mappingStore = ReminderMappingStore() + /// When true, the next `EKEventStoreChanged` notification is skipped because + /// it was triggered by our own save. Prevents a race where `load()` reads stale + /// mapping data from disk before `saveMappings()` finishes writing. + private var ignoreNextExternalChange = false + init() { NotificationCenter.default.addObserver( forName: .EKEventStoreChanged, @@ -43,6 +48,10 @@ class RemindersGroceryStore { ) { [weak self] _ in Task { @MainActor in guard let self else { return } + if self.ignoreNextExternalChange { + self.ignoreNextExternalChange = false + return + } await self.load() self.onDataChanged?() } @@ -104,11 +113,13 @@ class RemindersGroceryStore { reminder.title = itemName reminder.calendar = calendar do { + ignoreNextExternalChange = true try eventStore.save(reminder, commit: true) let name = recipeName ?? "-" addMapping(reminderIdentifier: reminder.calendarItemIdentifier, recipeId: recipeId, recipeName: name, itemName: itemName) appendToCache(itemName: itemName, recipeId: recipeId, recipeName: name) } catch { + ignoreNextExternalChange = false Logger.view.error("Failed to save reminder: \(error.localizedDescription)") } } @@ -132,8 +143,10 @@ class RemindersGroceryStore { } } do { + ignoreNextExternalChange = true try eventStore.commit() } catch { + ignoreNextExternalChange = false Logger.view.error("Failed to commit reminders: \(error.localizedDescription)") } saveMappings() @@ -153,8 +166,10 @@ class RemindersGroceryStore { let reminders = await fetchReminders(matching: predicate, in: store) for reminder in reminders where reminder.calendarItemIdentifier == identifier { do { + self.ignoreNextExternalChange = true try self.eventStore.remove(reminder, commit: true) } catch { + self.ignoreNextExternalChange = false Logger.view.error("Failed to remove reminder: \(error.localizedDescription)") } break @@ -181,8 +196,10 @@ class RemindersGroceryStore { } } do { + self.ignoreNextExternalChange = true try self.eventStore.commit() } catch { + self.ignoreNextExternalChange = false Logger.view.error("Failed to commit reminder removal: \(error.localizedDescription)") } self.mappingStore.recipes.removeValue(forKey: recipeId) @@ -209,8 +226,10 @@ class RemindersGroceryStore { } } do { + self.ignoreNextExternalChange = true try self.eventStore.commit() } catch { + self.ignoreNextExternalChange = false Logger.view.error("Failed to commit reminder removal: \(error.localizedDescription)") } self.mappingStore.recipes = [:] diff --git a/Nextcloud Cookbook iOS Client/Localizable.xcstrings b/Nextcloud Cookbook iOS Client/Localizable.xcstrings index 8424d72..73ccf28 100644 --- a/Nextcloud Cookbook iOS Client/Localizable.xcstrings +++ b/Nextcloud Cookbook iOS Client/Localizable.xcstrings @@ -3802,12 +3802,12 @@ } } }, - "Remove from Grocery List" : { + "Remove all from Grocery List" : { "localizations" : { "de" : { "stringUnit" : { "state" : "translated", - "value" : "Von der Einkaufsliste entfernen" + "value" : "Alle von Einkaufsliste entfernen" } }, "es" : { diff --git a/Nextcloud Cookbook iOS Client/Views/Recipes/RecipeView.swift b/Nextcloud Cookbook iOS Client/Views/Recipes/RecipeView.swift index 02e24d0..3da0678 100644 --- a/Nextcloud Cookbook iOS Client/Views/Recipes/RecipeView.swift +++ b/Nextcloud Cookbook iOS Client/Views/Recipes/RecipeView.swift @@ -88,8 +88,12 @@ struct RecipeView: View { // Reconcile server grocery state with local data if UserSettings.shared.grocerySyncEnabled { + let serverRecipe = await appState.getRecipe( + id: viewModel.recipe.recipe_id, + fetchMode: .onlyServer + ) groceryList.syncManager?.reconcileFromServer( - serverState: viewModel.recipeDetail.groceryState, + serverState: serverRecipe?.groceryState, recipeId: String(viewModel.recipe.recipe_id), recipeName: viewModel.recipeDetail.name ) diff --git a/Nextcloud Cookbook iOS Client/Views/Recipes/RecipeViewSections/RecipeIngredientSection.swift b/Nextcloud Cookbook iOS Client/Views/Recipes/RecipeViewSections/RecipeIngredientSection.swift index 74fe6f4..224ebd4 100644 --- a/Nextcloud Cookbook iOS Client/Views/Recipes/RecipeViewSections/RecipeIngredientSection.swift +++ b/Nextcloud Cookbook iOS Client/Views/Recipes/RecipeViewSections/RecipeIngredientSection.swift @@ -61,14 +61,20 @@ struct RecipeIngredientSection: View { } } } label: { - HStack { - Image(systemName: groceryList.containsRecipe(viewModel.observableRecipeDetail.id) ? "cart.badge.minus" : "cart.badge.plus") - .foregroundStyle(groceryList.containsRecipe(viewModel.observableRecipeDetail.id) ? .red : .green) - Text(groceryList.containsRecipe(viewModel.observableRecipeDetail.id) ? "Remove from Grocery List" : "Add All to Grocery List") - } + Label( + groceryList.containsRecipe(viewModel.observableRecipeDetail.id) ? "Remove all from Grocery List" : "Add All to Grocery List", + systemImage: groceryList.containsRecipe(viewModel.observableRecipeDetail.id) ? "cart.badge.minus" : "cart.badge.plus" + ) + .font(.subheadline) + .fontWeight(.medium) .frame(maxWidth: .infinity) + .padding(.vertical, 10) + .foregroundStyle(groceryList.containsRecipe(viewModel.observableRecipeDetail.id) ? Color.red : Color.green) + .background( + RoundedRectangle(cornerRadius: 10) + .fill((groceryList.containsRecipe(viewModel.observableRecipeDetail.id) ? Color.red : Color.green).opacity(0.1)) + ) } - .buttonStyle(.bordered) .padding(.top, 8) } .padding()