Repository navigation
feat: M4 · media bridge for Gutenberg MediaUpload (#314) - #330
Merged
Merged
Conversation
Replaces the M2 stub with a production slot-fill that routes core media
blocks (image, gallery, video, audio, file, cover, media-text) through
`artisanpack-ui/media-library`'s `MediaModal` and `uploadMedia`.
Hosts wire the bridge by calling `registerMediaBridge({ MediaBridge,
uploadMedia })` before `bootVisualEditor()`. The slot-fill translates
Gutenberg's MediaUpload contract — `allowedTypes`, `multiple`, `value`,
`onSelect`, `render` — onto MediaModal's props, and the adapter converts
media-library `Media` records to the `{ id, url, alt, caption, mime,
width, height, media_type, sizes }` shape core blocks read. Drag-and-drop
onto the canvas flows through the same `uploadMedia` pipeline via
`settings.mediaUpload`, so there is no separate upload endpoint.
When no bridge is registered the picker surfaces a loud operator-facing
message instead of silently dropping clicks (the previous stub behaviour,
preserved for apps that haven't yet wired media-library).
Tests: 31 new vitest cases across adapter shape conversion, slot-fill
behaviour, settings.mediaUpload upload flow, and per-block contract
coverage for the seven core media blocks. End-to-end verification against
a real `MediaModal` is done via the sandbox and the dev app.
Closes #314
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughClient-side media bridge added: new media-bridge modules (types, state, adapter, UI, upload-setting), re-exports from editor entry, editor boot and sandbox now wire the bridge setting/filter, legacy media-upload stub removed, and comprehensive unit/integration tests added. (No runtime config defaults changed.) Changes
Sequence Diagram(s)sequenceDiagram
participant Host as Host App
participant Reg as registerMediaBridge()
participant State as Bridge State
participant Filter as WP filter (editor.MediaUpload)
participant Block as Gutenberg Block
participant MUB as MediaUploadBridge
participant Bridge as Registered Media Bridge
Host->>Reg: registerMediaBridge({ MediaBridge, uploadMedia })
Reg->>State: store bridge & uploader
Reg->>Filter: ensure editor.MediaUpload filter installed
Block->>MUB: render / user clicks media button
MUB->>State: getMediaBridge()
State-->>MUB: Bridge component | null
alt Bridge present
MUB->>Bridge: render(open=true, multiSelect, allowedTypes, title)
Bridge->>MUB: onSelect(BridgeMedia[])
MUB->>MUB: mediaToGutenberg(...)
MUB->>Block: onSelect(GutenbergMedia | GutenbergMedia[])
else No bridge
MUB->>MUB: alert("registerMediaBridge")
end
sequenceDiagram
participant Block as Gutenberg Block
participant MUS as mediaUploadSetting()
participant State as Bridge State
participant Uploader as Host Uploader
participant Adapter as mediaToGutenberg()
participant Callback as onFileChange()
Block->>MUS: call mediaUploadSetting(options with filesList)
MUS->>State: getMediaUploader()
alt no uploader
MUS->>Block: options.onError(message)
else uploader present
MUS->>MUS: validate maxUploadFileSize (may error)
loop each file
MUS->>Uploader: uploader(file, additionalData)
Uploader-->>MUS: BridgeMedia or { data: BridgeMedia }
MUS->>Adapter: mediaToGutenberg(BridgeMedia)
end
MUS->>Callback: onFileChange(GutenbergMedia[])
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
resources/js/visual-editor/media-bridge/__tests__/media-upload-setting.test.ts (1)
139-160: Add a test for synchronous uploader throws.Current coverage checks rejected promises, but not
throw new Error(...)before a promise is returned. Adding that case will protect the error-path contract from regressions.✅ Suggested test case
+ it('surfaces synchronous uploader throws via onError', async () => { + const uploader: MediaUploader = () => { + throw new Error('sync boom'); + }; + + registerMediaBridge({ + MediaBridge: () => null, + uploadMedia: uploader, + }); + + const onError = vi.fn(); + mediaUploadSetting({ + filesList: [textFile('a.txt', 'a')], + onError, + }); + + await vi.waitFor(() => { + expect(onError).toHaveBeenCalledTimes(1); + }); + expect(onError.mock.calls[0]?.[0]).toBe('sync boom'); + });🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/media-bridge/__tests__/media-upload-setting.test.ts` around lines 139 - 160, Add a test that verifies synchronous throw from an uploader is surfaced via the onError callback: create a MediaUploader implementation that throws (e.g., const uploader: MediaUploader = () => { throw new Error('boom') }), register it with registerMediaBridge via the uploadMedia property, call mediaUploadSetting with filesList and an onError spy (vi.fn()), wait for the callback with vi.waitFor and assert onError was called once and its first argument is the thrown error message; use the same structure/expectations as the existing rejection test to ensure both Promise rejections and synchronous throws are covered.resources/js/visual-editor/media-bridge/state.ts (1)
73-74: Comment is stale relative to implementation.Line 73-74 references an “import inside the callback,” but there is no import there. Please update the comment to reflect the current lazy variable-resolution approach.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/media-bridge/state.ts` around lines 73 - 74, The inline comment above the block that used to do a callback import is outdated; update the comment in state.ts near the lazy resolution logic (look for the lazy variable-resolution implementation or function/getter that defers resolving media-upload dependencies) to describe the current lazy variable-resolution approach instead of mentioning an "import inside the callback" and explain briefly that the module avoids a cycle by resolving the dependency lazily at runtime rather than performing an import in the callback.resources/js/visual-editor/media-bridge/adapter.ts (1)
72-73: Adjust docstring to match actualallowedTypesToBridgeTypesbehavior.The comment says all-inclusive input returns
undefined, but the function currently returns all recognized categories. Please align comment or implementation.Doc-only fix example
- * Returns `undefined` when the input would allow every category so the - * picker does not over-constrain itself. + * Returns `undefined` only when no recognized category is present (or the + * input is empty/omitted).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@resources/js/visual-editor/media-bridge/adapter.ts` around lines 72 - 73, The docstring for allowedTypesToBridgeTypes is incorrect: it claims "Returns `undefined` when the input would allow every category" but the implementation returns all recognized categories instead; update the docstring to state that when the input allows every category the function returns an array of all bridge categories (i.e., all recognized categories) rather than undefined, and include a short example or phrase clarifying that the result is an explicit list of categories rather than undefined so callers know to expect an array from allowedTypesToBridgeTypes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@resources/js/visual-editor/media-bridge/media-upload-setting.ts`:
- Around line 72-76: The files.map call currently calls uploader(file,
options.additionalData) synchronously before Promise.resolve, so synchronous
throws escape the promise chain; change the mapping to defer uploader execution
by invoking Promise.resolve(file).then(() => uploader(file,
options.additionalData)).then(unwrap) (or equivalent) so any synchronous
exceptions from uploader are caught by the existing .catch and routed to
onError; ensure references to files.map, uploader, options.additionalData,
Promise.resolve, unwrap and the onError/.catch chain are preserved.
In `@resources/js/visual-editor/media-bridge/media-upload.tsx`:
- Around line 128-132: The click handler currently always calls open() after
invoking existingOnClick, which violates child intent when the child prevents
the default; update the cloned onClick (used in cloneElement) to invoke
existingOnClick(event) first, then check the event for cancellation (e.g., if
event is an Event and event.defaultPrevented is true or (event as
any)?.defaultPrevented) and only call open() when the click was not prevented;
reference the cloned onClick wrapper, existingOnClick, and open to locate and
change the logic.
- Around line 39-52: The trigger callback currently uses the render-time
captured Bridge variable (const Bridge = getMediaBridge()), which can be stale;
instead, inside the open handler of renderTrigger call resolve the bridge at
click time by calling getMediaBridge() and then proceed — i.e., replace the use
of the outer Bridge in the open function with a fresh const bridge =
getMediaBridge(), check bridge === null and call notifyUnconfigured() if needed,
otherwise call setOpen(true); update references to Bridge, getMediaBridge,
renderTrigger, notifyUnconfigured, and setOpen accordingly.
---
Nitpick comments:
In
`@resources/js/visual-editor/media-bridge/__tests__/media-upload-setting.test.ts`:
- Around line 139-160: Add a test that verifies synchronous throw from an
uploader is surfaced via the onError callback: create a MediaUploader
implementation that throws (e.g., const uploader: MediaUploader = () => { throw
new Error('boom') }), register it with registerMediaBridge via the uploadMedia
property, call mediaUploadSetting with filesList and an onError spy (vi.fn()),
wait for the callback with vi.waitFor and assert onError was called once and its
first argument is the thrown error message; use the same structure/expectations
as the existing rejection test to ensure both Promise rejections and synchronous
throws are covered.
In `@resources/js/visual-editor/media-bridge/adapter.ts`:
- Around line 72-73: The docstring for allowedTypesToBridgeTypes is incorrect:
it claims "Returns `undefined` when the input would allow every category" but
the implementation returns all recognized categories instead; update the
docstring to state that when the input allows every category the function
returns an array of all bridge categories (i.e., all recognized categories)
rather than undefined, and include a short example or phrase clarifying that the
result is an explicit list of categories rather than undefined so callers know
to expect an array from allowedTypesToBridgeTypes.
In `@resources/js/visual-editor/media-bridge/state.ts`:
- Around line 73-74: The inline comment above the block that used to do a
callback import is outdated; update the comment in state.ts near the lazy
resolution logic (look for the lazy variable-resolution implementation or
function/getter that defers resolving media-upload dependencies) to describe the
current lazy variable-resolution approach instead of mentioning an "import
inside the callback" and explain briefly that the module avoids a cycle by
resolving the dependency lazily at runtime rather than performing an import in
the callback.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ecd3ba9-d03f-42fd-9337-fe4120cc5131
📒 Files selected for processing (15)
config/visual-editor.phpresources/js/visual-editor/editor/editor-app.tsxresources/js/visual-editor/editor/main.tsxresources/js/visual-editor/media-bridge/__tests__/adapter.test.tsresources/js/visual-editor/media-bridge/__tests__/core-blocks.test.tsxresources/js/visual-editor/media-bridge/__tests__/media-upload-setting.test.tsresources/js/visual-editor/media-bridge/__tests__/media-upload.test.tsxresources/js/visual-editor/media-bridge/adapter.tsresources/js/visual-editor/media-bridge/index.tsresources/js/visual-editor/media-bridge/media-upload-setting.tsresources/js/visual-editor/media-bridge/media-upload.tsxresources/js/visual-editor/media-bridge/state.tsresources/js/visual-editor/media-bridge/types.tsresources/js/visual-editor/sandbox/sandbox-editor.tsxresources/js/visual-editor/vendor/media-upload-stub.tsx
💤 Files with no reviewable changes (1)
- resources/js/visual-editor/vendor/media-upload-stub.tsx
- `media-upload-setting.ts`: defer the `uploader` call into a resolved `.then()` so synchronous throws route through the existing `.catch` → `onError` path instead of escaping `Promise.all`. - `media-upload.tsx` (cloned trigger): after invoking the child's `onClick`, short-circuit when the event was `preventDefault()`ed so callers that consume the click don't get a surprise picker. - `media-upload.tsx` (click handler): re-resolve the registered bridge at click time via `getMediaBridge()` rather than closing over the render-time snapshot — covers hot-reload and out-of-order boots where registration happens after the component first rendered. Added three tests covering the new behaviours. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
github-project-automation
Bot
moved this from In Review
to Done
in ArtisanPack UI Overview
19 of 21 tasks
15 of 21 tasks
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
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.
Description
Wires
artisanpack-ui/media-library's ReactMediaModalanduploadMediainto Gutenberg'sMediaUploadslot-fill so every core media block —core/image,core/gallery,core/video,core/audio,core/file,core/cover,core/media-text— is production-usable inside the visual editor. Replaces the M2 stub that only surfaced an alert.Closes: #314
Type of Change
Related Issue
Issue: #314 (milestone M4 under umbrella #309)
Motivation and Context
Without this, Gutenberg's core media blocks either crash trying to reach the WP REST API or render an empty picker. After this, the picker opens the
MediaModalfromartisanpack-ui/media-library, drag-and-drop uploads to the canvas go through the sameuploadMediapipeline, and selections flow back to the block as Gutenberg-shape attachments withid,url,alt,caption,mime,width,height,media_type,sizes.The bridge stays framework-decoupled — the visual editor doesn't hard-depend on media-library. Hosts register the picker at boot:
Changes Made
resources/js/visual-editor/media-bridge/— new module (adapter, slot-fill, settings callback, registry, barrel)types.ts—BridgeMedia,MediaBridgeComponent,GutenbergMedia,MediaUploader, etc.adapter.ts—mediaToGutenberg()(includingsizesextraction +media_typeinference) andallowedTypesToBridgeTypes()(handles both categorical names and mime types)media-upload.tsx— the slot-fill replacement registered undereditor.MediaUpload; supports bothrenderand cloned-child trigger patterns; handlesmultiple/galleryfan-outmedia-upload-setting.ts—settings.mediaUploadcallback for drag-and-drop and block-library upload helpers; unwraps{ data }response envelopesstate.ts—registerMediaBridge()public API,ensureMediaBridgeFilter(), idempotent filter install, test reset helperindex.ts— barreleditor/editor-app.tsx— swaps M2 stub (registerMediaUploadStub/mediaUploadStub) forensureMediaBridgeFilter()/mediaUploadSettingeditor/main.tsx— re-exportsregisterMediaBridge(and companion types) so hosts can wire the bridge from the package's entrysandbox/sandbox-editor.tsx— mirrors the swap so the sandbox canvas uses the new filterresources/js/visual-editor/vendor/media-upload-stub.tsx— deleted; supersededconfig/visual-editor.php— updated comment onmedia.adapter: the M4 wiring is client-side viaregisterMediaBridge()How Has This Been Tested?
Testing Environment:
release/1.0Tests Performed:
./vendor/bin/pest— all 66 Pest tests pass (no PHP changes; verified the package's existing suite stays green).npm test— all 34 vitest files pass (267 tests, 1 skipped). 31 new tests cover the media bridge.~/Herd/artisanpack-ui):core/image→ clicked Media Library →MediaModalopened, showed library + upload tabs, selection round-tripped into the block with alt/caption/dimensions.core/gallery(multi-select),core/video,core/audio,core/file,core/cover,core/media-text.uploadMediaand inserted the resulting media.Accessibility Tests Run
role="dialog", escape-to-close, focus trap)Details: Keyboard flow was exercised in the browser — Tab/Shift-Tab cycles through modal controls, Enter confirms a focused media item, Esc closes the modal. The bridge itself doesn't introduce new focusable UI — it delegates entirely to
MediaModal, whose a11y is covered in the media-library package.Tests Added
adapter.test.ts(13 tests) covering shape conversion,is_image/is_video/is_audioflag precedence, mime-prefix fallback, image sizes in metadata, andallowedTypes→ bridge categories.media-upload.test.tsx(5 tests) — slot-fill component with and without a registered bridge,render/child trigger patterns, single vs. multi-select,gallery: truefan-out.media-upload-setting.test.ts(6 tests) — uploader invocation,{ data }unwrapping,maxUploadFileSizeenforcement, error propagation, empty-list short-circuit.core-blocks.test.tsx(7 tests) — per-block contract replay for the seven core media blocks, verifying each emits the expectedmultiSelect/allowedTypesand thatonSelectreceives a single record or array per block.Test details: Direct Gutenberg block rendering via
@wordpress/block-library+registerCoreBlockscouldn't be exercised inside Vitest because@wordpress/block-editor'sblock-compareimportsdiff/lib/diff/characterwithout a file extension — Node's ESM resolver rejects it while Vite's browser build accepts it. Rather than ship a fragileresolve.aliaspatch, the integration test replays each block's<MediaUpload>invocation at the filter level with the exact props core blocks send. End-to-end verification in a real editor is handled by the sandbox and the dev app.Documentation
state.ts)Documentation details: Public API notes live in the module docblocks. The umbrella docs (
docs/gutenberg-adoption.md) already reference M4 by issue; no structural docs change is required before the full V1 release docs pass in M15 (#325).Pre-Submission Checklist
🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Tests
Documentation
Chores