From c21d9b99da4b4bcf385073c36b05c69e2d427de7 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 4 Sep 2026 17:18:40 -0600 Subject: [PATCH 1/2] fix(ios): import picker media per-item so one failure doesn't drop the rest MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The native block inserter imported a multi-photo selection in a single do/catch loop, so the first item that failed to import aborted the whole selection — every item after it was skipped, and no error was shown unless nothing imported at all. A failure alongside a success was dropped silently. Import each item independently: insert the ones that succeed and surface an error for any that fail, including a partial failure (which now names how many items were skipped). A total failure keeps the existing message and leaves the inserter open to retry. Android's native block picker routes picked media through inert callbacks today, so there's no equivalent import loop to change. --- .../Sources/EditorLocalization.swift | 5 +++ .../BlockInserter/BlockInserterView.swift | 34 +++++++++++++-- .../BlockInserterViewModel.swift | 43 +++++++++++++------ .../EditorLocalizationTests.swift | 8 ++++ .../Views/BlockInserterViewModelTests.swift | 41 ++++++++++++++++++ 5 files changed, 115 insertions(+), 16 deletions(-) create mode 100644 ios/Tests/GutenbergKitTests/Views/BlockInserterViewModelTests.swift 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/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..74bda5933 100644 --- a/ios/Sources/GutenbergKit/Sources/Views/BlockInserter/BlockInserterViewModel.swift +++ b/ios/Sources/GutenbergKit/Sources/Views/BlockInserter/BlockInserterViewModel.swift @@ -74,25 +74,30 @@ class BlockInserterViewModel: ObservableObject { 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 +105,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..9f228723e --- /dev/null +++ b/ios/Tests/GutenbergKitTests/Views/BlockInserterViewModelTests.swift @@ -0,0 +1,41 @@ +import Foundation +import Testing +@testable import GutenbergKit + +#if canImport(UIKit) + +/// Covers the decision `processSelectedPhotosPickerItems` makes after importing +/// a selection: which failures surface an alert, and which message. The import +/// loop itself takes `PhotosPickerItem`s, which can't be constructed in a test, +/// so the outcome logic is factored into `importError` and verified here. +@MainActor +@Suite("BlockInserterViewModel media import error") +struct BlockInserterViewModelTests { + + @Test("Every item imported: no alert") + func allSucceededProducesNoError() { + #expect(BlockInserterViewModel.importError(failureCount: 0, successCount: 3) == nil) + #expect(BlockInserterViewModel.importError(failureCount: 0, successCount: 0) == nil) + } + + @Test("Every item failed: surfaces an alert") + func totalFailureSurfacesError() { + #expect(BlockInserterViewModel.importError(failureCount: 2, successCount: 0) != nil) + } + + // The bug this fixes: a failure alongside a success used to be dropped + // silently because the alert only showed when nothing imported. + @Test("Partial failure still surfaces an alert") + func partialFailureSurfacesError() { + #expect(BlockInserterViewModel.importError(failureCount: 1, successCount: 2) != 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) + } +} + +#endif From 1c1df6e9769bd3fa57227cf6edf8f68269055534 Mon Sep 17 00:00:00 2001 From: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Date: Fri, 4 Sep 2026 19:41:55 -0600 Subject: [PATCH 2/2] test(ios): cover the block-inserter import loop via a mockable item protocol MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit processSelectedPhotosPickerItems' import loop couldn't be tested because PhotosPickerItem can't be constructed in a test — the earlier tests only exercised the error-message helper in isolation. Add an ImportableMediaItem protocol that PhotosPickerItem conforms to via an empty extension. MediaFileManager.import and processSelectedPhotosPickerItems now take `some ImportableMediaItem`, and the view model's MediaFileManager is injectable. Tests drive the real loop with a mock item and a temp-directory file manager, covering partial failure (successes kept, error surfaced), total failure, and full success. No production behavior change. --- .../Sources/Media/ImportableMediaItem.swift | 20 ++++ .../Sources/Media/MediaFileManager.swift | 5 +- .../BlockInserterViewModel.swift | 9 +- .../Views/BlockInserterViewModelTests.swift | 107 +++++++++++++++--- 4 files changed, 120 insertions(+), 21 deletions(-) create mode 100644 ios/Sources/GutenbergKit/Sources/Media/ImportableMediaItem.swift 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/BlockInserterViewModel.swift b/ios/Sources/GutenbergKit/Sources/Views/BlockInserter/BlockInserterViewModel.swift index 74bda5933..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,7 +71,7 @@ class BlockInserterViewModel: ObservableObject { // MARK: - Media Processing - func processSelectedPhotosPickerItems(_ items: [PhotosPickerItem]) async -> [MediaInfo] { + func processSelectedPhotosPickerItems(_ items: [some ImportableMediaItem]) async -> [MediaInfo] { isProcessingMedia = true defer { isProcessingMedia = false } diff --git a/ios/Tests/GutenbergKitTests/Views/BlockInserterViewModelTests.swift b/ios/Tests/GutenbergKitTests/Views/BlockInserterViewModelTests.swift index 9f228723e..9f49485ff 100644 --- a/ios/Tests/GutenbergKitTests/Views/BlockInserterViewModelTests.swift +++ b/ios/Tests/GutenbergKitTests/Views/BlockInserterViewModelTests.swift @@ -1,33 +1,78 @@ import Foundation +import SwiftUI import Testing +import UniformTypeIdentifiers @testable import GutenbergKit #if canImport(UIKit) -/// Covers the decision `processSelectedPhotosPickerItems` makes after importing -/// a selection: which failures surface an alert, and which message. The import -/// loop itself takes `PhotosPickerItem`s, which can't be constructed in a test, -/// so the outcome logic is factored into `importError` and verified here. +/// 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 error") +@Suite("BlockInserterViewModel media import") struct BlockInserterViewModelTests { - @Test("Every item imported: no alert") - func allSucceededProducesNoError() { - #expect(BlockInserterViewModel.importError(failureCount: 0, successCount: 3) == nil) - #expect(BlockInserterViewModel.importError(failureCount: 0, successCount: 0) == nil) + 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) } - @Test("Every item failed: surfaces an alert") - func totalFailureSurfacesError() { - #expect(BlockInserterViewModel.importError(failureCount: 2, successCount: 0) != nil) + // 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) } - // The bug this fixes: a failure alongside a success used to be dropped - // silently because the alert only showed when nothing imported. - @Test("Partial failure still surfaces an alert") - func partialFailureSurfacesError() { - #expect(BlockInserterViewModel.importError(failureCount: 1, successCount: 2) != nil) + @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") @@ -38,4 +83,32 @@ struct BlockInserterViewModelTests { } } +/// 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