From 3afbf8fb4d39a50d073b15a7cb82e7ed9e2ce147 Mon Sep 17 00:00:00 2001 From: Reece Browne Date: Thu, 21 Aug 2025 11:39:07 +0100 Subject: [PATCH] Address copilot issues --- frontend/src/contexts/file/fileActions.ts | 46 ++++++++++++++++++- .../hooks/tools/shared/useToolResources.ts | 15 ++---- frontend/src/hooks/useIndexedDBThumbnail.ts | 4 +- .../src/services/fileProcessingService.ts | 14 +----- .../services/thumbnailGenerationService.ts | 13 ++++++ frontend/src/utils/thumbnailUtils.ts | 22 +++++---- 6 files changed, 78 insertions(+), 36 deletions(-) diff --git a/frontend/src/contexts/file/fileActions.ts b/frontend/src/contexts/file/fileActions.ts index 3f63753d2..3c272e3af 100644 --- a/frontend/src/contexts/file/fileActions.ts +++ b/frontend/src/contexts/file/fileActions.ts @@ -19,6 +19,40 @@ import { buildQuickKeySet, buildQuickKeySetFromMetadata } from './fileSelectors' const DEBUG = process.env.NODE_ENV === 'development'; +/** + * Simple mutex to prevent race conditions in addFiles + */ +class SimpleMutex { + private locked = false; + private queue: Array<() => void> = []; + + async lock(): Promise { + if (!this.locked) { + this.locked = true; + return Promise.resolve(); + } + + return new Promise((resolve) => { + this.queue.push(() => { + this.locked = true; + resolve(); + }); + }); + } + + unlock(): void { + if (this.queue.length > 0) { + const next = this.queue.shift()!; + next(); + } else { + this.locked = false; + } + } +} + +// Global mutex for addFiles operations +const addFilesMutex = new SimpleMutex(); + /** * Helper to create ProcessedFile metadata structure */ @@ -63,6 +97,10 @@ export async function addFiles( dispatch: React.Dispatch, lifecycleManager: FileLifecycleManager ): Promise> { + // Acquire mutex to prevent race conditions + await addFilesMutex.lock(); + + try { const fileRecords: FileRecord[] = []; const addedFiles: Array<{ file: File; id: FileId; thumbnail?: string }> = []; @@ -99,7 +137,7 @@ export async function addFiles( const result = await generateThumbnailWithMetadata(file); thumbnail = result.thumbnail; pageCount = result.pageCount; - if (DEBUG) console.log(`📄 Generated PDF metadata for ${file.name}: ${pageCount} pages, thumbnail: ${!!thumbnail}`); + if (DEBUG) console.log(`📄 Generated PDF metadata for ${file.name}: ${pageCount} pages, thumbnail: SUCCESS`); } catch (error) { if (DEBUG) console.warn(`📄 Failed to generate PDF metadata for ${file.name}:`, error); } @@ -110,7 +148,7 @@ export async function addFiles( const { generateThumbnailForFile } = await import('../../utils/thumbnailUtils'); thumbnail = await generateThumbnailForFile(file); pageCount = 0; // Non-PDFs have no page count - if (DEBUG) console.log(`📄 Generated simple thumbnail for ${file.name}: no page count, thumbnail: ${!!thumbnail}`); + if (DEBUG) console.log(`📄 Generated simple thumbnail for ${file.name}: no page count, thumbnail: SUCCESS`); } catch (error) { if (DEBUG) console.warn(`📄 Failed to generate simple thumbnail for ${file.name}:`, error); } @@ -255,6 +293,10 @@ export async function addFiles( } return addedFiles; + } finally { + // Always release mutex even if error occurs + addFilesMutex.unlock(); + } } /** diff --git a/frontend/src/hooks/tools/shared/useToolResources.ts b/frontend/src/hooks/tools/shared/useToolResources.ts index 9fd4fe7f1..68e142128 100644 --- a/frontend/src/hooks/tools/shared/useToolResources.ts +++ b/frontend/src/hooks/tools/shared/useToolResources.ts @@ -50,13 +50,8 @@ export const useToolResources = () => { try { console.log(`🖼️ Generating thumbnail for: ${file.name} (${file.type}, ${file.size} bytes)`); const thumbnail = await generateThumbnailForFile(file); - console.log(`🖼️ Generated thumbnail for ${file.name}:`, thumbnail ? 'SUCCESS' : 'FAILED (no thumbnail returned)'); - if (thumbnail) { - thumbnails.push(thumbnail); - } else { - console.warn(`🖼️ No thumbnail returned for ${file.name}`); - thumbnails.push(''); - } + console.log(`🖼️ Generated thumbnail for ${file.name}: SUCCESS`); + thumbnails.push(thumbnail); } catch (error) { console.warn(`🖼️ Failed to generate thumbnail for ${file.name}:`, error); thumbnails.push(''); @@ -74,9 +69,7 @@ export const useToolResources = () => { try { console.log(`🖼️ Generating thumbnail with metadata for: ${file.name} (${file.type}, ${file.size} bytes)`); const result = await generateThumbnailWithMetadata(file); - console.log(`🖼️ Generated thumbnail with metadata for ${file.name}:`, - result.thumbnail ? 'SUCCESS' : 'FAILED (no thumbnail)', - `${result.pageCount} pages`); + console.log(`🖼️ Generated thumbnail with metadata for ${file.name}: SUCCESS, ${result.pageCount} pages`); results.push(result); } catch (error) { console.warn(`🖼️ Failed to generate thumbnail with metadata for ${file.name}:`, error); @@ -84,7 +77,7 @@ export const useToolResources = () => { } } - console.log(`🖼️ useToolResources.generateThumbnailsWithMetadata: Complete. Generated ${results.filter(r => r.thumbnail).length}/${files.length} thumbnails with metadata`); + console.log(`🖼️ useToolResources.generateThumbnailsWithMetadata: Complete. Generated ${results.length}/${files.length} thumbnails with metadata`); return results; }, []); diff --git a/frontend/src/hooks/useIndexedDBThumbnail.ts b/frontend/src/hooks/useIndexedDBThumbnail.ts index edc7d19d2..3e2b6597a 100644 --- a/frontend/src/hooks/useIndexedDBThumbnail.ts +++ b/frontend/src/hooks/useIndexedDBThumbnail.ts @@ -64,10 +64,8 @@ export function useIndexedDBThumbnail(file: FileMetadata | undefined | null): { // Use the universal thumbnail generator const thumbnail = await generateThumbnailForFile(fileObject); - if (!cancelled && thumbnail) { + if (!cancelled) { setThumb(thumbnail); - } else if (!cancelled) { - setThumb(null); } } catch (error) { console.warn('Failed to generate thumbnail for file', file.name, error); diff --git a/frontend/src/services/fileProcessingService.ts b/frontend/src/services/fileProcessingService.ts index 93630a5c6..cb7682c8b 100644 --- a/frontend/src/services/fileProcessingService.ts +++ b/frontend/src/services/fileProcessingService.ts @@ -107,18 +107,8 @@ class FileProcessingService { throw new Error('Processing cancelled'); } } catch (pdfError) { - console.warn(`📁 FileProcessingService: PDF.js failed for ${file.name}, trying fallback:`, pdfError); - - // Fallback to text analysis (reuse same arrayBuffer) - try { - const text = new TextDecoder('latin1').decode(arrayBuffer); - const pageMatches = text.match(/\/Type\s*\/Page[^s]/g); - totalPages = pageMatches ? pageMatches.length : 1; - console.log(`📁 FileProcessingService: Text analysis discovered ${totalPages} pages for ${file.name}`); - } catch (textError) { - console.warn(`📁 FileProcessingService: Text analysis also failed for ${file.name}:`, textError); - totalPages = 1; - } + console.warn(`📁 FileProcessingService: PDF.js failed for ${file.name}, setting pages to 0:`, pdfError); + totalPages = 0; // Unknown page count - UI will hide page count display } } diff --git a/frontend/src/services/thumbnailGenerationService.ts b/frontend/src/services/thumbnailGenerationService.ts index d59a2341e..a1c221269 100644 --- a/frontend/src/services/thumbnailGenerationService.ts +++ b/frontend/src/services/thumbnailGenerationService.ts @@ -117,6 +117,19 @@ export class ThumbnailGenerationService { options: ThumbnailGenerationOptions = {}, onProgress?: (progress: { completed: number; total: number; thumbnails: ThumbnailResult[] }) => void ): Promise { + // Input validation + if (!fileId || typeof fileId !== 'string' || fileId.trim() === '') { + throw new Error('generateThumbnails: fileId must be a non-empty string'); + } + + if (!pdfArrayBuffer || pdfArrayBuffer.byteLength === 0) { + throw new Error('generateThumbnails: pdfArrayBuffer must not be empty'); + } + + if (!pageNumbers || pageNumbers.length === 0) { + throw new Error('generateThumbnails: pageNumbers must not be empty'); + } + const { scale = 0.2, quality = 0.8 diff --git a/frontend/src/utils/thumbnailUtils.ts b/frontend/src/utils/thumbnailUtils.ts index adccead4f..7bf5a2194 100644 --- a/frontend/src/utils/thumbnailUtils.ts +++ b/frontend/src/utils/thumbnailUtils.ts @@ -1,7 +1,7 @@ import { pdfWorkerManager } from '../services/pdfWorkerManager'; export interface ThumbnailWithMetadata { - thumbnail: string | undefined; + thumbnail: string; // Always returns a thumbnail (placeholder if needed) pageCount: number; } @@ -325,9 +325,9 @@ async function generatePDFThumbnail(arrayBuffer: ArrayBuffer, file: File, scale: } /** - * Generate thumbnail for any file type + * Generate thumbnail for any file type - always returns a thumbnail (placeholder if needed) */ -export async function generateThumbnailForFile(file: File): Promise { +export async function generateThumbnailForFile(file: File): Promise { // Skip very large files if (file.size >= 100 * 1024 * 1024) { return generatePlaceholderThumbnail(file); @@ -351,11 +351,17 @@ export async function generateThumbnailForFile(file: File): Promise { // Non-PDF files have no page count