Conversation
The protocol no longer uploads anything — the previous commit removed `uploadFile`, leaving `handlesFile` and `processFile`. "UploadDelegate" now describes the one thing it can't do, and next to `MediaUploader` the two names read as variations on the same job rather than the two halves of a deliberate split. `MediaProcessor` says what is left: it transforms bytes, GutenbergKit delivers them. Mechanical throughout — the property becomes `mediaProcessor`, the server parameter `processor`, the file `MediaHandlers.swift` (it holds both protocols now), and Android's demo `DemoMediaProcessor`. Prose follows the types. The `weak_delegate` suppression added when the property became strong goes away with the name: the rule was arguably right that a strongly-held "delegate" is a smell, and the answer was that this was never a delegate. BREAKING CHANGE: `mediaUploadDelegate` is now `mediaProcessor`, and `MediaUploadDelegate` is `MediaProcessor`. Conformances need no changes beyond the name.
`MediaProcessor` and `MediaUploader` were both `AnyObject`-bound, and `EditorViewController` holds both strongly. A conformer that holds the view controller back therefore closes a retain cycle ARC cannot break: the editor is never freed, so `deinit` never runs, so `uploadServer.stop()` — its only caller — never runs either, and a bound loopback `NWListener` outlives the editing session. Nothing needed class-boundness. There is no `weak`, `===`, or `ObjectIdentifier` use against either protocol anywhere in the tree, and every existing conformer is a class, which conforms unchanged. Dropping the requirement lets a host conform with a value type capturing only what the work needs — the shape that avoids the cycle, and the one a class-bound `Delegate` discouraged. This does not make the cycle impossible: a struct that stores the view controller cycles just the same. The docs say so rather than implying the type system settles it.
…irst
`countServerStarted` resolved its name as `processor.map { … } ?? uploader.map { … }`,
so with both supplied it always named the processor. The retainer is as likely to be
the uploader — and after the class bound came off `MediaProcessor`, the processor it
names may be a value type holding nothing at all, which is the one shape that provably
cannot close the cycle the fault is reporting.
A host following the docs hits this on the recommended shape: a leaf processor for the
transform plus an uploader on the coordinator that owns the editor. The fault named the
leaf, so the reader audits an object with no stored references, finds nothing, and
concludes the census is broken.
Names every handler that was supplied, and softens the assertion from "is its own media
handler" to "is one of its own media handlers" — with two names it is a candidate list,
not an accusation.
DEBUG-only, and still behind the `count >= liveServerLeakThreshold` guard, so
`String(describing: type(of:))` stays off the start path.
…loader `startUploadServer()` asks "did the host supply a media handler" twice — once before starting, once after the bind returns, because `stopMediaHandling()` can land while that `await` is suspended. The two reads had drifted. #628 widened the first to `delegate != nil || uploader != nil` and left the second checking the delegate alone. So a host that passed only a `mediaUploader` cleared the entry check, bound a loopback listener, then failed the post-bind check and stopped the server it had just started. `uploadServer` stayed nil, `buildEditorConfiguration` advertised `nativeUploadPort: nil`, and `api-fetch.js` fell through to the plain WebView path. The host's `upload(_:)` was **never called, for any file** — no error, no log. Uploads appeared to work; they just never reached the host's background session, offline queue, or retry policy, which is the whole reason to supply an uploader. Both reads now go through one `hasMediaHandling`, so they cannot disagree again. That is the actual defect — two hand-maintained copies of one predicate — and it is the same failure `MediaServerCredentials` was extracted for, where a check "diverged silently between iOS and Android once". `uploadServer` and `startUploadServer()` become internal so the suite can reach them. The test is parameterized over uploader-only, processor-only and both. Mutation-checked: restoring the old post-bind guard fails **only** the uploader-only case, which is the regression and nothing else. Android already pinned this gate (`GutenbergViewUploadServerTest`, "the upload server starts for an uploader with no delegate"); iOS had no equivalent, which is why the drift survived three commits green.
The rename swept the helper types and left the names around them. 101 sites across four files: iOS test functions and `@Test` display strings, Kotlin backtick names, `let delegate = ProcessOnlyProcessor()` bindings that contradicted themselves on one line, `weakDelegate`, and a `// MARK: - Upload with delegate` header over code the production file had already renamed to `// MARK: - Processor Pipeline`. These are the strings CI prints. A red build named `retainsDelegateForServerLifetime` or `processes with the delegate, then delivers through the internal client` for a codebase where no symbol contains the word — Kotlin backticks are literally the JUnit report strings — so the first move on a failure was to grep for an API this stack deleted. Safe as a plain substring replacement: none of the four files reference a genuine delegate. `HttpServerDelegate` and `EditorViewControllerDelegate` live in other test files and are untouched. Two names would have read as stutters after a mechanical pass, so they say what the test does instead: `processesThenDelivers` and `processorRunsForUploader`. Test counts are unchanged — 590 iOS, and Android green on `--rerun-tasks` — so this renames tests rather than adding or dropping any. Note it does reset Buildkite Test Analytics history for the renamed cases, which is the deliberate cost.
Dropping `: AnyObject` is what the second commit here exists to deliver, and nothing
exercised it — all eleven conformers in the tree were classes, so the boxed-existential
path was never walked: copied into `UploadContext`, captured by the `@Sendable` handler
closure, read again at `processFile`.
Re-imposing the class bound, or breaking that path, would have compiled and passed
green and surfaced only in a host's build. It now fails at compile time:
error: non-class type 'ValueTypeProcessor' cannot conform to class protocol 'MediaProcessor'
`ValueTypeProcessor` is `Sendable` without `@unchecked` — also the point, since that is
the shape the protocol's documentation now recommends and the escape hatch it describes.
The assertions run through `MockInternalMediaClient`'s recorded metadata rather than
state on the processor, because a `struct` witnessing a non-mutating requirement cannot
record anything. The transcoded mimeType and filename reaching the client could only
come from `processFile` having actually run, so this pins invocation, not just storage.
jkmassel
added
[Type] Breaking Change
4 tasks
jkmassel
added this pull request to stack #690
September 17, 2026 18:33
XCFramework BuildThis PR's XCFramework is available for testing. Add the following to your .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/685")Built from 42bdb8d |
This was referenced Sep 17, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Stacked on #684. Sixth of nine PRs splitting #621.
Two breaking changes — the rename and the class-bound relaxation that follows from it — plus two
fix(ios):commits an adversarial review turned up. Both fixes are real behavior changes, so they are called out below rather than left to be found in the diff.What?
MediaUploadDelegate→MediaProcessor,mediaUploadDelegate→mediaProcessor, the server parameter →processor, the file →MediaHandlers.swift, and Android's demo →DemoMediaProcessor.Why?
The protocol no longer uploads anything — #684 removed
uploadFile, leavinghandlesFileandprocessFile. "UploadDelegate" now describes the one thing it can't do, and next toMediaUploaderthe two names read as variations on the same job rather than the two halves of a deliberate split.MediaProcessorsays what is left: it transforms bytes, GutenbergKit delivers them.How?
A rename sweep, ~190 sites across both platforms including the demo apps,
docs/integration.md, and the media test suites' own vocabulary — test function names,@Testdisplay strings and Kotlin backtick names are what CI prints, and they named a type this stack deletes. Prose in doc comments follows the types, including the property abstracts Xcode Quick Help and Android Studio hover actually render.MediaHandlers.swiftbecause the file holds both protocols now.The
weak_delegatesuppression added in #625 goes away with the name: the rule was arguably right that a strongly-held "delegate" is a smell, and the answer was that this was never a delegate. The rule keys on the identifier suffix, so it cannot fire onmediaProcessor— the suppression and the paragraph arguing with it are both dead, and nothing in CI would ever have said so.Following that through:
MediaProcessorandMediaUploaderalso drop: AnyObject. Nothing needed class-boundness — there is noweak,===, orObjectIdentifieruse against either protocol anywhere in the tree — andEditorViewControllerholds both strongly, so a class-bound protocol was quietly steering hosts toward a conformer that holds the view controller back and closes a retain cycle ARC cannot break. Dropping it lets a host conform with a value type capturing only what the work needs. Every existing conformer is a class and is unaffected.Two fixes ride along, each in its own
fix(ios):commit rather than buried in the rename.An uploader-only host never got an upload server.
startUploadServer()asks "did the host supply a handler" twice — once before starting, once after the bind returns, becausestopMediaHandling()can land across thatawait. The two reads had drifted: #683 widened the first to covermediaUploaderand left the second checking the processor alone. A host passing only an uploader bound a listener, failed the second check, and stopped the server it had just started — sonativeUploadPortwas nil,api-fetch.jsfell through to the WebView path, andupload(_:)was never called for any file, silently. Both reads now go through onehasMediaHandling. Android already pinned this gate; iOS had no equivalent, which is why it survived three commits green.The leak census named the wrong object. It resolved as
processor ?? uploader, so with both set it always accused the processor — which, after the class bound comes off, may be a value type holding nothing at all. It now names every handler supplied.Testing Instructions
xcodebuild test— 585 + 395 tests green:Gutenberg:testgreen (re-run with--rerun-tasks); Android and iOS demo apps compiledocs/integration.mdswept —git grep MediaUploadDelegateover the tree now returns nothinguploadServerStartsForAnyHandlertest mutation-checked — restoring the old post-bind guard fails only the uploader-only casevalueTypeProcessorRunstest mutation-checked — re-imposing: AnyObjectfails to compile (non-class type 'ValueTypeProcessor' cannot conform to class protocol)Breaking change
mediaUploadDelegateis nowmediaProcessor, andMediaUploadDelegateisMediaProcessor. Conformances need no changes beyond the name.MediaProcessorandMediaUploaderare also no longerAnyObject-bound. Class conformers are unaffected; a host that declared its ownweakreference to one of these existentials would need to hold it strongly instead.Note this relaxes a constraint rather than fixing the cycle outright — a
structthat stores theEditorViewControllercycles just the same. The doc comments say so rather than implying the type system settles it.A value-type conformer also carries a caveat a class did not: it is copied when you hand it to the initializer, and the editor holds that copy for its lifetime. Mutating your own instance afterwards changes nothing the editor will run, and there is no way to swap in a new value — the property is
private(set), so a different processor means a different editor. If you need settings the host can change mid-session, read them insideprocessFilethrough a reference the conformer captures. Documented on both protocols.