[paodb]

Changing the image source now discards the cropped image of the previous one. Each cropped image is tied to the image it was cropped from through a revision counter: the server increments it when the image changes, the client records it when the image loads and sends it back with every crop, and getCroppedImageDataUri() only returns a crop whose revision matches. This also ignores late crops of a previous image, and detects the output format from the image actually drawn instead of image.src.

Breaking changes:

• getCroppedImageDataUri() and getCroppedImageBase64() return null until the image is cropped (ImageCrop(String) no longer reports the image source as the cropped image). To get the full image without user interaction, set an initial crop such as new Crop("%", 0, 0, 100, 100).
• addCroppedImageListener was removed, and the CroppedImageEvent constructor takes an additional image revision parameter.

The demos show "Select an area first" when there is no cropped image yet.

Close #38
Close #41

[coderabbitai]

No actionable comments were generated in the recent review.

ℹ️ Recent review info

⚙️ Run configuration

• Configuration used: Organization UI
• Review profile: CHILL
• Plan: Advanced
• Run ID: 07b598ba-36b7-494d-a6d5-a6bade6f5322

📥 Commits

Reviewing files that changed from the base of the PR and between fd10ffc and bb7293d.

📒 Files selected for processing: 5

• README.md
• src/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.java
• src/test/java/com/flowingcode/vaadin/addons/imagecrop/BasicImageCropDemo.java
• src/test/java/com/flowingcode/vaadin/addons/imagecrop/OutputFormatImageCropDemo.java
• src/test/java/com/flowingcode/vaadin/addons/imagecrop/test/ImageCropTest.java

Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review.

Walkthrough

The change updates cropped-image state for string image sources. It also updates tests and examples to handle a null cropped-image data URI.

Changes

Cropped-image state lifecycle

Layer / File(s)	Summary
Source changes and null-safe crop output src/main/java/.../ImageCrop.java, src/test/java/.../ImageCropTest.java, src/test/java/.../BasicImageCropDemo.java, src/test/java/.../OutputFormatImageCropDemo.java, README.md	The constructor no longer initializes the cropped-image URI from the source URL. A different string source clears the URI, while setting the current source preserves it. Tests cover source changes and initial state. The README and demo handlers check for a non-null URI before displaying an image.

Priority: Low

Estimated code review effort: 2 (Simple) | ~10 minutes

Change: Bug fix · Severity of issue fixed: Low

Merge Risk: Low · up to bb729

If an image source changes while a crop result is in flight, the previous image’s result may still be returned. This is a narrow stale-output risk; the rest of the change appears ready to merge.

Pre-merge checks: 4

Check name	Status	Explanation
Linked Issues check		Issue #38 requires setImageSrc(String) to clear the previous cropped result when the image source changes. ImageCrop.setImageSrc(String) clears croppedImageDataUri when the source changes and re…
Out of Scope Changes check		The changes stay within issue #38 and the stated breaking behavior. README and demo updates handle the documented null result. ImageCropTest adds coverage for source changes and constructor state. N…
Title check		The title clearly and concisely describes the main change: resetting the cropped image when the image source changes.
Description Check		Check skipped - CodeRabbit’s high-level summary is enabled.

Full details: Linked Issues check

Explanation

Issue #38 requires setImageSrc(String) to clear the previous cropped result when the image source changes. ImageCrop.setImageSrc(String) clears croppedImageDataUri when the source changes and retains it only when the same non-resource source is set. The String constructor no longer initializes a cropped result. Tests cover changed String sources, Base64 clearing, same-source retention, resource URL changes, and null initial results.

Full details: Out of Scope Changes check

Explanation

The changes stay within issue #38 and the stated breaking behavior. README and demo updates handle the documented null result. ImageCropTest adds coverage for source changes and constructor state. No unrelated product behavior appears in the supplied change summary or inspected files.

✨ Finishing Touches: 💡 1

📝 Generate docstrings: 💡

• Commit to this branch
• Create a new PR

🧪 Generate unit tests (beta)

• Commit to this branch
• Create a new PR

• Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[javier-godoy]

Thanks for the fix! Requesting changes for the stale-event handling and a few demo/README issues (inline).

Tie each cropped image to the image it came from. A cropped-image event doesn't say which image it was produced from, so resetting the field in the setters isn't enough:

1. A late event undoes the reset. If the user crops image A and, before that event is processed, server code calls setImageSrc("b.png") (e.g. another listener in the same request, or a ui.access push), the field is cleared and then the late event sets it back to A's crop. While B is shown, getCroppedImageDataUri() returns a crop of A, and if B has no crop defined it stays that way. (The same race already exists in setImageSrc(DownloadHandler).)
2. A → B → A in one round trip loses the crop. sameImage only compares against the server state, so setImageSrc("b.png"); setImageSrc("a.png"); clears the field, but the client never sees a change, never reloads and never re-crops. getCroppedImageDataUri() returns null although A still has a crop, contradicting the new Javadoc. On master this case works.

Suggested approach: include the source of the cropped image in the event detail, store the crop together with that source, and have getCroppedImageDataUri()/getCroppedImageBase64() return it only when it matches getImageSrc() (or ignore non-matching events in updateCroppedImage). That fixes both cases and removes the need for the sameImage bookkeeping.

Please record that source in onLoad (the image whose pixels are actually on the canvas) rather than reading image.src when encoding: that way the same change also fixes #41, where image.src already points to the new image while the old one is still being encoded. Also keep in mind that the DownloadHandler resource URL is regenerated in onAttach, so the stored source should follow it there (or use a server-controlled id instead of the URL).

[paodb]

@javier-godoy Done in 96f5051. Each cropped-image event now carries the source of the image it was cropped from, recorded in onLoad. The server only returns a crop that matches getImageSrc(), so a late event for a previous image is ignored and A → B → A keeps A's crop. I removed the sameImage bookkeeping and the resets in the setters. The output type is also detected from the loaded source instead of image.src, so this fixes #41 too. On attach, the stored source follows the regenerated DownloadHandler URL.

[javier-godoy]

The main concern is the first three inline comments: the image source is sent back with every crop event, and checking it on the server is expensive for data-URI sources. The revision counter suggested on onAttach would address both.

One more comment, on a line outside the diff:

ImageCrop/src/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.java

Line 538 in a8bddb9

* @return byte[] the decoded byte array of the cropped image

The null return isn't new: the isBlank check is already on master. What this PR changes is how often it happens. Before, the String constructor stored the image URL as the "cropped image", so this method had something to decode (and failed on a URL, which this PR fixes). Now it returns null until the user crops, or until a configured crop is applied when the image loads.

A caller who reads the current @return ("the decoded byte array of the cropped image") will assume it never returns null, and calls like getCroppedImageBase64().length will throw NullPointerException. Suggested text: "@return the decoded bytes of the cropped image, or null if the image has not been cropped yet".

[javier-godoy]

Everything in the event's detail is serialized to JSON and sent from the browser to the server. Adding imageSrc sends the image source along with every cropped-image event.

With a normal URL (images/photo.png) that's a few bytes. A data URI is different: it contains the entire image file encoded as base64, which makes it about 33% bigger than the file. UploadImageCropDemo builds exactly that from an upload of up to 10 MB, so imageSrc can be about 13 MB. The event fires on every onComplete, which happens each time the user releases the mouse after moving or resizing the selection. Every one of those uploads the full original image back to the server, which already has it.

It also costs memory on the server. Vaadin turns the JSON into a new String, and that is stored here:

ImageCrop/src/main/java/com/flowingcode/vaadin/addons/imagecrop/ImageCrop.java

Line 104 in a8bddb9

this.croppedImageSrc = event.getImageSrc();

The component state already holds one copy, so each session now holds two copies of the source.

To see it: run the upload demo, upload a large photo, open the browser DevTools Network tab, and drag the selection a few times. Each request carries the whole image.

The server doesn't need the image back, only something that says which image was cropped. The comment on onAttach suggests a way to do that.

[paodb]

Done in 3199648: the event now carries the image revision recorded in onLoad instead of the source.

[paodb]

@javier-godoy Updated the @return of getCroppedImageBase64() with your text in 3199648.

[javier-godoy]

LGTM, please squash the WIP commits, and update the commit message so that the BREAKING CHANGE footer also covers the removal of addCroppedImageListener and the new CroppedImageEvent constructor, and add Close #41.

Also, please add "This event is for internal use only." to the CroppedImageEvent class Javadoc.

[sonarqubecloud]

Quality Gate passed

Issues
2 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud