feat: add heic support #23
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds WordPress hook integration to the image converter: Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ImageConverter
participant WP_Hooks as applyFilters
participant ConversionCore
User->>ImageConverter: upload file / request convert
ImageConverter->>WP_Hooks: applyFilters('cimo.imageConverter.mimeTypes', mimeTypes)
WP_Hooks-->>ImageConverter: (possibly modified) mimeTypes
ImageConverter->>WP_Hooks: applyFilters('cimo.convertImage.prepare', fileItem.file, outputFormat, options)
WP_Hooks-->>ImageConverter: (possibly modified) file (or original)
ImageConverter->>ConversionCore: perform conversion with final file
ConversionCore-->>ImageConverter: converted output
ImageConverter-->>User: deliver converted image
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 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 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
🤖 Fix all issues with AI agents
In `@src/shared/converters/image-converter.js`:
- Around line 133-149: Wrap the HEIC conversion step so failures from
HEICConverter.convert() are caught and re-thrown (or logged) with an
HEIC-specific error message; locate the HEIC branch using
applyFilters('cimo.getHEICConverter', ...) and the HEICConverter.convert() call
and replace the bare await with a try/catch that captures any error and throws a
new Error (or augment the original) like "HEIC conversion failed: <original
error message>" so the outer catch can surface an HEIC-specific failure instead
of the generic "Failed to convert image".
- Line 23: The converter currently only lists 'image/heic' as a HEIC MIME type;
expand the HEIC/HEIF MIME set to include 'image/heif', 'image/heic-sequence',
and 'image/heif-sequence' in the MIME array (the array that contains
'image/heic' in image-converter.js), and update the conditional that checks for
HEIC input (the check around the HEIC conversion decision, referenced near line
135) to test against this expanded set rather than a single literal 'image/heic'
string so all HEIC/HEIF variants trigger conversion.
| 'image/png', | ||
| 'image/webp', | ||
| 'image/jpg', | ||
| 'image/heic', |
There was a problem hiding this comment.
Consider supporting additional HEIC/HEIF MIME type variants.
HEIC files may also be reported with MIME types image/heif or sequence variants (image/heic-sequence, image/heif-sequence). The current implementation only handles image/heic, which could cause some valid HEIC/HEIF files to be passed through without conversion.
Suggested fix
return [
'image/jpeg',
'image/png',
'image/webp',
'image/jpg',
'image/heic',
+ 'image/heif',
]And update the check at line 135:
- if ( fileItem.file.type === 'image/heic' ) {
+ if ( fileItem.file.type === 'image/heic' || fileItem.file.type === 'image/heif' ) {🤖 Prompt for AI Agents
In `@src/shared/converters/image-converter.js` at line 23, The converter currently
only lists 'image/heic' as a HEIC MIME type; expand the HEIC/HEIF MIME set to
include 'image/heif', 'image/heic-sequence', and 'image/heif-sequence' in the
MIME array (the array that contains 'image/heic' in image-converter.js), and
update the conditional that checks for HEIC input (the check around the HEIC
conversion decision, referenced near line 135) to test against this expanded set
rather than a single literal 'image/heic' string so all HEIC/HEIF variants
trigger conversion.
| // If file type is HEIC, convert first to PNG | ||
| // then proceed to regular conversion | ||
| if ( fileItem.file.type === 'image/heic' ) { | ||
| const HEICConverter = applyFilters( | ||
| 'cimo.getHEICConverter', | ||
| null, | ||
| fileItem.file, | ||
| { format: 'png', quality: 1 } | ||
| ) | ||
|
|
||
| if ( ! HEICConverter ) { | ||
| throw new Error( `HEIC converter is not found. HEIC conversion is only available in premium version.` ) | ||
| } | ||
|
|
||
| const optimizedResult = await HEICConverter.convert() | ||
| fileItem.file = optimizedResult.file | ||
| } |
There was a problem hiding this comment.
Add specific error handling for HEIC conversion failure.
The HEICConverter.convert() call lacks specific error handling. If the HEIC conversion fails, the error will propagate to the outer catch block at line 321-323, resulting in a generic "Failed to convert image" message rather than indicating an HEIC-specific failure.
Suggested fix
if ( fileItem.file.type === 'image/heic' ) {
const HEICConverter = applyFilters(
'cimo.getHEICConverter',
null,
fileItem.file,
{ format: 'png', quality: 1 }
)
if ( ! HEICConverter ) {
throw new Error( `HEIC converter is not found. HEIC conversion is only available in premium version.` )
}
- const optimizedResult = await HEICConverter.convert()
- fileItem.file = optimizedResult.file
+ try {
+ const optimizedResult = await HEICConverter.convert()
+ fileItem.file = optimizedResult.file
+ } catch ( heicError ) {
+ throw new Error( `Failed to convert HEIC image: ${ heicError.message }` )
+ }
}📝 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.
| // If file type is HEIC, convert first to PNG | |
| // then proceed to regular conversion | |
| if ( fileItem.file.type === 'image/heic' ) { | |
| const HEICConverter = applyFilters( | |
| 'cimo.getHEICConverter', | |
| null, | |
| fileItem.file, | |
| { format: 'png', quality: 1 } | |
| ) | |
| if ( ! HEICConverter ) { | |
| throw new Error( `HEIC converter is not found. HEIC conversion is only available in premium version.` ) | |
| } | |
| const optimizedResult = await HEICConverter.convert() | |
| fileItem.file = optimizedResult.file | |
| } | |
| // If file type is HEIC, convert first to PNG | |
| // then proceed to regular conversion | |
| if ( fileItem.file.type === 'image/heic' ) { | |
| const HEICConverter = applyFilters( | |
| 'cimo.getHEICConverter', | |
| null, | |
| fileItem.file, | |
| { format: 'png', quality: 1 } | |
| ) | |
| if ( ! HEICConverter ) { | |
| throw new Error( `HEIC converter is not found. HEIC conversion is only available in premium version.` ) | |
| } | |
| try { | |
| const optimizedResult = await HEICConverter.convert() | |
| fileItem.file = optimizedResult.file | |
| } catch ( heicError ) { | |
| throw new Error( `Failed to convert HEIC image: ${ heicError.message }` ) | |
| } | |
| } |
🤖 Prompt for AI Agents
In `@src/shared/converters/image-converter.js` around lines 133 - 149, Wrap the
HEIC conversion step so failures from HEICConverter.convert() are caught and
re-thrown (or logged) with an HEIC-specific error message; locate the HEIC
branch using applyFilters('cimo.getHEICConverter', ...) and the
HEICConverter.convert() call and replace the bare await with a try/catch that
captures any error and throws a new Error (or augment the original) like "HEIC
conversion failed: <original error message>" so the outer catch can surface an
HEIC-specific failure instead of the generic "Failed to convert image".
There was a problem hiding this comment.
Error handling for HEIC conversion is done here: https://github.com/bfintal/Cimo-Premium/pull/4/changes#diff-4e092d99aaf69f51fd49dd138c20e7f59fa3b49e8683abbc023b3d721718eafb
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Try to access the premium repository PR to verify error handling
gh pr view 4 --repo bfintal/Cimo-Premium --json url,title,state 2>&1 || echo "Repository not accessible"
# Also try to fetch the specific diff URL
curl -s -o /dev/null -w "%{http_code}" "https://github.com/bfintal/Cimo-Premium/pull/4/changes" || echo "Failed to access URL"
Length of output: 123
@Arukuen, thank you for the clarification! I can see that the premium repository is private (I'm unable to access it), but I understand that error handling for HEIC conversion is implemented within the premium HEICConverter class itself.
That makes sense architecturally—if the HEICConverter.convert() method already handles errors appropriately and throws meaningful error messages, then additional wrapping in this file would be redundant. The error would naturally propagate to the outer catch block at line 321-323 with the proper HEIC-specific context.
I'll withdraw my suggestion. Your approach of handling errors within the premium converter implementation is a cleaner separation of concerns.
| // If file type is HEIC, convert first to PNG | ||
| // then proceed to regular conversion | ||
| if ( fileItem.file.type === 'image/heic' ) { | ||
| const HEICConverter = applyFilters( | ||
| 'cimo.getHEICConverter', | ||
| null, | ||
| fileItem.file, | ||
| { format: 'png', quality: 1 } | ||
| ) | ||
|
|
||
| if ( ! HEICConverter ) { | ||
| throw new Error( `HEIC converter is not found. HEIC conversion is only available in premium version.` ) | ||
| } | ||
|
|
||
| const optimizedResult = await HEICConverter.convert() | ||
| fileItem.file = optimizedResult.file | ||
| } |
There was a problem hiding this comment.
2 things:
- I would recommend moving this entire logic in the premium codebase and replacing this with a filter:
const modifiedFile = applyFilters( 'cimo.convertImage.prepare', fileItem, outputFormat, options )
if ( modifiedFile ) {
fileItem.file = modifiedFile
}- There is a
throwhere, make sure that the conversion (or the falling back to no conversion) is carried out if the error is thrown.
| 'image/webp', | ||
| 'image/jpg', | ||
| 'image/heic', | ||
| ] |
There was a problem hiding this comment.
use a filter here and our premium code will hook into it:
return applyFilters( 'cimo.imageConverter.mimeTypes', [
'image/jpeg',
// ...
] )
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/shared/converters/image-converter.js`:
- Around line 132-135: applyFilters is being called with the whole fileItem
instead of the underlying Blob/File so when no filters are registered
applyFilters returns fileItem and the code assigns fileItem.file = fileItem
corrupting the blob; change the call to pass fileItem.file to applyFilters (i.e.
call applyFilters('cimo.convertImage.prepare', fileItem.file, outputFormat,
options)), then if a modifiedFile is returned assign it back to fileItem.file
(keep the existing conditional that assigns modifiedFile to fileItem.file) so
URL.createObjectURL(fileItem.file) receives a proper Blob/File.
| } | ||
|
|
||
| // Allow preprocessing of the image before the conversion | ||
| fileItem.file = await applyFilters( 'cimo.convertImage.prepare', fileItem.file, outputFormat, options ) |
There was a problem hiding this comment.
Let's only mutate the value if it was changed
| fileItem.file = await applyFilters( 'cimo.convertImage.prepare', fileItem.file, outputFormat, options ) | |
| const updatedFile = await applyFilters( 'cimo.convertImage.prepare', fileItem.file, outputFormat, options ) | |
| if ( updatedFile ) { | |
| fileItem.file = updatedFile | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@src/shared/converters/image-converter.js`:
- Around line 132-136: Replace the synchronous applyFilters usage with the async
hook helper: change the call that assigns updatedFile (the line using
applyFilters('cimo.convertImage.prepare', fileItem.file, outputFormat, options)
awaited) to use applyFiltersAsync instead so chained async filters receive
resolved values, and update the import to pull applyFiltersAsync from
`@wordpress/hooks` (locate the usage in image conversion/preprocessing around
updatedFile assignment in src/shared/converters/image-converter.js).
| // Allow preprocessing of the image before the conversion | ||
| const updatedFile = await applyFilters( 'cimo.convertImage.prepare', fileItem.file, outputFormat, options ) | ||
| if ( updatedFile ) { | ||
| fileItem.file = updatedFile | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
@wordpress/hooks applyFilters async filter callback support
💡 Result:
@wordpress/hooks does not make applyFilters() await async (Promise-returning) filter callbacks—applyFilters() is the synchronous API. If a filter callback is async / returns a Promise, applyFilters() will just receive/pass along that Promise like a normal value; it will not wait for it to resolve. [1]
If you need “async filter callback support”, use applyFiltersAsync(), which is the async counterpart specifically added for running hooks in async mode (along with doActionAsync()). [1] [2]
Example:
import { addFilter, applyFiltersAsync } from '@wordpress/hooks';
addFilter('my.hook', 'my-plugin/ns', async (value) => {
const extra = await fetch('/api').then(r => r.text());
return value + extra;
});
const result = await applyFiltersAsync('my.hook', 'start: ');Sources:
[1] WordPress Block Editor Handbook – @wordpress/hooks package reference (lists applyFilters() and applyFiltersAsync()) (developer.wordpress.org)
[2] @wordpress/hooks changelog noting addition of doActionAsync / applyFiltersAsync (app.unpkg.com)
Citations:
- 1: https://developer.wordpress.org/block-editor/reference-guides/packages/packages-hooks/?utm_source=openai
- 2: https://app.unpkg.com/%40wordpress/hooks%404.38.0/files/CHANGELOG.md?utm_source=openai
🏁 Script executed:
cd src/shared/converters && cat -n image-converter.js | sed -n '130,140p'Repository: gambitph/Cimo
Length of output: 465
🏁 Script executed:
rg "applyFilters|applyFiltersAsync" --type js -A 2 -B 2Repository: gambitph/Cimo
Length of output: 2545
🏁 Script executed:
rg "cimo.convertImage.prepare" --type js -A 3 -B 1Repository: gambitph/Cimo
Length of output: 489
🏁 Script executed:
rg "addFilter.*cimo.convertImage.prepare" --type js -A 5Repository: gambitph/Cimo
Length of output: 39
🏁 Script executed:
rg "cimo.convertImage.prepare" --type js -A 3 -B 2Repository: gambitph/Cimo
Length of output: 531
Use applyFiltersAsync() instead of applyFilters() with await for proper async filter support.
@wordpress/hooks provides applyFiltersAsync() specifically for async filter callbacks. The current code uses applyFilters() (synchronous) with await, which works for a single async filter but breaks when multiple async filters are chained—the second filter receives an unresolved Promise from the first instead of the resolved value.
Replace applyFilters() with applyFiltersAsync() to properly handle async filter chains:
Suggested fix
- const updatedFile = await applyFilters( 'cimo.convertImage.prepare', fileItem.file, outputFormat, options )
+ const updatedFile = await applyFiltersAsync( 'cimo.convertImage.prepare', fileItem.file, outputFormat, options )Also update the import:
- import { applyFilters } from '@wordpress/hooks'
+ import { applyFiltersAsync } from '@wordpress/hooks'🤖 Prompt for AI Agents
In `@src/shared/converters/image-converter.js` around lines 132 - 136, Replace the
synchronous applyFilters usage with the async hook helper: change the call that
assigns updatedFile (the line using applyFilters('cimo.convertImage.prepare',
fileItem.file, outputFormat, options) awaited) to use applyFiltersAsync instead
so chained async filters receive resolved values, and update the import to pull
applyFiltersAsync from `@wordpress/hooks` (locate the usage in image
conversion/preprocessing around updatedFile assignment in
src/shared/converters/image-converter.js).
2 participants
fixes #14
Summary by CodeRabbit