diff --git a/src/features/flyer/FlyerUploader.test.tsx b/src/features/flyer/FlyerUploader.test.tsx index 6af6c929..422ab928 100644 --- a/src/features/flyer/FlyerUploader.test.tsx +++ b/src/features/flyer/FlyerUploader.test.tsx @@ -65,8 +65,6 @@ describe('FlyerUploader', () => { return () => {}; }); console.log(`\n--- [TEST LOG] ---: Starting test: "${expect.getState().currentTestName}"`); - // Use fake timers to control polling intervals. - vi.useFakeTimers(); vi.resetAllMocks(); // Resets mock implementations AND call history. console.log('--- [TEST LOG] ---: Mocks reset.'); mockedChecksumModule.generateFileChecksum.mockResolvedValue('mock-checksum'); @@ -74,7 +72,6 @@ describe('FlyerUploader', () => { }); afterEach(() => { - vi.useRealTimers(); console.log(`--- [TEST LOG] ---: Finished test: "${expect.getState().currentTestName}"\n`); }); @@ -117,21 +114,18 @@ describe('FlyerUploader', () => { expect(mockedAiApiClient.getJobStatus).toHaveBeenCalledTimes(1); console.log('--- [TEST LOG] ---: 7. Mocks verified. Advancing timers now...'); - await act(async () => { - console.log('--- [TEST LOG] ---: 8a. vi.advanceTimersByTime(3000) starting...'); - vi.advanceTimersByTime(3000); - console.log('--- [TEST LOG] ---: 8b. vi.advanceTimersByTime(3000) complete.'); - }); + // With real timers, we now wait for the polling interval to elapse. console.log( `--- [TEST LOG] ---: 9. Act block finished. Now checking if getJobStatus was called again.`, ); try { + // The polling interval is 3s, so we wait for a bit longer. await waitFor(() => { const calls = mockedAiApiClient.getJobStatus.mock.calls.length; console.log(`--- [TEST LOG] ---: 10. waitFor check: getJobStatus calls = ${calls}`); expect(mockedAiApiClient.getJobStatus).toHaveBeenCalledTimes(2); - }); + }, { timeout: 4000 }); console.log('--- [TEST LOG] ---: 11. SUCCESS: Second poll confirmed.'); } catch (error) { console.error('--- [TEST LOG] ---: 11. ERROR: waitFor for second poll timed out.'); @@ -194,19 +188,16 @@ describe('FlyerUploader', () => { expect(mockedAiApiClient.getJobStatus).toHaveBeenCalledTimes(1); console.log('--- [TEST LOG] ---: 5. First poll confirmed. Now AWAITING timer advancement.'); - await act(async () => { - console.log(`--- [TEST LOG] ---: 6. Advancing timers by 4000ms for the second poll...`); - vi.advanceTimersByTime(4000); - }); - console.log(`--- [TEST LOG] ---: 7. Timers advanced. Now AWAITING completion message.`); - try { console.log( '--- [TEST LOG] ---: 8a. waitFor check: Waiting for completion text and job status count.', ); + // Wait for the second poll to occur and the UI to update. await waitFor(() => { console.log( - `--- [TEST LOG] ---: 8b. waitFor interval: calls=${mockedAiApiClient.getJobStatus.mock.calls.length}`, + `--- [TEST LOG] ---: 8b. waitFor interval: calls=${ + mockedAiApiClient.getJobStatus.mock.calls.length + }`, ); expect( screen.getByText('Processing complete! Redirecting to flyer 42...'), @@ -221,12 +212,9 @@ describe('FlyerUploader', () => { } expect(mockedAiApiClient.getJobStatus).toHaveBeenCalledTimes(2); - await act(async () => { - console.log(`--- [TEST LOG] ---: 10. Advancing timers by 2000ms for redirect...`); - vi.advanceTimersByTime(2000); - }); + // Wait for the redirect timer (1.5s in component) to fire. + await act(() => new Promise((r) => setTimeout(r, 2000))); console.log(`--- [TEST LOG] ---: 11. Timers advanced. Now asserting navigation.`); - expect(onProcessingComplete).toHaveBeenCalled(); expect(navigateSpy).toHaveBeenCalledWith('/flyers/42'); console.log('--- [TEST LOG] ---: 12. Callback and navigation confirmed.'); @@ -291,22 +279,16 @@ describe('FlyerUploader', () => { // Wait for the first poll to complete and UI to update to "Working..." await screen.findByText('Working...'); - // Advance time to trigger the second poll - await act(async () => { - vi.advanceTimersByTime(3000); - }); - // Wait for the failure UI - await screen.findByText(/Processing failed: Fatal Error/i); + await waitFor(() => expect(screen.getByText(/Processing failed: Fatal Error/i)).toBeInTheDocument(), { timeout: 4000 }); // Verify clearTimeout was called expect(clearTimeoutSpy).toHaveBeenCalled(); // Verify no further polling occurs const callsBefore = mockedAiApiClient.getJobStatus.mock.calls.length; - await act(async () => { - vi.advanceTimersByTime(10000); - }); + // Wait for a duration longer than the polling interval + await act(() => new Promise((r) => setTimeout(r, 4000))); expect(mockedAiApiClient.getJobStatus).toHaveBeenCalledTimes(callsBefore); clearTimeoutSpy.mockRestore(); diff --git a/src/hooks/useAuth.test.tsx b/src/hooks/useAuth.test.tsx index 5a2f9b3f..bdcf7a8a 100644 --- a/src/hooks/useAuth.test.tsx +++ b/src/hooks/useAuth.test.tsx @@ -150,6 +150,10 @@ describe('useAuth Hook and AuthProvider', () => { describe('login function', () => { // This was the failing test it('sets token, fetches profile, and updates state on successful login', async () => { + // --- FIX --- + // Explicitly mock that no token exists initially to prevent state leakage from other tests. + mockedTokenStorage.getToken.mockReturnValue(null); + // --- FIX --- // The mock for `getAuthenticatedUserProfile` must resolve to a `Response`-like object, // as this is the return type of the actual function. The `useApi` hook then @@ -302,6 +306,10 @@ describe('useAuth Hook and AuthProvider', () => { }); it('should not update profile if user is not authenticated', async () => { + // --- FIX --- + // Explicitly mock that no token exists initially to prevent state leakage from other tests. + mockedTokenStorage.getToken.mockReturnValue(null); + const { result } = renderHook(() => useAuth(), { wrapper }); // Wait for initial check to complete diff --git a/src/middleware/multer.middleware.ts b/src/middleware/multer.middleware.ts new file mode 100644 index 00000000..fa82db7e --- /dev/null +++ b/src/middleware/multer.middleware.ts @@ -0,0 +1,119 @@ +// src/middleware/multer.middleware.ts +import multer from 'multer'; +import path from 'path'; +import fs from 'node:fs/promises'; +import { Request, Response, NextFunction } from 'express'; +import { UserProfile } from '../types'; +import { sanitizeFilename } from '../utils/stringUtils'; +import { logger } from '../services/logger.server'; + +export const flyerStoragePath = + process.env.STORAGE_PATH || '/var/www/flyer-crawler.projectium.com/flyer-images'; +export const avatarStoragePath = path.join(process.cwd(), 'public', 'uploads', 'avatars'); + +// Ensure directories exist at startup +(async () => { + try { + await fs.mkdir(flyerStoragePath, { recursive: true }); + await fs.mkdir(avatarStoragePath, { recursive: true }); + logger.info('Ensured multer storage directories exist.'); + } catch (error) { + const err = error instanceof Error ? error : new Error(String(error)); + logger.error({ error: err }, 'Failed to create multer storage directories on startup.'); + } +})(); + +type StorageType = 'flyer' | 'avatar'; + +const getStorageConfig = (type: StorageType) => { + switch (type) { + case 'avatar': + return multer.diskStorage({ + destination: (req, file, cb) => cb(null, avatarStoragePath), + filename: (req, file, cb) => { + const user = req.user as UserProfile | undefined; + if (!user) { + // This should ideally not happen if auth middleware runs first. + return cb(new Error('User not authenticated for avatar upload'), ''); + } + const uniqueSuffix = `${user.user.user_id}-${Date.now()}${path.extname( + file.originalname, + )}`; + cb(null, uniqueSuffix); + }, + }); + case 'flyer': + default: + return multer.diskStorage({ + destination: (req, file, cb) => cb(null, flyerStoragePath), + filename: (req, file, cb) => { + if (process.env.NODE_ENV === 'test') { + // Use a predictable filename for test flyers for easy cleanup. + const ext = path.extname(file.originalname); + return cb(null, `${file.fieldname}-test-flyer-image${ext || '.jpg'}`); + } + const uniqueSuffix = `${Date.now()}-${Math.round(Math.random() * 1e9)}`; + const sanitizedOriginalName = sanitizeFilename(file.originalname); + cb(null, `${file.fieldname}-${uniqueSuffix}-${sanitizedOriginalName}`); + }, + }); + } +}; + +const imageFileFilter = (req: Request, file: Express.Multer.File, cb: multer.FileFilterCallback) => { + if (file.mimetype.startsWith('image/')) { + cb(null, true); + } else { + // Reject the file with a specific error that can be caught by a middleware. + const err = new Error('Only image files are allowed!'); + cb(err); + } +}; + +interface MulterOptions { + storageType: StorageType; + fileSize?: number; + fileFilter?: 'image'; +} + +/** + * Creates a configured multer instance for file uploads. + * @param options - Configuration for storage type, file size, and file filter. + * @returns A multer instance. + */ +export const createUploadMiddleware = (options: MulterOptions) => { + const multerOptions: multer.Options = { + storage: getStorageConfig(options.storageType), + }; + + if (options.fileSize) { + multerOptions.limits = { fileSize: options.fileSize }; + } + + if (options.fileFilter === 'image') { + multerOptions.fileFilter = imageFileFilter; + } + + return multer(multerOptions); +}; + +/** + * A general error handler for multer. Place this after all routes using multer in your router file. + * It catches errors from `fileFilter` and other multer issues (e.g., file size limits). + */ +export const handleMulterError = ( + err: Error, + req: Request, + res: Response, + next: NextFunction, +) => { + if (err instanceof multer.MulterError) { + // A Multer error occurred when uploading (e.g., file too large). + return res.status(400).json({ message: `File upload error: ${err.message}` }); + } else if (err && err.message === 'Only image files are allowed!') { + // A custom error from our fileFilter. + return res.status(400).json({ message: err.message }); + } + // If it's not a multer error, pass it on. + next(err); +}; \ No newline at end of file diff --git a/src/pages/admin/components/ProfileManager.test.tsx b/src/pages/admin/components/ProfileManager.test.tsx index bd594c8c..fffdd72d 100644 --- a/src/pages/admin/components/ProfileManager.test.tsx +++ b/src/pages/admin/components/ProfileManager.test.tsx @@ -436,7 +436,9 @@ describe('ProfileManager', () => { }); }); - it('should automatically geocode address after user stops typing', async () => { + it('should automatically geocode address after user stops typing (using fake timers)', async () => { + // Use real timers for the initial async render and data fetch + vi.useRealTimers(); const addressWithoutCoords = { ...mockAddress, latitude: undefined, longitude: undefined }; mockedApiClient.getUserAddress.mockResolvedValue( new Response(JSON.stringify(addressWithoutCoords)), @@ -454,13 +456,19 @@ describe('ProfileManager', () => { fireEvent.change(screen.getByLabelText(/city/i), { target: { value: 'NewCity' } }); expect(mockedApiClient.geocodeAddress).not.toHaveBeenCalled(); - console.log('[TEST LOG] Waiting 1600ms for debounce...'); - // Wait for debounce (1500ms) + buffer using real timers to avoid freeze - await act(async () => { - await new Promise((resolve) => setTimeout(resolve, 1600)); + // Switch to fake timers to control the debounce timeout + vi.useFakeTimers(); + + // Advance timers to trigger the debounced function + act(() => { + vi.advanceTimersByTime(1500); // Must match debounce delay in useProfileAddress }); console.log('[TEST LOG] Wait complete. Checking results.'); + // Switch back to real timers to allow the async geocodeAddress promise to resolve + vi.useRealTimers(); + + // Now wait for the UI to update after the promise resolves await waitFor(() => { expect(mockedApiClient.geocodeAddress).toHaveBeenCalledWith( expect.stringContaining('NewCity'), @@ -470,17 +478,19 @@ describe('ProfileManager', () => { }); }); - it('should not geocode if address already has coordinates', async () => { - console.log('[TEST LOG] Rendering for no-geocode test (Real Timers + Wait)'); + it('should not geocode if address already has coordinates (using fake timers)', async () => { + // Use real timers for the initial async render and data fetch + vi.useRealTimers(); render(); console.log('[TEST LOG] Waiting for initial address load...'); await waitFor(() => expect(screen.getByLabelText(/city/i)).toHaveValue('Anytown')); - console.log( - '[TEST LOG] Initial address loaded. Waiting 1600ms to ensure no geocode triggers...', - ); - await act(async () => { - await new Promise((resolve) => setTimeout(resolve, 1600)); + // Switch to fake timers to control the debounce check + vi.useFakeTimers(); + + // Advance timers past the debounce threshold. Nothing should happen. + act(() => { + vi.advanceTimersByTime(1600); }); console.log('[TEST LOG] Wait complete. Verifying no geocode call.'); @@ -524,7 +534,7 @@ describe('ProfileManager', () => { expect(await screen.findByRole('heading', { name: /theme/i })).toBeInTheDocument(); // Switch back to Profile - fireEvent.click(screen.getByRole('button', { name: /profile/i })); + fireEvent.click(screen.getByRole('button', { name: /^profile$/i })); expect(await screen.findByLabelText('Profile Form')).toBeInTheDocument(); }); @@ -735,9 +745,7 @@ describe('ProfileManager', () => { }); it('should handle account deletion flow', async () => { - // Use fake timers to test the setTimeout call - vi.useFakeTimers(); - const { unmount } = render(); + render(); fireEvent.click(screen.getByRole('button', { name: /data & privacy/i })); @@ -767,15 +775,16 @@ describe('ProfileManager', () => { ); }); + // Now, switch to fake timers to control the setTimeout. + vi.useFakeTimers(); + // Advance timers to trigger the logout and close act(() => { vi.advanceTimersByTime(3000); }); - await waitFor(() => { - expect(mockOnClose).toHaveBeenCalled(); - expect(mockOnSignOut).toHaveBeenCalled(); - }); + expect(mockOnClose).toHaveBeenCalled(); + expect(mockOnSignOut).toHaveBeenCalled(); }); it('should allow toggling dark mode', async () => { diff --git a/src/routes/admin.content.routes.test.ts b/src/routes/admin.content.routes.test.ts index ac01e469..b697f729 100644 --- a/src/routes/admin.content.routes.test.ts +++ b/src/routes/admin.content.routes.test.ts @@ -244,7 +244,7 @@ describe('Admin Content Management Routes (/api/admin)', () => { expect(response.body.message).toBe('Brand logo updated successfully.'); expect(vi.mocked(mockedDb.adminRepo.updateBrandLogo)).toHaveBeenCalledWith( brandId, - expect.stringContaining('/assets/'), + expect.stringContaining('/flyer-images/'), expect.anything(), ); }); diff --git a/src/routes/admin.routes.ts b/src/routes/admin.routes.ts index 80085694..c06c2d42 100644 --- a/src/routes/admin.routes.ts +++ b/src/routes/admin.routes.ts @@ -2,7 +2,6 @@ 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'; @@ -10,6 +9,10 @@ import * as db from '../services/db/index.db'; import type { UserProfile } from '../types'; import { geocodingService } from '../services/geocodingService.server'; import { requireFileUpload } from '../middleware/fileUpload.middleware'; // This was a duplicate, fixed. +import { + createUploadMiddleware, + handleMulterError, +} from '../middleware/multer.middleware'; import { NotFoundError, ValidationError } from '../services/db/errors.db'; import { validateRequest } from '../middleware/validation.middleware'; @@ -42,6 +45,7 @@ import { optionalNumeric, } from '../utils/zodUtils'; import { logger } from '../services/logger.server'; +import fs from 'node:fs/promises'; /** * Safely deletes a file from the filesystem, ignoring errors if the file doesn't exist. @@ -102,19 +106,7 @@ const jobRetrySchema = z.object({ const router = Router(); -// --- Multer Configuration for File Uploads --- -const storagePath = - process.env.STORAGE_PATH || '/var/www/flyer-crawler.projectium.com/flyer-images'; -const storage = multer.diskStorage({ - destination: function (req, file, cb) { - cb(null, storagePath); - }, - filename: function (req, file, cb) { - const uniqueSuffix = Date.now() + '-' + Math.round(Math.random() * 1e9); - cb(null, file.fieldname + '-' + uniqueSuffix + '-' + file.originalname); - }, -}); -const upload = multer({ storage: storage }); +const upload = createUploadMiddleware({ storageType: 'flyer' }); // --- Bull Board (Job Queue UI) Setup --- const serverAdapter = new ExpressAdapter(); @@ -698,4 +690,7 @@ router.post( }, ); +/* Catches errors from multer (e.g., file size, file filter) */ +router.use(handleMulterError); + export default router; diff --git a/src/routes/ai.routes.ts b/src/routes/ai.routes.ts index 142bcb3c..7f1ac5b7 100644 --- a/src/routes/ai.routes.ts +++ b/src/routes/ai.routes.ts @@ -1,6 +1,5 @@ // src/routes/ai.routes.ts import { Router, Request, Response, NextFunction } from 'express'; -import multer from 'multer'; import path from 'path'; import fs from 'node:fs'; import { z } from 'zod'; @@ -9,8 +8,11 @@ import { optionalAuth } from './passport.routes'; import * as db from '../services/db/index.db'; import { createFlyerAndItems } from '../services/db/flyer.db'; import * as aiService from '../services/aiService.server'; // Correctly import server-side AI service +import { + createUploadMiddleware, + handleMulterError, +} from '../middleware/multer.middleware'; import { generateFlyerIcon } from '../utils/imageProcessor'; -import { sanitizeFilename } from '../utils/stringUtils'; import { logger } from '../services/logger.server'; import { UserProfile, ExtractedCoreData, ExtractedFlyerItem } from '../types'; import { flyerQueue } from '../services/queueService.server'; @@ -154,40 +156,7 @@ const searchWebSchema = z.object({ body: z.object({ query: requiredString('A search query is required.') }), }); -// --- Multer Configuration for File Uploads --- -const storagePath = - process.env.STORAGE_PATH || '/var/www/flyer-crawler.projectium.com/flyer-images'; - -// Ensure the storage path exists at startup so multer can write files there. -try { - fs.mkdirSync(storagePath, { recursive: true }); - logger.debug(`AI upload storage path ready: ${storagePath}`); -} catch (err) { - logger.error( - { error: errMsg(err) }, - `Failed to create storage path (${storagePath}). File uploads may fail.`, - ); -} -const diskStorage = multer.diskStorage({ - destination: function (req, file, cb) { - cb(null, storagePath); - }, - filename: function (req, file, cb) { - // If in a test environment, use a predictable filename for easy cleanup. - if (process.env.NODE_ENV === 'test') { - return cb(null, `${file.fieldname}-test-flyer-image.jpg`); - } else { - const uniqueSuffix = Date.now() + '-' + Math.round(Math.random() * 1e9); - // Sanitize the original filename to remove spaces and special characters - return cb( - null, - file.fieldname + '-' + uniqueSuffix + '-' + sanitizeFilename(file.originalname), - ); - } - }, -}); - -const uploadToDisk = multer({ storage: diskStorage }); +const uploadToDisk = createUploadMiddleware({ storageType: 'flyer' }); // Diagnostic middleware: log incoming AI route requests (headers and sizes) router.use((req: Request, res: Response, next: NextFunction) => { @@ -722,4 +691,7 @@ router.post( }, ); +/* Catches errors from multer (e.g., file size, file filter) */ +router.use(handleMulterError); + export default router; diff --git a/src/routes/health.routes.ts b/src/routes/health.routes.ts index ea8392cc..ba5ab528 100644 --- a/src/routes/health.routes.ts +++ b/src/routes/health.routes.ts @@ -53,7 +53,7 @@ router.get('/db-schema', validateRequest(emptySchema), async (req, res, next: Ne * This is important for features like file uploads. */ router.get('/storage', validateRequest(emptySchema), async (req, res, next: NextFunction) => { - const storagePath = process.env.STORAGE_PATH || '/var/www/flyer-crawler.projectium.com/assets'; + const storagePath = process.env.STORAGE_PATH || '/var/www/flyer-crawler.projectium.com/flyer-images'; try { await fs.access(storagePath, fs.constants.W_OK); // Use fs.promises return res diff --git a/src/routes/user.routes.ts b/src/routes/user.routes.ts index bed10dfd..dc0f9542 100644 --- a/src/routes/user.routes.ts +++ b/src/routes/user.routes.ts @@ -1,13 +1,16 @@ // src/routes/user.routes.ts import express, { Request, Response, NextFunction } from 'express'; import passport from './passport.routes'; -import multer from 'multer'; -import path from 'path'; +import multer from 'multer'; // Keep for MulterError type check import fs from 'node:fs/promises'; import * as bcrypt from 'bcrypt'; // This was a duplicate, fixed. import { z } from 'zod'; import { logger } from '../services/logger.server'; import { UserProfile } from '../types'; +import { + createUploadMiddleware, + handleMulterError, +} from '../middleware/multer.middleware'; import { userService } from '../services/userService'; import { ForeignKeyConstraintError } from '../services/db/errors.db'; import { validateRequest } from '../middleware/validation.middleware'; @@ -85,35 +88,10 @@ const emptySchema = z.object({}); // Any request to a /api/users/* endpoint will now require a valid JWT. router.use(passport.authenticate('jwt', { session: false })); -// --- Multer Configuration for Avatar Uploads --- - -// Ensure the directory for avatar uploads exists. -const avatarUploadDir = path.join(process.cwd(), 'public', 'uploads', 'avatars'); -fs.mkdir(avatarUploadDir, { recursive: true }).catch((err) => { - logger.error({ err }, 'Failed to create avatar upload directory'); -}); - -// Define multer storage configuration. The `req.user` object will be available -// here because the passport middleware runs before this route handler. -const avatarStorage = multer.diskStorage({ - destination: (req, file, cb) => cb(null, avatarUploadDir), - filename: (req, file, cb) => { - const uniqueSuffix = `${(req.user as UserProfile).user.user_id}-${Date.now()}${path.extname(file.originalname)}`; - cb(null, uniqueSuffix); - }, -}); - -const avatarUpload = multer({ - storage: avatarStorage, - limits: { fileSize: 1 * 1024 * 1024 }, // 1MB file size limit - fileFilter: (req, file, cb) => { - if (file.mimetype.startsWith('image/')) { - cb(null, true); - } else { - // Reject the file with a specific error - cb(new Error('Only image files are allowed!')); - } - }, +const avatarUpload = createUploadMiddleware({ + storageType: 'avatar', + fileSize: 1 * 1024 * 1024, // 1MB + fileFilter: 'image', }); /** @@ -857,18 +835,7 @@ router.put( }, ); -// --- General Multer Error Handler --- -// This should be placed after all routes that use multer. -// It catches errors from `fileFilter` and other multer issues (e.g., file size limits). -router.use((err: Error, req: Request, res: Response, next: NextFunction) => { - if (err instanceof multer.MulterError) { - // A Multer error occurred when uploading (e.g., file too large). - return res.status(400).json({ message: `File upload error: ${err.message}` }); - } else if (err && err.message === 'Only image files are allowed!') { - // A custom error from our fileFilter. - return res.status(400).json({ message: err.message }); - } - next(err); // Pass on to the next error handler if it's not a multer error we handle. -}); +/* Catches errors from multer (e.g., file size, file filter) */ +router.use(handleMulterError); export default router; diff --git a/src/services/aiApiClient.ts b/src/services/aiApiClient.ts index 84cbf1bb..2883c4a7 100644 --- a/src/services/aiApiClient.ts +++ b/src/services/aiApiClient.ts @@ -44,10 +44,12 @@ export const uploadAndProcessFlyer = async ( if (!response.ok) { let errorBody; + // Clone the response so we can read the body twice (once as JSON, and as text on failure). + const clonedResponse = response.clone(); try { errorBody = await response.json(); } catch (e) { - errorBody = { message: await response.text() }; + errorBody = { message: await clonedResponse.text() }; } // Throw a structured error so the component can inspect the status and body throw { status: response.status, body: errorBody };