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: Pro Plus 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.
|
2 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).