feat: add smart optimization to image instant optimization#39
Open
feat: add smart optimization to image instant optimization#39
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:
📝 WalkthroughWalkthroughUpload and select-file flows now call Converter.optimize(); Converter gained a default async optimize() delegating to convert(); ImageConverter implements a filter-driven optimize() with fallback to convert(); converter selection falls back to original MIME when WebP unsupported; admin adds and exposes smartOptimization and UI toggle. Changes
Sequence Diagram(s)sequenceDiagram
participant Handler as Media Handler
participant Conv as ImageConverter
participant Filter as Filter System
participant Core as Converter.convert
Handler->>Conv: optimize(file)
Conv->>Filter: applyFiltersAsync('cimo.imageConverter.optimize', {file, metadata: null, reason: 'no-optimizer'}, this)
alt Filter returns optimized result with metadata
Filter-->>Conv: { file, metadata }
Conv-->>Handler: resolved optimized result
else Filter disabled / returns no-optimizer / no metadata
Filter-->>Conv: { reason: 'no-optimizer' } or null
Conv->>Core: convert(file) (fallback)
Core-->>Conv: converted result
Conv-->>Handler: resolved converted result
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 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 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
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)
222-227:⚠️ Potential issue | 🟠 MajorEither restore or retire
forcehere.After the same-format short-circuit was removed,
convert( false )andconvert( true )behave identically. That is the currentno-unused-varspipeline failure, and it leaves the method contract misleading.🧹 Minimal fix if the flag is intentionally deprecated
- * `@param` {boolean} [force=false] - Force conversion even if the file is already in the desired format. + * `@param` {boolean} [_force=false] - Deprecated; kept only for backward compatibility. * `@param` {Object} [options] - Options for the conversion. * `@return` {Promise<{file: File|Blob, metadata?: Object}>} Promise resolving to the converted file and optional metadata. */ - async convert( force = false, options = {} ) { + async convert( _force = false, options = {} ) {If callers still rely on
force, restore aforce-controlled branch instead of just renaming it.Also applies to: 242-250
🤖 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 222 - 227, The convert method currently declares the parameter force but no longer uses it (causing a no-unused-vars failure) so either remove the parameter from convert(...) and update callers OR restore behavior controlled by force: in src/shared/converters/image-converter.js locate the async convert(force = false, options = {}) method and either (A) retire the flag by removing the force parameter and any callers that pass it, or (B) reintroduce a force-controlled branch (e.g., before the same-format short-circuit/early-return logic) so that when force === true the converter skips the "same format" short-circuit and always performs conversion; ensure any related logic around forceSize and options still composes with this behavior and mirror the same change for the similar block referenced around lines 242-250.
🤖 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/shared/converters/image-converter.js`:
- Around line 361-371: The optimize method currently treats missing
result.metadata as an optimizer failure and falls back to convert(), but
metadata can validly be null; update the fallback logic in optimize (the method
using applyFiltersAsync('cimo.imageConverter.optimize')) to only fall back when
the optimizer explicitly signals failure (result.reason === 'no-optimizer') or
when the optimizer did not return a valid file (e.g., !result.file), leaving
results with metadata: null intact and returned as-is.
---
Outside diff comments:
In `@src/shared/converters/image-converter.js`:
- Around line 222-227: The convert method currently declares the parameter force
but no longer uses it (causing a no-unused-vars failure) so either remove the
parameter from convert(...) and update callers OR restore behavior controlled by
force: in src/shared/converters/image-converter.js locate the async
convert(force = false, options = {}) method and either (A) retire the flag by
removing the force parameter and any callers that pass it, or (B) reintroduce a
force-controlled branch (e.g., before the same-format short-circuit/early-return
logic) so that when force === true the converter skips the "same format"
short-circuit and always performs conversion; ensure any related logic around
forceSize and options still composes with this behavior and mirror the same
change for the similar block referenced around lines 242-250.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 89a48463-53e7-45b0-80bf-ccbe165ebe7f
📒 Files selected for processing (5)
src/admin/js/media-manager/drop-zone.jssrc/admin/js/media-manager/select-files.jssrc/shared/converters/converter-abstract.jssrc/shared/converters/image-converter.jssrc/shared/converters/index.js
🤖 Pull request artifacts
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
src/shared/converters/image-converter.js (1)
359-360:⚠️ Potential issue | 🟠 MajorDon't discard valid optimizer results with
metadata: null.
metadatais optional in the converter contract, so a result like{ file, metadata: null }is still valid. Falling back on! result.metadatathrows away successful smart-optimization output and also misses the real guard here: whether a file was returned.✅ Safer fallback
- if ( result.reason === 'no-optimizer' || ! result.metadata ) { + if ( ! result || result.reason === 'no-optimizer' || ! result.file ) { result = await this.convert() }🤖 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 359 - 360, The current guard in image-converter.js that checks "if (result.reason === 'no-optimizer' || !result.metadata) { result = await this.convert() }" improperly treats valid results with metadata: null as failures; change the fallback to only trigger when there is no returned file or when reason === 'no-optimizer' (e.g., use a check like result.file == null or !result.file) so valid {file, metadata: null} responses are preserved and only truly missing-file results call this.convert(); keep references to the existing result variable and this.convert() call.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@src/shared/converters/image-converter.js`:
- Around line 359-360: The current guard in image-converter.js that checks "if
(result.reason === 'no-optimizer' || !result.metadata) { result = await
this.convert() }" improperly treats valid results with metadata: null as
failures; change the fallback to only trigger when there is no returned file or
when reason === 'no-optimizer' (e.g., use a check like result.file == null or
!result.file) so valid {file, metadata: null} responses are preserved and only
truly missing-file results call this.convert(); keep references to the existing
result variable and this.convert() call.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 0575b7f4-2803-4d71-b13e-442ea207723e
📒 Files selected for processing (1)
src/shared/converters/image-converter.js
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/admin/class-script-loader.php (1)
93-93: Make the localized smart-optimization default build-aware.Line 93 is the boolean gate consumed by
src/shared/converters/image-converter.js:352-368. Defaulting it to1for every build makes the free/premium split depend on downstream JS never resolving a smart optimizer. It would be safer to localize the effective default here and keep any Safari/iOS MIME fallback inside the converter.♻️ Suggested change
- 'smartOptimization' => isset( $settings['smart_optimization'] ) ? (int) $settings['smart_optimization'] : 1, + 'smartOptimization' => CIMO_BUILD === 'premium' + ? ( isset( $settings['smart_optimization'] ) ? (int) $settings['smart_optimization'] : 1 ) + : 0,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/admin/class-script-loader.php` at line 93, The localized smartOptimization default currently hardcodes 1 when $settings['smart_optimization'] is unset, which forces the JS converter (src/shared/converters/image-converter.js) to assume smart optimization is enabled; change the default to be build-aware by computing it from a build/licensing check instead of 1 — e.g., replace the fallback 1 with a call that returns whether this build is a pro/premium build (use an existing build/license check method or a new is_premium_build()/is_pro() helper) so 'smartOptimization' uses (isset($settings['smart_optimization']) ? (int)$settings['smart_optimization'] : (int) $this->is_premium_build()) while keeping any Safari/iOS MIME fallback inside the converter.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/admin/class-script-loader.php`:
- Line 93: The localized smartOptimization default currently hardcodes 1 when
$settings['smart_optimization'] is unset, which forces the JS converter
(src/shared/converters/image-converter.js) to assume smart optimization is
enabled; change the default to be build-aware by computing it from a
build/licensing check instead of 1 — e.g., replace the fallback 1 with a call
that returns whether this build is a pro/premium build (use an existing
build/license check method or a new is_premium_build()/is_pro() helper) so
'smartOptimization' uses (isset($settings['smart_optimization']) ?
(int)$settings['smart_optimization'] : (int) $this->is_premium_build()) while
keeping any Safari/iOS MIME fallback inside the converter.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c2c3081c-708c-4555-9288-d7580009e4cb
📒 Files selected for processing (5)
src/admin/class-admin.phpsrc/admin/class-script-loader.phpsrc/admin/js/page/admin-settings.jssrc/shared/converters/image-converter.jssrc/shared/converters/index.js
🚧 Files skipped from review as they are similar to previous changes (2)
- src/shared/converters/index.js
- src/shared/converters/image-converter.js
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/admin/js/page/admin-settings.js (1)
180-199:⚠️ Potential issue | 🟠 MajorKeep free presets from overwriting the hidden premium setting.
Line 184 and Line 195 unconditionally force
smartOptimizationto1. In a free build, a downgraded user can click Recommended or Reset to Default, save, and lose their previous premium choice even though the control itself is hidden/disabled. Leave this field unchanged whenbuildType === 'free'.🔧 Possible fix
const applyImageRecommendedSettings = () => { setSettings( settings => { return { ...settings, - smartOptimization: 1, + smartOptimization: buildType === 'free' ? settings.smartOptimization : 1, webpQuality: 80, maxImageDimension: 1920, } } ) } const applyImageDefaultSettings = () => { setSettings( settings => { return { ...settings, - smartOptimization: 1, + smartOptimization: buildType === 'free' ? settings.smartOptimization : 1, webpQuality: '', maxImageDimension: '', } } ) }Based on learnings, premium-only toggles in this file intentionally stay visually off in free while preserving the stored server-side value so it comes back after upgrading.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/admin/js/page/admin-settings.js` around lines 180 - 199, Both applyImageRecommendedSettings and applyImageDefaultSettings unconditionally overwrite smartOptimization to 1, which wipes a hidden premium value in free builds; update each function (applyImageRecommendedSettings and applyImageDefaultSettings) to only set smartOptimization when buildType !== 'free' (i.e., preserve settings.smartOptimization in the new object when buildType === 'free') so the server-side premium value remains unchanged for free users while other fields are updated via setSettings.
🤖 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/js/page/admin-settings.js`:
- Around line 302-303: The code currently writes webp_quality from
settings.webpQuality and still renders an editable slider for non-paid users;
change the save logic so that when the current account is on free tier the saved
webp_quality is forced to 80 (e.g., replace writing webp_quality:
parseInt(settings.webpQuality) || 0 with conditional logic that sets
webp_quality = 80 for free users, otherwise uses
parseInt(settings.webpQuality)), and update the settings UI rendering (the
component that renders the webp quality slider) to be read-only or hidden for
free users so the control is not editable—use the existing user-tier flag/check
in the code to gate both the save in the block that sets
smart_optimization/webp_quality and the slider rendering.
---
Outside diff comments:
In `@src/admin/js/page/admin-settings.js`:
- Around line 180-199: Both applyImageRecommendedSettings and
applyImageDefaultSettings unconditionally overwrite smartOptimization to 1,
which wipes a hidden premium value in free builds; update each function
(applyImageRecommendedSettings and applyImageDefaultSettings) to only set
smartOptimization when buildType !== 'free' (i.e., preserve
settings.smartOptimization in the new object when buildType === 'free') so the
server-side premium value remains unchanged for free users while other fields
are updated via setSettings.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4fe67036-6f8e-4f86-a7b3-41ecfb481408
📒 Files selected for processing (2)
src/admin/css/admin-page.csssrc/admin/js/page/admin-settings.js
✅ Files skipped from review due to trivial changes (1)
- src/admin/css/admin-page.css
Comment on lines
+302
to
303
| smart_optimization: settings.smartOptimization, | ||
| webp_quality: parseInt( settings.webpQuality ) || 0, |
There was a problem hiding this comment.
Free mode still allows a non-static image quality.
Line 303 saves whatever webpQuality is in state, and Line 600 still renders an editable slider for free users. Issue #35 says free uploads should stay on a fixed 80% quality, so this either violates the spec or exposes a control the runtime is supposed to ignore. Lock free saves to 80 and make the free UI read-only or hide it.
🔧 Possible fix
// Image Optimization settings
smart_optimization: settings.smartOptimization,
- webp_quality: parseInt( settings.webpQuality ) || 0,
+ webp_quality: buildType === 'free' ? 80 : ( parseInt( settings.webpQuality, 10 ) || 0 ),
max_image_dimension: parseInt( settings.maxImageDimension ) || 0,
@@
- { ( buildType === 'free' || settings.smartOptimization === 0 ) && (
+ { buildType === 'free' && (
+ <div className="cimo-setting-field cimo-webp-quality-range-control">
+ <RangeControl
+ id="webpQuality"
+ label={ __( 'WebP Image Quality', 'cimo-image-optimizer' ) }
+ value={ 80 }
+ min="1"
+ max="100"
+ step="1"
+ disabled
+ __next40pxDefaultSize
+ help={ __( 'Free builds use a fixed 80% WebP quality.', 'cimo-image-optimizer' ) }
+ />
+ </div>
+ ) }
+ { buildType !== 'free' && settings.smartOptimization === 0 && (
<div className="cimo-setting-field cimo-webp-quality-range-control">
<RangeControl
id="webpQuality"Also applies to: 600-614
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/admin/js/page/admin-settings.js` around lines 302 - 303, The code
currently writes webp_quality from settings.webpQuality and still renders an
editable slider for non-paid users; change the save logic so that when the
current account is on free tier the saved webp_quality is forced to 80 (e.g.,
replace writing webp_quality: parseInt(settings.webpQuality) || 0 with
conditional logic that sets webp_quality = 80 for free users, otherwise uses
parseInt(settings.webpQuality)), and update the settings UI rendering (the
component that renders the webp quality slider) to be read-only or hidden for
free users so the control is not editable—use the existing user-tier flag/check
in the code to gate both the save in the block that sets
smart_optimization/webp_quality and the slider rendering.
Contributor
|
@Arukuen it looks the popup/dialog that should show up when the image optimization surpasses 500ms long isn't implemented yet |
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
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
fixes #35
Summary by CodeRabbit
New Features
Bug Fixes / Behavior