Conversation
📝 WalkthroughWalkthroughAdds bulk image optimization support: new dependencies, admin localization flags, metadata and stats handling for bulk operations, bulk-optimizer admin UI and styles, enhanced client-side file conversion with options/error handling, and new ImageConverter optimize API. Changes
Sequence DiagramsequenceDiagram
actor User
participant UI as Bulk Optimizer UI
participant FileProc as File Processor (drop/select)
participant Converter as ImageConverter
participant Stats as Cimo_Stats
participant Admin as Backend (Meta/Options)
User->>UI: Select or drop files
UI->>FileProc: Start processing files
FileProc->>Converter: convert(force, options)
alt conversion error or unsupported
Converter-->>FileProc: { error, reason, message }
FileProc-->>FileProc: log error, mark hasError
else conversion success
Converter-->>FileProc: { file, metadata }
FileProc->>Admin: attach optimized_during_upload flag / save metadata
FileProc->>Stats: update_stats_bulk_optimized( attachment, originalKB, optimizedKB )
Stats-->>Admin: update_option(stats)
end
UI->>UI: update progress / animations
UI-->>User: show results and totals
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Stylelint (17.4.0)src/admin/css/index.cssConfigurationError: Could not find "@wordpress/stylelint-config". Do you need to install the package or use the "configBasedir" option? 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 Tip CodeRabbit can use your project's `biome` configuration to improve the quality of JS/TS/CSS/JSON code reviews.Add a configuration file to your project to customize how CodeRabbit runs |
🤖 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: 4
🧹 Nitpick comments (3)
src/admin/class-stats.php (2)
183-189: Minor: Unused$attachment_idparameter.Static analysis notes this parameter is unused. If reserved for future tracking, prefix with underscore to silence warnings.
- public static function update_stats_bulk_optimized( $attachment_id, $original_size, $optimized_size ) { + public static function update_stats_bulk_optimized( $_attachment_id, $original_size, $optimized_size ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/admin/class-stats.php` around lines 183 - 189, The parameter $attachment_id in the static method update_stats_bulk_optimized is unused and triggers static-analysis warnings; to silence them, rename the parameter to $_attachment_id (or otherwise prefix it with an underscore) in the method signature and any callers, leaving the body unchanged; reference: update_stats_bulk_optimized and its parameter $attachment_id.
93-93: Minor: Unused loop variable$size.Static analysis flagged that
$sizeis unused in the loop body. If it's not needed, use$_or just iterate values.- foreach ( $cimo_data['bulk_optimization'] as $size => $data ) { + foreach ( $cimo_data['bulk_optimization'] as $data ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/admin/class-stats.php` at line 93, The foreach over $cimo_data['bulk_optimization'] declares an unused loop key $size; update the loop in src/admin/class-stats.php to avoid the unused variable by iterating only values (e.g., change the foreach signature that currently uses $size to use just $data) or explicitly use an underscore key (e.g., $_ => $data) so static analysis no longer flags $size as unused while preserving the existing loop body that references $data.src/shared/converters/image-converter.js (1)
360-363: Consider documenting the expected filter contract.The
optimize()method delegates to a WordPress filter, but the expected return type and filter behavior are undocumented. Consider adding a JSDoc comment explaining what filters should return.📝 Suggested documentation
+ /** + * Optimize the image using registered filters. + * Filters receive this ImageConverter instance and should return + * an object with { file, metadata } or the original instance if no optimization is performed. + * `@return` {Promise<Object>} Promise resolving to optimization result. + */ async optimize() { return await applyFiltersAsync( 'cimo.imageConverter.optimize', this ) }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/shared/converters/image-converter.js` around lines 360 - 363, The optimize() method delegates to the WordPress filter 'cimo.imageConverter.optimize' via applyFiltersAsync(this), but there is no JSDoc describing the expected filter contract; add a JSDoc block above the optimize() method that documents the expected input (this ImageConverter instance), the expected return type (e.g. a Promise resolving to void, an updated ImageConverter, or a specific result object), whether filters should mutate the instance or return a new one, and any error/exception behavior; reference the optimize() method name and the filter hook 'cimo.imageConverter.optimize' in the comment so implementers know what to implement when hooking into applyFiltersAsync.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/admin/class-metadata.php`:
- Around line 162-164: You added an associative key 'optimized_during_upload' =>
true into $sanitized_metadata (a numerically-indexed array), which mixes array
types and breaks consumers like add_attachment_metadata() that expect each item
to be an array with a 'filename' key; instead, either (A) push a properly
structured metadata entry into $sanitized_metadata (e.g., an array with
'filename' and a flag field) so iteration still yields arrays, or (B) keep the
boolean flag separate (e.g., set it on the attachment post meta or a separate
variable/property) and do not inject an associative key into
$sanitized_metadata; update any usages in add_attachment_metadata(),
class-meta-box.php, and class-stats.php to read the flag from the new location.
In `@src/admin/class-stats.php`:
- Around line 198-204: In update_stats_bulk_restored, the two size fields are
using the wrong parameters: swap the size arithmetic so total_optimized_size
subtracts $optimized_size/1024 and total_original_size is adjusted only
according to how originals were tracked during optimization (e.g. subtract
$restored_size/1024 if originals were previously added, otherwise leave
unchanged) — update the lines in update_stats_bulk_restored to use
$optimized_size and $restored_size in the correct places (reference function
update_stats_bulk_restored and variables $optimized_size, $restored_size,
$stats['total_original_size'], $stats['total_optimized_size']); also address the
unused $attachment_id by either prefixing it with an underscore or adding a //
phpcs:ignore comment.
- Around line 87-106: The code double-counts media_optimized_num because it
increments inside the foreach over $cimo_data['bulk_optimization'] and then
unconditionally again after that block; update the logic around $cimo_data and
$updated_stats so the final $updated_stats['media_optimized_num']++ only runs
for individually optimized images (i.e., when bulk_optimization is not present
or empty). Concretely, use an if/else or guard that checks
isset($cimo_data['bulk_optimization']) (and non-empty if appropriate) and only
perform the standalone increment when no bulk_optimization entries were
processed, leaving the increments inside the foreach unchanged.
In `@src/shared/converters/image-converter.js`:
- Around line 229-231: The format normalization can leave formatTo as an
unsupported string (e.g., "jpeg") which causes formatInfo to be undefined later;
update the logic around formatTo (derived from options.format and
this.options?.format) to canonicalize common aliases (e.g., "jpeg" -> "jpg")
and/or map "image/..." MIME types to supportedFormats values, then look up
supportedFormats to get formatInfo and if lookup returns undefined fall back to
a safe default like 'webp' (and log or throw a clear error if appropriate) so
that any subsequent use of formatInfo.mimeType in the image conversion flow
cannot dereference undefined.
---
Nitpick comments:
In `@src/admin/class-stats.php`:
- Around line 183-189: The parameter $attachment_id in the static method
update_stats_bulk_optimized is unused and triggers static-analysis warnings; to
silence them, rename the parameter to $_attachment_id (or otherwise prefix it
with an underscore) in the method signature and any callers, leaving the body
unchanged; reference: update_stats_bulk_optimized and its parameter
$attachment_id.
- Line 93: The foreach over $cimo_data['bulk_optimization'] declares an unused
loop key $size; update the loop in src/admin/class-stats.php to avoid the unused
variable by iterating only values (e.g., change the foreach signature that
currently uses $size to use just $data) or explicitly use an underscore key
(e.g., $_ => $data) so static analysis no longer flags $size as unused while
preserving the existing loop body that references $data.
In `@src/shared/converters/image-converter.js`:
- Around line 360-363: The optimize() method delegates to the WordPress filter
'cimo.imageConverter.optimize' via applyFiltersAsync(this), but there is no
JSDoc describing the expected filter contract; add a JSDoc block above the
optimize() method that documents the expected input (this ImageConverter
instance), the expected return type (e.g. a Promise resolving to void, an
updated ImageConverter, or a specific result object), whether filters should
mutate the instance or return a new one, and any error/exception behavior;
reference the optimize() method name and the filter hook
'cimo.imageConverter.optimize' in the comment so implementers know what to
implement when hooking into applyFiltersAsync.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 3daa2999-3f41-463c-a3d5-042f6c705705
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (10)
package.jsonreadme.txtsrc/admin/class-admin.phpsrc/admin/class-metadata.phpsrc/admin/class-stats.phpsrc/admin/css/admin-page.csssrc/admin/js/media-manager/drop-zone.jssrc/admin/js/media-manager/select-files.jssrc/admin/js/page/admin-settings.jssrc/shared/converters/image-converter.js
Comment on lines
+162
to
+164
| // Add our metadata key to note that this was optimized during upload. | ||
| $sanitized_metadata['optimized_during_upload'] = true; | ||
|
|
There was a problem hiding this comment.
Data structure inconsistency: mixing non-numeric key into a numerically-indexed array.
$sanitized_metadata is a numerically-indexed array of metadata entries. Adding 'optimized_during_upload' => true as an associative key creates a mixed array that will cause issues downstream:
-
In
add_attachment_metadata()(line 215), theforeachiterates expecting each$itemto be an array with a'filename'key. When it encounters the booleantruevalue,isset($item['filename'])will fail or produce unexpected behavior. -
This could also affect consumers in
class-meta-box.phpandclass-stats.phpthat expect consistent data structures.
Consider storing this flag separately or restructuring to avoid mixing numeric and associative keys:
Proposed fix
- // Add our metadata key to note that this was optimized during upload.
- $sanitized_metadata['optimized_during_upload'] = true;
-
// Save to a transient queue for metadata waiting for attachment creation
$transient_key = 'cimo_metadata_queue';
$queue = get_transient( $transient_key );
if ( ! is_array( $queue ) ) {
$queue = [];
}
// Append new metadata to the queue
- $queue = array_merge( $queue, $sanitized_metadata );
+ // Mark each entry as optimized during upload
+ foreach ( $sanitized_metadata as &$entry ) {
+ $entry['optimized_during_upload'] = true;
+ }
+ $queue = array_merge( $queue, $sanitized_metadata );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/admin/class-metadata.php` around lines 162 - 164, You added an
associative key 'optimized_during_upload' => true into $sanitized_metadata (a
numerically-indexed array), which mixes array types and breaks consumers like
add_attachment_metadata() that expect each item to be an array with a 'filename'
key; instead, either (A) push a properly structured metadata entry into
$sanitized_metadata (e.g., an array with 'filename' and a flag field) so
iteration still yields arrays, or (B) keep the boolean flag separate (e.g., set
it on the attachment post meta or a separate variable/property) and do not
inject an associative key into $sanitized_metadata; update any usages in
add_attachment_metadata(), class-meta-box.php, and class-stats.php to read the
flag from the new location.
Comment on lines
+87
to
106
|
|
||
| /** | ||
| * This data comes from bulk optimizing media in the Media Library: | ||
| */ | ||
|
|
||
| if ( isset( $cimo_data['bulk_optimization'] ) ) { | ||
| foreach ( $cimo_data['bulk_optimization'] as $size => $data ) { | ||
| $updated_stats['media_optimized_num']++; | ||
| $original_size_b = isset( $data['originalFilesize'] ) ? (int) $data['originalFilesize'] : 0; | ||
| $converted_size_b = isset( $data['convertedFilesize'] ) ? (int) $data['convertedFilesize'] : 0; | ||
| $updated_stats['total_original_size'] += $original_size_b / 1024; | ||
| $updated_stats['total_optimized_size'] += $converted_size_b / 1024; | ||
| } | ||
| } | ||
|
|
||
| /** | ||
| * This data comes from individually optimized images from the user uploading media: | ||
| */ | ||
|
|
||
| $updated_stats['media_optimized_num']++; |
There was a problem hiding this comment.
Double-counting issue: media_optimized_num is incremented both inside and outside the bulk optimization loop.
When bulk_optimization data exists, the code increments media_optimized_num for each entry inside the loop (line 94), but then also unconditionally increments it again at line 106. This results in double-counting.
Proposed fix: only count individually optimized images when bulk_optimization is absent
if ( isset( $cimo_data['bulk_optimization'] ) ) {
foreach ( $cimo_data['bulk_optimization'] as $size => $data ) {
$updated_stats['media_optimized_num']++;
$original_size_b = isset( $data['originalFilesize'] ) ? (int) $data['originalFilesize'] : 0;
$converted_size_b = isset( $data['convertedFilesize'] ) ? (int) $data['convertedFilesize'] : 0;
$updated_stats['total_original_size'] += $original_size_b / 1024;
$updated_stats['total_optimized_size'] += $converted_size_b / 1024;
}
- }
-
- /**
- * This data comes from individually optimized images from the user uploading media:
- */
-
- $updated_stats['media_optimized_num']++;
-
- // Extract file sizes (bytes) and add to KB totals
- $original_size_b = isset( $cimo_data['originalFilesize'] ) ? (int) $cimo_data['originalFilesize'] : 0;
- $converted_size_b = isset( $cimo_data['convertedFilesize'] ) ? (int) $cimo_data['convertedFilesize'] : 0;
-
- $updated_stats['total_original_size'] += $original_size_b / 1024;
- $updated_stats['total_optimized_size'] += $converted_size_b / 1024;
+ } else {
+ /**
+ * This data comes from individually optimized images from the user uploading media:
+ */
+ $updated_stats['media_optimized_num']++;
+
+ // Extract file sizes (bytes) and add to KB totals
+ $original_size_b = isset( $cimo_data['originalFilesize'] ) ? (int) $cimo_data['originalFilesize'] : 0;
+ $converted_size_b = isset( $cimo_data['convertedFilesize'] ) ? (int) $cimo_data['convertedFilesize'] : 0;
+
+ $updated_stats['total_original_size'] += $original_size_b / 1024;
+ $updated_stats['total_optimized_size'] += $converted_size_b / 1024;
+ }🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 93-93: Avoid unused local variables such as '$size'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/admin/class-stats.php` around lines 87 - 106, The code double-counts
media_optimized_num because it increments inside the foreach over
$cimo_data['bulk_optimization'] and then unconditionally again after that block;
update the logic around $cimo_data and $updated_stats so the final
$updated_stats['media_optimized_num']++ only runs for individually optimized
images (i.e., when bulk_optimization is not present or empty). Concretely, use
an if/else or guard that checks isset($cimo_data['bulk_optimization']) (and
non-empty if appropriate) and only perform the standalone increment when no
bulk_optimization entries were processed, leaving the increments inside the
foreach unchanged.
Comment on lines
+198
to
+204
| public static function update_stats_bulk_restored( $attachment_id, $optimized_size, $restored_size ) { | ||
| $stats = self::get_stats(); | ||
| $stats['media_optimized_num']--; | ||
| $stats['total_original_size'] -= $optimized_size / 1024; | ||
| $stats['total_optimized_size'] -= $restored_size / 1024; | ||
| update_option( self::OPTION_KEY, $stats, false ); | ||
| } |
There was a problem hiding this comment.
Verify the restore stats logic: parameter usage appears inverted.
The subtraction logic seems backwards:
total_original_size -= $optimized_size— but when restoring, the original size should remain unchanged or increasetotal_optimized_size -= $restored_size— this should subtract the optimized size being removed
Also, $attachment_id is unused per static analysis. If it's reserved for future use, consider adding a // phpcs:ignore comment or prefixing with underscore.
Suggested fix (verify intended behavior)
- public static function update_stats_bulk_restored( $attachment_id, $optimized_size, $restored_size ) {
+ public static function update_stats_bulk_restored( $_attachment_id, $optimized_size, $restored_size ) {
$stats = self::get_stats();
$stats['media_optimized_num']--;
- $stats['total_original_size'] -= $optimized_size / 1024;
- $stats['total_optimized_size'] -= $restored_size / 1024;
+ $stats['total_original_size'] -= $restored_size / 1024;
+ $stats['total_optimized_size'] -= $optimized_size / 1024;
update_option( self::OPTION_KEY, $stats, false );
}🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 198-198: Avoid unused parameters such as '$attachment_id'. (undefined)
(UnusedFormalParameter)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/admin/class-stats.php` around lines 198 - 204, In
update_stats_bulk_restored, the two size fields are using the wrong parameters:
swap the size arithmetic so total_optimized_size subtracts $optimized_size/1024
and total_original_size is adjusted only according to how originals were tracked
during optimization (e.g. subtract $restored_size/1024 if originals were
previously added, otherwise leave unchanged) — update the lines in
update_stats_bulk_restored to use $optimized_size and $restored_size in the
correct places (reference function update_stats_bulk_restored and variables
$optimized_size, $restored_size, $stats['total_original_size'],
$stats['total_optimized_size']); also address the unused $attachment_id by
either prefixing it with an underscore or adding a // phpcs:ignore comment.
Comment on lines
+229
to
+231
| let formatTo = options.format || this.options?.format || '' | ||
| formatTo = ( formatTo.startsWith( 'image/' ) ? supportedFormats.find( f => f.mimeType === formatTo )?.value : formatTo ) || | ||
| 'webp' |
There was a problem hiding this comment.
Potential null dereference when formatTo is an unsupported format string.
If a caller passes formatTo = 'jpeg' (rather than 'jpg' or 'image/jpeg'), the normalization logic preserves it unchanged. Then at line 243, formatInfo becomes undefined since supportedFormats uses 'jpg' as the value. This causes a crash at line 336 when accessing formatInfo.mimeType.
Consider adding early validation:
🛡️ Proposed fix to validate formatTo
let formatTo = options.format || this.options?.format || ''
formatTo = ( formatTo.startsWith( 'image/' ) ? supportedFormats.find( f => f.mimeType === formatTo )?.value : formatTo ) ||
'webp'
+
+ // Normalize 'jpeg' to 'jpg' for consistency
+ if ( formatTo === 'jpeg' ) {
+ formatTo = 'jpg'
+ }
+
+ // Validate formatTo is a supported format
+ const formatInfo = supportedFormats.find( f => f.value === formatTo )
+ if ( ! formatInfo ) {
+ return {
+ file,
+ metadata: null,
+ reason: 'unsupported-format',
+ error: `Unsupported output format: ${ formatTo }`,
+ }
+ }📝 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.
Suggested change
| let formatTo = options.format || this.options?.format || '' | |
| formatTo = ( formatTo.startsWith( 'image/' ) ? supportedFormats.find( f => f.mimeType === formatTo )?.value : formatTo ) || | |
| 'webp' | |
| let formatTo = options.format || this.options?.format || '' | |
| formatTo = ( formatTo.startsWith( 'image/' ) ? supportedFormats.find( f => f.mimeType === formatTo )?.value : formatTo ) || | |
| 'webp' | |
| // Normalize 'jpeg' to 'jpg' for consistency | |
| if ( formatTo === 'jpeg' ) { | |
| formatTo = 'jpg' | |
| } | |
| // Validate formatTo is a supported format | |
| const formatInfo = supportedFormats.find( f => f.value === formatTo ) | |
| if ( ! formatInfo ) { | |
| return { | |
| file, | |
| metadata: null, | |
| reason: 'unsupported-format', | |
| error: `Unsupported output format: ${ formatTo }`, | |
| } | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/shared/converters/image-converter.js` around lines 229 - 231, The format
normalization can leave formatTo as an unsupported string (e.g., "jpeg") which
causes formatInfo to be undefined later; update the logic around formatTo
(derived from options.format and this.options?.format) to canonicalize common
aliases (e.g., "jpeg" -> "jpg") and/or map "image/..." MIME types to
supportedFormats values, then look up supportedFormats to get formatInfo and if
lookup returns undefined fall back to a safe default like 'webp' (and log or
throw a clear error if appropriate) so that any subsequent use of
formatInfo.mimeType in the image conversion flow cannot dereference undefined.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
src/admin/class-meta-box.php (1)
101-109: Drop the unused bulk locals.
$bulk_optimization_sizesand$size_keyare never read, so they just add PHPMD noise here.Small cleanup
- $bulk_optimization_sizes = array_keys( $cimo['bulk_optimization'] ); @@ - foreach ( $cimo['bulk_optimization'] as $size_key => $bulk_data ) { + foreach ( $cimo['bulk_optimization'] as $bulk_data ) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/admin/class-meta-box.php` around lines 101 - 109, Remove the unused locals $bulk_optimization_sizes and the unused foreach key variable $size_key to silence PHPMD; delete the line that sets $bulk_optimization_sizes = array_keys( $cimo['bulk_optimization'] ) and change the foreach over $cimo['bulk_optimization'] to use a value-only form (foreach ( $cimo['bulk_optimization'] as $bulk_data ) ) inside the block guarded by $is_bulk_optimized, leaving the existing $orig, $bulk_original_filesize and $bulk_converted_filesize logic intact.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/admin/class-meta-box.php`:
- Around line 94-120: Short-circuit rendering and avoid dividing by zero: detect
when the per-image stat compressionSavings is missing and $is_bulk_optimized is
false/empty and return/skip rendering the meta box (or the stats block) early;
when handling $cimo['bulk_optimization'] in the block around $is_bulk_optimized
and the loop that computes $bulk_original_filesize/$bulk_converted_filesize, add
a guard before computing $optimization_savings to ensure $original_filesize > 0
(otherwise set $optimization_savings to 0 or skip showing it) and remove the
unconditional division; update logic that sets
$original_filesize/$converted_filesize and $optimization_savings so it only runs
when there are usable stats (use $is_bulk_optimized and nonzero
$bulk_original_filesize as conditions).
In `@src/admin/js/media-manager/sidebar-info.js`:
- Around line 98-112: The code currently initializes originalFilesize and
convertedFilesize from top-level customMetadata and then adds every entry in
customMetadata.bulk_optimization, but the meta-box renderer replaces the totals
with the bulk sum; change the logic in sidebar-info.js so that when
customMetadata.bulk_optimization is present and non-empty you compute the totals
only from the bulk_optimization entries (do not start from the top-level
fields), i.e. set originalFilesize/convertedFilesize to 0 and sum each
bulk_optimization[ size ].originalFilesize and .convertedFilesize into those
variables so the sidebar totals match class-meta-box.php behavior.
- Around line 132-134: The computed optimizationSavings can produce NaN/Infinity
when byte totals are zero; update the logic in the optimizationSavings
calculation (referencing optimizationSavings, customMetadata.compressionSavings,
originalFilesize, convertedFilesize and any bulk_optimization totals) to first
check for a non-null compressionSavings using nullish coalescing and to ensure
the denominator (originalFilesize or bulk total) is > 0 before doing the
division; if totals are zero or missing, return a safe fallback (e.g., null or
'0.00') instead of performing the calculation so you never produce NaN/Infinity
in the sidebar.
---
Nitpick comments:
In `@src/admin/class-meta-box.php`:
- Around line 101-109: Remove the unused locals $bulk_optimization_sizes and the
unused foreach key variable $size_key to silence PHPMD; delete the line that
sets $bulk_optimization_sizes = array_keys( $cimo['bulk_optimization'] ) and
change the foreach over $cimo['bulk_optimization'] to use a value-only form
(foreach ( $cimo['bulk_optimization'] as $bulk_data ) ) inside the block guarded
by $is_bulk_optimized, leaving the existing $orig, $bulk_original_filesize and
$bulk_converted_filesize logic intact.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 69588b7e-a5dc-4a94-9ccb-392830b42904
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (3)
src/admin/class-meta-box.phpsrc/admin/css/index.csssrc/admin/js/media-manager/sidebar-info.js
✅ Files skipped from review due to trivial changes (1)
- src/admin/css/index.css
Comment on lines
+94
to
+120
| $is_bulk_optimized = isset( $cimo['bulk_optimization'] ) && | ||
| is_array( $cimo['bulk_optimization'] ) && | ||
| ! empty( $cimo['bulk_optimization'] ); | ||
|
|
||
| // For bulk optimization, we need to recalculate the optimization savings percentage | ||
| // based on the original and converted file sizes for all the thumbnails | ||
| if ( $is_bulk_optimized ) { | ||
| $bulk_optimization_count = count( $cimo['bulk_optimization'] ); | ||
| $bulk_optimization_sizes = array_keys( $cimo['bulk_optimization'] ); | ||
|
|
||
| // Gather all original and converted file sizes for bulk_optimization | ||
| $bulk_original_filesize = 0; | ||
| $bulk_converted_filesize = 0; | ||
| if ( $is_bulk_optimized ) { | ||
| foreach ( $cimo['bulk_optimization'] as $size_key => $bulk_data ) { | ||
| $orig = isset( $bulk_data['originalFilesize'] ) ? floatval( $bulk_data['originalFilesize'] ) : 0; | ||
| $conv = isset( $bulk_data['convertedFilesize'] ) ? floatval( $bulk_data['convertedFilesize'] ) : 0; | ||
| $bulk_original_filesize += $orig; | ||
| $bulk_converted_filesize += $conv; | ||
| } | ||
| } | ||
|
|
||
| $original_filesize = $bulk_original_filesize; | ||
| $converted_filesize = $bulk_converted_filesize; | ||
|
|
||
| $optimization_savings = number_format( 100 * ( $original_filesize - $converted_filesize ) / $original_filesize, 2 ); | ||
| } |
There was a problem hiding this comment.
Short-circuit the meta box when there are no usable optimization stats.
sidebar-info.js now returns early when compressionSavings is missing and bulk_optimization is absent/empty. This path still renders the full box for that state, which leaves blank/zeroed stats, and the bulk percentage calculation still divides by $original_filesize unconditionally.
Suggested guard
$is_bulk_optimized = isset( $cimo['bulk_optimization'] ) &&
is_array( $cimo['bulk_optimization'] ) &&
! empty( $cimo['bulk_optimization'] );
+
+ if ( $compression_savings === null && ! $is_bulk_optimized ) {
+ echo '<p>' . esc_html__( 'Cimo did not optimize this attachment.', 'cimo-image-optimizer' ) . '</p>';
+ return;
+ }
@@
- $optimization_savings = number_format( 100 * ( $original_filesize - $converted_filesize ) / $original_filesize, 2 );
+ $optimization_savings = $original_filesize > 0
+ ? number_format( 100 * ( $original_filesize - $converted_filesize ) / $original_filesize, 2 )
+ : '0.00';
}📝 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.
Suggested change
| $is_bulk_optimized = isset( $cimo['bulk_optimization'] ) && | |
| is_array( $cimo['bulk_optimization'] ) && | |
| ! empty( $cimo['bulk_optimization'] ); | |
| // For bulk optimization, we need to recalculate the optimization savings percentage | |
| // based on the original and converted file sizes for all the thumbnails | |
| if ( $is_bulk_optimized ) { | |
| $bulk_optimization_count = count( $cimo['bulk_optimization'] ); | |
| $bulk_optimization_sizes = array_keys( $cimo['bulk_optimization'] ); | |
| // Gather all original and converted file sizes for bulk_optimization | |
| $bulk_original_filesize = 0; | |
| $bulk_converted_filesize = 0; | |
| if ( $is_bulk_optimized ) { | |
| foreach ( $cimo['bulk_optimization'] as $size_key => $bulk_data ) { | |
| $orig = isset( $bulk_data['originalFilesize'] ) ? floatval( $bulk_data['originalFilesize'] ) : 0; | |
| $conv = isset( $bulk_data['convertedFilesize'] ) ? floatval( $bulk_data['convertedFilesize'] ) : 0; | |
| $bulk_original_filesize += $orig; | |
| $bulk_converted_filesize += $conv; | |
| } | |
| } | |
| $original_filesize = $bulk_original_filesize; | |
| $converted_filesize = $bulk_converted_filesize; | |
| $optimization_savings = number_format( 100 * ( $original_filesize - $converted_filesize ) / $original_filesize, 2 ); | |
| } | |
| $is_bulk_optimized = isset( $cimo['bulk_optimization'] ) && | |
| is_array( $cimo['bulk_optimization'] ) && | |
| ! empty( $cimo['bulk_optimization'] ); | |
| if ( $compression_savings === null && ! $is_bulk_optimized ) { | |
| echo '<p>' . esc_html__( 'Cimo did not optimize this attachment.', 'cimo-image-optimizer' ) . '</p>'; | |
| return; | |
| } | |
| // For bulk optimization, we need to recalculate the optimization savings percentage | |
| // based on the original and converted file sizes for all the thumbnails | |
| if ( $is_bulk_optimized ) { | |
| $bulk_optimization_count = count( $cimo['bulk_optimization'] ); | |
| $bulk_optimization_sizes = array_keys( $cimo['bulk_optimization'] ); | |
| // Gather all original and converted file sizes for bulk_optimization | |
| $bulk_original_filesize = 0; | |
| $bulk_converted_filesize = 0; | |
| if ( $is_bulk_optimized ) { | |
| foreach ( $cimo['bulk_optimization'] as $size_key => $bulk_data ) { | |
| $orig = isset( $bulk_data['originalFilesize'] ) ? floatval( $bulk_data['originalFilesize'] ) : 0; | |
| $conv = isset( $bulk_data['convertedFilesize'] ) ? floatval( $bulk_data['convertedFilesize'] ) : 0; | |
| $bulk_original_filesize += $orig; | |
| $bulk_converted_filesize += $conv; | |
| } | |
| } | |
| $original_filesize = $bulk_original_filesize; | |
| $converted_filesize = $bulk_converted_filesize; | |
| $optimization_savings = $original_filesize > 0 | |
| ? number_format( 100 * ( $original_filesize - $converted_filesize ) / $original_filesize, 2 ) | |
| : '0.00'; | |
| } |
🧰 Tools
🪛 PHPMD (2.15.0)
[warning] 102-102: Avoid unused local variables such as '$bulk_optimization_sizes'. (undefined)
(UnusedLocalVariable)
[warning] 108-108: Avoid unused local variables such as '$size_key'. (undefined)
(UnusedLocalVariable)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/admin/class-meta-box.php` around lines 94 - 120, Short-circuit rendering
and avoid dividing by zero: detect when the per-image stat compressionSavings is
missing and $is_bulk_optimized is false/empty and return/skip rendering the meta
box (or the stats block) early; when handling $cimo['bulk_optimization'] in the
block around $is_bulk_optimized and the loop that computes
$bulk_original_filesize/$bulk_converted_filesize, add a guard before computing
$optimization_savings to ensure $original_filesize > 0 (otherwise set
$optimization_savings to 0 or skip showing it) and remove the unconditional
division; update logic that sets $original_filesize/$converted_filesize and
$optimization_savings so it only runs when there are usable stats (use
$is_bulk_optimized and nonzero $bulk_original_filesize as conditions).
Comment on lines
+98
to
+112
| let originalFilesize = parseInt( customMetadata.originalFilesize || 0 ) | ||
| let convertedFilesize = parseInt( customMetadata.convertedFilesize || 0 ) | ||
|
|
||
| const isBulkOptimized = customMetadata.bulk_optimization && | ||
| typeof customMetadata.bulk_optimization === 'object' && | ||
| ! Array.isArray( customMetadata.bulk_optimization ) && | ||
| Object.keys( customMetadata.bulk_optimization ).length > 0 | ||
|
|
||
| // Bulk optimization keys are the sizes, so we need to sum the original filesizes of the bulk optimization | ||
| if ( isBulkOptimized ) { | ||
| for ( const size in customMetadata.bulk_optimization ) { | ||
| originalFilesize += parseInt( customMetadata.bulk_optimization[ size ].originalFilesize || 0 ) | ||
| convertedFilesize += parseInt( customMetadata.bulk_optimization[ size ].convertedFilesize || 0 ) | ||
| } | ||
| } |
There was a problem hiding this comment.
Align the bulk filesize aggregation with the meta-box renderer.
This starts from the top-level filesize fields and then adds every bulk_optimization entry. src/admin/class-meta-box.php Line 116 and Line 117 replace the totals with the bulk sum instead, so the same attachment can show different original/optimized sizes depending on which admin screen is open.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/admin/js/media-manager/sidebar-info.js` around lines 98 - 112, The code
currently initializes originalFilesize and convertedFilesize from top-level
customMetadata and then adds every entry in customMetadata.bulk_optimization,
but the meta-box renderer replaces the totals with the bulk sum; change the
logic in sidebar-info.js so that when customMetadata.bulk_optimization is
present and non-empty you compute the totals only from the bulk_optimization
entries (do not start from the top-level fields), i.e. set
originalFilesize/convertedFilesize to 0 and sum each bulk_optimization[ size
].originalFilesize and .convertedFilesize into those variables so the sidebar
totals match class-meta-box.php behavior.
Comment on lines
132
to
+134
| const optimizationSavings = customMetadata.compressionSavings | ||
| ? ( 100 - ( customMetadata.compressionSavings * 100 ) ).toFixed( 2 ) | ||
| : null | ||
| : ( 100 * ( originalFilesize - convertedFilesize ) / originalFilesize ).toFixed( 2 ) |
There was a problem hiding this comment.
Guard the derived savings percentage when bulk totals are zero.
If bulk_optimization exists but its byte totals are empty/zero, this renders NaN/Infinity in the sidebar. Using a nullish check here also avoids treating a numeric 0 as “missing”.
Suggested guard
- const optimizationSavings = customMetadata.compressionSavings
- ? ( 100 - ( customMetadata.compressionSavings * 100 ) ).toFixed( 2 )
- : ( 100 * ( originalFilesize - convertedFilesize ) / originalFilesize ).toFixed( 2 )
+ const optimizationSavings =
+ customMetadata.compressionSavings != null
+ ? ( 100 - ( customMetadata.compressionSavings * 100 ) ).toFixed( 2 )
+ : originalFilesize > 0
+ ? ( 100 * ( originalFilesize - convertedFilesize ) / originalFilesize ).toFixed( 2 )
+ : '0.00'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/admin/js/media-manager/sidebar-info.js` around lines 132 - 134, The
computed optimizationSavings can produce NaN/Infinity when byte totals are zero;
update the logic in the optimizationSavings calculation (referencing
optimizationSavings, customMetadata.compressionSavings, originalFilesize,
convertedFilesize and any bulk_optimization totals) to first check for a
non-null compressionSavings using nullish coalescing and to ensure the
denominator (originalFilesize or bulk total) is > 0 before doing the division;
if totals are zero or missing, return a safe fallback (e.g., null or '0.00')
instead of performing the calculation so you never produce NaN/Infinity in the
sidebar.
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
1 participant
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.
fixes #26
Summary by CodeRabbit
New Features
Bug Fixes
Chores