Skip to content

feat: add MediaUploader, for a host that owns the whole upload - #628

Draft
jkmassel wants to merge 2 commits into
refactor/internal-media-clientfrom
feat/media-uploader-protocol
Draft

feat: add MediaUploader, for a host that owns the whole upload#628
jkmassel wants to merge 2 commits into
refactor/internal-media-clientfrom
feat/media-uploader-protocol

Conversation

@jkmassel

@jkmassel jkmassel commented Sep 5, 2026

Copy link
Copy Markdown
Contributor

Stacked on #627. Fifth of ten PRs splitting #621, and the heart of it. Purely additive — nothing existing changes shape.

What?

A MediaUploader protocol that takes over performing a media upload on the host's own stack, and owns its whole lifecycle: retries, recovery, and cleanup.

Why?

Performing a media upload — and retrying it — should be a single, all-or-nothing responsibility: either GutenbergKit performs the upload and owns its retries, or the host does. Both go to the same configured site; the only difference is who executes the requests.

MediaUploadDelegate.uploadFile doesn't offer that. A host performs the POST /wp/v2/media and returns the raw response it received — then the editor, reading that response, drives the post-process retries and the orphan cleanup behind it, through the WebView rather than the host's stack. A host that took over uploads to run them through its own networking still didn't own the retries. It also receives no form fields, so an attachment it uploads lands unattached to its post.

Neither is fixable while the hook returns a raw response, which is what the replacement changes.

How?

MediaUploader

upload(_:) returns the finished attachment or throws. There is no raw response left for the editor to retry behind it, so the host drives its own post-process recovery and force-deletes its own orphan on terminal failure.

MediaUpload

Carries the file, its metadata, the editor's non-file form fields (post, additionalData) and the request query (?_embed) — everything needed to reproduce a native request.

MediaUploadField

Fields are an ordered list rather than a dictionary, so repeated names (a field[] array) survive verbatim and in order. A named type rather than a tuple: tuples are not nominal, so a tuple-typed property would permanently block Equatable/Hashable/Codable synthesis on MediaUpload, and that is not fixable later without a source break for every host.

Precedence

uploadFile still works and is marked deprecated, pointing hosts at the replacement; an uploader takes precedence when both are set. #629 removes the old hook, so hosts get a migration window rather than a flag day.

This deliberately leaves one deprecation warning in GutenbergKit's own build, at the call site that supports the old hook. The marker exists to tell hosts to migrate, and supporting the hook until it is removed means calling it. It goes away with #629.

The admission gate

With an uploader set, the delegate's metadata gate can no longer decline a file. The gate exists to skip a temp copy for a file the delegate won't touch, but an uploader takes over delivery for every file, so passing through would silently bypass it.

Testing Instructions

Five new tests on iOS, three on Android: delivery, form fields and query, precedence over uploadFile, the declined-file case, and a terminal throw surfacing without GutenbergKit re-delivering.

  • swift test — host suite green
  • Android :Gutenberg:testDebugUnitTest green
  • iOS Simulator xcodebuild
  • SwiftLint + Detekt clean

MediaUploadServerTest crosses Detekt's LargeClass threshold; baselined rather than split, which is its own change.

@wpmobilebot

wpmobilebot commented Sep 5, 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/628")

Built from fadbd45

@jkmassel
jkmassel force-pushed the feat/media-uploader-protocol branch from 0ab9ee5 to fadbd45 Compare September 8, 2026 16:12
Performing a media upload — and retrying it — should be a single,
all-or-nothing responsibility: either GutenbergKit performs the upload and
owns its retries, or the host does. Both go to the same configured site;
the only difference is who executes the requests.

`MediaUploadDelegate.uploadFile` doesn't offer that. A host performs the
`POST /wp/v2/media` and returns the raw response it received — then the
editor, reading that response, drives the `post-process` retries and the
orphan cleanup behind it, through the WebView rather than the host's
stack. A host that took over uploads to run them through its own
networking still didn't own the retries. It also receives no form fields,
so an attachment it uploads lands unattached to its post.

Add `MediaUploader`, which owns the upload end to end:

- `upload(_:)` returns the finished attachment or throws. There is no raw
  response left for the editor to retry behind it, so the host drives its
  own post-process recovery and force-deletes its own orphan on terminal
  failure.
- It receives a `MediaUpload` carrying the file, its metadata, the
  editor's non-file form fields (`post`, additionalData) and the request
  query (`?_embed`) — everything needed to reproduce a native request.
- Fields are a `MediaUploadField` list rather than a dictionary, so
  repeated names (a `field[]` array) survive verbatim and in order.

Purely additive. `uploadFile` still works and is marked deprecated,
pointing hosts at the replacement; an uploader takes precedence when both
are set. GutenbergKit's own build keeps one deprecation warning at the
call site that supports the old hook — the marker exists to tell hosts to
migrate, and supporting the hook until it is removed means calling it.

With an uploader set, the delegate's metadata gate can no longer decline a
file: the gate exists to skip a temp copy for a file the delegate won't
touch, but an uploader takes over delivery for *every* file, so passing
through would silently bypass it. Covered on both platforms.

`MediaUploadServerTest` crosses Detekt's LargeClass threshold; baselined
rather than split, which is its own change.
…ound it

Adversarial review of the MediaUploader commit, with each finding argued by a
prosecutor and a defender before it was actioned. Nine survived; six were
refuted and are not addressed here.

## Correctness

- `processFile` ran on a file the delegate's metadata gate had declined. Widening
  the gate to `uploader != nil || delegateWantsFile` left `processFile` called
  unconditionally, so an image-only delegate paired with an uploader was handed
  the `.mov` it had just said it won't touch — breaking the contract
  `handlesFile` documents. `delegateWantsFile` is now carried into
  `processAndUpload` and gates `processFile`. With an uploader set the file is
  still delivered; it just skips processing on the way.

- iOS evaluated `handlesFile` eagerly while Android's `&&` short-circuited past
  it, so the same host saw one callback per upload on iOS and zero on Android.
  Android now binds it eagerly too: asked **exactly once per upload** on both.

- Android had no pre-flight cancellation check before handing work to the host
  uploader, where iOS has `Task.checkCancellation()`. Added
  `currentCoroutineContext().ensureActive()`, so a torn-down editor no longer
  starts an upload whose attachment nobody would clean up.

## Documentation

- The recovery recipe omitted `post-process`'s required `action` parameter. Core
  registers `action` as required, so a host following the doc verbatim would 400
  five times and then run the doc's *other* instruction —
  `DELETE /wp/v2/media/<id>?force=true` — destroying an attachment
  `wp_update_image_subsizes()` would have recovered.

- `mediaUploader`'s doc had been appended to `mediaUploadDelegate`'s `///` block,
  merging the two: `mediaUploadDelegate` shipped with no documentation and
  `mediaUploader` opened by describing a delegate. Confirmed with
  `swiftc -emit-symbol-graph` (`mediaUploadDelegate => None`); both now bind
  their own 11 lines.

- `formFields` and `deprecatedUploadFile` were inserted between
  `attachmentId(fromPath:)`'s doc and its declaration — merging into it on iOS,
  dropping it outright on Android — costing the "deliberately narrow, not a
  general REST proxy" rationale. Moved below their only caller, per AGENTS.md's
  call-order rule.

- `ReplaceWith("MediaUploader")` takes a replacement *expression*; applying the
  quick-fix drops all three arguments and leaves a type name where a
  `MediaUploadResponse?` was expected. Removed, with a note so it doesn't return.

## Tests

- Nothing pinned the Android gate or the uploader/deprecated-hook precedence:
  deleting `&& mediaUploader == null` or reordering the two delivery paths left
  the suite green. Three tests added; both mutations now fail.

- `DecliningDelegate` duplicated the pre-existing `DeclineByMetadataDelegate`
  minus its `processFileCalled` recorder — the one probe that catches the
  `processFile` bug above. Merged, and the declined-file test now asserts it.

## Hygiene

- Two locals named `uploader` shadowed the new `MediaUploader` property, silently
  (kotlinc has no diagnostic for it, detekt no rule). Renamed to `client`.
- `RecordingUploader`'s fixture carried no `title`, so the repo's only worked
  example of an uploader result was a body that trips `transformAttachment`.
@jkmassel
jkmassel force-pushed the feat/media-uploader-protocol branch from fadbd45 to 589f32f Compare September 9, 2026 00:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Android iOS [Type] Enhancement A suggestion for improvement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants