Fix GutenbergKit site media selection - #25857
Conversation
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 33581 | |
| Version | PR #25857 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | f8e7e19 | |
| Installation URL | 1fa5ki94et1v0 |
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 33581 | |
| Version | PR #25857 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | f8e7e19 | |
| Installation URL | 3gg9o16rduqsg |
589483f to
f8e7e19
Compare
dcalhoun
left a comment
There was a problem hiding this comment.
The changes look good to me. I performed regression testing in both GBK and GBM on Simple, Atomic, and self-hosted sites. I focused on selecting various file types from the Media Library. I did not encounter any issues.
| let request = NSFetchRequest<NSManagedObject>(entityName: "Media") | ||
| request.predicate = NSPredicate(format: "mediaID IN %@", mediaIds.map { NSNumber(value: $0) }) | ||
| request.predicate = NSPredicate( | ||
| format: "blog == %@ AND mediaID IN %@", |
There was a problem hiding this comment.
@crazytonyli question about the blog == %@ constraint this PR added to mapMediaIdsToMedia: Claude flagged it while working on a follow-up in #25866 and I'm not sure whether it's a real problem or not. Below is a summary. WDYT?
The fetch runs on
ContextManager.shared.mainContext, but the predicate constrains onblog, which is whatever was passed toGutenbergMediaPickerHelper's initializer:let context = ContextManager.shared.mainContext let request = NSFetchRequest<NSManagedObject>(entityName: "Media") request.predicate = NSPredicate( format: "blog == %@ AND mediaID IN %@", blog, mediaIds.map { NSNumber(value: $0) } )A Core Data predicate comparing a relationship against a managed object from a different context matches nothing, so if
blogever belongs to a context other thanmainContext, the fetch silently returns no rows and the picker opens with no preselection.Every path I traced passes a
mainContextblog, though I didn't audit this exhaustively — so this may well be unreachable by construction. Is it? IfGutenbergMediaPickerHelperis only ever handed amainContextblog by design, this is a non-issue and I'll leave it alone.I did try fetching on
blog.managedObjectContextlocally, but backed it out —SiteMediaCollectionViewController's fetched results controller is hardcoded tomainContextand compares selection by object identity, so that just moves the mismatch somewhere worse.
There was a problem hiding this comment.
I think we should be okay here. The theory is correct, and we can certainly make the code more robust by not using a Blog instance. But I believe the Blog instance used here is in the main context. It's a convention where the UI related types uses Core Data instance in the main context.
There was a problem hiding this comment.
Thank you for providing your perspective. Very helpful. 🙇🏻♂️


Description
Follow up review comments in #25855