Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe PR changes _updateCroppedImage to export cropped images at the source image’s natural pixel dimensions by computing outWidth/outHeight from the pixel crop and natural/display sizes, resizing the canvas to those values, adjusting circular-clip geometry, and drawing with the natural-size destination. ChangesImage natural-pixel export pipeline
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 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)
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 164-180: Preconfigured pixel crops (crop.unit === 'px') are being
converted to natural pixels in onImageLoad() and then again scaled by
scaleX/scaleY in _updateCroppedImage(), causing double-scaling; fix by
normalizing incoming pixel-based crop to the image's rendered/display
coordinates before passing it into ReactCrop (or by keeping crop state
consistently in natural pixels end-to-end). Concretely, update onImageLoad() to
detect crop.unit === 'px' and convert the provided crop from natural pixels to
displayed pixels (divide width/height/x/y by scaleX/scaleY) before calling
setCrop/react-state for ReactCrop, or alternatively change state handling so
onImageLoad() stores the crop in natural pixels and remove the extra
scaleX/scaleY multiplication from _updateCroppedImage(); ensure both onImageLoad
and _updateCroppedImage use the same pixel-space convention so programmatic
crops export at the intended dimensions.
🪄 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: 4310bc0f-9153-492b-91ff-61ef45977caa
📒 Files selected for processing (1)
src/main/resources/META-INF/resources/frontend/src/image-crop.tsx
|
@CodeRabbit full review |
✅ Action performedFull review finished. |
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 164-180: The existing block incorrectly claims that a crop
measured in "px" always yields the same configured output size; update the
comment to state that react-image-crop reports the crop rectangle in rendered
(display) pixels, so when we map that rectangle to the source using
scaleX/scaleY and draw to an offscreen output canvas the resulting pixel
dimensions can differ if the image was scaled for display. Remove the assertion
that a configured 500x500 always yields 500x500, clarify that we intentionally
do NOT multiply output dimensions by window.devicePixelRatio (so exported pixels
reflect the source resolution, not DPR upsampling), and mention that preserving
configured-size behavior is a separate concern for future %-based crop state
work.
🪄 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: f017d9b1-5248-4cee-9194-0168a9318aa7
📒 Files selected for processing (1)
src/main/resources/META-INF/resources/frontend/src/image-crop.tsx
|
✅ Action performedReview finished.
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
1 participant
Problem
The cropped output was sized in the image's displayed (rendered) pixels and
multiplied by
window.devicePixelRatio, instead of using the image's naturalresolution. This caused two related bugs:
out smaller than the configured size (e.g. a 500×500 crop yielded 375×375),
because the output was taken from the scaled-down on-screen size.
devicePixelRatio, so the cropped image ended up larger than the originaland resampled (e.g. a 900×640 image produced 1080×768).
Fix
Size the output canvas and the
drawImagedestination in the crop region'snatural pixels (
ccrop.width * scaleX/ccrop.height * scaleY) and dropthe
devicePixelRatiomultiplier. The cropped image now keeps the configureddimensions regardless of how the image is displayed, and the original is used
verbatim. The same correction applies to circular crops, which previously used
the displayed size because the branch reset the canvas context mid-draw.
Note
Cropped images are now returned at their natural resolution, so their pixel
dimensions may differ from the previous (incorrect) output. No public API
changes — method signatures and events are unchanged.
Closes #26
Closes #21
Summary by CodeRabbit