Skip to content

fix(ios): import picker media per-item so one failure doesn't drop the rest - #623

Draft
jkmassel wants to merge 2 commits into
trunkfrom
jkmassel/inserter-image-picker-issue
Draft

fix(ios): import picker media per-item so one failure doesn't drop the rest#623
jkmassel wants to merge 2 commits into
trunkfrom
jkmassel/inserter-image-picker-issue

Conversation

@jkmassel

@jkmassel jkmassel commented Sep 4, 2026

Copy link
Copy Markdown
Contributor

Fixes a bug where importing a multi-photo selection in the native block inserter dropped every item after the first one that failed to import — and, when a failure landed alongside a success, surfaced no error at all.

Summary

  • The block inserter's photo picker imported a 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.
  • The error notice was gated on results.isEmpty, so a failure alongside a success was dropped silently.
  • Now each item imports independently: the ones that succeed are inserted, and any failure is surfaced — including a partial failure, which now names how many items were skipped.

Root Cause

BlockInserterViewModel.processSelectedPhotosPickerItems wrapped the entire for loop in one do/catch. A throw from MediaFileManager.import — e.g. a photo not fully downloaded from iCloud, or one too large to materialize into memory — jumped straight to the catch, so items after the failure were never attempted. The if results.isEmpty gate then meant a partial failure showed nothing.

Fix

  • BlockInserterViewModel.swift: import each item in its own do/catch, counting failures. After the loop, resolve the alert through a new testable importError(failureCount:successCount:) — the existing "nothing could be loaded" copy when everything failed, or a new count-aware message on a partial failure.
  • BlockInserterView.swift: on any failure, insert whatever succeeded and keep the inserter open so its alert is visible. Acknowledging the alert closes the inserter only on a partial success; a total failure leaves it open to retry — unchanged from today.
  • EditorLocalization.swift: add someSelectedMediaFailedToLoad(Int) with singular/plural defaults.

Testability

PhotosPickerItem can't be constructed in a test, so the import loop had no direct coverage. A new ImportableMediaItem protocol captures the slice of PhotosPickerItem that MediaFileManager.import uses (supportedContentTypes, loadTransferable); PhotosPickerItem conforms via an empty extension, and import / processSelectedPhotosPickerItems now take some ImportableMediaItem. The view model's MediaFileManager is injectable, so tests drive the real loop with a mock item and a temp-directory file manager. No production behavior change.

Test Plan

  • make lint-swift clean on the changed files.
  • Swift package builds for the iOS Simulator; unit tests pass. BlockInserterViewModelTests drives the real import loop — partial failure (successes kept, error surfaced), total failure, and full success — plus EditorLocalizationTests.defaultsPluralizeSkippedMediaCounts.
  • Device: pick several photos where one isn't fully downloaded from iCloud → the others insert and an alert names the skipped count.
  • Device: pick a single unloadable photo → alert shown, inserter stays open to retry.

Notes

  • iOS only. Android's native block picker routes picked media through inert callbacks today (editor hand-off is a documented follow-up), so there's no equivalent import loop to change.
  • Separate from the WordPress-iOS ItemProviderMediaExporter upload hang: that's the host's Media Library picker, reached via openMediaLibrary → the host delegate — a different code path from this inserter import.

🤖 Generated with Claude Code

…e rest

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.
@jkmassel jkmassel added [Type] Bug An existing feature does not function as intended iOS labels Sep 4, 2026
@jkmassel jkmassel self-assigned this Sep 4, 2026
@wpmobilebot

wpmobilebot commented Sep 4, 2026

Copy link
Copy Markdown

XCFramework Build

This PR's XCFramework is available for testing. Add the following to your Package.swift:

.package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/623")

Built from 1c1df6e

@jkmassel
jkmassel marked this pull request as draft September 5, 2026 01:28
…rotocol

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

iOS [Type] Bug An existing feature does not function as intended

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants