Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions ios/Sources/GutenbergKit/Sources/EditorLocalization.swift
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ public enum EditorLocalizableString {
// MARK: - Media
case failedToInsertMedia
case failedToLoadSelectedMedia
case someSelectedMediaFailedToLoad(Int)
case failedToProcessCapturedMedia

// MARK: - Common
Expand Down Expand Up @@ -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"
Expand Down
20 changes: 20 additions & 0 deletions ios/Sources/GutenbergKit/Sources/Media/ImportableMediaItem.swift
Original file line number Diff line number Diff line change
@@ -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<T: Transferable>(type: T.Type) async throws -> T?
}

extension PhotosPickerItem: ImportableMediaItem {}
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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))
}
}
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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<AnyCancellable>()

Expand All @@ -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()
Expand Down Expand Up @@ -68,38 +71,57 @@ 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
}
processingTask = task
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 }
Expand Down
8 changes: 8 additions & 0 deletions ios/Tests/GutenbergKitTests/EditorLocalizationTests.swift
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down
114 changes: 114 additions & 0 deletions ios/Tests/GutenbergKitTests/Views/BlockInserterViewModelTests.swift
Original file line number Diff line number Diff line change
@@ -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<T: Transferable>(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
Loading