fix: make px crops export at a deterministic size - #35
Conversation
|
Important
This repository does not receive automatic reviews because it has fewer than 10 stars. ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: WalkthroughChangesCrop normalization
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![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 current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/main/resources/META-INF/resources/frontend/src/image-crop.tsx`:
- Around line 144-157: Update the onImageLoad normalization to preserve
explicitly supplied x and y coordinates by removing the unconditional
makeAspectCrop and centerCrop calls. Move px-to-% conversion into a useEffect
that observes the crop state and image dimensions, converting programmatic
server updates before passing the crop to ReactCrop while leaving percentage
crops unchanged.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 44225dc7-6409-410f-952f-89ba91e1c764
📒 Files selected for processing (3)
src/main/java/com/flowingcode/vaadin/addons/imagecrop/Crop.javasrc/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.javasrc/main/resources/META-INF/resources/frontend/src/image-crop.tsx
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/resources/META-INF/resources/frontend/src/image-crop.tsx (1)
271-278: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winPrevent
IndexSizeErrorcrash on zero-dimension crops.If the crop region has a width or height of zero (e.g., from an uninitialized state or programmatic update),
outWidthoroutHeightwill evaluate to0. CallingdrawImagewith a zero source width or height throws anIndexSizeError(orInvalidStateError) in browsers, which crashes the script execution and prevents subsequent logic from running. Consider adding an early return to handle this gracefully.🛡️ Proposed fix
const outWidth = Math.round(ccrop.width); const outHeight = Math.round(ccrop.height); + if (outWidth <= 0 || outHeight <= 0) { + return; + } + // Setting canvas dimensions resets the 2D context, so it must happen // before any drawing/clipping state is configured below. canvas.width = outWidth; canvas.height = outHeight;🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/main/resources/META-INF/resources/frontend/src/image-crop.tsx` around lines 271 - 278, Add an early return in the crop-rendering flow after computing outWidth and outHeight, before assigning canvas dimensions or calling drawImage, when either dimension is zero or otherwise non-positive. Preserve the existing rendering path for positive dimensions and allow subsequent logic to continue without throwing.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/main/resources/META-INF/resources/frontend/src/image-crop.tsx`:
- Around line 271-278: Add an early return in the crop-rendering flow after
computing outWidth and outHeight, before assigning canvas dimensions or calling
drawImage, when either dimension is zero or otherwise non-positive. Preserve the
existing rendering path for positive dimensions and allow subsequent logic to
continue without throwing.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 808376d8-5c44-4867-9897-a49725681424
📒 Files selected for processing (3)
src/main/java/com/flowingcode/vaadin/addons/imagecrop/Crop.javasrc/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.javasrc/main/resources/META-INF/resources/frontend/src/image-crop.tsx
🚧 Files skipped from review as they are similar to previous changes (1)
- src/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.java
javier-godoy
left a comment
javier-godoy
left a comment
There was a problem hiding this comment.
Automated review of the changes introduced by this PR (px/percent crop normalization rework).
| /** | ||
| * Adjusts the crop size proportionally when the image is resized. | ||
| * Normalizes the configured crop when the image loads. The crop is kept as a | ||
| * percentage of the image's natural size, so both the on-screen selection and |
There was a problem hiding this comment.
toPercent returns 0 when dimension is falsy:
const toPercent = (value: number, dimension: number) =>
dimension ? (value / dimension) * 100 : 0;For an image whose naturalWidth/naturalHeight is 0 even after load fires (e.g. an SVG source without width/height/viewBox), all four toPercent calls in onImageLoad collapse to 0, producing a zero-size crop that gets fed into _updateCroppedImage — an empty/invalid exported image instead of the configured crop, with no error surfaced.
There was a problem hiding this comment.
Valid, fixed — though the guard belongs further down: with a zero naturalWidth/naturalHeight any crop maps to zero, % included, so patching toPercent alone wouldn't have covered it. _updateCroppedImage now bails out instead of drawing a 0×0 canvas and firing a blank data:, URI (4ec6120), and the normalization helper returns null while the image has no intrinsic size (4bed3ba).
| /** | ||
| * Adjusts the crop size proportionally when the image is resized. | ||
| * Normalizes the configured crop when the image loads. The crop is kept as a | ||
| * percentage of the image's natural size, so both the on-screen selection and |
There was a problem hiding this comment.
This px→percent conversion is hand-rolled here and duplicated again in the useEffect below (normalizing a late setCrop). Consider sharing one helper (or using react-image-crop's own convertToPercentCrop, already available alongside convertToPixelCrop) so the two normalization paths can't drift out of sync — which is exactly what happened with the missing aspect-ratio step noted in the other comment.
There was a problem hiding this comment.
Agreed, done in 4bed3ba: both paths now go through a single helper built on convertToPercentCrop, which also passes a % crop through untouched, so the ternary and toPercent are gone. One thing worth noting: its own zero-guard yields Infinity rather than 0 for a zero-size image, so the helper checks the natural size before calling it.
| * has loaded. onImageLoad only runs on the initial load, so without this a | ||
| * later setCrop("px", ...) would be rendered by ReactCrop as on-screen pixels | ||
| * and the selection box would diverge from the natural-pixel export (issue | ||
| * #33). The configured x/y are preserved (no centering) since the crop is |
There was a problem hiding this comment.
Unlike onImageLoad (which calls makeAspectCrop before centerCrop), this effect never re-applies the configured aspect when normalizing a late programmatic px crop.
Repro: configure aspect={1} with the image already loaded, then call setCrop(new Crop("px", 10, 10, 200, 50)) (non-square). onImageLoad won't re-run since the image is already loaded, so only this effect fires — it converts x/y/width/height to percent verbatim with no aspect enforcement, leaving the crop box (and exported image) non-square despite aspect=1, until the user manually drags a handle.
There was a problem hiding this comment.
Confirmed with that exact repro, and fixed in 26b6213: aspect enforcement is now a shared applyAspect helper called from both the load path and this effect. A late px crop of 200×50 with aspect=1 now comes out 200×200 with the configured x/y preserved. The aspect ? guard stays, since makeAspectCrop with an undefined aspect returns height: 0.
| * has loaded. onImageLoad only runs on the initial load, so without this a | ||
| * later setCrop("px", ...) would be rendered by ReactCrop as on-screen pixels | ||
| * and the selection box would diverge from the natural-pixel export (issue | ||
| * #33). The configured x/y are preserved (no centering) since the crop is |
There was a problem hiding this comment.
This normalization always rewrites the crop's unit to "%", so getCrop() on the Java side can now return a different unit and different numeric values than what was passed to setCrop().
On master, onImageLoad preserved crop.unit verbatim, so a px crop stayed px after load. With this change, setCrop(new Crop("px", 100, 100, 300, 300)) followed by getCrop() (after load or any interaction) returns unit % with fractional values instead of the original px values — silently breaking any caller code that persists getCrop() and re-applies it later, or that branches on crop.unit().equals("px").
There was a problem hiding this comment.
Confirmed: after this PR, getCrop() returns a % crop even when setCrop() was given px.
One correction on the premise, though — master didn't preserve the unit either. Its onChange stored react-image-crop's PixelCrop argument, so as soon as the user dragged the selection, a caller-configured % crop came back from getCrop() as px in rendered pixels. The unit was never stable across a round trip; this PR only changes which unit it settles on.
What I do think is worth acting on is precision rather than the unit. Crop stores x/y/width/height as int, so a % crop gets rounded to whole percentages. On a 4000 px-wide image 1% is 40 source pixels, where the old rendered-pixel values were off by about 1 — so getCrop() → persist → setCrop() later now moves and resizes the selection noticeably.
Fixing that means changing Crop to double, which breaks a public record and is outside #33. For this PR I'll document on getCrop() that the crop comes back normalized to %, and open a follow-up for the int → double change.
scardanzan
left a comment
scardanzan
left a comment
There was a problem hiding this comment.
Reviewed the crop math by extracting the react-image-crop@11.0.6 helpers from the bundled dist/index.js and simulating both pipelines. The fix checks out — with a 4000×3000 source and Crop("px", 0, 0, 500, 500) at aspect 1, master exports 2500×2500 when the image renders at 800×600 and 3000×3000 at 600×450, while this branch exports 500×500 in both cases. Dropping the ResizeObserver is right too: resizeCrop scaled crop.width by the layout factor regardless of unit, which was plainly wrong for a % crop.
One thing worth adding to the description: the applyAspect no-op guard fixes a second, currently live bug. makeAspectCrop(..., undefined, ...) computes height = width / undefined → NaN, and convertToPercentCrop coerces the falsy NaN to 0, so any crop configured without setAspect collapses on load — master turns Crop("%", 25, 25, 50, 50) into {x: 50, y: 50, width: 0, height: 0}. That is exactly OutputFormatImageCropDemo, whose initial selection is invisible today.
Also worth an explicit line in the release notes: px crop semantics change from rendered to source pixels, so anyone who tuned a px crop against the on-screen size will see it move. That is the point of the fix and 1.3.0 is the right place for it, but it should not live only in Javadoc.
Three inline notes below; none of them block from my side.
|
github-project-automation
Bot
moved this from In Progress
to Pending release
in Flowing Code Addons
3 participants
Configured
pxcrops mapped to the exported image via rendered (on-screen) pixels, so the output size varied with how the browser scaled the image at crop time (e.g. a 500×500 px crop could export at ~500 or ~667 px). This makes the mapping deterministic.Changes
%(resolution-independent):onChange/onCompletenow use react-image-crop'spercentCrop.onImageLoadnormalizes the configured crop against the image's natural size; apxcrop is interpreted as source (natural) pixels._updateCroppedImagemaps the crop withconvertToPixelCropagainstnaturalWidth/naturalHeight, dropping the rendered→natural rescaling.ResizeObserver/resizeCropworkaround (a%crop needs no rescaling on layout changes) and guardmakeAspectCropagainst an unset aspect.Close #33
Summary by CodeRabbit
Bug Fixes
Documentation
%(resolution-independent) versuspx(based on the image’s natural/source pixels).