diff --git a/ios/Sources/GutenbergKit/Sources/EditorLocalization.swift b/ios/Sources/GutenbergKit/Sources/EditorLocalization.swift index 27a406724..685c65d6a 100644 --- a/ios/Sources/GutenbergKit/Sources/EditorLocalization.swift +++ b/ios/Sources/GutenbergKit/Sources/EditorLocalization.swift @@ -12,6 +12,7 @@ public enum EditorLocalizableString { // MARK: - Media case failedToInsertMedia case failedToLoadSelectedMedia + case someSelectedMediaFailedToLoad(Int) case failedToProcessCapturedMedia // MARK: - Common @@ -102,6 +103,10 @@ public final class EditorLocalization { case .insertBlock: "Insert Block" case .failedToInsertMedia: "Failed to insert media" case .failedToLoadSelectedMedia: "The selected media could not be loaded. It may not be fully downloaded to this device." + case .someSelectedMediaFailedToLoad(let count): + count == 1 + ? "1 item could not be loaded and was skipped. It may not be fully downloaded to this device." + : "\(count) items could not be loaded and were skipped. They may not be fully downloaded to this device." case .failedToProcessCapturedMedia: "The captured media could not be processed." case .ok: "OK" case .patterns: "Patterns" diff --git a/ios/Sources/GutenbergKit/Sources/Media/ImportableMediaItem.swift b/ios/Sources/GutenbergKit/Sources/Media/ImportableMediaItem.swift new file mode 100644 index 000000000..31ae5e152 --- /dev/null +++ b/ios/Sources/GutenbergKit/Sources/Media/ImportableMediaItem.swift @@ -0,0 +1,20 @@ +import PhotosUI +import SwiftUI +import UniformTypeIdentifiers + +/// The slice of `PhotosPickerItem` that ``MediaFileManager/import(_:)`` relies on. +/// +/// `PhotosPickerItem` can't be constructed in a test, so the import path is +/// written against this protocol rather than the concrete type. `PhotosPickerItem` +/// already provides both members, so it conforms with an empty extension; tests +/// substitute a mock that returns canned transfer data or throws. +/// +/// `Sendable` because items cross into the `MediaFileManager` actor — a +/// requirement `PhotosPickerItem` already meets (the picker selection is captured +/// by a `@Sendable` task before it gets here). +protocol ImportableMediaItem: Sendable { + var supportedContentTypes: [UTType] { get } + func loadTransferable(type: T.Type) async throws -> T? +} + +extension PhotosPickerItem: ImportableMediaItem {} diff --git a/ios/Sources/GutenbergKit/Sources/Media/MediaFileManager.swift b/ios/Sources/GutenbergKit/Sources/Media/MediaFileManager.swift index 8d00208cd..42fe7ee50 100644 --- a/ios/Sources/GutenbergKit/Sources/Media/MediaFileManager.swift +++ b/ios/Sources/GutenbergKit/Sources/Media/MediaFileManager.swift @@ -26,8 +26,11 @@ actor MediaFileManager { /// Imports a photo picker item and saves it to the uploads directory. /// + /// Takes ``ImportableMediaItem`` rather than `PhotosPickerItem` directly so the + /// import path can be exercised with a mock under test. + /// /// - Returns: MediaInfo with a `gbk-media-file://` URL and detected media type - func `import`(_ item: PhotosPickerItem) async throws -> MediaInfo { + func `import`(_ item: some ImportableMediaItem) async throws -> MediaInfo { let data: Data? do { data = try await item.loadTransferable(type: Data.self) diff --git a/ios/Sources/GutenbergKit/Sources/Views/BlockInserter/BlockInserterView.swift b/ios/Sources/GutenbergKit/Sources/Views/BlockInserter/BlockInserterView.swift index 7d4554d36..d6e25935d 100644 --- a/ios/Sources/GutenbergKit/Sources/Views/BlockInserter/BlockInserterView.swift +++ b/ios/Sources/GutenbergKit/Sources/Views/BlockInserter/BlockInserterView.swift @@ -27,6 +27,11 @@ struct BlockInserterView: View { @State private var isShowingCamera = false @State private var availableWidth: CGFloat = 0 + /// Whether acknowledging the media-import alert should also close the + /// inserter. Set when a partial import inserted something worth keeping; a + /// total failure leaves it `false` so the inserter stays open to retry. + @State private var shouldDismissAfterError = false + @ScaledMetric(relativeTo: .largeTitle) private var inlinePickerHeight = 116 @Environment(\.dismiss) private var dismiss @@ -77,7 +82,15 @@ struct BlockInserterView: View { Alert( title: Text(EditorLocalization[.failedToInsertMedia]), message: Text(error.message), - dismissButton: .default(Text(EditorLocalization[.ok])) + dismissButton: .default(Text(EditorLocalization[.ok])) { + // A partial import already inserted what it could and this + // alert reported the rest, so acknowledging it finishes the + // flow. A total failure inserted nothing, so stay open to retry. + if shouldDismissAfterError { + shouldDismissAfterError = false + dismiss() + } + } ) } .animation(.smooth(duration: 2), value: viewModel.isProcessingMedia) @@ -288,10 +301,23 @@ struct BlockInserterView: View { private func insertMedia(_ items: [PhotosPickerItem]) { Task { - let items = await viewModel.processSelectedPhotosPickerItems(items) - if !items.isEmpty { + let mediaInfo = await viewModel.processSelectedPhotosPickerItems(items) + + guard viewModel.error == nil else { + // Some or all items failed to import. Insert whatever succeeded, + // then let the alert report the rest. Closing the alert dismisses + // the inserter only when something was inserted (a partial + // success); a total failure keeps it open so the user can retry. + if !mediaInfo.isEmpty { + onSelection(.media(mediaInfo)) + } + shouldDismissAfterError = !mediaInfo.isEmpty + return + } + + if !mediaInfo.isEmpty { dismiss() - onSelection(.media(items)) + onSelection(.media(mediaInfo)) } } } diff --git a/ios/Sources/GutenbergKit/Sources/Views/BlockInserter/BlockInserterViewModel.swift b/ios/Sources/GutenbergKit/Sources/Views/BlockInserter/BlockInserterViewModel.swift index 80ce1ad99..9c7c35588 100644 --- a/ios/Sources/GutenbergKit/Sources/Views/BlockInserter/BlockInserterViewModel.swift +++ b/ios/Sources/GutenbergKit/Sources/Views/BlockInserter/BlockInserterViewModel.swift @@ -12,7 +12,7 @@ class BlockInserterViewModel: ObservableObject { @Published private(set) var isProcessingMedia = false private let allSections: [BlockInserterSection] - private let fileManager: MediaFileManager = .shared + private let fileManager: MediaFileManager private var processingTask: Task<[MediaInfo], Never>? private var cancellables = Set() @@ -21,8 +21,11 @@ class BlockInserterViewModel: ObservableObject { let message: String } - init(sections: [BlockInserterSection]) { + /// - Parameter fileManager: Injectable so tests can point imports at a + /// temporary directory; defaults to the app-wide shared instance. + init(sections: [BlockInserterSection], fileManager: MediaFileManager = .shared) { self.allSections = sections + self.fileManager = fileManager self.sections = sections.filter { $0.category != "gbk-search-only" } setupSearchObserver() @@ -68,31 +71,36 @@ class BlockInserterViewModel: ObservableObject { // MARK: - Media Processing - func processSelectedPhotosPickerItems(_ items: [PhotosPickerItem]) async -> [MediaInfo] { + func processSelectedPhotosPickerItems(_ items: [some ImportableMediaItem]) async -> [MediaInfo] { isProcessingMedia = true defer { isProcessingMedia = false } let task = Task<[MediaInfo], Never> { @MainActor in var results: [MediaInfo] = [] - var anyError: Error? - - do { - for item in items { - let item = try await self.fileManager.import(item) - results.append(item) + var failureCount = 0 + + // Import each item independently so one failure doesn't abandon the + // rest of the selection. A single unreadable photo — e.g. one not + // fully downloaded from iCloud — should skip only itself, not drop + // the items picked alongside it. + for item in items { + if Task.isCancelled { break } + do { + results.append(try await self.fileManager.import(item)) + } catch { + failureCount += 1 + Logger.media.error("Failed to import picker selection: \(error)") } - } catch { - anyError = error - Logger.media.error("Failed to import picker selection: \(error)") } guard !Task.isCancelled else { return [] } - if results.isEmpty, anyError != nil { - self.error = MediaError(message: EditorLocalization[.failedToLoadSelectedMedia]) - } + self.error = Self.importError( + failureCount: failureCount, + successCount: results.count + ) return results } @@ -100,6 +108,20 @@ class BlockInserterViewModel: ObservableObject { return await task.value } + /// The alert to show after importing a picker selection, or `nil` when every + /// item imported. + /// + /// A partial failure still surfaces an error — naming how many items were + /// skipped — so the dropped items aren't lost silently; a total failure + /// keeps the existing "nothing could be loaded" message. + static func importError(failureCount: Int, successCount: Int) -> MediaError? { + guard failureCount > 0 else { return nil } + if successCount == 0 { + return MediaError(message: EditorLocalization[.failedToLoadSelectedMedia]) + } + return MediaError(message: EditorLocalization[.someSelectedMediaFailedToLoad(failureCount)]) + } + func processCameraMedia(_ media: CameraMedia) async -> [MediaInfo] { isProcessingMedia = true defer { isProcessingMedia = false } diff --git a/ios/Tests/GutenbergKitTests/EditorLocalizationTests.swift b/ios/Tests/GutenbergKitTests/EditorLocalizationTests.swift index c793ada83..7a2d4ce3d 100644 --- a/ios/Tests/GutenbergKitTests/EditorLocalizationTests.swift +++ b/ios/Tests/GutenbergKitTests/EditorLocalizationTests.swift @@ -42,6 +42,14 @@ struct EditorLocalizationTests { } } + @Test + func defaultsPluralizeSkippedMediaCounts() { + withLocalization { + #expect(EditorLocalization[.someSelectedMediaFailedToLoad(1)].hasPrefix("1 item ")) + #expect(EditorLocalization[.someSelectedMediaFailedToLoad(3)].hasPrefix("3 items ")) + } + } + @Test func subscriptUsesTheDefaultsWithoutAHostOverride() { withLocalization { diff --git a/ios/Tests/GutenbergKitTests/Views/BlockInserterViewModelTests.swift b/ios/Tests/GutenbergKitTests/Views/BlockInserterViewModelTests.swift new file mode 100644 index 000000000..9f49485ff --- /dev/null +++ b/ios/Tests/GutenbergKitTests/Views/BlockInserterViewModelTests.swift @@ -0,0 +1,114 @@ +import Foundation +import SwiftUI +import Testing +import UniformTypeIdentifiers +@testable import GutenbergKit + +#if canImport(UIKit) + +/// Exercises `processSelectedPhotosPickerItems` end-to-end. The picker hands it +/// `PhotosPickerItem`s, which can't be constructed in a test, so imports run +/// against a mock `ImportableMediaItem` instead. The view model's file manager is +/// pointed at a throwaway directory so successful imports stay off the real +/// Library folder. +@MainActor +@Suite("BlockInserterViewModel media import") +struct BlockInserterViewModelTests { + + private static let jpeg = Data([0xFF, 0xD8, 0xFF, 0xD9]) + + /// A view model whose imports write under a unique temp directory. Returns the + /// root too, so the caller can delete it when the test ends. + private func makeViewModel() -> (BlockInserterViewModel, root: URL) { + let root = URL.temporaryDirectory.appending(component: "GBKTests-\(UUID().uuidString)") + let viewModel = BlockInserterViewModel( + sections: [], + fileManager: MediaFileManager(rootURL: root) + ) + return (viewModel, root) + } + + // The bug this fixes: a failure used to abort the loop, so items picked + // alongside a bad one were dropped and — because something did import — no + // error was shown. + @Test("A failure alongside successes still inserts the successes and reports the failure") + func partialFailureKeepsSuccessesAndReportsError() async { + let (viewModel, root) = makeViewModel() + defer { try? FileManager.default.removeItem(at: root) } + + let results = await viewModel.processSelectedPhotosPickerItems([ + MockImportableMediaItem(.data(Self.jpeg)), + MockImportableMediaItem(.failure), + MockImportableMediaItem(.data(Self.jpeg)), + ]) + + #expect(results.count == 2) + #expect(viewModel.error != nil) + #expect(viewModel.isProcessingMedia == false) + } + + @Test("Every item failing inserts nothing and reports an error") + func totalFailureReportsErrorAndInsertsNothing() async { + let (viewModel, root) = makeViewModel() + defer { try? FileManager.default.removeItem(at: root) } + + let results = await viewModel.processSelectedPhotosPickerItems([ + MockImportableMediaItem(.failure), + MockImportableMediaItem(.empty), + ]) + + #expect(results.isEmpty) + #expect(viewModel.error != nil) + } + + @Test("Every item importing reports no error") + func allSuccessReportsNoError() async { + let (viewModel, root) = makeViewModel() + defer { try? FileManager.default.removeItem(at: root) } + + let results = await viewModel.processSelectedPhotosPickerItems([ + MockImportableMediaItem(.data(Self.jpeg)), + MockImportableMediaItem(.data(Self.jpeg)), + ]) + + #expect(results.count == 2) + #expect(viewModel.error == nil) + } + + @Test("Partial and total failures read differently") + func partialAndTotalMessagesDiffer() { + let partial = BlockInserterViewModel.importError(failureCount: 2, successCount: 3)?.message + let total = BlockInserterViewModel.importError(failureCount: 2, successCount: 0)?.message + #expect(partial != total) + } +} + +/// A stand-in for `PhotosPickerItem` that returns canned transfer data or fails, +/// so the import loop can be driven without the real Photos picker. +private struct MockImportableMediaItem: ImportableMediaItem { + enum Load: Sendable { + case data(Data) // `loadTransferable` returns this + case failure // `loadTransferable` throws + case empty // `loadTransferable` returns nil + } + + let load: Load + let supportedContentTypes: [UTType] + + init(_ load: Load, contentTypes: [UTType] = [.jpeg]) { + self.load = load + self.supportedContentTypes = contentTypes + } + + func loadTransferable(type: T.Type) async throws -> T? { + switch load { + case .data(let data): return data as? T + case .failure: throw MockError.loadFailed + case .empty: return nil + } + } + + private enum MockError: Error { case loadFailed } +} + +#endif