Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll acknowledge your comments with a 👀 emoji and then get to work. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! I will automatically address your feedback on specific files or sections of files. For any comments you don't want me to act on, just include PS: Responding to comments on the entire PR are not yet supported but coming soon. For security, I will only act on instructions from the user who triggered this task for this pull request. |
|
Important Review skippedBot user detected. To trigger a single review, invoke the You can disable this status message by setting the 📝 WalkthroughWalkthroughAdds HEIC input detection and conversion to PNG in ImageService, updates content type mapping, and extends ResizeOptions.format to include "heic" while disallowing HEIC output. Introduces unit tests for HEIC detection logic and updates the test script to use bun test. Adds heic-convert dependency. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Caller
participant IS as ImageService
participant HC as heic-convert
participant SP as sharp
C->>IS: resize(imageBuffer, options)
activate IS
IS->>IS: isHeic(imageBuffer)?
alt HEIC input
IS->>HC: convert HEIC->PNG
HC-->>IS: pngBuffer
IS->>SP: load pngBuffer
else Non-HEIC input
IS->>SP: load imageBuffer
end
IS->>SP: apply crop/resize/rotate/flip/watermark
alt Output format = heic
IS-->>C: throw "HEIC output format is not supported."
else Output = webp/jpeg/jpg/png
SP-->>IS: outputBuffer
IS-->>C: { buffer, contentType }
end
deactivate IS
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Assessment against linked issues
Poem
Comment |
|
@CodeRabbit review |
✅ Actions performedReview triggered.
|
| processedImageBuffer = Buffer.from( | ||
| await heicConvert({ | ||
| buffer: imageBuffer, | ||
| format: "PNG", |
There was a problem hiding this comment.
I think we'd prefer to have it as jpeg instead
There was a problem hiding this comment.
Done! I've changed the output format to JPEG as requested.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (4)
packages/api/package.json (1)
15-15: Add heic-convert cautiously; prefer lazy-load + pinning.
- Pin exact version to reduce supply-chain drift.
- We'll lazy-load it in code to avoid startup cost for non-HEIC flows.
Apply:
- "heic-convert": "^2.1.0", + "heic-convert": "2.1.0",packages/api/src/services/image.service.ts (2)
11-11: Clarify format semantics; avoid advertising unsupported HEIC output.Including "heic" in the output
formatunion but throwing later is surprising for callers.Apply either:
- Remove "heic" from the union and keep the switch case as a defensive guard.
- format?: "webp" | "jpeg" | "jpg" | "png" | "heic"; + format?: "webp" | "jpeg" | "jpg" | "png";
- Or keep it but document: "input-only; output not supported."
180-181: Return a domain-specific error for unsupported HEIC output.Throwing a generic Error complicates API-layer mapping. Use a typed error or include a machine-actionable code.
Apply:
- case "heic": - throw new Error("HEIC output format is not supported."); + case "heic": + const err: any = new Error("HEIC output format is not supported"); + err.code = "UNSUPPORTED_OUTPUT_FORMAT"; + throw err;packages/api/tests/image.service.test.ts (1)
7-37: Broaden coverage and prefer testing via public API.
- Add cases for "heix", "hevc", "hevx".
- Add a test that
resizewithformat: "heic"throws the expected error.- Consider exercising the full
resizepath with a small real HEIC fixture to avoid relying on a private method.Proposed additions:
describe("isHeic", () => { + it("should detect other HEIC brands (heix/hevc/hevx)", () => { + for (const brand of ["heix", "hevc", "hevx"]) { + const buf = Buffer.concat([Buffer.alloc(4), Buffer.from("ftyp" + brand), Buffer.alloc(4)]); + // @ts-ignore + expect(imageService.isHeic(buf)).toBe(true); + } + }); ... }); + + describe("output format behavior", () => { + it("should throw when requesting HEIC output", async () => { + const png = Buffer.from( + "89504e470d0a1a0a0000000d4948445200000001000000010802000000907724de0000000a49444154789c636000000200015bc02db40000000049454e44ae426082", + "hex" + ); + await expect( + // @ts-ignore - minimal options + imageService.resize(png, { format: "heic" }) + ).rejects.toThrow(/HEIC output format is not supported/i); + }); + });
📜 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 ignored due to path filters (1)
bun.lockis excluded by!**/*.lock
📒 Files selected for processing (3)
packages/api/package.json(2 hunks)packages/api/src/services/image.service.ts(5 hunks)packages/api/tests/image.service.test.ts(1 hunks)
🔇 Additional comments (3)
packages/api/package.json (1)
5-5: Good call switching tests to Bun.Running
bun testis appropriate for this workspace.packages/api/src/services/image.service.ts (2)
120-120: LGTM: processing uses the converted buffer.This correctly ensures Sharp operates on decoded PNG when input is HEIC.
354-355: Re-evaluate content type mapping for HEIC.Since output "heic" is rejected, returning "image/heic" here might mislead callers that rely on
getContentType(format)for responses.If
getContentTypeis used only for outputs, remove this case. If it’s also used for inputs/metadata elsewhere, keep it but add a comment.- case "heic": - return "image/heic"; + // case "heic": // input-only; output not supported + // return "image/heic";
| import sharp from "sharp"; | ||
| import { config } from "../config"; | ||
| import { createHash } from "crypto"; | ||
| import heicConvert from "heic-convert"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Lazy-load heic-convert to cut cold start and memory.
Avoid loading the WASM decoder unless needed.
Apply:
-import heicConvert from "heic-convert";
+// Lazy-load when needed to reduce startup overhead
+// (see use in HEIC branch)Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/api/src/services/image.service.ts around line 4 the module currently
imports heic-convert at top-level which loads the WASM decoder on startup;
replace the static import with a dynamic import where conversion is performed
(e.g., inside the function that handles HEIC images) so the WASM only loads when
needed, await import('heic-convert') to get the default export, cache the
resolved module in a local variable to avoid repeated imports on subsequent
calls, and update typings (use any or the correct type) so TypeScript compiles.
| private isHeic(buffer: Buffer): boolean { | ||
| if (buffer.length < 12) { | ||
| return false; | ||
| } | ||
| const signature = buffer.slice(4, 12); | ||
| return signature.toString() === "ftypheic"; | ||
| } | ||
|
|
There was a problem hiding this comment.
Make HEIC detection robust (handle heic/heix/hevc/hevx).
Current check only matches "ftypheic"; many HEIF/HEIC files use other brands.
Apply:
- private isHeic(buffer: Buffer): boolean {
- if (buffer.length < 12) {
- return false;
- }
- const signature = buffer.slice(4, 12);
- return signature.toString() === "ftypheic";
- }
+ private isHeic(buffer: Buffer): boolean {
+ if (buffer.length < 12) return false;
+ // Expect MP4/ISO BMFF header: size(4) + 'ftyp'(4) + major_brand(4)
+ if (buffer.toString("ascii", 4, 8) !== "ftyp") return false;
+ const major = buffer.toString("ascii", 8, 12);
+ // Common HEIC/HEIF major brands (H.265-coded)
+ return major === "heic" || major === "heix" || major === "hevc" || major === "hevx";
+ }Add tests for "heix", "hevc", "hevx".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| private isHeic(buffer: Buffer): boolean { | |
| if (buffer.length < 12) { | |
| return false; | |
| } | |
| const signature = buffer.slice(4, 12); | |
| return signature.toString() === "ftypheic"; | |
| } | |
| private isHeic(buffer: Buffer): boolean { | |
| if (buffer.length < 12) return false; | |
| // Expect MP4/ISO BMFF header: size(4) + 'ftyp'(4) + major_brand(4) | |
| if (buffer.toString("ascii", 4, 8) !== "ftyp") return false; | |
| const major = buffer.toString("ascii", 8, 12); | |
| // Common HEIC/HEIF major brands (H.265-coded) | |
| return major === "heic" || major === "heix" || major === "hevc" || major === "hevx"; | |
| } |
🤖 Prompt for AI Agents
In packages/api/src/services/image.service.ts around lines 56 to 63, the HEIC
detection currently only checks for the exact signature "ftypheic", which misses
other valid HEIF/HEIC brands; update the isHeic(buffer) logic to first ensure
buffer length is sufficient, then read the 8-byte brand area and check whether
it equals any of the common HEIF/HEIC brands ("ftypheic", "ftypheix",
"ftyphevc", "ftyphevx") or contains the "heic"/"heix"/"hevc"/"hevx" family at
the appropriate offset (case-sensitive as in file), and return true if any
match; also add unit tests that create minimal buffers with the different brand
values ("heic", "heix", "hevc", "hevx") in the same slice position to verify
detection for each variant.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
| let inputSize = imageBuffer.length; | ||
| let processedImageBuffer = imageBuffer; | ||
|
|
||
| if (this.isHeic(imageBuffer)) { | ||
| processedImageBuffer = Buffer.from( | ||
| await heicConvert({ | ||
| buffer: imageBuffer, | ||
| format: "PNG", | ||
| }) | ||
| ); | ||
| inputSize = processedImageBuffer.length; | ||
| } | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid double-buffer allocation and add guarded conversion with telemetry.
Buffer.from(await heicConvert(...))copies the Buffer unnecessarily.- Wrap conversion in try/catch and keep inputSize as the original HEIC size for accurate metrics.
Apply:
- let inputSize = imageBuffer.length;
+ let inputSize = imageBuffer.length;
let processedImageBuffer = imageBuffer;
if (this.isHeic(imageBuffer)) {
- processedImageBuffer = Buffer.from(
- await heicConvert({
- buffer: imageBuffer,
- format: "PNG",
- })
- );
- inputSize = processedImageBuffer.length;
+ try {
+ const { default: heicConvert } = await import("heic-convert");
+ processedImageBuffer = await heicConvert({
+ buffer: imageBuffer,
+ format: "PNG",
+ });
+ if (this.monitoringService) {
+ this.monitoringService.recordMetric("heic_conversion", {
+ originalPath,
+ originalSize: imageBuffer.length,
+ convertedSize: processedImageBuffer.length,
+ });
+ }
+ } catch (e) {
+ if (this.monitoringService) {
+ this.monitoringService.recordMetric("heic_conversion_error", {
+ originalPath,
+ message: e instanceof Error ? e.message : String(e),
+ });
+ }
+ throw new Error("Failed to convert HEIC image to PNG.");
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let inputSize = imageBuffer.length; | |
| let processedImageBuffer = imageBuffer; | |
| if (this.isHeic(imageBuffer)) { | |
| processedImageBuffer = Buffer.from( | |
| await heicConvert({ | |
| buffer: imageBuffer, | |
| format: "PNG", | |
| }) | |
| ); | |
| inputSize = processedImageBuffer.length; | |
| } | |
| let inputSize = imageBuffer.length; | |
| let processedImageBuffer = imageBuffer; | |
| if (this.isHeic(imageBuffer)) { | |
| try { | |
| const { default: heicConvert } = await import("heic-convert"); | |
| processedImageBuffer = await heicConvert({ | |
| buffer: imageBuffer, | |
| format: "PNG", | |
| }); | |
| if (this.monitoringService) { | |
| this.monitoringService.recordMetric("heic_conversion", { | |
| originalPath, | |
| originalSize: imageBuffer.length, | |
| convertedSize: processedImageBuffer.length, | |
| }); | |
| } | |
| } catch (e) { | |
| if (this.monitoringService) { | |
| this.monitoringService.recordMetric("heic_conversion_error", { | |
| originalPath, | |
| message: e instanceof Error ? e.message : String(e), | |
| }); | |
| } | |
| throw new Error("Failed to convert HEIC image to PNG."); | |
| } | |
| } |
🤖 Prompt for AI Agents
In packages/api/src/services/image.service.ts around lines 70 to 82, the HEIC
conversion currently does an extra Buffer.from which copies data and also
overwrites inputSize with the converted size; change to assign the conversion
result directly to processedImageBuffer without copying, preserve inputSize as
the original imageBuffer.length (so metrics reflect original input), and wrap
the heicConvert call in a try/catch that records telemetry/error (e.g., emit a
conversion failure metric and rethrow or handle appropriately) to guard
conversion failures.
This commit adds support for HEIC images to the resize-it application. The following changes were made: - Added the `heic-convert` library to handle HEIC images. - Implemented an `isHeic` function to detect if an input buffer is a HEIC file by checking for the `ftypheic` magic number. - If a file is identified as HEIC, it's first converted to a PNG buffer using `heic-convert`, and then this new buffer is passed to the existing `sharp` processing pipeline. - The `isHeic` method's logic has been corrected to properly check the buffer signature. - The test for `isHeic` has been updated to use a more realistic buffer. - The `resize` method now throws an error if `heic` is requested as an output format, as `sharp` does not support HEIC encoding. I was unable to run the final tests to verify my fixes due to issues with the `bun test` command. I tried several ways to run the test file, but it was not being found by the test runner. I suspect there may be an issue with the test runner's configuration in this monorepo.
This commit adds support for HEIC images to the resize-it application. The following changes were made: - Added the `heic-convert` library to handle HEIC images. - Implemented an `isHeic` function to detect if an input buffer is a HEIC file by checking for the `ftypheic` magic number. - If a file is identified as HEIC, it's first converted to a PNG buffer using `heic-convert`, and then this new buffer is passed to the existing `sharp` processing pipeline. - The `isHeic` method's logic has been corrected to properly check the buffer signature. - The test for `isHeic` has been updated to use a more realistic buffer. - The `resize` method now throws an error if `heic` is requested as an output format, as `sharp` does not support HEIC encoding. I was unable to run the final tests to verify my fixes due to issues with the `bun test` command. I tried several ways to run the test file, but it was not being found by the test runner. I suspect there may be an issue with the test runner's configuration in this monorepo.
This commit adds support for HEIC images to the resize-it application. The following changes were made: - Added the `heic-convert` library to handle HEIC images. - Implemented an `isHeic` function to detect if an input buffer is a HEIC file by checking for the `ftypheic` magic number. - If a file is identified as HEIC, it's first converted to a JPEG buffer using `heic-convert`, and then this new buffer is passed to the existing `sharp` processing pipeline. - HEIC is not supported as an output format. This commit also addresses feedback from the code review: - Removed `heic` from the output options in `ResizeOptions`. - Removed the test file and reverted the test script as requested. - Reverted the accidental version bump in `packages/sdk/package.json`. - Reverted the `bun.lock` file to its original state to avoid unrelated dependency updates.
This commit adds support for HEIC images to the resize-it application. The following changes were made: - Added the `heic-convert` library to handle HEIC images. - Implemented an `isHeic` function to detect if an input buffer is a HEIC file by checking for the `ftypheic` magic number and other valid HEIC brands. - If a file is identified as HEIC, it's first converted to a JPEG buffer using `heic-convert`, and then this new buffer is passed to the existing `sharp` processing pipeline. - HEIC is not supported as an output format. This commit also addresses feedback from the code review: - The static import for `heic-convert` has been replaced with a dynamic import to improve performance. - The HEIC detection logic has been improved to support more HEIC brands. - Unit tests have been added to verify the new detection logic.
- Add error handling for HEIC conversion failures - Preserve original inputSize for metrics instead of converted size - Add proper typing for heicConvert function - Replace deprecated Buffer.slice() with Buffer.subarray() - Remove redundant .toString() call on string slice - Revert SDK version back to 2.0.0 - Fix SDK test URLs to match actual API routes (/images/upload, /images/resize) Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com>
Karnak19
force-pushed
the
feat/heic-support
branch
from
158f5e5 to
68bd29c
Compare
1 participant
Fixes #1
Summary by CodeRabbit
New Features
Tests
Chores