Skip to content

Conversation

@kean
Copy link
Contributor

@kean kean commented Nov 20, 2025

What?

The subsequent editor loads are now ~1.3s faster. The first launch is ~1s faster.

There are a few changes compared to the previous implementation:

  • Move code that fetches assets (manifest and files) to the new EditorService. The assets manifest is now fetched in parallel with the editor settings.
  • Only EditorService now triggers the actual download of the assets. The assets are put into a GutenbergKit/{siteURL}/assets folder.
  • Remove EditorAssetsProvider and instead inject the plugin styles and scripts in window.GBKit when launching the editor (the same way it's done for editor settings). It happens only if EditorService verifies that all the required files exist on disk. If not, the plugins are disabled until the next time.
  • EditorService reads editor settings and assets manifest in one atomic operation. When new manifest is loaded, it replaces the current one only after fetching all assets for it. Until then, the editor will always use the previous version of the manifest.
  • Remove editorSettings from EditorConfiguration. Add them to internal EditorDependencies types fetched by EditorViewController

Minor changes:

  • Remove editorAssetsEndpoint parameter from EditorConfiguration
  • Make logging a cross-cutting concern, so it could be configured once and would be easier to use across GBK

Planned future changes:

  • Cleanup unused assets on launch to make sure the disk usage does not grow
  • Improve how and when refresh and assets download is triggered
  • Cover with unit tests and generate the same code in Kotlin

Why?

How?

Testing Instructions

Accessibility Testing Instructions

Screenshots or screencast

Before / After

Note: the preparing editor message disappears quicker because "fetch editor settings" is now a step inside EditorViewController.

comparison.mp4

import CryptoKit
import SwiftSoup

struct EditorAssetsManifest: Codable {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an existing struct. I made no changes to it – just renamed the file.

@kean kean marked this pull request as draft November 20, 2025 23:02
@kean kean force-pushed the task/editor-assets branch from a9806a5 to 0b9616f Compare November 21, 2025 12:54
@kean kean force-pushed the task/editor-assets branch from 0b9616f to dac7dc6 Compare November 21, 2025 13:26
Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for creating this!

This makes sense to me a high level. It feels like a worthwhile improvement overall, especially given the performance improvements.

I left a few thoughts to consider. Many are for bettering my own understanding.

Comment on lines 35 to 36
/// Endpoint for loading editor assets, used when enabling `shouldUsePlugins`
public var editorAssetsEndpoint: URL?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we removing editorAssetsEndpoint? I believe it's useful in the future whenever we change the REST API endpoint used, it also allows alternatives for third-parties as needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest to add it again in the future if it's needed. For now, I kept it simple as EditorService knows what endpoints to use, and the code in the app was duplicating the same endpoint.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I believe we must retain this to support WPCOM sites. They require setting a namespace endpoint—e.g., https://public-api.wordpress.com/wpcom/v2/sites/<site_id>/editor-assets.

From my testing, plugin loading currently fails for WPCOM Simple sites.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch. So far, I've only tested it with an Atomic site as the demo doesn't seem to support Simple sites:

Screenshot 2025-12-02 at 9 46 59 AM

I'll probably revert it for now, but there has to be a better way to do it. I'm not completely happy with how the configuration is spread out between GBK and the app. I'm leaning towards centralizing this knowledge in GBK so it would work in both the app and the demo. The configuration code is also a bit hard to follow at the moment, but it seems like it's better be left for future PRs.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct, the demo app does not support WPCOM sites. I tested via integrating with the WordPress-iOS app.

I open to refactoring the API for configuring and starting the editor.

As much as possible, we should avoid WPCOM/Jetpack references in the GutenbergKit code base. There are few that exist today, but they need to be removed in the future.

},
logLevel: '\(configuration.logLevel)'
logLevel: '\(EditorLogger.logLevel.rawValue)',
manifest: \(dependencies?.manifest ?? "undefined")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any performance implications from adding this? 🤔 I suppose it is no larger than editorSettings, eh?

I wonder if it's worth spreading the properties over the top-level object. I could see one day allowing GBK configuration to control allowedBlockTypes.

Copy link
Contributor Author

@kean kean Nov 21, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any performance implications from adding this? 🤔 I suppose it is no larger than editorSettings, eh?

I wouldn't expect it to be slower than the current implementation, but I already had some second thoughts yesterday. Is it safe to pass these larger strings directly in JS this way? Does it need escaping? Is there a more robust way to do it? Should we pass the whole pre-processed manifest as a separate object? Both settings and manifest seem to have <scripts> and <styles>, is there a better way to inject them? What about allowedBlockTypes? Should the app combine them (for core and plugins)?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, my original performance concerns are likely invalid; I figured I'd share my thought nonetheless.

I do not have a better idea for injecting them at this time.

Regarding escaping, maybe there is benefit. That said, if the itself relies upon the same scripts we are retrieving, would site already be compromised? Is the mobile editor exposing anything that site does not already?

For allowedBlockTypes, I envision this eventually becoming an editor configuration option rather than a value returned from the editor assets endpoint. I consider its current location a stopgap solution while various blocks do not function in the mobile editor.

Comment on lines 340 to 348
const gbkit = getGBKit();

// iOS provides manifest directly in GBKit configuration
if ( window.webkit && gbkit.manifest ) {
return gbkit.manifest;
}

const { siteApiRoot, editorAssetsEndpoint, authHeader } = getGBKit();
// Android fallback - fetch from API
const { siteApiRoot, editorAssetsEndpoint, authHeader } = gbkit;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't believe there is a downside to de-structuring all values up front.

Suggested change
const gbkit = getGBKit();
// iOS provides manifest directly in GBKit configuration
if ( window.webkit && gbkit.manifest ) {
return gbkit.manifest;
}
const { siteApiRoot, editorAssetsEndpoint, authHeader } = getGBKit();
// Android fallback - fetch from API
const { siteApiRoot, editorAssetsEndpoint, authHeader } = gbkit;
const { manifest, siteApiRoot, editorAssetsEndpoint, authHeader } = getGBKit();
// iOS provides manifest directly in GBKit configuration
if ( window.webkit && manifest ) {
return manifest;
}
// Android fallback - fetch from API

Alternatively, I don't believe there is a downside to invoking getGBKit() multiple times, as it early returns with the cached value on the window.

Suggested change
const gbkit = getGBKit();
// iOS provides manifest directly in GBKit configuration
if ( window.webkit && gbkit.manifest ) {
return gbkit.manifest;
}
const { siteApiRoot, editorAssetsEndpoint, authHeader } = getGBKit();
// Android fallback - fetch from API
const { siteApiRoot, editorAssetsEndpoint, authHeader } = gbkit;
const { manifest } = getGBKit();
// iOS provides manifest directly in GBKit configuration
if ( window.webkit && gbkit.manifest ) {
return gbkit.manifest;
}
// Android fallback - fetch from API
const { siteApiRoot, editorAssetsEndpoint, authHeader } = getGBKit();

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I reverted this change together with how manifest is passed to the editor (now it again uses separate post message scheme).

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you please explain the rationale for reverting for my own education? 🙇🏻‍♂️

@kean kean changed the title WIP: Faster and more robust editor asset management Faster and more robust editor asset management Nov 21, 2025
@kean kean marked this pull request as ready for review November 21, 2025 21:43
@kean
Copy link
Contributor Author

kean commented Nov 21, 2025

I made a few more changes:

  • Added unit tests and fixed a couple of issues (it explains most of the line additions)
  • Updated EditorService to store processed manifest on disk (it takes around 100ms to generate it)
  • Added support for cleaning up orphaned asset files, so we don’t take too much space on disk

With these changes, this whole step now takes 2ms.

I had to switch to a different task today and didn't have time to review the final PR. I'll do it when I have time. It should be good.

@kean kean requested a review from dcalhoun November 21, 2025 21:44
kean added 3 commits November 27, 2025 10:39
This restores the EditorAssetsProvider to handle manifest loading via
WebKit message handler instead of injecting it directly into the
configuration.

Changes:
- Restored EditorAssetsProvider.swift to handle manifest requests
- Added EditorAssetsLibrary wrapper around EditorService
- Removed manifest property from EditorConfiguration and EditorDependencies
- Updated EditorViewController to use EditorAssetsProvider
- Updated bridge.js to use message handler for iOS manifest fetching
- Removed manifest from GBKit configuration
@kean kean requested review from dcalhoun and removed request for dcalhoun November 28, 2025 17:59
@kean
Copy link
Contributor Author

kean commented Dec 1, 2025

I updated the unit tests and added a few more. It should be good for review and merge now.

Copy link
Member

@dcalhoun dcalhoun left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall, this tested well for me in the demo app.

As stated in a new inline comment, I believe we must retain the editorAssetsEndpoint configuration. I also noted a few oddities likely worth addressing.

This may be understood, but we need to open a sibling WordPress-iOS PR integrating these changes.

case .none: self.activeEditorConfiguration = nil
case .bundledEditor:
let config = createBundledConfiguration()
activeEditorConfiguration = config
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Likely worthwhile consistently using self in this switch.

Suggested change
activeEditorConfiguration = config
self.activeEditorConfiguration = config

Comment on lines 35 to 36
/// Endpoint for loading editor assets, used when enabling `shouldUsePlugins`
public var editorAssetsEndpoint: URL?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, I believe we must retain this to support WPCOM sites. They require setting a namespace endpoint—e.g., https://public-api.wordpress.com/wpcom/v2/sites/<site_id>/editor-assets.

From my testing, plugin loading currently fails for WPCOM Simple sites.

Copy link
Contributor

@jkmassel jkmassel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After some discussion with @dcalhoun and @kean, I'm approving this to merge and we can deal with breakage later. This and #232 represent some pretty serious refactoring, so we should get them landed then incrementally adjust after.

Base automatically changed from task/editor-settings to trunk December 2, 2025 21:35
@jkmassel jkmassel enabled auto-merge (squash) December 2, 2025 21:59
@jkmassel jkmassel merged commit 109fc5f into trunk Dec 2, 2025
9 of 10 checks passed
@jkmassel jkmassel deleted the task/editor-assets branch December 2, 2025 22:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants