Lambda to allow gif types #1143
Conversation
Also some general lambda cleanup
|
@NiallJoeMaher is attempting to deploy a commit to the Codú Team on Vercel. A member of the Team first needs to authorize it. |
|
Caution Review failedThe pull request is closed. WalkthroughThe pull request includes modifications to two AWS Lambda functions, Changes
Possibly related PRs
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
NiallJoeMaher
temporarily deployed
to
production
GitHub Actions
Inactive
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
cdk/lambdas/avatarResize/index.js (1)
Line range hint
1-69: Missing implementation for GIF handlingWhile the code changes improve clarity and consistency, the main objective of the PR to "implement a conditional check that prevents GIF files from being converted to the WebP format" is not addressed in this file. Consider adding logic to skip processing for GIF files.
Here's a suggested implementation to handle GIF files:
const imageBuffer = Buffer.concat(await stream.toArray()); + // Check if the file is a GIF + if (contentType === 'image/gif') { + console.log('GIF file detected. Skipping resize operation.'); + return { + statusCode: 200, + body: JSON.stringify({ message: "GIF file. Skipping resize." }), + }; + } // Resize the image const resizedImageBuffer = await sharp(imageBuffer) .resize({ width: 220, height: 220, fit: "cover" }) .toBuffer();This change will prevent GIF files from being resized, addressing the PR's main objective. Would you like me to implement this change or create a GitHub issue to track this task?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
- cdk/lambdas/avatarResize/index.js (3 hunks)
- cdk/lambdas/uploadResize/index.js (1 hunks)
🧰 Additional context used
🔇 Additional comments (5)
cdk/lambdas/avatarResize/index.js (4)
22-22: Improved variable naming for clarityThe change from
metadatatos3Metadataenhances code readability by explicitly indicating the source of the metadata. This aligns well with the PR's objective of general cleanup and improved code quality.
25-25: Consistent use of renamed variableThe condition has been updated to use
s3Metadata, maintaining consistency with the earlier variable renaming. This change preserves the existing logic while improving code clarity.
40-40: Code structure and simplification improvements
- The added blank line (line 40) enhances readability by clearly separating logical sections of the code.
- The simplification of the Sharp object creation (line 46) makes the code more concise without changing its functionality.
Both changes contribute to the overall code cleanup objective of the PR.
Also applies to: 46-48
56-56: Consistent metadata handlingThe use of
s3Metadatain thePutObjectCommandensures consistency with the earlier variable renaming. This change maintains the existing functionality of preserving all original metadata and adding theresizedflag, while improving overall code consistency.cdk/lambdas/uploadResize/index.js (1)
65-66:⚠️ Potential issueEnsure proper handling of animated GIFs during resizing.
The
sharplibrary does not automatically handle animated GIFs correctly. By default, when processing GIFs,sharponly processes the first frame, which means animated GIFs will lose their animation after resizing. To preserve the animation across all frames, you need to specify theanimatedoption in your sharp operations.Consider modifying your code to handle animated GIFs properly:
-const imageMetadata = await sharp(imageRaw).metadata(); +const imageMetadata = await sharp(imageRaw, { animated: true }).metadata(); const isGif = imageMetadata.format === "gif";And update the
resizeImagefunction:const resizeImage = async (buffer, size) => { - const resizedImage = sharp(buffer).resize({ + const resizedImage = sharp(buffer, { animated: isGif }).resize({ width: size.maxWidth, height: size.maxHeight, fit: "inside", withoutEnlargement: false, }); if (isGif) { - return resizedImage.gif().toBuffer(); + return resizedImage + .gif({ reoptimise: false }) + .toBuffer(); } else { return resizedImage.webp({ quality: 80 }).toBuffer(); } };This ensures that all frames of an animated GIF are processed and the animation is preserved after resizing.
| : key; | ||
|
|
||
| await s3.send( | ||
| new PutObjectCommand({ | ||
| Bucket: bucket, | ||
| Key: resizedKey, | ||
| Body: resizedImage, | ||
| ContentType: isGif ? "image/gif" : "image/webp", |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Set appropriate Content-Encoding header for WebP images.
When serving WebP images, it's beneficial to include the Content-Encoding header to ensure proper handling by clients and proxies that support WebP compression.
Modify the PutObjectCommand parameters:
await s3.send(
new PutObjectCommand({
Bucket: bucket,
Key: resizedKey,
Body: resizedImage,
ContentType: isGif ? "image/gif" : "image/webp",
+ ContentEncoding: isGif ? undefined : "webp",
}),
);This addition helps improve compatibility and performance with clients that support WebP.
📝 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.
| ContentType: isGif ? "image/gif" : "image/webp", | |
| ContentType: isGif ? "image/gif" : "image/webp", | |
| ContentEncoding: isGif ? undefined : "webp", |
🛠️ Refactor suggestion
Set the correct Cache-Control header for optimized content delivery.
Setting appropriate cache headers can improve the performance of your application by enabling clients and intermediaries to cache the resized images. This reduces load on your server and decreases image load times for users.
Add the CacheControl parameter to your PutObjectCommand:
await s3.send(
new PutObjectCommand({
Bucket: bucket,
Key: resizedKey,
Body: resizedImage,
ContentType: isGif ? "image/gif" : "image/webp",
+ CacheControl: "max-age=31536000, public",
}),
);This sets the cache to expire after one year (31536000 seconds), which is appropriate for versioned files that won't change.
📝 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.
| ContentType: isGif ? "image/gif" : "image/webp", | |
| ContentType: isGif ? "image/gif" : "image/webp", | |
| CacheControl: "max-age=31536000, public", |
| ? key.replace( | ||
| /(\.[\w\d_-]+)$/i, | ||
| `_${size.suffix}${isGif ? ".gif" : ".webp"}`, | ||
| ) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use the Node.js path module for reliable file extension handling.
The regular expression used to replace the file extension may not handle all possible file names correctly, such as those without an extension or with multiple dots in their names. Using the built-in path module provides a more robust solution for handling file paths and extensions.
Modify your code to use the path module:
+const path = require("path");
...
const resizedKey = size.suffix
? key.replace(
- /(\.[\w\d_-]+)$/i,
- `_${size.suffix}${isGif ? ".gif" : ".webp"}`,
+ path.extname(key),
+ `_${size.suffix}${isGif ? ".gif" : ".webp"}`
)
: key;This change ensures that the file extension is accurately identified and replaced, regardless of the file name format.
Committable suggestion was skipped due to low confidence.
cdk/lambdas/uploadResize/index.js
Outdated
| if (isGif) { | ||
| return resizedImage.gif().toBuffer(); | ||
| } else { | ||
| return resizedImage.webp({ quality: 80 }).toBuffer(); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize image processing by reusing the resizedImage instance.
Instead of calling .toBuffer() directly after formatting, you can chain the output format method and call .toBuffer() once. However, since resizedImage is already an instance returned by sharp(buffer).resize(), you can streamline the code.
Simplify the return statements:
if (isGif) {
- return resizedImage
- .gif({ reoptimise: false })
- .toBuffer();
+ resizedImage.gif({ reoptimise: false });
} else {
return resizedImage.webp({ quality: 80 }).toBuffer();
}
+return resizedImage.toBuffer();Alternatively, you can write:
+const formatOptions = isGif ? { reoptimise: false } : { quality: 80 };
+const formatMethod = isGif ? 'gif' : 'webp';
+resizedImage[formatMethod](formatOptions);
+return resizedImage.toBuffer();This approach reduces code duplication and makes the code more maintainable.
Committable suggestion was skipped due to low confidence.
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
NiallJoeMaher
temporarily deployed
to
production
GitHub Actions
Inactive
|
Nice @NiallJoeMaher Does this mean we can have gifs for avatars now? |
I didn't enable it on this one it was for articles and discussions. But... We can get that working easily with the same logic 😂 |
✨ Codu Pull Request 💻
Fixs #998
Pull Request details
Any Breaking changes