diff --git a/src/features/flyer/FlyerUploader.test.tsx b/src/features/flyer/FlyerUploader.test.tsx index c3ab5c14..61ef1bfe 100644 --- a/src/features/flyer/FlyerUploader.test.tsx +++ b/src/features/flyer/FlyerUploader.test.tsx @@ -328,10 +328,11 @@ describe('FlyerUploader', () => { it('should handle a duplicate flyer error (409)', async () => { console.log('--- [TEST LOG] ---: 1. Setting up mock for 409 duplicate error.'); - // The API client now throws a structured error for non-2xx responses. + // The API client throws a structured error, which useFlyerUploader now parses + // to set both the errorMessage and the duplicateFlyerId. mockedAiApiClient.uploadAndProcessFlyer.mockRejectedValue({ status: 409, - body: { flyerId: 99, message: 'Duplicate' }, + body: { flyerId: 99, message: 'This flyer has already been processed.' }, }); console.log('--- [TEST LOG] ---: 2. Rendering and uploading.'); @@ -345,9 +346,10 @@ describe('FlyerUploader', () => { try { console.log('--- [TEST LOG] ---: 4. AWAITING duplicate flyer message...'); - expect( - await screen.findByText(/This flyer has already been processed/i), - ).toBeInTheDocument(); + // With the fix, the duplicate error message and the link are combined into a single paragraph. + // We now look for this combined message. + const errorMessage = await screen.findByText(/This flyer has already been processed. You can view it here:/i); + expect(errorMessage).toBeInTheDocument(); console.log('--- [TEST LOG] ---: 5. SUCCESS: Duplicate message found.'); } catch (error) { console.error('--- [TEST LOG] ---: 5. ERROR: findByText for duplicate message timed out.'); diff --git a/src/features/flyer/FlyerUploader.tsx b/src/features/flyer/FlyerUploader.tsx index 3983ff81..75277128 100644 --- a/src/features/flyer/FlyerUploader.tsx +++ b/src/features/flyer/FlyerUploader.tsx @@ -30,6 +30,12 @@ export const FlyerUploader: React.FC = ({ onProcessingComple if (statusMessage) logger.info(`FlyerUploader Status: ${statusMessage}`); }, [statusMessage]); + useEffect(() => { + if (errorMessage) { + logger.error(`[FlyerUploader] Error encountered: ${errorMessage}`, { duplicateFlyerId }); + } + }, [errorMessage, duplicateFlyerId]); + // Handle completion and navigation useEffect(() => { if (processingState === 'completed' && flyerId) { @@ -94,14 +100,15 @@ export const FlyerUploader: React.FC = ({ onProcessingComple {errorMessage && (
-

{errorMessage}

- {duplicateFlyerId && ( + {duplicateFlyerId ? (

- This flyer has already been processed. You can view it here:{' '} + {errorMessage} You can view it here:{' '} Flyer #{duplicateFlyerId}

+ ) : ( +

{errorMessage}

)}
)} diff --git a/src/hooks/useApi.ts b/src/hooks/useApi.ts index 28057e63..0b79015d 100644 --- a/src/hooks/useApi.ts +++ b/src/hooks/useApi.ts @@ -29,6 +29,14 @@ export function useApi( const lastErrorMessageRef = useRef(null); const abortControllerRef = useRef(new AbortController()); + // Use a ref to track the latest apiFunction. This allows us to keep `execute` stable + // even if `apiFunction` is recreated on every render (common with inline arrow functions). + const apiFunctionRef = useRef(apiFunction); + + useEffect(() => { + apiFunctionRef.current = apiFunction; + }, [apiFunction]); + // This effect ensures that when the component using the hook unmounts, // any in-flight request is cancelled. useEffect(() => { @@ -59,7 +67,7 @@ export function useApi( } try { - const response = await apiFunction(...args, abortControllerRef.current.signal); + const response = await apiFunctionRef.current(...args, abortControllerRef.current.signal); if (!response.ok) { // Attempt to parse a JSON error response. This is aligned with ADR-003, @@ -98,7 +106,17 @@ export function useApi( } return result; } catch (e) { - const err = e instanceof Error ? e : new Error('An unknown error occurred.'); + let err: Error; + if (e instanceof Error) { + err = e; + } else if (typeof e === 'object' && e !== null && 'status' in e) { + // Handle structured errors (e.g. { status: 409, body: { ... } }) + const structuredError = e as { status: number; body?: { message?: string } }; + const message = structuredError.body?.message || `Request failed with status ${structuredError.status}`; + err = new Error(message); + } else { + err = new Error('An unknown error occurred.'); + } // If the error is an AbortError, it's an intentional cancellation, so we don't set an error state. if (err.name === 'AbortError') { logger.info('API request was cancelled.', { functionName: apiFunction.name }); @@ -122,7 +140,7 @@ export function useApi( setIsRefetching(false); } }, - [apiFunction], + [], // execute is now stable because it uses apiFunctionRef ); // abortControllerRef is stable return { execute, loading, isRefetching, error, data, reset }; diff --git a/src/hooks/useFlyerUploader.ts b/src/hooks/useFlyerUploader.ts index 8d02ac60..065a29e7 100644 --- a/src/hooks/useFlyerUploader.ts +++ b/src/hooks/useFlyerUploader.ts @@ -1,6 +1,6 @@ // src/hooks/useFlyerUploader.ts // src/hooks/useFlyerUploader.ts -import { useState, useCallback } from 'react'; +import { useState, useCallback, useMemo } from 'react'; import { useQuery, useMutation, useQueryClient } from '@tanstack/react-query'; import { uploadAndProcessFlyer, @@ -14,6 +14,28 @@ import type { ProcessingStage } from '../types'; export type ProcessingState = 'idle' | 'uploading' | 'polling' | 'completed' | 'error'; +// Define a type for the structured error thrown by the API client +interface ApiError { + status: number; + body: { + message: string; + flyerId?: number; + }; +} + +// Type guard to check if an error is a structured API error +function isApiError(error: unknown): error is ApiError { + return ( + typeof error === 'object' && + error !== null && + 'status' in error && + typeof (error as { status: unknown }).status === 'number' && + 'body' in error && + typeof (error as { body: unknown }).body === 'object' && + (error as { body: unknown }).body !== null && + 'message' in ((error as { body: unknown }).body as object) + ); +} export const useFlyerUploader = () => { const queryClient = useQueryClient(); const [jobId, setJobId] = useState(null); @@ -81,40 +103,55 @@ export const useFlyerUploader = () => { queryClient.removeQueries({ queryKey: ['jobStatus'] }); }, [uploadMutation, queryClient]); - // Consolidate state for the UI from the react-query hooks - const processingState = ((): ProcessingState => { - if (uploadMutation.isPending) return 'uploading'; - if (jobStatus && (jobStatus.state === 'active' || jobStatus.state === 'waiting')) - return 'polling'; - if (jobStatus?.state === 'completed') { - // If the job is complete but didn't return a flyerId, it's an error state. - if (!jobStatus.returnValue?.flyerId) { - return 'error'; + // Consolidate state derivation for the UI from the react-query hooks using useMemo. + // This improves performance by memoizing the derived state and makes the logic easier to follow. + const { processingState, errorMessage, duplicateFlyerId, flyerId, statusMessage } = useMemo(() => { + const state: ProcessingState = (() => { + if (uploadMutation.isPending) return 'uploading'; + if (jobStatus && (jobStatus.state === 'active' || jobStatus.state === 'waiting')) + return 'polling'; + if (jobStatus?.state === 'completed') { + if (!jobStatus.returnValue?.flyerId) return 'error'; + return 'completed'; } - return 'completed'; - } - if (uploadMutation.isError || jobStatus?.state === 'failed' || pollError) return 'error'; - return 'idle'; - })(); + if (uploadMutation.isError || jobStatus?.state === 'failed' || pollError) return 'error'; + return 'idle'; + })(); - const getErrorMessage = () => { - const uploadError = uploadMutation.error as any; - if (uploadMutation.isError) { - return uploadError?.body?.message || uploadError?.message || 'Upload failed.'; - } - if (pollError) return `Polling failed: ${pollError.message}`; - if (jobStatus?.state === 'failed') { - return `Processing failed: ${jobStatus.progress?.message || jobStatus.failedReason}`; - } - if (jobStatus?.state === 'completed' && !jobStatus.returnValue?.flyerId) { - return 'Job completed but did not return a flyer ID.'; - } - return null; - }; + let msg: string | null = null; + let dupId: number | null = null; - const errorMessage = getErrorMessage(); - const duplicateFlyerId = (uploadMutation.error as any)?.body?.flyerId ?? null; - const flyerId = jobStatus?.state === 'completed' ? jobStatus.returnValue?.flyerId : null; + if (state === 'error') { + if (uploadMutation.isError) { + const uploadError = uploadMutation.error; + if (isApiError(uploadError)) { + msg = uploadError.body.message; + // Specifically handle 409 Conflict for duplicate flyers + if (uploadError.status === 409) { + dupId = uploadError.body.flyerId ?? null; + } + } else if (uploadError instanceof Error) { + msg = uploadError.message; + } else { + msg = 'An unknown upload error occurred.'; + } + } else if (pollError) { + msg = `Polling failed: ${pollError.message}`; + } else if (jobStatus?.state === 'failed') { + msg = `Processing failed: ${jobStatus.progress?.message || jobStatus.failedReason || 'Unknown reason'}`; + } else if (jobStatus?.state === 'completed' && !jobStatus.returnValue?.flyerId) { + msg = 'Job completed but did not return a flyer ID.'; + } + } + + return { + processingState: state, + errorMessage: msg, + duplicateFlyerId: dupId, + flyerId: jobStatus?.state === 'completed' ? jobStatus.returnValue?.flyerId ?? null : null, + statusMessage: uploadMutation.isPending ? 'Uploading file...' : jobStatus?.progress?.message, + }; + }, [uploadMutation, jobStatus, pollError]); return { processingState, diff --git a/src/routes/admin.content.routes.test.ts b/src/routes/admin.content.routes.test.ts index d1e9a181..a5d86bc9 100644 --- a/src/routes/admin.content.routes.test.ts +++ b/src/routes/admin.content.routes.test.ts @@ -1,7 +1,8 @@ // src/routes/admin.content.routes.test.ts -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterAll } from 'vitest'; import supertest from 'supertest'; import type { Request, Response, NextFunction } from 'express'; +import path from 'path'; import { createMockUserProfile, createMockSuggestedCorrection, @@ -15,6 +16,7 @@ import type { SuggestedCorrection, Brand, UserProfile, UnmatchedFlyerItem } from import { NotFoundError } from '../services/db/errors.db'; // This can stay, it's a type/class not a module with side effects. import fs from 'node:fs/promises'; import { createTestApp } from '../tests/utils/createTestApp'; +import { cleanupFiles } from '../tests/utils/cleanupFiles'; // Mock the file upload middleware to allow testing the controller's internal check vi.mock('../middleware/fileUpload.middleware', () => ({ @@ -140,6 +142,26 @@ describe('Admin Content Management Routes (/api/admin)', () => { vi.clearAllMocks(); }); + afterAll(async () => { + // Safeguard to clean up any logo files created during tests. + const uploadDir = path.resolve(__dirname, '../../../flyer-images'); + try { + const allFiles = await fs.readdir(uploadDir); + // Files are named like 'logoImage-timestamp-original.ext' + const testFiles = allFiles + .filter((f) => f.startsWith('logoImage-')) + .map((f) => path.join(uploadDir, f)); + + if (testFiles.length > 0) { + await cleanupFiles(testFiles); + } + } catch (error) { + if (error instanceof Error && (error as NodeJS.ErrnoException).code !== 'ENOENT') { + console.error('Error during admin content test file cleanup:', error); + } + } + }); + describe('Corrections Routes', () => { it('GET /corrections should return corrections data', async () => { const mockCorrections: SuggestedCorrection[] = [ diff --git a/src/routes/user.routes.test.ts b/src/routes/user.routes.test.ts index a4c3eba3..026adeb9 100644 --- a/src/routes/user.routes.test.ts +++ b/src/routes/user.routes.test.ts @@ -1,7 +1,8 @@ // src/routes/user.routes.test.ts -import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { describe, it, expect, vi, beforeEach, afterAll } from 'vitest'; import supertest from 'supertest'; import express from 'express'; +import path from 'path'; import fs from 'node:fs/promises'; import { createMockUserProfile, @@ -19,6 +20,7 @@ import { Appliance, Notification, DietaryRestriction } from '../types'; import { ForeignKeyConstraintError, NotFoundError, ValidationError } from '../services/db/errors.db'; import { createTestApp } from '../tests/utils/createTestApp'; import { mockLogger } from '../tests/utils/mockLogger'; +import { cleanupFiles } from '../tests/utils/cleanupFiles'; import { logger } from '../services/logger.server'; import { userService } from '../services/userService'; @@ -166,6 +168,26 @@ describe('User Routes (/api/users)', () => { beforeEach(() => { // All tests in this block will use the authenticated app }); + + afterAll(async () => { + // Safeguard to clean up any avatar files created during tests. + const uploadDir = path.resolve(__dirname, '../../../uploads/avatars'); + try { + const allFiles = await fs.readdir(uploadDir); + // Files are named like 'avatar-user-123-timestamp.ext' + const testFiles = allFiles + .filter((f) => f.startsWith(`avatar-${mockUserProfile.user.user_id}`)) + .map((f) => path.join(uploadDir, f)); + + if (testFiles.length > 0) { + await cleanupFiles(testFiles); + } + } catch (error) { + if (error instanceof Error && (error as NodeJS.ErrnoException).code !== 'ENOENT') { + console.error('Error during user routes test file cleanup:', error); + } + } + }); describe('GET /profile', () => { it('should return the full user profile', async () => { vi.mocked(db.userRepo.findUserProfileById).mockResolvedValue(mockUserProfile); diff --git a/src/tests/integration/ai.integration.test.ts b/src/tests/integration/ai.integration.test.ts index 1c612e0f..af4ceacc 100644 --- a/src/tests/integration/ai.integration.test.ts +++ b/src/tests/integration/ai.integration.test.ts @@ -40,17 +40,19 @@ describe('AI API Routes Integration Tests', () => { // 1. Clean up database records await cleanupDb({ userIds: [testUserId] }); - // 2. Clean up any leftover files from the 'image' and 'images' multer instances. - // Most routes clean up after themselves, but this is a safeguard for failed tests. + // 2. Safeguard: Clean up any leftover files from failed tests. + // The routes themselves should clean up on success, but this handles interruptions. const uploadDir = path.resolve(__dirname, '../../../flyer-images'); try { - const files = await fs.readdir(uploadDir); - // Target files created by the 'image' and 'images' multer instances. - const testFileNames = files.filter((f) => f.startsWith('image-') || f.startsWith('images-')); - const testFilePaths = testFileNames.map((f) => path.join(uploadDir, f)); - await cleanupFiles(testFilePaths); + const allFiles = await fs.readdir(uploadDir); + const testFiles = allFiles + .filter((f) => f.startsWith('image-') || f.startsWith('images-')) + .map((f) => path.join(uploadDir, f)); + + if (testFiles.length > 0) { + await cleanupFiles(testFiles); + } } catch (error) { - // If readdir fails (e.g., directory doesn't exist), we can ignore it. if (error instanceof Error && (error as NodeJS.ErrnoException).code !== 'ENOENT') { console.error('Error during AI integration test file cleanup:', error); } diff --git a/src/tests/utils/cleanup.ts b/src/tests/utils/cleanup.ts index ac206ab1..4cb8b302 100644 --- a/src/tests/utils/cleanup.ts +++ b/src/tests/utils/cleanup.ts @@ -43,7 +43,7 @@ export const cleanupDb = async (ids: TestResourceIds) => { await pool.query('DELETE FROM public.user_watched_items WHERE user_id = ANY($1::uuid[])', [userIds]); await pool.query('DELETE FROM public.user_achievements WHERE user_id = ANY($1::uuid[])', [userIds]); await pool.query('DELETE FROM public.activity_log WHERE user_id = ANY($1::uuid[])', [userIds]); - await pool.query('DELETE FROM public.refresh_tokens WHERE user_id = ANY($1::uuid[])', [userIds]); + await pool.query('DELETE FROM public.user_refresh_tokens WHERE user_id = ANY($1::uuid[])', [userIds]); await pool.query('DELETE FROM public.password_reset_tokens WHERE user_id = ANY($1::uuid[])', [userIds]); }