Conversation
jkmassel
added
[Type] Breaking Change
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/630")Built from 42bdb8d |
f6a9bac to
2a835c1
Compare
3910919 to
43806eb
Compare
43806eb to
44d6286
Compare
ae03329 to
382dd01
Compare
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.
382dd01 to
30ff7d9
Compare
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
changed the title
dcalhoun
left a comment
dcalhoun
left a comment
There was a problem hiding this comment.
Changes look good. I tested uploads using the Demo app. I captured a few findings that are worth considering.
| // properties are strong — so there's no released-before-load case to guard | ||
| // against; they live as long as it does. | ||
| guard mediaUploadDelegate != nil || mediaUploader != nil else { | ||
| guard hasMediaHandling else { |
There was a problem hiding this comment.
This appears technically correct, but it may not be worth addressing given current use cases.
Finding from Claude:
The handler gate widened here, but the credentials guard just below (606-619) did not — it still returns when siteApiRoot/authHeader are unusable. An uploader-only host has no reason to supply either (MediaUploader's own doc promises GutenbergKit stays out of the network entirely for media), so it clears this guard and dies on the next one: server down, nativeUploadPort: nil, silent fallback to the WebView path. That is the failure this PR fixes, one guard lower — and both returns are silent, so nothing is logged.
Only InternalMediaClient needs those credentials, and MediaUploadServer.start already takes it as optional:
let internalClient = MediaServerCredentials.areUsable(
siteApiRoot: configuration.siteApiRoot,
authHeader: configuration.authHeader
) ? InternalMediaClient(...) : nil
guard internalClient != nil || mediaUploader != nil else { return }handleDelete already guards on a nil client, so the delete path degrades rather than breaking.
|
|
||
| /// Default implementations. | ||
| extension MediaUploadDelegate { | ||
| extension MediaProcessor { |
There was a problem hiding this comment.
May not be worthwhile if we are unconcerned with this edge case.
Finding from Claude:
The processFile default predates this PR, but the new protocol doc now leans on it: "Either mistake resolves to the no-op default below instead of failing to build."
Could we delete that default (125-127) so it does fail to build? A conformer that doesn't process files has nothing to do, and the silent miss isn't free at runtime: handlesFile still defaults to true, so the server materializes the whole part to a temp file (MediaUploadServer.swift:219-225), calls the no-op, gets .original, and throws the copy away — a full-size write and delete on every upload for a processor that never runs.
Dropping it turns both the mutating witness and the mistyped label into compile errors, which is the outcome the doc is asking prose to achieve.
| mediaProcessor != nil || mediaUploader != nil | ||
| } | ||
|
|
||
| private(set) var uploadServer: MediaUploadServer? |
There was a problem hiding this comment.
May not be worth refactoring if we are less concerned about this edge case or the now public attributes.
Finding from Claude:
This and startUploadServer() (597) both dropped private for the new tests. A second in-module call to startUploadServer() now overwrites uploadServer at 644 without stopping the previous one — stranding a bound loopback listener, which is the leak the census at MediaUploadServer.swift:105 exists to catch.
buildEditorConfiguration's nativeUploadPort (578) already reflects whether the server came up, and it is what the page actually consumes. Could the new tests assert on that instead and leave both members private?
| @@ -99,3 +145,29 @@ private final class StandaloneDelegate: MediaUploadDelegate { | |||
| } | |||
|
|
|||
| #endif | |||
There was a problem hiding this comment.
Finding from Claude:
InertUploader, canBindUploadServer, and UnsafeSendableBox land after this #endif, while their only call sites are the tests inside it.
Package.swift declares macOS too, so on that build the tests vanish and these three compile as dead code. And if MediaUploader/MediaUploadServer ever move under a UIKit guard — as EditorViewController and MediaServerCredentials already are — the macOS test target stops compiling on code that was meant to be excluded. Move the #endif to the end of the file?
| /// A processor that produces a new file with changed metadata (e.g. a transcode). | ||
| /// A value-type processor. `struct`, and `Sendable` without `@unchecked` — both are the | ||
| /// point: this is the shape ``MediaProcessor``'s documentation now recommends. | ||
| private struct ValueTypeProcessor: MediaProcessor { |
There was a problem hiding this comment.
Finding from Claude:
ValueTypeProcessor inherited ResizingProcessor's doc line, so it now opens with two unrelated sentences and ResizingProcessor (1140) has none.
| /// A processor that produces a new file with changed metadata (e.g. a transcode). | |
| /// A value-type processor. `struct`, and `Sendable` without `@unchecked` — both are the | |
| /// point: this is the shape ``MediaProcessor``'s documentation now recommends. | |
| private struct ValueTypeProcessor: MediaProcessor { | |
| /// A value-type processor. `struct`, and `Sendable` without `@unchecked` — both are the | |
| /// point: this is the shape ``MediaProcessor``'s documentation now recommends. | |
| private struct ValueTypeProcessor: MediaProcessor { |
The transcode line belongs back above ResizingProcessor.
| /// Two requirements a `struct` makes easy to miss, both of which compile silently: | ||
| /// `processFile` cannot be `mutating` (a `mutating` witness does not satisfy a | ||
| /// non-mutating requirement), and its argument labels must match exactly. Either | ||
| /// mistake resolves to the no-op default below instead of failing to build, leaving a | ||
| /// processor that is never called. That | ||
| /// reference must itself be `Sendable` — an actor, or a class made safe with a lock — | ||
| /// because this protocol is `Sendable` and a `struct` conformer's stored properties | ||
| /// inherit that requirement. | ||
| /// | ||
| /// Two requirements a `struct` makes easy to miss, both of which compile silently: | ||
| /// `processFile` cannot be `mutating` (a `mutating` witness does not satisfy a | ||
| /// non-mutating requirement), and its argument labels must match exactly. Either | ||
| /// mistake resolves to the no-op default below instead of failing to build, leaving a | ||
| /// processor that is never called. |
There was a problem hiding this comment.
There appears to be erroneous duplication between these comments.
Atop that, all of the comments in this PR feel verbose. Can we make them more concise while retaining the important pieces? In general, I feel comments should only explain "why" for code that is unclear, we should avoid letting agents explain the reasoning for how they arrived at the code.
Maybe have an agent reassess and update comments with this guidance? WDYT?
3 participants
Stacked on #629. Seventh of ten 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 — #629 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: #628 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.