Conversation
📝 WalkthroughWalkthroughAdds AVIF output support: runtime dependency, REST option and sanitization, admin UI toggle and localization, client-side format-support detection, AVIF encoding path in the image converter, and async converter selection based on configured output format. Changes
Sequence Diagram(s)sequenceDiagram
participant Admin as Admin User
participant UI as Admin Settings UI
participant API as REST API
participant Backend as PHP Backend
participant Client as Client (Browser)
participant Conv as Image Converter
Admin->>UI: choose output "avif"
UI->>API: POST cimo_options { image_output_format: "avif" }
API->>Backend: persist setting
Backend-->>API: OK
API-->>UI: saved
Note over Client,Conv: On file upload/processing
Client->>Client: read window.cimoSettings.imageOutputFormat
Client->>Conv: getFileConverter(file) (async)
Conv->>Conv: isFormatSupported("avif") (async)
alt supported
Conv->>Conv: load canvas imageData
Conv->>Conv: import `@jsquash/avif` and encode → Blob(image/avif)
Conv-->>Client: return converted Blob
else not supported
Conv-->>Client: return original file (no conversion)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
🤖 Pull request artifacts
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/shared/converters/image-converter.js (2)
179-214:⚠️ Potential issue | 🔴 CriticalCritical: Duplicate
canvas.toBlobcauses double promise resolution and missing resource cleanup.The new AVIF/non-AVIF
if/elseblock (lines 179–197) resolves the promise, but the originalcanvas.toBlobcall (lines 199–214) was not removed. This means:
- For AVIF: The promise is resolved at line 185, then the leftover
canvas.toBlobat line 199 fires again — attempting to encodeimage/avifvia canvas (which likely falls back to PNG), doing unnecessary work and callingresolveon an already-settled promise.- For non-AVIF: The
elsebranch at line 190 resolves the promise, then the secondtoBlobat line 199 also callsresolve— again a no-op but wasteful.- Resource leak in the new path:
URL.revokeObjectURL(objectUrl)and canvas cleanup only happen inside the old block (lines 200–207), not in the newif/elseblock. While the old block still runs as a side-effect, this is fragile and accidental.Remove the original
canvas.toBlobblock (lines 199–214) and move the cleanup logic into the newif/elseblock:Proposed fix
+ const cleanup = () => { + URL.revokeObjectURL( objectUrl ) + objectUrl = null + ctx.clearRect( 0, 0, canvas.width, canvas.height ) + canvas.width = 0 + canvas.height = 0 + } + if ( outputFormat === 'avif' ) { try { const imageData = ctx.getImageData( 0, 0, width, height ) const avifQuality = Math.max( 1, Math.min( 100, Math.round( ( ( q ?? 0.5 ) * 100 ) ) ) ) const avifBuffer = await encodeAvif( imageData, { quality: avifQuality } ) const avifBlob = new Blob( [ avifBuffer ], { type: 'image/avif' } ) + cleanup() resolve( avifBlob ) } catch ( e ) { + cleanup() reject( new Error( `Failed to encode AVIF: ${ e instanceof Error ? e.message : 'Unknown error' }` ) ) } } else { canvas.toBlob( function( blob ) { + cleanup() if ( blob ) { resolve( blob ) } else { reject( new Error( 'Failed to convert image' ) ) } }, format.mimeType, q ) } - - canvas.toBlob( function( blob ) { - // Clean up resources - URL.revokeObjectURL( objectUrl ) - objectUrl = null - - // Clear canvas to free memory - ctx.clearRect( 0, 0, canvas.width, canvas.height ) - canvas.width = 0 - canvas.height = 0 - - if ( blob ) { - resolve( blob ) - } else { - reject( new Error( 'Failed to convert image' ) ) - } - }, format.mimeType, q )
250-257:⚠️ Potential issue | 🔴 CriticalCanvas-based format check incorrectly blocks AVIF encoding.
The
convert()method (lines 251–252) tests browser support viatestCanvas.toDataURL(formatInfo.mimeType). Most browsers do not supportimage/avifin canvas natively, so this check will returnfalsefor AVIF — causing the method to bail out and return the original file without conversion, even though the WASM-based@jsquash/avifencoder at line 183 handles AVIF encoding independently of canvas support.Skip this canvas check when the format is
avif:Proposed fix
// Check if the browser supports the desired output format const testCanvas = document.createElement( 'canvas' ) - if ( formatInfo && ! testCanvas.toDataURL( formatInfo.mimeType ).startsWith( `data:${ formatInfo.mimeType }` ) ) { + if ( format !== 'avif' && formatInfo && ! testCanvas.toDataURL( formatInfo.mimeType ).startsWith( `data:${ formatInfo.mimeType }` ) ) { // If not supported, skip conversion and return the original file // eslint-disable-next-line no-console console.error( '[Cimo] ' + format + ' is not supported by the browser, please use another modern browser' ) return { file, metadata: null } }src/shared/converters/index.js (1)
77-107:⚠️ Potential issue | 🔴 Critical
isFormatSupporteduses canvas capability check, which will reject AVIF in most browsers.
isFormatSupported('avif')creates a canvas and testscanvas.toDataURL('image/avif'). Most browsers do not natively support AVIF output in canvas, so this returnsfalse— causing line 43–44 to returnNullConverterand silently skip all AVIF conversions.Since AVIF encoding is handled by the standalone WASM-based
@jsquash/avifencoder (not by canvas), this check is incorrect for AVIF. The function should exempt AVIF from the canvas test.Proposed fix
function isFormatSupported( format ) { if ( ! format || typeof format !== 'string' ) { return false } + // AVIF is encoded via a dedicated WASM encoder, not canvas + if ( format.toLowerCase() === 'avif' ) { + return true + } + // Map format names to MIME types const formatMap = { webp: 'image/webp', - avif: 'image/avif', }
🤖 Fix all issues with AI agents
In `@src/admin/class-admin.php`:
- Around line 59-61: The REST schema's 'image_output_format' entry lacks an enum
and sanitize_options does not validate allowed values; update the schema for
'image_output_format' (in class-admin.php) to include an 'enum' restricting
values to 'webp' and 'avif', and in the sanitize_options function validate the
incoming option by calling sanitize_text_field then checking against the
allowlist (e.g., in_array($val, ['webp','avif'], true)), returning a safe
default or removing/setting null when invalid; apply the same validation logic
to the related code block around lines 224-227 so only 'webp' or 'avif' can be
persisted.
In `@src/shared/converters/image-converter.js`:
- Line 196: The canvas.toBlob call is passing the raw variable quality instead
of the normalized q that was set earlier (see q assignment around line 177);
update the call where canvas.toBlob is invoked (the line containing "},
format.mimeType, quality)") to pass q instead of quality so lossless formats use
undefined and the behavior matches the earlier normalization; ensure the symbol
names remain q and format.mimeType in that invocation.
🧹 Nitpick comments (2)
src/shared/converters/index.js (1)
41-51: Consider: quality setting name is WebP-specific but now applies to AVIF too.
webpQuality(line 49) is now used as the quality parameter for both WebP and AVIF encoding. While functionally correct, the naming may confuse users or developers. The admin UI label also reads "WebP Image Quality" (admin-settings.js line 430), which is misleading when AVIF is selected.Consider renaming the label dynamically based on the selected format, or using a more generic name like "Image Quality". This can be addressed in a follow-up.
src/admin/js/page/admin-settings.js (1)
133-159: Pre-existing: preset functions don't flag unsaved changes.
applyImageRecommendedSettingsandapplyImageDefaultSettingscallsetSettingsbut notsetHasUnsavedChanges(true), so the "unsaved changes" indicator won't appear when a user clicks "Recommended" or "Reset to Default". This affects all preset buttons, not just the newimageOutputFormatfield — consider addressing in a separate PR.
| 'image_output_format' => [ | ||
| 'type' => 'string', | ||
| ], |
There was a problem hiding this comment.
Restrict image_output_format to an allowlist of valid values.
Currently the schema has no enum constraint and the sanitizer only calls sanitize_text_field, meaning any arbitrary string (including malicious input) can be persisted. Since only 'webp' and 'avif' are valid, add validation at both layers:
Proposed fix
In the REST schema (around line 59):
'image_output_format' => [
'type' => 'string',
+ 'enum' => [ 'webp', 'avif' ],
],In sanitize_options (around line 225):
if ( isset( $options['image_output_format'] ) ) {
- $sanitized['image_output_format'] = sanitize_text_field( $options['image_output_format'] );
+ $format = sanitize_text_field( $options['image_output_format'] );
+ $sanitized['image_output_format'] = in_array( $format, [ 'webp', 'avif' ], true ) ? $format : 'webp';
}Also applies to: 224-227
🤖 Prompt for AI Agents
In `@src/admin/class-admin.php` around lines 59 - 61, The REST schema's
'image_output_format' entry lacks an enum and sanitize_options does not validate
allowed values; update the schema for 'image_output_format' (in class-admin.php)
to include an 'enum' restricting values to 'webp' and 'avif', and in the
sanitize_options function validate the incoming option by calling
sanitize_text_field then checking against the allowlist (e.g., in_array($val,
['webp','avif'], true)), returning a safe default or removing/setting null when
invalid; apply the same validation logic to the related code block around lines
224-227 so only 'webp' or 'avif' can be persisted.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/shared/converters/image-converter.js (1)
243-249:⚠️ Potential issue | 🔴 CriticalCanvas support check will incorrectly reject AVIF in many browsers.
The AVIF encoding path uses the
@jsquash/avifWASM encoder — it never touchescanvas.toBlob/toDataURL. However,convert()gates all formats through atoDataURLsupport test (line 244). In browsers where the canvas doesn't natively support AVIF (common today), this check will fail and the method will bail out, returning the original file — even though the WASM encoder would work perfectly fine.🐛 Proposed fix — skip canvas check for AVIF
// Check if the browser supports the desired output format - const testCanvas = document.createElement( 'canvas' ) - if ( formatInfo && ! testCanvas.toDataURL( formatInfo.mimeType ).startsWith( `data:${ formatInfo.mimeType }` ) ) { - // If not supported, skip conversion and return the original file - // eslint-disable-next-line no-console - console.error( '[Cimo] ' + format + ' is not supported by the browser, please use another modern browser' ) - return { file, metadata: null } + if ( format !== 'avif' ) { + const testCanvas = document.createElement( 'canvas' ) + if ( formatInfo && ! testCanvas.toDataURL( formatInfo.mimeType ).startsWith( `data:${ formatInfo.mimeType }` ) ) { + // If not supported, skip conversion and return the original file + // eslint-disable-next-line no-console + console.error( '[Cimo] ' + format + ' is not supported by the browser, please use another modern browser' ) + return { file, metadata: null } + } }
🤖 Fix all issues with AI agents
In `@src/shared/converters/image-converter.js`:
- Around line 182-183: The AVIF encoder call using encodeAvif(imageData, {
quality: avifQuality }) is wrong because `@jsquash/avif` expects cqLevel (0–63)
not quality; update the encodeAvif call in the image conversion flow (where
avifQuality and q are calculated) to pass cqLevel instead, mapping q (0–1) to
the 0–63 scale using the provided formula: cqLevel: Math.round((100 - (q ?? 0.5)
* 100) * 63 / 100), so that encodeAvif(imageData, { cqLevel: ... }) receives the
correct parameter.
| const avifQuality = Math.max( 1, Math.min( 100, Math.round( ( ( q ?? 0.5 ) * 100 ) ) ) ) | ||
| const avifBuffer = await encodeAvif( imageData, { quality: avifQuality } ) |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@jsquash/avif encode function quality parameter documentation
💡 Result:
@jsquash/avif’s encode(imageData, options) does not expose a quality option. Instead, AVIF “quality” is controlled via quantizer / constant-quality options—primarily cqLevel (and cqAlphaLevel for alpha). The package’s EncodeOptions type includes cqLevel, cqAlphaLevel, speed, subsample, etc. [1].
cqLevel: constant/constrained quality level on a 0–63 scale (lower = better). In libavif/libaom tooling, 0 is lossless, and the range is 0–63 [2].- If you’re looking for a JPEG-style 0–100 “quality” like
avifenc -q 75 ..., that’s a libavif CLI convenience flag, not the option used by@jsquash/avif’s JS API [3].
Practical mapping (if you have a 0–100 “quality” and want a cqLevel):
// quality: 0..100 where 100 ~= best
const cqLevel = Math.round((100 - quality) * 63 / 100);(This mapping is approximate; different encoders/settings can shift results.)
Sources: [1] EncodeOptions in jsquash-avif (cqLevel, cqAlphaLevel, etc.) https://cdn.jsdelivr.net/npm/jsquash-avif@1.0.2/codec/enc/avif_enc.d.ts • [2] libavif/libaom quantizer range notes (0–63, 0 lossless) https://github.com/vHeemstra/imagemin-avifenc • [3] libavif CLI example using -q https://github.com/AOMediaCodec/libavif
Fix AVIF quality parameter: replace quality with cqLevel.
@jsquash/avif's encode() does not support a quality parameter. It uses cqLevel on a 0–63 scale (where 0 is lossless). The current code passes { quality: avifQuality }, which is silently ignored, and the encoder uses its default settings instead.
Map quality (0–1) to cqLevel (0–63) with: cqLevel: Math.round((100 - (q ?? 0.5) * 100) * 63 / 100).
🤖 Prompt for AI Agents
In `@src/shared/converters/image-converter.js` around lines 182 - 183, The AVIF
encoder call using encodeAvif(imageData, { quality: avifQuality }) is wrong
because `@jsquash/avif` expects cqLevel (0–63) not quality; update the encodeAvif
call in the image conversion flow (where avifQuality and q are calculated) to
pass cqLevel instead, mapping q (0–1) to the 0–63 scale using the provided
formula: cqLevel: Math.round((100 - (q ?? 0.5) * 100) * 63 / 100), so that
encodeAvif(imageData, { cqLevel: ... }) receives the correct parameter.
There was a problem hiding this comment.
https://github.com/jamsinclair/jSquash/blob/main/packages/avif/codec/enc/avif_enc.d.ts#L7
@jsquash/avif's encode has a quality parameter
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
…id circular dependency
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Fix all issues with AI agents
In `@src/shared/converters/image-converter.js`:
- Around line 180-189: The AVIF branch (around encodeAvif / ctx.getImageData)
currently resolves or rejects without revoking the objectUrl or clearing the
canvas, causing a resource leak; wrap the encodeAvif block in a try/finally (or
move cleanup to a shared finally) and in the finally call
URL.revokeObjectURL(objectUrl) and free the canvas buffer (e.g.,
ctx.clearRect(0,0,width,height) and/or zero canvas.width/height) so both the
success path that creates avifBlob and the failure path that rejects with the
encode error perform the same cleanup; reference encodeAvif,
avifBlob/avifBuffer, ctx, objectUrl, resolve, and reject when locating where to
add the finally cleanup.
In `@src/shared/converters/index.js`:
- Around line 42-46: The call to isFormatSupported (which is async) inside
getFileConverter is not awaited, so the unsupported-format branch never runs;
update getFileConverter to handle this by either making getFileConverter async
and awaiting isFormatSupported before returning a NullConverter (adjusting all
call sites to await getFileConverter), or alternatively perform the async
support checks at startup to populate a synchronous lookup/cache (then change
the check to consult that cache synchronously) so the code that selects between
NullConverter and ImageConverter (referencing getFileConverter,
isFormatSupported, NullConverter, ImageConverter, and
window.cimoSettings?.imageOutputFormat) works correctly.
In `@src/shared/converters/util.js`:
- Around line 50-59: The canvas MIME support check currently returns early
inside the try block (when canvas.toDataURL succeeds), bypassing writing the
result to formatSupportCache and causing repeated checks; remove the early
return in the try block so that both the success path (after computing supported
via canvas.toDataURL and startsWith) and the catch error path set the local
supported value and then fall through to the shared cache-write that stores
formatSupportCache[mimeType] = supported; update the code around the
canvas.toDataURL/mimeType logic (the try/catch using supported and
formatSupportCache) to ensure the cache write executes on both success and
failure.
🧹 Nitpick comments (1)
src/shared/converters/util.js (1)
42-43: Nit: extra leading space.Line 43 has a double space before
supported.- supported = await supportsAvifDecode() + supported = await supportsAvifDecode()
| if ( outputFormat === 'avif' ) { | ||
| try { | ||
| const imageData = ctx.getImageData( 0, 0, width, height ) | ||
| const avifQuality = Math.max( 1, Math.min( 100, Math.round( ( ( q ?? 0.5 ) * 100 ) ) ) ) | ||
| const avifBuffer = await encodeAvif( imageData, { quality: avifQuality } ) | ||
| const avifBlob = new Blob( [ avifBuffer ], { type: 'image/avif' } ) | ||
| resolve( avifBlob ) | ||
| } catch ( e ) { | ||
| reject( new Error( `Failed to encode AVIF: ${ e instanceof Error ? e.message : 'Unknown error' }` ) ) | ||
| } |
There was a problem hiding this comment.
Resource leak: AVIF path skips objectUrl revocation and canvas cleanup.
The non-AVIF branch (lines 192-199) revokes the object URL and zeros the canvas to free memory, but the AVIF branch resolves/rejects without doing either. This leaks the blob URL and keeps the canvas buffer alive.
Proposed fix
if ( outputFormat === 'avif' ) {
try {
const imageData = ctx.getImageData( 0, 0, width, height )
const avifQuality = Math.max( 1, Math.min( 100, Math.round( ( ( q ?? 0.5 ) * 100 ) ) ) )
const avifBuffer = await encodeAvif( imageData, { quality: avifQuality } )
const avifBlob = new Blob( [ avifBuffer ], { type: 'image/avif' } )
+ // Clean up resources
+ URL.revokeObjectURL( objectUrl )
+ objectUrl = null
+ ctx.clearRect( 0, 0, canvas.width, canvas.height )
+ canvas.width = 0
+ canvas.height = 0
resolve( avifBlob )
} catch ( e ) {
+ URL.revokeObjectURL( objectUrl )
+ objectUrl = null
reject( new Error( `Failed to encode AVIF: ${ e instanceof Error ? e.message : 'Unknown error' }` ) )
}🤖 Prompt for AI Agents
In `@src/shared/converters/image-converter.js` around lines 180 - 189, The AVIF
branch (around encodeAvif / ctx.getImageData) currently resolves or rejects
without revoking the objectUrl or clearing the canvas, causing a resource leak;
wrap the encodeAvif block in a try/finally (or move cleanup to a shared finally)
and in the finally call URL.revokeObjectURL(objectUrl) and free the canvas
buffer (e.g., ctx.clearRect(0,0,width,height) and/or zero canvas.width/height)
so both the success path that creates avifBlob and the failure path that rejects
with the encode error perform the same cleanup; reference encodeAvif,
avifBlob/avifBuffer, ctx, objectUrl, resolve, and reject when locating where to
add the finally cleanup.
| const imageOutputFormat = window.cimoSettings?.imageOutputFormat || 'webp' | ||
| // If the browser doesn't support set output format, then we can't convert it. | ||
| if ( ! isFormatSupported( imageOutputFormat ) ) { | ||
| return new NullConverter( file ) | ||
| } |
There was a problem hiding this comment.
Critical: isFormatSupported is async but is not awaited — the check is always truthy.
isFormatSupported() returns a Promise. Without await, ! promise is always false, so the unsupported-format guard on line 44 never fires. Every format—including unsupported ones—will reach ImageConverter.
Since getFileConverter is synchronous, you'll need to either:
- Make
getFileConverterasyncandawaitthe call, updating all call sites, or - Pre-check format support at initialization (e.g., eagerly populate the cache) and expose a synchronous lookup.
🤖 Prompt for AI Agents
In `@src/shared/converters/index.js` around lines 42 - 46, The call to
isFormatSupported (which is async) inside getFileConverter is not awaited, so
the unsupported-format branch never runs; update getFileConverter to handle this
by either making getFileConverter async and awaiting isFormatSupported before
returning a NullConverter (adjusting all call sites to await getFileConverter),
or alternatively perform the async support checks at startup to populate a
synchronous lookup/cache (then change the check to consult that cache
synchronously) so the code that selects between NullConverter and ImageConverter
(referencing getFileConverter, isFormatSupported, NullConverter, ImageConverter,
and window.cimoSettings?.imageOutputFormat) works correctly.
| try { | ||
| const dataUrl = canvas.toDataURL( mimeType ) | ||
| // If the browser doesn't support the format, it falls back to image/png | ||
| // Check if the data URL starts with the requested mime type | ||
| supported = dataUrl.startsWith( `data:${ mimeType }` ) | ||
|
|
||
| return supported | ||
| } catch ( e ) { | ||
| supported = false | ||
| } |
There was a problem hiding this comment.
Bug: successful canvas-based check bypasses the cache.
Line 56 returns early without writing to formatSupportCache, so every subsequent call for a non-AVIF format (e.g., webp) re-runs the canvas test instead of hitting the cache. The error path (line 58) also doesn't cache. Only the AVIF path falls through to line 62.
Remove the early return so both the success and error paths fall through to the shared cache-write on line 62.
Proposed fix
try {
const dataUrl = canvas.toDataURL( mimeType )
// If the browser doesn't support the format, it falls back to image/png
// Check if the data URL starts with the requested mime type
supported = dataUrl.startsWith( `data:${ mimeType }` )
-
- return supported
} catch ( e ) {
supported = false
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| const dataUrl = canvas.toDataURL( mimeType ) | |
| // If the browser doesn't support the format, it falls back to image/png | |
| // Check if the data URL starts with the requested mime type | |
| supported = dataUrl.startsWith( `data:${ mimeType }` ) | |
| return supported | |
| } catch ( e ) { | |
| supported = false | |
| } | |
| try { | |
| const dataUrl = canvas.toDataURL( mimeType ) | |
| // If the browser doesn't support the format, it falls back to image/png | |
| // Check if the data URL starts with the requested mime type | |
| supported = dataUrl.startsWith( `data:${ mimeType }` ) | |
| } catch ( e ) { | |
| supported = false | |
| } |
🤖 Prompt for AI Agents
In `@src/shared/converters/util.js` around lines 50 - 59, The canvas MIME support
check currently returns early inside the try block (when canvas.toDataURL
succeeds), bypassing writing the result to formatSupportCache and causing
repeated checks; remove the early return in the try block so that both the
success path (after computing supported via canvas.toDataURL and startsWith) and
the catch error path set the local supported value and then fall through to the
shared cache-write that stores formatSupportCache[mimeType] = supported; update
the code around the canvas.toDataURL/mimeType logic (the try/catch using
supported and formatSupportCache) to ensure the cache write executes on both
success and failure.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@src/admin/js/media-manager/drop-zone.js`:
- Around line 37-38: The drop handler is calling await Promise.all(...) before
calling event.preventDefault()/stopPropagation(), causing a race where native
drop behavior can occur; move event.preventDefault() and event.stopPropagation()
to immediately after entering the handler (before the first await), then proceed
to compute files and call getFileConverter(...) / Promise.all; if you determine
after conversion that no custom processing is needed, re-dispatch the original
event (or synthesize a new one) so native handling can continue. Update the code
around the drop handler where files, fileConverters, and getFileConverter are
used to reflect this ordering and add the re-dispatch fallback.
In `@src/admin/js/media-manager/select-files.js`:
- Around line 37-38: The change handler currently awaits
Promise.all(files.map(file => getFileConverter(file))) before calling
event.preventDefault() and event.stopImmediatePropagation(), which lets
WordPress process the original change and causes double uploads; move the calls
to event.preventDefault() and event.stopImmediatePropagation() to the top of the
handler (i.e., immediately after entering the handler and before the first
await/Promise.all) so the native upload is suppressed, then perform the async
getFileConverter/Promise.all work and dispatch the synthetic upload; apply the
same fix in the analogous handler in drop-zone.js.
🧹 Nitpick comments (1)
src/shared/converters/index.js (1)
6-12: JSDoc return type should reflect the async signature.
getFileConverteris nowasync, so it returns aPromise<Converter>, but the JSDoc still says@return {Converter}.- * `@return` {Converter} - The file converter. + * `@return` {Promise<Converter>} - The file converter.
| const files = Array.from( event.dataTransfer.files ) | ||
| const fileConverters = await Promise.all( files.map( file => getFileConverter( file ) ) ) |
There was a problem hiding this comment.
Critical: same await-before-preventDefault race condition as in select-files.js.
event.preventDefault() on line 65 is unreachable until after the await Promise.all(...) on line 38 resolves. By then the browser has already finished dispatching the drop event — native drop handling (and potentially navigation to the dropped file) will have already occurred.
Move preventDefault() / stopPropagation() before the first await, and re-dispatch the event if conversion turns out to be unnecessary.
Proposed fix (same pattern as select-files.js)
const customDropHandler = async event => {
if ( event.__cimo_converted ) {
return
}
const files = Array.from( event.dataTransfer.files )
+
+ // Must stop the event synchronously before any await
+ event.preventDefault()
+ event.stopPropagation()
+
const fileConverters = await Promise.all( files.map( file => getFileConverter( file ) ) )
if ( ! requiresFileConversion( fileConverters ) ) {
+ // No conversion needed — re-dispatch the original drop
+ const dropEvent = new DragEvent( 'drop', { bubbles: true } )
+ Object.defineProperty( dropEvent, 'dataTransfer', {
+ value: event.dataTransfer,
+ writable: false,
+ } )
+ dropEvent.__cimo_converted = true
+ event.target.dispatchEvent( dropEvent )
return
}
...
- event.preventDefault()
- event.stopPropagation()Also applies to: 65-66
🤖 Prompt for AI Agents
In `@src/admin/js/media-manager/drop-zone.js` around lines 37 - 38, The drop
handler is calling await Promise.all(...) before calling
event.preventDefault()/stopPropagation(), causing a race where native drop
behavior can occur; move event.preventDefault() and event.stopPropagation() to
immediately after entering the handler (before the first await), then proceed to
compute files and call getFileConverter(...) / Promise.all; if you determine
after conversion that no custom processing is needed, re-dispatch the original
event (or synthesize a new one) so native handling can continue. Update the code
around the drop handler where files, fileConverters, and getFileConverter are
used to reflect this ordering and add the re-dispatch fallback.
| const files = Array.from( event.target.files ) | ||
| const fileConverters = await Promise.all( files.map( file => getFileConverter( file ) ) ) |
There was a problem hiding this comment.
Critical: event.preventDefault() is called after await — the event will have already propagated, causing double uploads.
After the await Promise.all(...) on line 38, the synchronous portion of this async handler returns and the browser finishes dispatching the event. By the time execution resumes and reaches preventDefault() / stopImmediatePropagation() on lines 64–66, the original change event has already been fully processed by WordPress's native upload handler. This means the unconverted files get uploaded by WP and the converted files get uploaded by your synthetic event — a double-upload regression.
Before this PR getFileConverter was synchronous, so the preventDefault was reached synchronously. Now that it's async, you must call preventDefault before the first await.
Proposed fix
const selectFilesListener = async event => {
// Check if it's a file select.
if ( event.target.type !== 'file' ) {
return
}
// If this is a synthetic change event dispatched by us after conversion, skip conversion.
if ( event.__cimo_converted ) {
return
}
// Get the file converters for the incoming files.
const files = Array.from( event.target.files )
+
+ // Must stop the event synchronously before any await,
+ // otherwise the browser will finish dispatching the event
+ // while we're waiting for the async format-support check.
+ event.preventDefault()
+ event.stopPropagation()
+ event.stopImmediatePropagation()
+
const fileConverters = await Promise.all( files.map( file => getFileConverter( file ) ) )
// Do not continue if we do not need to convert any files.
if ( ! requiresFileConversion( fileConverters ) ) {
+ // No conversion needed — re-dispatch the original event
+ const changeEvent = new Event( 'change', { bubbles: true } )
+ changeEvent.__cimo_converted = true
+ event.target.dispatchEvent( changeEvent )
return
}
...
- // Prevent the default file handling
- event.preventDefault()
- event.stopPropagation()
- event.stopImmediatePropagation()Note: The same issue exists in src/admin/js/media-manager/drop-zone.js at lines 37–38 / 65–66.
Also applies to: 64-66
🤖 Prompt for AI Agents
In `@src/admin/js/media-manager/select-files.js` around lines 37 - 38, The change
handler currently awaits Promise.all(files.map(file => getFileConverter(file)))
before calling event.preventDefault() and event.stopImmediatePropagation(),
which lets WordPress process the original change and causes double uploads; move
the calls to event.preventDefault() and event.stopImmediatePropagation() to the
top of the handler (i.e., immediately after entering the handler and before the
first await/Promise.all) so the native upload is suppressed, then perform the
async getFileConverter/Promise.all work and dispatch the synthetic upload; apply
the same fix in the analogous handler in drop-zone.js.
1 participant
fixes #27
Summary by CodeRabbit
New Features
Improvements
Chores