Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Temporary File Cleanup (PNG Optimization) #254

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Temporary File Cleanup (PNG Optimization) #254

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
32 changes: 20 additions & 12 deletions _api/src/optimizeImages.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,23 +59,31 @@ export async function optimizeImages(dir: string) {

// Optimize PNG file
async function optimizePng(filePath: string) {
const image = sharp(filePath)
const metadata = await image.metadata()
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);
}
Comment on lines +63 to +72
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ Refactor suggestion

Enhance image processing robustness and efficiency.

The current implementation has several areas for improvement:

  1. Missing validation of image format
  2. Unnecessary buffer conversion for non-resized images
  3. 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.

Suggested change
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);
}


// 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);
}
Comment on lines +74 to +82
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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;
  }
}

Comment on lines +62 to +82
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🛠️ 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.

Suggested change
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);
}

}

// Replace the original file with the optimized file
fs.renameSync(tempFilePath, filePath)
}


// Optimize SVG file
function optimizeSvg(filePath: string) {
const data = fs.readFileSync(filePath, "utf8")
Expand Down