Repository navigation
fix: let a failed oEmbed request fail - #764
Merged
Merged
Conversation
`transformOEmbedApiResponse` caught every failed oEmbed request and resolved with a made-up response holding a link to the URL. Core then stored that as a finished preview. A block that polls for its preview never saw the failure. After a VideoPress upload, the first `/oembed/1.0/proxy` request returns `404` until WordPress.com can embed the new video. The block took the link for a preview, stopped asking, and rendered it in its player sandbox: an empty box, with the link saved into the block's `cacheHtml`. The middleware now leaves a failed request to reject, so core stores `false`, as it does everywhere else. The core Embed block is unaffected: it treats `false` and a link fallback the same way and shows "could not be embedded" for both. A link fallback that WordPress itself returns still passes through, and the wrapper stripping for YouTube, Vimeo, Dailymotion and TED embeds is unchanged.
XCFramework BuildThis PR's XCFramework is available for testing. Add the following to your .package(url: "https://github.com/wordpress-mobile/GutenbergKit", branch: "pr-build/764")Built from e3a4028 |
dcalhoun
approved these changes
Oct 6, 2026
dcalhoun
left a comment
dcalhoun
left a comment
Member
There was a problem hiding this comment.
Thanks for addressing this. The changes tested well for me. The inline suggestions are small and not blocking.
| ); | ||
| } | ||
|
|
||
| it( 'rejects when WordPress cannot embed the URL', async () => { |
Member
There was a problem hiding this comment.
Finding from Claude:
The E2E test should show a fallback link for a non-embeddable URL (e2e/embed-content.spec.js:110) still describes the removed fallback, and it passes with this fix reverted: it only waits for attributes.url, which is set on submit, before the request settles. Rename it and assert the stored preview so it guards this change?
await page.waitForFunction(
(url) => window.wp.data.select("core").getEmbedPreview(url) === false,
"https://example.com/not-embeddable",
{ timeout: 30_000 },
);
Comment on lines
+456
to
+460
| * A failed request is left to reject. Core stores `false` for it, which the | ||
| * Embed block shows as "could not be embedded" and which blocks that poll for a | ||
| * preview — VideoPress, while WordPress.com is still processing an upload — | ||
| * take as the cue to ask again. Resolving with a link in its place reads as a | ||
| * finished preview, so those blocks stop asking and render the link. |
Member
There was a problem hiding this comment.
Optional: trim this to the final-state "why". The last sentence argues against the removed approach, which fits the commit message better.
Suggested change
| * A failed request is left to reject. Core stores `false` for it, which the | |
| * Embed block shows as "could not be embedded" and which blocks that poll for a | |
| * preview — VideoPress, while WordPress.com is still processing an upload — | |
| * take as the cue to ask again. Resolving with a link in its place reads as a | |
| * finished preview, so those blocks stop asking and render the link. | |
| * A failed request is left to reject, so core stores `false` and blocks that | |
| * poll for a preview (e.g. VideoPress while processing an upload) ask again. |
The E2E test for a non-embeddable URL was named for a fallback link the editor no longer makes up, and it waited only for the block's `url` attribute. The Embed block sets that on submit, before the oEmbed request settles, so the test passed whether or not a failed request was replaced with a made-up response. It now waits for core to store `false` for the preview. With the middleware's catch restored, it times out there.
The comment also argued against the link fallback, which is gone. That argument is in the commit that removed it.
jkmassel
enabled auto-merge (squash)
October 6, 2026 22:50
jkmassel
added a commit
that referenced
this pull request
Oct 7, 2026
Thirteen of #747's fifteen commits as one, rebased onto the asset bundle cache policy branch. It holds five changes: - Uploads. The page sends media uploads to native code over `gbk-upload:`, a `WKURLSchemeHandler` in the editor's own web view, in 4 MB `ArrayBuffer` chunks. iOS reclaims a suspended app's sockets, so the loopback server this replaces stopped answering after an ordinary screen lock. `MediaUploadServer` is deleted and its processor, uploader and client routing moves into `MediaUploadService`. Android keeps its loopback server: the middleware picks a transport from what `GBKit` advertises. - `gbk-media-file:` responses are streamed in 1 MB chunks, a task WebKit has stopped is never answered, and a path outside the media directory is refused. - The native inserter imports a picked item as a file and hands it to the page as a real `File`, through a file input that `NativeFileInput` answers. WebKit asks for that panel from iOS 18.4; before that the inserter hides the photo library and the camera. - The page's REST requests are relayed through `gbk-rest:`, so a site that doesn't answer `Origin: file://` with CORS headers still loads under Lockdown Mode. - Media uploads get a ten-minute inactivity timeout. The fetch interceptor also leaves the editor's own URL schemes out of network logging, and `docs/code/media-uploads.md` is new. Breaking: `GBKitGlobal.init` takes `nativeUploadScheme:` and `restRelayBaseURL:` instead of `nativeUploadPort:` and `nativeUploadToken:`. Not here, against #747 as it was: - `GutenbergKitHTTP`, its tests, `GutenbergKitDebugServer` and the demo app's Media Proxy Server screen are no longer deleted. Nothing in GutenbergKit imports the library any more; removing it is its own change. - The oEmbed fix is #764. - The editor-assets slash fix landed on trunk in #573.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extracted from #747; nothing here depends on it.
What?
Fixes a bug where a video uploaded through the VideoPress block could end up as an empty box holding a link. A failed oEmbed request now fails; the editor used to answer it with a made-up response.
Why?
The editor caught every failed oEmbed request and resolved with a response holding a link to the URL. Core stored that as a finished preview, so a block that polls for its preview never saw the failure.
After a VideoPress upload, the first
/oembed/1.0/proxyrequest returns404until WordPress.com can embed the new video. Prior to this PR the block took the link for a preview, stopped asking, and rendered it in its player sandbox: an empty box, with the link saved into the block'scacheHtml. Now core storesfalse, as it does for a failed preview everywhere else, and the block asks again.How?
transformOEmbedApiResponseinsrc/utils/api-fetch.jsno longer catches a failed request.The core Embed block behaves as before. It treats
falseand a link fallback the same way, and shows "could not be embedded" for both.Successful responses are untouched. A link fallback that WordPress itself returns still passes through, and the wrapper is still stripped from YouTube, Vimeo, Dailymotion and TED embeds.
Testing Instructions
make test-web-unit— 333 tests, 4 of them new. The two that expect a rejection fail ontrunkwithout the fix:AssertionError: promise resolved "{ …(3) }" instead of rejecting.trunk.