Add Conditional editor theme styles#25160
Conversation
Generated by 🚫 Danger |
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 30792 | |
| Version | PR #25160 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 0727cc7 | |
| Installation URL | 191ui6avga14o |
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 30792 | |
| Version | PR #25160 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 0727cc7 | |
| Installation URL | 3pd81s7s7dfi0 |
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
| async let currentUserTask = try await api.users.retrieveMeWithEditContext().data | ||
| async let activeThemeTask = try await api.themes.listWithEditContext( | ||
| params: ThemeListParams(status: .active) | ||
| ).data.first(where: { $0.status == .active }) |
There was a problem hiding this comment.
If one of the requests fails, the whole loadSiteInfoTask fails, which means we won't get any of the "site info". That's probably not something we'd want here?
| case .blockTheme: isBlockTheme | ||
| case .plugins: apiRoot.hasRoute(route: "/wp/v2/plugins") | ||
| case .applicationPasswordExtras: apiRoot.hasRoute(route: "/application-password-extras/v1/admin-ajax") | ||
| } |
There was a problem hiding this comment.
Nitpick: using the urlResolver property in WordPressAPI to find the URL should work on both self-hosted and WP.com sites, so we can merge the two similar branches into one.
| let key = blog.locallyUniqueId + "-capabilities" | ||
|
|
||
| // Don't allow more than one concurrent invalidation | ||
| if self.prefetchTasks[key] != nil { |
There was a problem hiding this comment.
Unrelated to this PR: you can use OSAllocatedUnfairLock in LockingHashMap, which is Sendable, to get rid of the @unchecked declaration, which means it's impossible (as long as we continue to have the Swift compiler check in place) to forget use the lock.withLock.
There was a problem hiding this comment.
The key is AnyHashable which isn't Sendable so I'm not sure if there's an easy way to do this?
We could drop LockingHashMap once our minimum iOS target is 18.0 in favour of the Swift Mutex type though.
There was a problem hiding this comment.
You could keep a strong-typed key and use OSAllocatedUnfairLock<[CacheKey: CacheResult]>. Again, not really related to this PR, probably something for future PRs if needed.
I'm looking into GBK's custom post type support, and I think I'll need to update how this cache works (I probably need to use (Blog, PostType) as the cache key).
|
Noting some early testing results... Good:
Bad:
Theme styles settings reset
theme-styles-settings.MP4 |
cfc334e to
bc417dc
Compare
|
Version |
|
I updated the PR description to link this to CMM-1163. Also, should we target the |
74db783 to
796e829
Compare
Yep, I've rebased atop that branch. |
| case .success(let user): return user | ||
| case .failure(let error): | ||
| self.loadCurrentUserTask = newCurrentUserTask() | ||
| throw error |
There was a problem hiding this comment.
It might be better to await self.loadCurrentUserTask.value? This throw returns the preivous fetch attempt's error. I don't think that's something the caller wants when calling try await client.fetchCurrentUser()?
There was a problem hiding this comment.
Hmm I think you're probably right – I've taken another run at it in be373d5.
The way it'll work now is if if the first run fails, it'll drop the error but subsequent fetches will propagate it. I think this is an ok trade-off?
| case .blockEditorSettings: apiRoot.hasRoute(route: "/wp-block-editor/v1/sites/\(siteId)/settings") | ||
| case .blockTheme: isBlockTheme | ||
| case .plugins: apiRoot.hasRoute(route: "/wp/v2/plugins") | ||
| case .applicationPasswordExtras: apiRoot.hasRoute(route: "/application-password-extras/v1/admin-ajax") |
There was a problem hiding this comment.
Do the plugins and applicationPasswordExtras cases need /sites/\(siteId)/?
BTW, I'm not sure if this comment's suggestion would work, maybe worth give it ago https://github.com/wordpress-mobile/WordPress-iOS/pull/25160/changes#r2718516922
There was a problem hiding this comment.
I'm not sure they do, but this works and I'd like to do a separate PR that adopts the wprs path management.
There was a problem hiding this comment.
Yes, I believe the applicationPasswordExtras endpoint requires the site ID if fetched from the WP.com API. Presumably the plugins would need it also.
| // These tasks need to be manually restated here because we can't use the task constructors | ||
| self.loadSiteInfoTask = Task { try await api.apiRoot.get().data } | ||
| self.loadCurrentUserTask = Task { try await api.users.retrieveMeWithEditContext().data } | ||
| self.loadActiveThemeTask = Task { try await api.themes.listWithEditContext(params: ThemeListParams(status: .active)).data.first } |
There was a problem hiding this comment.
Call Task { self.refresh() } instead?
There was a problem hiding this comment.
That still violates task isolation – you can't call self from inside any of these tasks.
| let key = blog.locallyUniqueId + "-capabilities" | ||
|
|
||
| // Don't allow more than one concurrent invalidation | ||
| if self.prefetchTasks[key] != nil { |
There was a problem hiding this comment.
You could keep a strong-typed key and use OSAllocatedUnfairLock<[CacheKey: CacheResult]>. Again, not really related to this PR, probably something for future PRs if needed.
I'm looking into GBK's custom post type support, and I think I'll need to update how this cache works (I probably need to use (Blog, PostType) as the cache key).
| } | ||
|
|
||
| // Refresh editor capabilities | ||
| EditorDependencyManager.shared.fetchEditorCapabilities(for: blog) |
There was a problem hiding this comment.
Does this code need the if RemoteFeatureFlag.newGutenberg.enabled(), like the one above?
|
|
||
| @objc public func swiftRefreshSettings() { | ||
| // Refresh editor capabilities | ||
| EditorDependencyManager.shared.fetchEditorCapabilities(for: self.blog) |
There was a problem hiding this comment.
Does this code need if RemoteFeatureFlag.newGutenberg.enabled()?
There was a problem hiding this comment.
I'm not sure it's strictly needed but doesn't hurt – added in f22e21f
dcalhoun
left a comment
There was a problem hiding this comment.
The latest iteration tested well for me. I did not encounter any oddities while testing WP.com Simple, WoW, and self-hosted sites. I tested variations with and without the Gutenberg plugin. I tested GutenbergKit, Gutenberg Mobile, and Aztec.
WordPress/Classes/ViewRelated/Blog/Site Settings/SiteSettingsViewController.m
Outdated
Show resolved
Hide resolved
| case .blockEditorSettings: apiRoot.hasRoute(route: "/wp-block-editor/v1/sites/\(siteId)/settings") | ||
| case .blockTheme: isBlockTheme | ||
| case .plugins: apiRoot.hasRoute(route: "/wp/v2/plugins") | ||
| case .applicationPasswordExtras: apiRoot.hasRoute(route: "/application-password-extras/v1/admin-ajax") |
There was a problem hiding this comment.
Yes, I believe the applicationPasswordExtras endpoint requires the site ID if fetched from the WP.com API. Presumably the plugins would need it also.
…iewController.m Co-authored-by: David Calhoun <github@davidcalhoun.me>
Sites with plugins shouldn't be going through the WP.com APIs, we should be talking to those sites directly. Is the Application Password extras endpoint live? I'd suggest addressing this in a future PR either way to get this landed though. |
I agree with both points. I intended to add additional context, not imply we need to address it now. No, the Application Passwords Extras endpoint is not yet live. |
|
* Use API to determine theme style support * Add Tests * Split up WordPressClient cache fetches * Fix a warmup call introduced in the rebase * Fix theme styles persistence * Only fetch editor capabilities if new gutenberg is enabled * Address suggestions * Default to `true` for `isThemeStylesEnabled` * Update WordPress/Classes/ViewRelated/Blog/Site Settings/SiteSettingsViewController.m Co-authored-by: David Calhoun <github@davidcalhoun.me> --------- Co-authored-by: David Calhoun <github@davidcalhoun.me>
* Exclude sensitive headers from Pulse logs (#25213) * Add Conditional editor theme styles (#25160) * Use API to determine theme style support * Add Tests * Split up WordPressClient cache fetches * Fix a warmup call introduced in the rebase * Fix theme styles persistence * Only fetch editor capabilities if new gutenberg is enabled * Address suggestions * Default to `true` for `isThemeStylesEnabled` * Update WordPress/Classes/ViewRelated/Blog/Site Settings/SiteSettingsViewController.m Co-authored-by: David Calhoun <github@davidcalhoun.me> --------- Co-authored-by: David Calhoun <github@davidcalhoun.me> * Update strings for localization * Bump version number --------- Co-authored-by: Tony Li <tony.li@automattic.com> Co-authored-by: Jeremy Massel <1123407+jkmassel@users.noreply.github.com> Co-authored-by: David Calhoun <github@davidcalhoun.me>





Summary
Fix CMM-1163.
WordPressClientfeature detection and caching behaviour – there's more to do here, but that'll be a future PR.Changes
WordPressClientFeature DetectionWordPressClientAPIprotocol to abstract WordPress API access for testingsupports(_:forSiteId:)method to check API route availability for featuresTaskto avoid redundant API calls. In the future, we'll want a way to invalidate this, but we'll do that later.Editor Capability Fetching
EditorDependencyManagernow fetches and caches editor capabilities on My Site loadGutenbergSettingsstores feature support per-blog usingWordPressClient.Feature.stringValueas stable keysTesting Instructions
cpt.wpmt.coto the app. Ensure the editor loads as expected. Ensure that theme styles toggle cannot be activated in Site Settings.jetpack.wpmt.coto the app. Ensure the editor loads. Ensure that theme styles toggle can be activated.