Share Gallery media library handling between GutenbergKit editors - #25855
Conversation
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 33543 | |
| Version | PR #25855 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 35fff93 | |
| Installation URL | 5gvfrmfa7740o |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 33543 | |
| Version | PR #25855 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 35fff93 | |
| Installation URL | 6jenff60g4ln8 |
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
dcalhoun
left a comment
dcalhoun
left a comment
There was a problem hiding this comment.
Changes look sound to me. The Media Library button now consistently functions as expected. 🚀
It took me too long to reproduce the original bug. I was unable to reproduce with xmlrpc-disabled.wpmt.co or xmlrpc-disabled-by-plugin.wpmt.co. I only reproduced with cpt.wpmt.co + installing the Hostinger Tools plugin to disable XML-RPC; I also needed to pull-to-refresh on the site's My Site view after adding it. I'm still puzzled as to why I cannot reproduce on the other sites.
| assert(Thread.isMainThread, "mapMediaIdsToMedia should only be called on the main thread") | ||
| let context = ContextManager.shared.mainContext | ||
| let request = NSFetchRequest<NSManagedObject>(entityName: "Media") | ||
| request.predicate = NSPredicate(format: "mediaID IN %@", mediaIds.map { NSNumber(value: $0) }) |
There was a problem hiding this comment.
Noting a pre-existing issue finding by Claude that we might address in a follow-up PR:
Should this fetch be scoped to the blog? mediaID is the remote attachment ID, so it's only unique within a site — with two sites signed in, ID 42 can exist on both. The resolved objects flow through setInitialSelection → selectedMedia, neither of which checks blog membership, so a Gallery block could pre-select (and on Done, return) the wrong site's media. There's also a uniqueKeysWithValues trap just below if two sites both have a matching ID cached.
Pre-existing, and it only bites on the multiple-selection path — but now that the helper holds a blog, "blog == %@ AND mediaID IN %@" would close it. Happy to leave for a follow-up if you'd rather keep this diff a pure refactor.
| caption: item.caption, | ||
| title: item.filename, | ||
| alt: item.alt, | ||
| metadata: [:] |
There was a problem hiding this comment.
Noting a pre-existing issue finding by Claude that we might address in a follow-up PR:
Is passing metadata: [:] here intentional? The metadata dict built just above with videopressGUID never gets used, so the GUID is dropped before it reaches the web editor — MediaInfo.metadata is an encoded field. No compiler warning since it is mutated inside the if.
Moved verbatim, so pre-existing — but worth flagging since this path now serves custom post types too. Looks like a one-word fix (metadata: metadata) if you want it here rather than a follow-up.
| case .any: | ||
| mediaType = mediaType | GutenbergMediaType.all.rawValue | ||
| @unknown default: | ||
| fatalError() |
There was a problem hiding this comment.
Noting a pre-existing issue finding by Claude that we might address in a follow-up PR:
Could this fatalError() be a softer fallback? Keeping the @unknown default makes sense — it's required for a non-frozen enum from GutenbergKit, and it's what stops a new library case from being a breaking change. It's just that trapping in the body turns that safety net into a crash: a dependency bump alone arms it, and the next media-library request for the new type takes the app down mid-edit with unsaved content. It'd hit both editors now that this lives in the base class.
break looks sufficient — contributing no flag leaves an empty GutenbergMediaType, and both MediaPickerMenu.MediaFilter.init? and PHPickerFilter.init? already return nil for anything that isn't exactly .image/.video, which MediaPickerMenu treats as "no filter." So it degrades to an unfiltered picker rather than an empty one. (.all returns nil there too, so it'd behave identically — break just reads more honestly.) Fine as a follow-up if you'd rather keep this diff a pure refactor.
|
I'll merge this PR for now. I will look into those issues later. |
4 participants
Description
Fixes https://linear.app/a8c/issue/CMM-2193
I targeted at the release branch because the changes are low-risk: code formatting and movements.