fix: get lesser between wp threshold and user max dimension#44
Open
fix: get lesser between wp threshold and user max dimension#44
Conversation
📝 WalkthroughWalkthroughExposes WordPress's Changes
Sequence Diagram(s)sequenceDiagram
participant PHP as "Admin PHP\n(class-script-loader.php)"
participant Browser as "Browser\n(admin UI)"
participant CimoScript as "Frontend JS\n(cimo-script)"
participant Converter as "Image Converter\n(src/shared/converters)"
PHP->>CimoScript: localize cimoSettings { maxImageDimension, wpScalingThreshold }
Browser->>CimoScript: load cimo-script (reads cimoSettings for UI/help)
Browser->>Converter: user triggers conversion/upload
CimoScript->>Converter: convert(image, cimoSettings)
Converter->>Converter: parse values -> finalMax = min(non-zero values) or fallback
Converter->>Converter: call ImageConverter with maxDimension = finalMax
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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
|
Contributor
|
@Arukuen Please add:
|
![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
🤖 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 594-596: The help/placeholder text around the maxImageDimension
input is misleading because it always states the WP fallback is 2560px; update
the UI to read the actual localized value and conditionally change wording: read
window.cimoSettings.wpScalingThreshold and if truthy use that value in the
placeholder/help (e.g., "Leave empty to use WordPress maximum dimension (Xpx)"),
otherwise remove the fallback wording and clearly state that leaving it empty
will disable resizing (or no WP auto-scaling) — adjust the component rendering
where maxImageDimension, handleInputChange, and the help prop are set so the
placeholder/help reflect wpScalingThreshold and the behavior of
finalMaxDimension when wpScalingThreshold is falsy.
🪄 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: 578a78bb-b491-47c5-852f-7d7c71f5794a
📒 Files selected for processing (1)
src/admin/js/page/admin-settings.js
bfintal
requested changes
Apr 20, 2026
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 594-601: The placeholder currently uses a hardcoded '2560' and the
help text nests sprintf, causing inconsistent/unsplittable translations; update
the input's placeholder prop to use the dynamic
window.cimoSettings?.wpScalingThreshold (or undefined when falsy) so it matches
the help text logic, and refactor the help prop to choose between two complete,
self-contained translated sentences (one when wpScalingThreshold is truthy and
one when falsy) instead of nesting sprintf calls; adjust the JSX around
settings.disableWpScaling and handleInputChange('maxImageDimension', ...)
accordingly, and remove the stray blank line inside the JSX attributes.
🪄 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: 79c1e9f3-1a62-4963-b280-50b82c41a7a3
📒 Files selected for processing (2)
src/admin/class-admin.phpsrc/admin/js/page/admin-settings.js
✅ Files skipped from review due to trivial changes (1)
- src/admin/class-admin.php
Comment on lines
+594
to
+601
| placeholder={ settings.disableWpScaling === 1 ? '2560' : undefined } | ||
| onChange={ value => handleInputChange( 'maxImageDimension', value ) } | ||
| help={ __( 'Maximum width or height in pixels for uploaded images. Images exceeding this dimension will be automatically resized while preserving aspect ratio. Leave empty to disable resizing. We recommend a value of 1920px.', 'cimo-image-optimizer' ) } | ||
| help={ sprintf( | ||
| __( 'Maximum width or height in pixels for uploaded images. Images exceeding this dimension will be automatically resized while preserving aspect ratio. Leave empty to %s. We recommend a value of 1920px.', 'cimo-image-optimizer' ), | ||
| window.cimoSettings?.wpScalingThreshold | ||
| ? sprintf( __( "use WordPress's default auto-scaling at %spx", 'cimo-image-optimizer' ), window.cimoSettings.wpScalingThreshold ) | ||
| : __( 'disable auto-scaling', 'cimo-image-optimizer' ) | ||
| ) } |
There was a problem hiding this comment.
Placeholder should use wpScalingThreshold and avoid split translations.
Two issues in this block:
-
Placeholder is hardcoded to
'2560'while the help text uses the dynamicwindow.cimoSettings?.wpScalingThreshold. If a site filtersbig_image_size_thresholdto a non-default value, the placeholder and help will disagree. Use the localized value for both (and fall back to hiding the placeholder when the threshold is falsy, consistent with the branching already done for the help text). -
Nested
sprintfsplits a sentence across two translatable strings. Translators see an isolated fragment ("use WordPress's default auto-scaling at %spx") with no surrounding context, and word order can't be rearranged for languages that need it. Prefer two complete, self-contained sentences selected by a single conditional.
✏️ Suggested adjustment
- placeholder={ settings.disableWpScaling === 1 ? '2560' : undefined }
+ placeholder={ settings.disableWpScaling === 1 && window.cimoSettings?.wpScalingThreshold
+ ? String( window.cimoSettings.wpScalingThreshold )
+ : undefined }
onChange={ value => handleInputChange( 'maxImageDimension', value ) }
- help={ sprintf(
- __( 'Maximum width or height in pixels for uploaded images. Images exceeding this dimension will be automatically resized while preserving aspect ratio. Leave empty to %s. We recommend a value of 1920px.', 'cimo-image-optimizer' ),
- window.cimoSettings?.wpScalingThreshold
- ? sprintf( __( "use WordPress's default auto-scaling at %spx", 'cimo-image-optimizer' ), window.cimoSettings.wpScalingThreshold )
- : __( 'disable auto-scaling', 'cimo-image-optimizer' )
- ) }
+ help={ window.cimoSettings?.wpScalingThreshold
+ ? sprintf(
+ /* translators: %d is the WordPress big_image_size_threshold in pixels. */
+ __( "Maximum width or height in pixels for uploaded images. Images exceeding this dimension will be automatically resized while preserving aspect ratio. Leave empty to use WordPress's default auto-scaling at %dpx. We recommend a value of 1920px.", 'cimo-image-optimizer' ),
+ window.cimoSettings.wpScalingThreshold
+ )
+ : __( 'Maximum width or height in pixels for uploaded images. Images exceeding this dimension will be automatically resized while preserving aspect ratio. Leave empty to disable auto-scaling. We recommend a value of 1920px.', 'cimo-image-optimizer' )
+ }Also note the stray blank line at 602 inside the JSX attributes — safe to remove.
🤖 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 594 - 601, The placeholder
currently uses a hardcoded '2560' and the help text nests sprintf, causing
inconsistent/unsplittable translations; update the input's placeholder prop to
use the dynamic window.cimoSettings?.wpScalingThreshold (or undefined when
falsy) so it matches the help text logic, and refactor the help prop to choose
between two complete, self-contained translated sentences (one when
wpScalingThreshold is truthy and one when falsy) instead of nesting sprintf
calls; adjust the JSX around settings.disableWpScaling and
handleInputChange('maxImageDimension', ...) accordingly, and remove the stray
blank line inside the JSX attributes.
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 #43
Summary by CodeRabbit
New Features
Bug Fixes