From 594731233901bd18621d693cebb17626e2aff3f3 Mon Sep 17 00:00:00 2001 From: Ilyas Hallak Date: Thu, 4 Sep 2025 21:15:54 +0200 Subject: [PATCH] fix: Core Data threading and network error handling - Add thread-safe NSManagedObjectContext extension - Fix EXC_BAD_ACCESS with performAndWait wrappers - Add network error detection with retry functionality - Change hero image to aspectFill for better layout - Mark classes as @unchecked Sendable for Swift Concurrency --- URLShare/OfflineBookmarkManager.swift | 55 +++++++------ readeck.xcodeproj/project.pbxproj | 9 ++- .../NSManagedObjectContext+SafeFetch.swift | 77 +++++++++++++++++++ .../Data/Repository/OfflineSyncManager.swift | 19 +++-- .../Data/Repository/SettingsRepository.swift | 18 ++--- .../BookmarkDetail/BookmarkDetailView.swift | 2 +- readeck/UI/Bookmarks/BookmarksView.swift | 11 +-- readeck/UI/Bookmarks/BookmarksViewModel.swift | 43 ++++++++++- 8 files changed, 183 insertions(+), 51 deletions(-) create mode 100644 readeck/Data/Extensions/NSManagedObjectContext+SafeFetch.swift diff --git a/URLShare/OfflineBookmarkManager.swift b/URLShare/OfflineBookmarkManager.swift index c7a7dce..6b317b9 100644 --- a/URLShare/OfflineBookmarkManager.swift +++ b/URLShare/OfflineBookmarkManager.swift @@ -1,7 +1,7 @@ import Foundation import CoreData -class OfflineBookmarkManager { +class OfflineBookmarkManager: @unchecked Sendable { static let shared = OfflineBookmarkManager() private init() {} @@ -17,27 +17,31 @@ class OfflineBookmarkManager { func saveOfflineBookmark(url: String, title: String = "", tags: [String] = []) -> Bool { let tagsString = tags.joined(separator: ",") - // Check if URL already exists offline - let fetchRequest: NSFetchRequest = ArticleURLEntity.fetchRequest() - fetchRequest.predicate = NSPredicate(format: "url == %@", url) - do { - let existingEntities = try context.fetch(fetchRequest) - if let existingEntity = existingEntities.first { - // Update existing entry - existingEntity.tags = tagsString - existingEntity.title = title - } else { - // Create new entry - let entity = ArticleURLEntity(context: context) - entity.id = UUID() - entity.url = url - entity.title = title - entity.tags = tagsString + try context.safePerform { [weak self] in + guard let self = self else { return } + + // Check if URL already exists offline + let fetchRequest: NSFetchRequest = ArticleURLEntity.fetchRequest() + fetchRequest.predicate = NSPredicate(format: "url == %@", url) + + let existingEntities = try self.context.fetch(fetchRequest) + if let existingEntity = existingEntities.first { + // Update existing entry + existingEntity.tags = tagsString + existingEntity.title = title + } else { + // Create new entry + let entity = ArticleURLEntity(context: self.context) + entity.id = UUID() + entity.url = url + entity.title = title + entity.tags = tagsString + } + + try self.context.save() + print("Bookmark saved offline: \(url)") } - - try context.save() - print("Bookmark saved offline: \(url)") return true } catch { print("Failed to save offline bookmark: \(error)") @@ -46,11 +50,14 @@ class OfflineBookmarkManager { } func getTags() -> [String] { - let fetchRequest: NSFetchRequest = TagEntity.fetchRequest() - do { - let tagEntities = try context.fetch(fetchRequest) - return tagEntities.compactMap { $0.name }.sorted() + return try context.safePerform { [weak self] in + guard let self = self else { return [] } + + let fetchRequest: NSFetchRequest = TagEntity.fetchRequest() + let tagEntities = try self.context.fetch(fetchRequest) + return tagEntities.compactMap { $0.name }.sorted() + } } catch { print("Failed to fetch tags: \(error)") return [] diff --git a/readeck.xcodeproj/project.pbxproj b/readeck.xcodeproj/project.pbxproj index 04bb1dd..2b88c0d 100644 --- a/readeck.xcodeproj/project.pbxproj +++ b/readeck.xcodeproj/project.pbxproj @@ -81,6 +81,7 @@ membershipExceptions = ( Assets.xcassets, Data/CoreData/CoreDataManager.swift, + "Data/Extensions/NSManagedObjectContext+SafeFetch.swift", Data/KeychainHelper.swift, Domain/Model/Bookmark.swift, Domain/Model/BookmarkLabel.swift, @@ -435,7 +436,7 @@ buildSettings = { CODE_SIGN_ENTITLEMENTS = URLShare/URLShare.entitlements; CODE_SIGN_STYLE = Automatic; - CURRENT_PROJECT_VERSION = 21; + CURRENT_PROJECT_VERSION = 22; DEVELOPMENT_TEAM = 8J69P655GN; GENERATE_INFOPLIST_FILE = YES; INFOPLIST_FILE = URLShare/Info.plist; @@ -468,7 +469,7 @@ buildSettings = { CODE_SIGN_ENTITLEMENTS = URLShare/URLShare.entitlements; CODE_SIGN_STYLE = Automatic; - CURRENT_PROJECT_VERSION = 21; + CURRENT_PROJECT_VERSION = 22; DEVELOPMENT_TEAM = 8J69P655GN; GENERATE_INFOPLIST_FILE = YES; INFOPLIST_FILE = URLShare/Info.plist; @@ -623,7 +624,7 @@ CODE_SIGN_ENTITLEMENTS = readeck/readeck.entitlements; CODE_SIGN_IDENTITY = "Apple Development"; CODE_SIGN_STYLE = Automatic; - CURRENT_PROJECT_VERSION = 21; + CURRENT_PROJECT_VERSION = 22; DEVELOPMENT_ASSET_PATHS = "\"readeck/Preview Content\""; DEVELOPMENT_TEAM = 8J69P655GN; ENABLE_HARDENED_RUNTIME = YES; @@ -667,7 +668,7 @@ CODE_SIGN_ENTITLEMENTS = readeck/readeck.entitlements; CODE_SIGN_IDENTITY = "Apple Development"; CODE_SIGN_STYLE = Automatic; - CURRENT_PROJECT_VERSION = 21; + CURRENT_PROJECT_VERSION = 22; DEVELOPMENT_ASSET_PATHS = "\"readeck/Preview Content\""; DEVELOPMENT_TEAM = 8J69P655GN; ENABLE_HARDENED_RUNTIME = YES; diff --git a/readeck/Data/Extensions/NSManagedObjectContext+SafeFetch.swift b/readeck/Data/Extensions/NSManagedObjectContext+SafeFetch.swift new file mode 100644 index 0000000..4f4f14a --- /dev/null +++ b/readeck/Data/Extensions/NSManagedObjectContext+SafeFetch.swift @@ -0,0 +1,77 @@ +// +// NSManagedObjectContext+SafeFetch.swift +// readeck +// +// Created by Ilyas Hallak on 25.07.25. +// +// SPDX-License-Identifier: MIT +// +// This file is part of the readeck project and is licensed under the MIT License. +// + +import CoreData +import Foundation + +extension NSManagedObjectContext { + + /// Thread-safe fetch that automatically wraps the operation in performAndWait + func safeFetch(_ request: NSFetchRequest) throws -> [T] { + var results: [T] = [] + var fetchError: Error? + + performAndWait { + do { + results = try self.fetch(request) + } catch { + fetchError = error + } + } + + if let error = fetchError { + throw error + } + + return results + } + + /// Thread-safe perform operation with return value + func safePerform(_ operation: @escaping @Sendable () throws -> T) throws -> T { + var result: T? + var operationError: Error? + + performAndWait { + do { + result = try operation() + } catch { + operationError = error + } + } + + if let error = operationError { + throw error + } + + guard let unwrappedResult = result else { + throw NSError(domain: "SafePerformError", code: -1, userInfo: [NSLocalizedDescriptionKey: "Operation returned nil"]) + } + + return unwrappedResult + } + + /// Thread-safe perform operation without return value + func safePerform(_ operation: @escaping () throws -> Void) throws { + var operationError: Error? + + performAndWait { + do { + try operation() + } catch { + operationError = error + } + } + + if let error = operationError { + throw error + } + } +} \ No newline at end of file diff --git a/readeck/Data/Repository/OfflineSyncManager.swift b/readeck/Data/Repository/OfflineSyncManager.swift index b9335fa..2cdb0dd 100644 --- a/readeck/Data/Repository/OfflineSyncManager.swift +++ b/readeck/Data/Repository/OfflineSyncManager.swift @@ -2,7 +2,7 @@ import Foundation import CoreData import SwiftUI -class OfflineSyncManager: ObservableObject { +class OfflineSyncManager: ObservableObject, @unchecked Sendable { static let shared = OfflineSyncManager() @Published var isSyncing = false @@ -99,10 +99,9 @@ class OfflineSyncManager: ObservableObject { } private func getOfflineBookmarks() -> [ArticleURLEntity] { - let fetchRequest: NSFetchRequest = ArticleURLEntity.fetchRequest() - do { - return try coreDataManager.context.fetch(fetchRequest) + let fetchRequest: NSFetchRequest = ArticleURLEntity.fetchRequest() + return try coreDataManager.context.safeFetch(fetchRequest) } catch { print("Failed to fetch offline bookmarks: \(error)") return [] @@ -110,8 +109,16 @@ class OfflineSyncManager: ObservableObject { } private func deleteOfflineBookmark(_ entity: ArticleURLEntity) { - coreDataManager.context.delete(entity) - coreDataManager.save() + do { + try coreDataManager.context.safePerform { [weak self] in + guard let self = self else { return } + + self.coreDataManager.context.delete(entity) + self.coreDataManager.save() + } + } catch { + print("Failed to delete offline bookmark: \(error)") + } } // MARK: - Auto Sync on Server Connectivity Changes diff --git a/readeck/Data/Repository/SettingsRepository.swift b/readeck/Data/Repository/SettingsRepository.swift index ada23fb..6a30a0c 100644 --- a/readeck/Data/Repository/SettingsRepository.swift +++ b/readeck/Data/Repository/SettingsRepository.swift @@ -42,6 +42,15 @@ class SettingsRepository: PSettingsRepository { private let userDefault = UserDefaults.standard private let keychainHelper = KeychainHelper.shared + var hasFinishedSetup: Bool { + get { + return userDefault.value(forKey: "hasFinishedSetup") as? Bool ?? false + } + set { + userDefault.set(newValue, forKey: "hasFinishedSetup") + } + } + func saveSettings(_ settings: Settings) async throws { // Save credentials to keychain if let endpoint = settings.endpoint, !endpoint.isEmpty { @@ -206,15 +215,6 @@ class SettingsRepository: PSettingsRepository { } } - var hasFinishedSetup: Bool { - get { - return userDefault.value(forKey: "hasFinishedSetup") as? Bool ?? false - } - set { - userDefault.set(newValue, forKey: "hasFinishedSetup") - } - } - func saveCardLayoutStyle(_ cardLayoutStyle: CardLayoutStyle) async throws { let context = coreDataManager.context diff --git a/readeck/UI/BookmarkDetail/BookmarkDetailView.swift b/readeck/UI/BookmarkDetail/BookmarkDetailView.swift index 01541d5..4a2e833 100644 --- a/readeck/UI/BookmarkDetail/BookmarkDetailView.swift +++ b/readeck/UI/BookmarkDetail/BookmarkDetailView.swift @@ -190,7 +190,7 @@ struct BookmarkDetailView: View { let offset = geo.frame(in: .global).minY ZStack(alignment: .top) { CachedAsyncImage(url: URL(string: viewModel.bookmarkDetail.imageUrl)) - .scaledToFit() + .aspectRatio(contentMode: .fill) .frame(width: geometry.size.width, height: headerHeight + (offset > 0 ? offset : 0)) .clipped() .offset(y: (offset > 0 ? -offset : 0)) diff --git a/readeck/UI/Bookmarks/BookmarksView.swift b/readeck/UI/Bookmarks/BookmarksView.swift index d75860e..98e887c 100644 --- a/readeck/UI/Bookmarks/BookmarksView.swift +++ b/readeck/UI/Bookmarks/BookmarksView.swift @@ -88,7 +88,8 @@ struct BookmarksView: View { private var shouldShowCenteredState: Bool { let isEmpty = viewModel.bookmarks?.bookmarks.isEmpty == true - return isEmpty && (viewModel.isLoading || viewModel.errorMessage != nil) + let hasError = viewModel.errorMessage != nil + return (isEmpty && viewModel.isLoading) || hasError } // MARK: - View Components @@ -134,16 +135,16 @@ struct BookmarksView: View { @ViewBuilder private func errorView(message: String) -> some View { VStack(spacing: 16) { - Image(systemName: "exclamationmark.triangle.fill") + Image(systemName: viewModel.isNetworkError ? "wifi.slash" : "exclamationmark.triangle.fill") .font(.system(size: 48)) .foregroundColor(.orange) VStack(spacing: 8) { - Text("Unable to load bookmarks") + Text(viewModel.isNetworkError ? "No internet connection" : "Unable to load bookmarks") .font(.headline) .foregroundColor(.primary) - Text(message) + Text(viewModel.isNetworkError ? "Please check your internet connection and try again" : message) .font(.subheadline) .foregroundColor(.secondary) .multilineTextAlignment(.center) @@ -151,7 +152,7 @@ struct BookmarksView: View { Button("Try Again") { Task { - await viewModel.loadBookmarks(state: state, type: type, tag: tag) + await viewModel.retryLoading() } } .buttonStyle(.borderedProminent) diff --git a/readeck/UI/Bookmarks/BookmarksViewModel.swift b/readeck/UI/Bookmarks/BookmarksViewModel.swift index 7836fc6..b4e43e8 100644 --- a/readeck/UI/Bookmarks/BookmarksViewModel.swift +++ b/readeck/UI/Bookmarks/BookmarksViewModel.swift @@ -13,6 +13,7 @@ class BookmarksViewModel { var isLoading = false var isInitialLoading = true var errorMessage: String? + var isNetworkError = false var currentState: BookmarkState = .unread var currentType = [BookmarkType.article] var currentTag: String? = nil @@ -123,8 +124,22 @@ class BookmarksViewModel { ) bookmarks = newBookmarks hasMoreData = newBookmarks.currentPage != newBookmarks.totalPages // check if more data is available + isNetworkError = false } catch { - errorMessage = "Error loading bookmarks" + // Check if it's a network error + if let urlError = error as? URLError { + switch urlError.code { + case .notConnectedToInternet, .networkConnectionLost, .timedOut, .cannotConnectToHost, .cannotFindHost: + isNetworkError = true + errorMessage = "No internet connection" + default: + isNetworkError = false + errorMessage = "Error loading bookmarks" + } + } else { + isNetworkError = false + errorMessage = "Error loading bookmarks" + } // Don't clear bookmarks on error - keep existing data visible } @@ -151,7 +166,20 @@ class BookmarksViewModel { bookmarks?.bookmarks.append(contentsOf: newBookmarks.bookmarks) hasMoreData = newBookmarks.currentPage != newBookmarks.totalPages } catch { - errorMessage = "Error loading more bookmarks" + // Check if it's a network error + if let urlError = error as? URLError { + switch urlError.code { + case .notConnectedToInternet, .networkConnectionLost, .timedOut, .cannotConnectToHost, .cannotFindHost: + isNetworkError = true + errorMessage = "No internet connection" + default: + isNetworkError = false + errorMessage = "Error loading more bookmarks" + } + } else { + isNetworkError = false + errorMessage = "Error loading more bookmarks" + } } isLoading = false @@ -162,6 +190,13 @@ class BookmarksViewModel { await loadBookmarks(state: currentState) } + @MainActor + func retryLoading() async { + errorMessage = nil + isNetworkError = false + await loadBookmarks(state: currentState, type: currentType, tag: currentTag) + } + @MainActor func toggleArchive(bookmark: Bookmark) async { do { @@ -258,6 +293,10 @@ class BookmarksViewModel { private func executeDelete(bookmark: Bookmark) async { do { try await deleteBookmarkUseCase.execute(bookmarkId: bookmark.id) + // If delete succeeds, remove bookmark from the list + await MainActor.run { + bookmarks?.bookmarks.removeAll { $0.id == bookmark.id } + } } catch { // If delete fails, restore the bookmark await MainActor.run {