better errors for routes

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

@@ -16,6 +16,7 @@ import express, { Request, Response, NextFunction } from 'express';
 import adminRouter from './admin.routes';
 import { createMockUserProfile, createMockSuggestedCorrection, createMockBrand, createMockRecipe, createMockRecipeComment, createMockFlyerItem } from '../tests/utils/mockFactories';
 import { SuggestedCorrection, Brand, UserProfile, UnmatchedFlyerItem } from '../types';
+import { errorHandler } from '../middleware/errorHandler';
 
 vi.mock('../lib/queue', () => ({
   serverAdapter: {
@@ -114,9 +115,7 @@ const createApp = (user?: UserProfile) => {
     });
   }
   app.use('/api/admin', adminRouter);
-  app.use((err: Error, req: Request, res: Response, next: NextFunction) => {
-    res.status(500).json({ message: err.message || 'Internal Server Error' });
-  });
+  app.use(errorHandler);
   return app;
 };
 

src/routes/admin.jobs.routes.test.ts

@@ -15,6 +15,7 @@ import adminRouter from './admin.routes';
 import { createMockUserProfile } from '../tests/utils/mockFactories';
 import { Job } from 'bullmq';
 import { UserProfile } from '../types';
+import { errorHandler } from '../middleware/errorHandler';
 
 vi.mock('../lib/queue', () => ({
   serverAdapter: {
@@ -102,9 +103,7 @@ const createApp = (user?: UserProfile) => {
     });
   }
   app.use('/api/admin', adminRouter);
-  app.use((err: Error, req: Request, res: Response, next: NextFunction) => {
-    res.status(500).json({ message: err.message || 'Internal Server Error' });
-  });
+  app.use(errorHandler);
   return app;
 };
 

src/routes/admin.monitoring.routes.test.ts

@@ -17,6 +17,7 @@ import express, { Request, Response, NextFunction } from 'express';
 import adminRouter from './admin.routes';
 import { createMockUserProfile, createMockActivityLogItem } from '../tests/utils/mockFactories';
 import { UserProfile } from '../types';
+import { errorHandler } from '../middleware/errorHandler';
 
 vi.mock('../lib/queue', () => ({
   serverAdapter: {
@@ -111,9 +112,7 @@ const createApp = (user?: UserProfile) => {
     });
   }
   app.use('/api/admin', adminRouter);
-  app.use((err: Error, req: Request, res: Response, _next: NextFunction) => {
-    res.status(500).json({ message: err.message || 'Internal Server Error' });
-  });
+  app.use(errorHandler);
   return app;
 };
 

src/routes/admin.stats.routes.test.ts

@@ -12,6 +12,7 @@ import express, { Request, Response, NextFunction } from 'express';
 import adminRouter from './admin.routes';
 import { createMockUserProfile } from '../tests/utils/mockFactories';
 import { UserProfile } from '../types';
+import { errorHandler } from '../middleware/errorHandler';
 
 const { mockedDb } = vi.hoisted(() => {
   return {
@@ -80,9 +81,7 @@ const createApp = (user?: UserProfile) => {
     });
   }
   app.use('/api/admin', adminRouter);
-  app.use((err: Error, req: Request, res: Response) => {
-    res.status(500).json({ message: err.message || 'Internal Server Error' });
-  });
+  app.use(errorHandler);
   return app;
 };
 

src/routes/admin.system.routes.test.ts

@@ -5,6 +5,7 @@ import express, { Request, Response, NextFunction } from 'express';
 import adminRouter from './admin.routes';
 import { createMockUserProfile } from '../tests/utils/mockFactories';
 import { UserProfile } from '../types';
+import { errorHandler } from '../middleware/errorHandler';
 
 // Mock dependencies
 vi.mock('../services/geocodingService.server', () => ({
@@ -52,9 +53,7 @@ const createApp = () => {
   const app = express();
   app.use(express.json());
   app.use('/api/admin', adminRouter);
-  app.use((err: Error, req: Request, res: Response, next: NextFunction) => {
-    res.status(500).json({ message: err.message || 'Internal Server Error' });
-  });
+  app.use(errorHandler);
   return app;
 };
 

src/routes/admin.users.routes.test.ts

@@ -12,6 +12,7 @@ import express, { Request, Response, NextFunction } from 'express';
 import adminRouter from './admin.routes';
 import { createMockUserProfile } from '../tests/utils/mockFactories';
 import { User, UserProfile } from '../types';
+import { errorHandler } from '../middleware/errorHandler';
 
 const { mockedDb } = vi.hoisted(() => {
   return {
@@ -89,9 +90,7 @@ const createApp = (user?: UserProfile) => {
     });
   }
   app.use('/api/admin', adminRouter);
-  app.use((err: Error, req: Request, res: Response) => {
-    res.status(500).json({ message: err.message || 'Internal Server Error' });
-  });
+  app.use(errorHandler);
   return app;
 };
 

src/routes/ai.routes.test.ts

@@ -5,6 +5,7 @@ import express, { type Request, type Response, type NextFunction } from 'express
 import path from 'node:path';
 import aiRouter from './ai.routes';
 import { createMockUserProfile, createMockFlyer } from '../tests/utils/mockFactories';
+import { errorHandler } from '../middleware/errorHandler';
 import * as flyerDb from '../services/db/flyer.db';
 import * as db from '../services/db/index.db';
 import * as aiService from '../services/aiService.server';
@@ -59,13 +60,7 @@ vi.mock('./passport.routes', () => ({
 const app = express();
 app.use(express.json({ strict: false }));
 app.use('/api/ai', aiRouter);
-
-// FIX: Add a generic error handler with the correct 4-argument signature.
-// This ensures that errors passed via `next(error)` in the routes are caught
-// and formatted into a JSON response that the tests can assert against.
-app.use((err: Error, req: Request, res: Response, next: NextFunction) => {
-  res.status(500).json({ message: err.message || 'Internal Server Error' });
-});
+app.use(errorHandler);
 
 describe('AI Routes (/api/ai)', () => {
   beforeEach(() => {

src/routes/auth.routes.test.ts

@@ -12,6 +12,7 @@ import express, { Request, Response, NextFunction } from 'express';
 import cookieParser from 'cookie-parser';
 import * as bcrypt from 'bcrypt';
 import passport from 'passport';
+import { errorHandler } from '../middleware/errorHandler';
 import { UserProfile } from '../types';
 
 // --- FIX: Hoist passport mocks to be available for vi.mock ---
@@ -151,11 +152,7 @@ app.use((req: Request, res: Response, next: NextFunction) => {
 });
 
 app.use('/api/auth', authRouter);
-
-// Add error handler
-app.use((err: any, req: Request, res: Response, next: NextFunction) => {
-  res.status(err.status || 500).json({ message: err.message });
-});
+app.use(errorHandler);
 
 // --- 5. Tests ---
 describe('Auth Routes (/api/auth)', () => {

src/routes/budget.routes.test.ts

@@ -4,6 +4,7 @@ import supertest from 'supertest';
 import express, { Request, Response, NextFunction } from 'express';
 import budgetRouter from './budget.routes';
 import * as db from '../services/db/index.db';
+import { errorHandler } from '../middleware/errorHandler';
 import { createMockUserProfile, createMockBudget, createMockSpendingByCategory } from '../tests/utils/mockFactories';
 
 import { ForeignKeyConstraintError } from '../services/db/errors.db';
@@ -51,11 +52,7 @@ vi.mock('./passport.routes', () => ({
 const app = express();
 app.use(express.json());
 app.use('/api/budgets', budgetRouter);
-
-// Add a basic error handler to return JSON errors instead of Express default HTML
-app.use((err: Error, req: Request, res: Response, next: NextFunction) => {
-  res.status(500).json({ message: 'Internal Server Error' });
-});
+app.use(errorHandler);
 
 describe('Budget Routes (/api/budgets)', () => {
   const mockUserProfile = createMockUserProfile({ user_id: 'user-123', points: 100 });
@@ -84,8 +81,8 @@ describe('Budget Routes (/api/budgets)', () => {
       it('should return 500 if the database call fails', async () => {
         vi.mocked(db.budgetRepo.getBudgetsForUser).mockRejectedValue(new Error('DB Error'));
         const response = await supertest(app).get('/api/budgets');
-        expect(response.status).toBe(500);
-        expect(response.body.message).toBe('Internal Server Error');
+        expect(response.status).toBe(500); // The custom handler will now be used
+        expect(response.body.message).toBe('DB Error');
       });
     });
 
@@ -155,8 +152,8 @@ describe('Budget Routes (/api/budgets)', () => {
         const budgetUpdates = { amount_cents: 60000 };
         vi.mocked(db.budgetRepo.updateBudget).mockRejectedValue(new Error('DB Error'));
         const response = await supertest(app).put('/api/budgets/1').send(budgetUpdates);
-        expect(response.status).toBe(500);
-        expect(response.body.message).toBe('Internal Server Error');
+        expect(response.status).toBe(500); // The custom handler will now be used
+        expect(response.body.message).toBe('DB Error');
       });
     });
 
@@ -186,8 +183,8 @@ describe('Budget Routes (/api/budgets)', () => {
       it('should return 500 if a generic database error occurs', async () => {
         vi.mocked(db.budgetRepo.deleteBudget).mockRejectedValue(new Error('DB Error'));
         const response = await supertest(app).delete('/api/budgets/1');
-        expect(response.status).toBe(500);
-        expect(response.body.message).toBe('Internal Server Error');
+        expect(response.status).toBe(500); // The custom handler will now be used
+        expect(response.body.message).toBe('DB Error');
       });
     });
 

src/routes/flyer.routes.test.ts

@@ -2,6 +2,7 @@
 import { describe, it, expect, vi, beforeEach } from 'vitest';
 import supertest from 'supertest';
 import express, { Request, Response, NextFunction } from 'express';
+import { errorHandler } from '../middleware/errorHandler';
 import flyerRouter from './flyer.routes';
 import { createMockFlyer, createMockFlyerItem } from '../tests/utils/mockFactories';
 
@@ -32,11 +33,7 @@ const app = express();
 app.use(express.json());
 // Mount the router under its designated base path
 app.use('/api/flyers', flyerRouter);
-
-// Add a generic error handler to catch errors passed via next()
-app.use((err: Error, req: Request, res: Response, next: NextFunction) => {
-  res.status(500).json({ message: err.message || 'Internal Server Error' });
-});
+app.use(errorHandler);
 
 describe('Flyer Routes (/api/flyers)', () => {
   beforeEach(() => {
@@ -64,6 +61,7 @@ describe('Flyer Routes (/api/flyers)', () => {
       vi.mocked(db.flyerRepo.getFlyers).mockRejectedValue(new Error('DB Error'));
       const response = await supertest(app).get('/api/flyers');
       expect(response.status).toBe(500);
+      expect(response.body.message).toBe('DB Error');
     });
   });
 
@@ -96,6 +94,7 @@ describe('Flyer Routes (/api/flyers)', () => {
       vi.mocked(db.flyerRepo.getFlyerById).mockRejectedValue(new Error('DB Error'));
       const response = await supertest(app).get('/api/flyers/123');
       expect(response.status).toBe(500);
+      expect(response.body.message).toBe('DB Error');
     });
   });
 
@@ -120,6 +119,7 @@ describe('Flyer Routes (/api/flyers)', () => {
       vi.mocked(db.flyerRepo.getFlyerItems).mockRejectedValue(new Error('DB Error'));
       const response = await supertest(app).get('/api/flyers/123/items');
       expect(response.status).toBe(500);
+      expect(response.body.message).toBe('DB Error');
     });
   });
 
@@ -155,6 +155,7 @@ describe('Flyer Routes (/api/flyers)', () => {
       vi.mocked(db.flyerRepo.getFlyerItemsForFlyers).mockRejectedValue(new Error('DB Error'));
       const response = await supertest(app).post('/api/flyers/items/batch-fetch').send({ flyerIds: [1] });
       expect(response.status).toBe(500);
+      expect(response.body.message).toBe('DB Error');
     });
   });
 
@@ -189,6 +190,7 @@ describe('Flyer Routes (/api/flyers)', () => {
       vi.mocked(db.flyerRepo.countFlyerItemsForFlyers).mockRejectedValue(new Error('DB Error'));
       const response = await supertest(app).post('/api/flyers/items/batch-count').send({ flyerIds: [1] });
       expect(response.status).toBe(500);
+      expect(response.body.message).toBe('DB Error');
     });
   });
 

src/routes/gamification.routes.test.ts

@@ -4,6 +4,7 @@ import supertest from 'supertest';
 import express, { Request, Response, NextFunction } from 'express';
 import gamificationRouter from './gamification.routes';
 import * as db from '../services/db/index.db';
+import { errorHandler } from '../middleware/errorHandler';
 import { createMockUserProfile, createMockAchievement, createMockUserAchievement } from '../tests/utils/mockFactories';
 import { ForeignKeyConstraintError } from '../services/db/errors.db';
 
@@ -44,6 +45,7 @@ vi.mock('./passport.routes', () => ({
 const app = express();
 app.use(express.json({ strict: false }));
 app.use('/api/achievements', gamificationRouter);
+app.use(errorHandler);
 
 describe('Gamification Routes (/api/achievements)', () => {
   const mockUserProfile = createMockUserProfile({ user_id: 'user-123', points: 100 });
@@ -78,6 +80,7 @@ describe('Gamification Routes (/api/achievements)', () => {
 
       const response = await supertest(app).get('/api/achievements');
       expect(response.status).toBe(500);
+      expect(response.body.message).toBe('DB Connection Failed');
     });
 
     it('should return 400 if awarding an achievement to a non-existent user', async () => {
@@ -127,6 +130,7 @@ describe('Gamification Routes (/api/achievements)', () => {
       vi.mocked(db.gamificationRepo.getUserAchievements).mockRejectedValue(dbError);
       const response = await supertest(app).get('/api/achievements/me');
       expect(response.status).toBe(500);
+      expect(response.body.message).toBe('DB Error');
     });
   });
 
@@ -189,6 +193,7 @@ describe('Gamification Routes (/api/achievements)', () => {
 
       const response = await supertest(app).post('/api/achievements/award').send(awardPayload);
       expect(response.status).toBe(500);
+      expect(response.body.message).toBe('DB Error');
     });
 
     it('should return 400 if awarding an achievement to a non-existent user', async () => {
@@ -221,6 +226,7 @@ describe('Gamification Routes (/api/achievements)', () => {
       vi.mocked(db.gamificationRepo.getLeaderboard).mockRejectedValue(new Error('DB Error'));
       const response = await supertest(app).get('/api/achievements/leaderboard');
       expect(response.status).toBe(500);
+      expect(response.body.message).toBe('DB Error');
     });
   });
 });

src/routes/health.routes.test.ts

@@ -5,6 +5,7 @@ import express, { Request, Response, NextFunction } from 'express';
 import healthRouter from './health.routes';
 import * as dbConnection from '../services/db/connection.db';
 import { connection as redisConnection } from '../services/queueService.server';
+import { errorHandler } from '../middleware/errorHandler';
 import fs from 'node:fs/promises';
 
 // 1. Mock the dependencies of the health router.
@@ -46,10 +47,7 @@ const mockedFs = fs as Mocked<typeof fs>;
 // 2. Create a minimal Express app to host the router for testing.
 const app = express();
 app.use('/api/health', healthRouter);
-app.use((err: Error, req: Request, res: Response, next: NextFunction) => {
-  const statusCode = (err as any).status || 500;
-  res.status(statusCode).json({ message: err.message || 'Internal Server Error' });
-});
+app.use(errorHandler);
 
 describe('Health Routes (/api/health)', () => {
   beforeEach(() => {
@@ -115,6 +113,7 @@ describe('Health Routes (/api/health)', () => {
       // Assert
       expect(response.status).toBe(500);
       expect(response.body.success).toBe(false);
+      expect(response.body.success).toBe(false);
       expect(response.body.message).toBe('Failed to connect to Redis.');
       expect(response.body.error).toContain('Unexpected Redis ping response: OK');
     });
@@ -171,6 +170,7 @@ describe('Health Routes (/api/health)', () => {
       const response = await supertest(app).get('/api/health/db-schema');
 
       expect(response.status).toBe(500);
+      expect(response.body.message).toBe('An error occurred while checking the database schema.');
     });
   });
 
@@ -248,5 +248,6 @@ describe('Health Routes (/api/health)', () => {
     const response = await supertest(app).get('/api/health/db-pool');
 
     expect(response.status).toBe(500);
+    expect(response.body.message).toBe('An error occurred while checking the database pool status.');
   });
 });

src/routes/health.routes.ts

@@ -8,7 +8,7 @@ import { getSimpleWeekAndYear } from '../utils/dateUtils';
 
 const router = Router();
 
-router.get('/ping', (req: Request, res: Response) => {
+router.get('/ping', (_req: Request, res: Response) => {
     res.status(200).send('pong');
 });
 
@@ -21,19 +21,19 @@ router.get('/db-schema', async (req, res) => {
             return res.status(500).json({ success: false, message: `Database schema check failed. Missing tables: ${missingTables.join(', ')}.` });
         }
         return res.status(200).json({ success: true, message: 'All required database tables exist.' });
-    } catch (error) {
-        logger.error('Error during DB schema check:', { error });
-        return res.status(500).json({ success: false, message: 'An error occurred while checking the database schema.' });
+    } catch (error: unknown) {
+        logger.error('Error during DB schema check:', { error: error instanceof Error ? error.message : error });
+        return res.status(500).json({ success: false, message: 'An error occurred while checking the database schema.' }); // No change to next(error) as this is a health check, not a standard API route
     }
 });
 
 router.get('/storage', async (req, res) => {
     const storagePath = process.env.STORAGE_PATH || '/var/www/flyer-crawler.projectium.com/assets';
     try {
-        await fs.access(storagePath, fs.constants.W_OK);
+        await fs.access(storagePath, fs.constants.W_OK); // Use fs.promises
         return res.status(200).json({ success: true, message: `Storage directory '${storagePath}' is accessible and writable.` });
-    } catch (error) {
-        logger.error(`Storage check failed for path: ${storagePath}`, { error });
+    } catch (error: unknown) {
+        logger.error(`Storage check failed for path: ${storagePath}`, { error: error instanceof Error ? error.message : error });
         return res.status(500).json({ success: false, message: `Storage check failed. Ensure the directory '${storagePath}' exists and is writable by the application.` });
     }
 });
@@ -50,9 +50,9 @@ router.get('/db-pool', (req: Request, res: Response) => {
             logger.warn(`Database pool health check shows high waiting count: ${status.waitingCount}`);
             return res.status(500).json({ success: false, message: `Pool may be under stress. ${message}` });
         }
-    } catch (error) {
-        logger.error('Error during DB pool health check:', { error });
-        return res.status(500).json({ success: false, message: 'An error occurred while checking the database pool status.' });
+    } catch (error: unknown) {
+        logger.error('Error during DB pool health check:', { error: error instanceof Error ? error.message : error });
+        return res.status(500).json({ success: false, message: 'An error occurred while checking the database pool status.' }); // No change to next(error) as this is a health check
     }
 });
 
@@ -75,9 +75,9 @@ router.get('/redis', async (req: Request, res: Response) => {
         if (reply === 'PONG') {
             return res.status(200).json({ success: true, message: 'Redis connection is healthy.' });
         }
-        throw new Error(`Unexpected Redis ping response: ${reply}`);
-    } catch (error) {
-        const errorMessage = error instanceof Error ? error.message : 'An unknown error occurred.';
+        throw new Error(`Unexpected Redis ping response: ${reply}`); // This will be caught below
+    } catch (error: unknown) {
+        const errorMessage = error instanceof Error ? error.message : String(error);
         return res.status(500).json({ success: false, message: 'Failed to connect to Redis.', error: errorMessage });
     }
 });

src/routes/passport.routes.test.ts

@@ -62,6 +62,7 @@ vi.mock('../services/db/index.db', () => ({
 const mockedDb = db as Mocked<typeof db>;
 
 vi.mock('../services/logger.server', () => ({
+  // This mock is used by the module under test and can be imported in the test file.
   logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() },
 }));
 
@@ -87,6 +88,7 @@ vi.mock('passport', () => {
 
 // Now, import the passport configuration which will use our mocks
 import passport, { isAdmin, optionalAuth } from './passport.routes';
+import { logger } from '../services/logger.server';
 
 describe('Passport Configuration', () => {
   beforeEach(() => {
@@ -390,7 +392,6 @@ describe('Passport Configuration', () => {
       vi.mocked(passport.authenticate).mockImplementation(
         (_strategy, _options, callback) => () => callback?.(null, false, mockInfo)
       );
-      const { logger } = require('../services/logger.server');
 
       // Act
       optionalAuth(mockReq, mockRes as Response, mockNext);

src/routes/recipe.routes.test.ts

@@ -3,6 +3,7 @@ import { describe, it, expect, vi, beforeEach } from 'vitest';
 import supertest from 'supertest';
 import express, { Request, Response, NextFunction } from 'express';
 import recipeRouter from './recipe.routes';
+import { errorHandler } from '../middleware/errorHandler';
 import { createMockRecipe, createMockRecipeComment } from '../tests/utils/mockFactories';
 
 // 1. Mock the Service Layer directly.
@@ -31,11 +32,7 @@ const app = express();
 app.use(express.json());
 // Mount the router under its designated base path
 app.use('/api/recipes', recipeRouter);
-
-// Add a generic error handler to catch errors passed via next()
-app.use((err: Error, req: Request, res: Response, next: NextFunction) => {
-  res.status(500).json({ message: err.message || 'Internal Server Error' });
-});
+app.use(errorHandler);
 
 describe('Recipe Routes (/api/recipes)', () => {
   beforeEach(() => {

src/routes/stats.routes.test.ts

@@ -2,6 +2,7 @@
 import { describe, it, expect, vi, beforeEach } from 'vitest';
 import supertest from 'supertest';
 import express, { Request, Response, NextFunction } from 'express';
+import { errorHandler } from '../middleware/errorHandler';
 import statsRouter from './stats.routes';
 
 // 1. Mock the Service Layer directly.
@@ -26,11 +27,7 @@ const app = express();
 app.use(express.json());
 // Mount the router under its designated base path
 app.use('/api/stats', statsRouter);
-
-// Add a generic error handler to catch errors passed via next()
-app.use((err: Error, req: Request, res: Response, next: NextFunction) => {
-  res.status(500).json({ message: err.message || 'Internal Server Error' });
-});
+app.use(errorHandler);
 
 describe('Stats Routes (/api/stats)', () => {
   beforeEach(() => {
@@ -67,6 +64,7 @@ describe('Stats Routes (/api/stats)', () => {
       vi.mocked(db.adminRepo.getMostFrequentSaleItems).mockRejectedValue(new Error('DB Error'));
       const response = await supertest(app).get('/api/stats/most-frequent-sales');
       expect(response.status).toBe(500);
+      expect(response.body.message).toBe('DB Error');
     });
   });
 });