several fixes to various tests

src/routes/admin.content.routes.test.ts

@@ -12,6 +12,7 @@ import {
 } from '../tests/utils/mockFactories';
 import type { SuggestedCorrection, Brand, UserProfile, UnmatchedFlyerItem } from '../types';
 import { NotFoundError } from '../services/db/errors.db'; // This can stay, it's a type/class not a module with side effects.
+import fs from 'node:fs/promises';
 import { createTestApp } from '../tests/utils/createTestApp';
 
 // Mock the file upload middleware to allow testing the controller's internal check
@@ -265,6 +266,22 @@ describe('Admin Content Management Routes (/api/admin)', () => {
       );
     });
 
+    it('should clean up the uploaded file if updating the brand logo fails', async () => {
+      const brandId = 55;
+      const dbError = new Error('DB Connection Failed');
+      vi.mocked(mockedDb.adminRepo.updateBrandLogo).mockRejectedValue(dbError);
+
+      const response = await supertest(app)
+        .post(`/api/admin/brands/${brandId}/logo`)
+        .attach('logoImage', Buffer.from('dummy-logo-content'), 'test-logo.png');
+
+      expect(response.status).toBe(500);
+      // Verify that the cleanup function was called via the mocked fs module
+      expect(fs.unlink).toHaveBeenCalledTimes(1);
+      // The filename is predictable because of the multer config in admin.routes.ts
+      expect(fs.unlink).toHaveBeenCalledWith(expect.stringContaining('logoImage-'));
+    });
+
     it('POST /brands/:id/logo should return 400 for an invalid brand ID', async () => {
       const response = await supertest(app)
         .post('/api/admin/brands/abc/logo')

src/routes/admin.routes.ts

@@ -2,6 +2,7 @@
 import { Router, NextFunction, Request, Response } from 'express';
 import passport from './passport.routes';
 import { isAdmin } from './passport.routes'; // Correctly imported
+import fs from 'node:fs/promises';
 import multer from 'multer';
 import { z } from 'zod';
 
@@ -42,6 +43,19 @@ import {
 } from '../utils/zodUtils';
 import { logger } from '../services/logger.server';
 
+/**
+ * Safely deletes a file from the filesystem, ignoring errors if the file doesn't exist.
+ * @param file The multer file object to delete.
+ */
+const cleanupUploadedFile = async (file?: Express.Multer.File) => {
+  if (!file) return;
+  try {
+    await fs.unlink(file.path);
+  } catch (err) {
+    logger.warn({ err, filePath: file.path }, 'Failed to clean up uploaded logo file.');
+  }
+};
+
 const updateCorrectionSchema = numericIdParam('id').extend({
   body: z.object({
     suggested_value: requiredString('A new suggested_value is required.'),
@@ -254,12 +268,16 @@ router.post(
       if (!req.file) {
         throw new ValidationError([], 'Logo image file is missing.');
       }
-      const logoUrl = `/assets/${req.file.filename}`;
+      // The storage path is 'flyer-images', so the URL should reflect that for consistency.
+      const logoUrl = `/flyer-images/${req.file.filename}`;
       await db.adminRepo.updateBrandLogo(params.id, logoUrl, req.log);
 
       logger.info({ brandId: params.id, logoUrl }, `Brand logo updated for brand ID: ${params.id}`);
       res.status(200).json({ message: 'Brand logo updated successfully.', logoUrl });
     } catch (error) {
+      // If an error occurs after the file has been uploaded (e.g., DB error),
+      // we must clean up the orphaned file from the disk.
+      await cleanupUploadedFile(req.file);
       logger.error({ error }, 'Error updating brand logo');
       next(error);
     }

src/routes/ai.routes.test.ts

@@ -285,6 +285,21 @@ describe('AI Routes (/api/ai)', () => {
         '123 Pacific St, Anytown, BC, V8T 1A1, CA',
       );
     });
+
+    it('should clean up the uploaded file if validation fails (e.g., missing checksum)', async () => {
+      // Spy on the unlink function to ensure it's called on error
+      const unlinkSpy = vi.spyOn(fs.promises, 'unlink').mockResolvedValue(undefined);
+
+      const response = await supertest(app)
+        .post('/api/ai/upload-and-process')
+        .attach('flyerFile', imagePath); // No checksum field, will cause validation to throw
+
+      expect(response.status).toBe(400);
+      // The validation error is now caught inside the route handler, which then calls cleanup.
+      expect(unlinkSpy).toHaveBeenCalledTimes(1);
+
+      unlinkSpy.mockRestore();
+    });
   });
 
   describe('GET /jobs/:jobId/status', () => {
@@ -559,6 +574,51 @@ describe('AI Routes (/api/ai)', () => {
     });
   });
 
+  describe('POST /flyers/process (Legacy Error Handling)', () => {
+    const imagePath = path.resolve(__dirname, '../tests/assets/test-flyer-image.jpg');
+
+    it('should handle malformed JSON in data field and return 400', async () => {
+      const malformedDataString = '{"checksum":'; // Invalid JSON
+      vi.mocked(mockedDb.flyerRepo.findFlyerByChecksum).mockResolvedValue(undefined);
+
+      const response = await supertest(app)
+        .post('/api/ai/flyers/process')
+        .field('data', malformedDataString)
+        .attach('flyerImage', imagePath);
+
+      // The outer catch block should be hit, leading to empty parsed data.
+      // The handler then fails the checksum validation.
+      expect(response.status).toBe(400);
+      expect(response.body.message).toBe('Checksum is required.');
+      // It should log the critical error during parsing.
+      expect(mockLogger.error).toHaveBeenCalledWith(
+        expect.objectContaining({ error: expect.any(Error) }),
+        '[API /ai/flyers/process] Unexpected error while parsing request body',
+      );
+    });
+
+    it('should return 400 if checksum is missing from legacy payload', async () => {
+      const payloadWithoutChecksum = {
+        originalFileName: 'flyer.jpg',
+        extractedData: { store_name: 'Test Store', items: [] },
+      };
+      // Spy on fs.promises.unlink to verify file cleanup
+      const unlinkSpy = vi.spyOn(fs.promises, 'unlink').mockResolvedValue(undefined);
+
+      const response = await supertest(app)
+        .post('/api/ai/flyers/process')
+        .field('data', JSON.stringify(payloadWithoutChecksum))
+        .attach('flyerImage', imagePath);
+
+      expect(response.status).toBe(400);
+      expect(response.body.message).toBe('Checksum is required.');
+      // Ensure the uploaded file is cleaned up
+      expect(unlinkSpy).toHaveBeenCalledTimes(1);
+
+      unlinkSpy.mockRestore();
+    });
+  });
+
   describe('POST /check-flyer', () => {
     const imagePath = path.resolve(__dirname, '../tests/assets/test-flyer-image.jpg');
     it('should return 400 if no image is provided', async () => {
@@ -828,6 +888,39 @@ describe('AI Routes (/api/ai)', () => {
       expect(response.body.message).toBe('Maps API key invalid');
     });
 
+    it('POST /deep-dive should return 500 on a generic error', async () => {
+      vi.mocked(mockLogger.info).mockImplementationOnce(() => {
+        throw new Error('Deep dive logging failed');
+      });
+      const response = await supertest(app)
+        .post('/api/ai/deep-dive')
+        .send({ items: [{ name: 'test' }] });
+      expect(response.status).toBe(500);
+      expect(response.body.message).toBe('Deep dive logging failed');
+    });
+
+    it('POST /search-web should return 500 on a generic error', async () => {
+      vi.mocked(mockLogger.info).mockImplementationOnce(() => {
+        throw new Error('Search web logging failed');
+      });
+      const response = await supertest(app)
+        .post('/api/ai/search-web')
+        .send({ query: 'test query' });
+      expect(response.status).toBe(500);
+      expect(response.body.message).toBe('Search web logging failed');
+    });
+
+    it('POST /compare-prices should return 500 on a generic error', async () => {
+      vi.mocked(mockLogger.info).mockImplementationOnce(() => {
+        throw new Error('Compare prices logging failed');
+      });
+      const response = await supertest(app)
+        .post('/api/ai/compare-prices')
+        .send({ items: [{ name: 'Milk' }] });
+      expect(response.status).toBe(500);
+      expect(response.body.message).toBe('Compare prices logging failed');
+    });
+
     it('POST /quick-insights should return 400 if items are missing', async () => {
       const response = await supertest(app).post('/api/ai/quick-insights').send({});
       expect(response.status).toBe(400);

src/routes/ai.routes.ts

@@ -59,6 +59,13 @@ const cleanupUploadedFile = async (file?: Express.Multer.File) => {
   }
 };
 
+const cleanupUploadedFiles = async (files?: Express.Multer.File[]) => {
+  if (!files || !Array.isArray(files)) return;
+  // Use Promise.all to run cleanups in parallel for efficiency,
+  // as cleanupUploadedFile is designed to not throw errors.
+  await Promise.all(files.map((file) => cleanupUploadedFile(file)));
+};
+
 const cropAreaObjectSchema = z.object({
   x: z.number(),
   y: z.number(),
@@ -87,7 +94,6 @@ const rescanAreaSchema = z.object({
       })
       .pipe(cropAreaObjectSchema), // Further validate the structure of the parsed object
     extractionType: z.enum(['store_name', 'dates', 'item_details'], {
-      // This is the line with the error
       message: "extractionType must be one of 'store_name', 'dates', or 'item_details'.",
     }),
   }),
@@ -207,15 +213,19 @@ router.post(
   '/upload-and-process',
   optionalAuth,
   uploadToDisk.single('flyerFile'),
-  validateRequest(uploadAndProcessSchema),
+  // Validation is now handled inside the route to ensure file cleanup on failure.
+  // validateRequest(uploadAndProcessSchema),
   async (req, res, next: NextFunction) => {
     try {
+      // Manually validate the request body. This will throw if validation fails.
+      uploadAndProcessSchema.parse({ body: req.body });
+
       if (!req.file) {
         return res.status(400).json({ message: 'A flyer file (PDF or image) is required.' });
       }
 
       logger.debug(
-        { filename: req.file.originalname, size: req.file.size, checksum: req.body.checksum },
+        { filename: req.file.originalname, size: req.file.size, checksum: req.body?.checksum },
         'Handling /upload-and-process',
       );
 
@@ -267,6 +277,9 @@ router.post(
         jobId: job.id,
       });
     } catch (error) {
+      // If any error occurs (including validation), ensure the uploaded file is cleaned up.
+      await cleanupUploadedFile(req.file);
+      // Pass the error to the global error handler.
       next(error);
     }
   },
@@ -516,6 +529,8 @@ router.post(
       res.status(200).json({ is_flyer: true }); // Stubbed response
     } catch (error) {
       next(error);
+    } finally {
+      await cleanupUploadedFile(req.file);
     }
   },
 );
@@ -533,6 +548,8 @@ router.post(
       res.status(200).json({ address: 'not identified' }); // Updated stubbed response
     } catch (error) {
       next(error);
+    } finally {
+      await cleanupUploadedFile(req.file);
     }
   },
 );
@@ -550,6 +567,8 @@ router.post(
       res.status(200).json({ store_logo_base_64: null }); // Stubbed response
     } catch (error) {
       next(error);
+    } finally {
+      await cleanupUploadedFiles(req.files as Express.Multer.File[]);
     }
   },
 );
@@ -697,6 +716,8 @@ router.post(
       res.status(200).json(result);
     } catch (error) {
       next(error);
+    } finally {
+      await cleanupUploadedFile(req.file);
     }
   },
 );

src/routes/user.routes.test.ts

@@ -3,6 +3,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest';
 import supertest from 'supertest';
 import express from 'express';
 import * as bcrypt from 'bcrypt';
+import fs from 'node:fs/promises';
 import {
   createMockUserProfile,
   createMockMasterGroceryItem,
@@ -1135,6 +1136,27 @@ describe('User Routes (/api/users)', () => {
         expect(response.body.message).toBe('No avatar file uploaded.');
       });
 
+      it('should clean up the uploaded file if updating the profile fails', async () => {
+        // Spy on the unlink function to ensure it's called on error
+        const unlinkSpy = vi.spyOn(fs, 'unlink').mockResolvedValue(undefined);
+
+        const dbError = new Error('DB Connection Failed');
+        vi.mocked(db.userRepo.updateUserProfile).mockRejectedValue(dbError);
+        const dummyImagePath = 'test-avatar.png';
+
+        const response = await supertest(app)
+          .post('/api/users/profile/avatar')
+          .attach('avatar', Buffer.from('dummy-image-content'), dummyImagePath);
+
+        expect(response.status).toBe(500);
+        // Verify that the cleanup function was called
+        expect(unlinkSpy).toHaveBeenCalledTimes(1);
+        // The filename is predictable because of the multer config in user.routes.ts
+        expect(unlinkSpy).toHaveBeenCalledWith(expect.stringContaining('test-avatar.png'));
+
+        unlinkSpy.mockRestore();
+      });
+
       it('should return 400 for a non-numeric address ID', async () => {
         const response = await supertest(app).get('/api/users/addresses/abc');
         expect(response.status).toBe(400);

src/routes/user.routes.ts

@@ -20,6 +20,19 @@ import {
 } from '../utils/zodUtils';
 import * as db from '../services/db/index.db';
 
+/**
+ * Safely deletes a file from the filesystem, ignoring errors if the file doesn't exist.
+ * @param file The multer file object to delete.
+ */
+const cleanupUploadedFile = async (file?: Express.Multer.File) => {
+  if (!file) return;
+  try {
+    await fs.unlink(file.path);
+  } catch (err) {
+    logger.warn({ err, filePath: file.path }, 'Failed to clean up uploaded avatar file.');
+  }
+};
+
 const router = express.Router();
 
 const updateProfileSchema = z.object({
@@ -110,8 +123,8 @@ router.post(
   '/profile/avatar',
   avatarUpload.single('avatar'),
   async (req: Request, res: Response, next: NextFunction) => {
+    // The try-catch block was already correct here.
     try {
-      // The try-catch block was already correct here.
       if (!req.file) return res.status(400).json({ message: 'No avatar file uploaded.' });
       const userProfile = req.user as UserProfile;
       const avatarUrl = `/uploads/avatars/${req.file.filename}`;
@@ -122,6 +135,9 @@ router.post(
       );
       res.json(updatedProfile);
     } catch (error) {
+      // If an error occurs after the file has been uploaded (e.g., DB error),
+      // we must clean up the orphaned file from the disk.
+      await cleanupUploadedFile(req.file);
       logger.error({ error }, 'Error uploading avatar');
       next(error);
     }