feat: Preload GutenbergKit dependencies #25151
Conversation
GutenbergKit now manages its own dependencies.
Pass dependencies upon initialization to avoid flashing the progress bar before GutenbergKit's internal cache returns.
`EditorDependencyManager` was marked `@MainActor`, which prevented synchronous access to cached dependencies from `NewGutenbergViewController`'s `init` method. This caused the editor to always show the progress bar (async flow) instead of using the fast path with cached dependencies. Changed `EditorDependencyManager` to use `NSLock` for thread-safe cache access, allowing `dependencies(for:)` to be called synchronously from any context. The prefetch method remains async but now properly guards all cache mutations with the lock.
Generated by 🚫 Danger |
jkmassel
force-pushed
the
feat/preload-gutenberg-kit-dependencies
branch
from
adb717f to
9dad7ce
Compare
|
| App Name | Jetpack | |
| Configuration | Release-Alpha | |
| Build Number | 30525 | |
| Version | PR #25151 | |
| Bundle ID | com.jetpack.alpha | |
| Commit | 951b273 | |
| Installation URL | 5hscasgd4o398 |
|
| App Name | WordPress | |
| Configuration | Release-Alpha | |
| Build Number | 30525 | |
| Version | PR #25151 | |
| Bundle ID | org.wordpress.alpha | |
| Commit | 951b273 | |
| Installation URL | 4vdiql1hvh81o |
🤖 Build Failure AnalysisThis build has failures. Claude has analyzed them - check the build annotations for details. |
dcalhoun
changed the title
dcalhoun
left a comment
dcalhoun
left a comment
There was a problem hiding this comment.
Changes LGTM. 👏🏻
I left a few small inline suggestions worth considering before merging.
Testing went well for me overall. As noted in the PR description and my inline comment, editor loading fails for WPCOM Simples sites as expected with the [email protected] release. Self-hosted sites loaded as expected; displaying the progress bar initially and activity indicator for subsequent loads.
- Force quit and reopen the app, then immediately try to open the editor
- Expected: On first run (before cache is populated), the editor should still load very quickly but may very briefly show a progress bar
While I saw the progress bar for first load after switching to a site for the first time, I was unable to manually trigger it via quitting the app. Presumably the memory was retained between app quit → launch, which should be fine from a UX perspective.
Modules/Package.swift
Outdated
| .package(url: "https://github.com/wordpress-mobile/NSURL-IDN", revision: "b34794c9a3f32312e1593d4a3d120572afa0d010"), | ||
| .package(url: "https://github.com/zendesk/support_sdk_ios", from: "8.0.3"), | ||
| .package(url: "https://github.com/wordpress-mobile/GutenbergKit", from: "0.10.1"), | ||
| .package(url: "https://github.com/wordpress-mobile/GutenbergKit", from: "0.12.0"), |
There was a problem hiding this comment.
it's expected that this will fail for WP.com until we have a new release of GBKit
We might upgrade to recently published 0.12.1 to receive the latest namespaces endpoint fix for WPCOM Simple sites. From my testing, the following error no longer displays with 0.12.1.
Error message
|
Claude finished @dcalhoun's task in 3m 3s —— View job Comprehensive Code Review Complete ✅I've performed a detailed review of this PR across code quality, security, performance, testing, and documentation. Overall, the implementation is well-designed and addresses a real performance problem, but there are several important issues to address. Summary AssessmentStrengths:
Critical Issues:
Detailed Findings1. Code Quality & Design✅ Strengths
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| static let shared = EditorDependencyManager() | ||
|
|
||
| /// Cached dependencies keyed by blog's ObjectID string representation. | ||
| private let cache = LockingHashMap<EditorDependencies>() |
There was a problem hiding this comment.
Memory Management Concern: This cache has no expiration mechanism or size limits. As users switch between sites, EditorDependencies objects will accumulate indefinitely in memory.
Recommendation: Consider adding a TTL (time-to-live) mechanism or LRU eviction policy. For example:
private struct CachedDependencies {
let dependencies: EditorDependencies
let cachedAt: Date
}
func dependencies(for blog: Blog) -> EditorDependencies? {
guard let cached = cache[key],
Date().timeIntervalSince(cached.cachedAt) < 3600 else { // 1 hour TTL
cache.removeValue(forKey: key)
return nil
}
return cached.dependencies
}Alternatively, implement an LRU cache with a maximum size (e.g., 5-10 entries).
There was a problem hiding this comment.
I'm going to deliberately ignore this – the vast majority of users have 1-2 sites and each cache entry is pretty small – even with dozens of entries the memory usage is only a few MB.
| cache.removeValue(forKey: key) | ||
| prefetchTasks[key]?.cancel() |
There was a problem hiding this comment.
Potential Race Condition: There's a small window between canceling the task and removing it from the map where another thread could call _prefetchDependencies and start a new task that might get orphaned.
Suggestion: Consider using a single atomic operation or wrapping in a lock:
prefetchTasks.withLock { tasks in
tasks[key]?.cancel()
tasks.removeValue(forKey: key)
}Note: This would require extending LockingHashMap to support such operations.
| /// | ||
| /// - Parameter blog: The blog to invalidate cache for. | ||
| @MainActor | ||
| func invalidate(for blog: Blog, completion: @escaping () -> Void) { |
There was a problem hiding this comment.
Documentation Clarity: The comment says "completion is guaranteed to run on the main actor" but doesn't clarify that the completion runs before the async purge() operation completes (based on line 140 where completion is called, then the Task continues).
If this is intentional, consider clarifying:
/// `completion` is guaranteed to run on the main actor and will be called
/// immediately after clearing the in-memory cache, but before the async purge
/// operation completes.If completion should wait for purge, move it after the purge:
do {
try await EditorService(configuration: configuration).purge()
} catch {
DDLogError("EditorDependencyManager: Failed to clear cache: \(error)")
}
completion() // Move here
self.invalidationTasks[key] = nilThere was a problem hiding this comment.
The code suggested by Claude is exactly how this already works 🤨
I'm going to ignore this as a false positive.
There was a problem hiding this comment.
Mmm. Yeah, I agree we should ignore. This is an odd suggestion.
| let task = Task { | ||
| do { | ||
| let dependencies = try await service.prepare(progress: { _ in }) | ||
| self.cache[key] = dependencies | ||
| } catch { | ||
| // Prefetch failed - editor will fall back to async loading | ||
| DDLogError("EditorDependencyManager: Failed to prefetch dependencies: \(error)") | ||
| } | ||
|
|
||
| self.prefetchTasks.removeValue(forKey: key) | ||
| } | ||
|
|
||
| prefetchTasks[key] = task | ||
|
|
||
| return task |
There was a problem hiding this comment.
Performance Consideration: If users rapidly switch between many sites, this could create numerous concurrent network prefetch operations without bounds. Each Task runs independently and fetches over the network.
Suggestion: Consider one of:
- Limit concurrent prefetches (e.g., max 3 at a time)
- Cancel the oldest prefetch when starting a new one
- Use a queue-based approach
Example with limit:
private let maxConcurrentPrefetches = 3
if prefetchTasks.count >= maxConcurrentPrefetches {
// Cancel oldest prefetch or return early
return nil
}There was a problem hiding this comment.
Same note here about user site count. This is unlikely to matter in practice.
Co-authored-by: David Calhoun <[email protected]>
|
Added GBKit 0.12.1 update in 951b273, now setting auto-merge. |
|
Description
This PR improves block editor load times by preloading GutenbergKit dependencies in the background.
What it does:
EditorDependencyManager, a singleton that manages prefetched editor dependencies in memoryEditorViewController, avoiding the async loading flow and progress barNSLock-basedLockingHashMapfor thread-safe cache access, allowing synchronous retrieval from any context (includinginitmethods)Why:
Previously, every time the user opened the block editor, they would see a loading progress bar while GutenbergKit fetched its dependencies. By prefetching these dependencies while the user browses My Site, we can skip the loading step entirely and open the editor immediately.
Other changes:
RawBlockEditorSettingsServicesince GutenbergKit now manages its own dependencies internallyNewGutenbergViewControllerby delegating dependency management to the new managerTesting instructions