Temporary File Cleanup (PNG Optimization) #254
Conversation
WalkthroughThe pull request focuses on enhancing the Changes
Possibly related PRs
Suggested reviewers
Poem
Finishing Touches
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 (
|
![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)
_api/src/optimizeImages.ts (1)
62-82: Consider architectural improvements for better scalability.The current implementation could benefit from several architectural improvements:
- Add progress reporting for long-running operations
- Validate file size before optimization
- Implement concurrent optimization limits
Consider implementing these improvements:
- Add an EventEmitter or callback for progress reporting
- Add size checks before optimization
- Use a worker pool for concurrent optimizations
Example implementation:
import { Worker } from 'worker_threads'; import { EventEmitter } from 'events'; class ImageOptimizer extends EventEmitter { private workerPool: Worker[]; constructor(maxConcurrent = 4) { super(); this.workerPool = new Array(maxConcurrent); } async optimizePng(filePath: string) { const stats = await fs.promises.stat(filePath); if (stats.size <= PNG_SIZE_LIMIT) { this.emit('skip', filePath); return; } this.emit('start', filePath); // ... optimization logic ... this.emit('complete', filePath); } }
| // Replace original file with the optimized file | ||
| fs.renameSync(tempFilePath, filePath); | ||
| } catch (error) { | ||
| console.error(chalk.red(`Error optimizing PNG: ${filePath}`), error); | ||
|
|
||
| if (metadata.height && metadata.height > PNG_MAX_HEIGHT) { | ||
| await image.resize({ height: PNG_MAX_HEIGHT }).toFile(tempFilePath) | ||
| } else { | ||
| const data = await image.toBuffer() | ||
| await sharp(data).toFile(tempFilePath) | ||
| // Cleanup temporary file in case of errors | ||
| if (fs.existsSync(tempFilePath)) { | ||
| fs.unlinkSync(tempFilePath); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace synchronous operations with async alternatives.
The current implementation uses synchronous file operations which could block the event loop. Additionally, the file replacement isn't atomic and could leave the system in an inconsistent state if the process crashes between operations.
Apply this diff to improve file operations:
// Replace original file with the optimized file
- fs.renameSync(tempFilePath, filePath);
+ await fs.promises.rename(tempFilePath, filePath);
} catch (error) {
console.error(chalk.red(`Error optimizing PNG: ${filePath}`), error);
// Cleanup temporary file in case of errors
- if (fs.existsSync(tempFilePath)) {
- fs.unlinkSync(tempFilePath);
- }
+ try {
+ await fs.promises.unlink(tempFilePath);
+ } catch (cleanupError) {
+ // Ignore cleanup errors
+ }
}Also, consider using a more robust file replacement strategy:
// Add this helper function
async function safeReplaceFile(tempPath: string, targetPath: string) {
const backupPath = `${targetPath}.${uuidv4()}.bak`;
await fs.promises.rename(targetPath, backupPath);
try {
await fs.promises.rename(tempPath, targetPath);
await fs.promises.unlink(backupPath);
} catch (error) {
// Restore backup if replacement fails
await fs.promises.rename(backupPath, targetPath);
throw error;
}
}| const image = sharp(filePath); | ||
| const metadata = await image.metadata(); | ||
| const tempFilePath = `${filePath}.${uuidv4()}.tmp`; | ||
|
|
||
| if (metadata.height && metadata.height > PNG_MAX_HEIGHT) { | ||
| await image.resize({ height: PNG_MAX_HEIGHT }).toFile(tempFilePath); | ||
| } else { | ||
| const data = await image.toBuffer(); | ||
| await sharp(data).toFile(tempFilePath); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance image processing robustness and efficiency.
The current implementation has several areas for improvement:
- Missing validation of image format
- Unnecessary buffer conversion for non-resized images
- No memory optimization for large images
Apply this diff to address these issues:
const image = sharp(filePath);
const metadata = await image.metadata();
+ if (metadata.format !== 'png') {
+ throw new Error(`Invalid image format: ${metadata.format}. Expected PNG.`);
+ }
if (metadata.height && metadata.height > PNG_MAX_HEIGHT) {
- await image.resize({ height: PNG_MAX_HEIGHT }).toFile(tempFilePath);
+ await image
+ .resize({ height: PNG_MAX_HEIGHT })
+ .withMetadata()
+ .toFile(tempFilePath);
} else {
- const data = await image.toBuffer();
- await sharp(data).toFile(tempFilePath);
+ await image
+ .withMetadata()
+ .toFile(tempFilePath);
}📝 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.
| const image = sharp(filePath); | |
| const metadata = await image.metadata(); | |
| const tempFilePath = `${filePath}.${uuidv4()}.tmp`; | |
| if (metadata.height && metadata.height > PNG_MAX_HEIGHT) { | |
| await image.resize({ height: PNG_MAX_HEIGHT }).toFile(tempFilePath); | |
| } else { | |
| const data = await image.toBuffer(); | |
| await sharp(data).toFile(tempFilePath); | |
| } | |
| const image = sharp(filePath); | |
| const metadata = await image.metadata(); | |
| const tempFilePath = `${filePath}.${uuidv4()}.tmp`; | |
| if (metadata.format !== 'png') { | |
| throw new Error(`Invalid image format: ${metadata.format}. Expected PNG.`); | |
| } | |
| if (metadata.height && metadata.height > PNG_MAX_HEIGHT) { | |
| await image | |
| .resize({ height: PNG_MAX_HEIGHT }) | |
| .withMetadata() | |
| .toFile(tempFilePath); | |
| } else { | |
| await image | |
| .withMetadata() | |
| .toFile(tempFilePath); | |
| } |
| try { | ||
| const image = sharp(filePath); | ||
| const metadata = await image.metadata(); | ||
| const tempFilePath = `${filePath}.${uuidv4()}.tmp`; | ||
|
|
||
| if (metadata.height && metadata.height > PNG_MAX_HEIGHT) { | ||
| await image.resize({ height: PNG_MAX_HEIGHT }).toFile(tempFilePath); | ||
| } else { | ||
| const data = await image.toBuffer(); | ||
| await sharp(data).toFile(tempFilePath); | ||
| } | ||
|
|
||
| // Use a temporary file for the output | ||
| const tempFilePath = `${filePath}.${uuidv4()}.tmp` | ||
| // Replace original file with the optimized file | ||
| fs.renameSync(tempFilePath, filePath); | ||
| } catch (error) { | ||
| console.error(chalk.red(`Error optimizing PNG: ${filePath}`), error); | ||
|
|
||
| if (metadata.height && metadata.height > PNG_MAX_HEIGHT) { | ||
| await image.resize({ height: PNG_MAX_HEIGHT }).toFile(tempFilePath) | ||
| } else { | ||
| const data = await image.toBuffer() | ||
| await sharp(data).toFile(tempFilePath) | ||
| // Cleanup temporary file in case of errors | ||
| if (fs.existsSync(tempFilePath)) { | ||
| fs.unlinkSync(tempFilePath); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix variable scope and enhance error messaging.
The tempFilePath variable is declared inside the try block but referenced in the catch block. This could lead to a reference error if the error occurs before the variable is initialized.
Apply this diff to fix the scope and improve error messaging:
+ const tempFilePath = `${filePath}.${uuidv4()}.tmp`;
try {
const image = sharp(filePath);
const metadata = await image.metadata();
- const tempFilePath = `${filePath}.${uuidv4()}.tmp`;
if (metadata.height && metadata.height > PNG_MAX_HEIGHT) {
await image.resize({ height: PNG_MAX_HEIGHT }).toFile(tempFilePath);
} else {
const data = await image.toBuffer();
await sharp(data).toFile(tempFilePath);
}
// Replace original file with the optimized file
fs.renameSync(tempFilePath, filePath);
} catch (error) {
- console.error(chalk.red(`Error optimizing PNG: ${filePath}`), error);
+ console.error(chalk.red(`Failed to optimize PNG ${filePath}: ${error.message}`));
+ console.error(chalk.red(`Stack trace: ${error.stack}`));
// Cleanup temporary file in case of errors
if (fs.existsSync(tempFilePath)) {
fs.unlinkSync(tempFilePath);
}
}📝 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.
| try { | |
| const image = sharp(filePath); | |
| const metadata = await image.metadata(); | |
| const tempFilePath = `${filePath}.${uuidv4()}.tmp`; | |
| if (metadata.height && metadata.height > PNG_MAX_HEIGHT) { | |
| await image.resize({ height: PNG_MAX_HEIGHT }).toFile(tempFilePath); | |
| } else { | |
| const data = await image.toBuffer(); | |
| await sharp(data).toFile(tempFilePath); | |
| } | |
| // Use a temporary file for the output | |
| const tempFilePath = `${filePath}.${uuidv4()}.tmp` | |
| // Replace original file with the optimized file | |
| fs.renameSync(tempFilePath, filePath); | |
| } catch (error) { | |
| console.error(chalk.red(`Error optimizing PNG: ${filePath}`), error); | |
| if (metadata.height && metadata.height > PNG_MAX_HEIGHT) { | |
| await image.resize({ height: PNG_MAX_HEIGHT }).toFile(tempFilePath) | |
| } else { | |
| const data = await image.toBuffer() | |
| await sharp(data).toFile(tempFilePath) | |
| // Cleanup temporary file in case of errors | |
| if (fs.existsSync(tempFilePath)) { | |
| fs.unlinkSync(tempFilePath); | |
| } | |
| const tempFilePath = `${filePath}.${uuidv4()}.tmp`; | |
| try { | |
| const image = sharp(filePath); | |
| const metadata = await image.metadata(); | |
| if (metadata.height && metadata.height > PNG_MAX_HEIGHT) { | |
| await image.resize({ height: PNG_MAX_HEIGHT }).toFile(tempFilePath); | |
| } else { | |
| const data = await image.toBuffer(); | |
| await sharp(data).toFile(tempFilePath); | |
| } | |
| // Replace original file with the optimized file | |
| fs.renameSync(tempFilePath, filePath); | |
| } catch (error) { | |
| console.error(chalk.red(`Failed to optimize PNG ${filePath}: ${error.message}`)); | |
| console.error(chalk.red(`Stack trace: ${error.stack}`)); | |
| // Cleanup temporary file in case of errors | |
| if (fs.existsSync(tempFilePath)) { | |
| fs.unlinkSync(tempFilePath); | |
| } |
Checks if the temporary file exists using
fs.existsSync.Deletes the file using
fs.unlinkSync.Summary by CodeRabbit