feat: Recover content on WebView refresh via pull model #283
Conversation
Expose function for retrieving the latest content from the native host. This is useful for ensuring the content is updated after WebView refresh or re-initialization.
Enable the editor to request the latest content from the host.
Enable the editor to request the latest content from the host.
Ensure the editor always uses the latest content. This particularly important for subsequent initialization events--e.g., WebView refresh.
When crossing actor boundaries, Swift requires type to conform to `Sendable`, which `Any?` cannot. Constructing the dictionary outside of the `MainActor` run avoids this incompatibility.
|
Claude finished @dcalhoun's task in 1m 53s —— View job Code Review Complete ✅This PR implements a solid pull-based content recovery mechanism. The implementation is well-structured across all three platforms with comprehensive test coverage. Below is my detailed review. Overall AssessmentStrengths:
Architecture: The pull model is the right approach here. Having the web layer request content rather than push it eliminates timing issues and makes the recovery mechanism more reliable. Detailed Findings1. Code Quality
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| } | ||
| } | ||
|
|
||
| return null; |
There was a problem hiding this comment.
Error handling could be more informative
The error handling here returns null for any failure, which makes debugging difficult. Consider:
- Using the
error()logger instead ofdebug()for actual errors (per CLAUDE.md guidelines) - Distinguishing between "no content available" (expected) vs "bridge error" (unexpected)
if ( window.editorDelegate?.requestLatestContent ) {
try {
const result = window.editorDelegate.requestLatestContent();
return result ? JSON.parse( result ) : null;
} catch ( error ) {
// Use error() for actual errors, not debug()
error( 'Failed to request content from Android host', error );
return null;
}
}This helps distinguish between legitimate "no content" scenarios vs actual errors.
| * @return {Promise<{title: string, content: string}|null>} The latest content or null if unavailable. | ||
| */ | ||
| export async function requestLatestContent() { | ||
| if ( window.webkit?.messageHandlers?.requestLatestContent ) { |
There was a problem hiding this comment.
iOS bridge check could be more defensive
The iOS check uses optional chaining but doesn't verify that postMessage is actually a function. While unlikely to fail, consider adding runtime type validation for robustness:
if ( window.webkit?.messageHandlers?.requestLatestContent?.postMessage &&
typeof window.webkit.messageHandlers.requestLatestContent.postMessage === 'function' ) {This prevents potential crashes if the bridge is partially initialized or corrupted.
| } | ||
| } | ||
|
|
||
| fileprivate func controllerRequestsLatestContent(_ controller: GutenbergEditorController) -> (title: String, content: String)? { |
There was a problem hiding this comment.
Consider making delegate method non-optional in production
Currently returns nil when no delegate is set, which is fine for the demo but could mask integration issues in production. Consider:
- Adding debug logging when delegate is nil
- Or making the delegate method required (remove the
?from protocol method return type) and having a default implementation
This ensures integrators are aware they need to implement content recovery.
| delegate?.controllerRequestsLatestContent(self) | ||
| } | ||
|
|
||
| guard let content else { |
There was a problem hiding this comment.
Return type should be more explicit about "no content" vs "error"
Returning (nil, nil) for both "no content available" and error conditions makes it hard for the web layer to distinguish between:
- Normal case: no persisted content exists yet
- Error case: delegate failed to retrieve content
Consider returning (nil, "error message") when delegate is nil or using a more structured error response. This would help with debugging in production.
| * @return JSON string with title and content fields, or null if unavailable. | ||
| */ | ||
| @JavascriptInterface | ||
| fun requestLatestContent(): String? { |
There was a problem hiding this comment.
JSON serialization lacks error handling
The JSON object creation could throw if title/content contain characters that need escaping or if there are encoding issues. Consider wrapping in try-catch:
@JavascriptInterface
fun requestLatestContent(): String? {
val content = latestContentProvider?.getLatestContent() ?: return null
return try {
JSONObject().apply {
put("title", content.title)
put("content", content.content)
}.toString()
} catch (e: JSONException) {
Log.e("GutenbergView", "Failed to serialize latest content", e)
null
}
}This prevents crashes if content contains problematic characters.
| } ); | ||
|
|
||
| describe( 'priority', () => { | ||
| it( 'should prefer iOS bridge when both are available', async () => { |
There was a problem hiding this comment.
Excellent test coverage! 🎉
The priority test correctly validates that iOS bridge takes precedence over Android when both are available. This is important for preventing unexpected behavior in edge cases.
Minor suggestion: Add a comment explaining why iOS has priority (perhaps for consistency with other bridge methods or historical reasons).
| unregisterDisallowedBlocks( allowedBlockTypes ); | ||
| const post = getPost(); | ||
|
|
||
| const post = await getPost(); |
There was a problem hiding this comment.
Good async handling
The await here is correct, but consider adding error boundary or fallback behavior if getPost() throws. Currently an error would bubble up and potentially crash the app.
const post = await getPost().catch(error => {
// Log error and return safe defaults
console.error('Failed to get post content', error);
return {
id: -1,
type: 'post',
status: 'auto-draft',
title: { raw: '' },
content: { raw: '' },
};
});This makes initialization more resilient to bridge failures.
What?
Adds a pull-based content recovery mechanism that allows the web editor to request the latest content from the native host during initialization.
Why?
Ref CMM-1123.
When the WebView reinitializes (due to OS memory pressure or page refresh), the editor previously loaded stale content from
window.GBKit.postwhich was injected at original load time. The host apps have fresher content from autosave events (~1s intervals), but this wasn't being used on WebView recovery.Related WordPress app PRs:
How?
Implements a pull model where the web editor always requests content from the native host during initialization:
src/utils/bridge.js): AddedrequestLatestContent()function that calls the appropriate native bridge methodEditorViewController.swift): ImplementsWKScriptMessageHandlerWithReplyto respond to content requests via a new delegate methodeditorRequestsLatestContentGutenbergView.kt): AddsLatestContentProviderinterface and@JavascriptInterfacemethod that returns JSON contentsrc/utils/editor.jsx):getPost()now requests content from native host first, falling back towindow.GBKit.postonly when bridge is unavailableTesting Instructions
Accessibility Testing Instructions
No UI changes - this is a data recovery mechanism that happens during initialization.
Screenshots or screencast
N/A - No visual changes
🤖 Generated with Claude Code