Conversation
WalkthroughAdds an "assets" job to the CI workflow that checks out the PR, sets up Bun, installs dependencies, runs the asset optimization script, and fails if optimization produces uncommitted changes. The optimize-assets.js script was reworked to use disk-based temporary files with a Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
🔇 Additional comments (9)
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: 3
🧹 Nitpick comments (1)
scripts/optimize-assets.js (1)
69-104: Consider adding cleanup for temporary files on error.If the script encounters an error during optimization, temporary
.optimizedfiles could be left behind in the filesystem. Consider wrapping the main loop in error handling to ensure cleanup.async function main() { + const tempFiles = []; + try { for (const file of walk_directory(join(__dirname, '../static'))) { const relative_path = get_relative_path(file); const is_animated = file.endsWith('.gif'); if (!is_image(file)) continue; if (exceptions.some((exception) => relative_path.startsWith(exception))) continue; const image = sharp(file, { animated: is_animated }); const meta = await image.metadata(); const buffer = await image[meta.format](config[meta.format]) .resize(resize_config) .toBuffer(); await sharp(buffer).toFile(file + '.optimized'); + tempFiles.push(file + '.optimized'); const size_before = Bun.file(file).size; const size_after = Bun.file(file + '.optimized').size; const size_diff = size_before - size_after; const size_diff_percent = size_diff / size_before; if (size_diff <= 0 || size_diff_percent < 0.2) { await Bun.file(file + '.optimized').delete(); + tempFiles.pop(); continue; } await Bun.file(file).delete(); await Bun.write(file, Bun.file(file + '.optimized')); await Bun.file(file + '.optimized').delete(); + tempFiles.pop(); const diff_verbose = Math.round(size_diff_percent * 100); console.log(`✅ ${relative_path} has been optimized (-${diff_verbose}%)`); } + } catch (error) { + // Clean up any remaining temp files + for (const tempFile of tempFiles) { + try { + await Bun.file(tempFile).delete(); + } catch {} + } + throw error; + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/tests.yml(1 hunks)scripts/optimize-assets.js(1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: tests
🔇 Additional comments (3)
scripts/optimize-assets.js (1)
101-102: LGTM!Clear and informative logging that shows the optimization percentage. The use of
Math.roundfor display formatting is appropriate..github/workflows/tests.yml (2)
80-103: Well-structured asset optimization validation job.The workflow correctly validates that assets are optimized by running the optimization script and checking for uncommitted changes. The error messaging is clear and actionable.
One consideration: this job's reliability depends on the safety of the optimization script. Given the file loss risk identified in
scripts/optimize-assets.js(lines 97-99), ensure that script is hardened before relying on it in CI to avoid potential asset corruption.
94-102: Clear validation logic with helpful error messaging.The use of
git diff --exit-codecorrectly detects uncommitted changes, and the error output provides both the diff and clear instructions for remediation.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
scripts/optimize-assets.js (3)
106-107: CRITICAL: Lazy-loaded.sizeproperty still not fixed.The issue flagged in the past review remains unresolved.
Bun.file().sizeis lazy-loaded and will return invalid values until the file contents are read. Both size measurements will be incorrect, causing the optimization logic to fail.Apply the fix from the past review:
+ // Read files to ensure .size is loaded + const originalFile = Bun.file(file); + const optimizedFile = Bun.file(file + '.optimized'); + await originalFile.arrayBuffer(); + await optimizedFile.arrayBuffer(); + - const size_before = Bun.file(file).size; - const size_after = Bun.file(file + '.optimized').size; + const size_before = originalFile.size; + const size_after = optimizedFile.size;
109-110: Edge case: Division by zero still unhandled.The issue flagged in the past review remains unresolved. If
size_beforeis 0 (corrupted or empty file), line 110 will cause division by zero.Apply the guard from the past review:
const size_diff = size_before - size_after; +if (size_before === 0) { + await Bun.file(file + '.optimized').delete(); + continue; +} const size_diff_percent = size_diff / size_before;
116-118: CRITICAL: Non-atomic file replacement still not fixed.The issue flagged in the past review remains unresolved. The delete-write-delete sequence is non-atomic and creates a window where the original file can be permanently lost if line 117 fails after line 116 deletes the original. This could happen due to disk space, permissions, or I/O errors.
Apply the atomic replacement from the past review:
- await Bun.file(file).delete(); - await Bun.write(file, Bun.file(file + '.optimized')); - await Bun.file(file + '.optimized').delete(); + try { + const optimizedContent = await Bun.file(file + '.optimized').arrayBuffer(); + await Bun.write(file, optimizedContent); + await Bun.file(file + '.optimized').delete(); + } catch (error) { + // Clean up temp file if replacement fails + try { + await Bun.file(file + '.optimized').delete(); + } catch {} + throw new Error(`Failed to replace ${relative_path}: ${error.message}`); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
.github/workflows/tests.yml(1 hunks)scripts/optimize-assets.js(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- .github/workflows/tests.yml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: build
- GitHub Check: tests
🔇 Additional comments (5)
scripts/optimize-assets.js (5)
29-33: LGTM - GIF optimization disabled.The GIF configuration is properly commented out. This appears intentional and the implementation is clean.
83-86: LGTM - Sharp initialization with proper safeguards.Good addition of
limitInputPixels: 1920 * 1920to prevent excessive memory usage with large images, and the animated flag is properly preserved.
88-98: LGTM - Robust metadata error handling.Excellent addition of try/catch around metadata retrieval with appropriate CI vs non-CI behavior. This prevents the entire optimization from failing due to a single corrupted or problematic file.
104-104: LGTM - Disk-based temporary file approach.Writing to a
.optimizedtemporary file is a good approach that enables size comparison before modifying the original.
112-115: LGTM - Conditional optimization with 20% threshold.Good logic to skip optimization when the size reduction is not positive or below 20%. This ensures only meaningful optimizations are applied and properly cleans up the temporary file when not needed.
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (8)
static/email/discord.pngis excluded by!**/*.pngstatic/email/x.pngis excluded by!**/*.pngstatic/images/avatars/snezhanna.pngis excluded by!**/*.pngstatic/images/avatars/steven.pngis excluded by!**/*.pngstatic/images/blog/appwrite-pricing-update/add-ons.pngis excluded by!**/*.pngstatic/images/blog/appwrite-pricing-update/bandwidth-project.pngis excluded by!**/*.pngstatic/images/blog/appwrite-pricing-update/one-subscription.pngis excluded by!**/*.pngstatic/images/blog/appwrite-pricing-update/project.pngis excluded by!**/*.png
📒 Files selected for processing (3)
.github/workflows/tests.yml(1 hunks).gitignore(1 hunks)scripts/optimize-assets.js(1 hunks)
2 participants
What does this PR do?
Adds assetstest for CI/CD, ensuring all files are optmal
Test Plan
CI/CD should pass
Related PRs and Issues
x
Have you read the Contributing Guidelines on issues?
Yes
Summary by CodeRabbit
Tests
Chores