ci: Bump version to 0.9.23 [skip ci]

package-lock.json

@@ -1,12 +1,12 @@
 {
   "name": "flyer-crawler",
-  "version": "0.9.22",
+  "version": "0.9.23",
   "lockfileVersion": 3,
   "requires": true,
   "packages": {
     "": {
       "name": "flyer-crawler",
-      "version": "0.9.22",
+      "version": "0.9.23",
       "dependencies": {
         "@bull-board/api": "^6.14.2",
         "@bull-board/express": "^6.14.2",

package.json

@@ -1,7 +1,7 @@
 {
   "name": "flyer-crawler",
   "private": true,
-  "version": "0.9.22",
+  "version": "0.9.23",
   "type": "module",
   "scripts": {
     "dev": "concurrently \"npm:start:dev\" \"vite\"",

src/hooks/useUserProfileData.ts

@@ -0,0 +1,51 @@
+// src/hooks/useUserProfileData.ts
+import { useState, useEffect } from 'react';
+import * as apiClient from '../services/apiClient';
+import { UserProfile, Achievement, UserAchievement } from '../types';
+import { logger } from '../services/logger.client';
+
+export const useUserProfileData = () => {
+  const [profile, setProfile] = useState<UserProfile | null>(null);
+  const [achievements, setAchievements] = useState<(UserAchievement & Achievement)[]>([]);
+  const [isLoading, setIsLoading] = useState(true);
+  const [error, setError] = useState<string | null>(null);
+
+  useEffect(() => {
+    const fetchData = async () => {
+      setIsLoading(true);
+      try {
+        const [profileRes, achievementsRes] = await Promise.all([
+          apiClient.getAuthenticatedUserProfile(),
+          apiClient.getUserAchievements(),
+        ]);
+
+        if (!profileRes.ok) throw new Error('Failed to fetch user profile.');
+        if (!achievementsRes.ok) throw new Error('Failed to fetch user achievements.');
+
+        const profileData: UserProfile | null = await profileRes.json();
+        const achievementsData: (UserAchievement & Achievement)[] | null =
+          await achievementsRes.json();
+
+        logger.info(
+          { profileData, achievementsCount: achievementsData?.length },
+          'useUserProfileData: Fetched data',
+        );
+
+        if (profileData) {
+          setProfile(profileData);
+        }
+        setAchievements(achievementsData || []);
+      } catch (err) {
+        const errorMessage = err instanceof Error ? err.message : 'An unknown error occurred.';
+        setError(errorMessage);
+        logger.error({ err }, 'Error in useUserProfileData:');
+      } finally {
+        setIsLoading(false);
+      }
+    };
+
+    fetchData();
+  }, []);
+
+  return { profile, setProfile, achievements, isLoading, error };
+};

src/pages/ResetPasswordPage.test.tsx

@@ -109,6 +109,33 @@ describe('ResetPasswordPage', () => {
     );
   });
 
+  it('should show an error message if API returns a non-JSON error response', async () => {
+    // Simulate a server error returning HTML instead of JSON
+    mockedApiClient.resetPassword.mockResolvedValue(
+      new Response('<h1>Server Error</h1>', {
+        status: 500,
+        headers: { 'Content-Type': 'text/html' },
+      }),
+    );
+    renderWithRouter('test-token');
+
+    fireEvent.change(screen.getByPlaceholderText('New Password'), {
+      target: { value: 'newSecurePassword123' },
+    });
+    fireEvent.change(screen.getByPlaceholderText('Confirm New Password'), {
+      target: { value: 'newSecurePassword123' },
+    });
+    fireEvent.click(screen.getByRole('button', { name: /reset password/i }));
+
+    await waitFor(() => {
+      // The error from response.json() is implementation-dependent.
+      // We check for a substring that is likely to be present.
+      expect(screen.getByText(/not valid JSON/i)).toBeInTheDocument();
+    });
+
+    expect(logger.error).toHaveBeenCalledWith({ err: expect.any(SyntaxError) }, 'Failed to reset password.');
+  });
+
   it('should show a loading spinner while submitting', async () => {
     let resolvePromise: (value: Response) => void;
     const mockPromise = new Promise<Response>((resolve) => {

src/pages/UserProfilePage.test.tsx

@@ -123,6 +123,24 @@ describe('UserProfilePage', () => {
     });
   });
 
+  it('should handle null achievements data gracefully on fetch', async () => {
+    mockedApiClient.getAuthenticatedUserProfile.mockResolvedValue(
+      new Response(JSON.stringify(mockProfile)),
+    );
+    // Mock a successful response but with a null body for achievements
+    mockedApiClient.getUserAchievements.mockResolvedValue(new Response(JSON.stringify(null)));
+    render(<UserProfilePage />);
+
+    await waitFor(() => {
+      expect(screen.getByRole('heading', { name: 'Test User' })).toBeInTheDocument();
+      // The mock achievements list should show 0 achievements because the component
+      // should handle the null response and pass an empty array to the list.
+      expect(screen.getByTestId('achievements-list-mock')).toHaveTextContent(
+        'Achievements Count: 0',
+      );
+    });
+  });
+
   it('should render the profile and achievements on successful fetch', async () => {
     mockedApiClient.getAuthenticatedUserProfile.mockResolvedValue(
       new Response(JSON.stringify(mockProfile)),
@@ -294,6 +312,24 @@ describe('UserProfilePage', () => {
       });
     });
 
+    it('should handle non-ok response with null body when saving name', async () => {
+      // This tests the case where the server returns an error status but an empty/null body.
+      mockedApiClient.updateUserProfile.mockResolvedValue(new Response(null, { status: 500 }));
+      render(<UserProfilePage />);
+      await screen.findByText('Test User');
+
+      fireEvent.click(screen.getByRole('button', { name: /edit/i }));
+      fireEvent.change(screen.getByRole('textbox'), { target: { value: 'New Name' } });
+      fireEvent.click(screen.getByRole('button', { name: /save/i }));
+
+      await waitFor(() => {
+        // The component should fall back to the default error message.
+        expect(mockedNotificationService.notifyError).toHaveBeenCalledWith(
+          'Failed to update name.',
+        );
+      });
+    });
+
     it('should handle unknown errors when saving name', async () => {
       mockedApiClient.updateUserProfile.mockRejectedValue('Unknown update error');
       render(<UserProfilePage />);
@@ -420,6 +456,22 @@ describe('UserProfilePage', () => {
       });
     });
 
+    it('should handle non-ok response with null body when uploading avatar', async () => {
+      mockedApiClient.uploadAvatar.mockResolvedValue(new Response(null, { status: 500 }));
+      render(<UserProfilePage />);
+      await screen.findByAltText('User Avatar');
+
+      const fileInput = screen.getByTestId('avatar-file-input');
+      const file = new File(['(⌐□_□)'], 'chucknorris.png', { type: 'image/png' });
+      fireEvent.change(fileInput, { target: { files: [file] } });
+
+      await waitFor(() => {
+        expect(mockedNotificationService.notifyError).toHaveBeenCalledWith(
+          'Failed to upload avatar.',
+        );
+      });
+    });
+
     it('should handle unknown errors when uploading avatar', async () => {
       mockedApiClient.uploadAvatar.mockRejectedValue('Unknown upload error');
       render(<UserProfilePage />);

src/pages/UserProfilePage.tsx

@@ -1,15 +1,13 @@
 import React, { useState, useEffect, useRef } from 'react';
 import * as apiClient from '../services/apiClient';
-import { UserProfile, Achievement, UserAchievement } from '../types';
+import type { UserProfile } from '../types';
 import { logger } from '../services/logger.client';
 import { notifySuccess, notifyError } from '../services/notificationService';
 import { AchievementsList } from '../components/AchievementsList';
+import { useUserProfileData } from '../hooks/useUserProfileData';
 
 const UserProfilePage: React.FC = () => {
-  const [profile, setProfile] = useState<UserProfile | null>(null);
-  const [achievements, setAchievements] = useState<(UserAchievement & Achievement)[]>([]);
-  const [isLoading, setIsLoading] = useState(true);
-  const [error, setError] = useState<string | null>(null);
+  const { profile, setProfile, achievements, isLoading, error } = useUserProfileData();
   const [isEditingName, setIsEditingName] = useState(false);
   const [editingName, setEditingName] = useState('');
   const [isUploading, setIsUploading] = useState(false);
@@ -17,43 +15,10 @@ const UserProfilePage: React.FC = () => {
   const fileInputRef = useRef<HTMLInputElement>(null);
 
   useEffect(() => {
-    const fetchData = async () => {
-      setIsLoading(true);
-      try {
-        // Fetch profile and achievements data in parallel
-        const [profileRes, achievementsRes] = await Promise.all([
-          apiClient.getAuthenticatedUserProfile(),
-          apiClient.getUserAchievements(),
-        ]);
-
-        if (!profileRes.ok) throw new Error('Failed to fetch user profile.');
-        if (!achievementsRes.ok) throw new Error('Failed to fetch user achievements.');
-
-        const profileData: UserProfile = await profileRes.json();
-        const achievementsData: (UserAchievement & Achievement)[] = await achievementsRes.json();
-
-        logger.info(
-          { profileData, achievementsCount: achievementsData?.length },
-          'UserProfilePage: Fetched data',
-        );
-
-        setProfile(profileData);
-
-        if (profileData) {
-          setEditingName(profileData.full_name || '');
-        }
-        setAchievements(achievementsData);
-      } catch (err) {
-        const errorMessage = err instanceof Error ? err.message : 'An unknown error occurred.';
-        setError(errorMessage);
-        logger.error({ err }, 'Error fetching user profile data:');
-      } finally {
-        setIsLoading(false);
-      }
-    };
-
-    fetchData();
-  }, []); // Empty dependency array means this runs once on component mount
+    if (profile) {
+      setEditingName(profile.full_name || '');
+    }
+  }, [profile]);
 
   const handleSaveName = async () => {
     if (!profile) return;
@@ -61,8 +26,8 @@ const UserProfilePage: React.FC = () => {
     try {
       const response = await apiClient.updateUserProfile({ full_name: editingName });
       if (!response.ok) {
-        const errorData = await response.json();
-        throw new Error(errorData.message || 'Failed to update name.');
+        const errorData = await response.json().catch(() => null); // Gracefully handle non-JSON responses
+        throw new Error(errorData?.message || 'Failed to update name.');
       }
       const updatedProfile = await response.json();
       setProfile((prevProfile) => (prevProfile ? { ...prevProfile, ...updatedProfile } : null));
@@ -88,8 +53,8 @@ const UserProfilePage: React.FC = () => {
     try {
       const response = await apiClient.uploadAvatar(file);
       if (!response.ok) {
-        const errorData = await response.json();
-        throw new Error(errorData.message || 'Failed to upload avatar.');
+        const errorData = await response.json().catch(() => null); // Gracefully handle non-JSON responses
+        throw new Error(errorData?.message || 'Failed to upload avatar.');
       }
       const updatedProfile = await response.json();
       setProfile((prevProfile) => (prevProfile ? { ...prevProfile, ...updatedProfile } : null));

src/pages/admin/components/ProfileManager.test.tsx

@@ -264,6 +264,7 @@ describe('ProfileManager', () => {
     });
 
     it('should show an error if trying to save profile when not logged in', async () => {
+      const loggerSpy = vi.spyOn(logger.logger, 'warn');
       // This is an edge case, but good to test the safeguard
       render(<ProfileManager {...defaultAuthenticatedProps} userProfile={null} />);
       fireEvent.change(screen.getByLabelText(/full name/i), { target: { value: 'Updated Name' } });
@@ -271,6 +272,7 @@ describe('ProfileManager', () => {
 
       await waitFor(() => {
         expect(notifyError).toHaveBeenCalledWith('Cannot save profile, no user is logged in.');
+        expect(loggerSpy).toHaveBeenCalledWith('[handleProfileSave] Aborted: No user is logged in.');
       });
       expect(mockedApiClient.updateUserProfile).not.toHaveBeenCalled();
     });
@@ -496,6 +498,23 @@ describe('ProfileManager', () => {
       });
     });
 
+    it('should show an error when trying to link a GitHub account', async () => {
+      render(<ProfileManager {...defaultAuthenticatedProps} />);
+      fireEvent.click(screen.getByRole('button', { name: /security/i }));
+
+      await waitFor(() => {
+        expect(screen.getByRole('button', { name: /link github account/i })).toBeInTheDocument();
+      });
+
+      fireEvent.click(screen.getByRole('button', { name: /link github account/i }));
+
+      await waitFor(() => {
+        expect(notifyError).toHaveBeenCalledWith(
+          'Account linking with github is not yet implemented.',
+        );
+      });
+    });
+
     it('should switch between all tabs correctly', async () => {
       render(<ProfileManager {...defaultAuthenticatedProps} />);
 
@@ -804,6 +823,63 @@ describe('ProfileManager', () => {
       });
     });
 
+    it('should allow changing unit system when preferences are initially null', async () => {
+      const profileWithoutPrefs = { ...authenticatedProfile, preferences: null as any };
+      const { rerender } = render(
+        <ProfileManager {...defaultAuthenticatedProps} userProfile={profileWithoutPrefs} />,
+      );
+
+      fireEvent.click(screen.getByRole('button', { name: /preferences/i }));
+
+      const imperialRadio = await screen.findByLabelText(/imperial/i);
+      const metricRadio = screen.getByLabelText(/metric/i);
+
+      // With null preferences, neither should be checked.
+      expect(imperialRadio).not.toBeChecked();
+      expect(metricRadio).not.toBeChecked();
+
+      // Mock the API response for the update
+      const updatedProfileWithPrefs = {
+        ...profileWithoutPrefs,
+        preferences: { darkMode: false, unitSystem: 'metric' as const },
+      };
+      mockedApiClient.updateUserPreferences.mockResolvedValue({
+        ok: true,
+        json: () => Promise.resolve(updatedProfileWithPrefs),
+      } as Response);
+
+      fireEvent.click(metricRadio);
+
+      await waitFor(() => {
+        expect(mockedApiClient.updateUserPreferences).toHaveBeenCalledWith(
+          { unitSystem: 'metric' },
+          expect.anything(),
+        );
+        expect(mockOnProfileUpdate).toHaveBeenCalledWith(updatedProfileWithPrefs);
+      });
+
+      // Rerender with the new profile to check the UI update
+      rerender(
+        <ProfileManager {...defaultAuthenticatedProps} userProfile={updatedProfileWithPrefs} />,
+      );
+
+      fireEvent.click(screen.getByRole('button', { name: /preferences/i }));
+      expect(await screen.findByLabelText(/metric/i)).toBeChecked();
+      expect(screen.getByLabelText(/imperial/i)).not.toBeChecked();
+    });
+
+    it('should not call onProfileUpdate if updating unit system fails', async () => {
+      mockedApiClient.updateUserPreferences.mockRejectedValue(new Error('API failed'));
+      render(<ProfileManager {...defaultAuthenticatedProps} />);
+      fireEvent.click(screen.getByRole('button', { name: /preferences/i }));
+      const metricRadio = await screen.findByLabelText(/metric/i);
+      fireEvent.click(metricRadio);
+      await waitFor(() => {
+        expect(notifyError).toHaveBeenCalledWith('API failed');
+      });
+      expect(mockOnProfileUpdate).not.toHaveBeenCalled();
+    });
+
     it('should only call updateProfile when only profile data has changed', async () => {
       render(<ProfileManager {...defaultAuthenticatedProps} />);
       await waitFor(() =>
@@ -1004,5 +1080,19 @@ describe('ProfileManager', () => {
         expect(notifyError).toHaveBeenCalledWith('Permission denied');
       });
     });
+
+    it('should not trigger OAuth link if user profile is missing', async () => {
+      // This is an edge case to test the guard clause in handleOAuthLink
+      render(<ProfileManager {...defaultAuthenticatedProps} userProfile={null} />);
+      fireEvent.click(screen.getByRole('button', { name: /security/i }));
+
+      const linkButton = await screen.findByRole('button', { name: /link google account/i });
+      fireEvent.click(linkButton);
+
+      // The function should just return, so nothing should happen.
+      await waitFor(() => {
+        expect(notifyError).not.toHaveBeenCalled();
+      });
+    });
   });
 });

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

@@ -250,6 +250,17 @@ describe('Admin Content Management Routes (/api/admin)', () => {
       expect(response.status).toBe(404);
       expect(response.body.message).toBe('Correction with ID 999 not found');
     });
+
+    it('PUT /corrections/:id should return 500 on a generic DB error', async () => {
+      vi.mocked(mockedDb.adminRepo.updateSuggestedCorrection).mockRejectedValue(
+        new Error('Generic DB Error'),
+      );
+      const response = await supertest(app)
+        .put('/api/admin/corrections/101')
+        .send({ suggested_value: 'new value' });
+      expect(response.status).toBe(500);
+      expect(response.body.message).toBe('Generic DB Error');
+    });
   });
 
   describe('Flyer Review Routes', () => {
@@ -294,6 +305,13 @@ describe('Admin Content Management Routes (/api/admin)', () => {
       expect(response.body).toEqual(mockBrands);
     });
 
+    it('GET /brands should return 500 on DB error', async () => {
+      vi.mocked(mockedDb.flyerRepo.getAllBrands).mockRejectedValue(new Error('DB Error'));
+      const response = await supertest(app).get('/api/admin/brands');
+      expect(response.status).toBe(500);
+      expect(response.body.message).toBe('DB Error');
+    });
+
     it('POST /brands/:id/logo should upload a logo and update the brand', async () => {
       const brandId = 55;
       vi.mocked(mockedDb.adminRepo.updateBrandLogo).mockResolvedValue(undefined);
@@ -500,6 +518,16 @@ describe('Admin Content Management Routes (/api/admin)', () => {
       expect(response.body.message).toBe('Flyer with ID 999 not found.');
     });
 
+    it('DELETE /flyers/:flyerId should return 500 on a generic DB error', async () => {
+      const flyerId = 42;
+      vi.mocked(mockedDb.flyerRepo.deleteFlyer).mockRejectedValue(
+        new Error('Generic DB Error'),
+      );
+      const response = await supertest(app).delete(`/api/admin/flyers/${flyerId}`);
+      expect(response.status).toBe(500);
+      expect(response.body.message).toBe('Generic DB Error');
+    });
+
     it('DELETE /flyers/:flyerId should return 400 for an invalid flyerId', async () => {
       const response = await supertest(app).delete('/api/admin/flyers/abc');
       expect(response.status).toBe(400);

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

@@ -54,6 +54,14 @@ vi.mock('../services/workers.server', () => ({
   weeklyAnalyticsWorker: { name: 'weekly-analytics-reporting', isRunning: vi.fn() },
 }));
 
+// Mock the monitoring service directly to test route error handling
+vi.mock('../services/monitoringService.server', () => ({
+  monitoringService: {
+    getWorkerStatuses: vi.fn(),
+    getQueueStatuses: vi.fn(),
+  },
+}));
+
 // Mock other dependencies that are part of the adminRouter setup but not directly tested here
 vi.mock('../services/db/flyer.db');
 vi.mock('../services/db/recipe.db');
@@ -78,11 +86,8 @@ vi.mock('@bull-board/express', () => ({
 import adminRouter from './admin.routes';
 
 // Import the mocked modules to control them
-import * as queueService from '../services/queueService.server';
-import * as workerService from '../services/workers.server';
+import { monitoringService } from '../services/monitoringService.server';
 import { adminRepo } from '../services/db/index.db';
-const mockedQueueService = queueService as Mocked<typeof queueService>;
-const mockedWorkerService = workerService as Mocked<typeof workerService>;
 
 // Mock the logger
 vi.mock('../services/logger.server', () => ({
@@ -146,16 +151,26 @@ describe('Admin Monitoring Routes (/api/admin)', () => {
       expect(response.body.errors).toBeDefined();
       expect(response.body.errors.length).toBe(2); // Both limit and offset are invalid
     });
+
+    it('should return 500 if fetching activity log fails', async () => {
+      vi.mocked(adminRepo.getActivityLog).mockRejectedValue(new Error('DB Error'));
+      const response = await supertest(app).get('/api/admin/activity-log');
+      expect(response.status).toBe(500);
+      expect(response.body.message).toBe('DB Error');
+    });
   });
 
   describe('GET /workers/status', () => {
     it('should return the status of all registered workers', async () => {
       // Arrange: Set the mock status for each worker
-      vi.mocked(mockedWorkerService.flyerWorker.isRunning).mockReturnValue(true);
-      vi.mocked(mockedWorkerService.emailWorker.isRunning).mockReturnValue(true);
-      vi.mocked(mockedWorkerService.analyticsWorker.isRunning).mockReturnValue(false); // Simulate one worker being stopped
-      vi.mocked(mockedWorkerService.cleanupWorker.isRunning).mockReturnValue(true);
-      vi.mocked(mockedWorkerService.weeklyAnalyticsWorker.isRunning).mockReturnValue(true);
+      const mockStatuses = [
+        { name: 'flyer-processing', isRunning: true },
+        { name: 'email-sending', isRunning: true },
+        { name: 'analytics-reporting', isRunning: false },
+        { name: 'file-cleanup', isRunning: true },
+        { name: 'weekly-analytics-reporting', isRunning: true },
+      ];
+      vi.mocked(monitoringService.getWorkerStatuses).mockResolvedValue(mockStatuses);
 
       // Act
       const response = await supertest(app).get('/api/admin/workers/status');
@@ -170,51 +185,41 @@ describe('Admin Monitoring Routes (/api/admin)', () => {
         { name: 'weekly-analytics-reporting', isRunning: true },
       ]);
     });
+
+    it('should return 500 if fetching worker statuses fails', async () => {
+      vi.mocked(monitoringService.getWorkerStatuses).mockRejectedValue(new Error('Worker Error'));
+      const response = await supertest(app).get('/api/admin/workers/status');
+      expect(response.status).toBe(500);
+      expect(response.body.message).toBe('Worker Error');
+    });
   });
 
   describe('GET /queues/status', () => {
     it('should return job counts for all registered queues', async () => {
       // Arrange: Set the mock job counts for each queue
-      vi.mocked(mockedQueueService.flyerQueue.getJobCounts).mockResolvedValue({
-        waiting: 5,
-        active: 1,
-        completed: 100,
-        failed: 2,
-        delayed: 0,
-        paused: 0,
-      });
-      vi.mocked(mockedQueueService.emailQueue.getJobCounts).mockResolvedValue({
-        waiting: 0,
-        active: 0,
-        completed: 50,
-        failed: 0,
-        delayed: 0,
-        paused: 0,
-      });
-      vi.mocked(mockedQueueService.analyticsQueue.getJobCounts).mockResolvedValue({
-        waiting: 0,
-        active: 1,
-        completed: 10,
-        failed: 1,
-        delayed: 0,
-        paused: 0,
-      });
-      vi.mocked(mockedQueueService.cleanupQueue.getJobCounts).mockResolvedValue({
-        waiting: 2,
-        active: 0,
-        completed: 25,
-        failed: 0,
-        delayed: 0,
-        paused: 0,
-      });
-      vi.mocked(mockedQueueService.weeklyAnalyticsQueue.getJobCounts).mockResolvedValue({
-        waiting: 1,
-        active: 0,
-        completed: 5,
-        failed: 0,
-        delayed: 0,
-        paused: 0,
-      });
+      const mockStatuses = [
+        {
+          name: 'flyer-processing',
+          counts: { waiting: 5, active: 1, completed: 100, failed: 2, delayed: 0, paused: 0 },
+        },
+        {
+          name: 'email-sending',
+          counts: { waiting: 0, active: 0, completed: 50, failed: 0, delayed: 0, paused: 0 },
+        },
+        {
+          name: 'analytics-reporting',
+          counts: { waiting: 0, active: 1, completed: 10, failed: 1, delayed: 0, paused: 0 },
+        },
+        {
+          name: 'file-cleanup',
+          counts: { waiting: 2, active: 0, completed: 25, failed: 0, delayed: 0, paused: 0 },
+        },
+        {
+          name: 'weekly-analytics-reporting',
+          counts: { waiting: 1, active: 0, completed: 5, failed: 0, delayed: 0, paused: 0 },
+        },
+      ];
+      vi.mocked(monitoringService.getQueueStatuses).mockResolvedValue(mockStatuses);
 
       // Act
       const response = await supertest(app).get('/api/admin/queues/status');
@@ -246,7 +251,7 @@ describe('Admin Monitoring Routes (/api/admin)', () => {
     });
 
     it('should return 500 if fetching queue counts fails', async () => {
-      vi.mocked(mockedQueueService.flyerQueue.getJobCounts).mockRejectedValue(
+      vi.mocked(monitoringService.getQueueStatuses).mockRejectedValue(
         new Error('Redis is down'),
       );
 

src/routes/ai.routes.test.ts

@@ -318,6 +318,76 @@ describe('AI Routes (/api/ai)', () => {
     // because URL parameters cannot easily simulate empty strings for min(1) validation checks via supertest routing.
   });
 
+  describe('POST /upload-legacy', () => {
+    const imagePath = path.resolve(__dirname, '../tests/assets/test-flyer-image.jpg');
+    const mockUser = createMockUserProfile({
+      user: { user_id: 'legacy-user-1', email: 'legacy-user@test.com' },
+    });
+    // This route requires authentication, so we create an app instance with a user.
+    const authenticatedApp = createTestApp({
+      router: aiRouter,
+      basePath: '/api/ai',
+      authenticatedUser: mockUser,
+    });
+
+    it('should process a legacy flyer and return 200 on success', async () => {
+      // Arrange
+      const mockFlyer = createMockFlyer({ flyer_id: 10 });
+      vi.mocked(aiService.aiService.processLegacyFlyerUpload).mockResolvedValue(mockFlyer);
+
+      // Act
+      const response = await supertest(authenticatedApp)
+        .post('/api/ai/upload-legacy')
+        .field('some_legacy_field', 'value') // simulate some body data
+        .attach('flyerFile', imagePath);
+
+      // Assert
+      expect(response.status).toBe(200);
+      expect(response.body).toEqual(mockFlyer);
+      expect(aiService.aiService.processLegacyFlyerUpload).toHaveBeenCalledWith(
+        expect.any(Object), // req.file
+        expect.any(Object), // req.body
+        mockUser,
+        expect.any(Object), // req.log
+      );
+    });
+
+    it('should return 400 if no flyer file is uploaded', async () => {
+      const response = await supertest(authenticatedApp)
+        .post('/api/ai/upload-legacy')
+        .field('some_legacy_field', 'value');
+
+      expect(response.status).toBe(400);
+      expect(response.body.message).toBe('No flyer file uploaded.');
+    });
+
+    it('should return 409 and cleanup file if a duplicate flyer is detected', async () => {
+      const duplicateError = new aiService.DuplicateFlyerError('Duplicate legacy flyer.', 101);
+      vi.mocked(aiService.aiService.processLegacyFlyerUpload).mockRejectedValue(duplicateError);
+      const unlinkSpy = vi.spyOn(fs.promises, 'unlink').mockResolvedValue(undefined);
+
+      const response = await supertest(authenticatedApp).post('/api/ai/upload-legacy').attach('flyerFile', imagePath);
+
+      expect(response.status).toBe(409);
+      expect(response.body.message).toBe('Duplicate legacy flyer.');
+      expect(response.body.flyerId).toBe(101);
+      expect(unlinkSpy).toHaveBeenCalledTimes(1);
+      unlinkSpy.mockRestore();
+    });
+
+    it('should return 500 and cleanup file on a generic service error', async () => {
+      vi.mocked(aiService.aiService.processLegacyFlyerUpload).mockRejectedValue(new Error('Internal service failure'));
+      const unlinkSpy = vi.spyOn(fs.promises, 'unlink').mockResolvedValue(undefined);
+
+      const response = await supertest(authenticatedApp).post('/api/ai/upload-legacy').attach('flyerFile', imagePath);
+
+      expect(response.status).toBe(500);
+      expect(response.body.message).toBe('Internal service failure');
+      expect(unlinkSpy).toHaveBeenCalledTimes(1);
+      unlinkSpy.mockRestore();
+    });
+  });
+
   describe('POST /flyers/process (Legacy)', () => {
     const imagePath = path.resolve(__dirname, '../tests/assets/test-flyer-image.jpg');
     const mockDataPayload = {

src/routes/auth.routes.test.ts

@@ -550,12 +550,15 @@ describe('Auth Routes (/api/auth)', () => {
       expect(setCookieHeader[0]).toContain('Max-Age=0');
     });
 
-    it('should still return 200 OK even if deleting the refresh token from DB fails', async () => {
+    it('should still return 200 OK and log an error if deleting the refresh token from DB fails', async () => {
       // Arrange
       const dbError = new Error('DB connection lost');
       mockedAuthService.logout.mockRejectedValue(dbError);
       const { logger } = await import('../services/logger.server');
 
+      // Spy on logger.error to ensure it's called
+      const errorSpy = vi.spyOn(logger, 'error');
+
       // Act
       const response = await supertest(app)
         .post('/api/auth/logout')
@@ -563,7 +566,12 @@ describe('Auth Routes (/api/auth)', () => {
 
       // Assert
       expect(response.status).toBe(200);
-      expect(logger.error).toHaveBeenCalledWith(
+
+      // Because authService.logout is fire-and-forget (not awaited), we need to
+      // give the event loop a moment to process the rejected promise and trigger the .catch() block.
+      await new Promise((resolve) => setImmediate(resolve));
+
+      expect(errorSpy).toHaveBeenCalledWith(
         expect.objectContaining({ error: dbError }),
         'Logout token invalidation failed in background.',
       );

src/routes/flyer.routes.test.ts

@@ -50,6 +50,8 @@ describe('Flyer Routes (/api/flyers)', () => {
 
       expect(response.status).toBe(200);
       expect(response.body).toEqual(mockFlyers);
+      // Also assert that the default limit and offset were used.
+      expect(db.flyerRepo.getFlyers).toHaveBeenCalledWith(expectLogger, 20, 0);
     });
 
     it('should pass limit and offset query parameters to the db function', async () => {
@@ -58,6 +60,18 @@ describe('Flyer Routes (/api/flyers)', () => {
       expect(db.flyerRepo.getFlyers).toHaveBeenCalledWith(expectLogger, 15, 30);
     });
 
+    it('should use default for offset when only limit is provided', async () => {
+      vi.mocked(db.flyerRepo.getFlyers).mockResolvedValue([]);
+      await supertest(app).get('/api/flyers?limit=5');
+      expect(db.flyerRepo.getFlyers).toHaveBeenCalledWith(expectLogger, 5, 0);
+    });
+
+    it('should use default for limit when only offset is provided', async () => {
+      vi.mocked(db.flyerRepo.getFlyers).mockResolvedValue([]);
+      await supertest(app).get('/api/flyers?offset=10');
+      expect(db.flyerRepo.getFlyers).toHaveBeenCalledWith(expectLogger, 20, 10);
+    });
+
     it('should return 500 if the database call fails', async () => {
       const dbError = new Error('DB Error');
       vi.mocked(db.flyerRepo.getFlyers).mockRejectedValue(dbError);
@@ -151,7 +165,7 @@ describe('Flyer Routes (/api/flyers)', () => {
       expect(response.status).toBe(500);
       expect(response.body.message).toBe('DB Error');
       expect(mockLogger.error).toHaveBeenCalledWith(
-        { error: dbError },
+        { error: dbError, flyerId: '123' },
         'Error fetching flyer items in /api/flyers/:id/items:',
       );
     });
@@ -276,5 +290,24 @@ describe('Flyer Routes (/api/flyers)', () => {
         .send({ type: 'invalid' });
       expect(response.status).toBe(400);
     });
+
+    it('should return 202 and log an error if the tracking function fails', async () => {
+      const trackingError = new Error('Tracking DB is down');
+      vi.mocked(db.flyerRepo.trackFlyerItemInteraction).mockRejectedValue(trackingError);
+
+      const response = await supertest(app)
+        .post('/api/flyers/items/99/track')
+        .send({ type: 'click' });
+
+      expect(response.status).toBe(202);
+
+      // Allow the event loop to process the unhandled promise rejection from the fire-and-forget call
+      await new Promise((resolve) => setImmediate(resolve));
+
+      expect(mockLogger.error).toHaveBeenCalledWith(
+        { error: trackingError, itemId: 99 },
+        'Flyer item interaction tracking failed',
+      );
+    });
   });
 });

src/routes/flyer.routes.ts

@@ -48,12 +48,12 @@ const trackItemSchema = z.object({
 /**
  * GET /api/flyers - Get a paginated list of all flyers.
  */
-type GetFlyersRequest = z.infer<typeof getFlyersSchema>;
 router.get('/', validateRequest(getFlyersSchema), async (req, res, next): Promise<void> => {
-  const { query } = req as unknown as GetFlyersRequest;
   try {
-    const limit = query.limit ? Number(query.limit) : 20;
-    const offset = query.offset ? Number(query.offset) : 0;
+    // The `validateRequest` middleware ensures `req.query` is valid.
+    // We parse it here to apply Zod's coercions (string to number) and defaults.
+    const { limit, offset } = getFlyersSchema.shape.query.parse(req.query);
+
     const flyers = await db.flyerRepo.getFlyers(req.log, limit, offset);
     res.json(flyers);
   } catch (error) {
@@ -65,14 +65,14 @@ router.get('/', validateRequest(getFlyersSchema), async (req, res, next): Promis
 /**
  * GET /api/flyers/:id - Get a single flyer by its ID.
  */
-type GetFlyerByIdRequest = z.infer<typeof flyerIdParamSchema>;
 router.get('/:id', validateRequest(flyerIdParamSchema), async (req, res, next): Promise<void> => {
-  const { params } = req as unknown as GetFlyerByIdRequest;
   try {
-    const flyer = await db.flyerRepo.getFlyerById(params.id);
+    // Explicitly parse to get the coerced number type for `id`.
+    const { id } = flyerIdParamSchema.shape.params.parse(req.params);
+    const flyer = await db.flyerRepo.getFlyerById(id);
     res.json(flyer);
   } catch (error) {
-    req.log.error({ error, flyerId: params.id }, 'Error fetching flyer by ID:');
+    req.log.error({ error, flyerId: req.params.id }, 'Error fetching flyer by ID:');
     next(error);
   }
 });
@@ -84,12 +84,14 @@ router.get(
   '/:id/items',
   validateRequest(flyerIdParamSchema),
   async (req, res, next): Promise<void> => {
-    const { params } = req as unknown as GetFlyerByIdRequest;
+    type GetFlyerByIdRequest = z.infer<typeof flyerIdParamSchema>;
     try {
-      const items = await db.flyerRepo.getFlyerItems(params.id, req.log);
+      // Explicitly parse to get the coerced number type for `id`.
+      const { id } = flyerIdParamSchema.shape.params.parse(req.params);
+      const items = await db.flyerRepo.getFlyerItems(id, req.log);
       res.json(items);
     } catch (error) {
-      req.log.error({ error }, 'Error fetching flyer items in /api/flyers/:id/items:');
+      req.log.error({ error, flyerId: req.params.id }, 'Error fetching flyer items in /api/flyers/:id/items:');
       next(error);
     }
   },
@@ -105,6 +107,8 @@ router.post(
   async (req, res, next): Promise<void> => {
     const { body } = req as unknown as BatchFetchRequest;
     try {
+      // No re-parsing needed here as `validateRequest` has already ensured the body shape,
+      // and `express.json()` has parsed it. There's no type coercion to apply.
       const items = await db.flyerRepo.getFlyerItemsForFlyers(body.flyerIds, req.log);
       res.json(items);
     } catch (error) {
@@ -124,8 +128,9 @@ router.post(
   async (req, res, next): Promise<void> => {
     const { body } = req as unknown as BatchCountRequest;
     try {
-      // The DB function handles an empty array, so we can simplify.
-      const count = await db.flyerRepo.countFlyerItemsForFlyers(body.flyerIds ?? [], req.log);
+      // The schema ensures flyerIds is an array of numbers.
+      // The `?? []` was redundant as `validateRequest` would have already caught a missing `flyerIds`.
+      const count = await db.flyerRepo.countFlyerItemsForFlyers(body.flyerIds, req.log);
       res.json({ count });
     } catch (error) {
       req.log.error({ error }, 'Error counting batch flyer items');
@@ -137,11 +142,22 @@ router.post(
 /**
  * POST /api/flyers/items/:itemId/track - Tracks a user interaction with a flyer item.
  */
-type TrackItemRequest = z.infer<typeof trackItemSchema>;
-router.post('/items/:itemId/track', validateRequest(trackItemSchema), (req, res): void => {
-  const { params, body } = req as unknown as TrackItemRequest;
-  db.flyerRepo.trackFlyerItemInteraction(params.itemId, body.type, req.log);
-  res.status(202).send();
+router.post('/items/:itemId/track', validateRequest(trackItemSchema), (req, res, next): void => {
+  try {
+    // Explicitly parse to get coerced types.
+    const { params, body } = trackItemSchema.parse({ params: req.params, body: req.body });
+
+    // Fire-and-forget: we don't await the tracking call to avoid delaying the response.
+    // We add a .catch to log any potential errors without crashing the server process.
+    db.flyerRepo.trackFlyerItemInteraction(params.itemId, body.type, req.log).catch((error) => {
+      req.log.error({ error, itemId: params.itemId }, 'Flyer item interaction tracking failed');
+    });
+
+    res.status(202).send();
+  } catch (error) {
+    // This will catch Zod parsing errors if they occur.
+    next(error);
+  }
 });
 
 export default router;

src/routes/passport.routes.test.ts

@@ -414,6 +414,29 @@ describe('Passport Configuration', () => {
       // Assert
       expect(done).toHaveBeenCalledWith(dbError, false);
     });
+
+    it('should call done(err, false) if jwt_payload is null', async () => {
+      // Arrange
+      const jwtPayload = null;
+      const done = vi.fn();
+
+      // Act
+      // We know the mock setup populates the callback.
+      if (verifyCallbackWrapper.callback) {
+        // The strategy would not even call the callback if the token is invalid/missing.
+        // However, to test the robustness of our callback, we can invoke it directly with null.
+        await verifyCallbackWrapper.callback(jwtPayload as any, done);
+      }
+
+      // Assert
+      // The code will throw a TypeError because it tries to access 'user_id' of null.
+      // The catch block in the strategy will catch this and call done(err, false).
+      expect(done).toHaveBeenCalledWith(expect.any(TypeError), false);
+      expect(logger.error).toHaveBeenCalledWith(
+        { error: expect.any(TypeError) },
+        'Error during JWT authentication strategy:',
+      );
+    });
   });
 
   describe('isAdmin Middleware', () => {
@@ -477,6 +500,54 @@ describe('Passport Configuration', () => {
       expect(mockRes.status).toHaveBeenCalledWith(403);
     });
 
+    it('should return 403 Forbidden for various invalid user object shapes', () => {
+      const mockNext = vi.fn();
+      const mockRes: Partial<Response> = {
+        status: vi.fn().mockReturnThis(),
+        json: vi.fn(),
+      };
+
+      // Case 1: user is not an object (e.g., a string)
+      const req1 = { user: 'not-an-object' } as unknown as Request;
+      isAdmin(req1, mockRes as Response, mockNext);
+      expect(mockRes.status).toHaveBeenLastCalledWith(403);
+      expect(mockNext).not.toHaveBeenCalled();
+      vi.clearAllMocks();
+
+      // Case 2: user is null
+      const req2 = { user: null } as unknown as Request;
+      isAdmin(req2, mockRes as Response, mockNext);
+      expect(mockRes.status).toHaveBeenLastCalledWith(403);
+      expect(mockNext).not.toHaveBeenCalled();
+      vi.clearAllMocks();
+
+      // Case 3: user object is missing 'user' property
+      const req3 = { user: { role: 'admin' } } as unknown as Request;
+      isAdmin(req3, mockRes as Response, mockNext);
+      expect(mockRes.status).toHaveBeenLastCalledWith(403);
+      expect(mockNext).not.toHaveBeenCalled();
+      vi.clearAllMocks();
+
+      // Case 4: user.user is not an object
+      const req4 = { user: { role: 'admin', user: 'not-an-object' } } as unknown as Request;
+      isAdmin(req4, mockRes as Response, mockNext);
+      expect(mockRes.status).toHaveBeenLastCalledWith(403);
+      expect(mockNext).not.toHaveBeenCalled();
+      vi.clearAllMocks();
+
+      // Case 5: user.user is missing 'user_id'
+      const req5 = {
+        user: { role: 'admin', user: { email: 'test@test.com' } },
+      } as unknown as Request;
+      isAdmin(req5, mockRes as Response, mockNext);
+      expect(mockRes.status).toHaveBeenLastCalledWith(403);
+      expect(mockNext).not.toHaveBeenCalled();
+      vi.clearAllMocks();
+
+      // Reset the main mockNext for other tests in the suite
+      mockNext.mockClear();
+    });
+
     it('should return 403 Forbidden if req.user is not a valid UserProfile object', () => {
       // Arrange
       const mockReq: Partial<Request> = {
@@ -611,13 +682,18 @@ describe('Passport Configuration', () => {
       optionalAuth(mockReq, mockRes as Response, mockNext);
 
       // Assert
+      // The new implementation logs a warning and proceeds.
+      expect(logger.warn).toHaveBeenCalledWith(
+        { error: authError },
+        'Optional auth encountered an error, proceeding anonymously.',
+      );
       expect(mockReq.user).toBeUndefined();
       expect(mockNext).toHaveBeenCalledTimes(1);
     });
   });
 
   describe('mockAuth Middleware', () => {
-    const mockNext: NextFunction = vi.fn();
+    const mockNext: NextFunction = vi.fn(); // This was a duplicate, fixed.
     const mockRes: Partial<Response> = {
       status: vi.fn().mockReturnThis(),
       json: vi.fn(),

src/routes/passport.routes.ts

@@ -323,12 +323,17 @@ export const optionalAuth = (req: Request, res: Response, next: NextFunction) =>
     'jwt',
     { session: false },
     (err: Error | null, user: Express.User | false, info: { message: string } | Error) => {
-      // If there's an authentication error (e.g., malformed token), log it but don't block the request.
+      if (err) {
+        // An actual error occurred during authentication (e.g., malformed token).
+        // For optional auth, we log this but still proceed without a user.
+        logger.warn({ error: err }, 'Optional auth encountered an error, proceeding anonymously.');
+        return next();
+      }
       if (info) {
         // The patch requested this specific error handling.
         logger.info({ info: info.message || info.toString() }, 'Optional auth info:');
-      } // The patch requested this specific error handling.
-      if (user) (req as Express.Request).user = user; // Attach user if authentication succeeds
+      }
+      if (user) (req as Express.Request).user = user; // Attach user if authentication succeeds.
 
       next(); // Always proceed to the next middleware
     },

src/routes/user.routes.test.ts

@@ -1140,6 +1140,19 @@ describe('User Routes (/api/users)', () => {
         expect(logger.error).toHaveBeenCalled();
       });
 
+      it('DELETE /recipes/:recipeId should return 404 if recipe not found', async () => {
+        vi.mocked(db.recipeRepo.deleteRecipe).mockRejectedValue(new NotFoundError('Recipe not found'));
+        const response = await supertest(app).delete('/api/users/recipes/999');
+        expect(response.status).toBe(404);
+        expect(response.body.message).toBe('Recipe not found');
+      });
+
+      it('DELETE /recipes/:recipeId should return 400 for invalid recipe ID', async () => {
+        const response = await supertest(app).delete('/api/users/recipes/abc');
+        expect(response.status).toBe(400);
+        expect(response.body.errors[0].message).toContain('received NaN');
+      });
+
       it("PUT /recipes/:recipeId should update a user's own recipe", async () => {
         const updates = { description: 'A new delicious description.' };
         const mockUpdatedRecipe = createMockRecipe({ recipe_id: 1, ...updates });
@@ -1181,6 +1194,14 @@ describe('User Routes (/api/users)', () => {
         expect(response.body.errors[0].message).toBe('No fields provided to update.');
       });
 
+      it('PUT /recipes/:recipeId should return 400 for invalid recipe ID', async () => {
+        const response = await supertest(app)
+          .put('/api/users/recipes/abc')
+          .send({ name: 'New Name' });
+        expect(response.status).toBe(400);
+        expect(response.body.errors[0].message).toContain('received NaN');
+      });
+
       it('GET /shopping-lists/:listId should return 404 if list is not found', async () => {
         vi.mocked(db.shoppingRepo.getShoppingListById).mockRejectedValue(
           new NotFoundError('Shopping list not found'),

src/services/flyerProcessingService.server.test.ts

@@ -36,6 +36,7 @@ import {
   PdfConversionError,
   UnsupportedFileTypeError,
   TransformationError,
+  DatabaseError,
 } from './processingErrors';
 import { NotFoundError } from './db/errors.db';
 import { FlyerFileHandler } from './flyerFileHandler.server';
@@ -400,8 +401,8 @@ describe('FlyerProcessingService', () => {
       // This is more realistic than mocking the inner function `createFlyerAndItems`.
       vi.mocked(mockedDb.withTransaction).mockRejectedValue(dbError);
 
-      // The service wraps the generic DB error in a DatabaseError, but _reportErrorAndThrow re-throws the original.
-      await expect(service.processJob(job)).rejects.toThrow(dbError);
+      // The service wraps the generic DB error in a DatabaseError.
+      await expect(service.processJob(job)).rejects.toThrow(DatabaseError);
 
       // The final progress update should reflect the structured DatabaseError.
       expect(job.updateProgress).toHaveBeenLastCalledWith({