From 1b0100cfad2be169d0b28664dd4b3ea89b391c20 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Deuerling?= Date: Tue, 30 Jun 2026 21:08:02 +0200 Subject: [PATCH 1/3] =?UTF-8?q?=E2=9C=A8=20Surface=20which=20files=20faile?= =?UTF-8?q?d=20during=20photo=20upload?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A partial upload only told the admin "some files failed" with no way to find out which ones — even though the data existed. The backend already returns per-file rejections (response.errors: [{filename, error}]) and the progress hook already exposes failedPhotos, but both were dropped. Add a dismissible failure report to the upload modal listing every file that didn't make it into the gallery, grouped by stage with its reason: - rejected: per-file validation rejections from the upload response (previously discarded entirely) - transfer: whole-chunk request failures (now captured with the error, not just the filename) - processing: background-worker failures from useUploadProgress.failedPhotos Replace the count-only "some files failed" toast with one that points at the list. Add en/de keys under upload.failures.* and a component test covering the rejected + processing rows and dismissal. --- frontend/src/components/admin/PhotoUpload.tsx | 128 ++++++++++++++++-- .../PhotoUpload.failureReport.test.tsx | 107 +++++++++++++++ frontend/src/i18n/locales/de.json | 10 ++ frontend/src/i18n/locales/en.json | 10 ++ 4 files changed, 247 insertions(+), 8 deletions(-) create mode 100644 frontend/src/components/admin/__tests__/PhotoUpload.failureReport.test.tsx diff --git a/frontend/src/components/admin/PhotoUpload.tsx b/frontend/src/components/admin/PhotoUpload.tsx index 2cae80c8..57ff78fd 100644 --- a/frontend/src/components/admin/PhotoUpload.tsx +++ b/frontend/src/components/admin/PhotoUpload.tsx @@ -1,5 +1,5 @@ import React, { useState, useRef, useMemo, useEffect } from 'react'; -import { Upload, X, Image, Loader2, Cog } from 'lucide-react'; +import { Upload, X, Image, Loader2, Cog, AlertTriangle } from 'lucide-react'; import { Button } from '../common'; import { clsx } from 'clsx'; import { api } from '../../config/api'; @@ -28,6 +28,21 @@ type UploadPhase = | { kind: 'transferring'; chunkIndex: number; totalChunks: number; bytePct: number } | { kind: 'processing'; chunkIndex: number; totalChunks: number; filesInChunk: number }; +// Why a file didn't make it into the gallery. Each maps to a distinct +// stage so the user knows whether to re-pick the file (rejected), retry +// the network (transfer), or check the source image (processing). +// - rejected: validation/queueing refused it (bad type, too large, +// corrupt) — returned per-file in the upload response. +// - transfer: the whole chunk request failed (timeout, 5xx, network). +// - processing: stored fine, but the background worker couldn't process +// it (from useUploadProgress's failedPhotos). +type UploadFailureKind = 'rejected' | 'transfer' | 'processing'; +interface UploadFailure { + filename: string; + reason: string; + kind: UploadFailureKind; +} + export const PhotoUpload: React.FC = ({ eventId, onUploadComplete }) => { const { t } = useTranslation(); const [isUploading, setIsUploading] = useState(false); @@ -42,11 +57,28 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl // hook merges status across all of them so the user sees one unified // progress count even when the upload spans multiple HTTP requests. const [uploadIds, setUploadIds] = useState([]); + // Per-file failures surfaced from the transfer stage (chunk POSTs): + // validation rejections (response.errors) and whole-chunk failures. + // Processing failures are merged in from the progress hook below. + const [transferFailures, setTransferFailures] = useState([]); + const [failuresDismissed, setFailuresDismissed] = useState(false); const fileInputRef = useRef(null); const { aggregate: processingAggregate } = useUploadProgress(uploadIds, { enabled: phase.kind === 'processing' && uploadIds.length > 0, }); + + // Single source of truth for the "which files failed" report: transfer + // stage failures (collected during handleUpload) plus processing + // failures (live, from the progress hook). Both carry filename + reason. + const failures = useMemo(() => { + const processing: UploadFailure[] = processingAggregate.failedPhotos.map((p) => ({ + filename: p.filename, + reason: p.error || t('upload.failures.unknownReason', 'Unknown error'), + kind: 'processing', + })); + return [...transferFailures, ...processing]; + }, [transferFailures, processingAggregate.failedPhotos, t]); // Fetch categories for this event const { data: categories = [] } = useQuery({ @@ -162,6 +194,9 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl setIsUploading(true); setUploadProgress(0); setUploadIds([]); + // Clear any prior failure report before this run. + setTransferFailures([]); + setFailuresDismissed(false); // For large uploads, chunk the files by both count AND size to prevent memory/network issues. // #509: the per-chunk byte cap MUST be tunable so users behind Cloudflare Tunnel and other @@ -195,9 +230,10 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl } setTotalChunks(chunks.length); - let totalUploaded = 0; let totalReplaced = 0; - let failedFiles = []; + // Accumulates transfer-stage failures (per-file rejections + whole-chunk + // failures) with their reasons, so the report can name each one. + const collected: UploadFailure[] = []; try { for (let chunkIndex = 0; chunkIndex < chunks.length; chunkIndex++) { @@ -258,8 +294,20 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl }, }); - totalUploaded += (response.data?.successCount || chunk.length); totalReplaced += (response.data?.replacedCount || 0); + // The backend accepts the request (202) but may reject individual + // files (bad type, too large, corrupt) and reports them in + // `errors: [{ filename, error }]`. Surface each one by name. + const rejected = response.data?.errors; + if (Array.isArray(rejected)) { + for (const r of rejected) { + collected.push({ + filename: r?.filename || t('upload.failures.unknownFile', 'Unknown file'), + reason: r?.error || t('upload.failures.unknownReason', 'Unknown error'), + kind: 'rejected', + }); + } + } // Backend returns a per-request upload_id. Track it so the // processing-status hook can poll/stream live progress. if (response.data?.upload_id) { @@ -268,7 +316,13 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl } } catch (error: any) { console.error(`Error uploading chunk ${chunkIndex + 1}:`, error); - failedFiles.push(...chunk.map(f => f.name)); + const reason = + error?.response?.data?.error || + error?.message || + t('upload.failures.transferReason', 'Transfer failed'); + collected.push( + ...chunk.map((f) => ({ filename: f.name, reason, kind: 'transfer' as const })) + ); // Continue with next chunk even if one fails continue; @@ -288,10 +342,15 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl if (totalReplaced > 0) { toast.info(t('upload.replacedFiles', { count: totalReplaced }) || `${totalReplaced} photo(s) replaced`); } - if (failedFiles.length > 0) { + // Publish transfer-stage failures to the report (rendered below with + // each filename + reason). The toast is just the headline; the list + // is where the user finds out *which* files failed. + setTransferFailures(collected); + if (collected.length > 0) { toast.warning( - t('upload.someFilesFailed') || - `Transferred ${totalUploaded} files. ${failedFiles.length} files failed to transfer.` + t('upload.failures.toast', '{{count}} file(s) could not be uploaded — see the list below.', { + count: collected.length, + }) ); } @@ -487,6 +546,59 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl + {/* Failure report — names every file that didn't make it into the + gallery, grouped by failure stage, so the user can act on each. + Persists until dismissed or a new upload starts. */} + {!failuresDismissed && failures.length > 0 && ( +
+
+
+ +

+ {t('upload.failures.title', '{{count}} file(s) could not be uploaded', { + count: failures.length, + })} +

+
+ +
+
    + {failures.map((f, i) => ( +
  • + + {f.kind === 'rejected' && t('upload.failures.kindRejected', 'Rejected')} + {f.kind === 'transfer' && t('upload.failures.kindTransfer', 'Transfer failed')} + {f.kind === 'processing' && t('upload.failures.kindProcessing', 'Processing failed')} + + + + {f.filename} + + — {f.reason} + +
    • + ))} +
+
+ )} + {/* Progress display — two distinct phases. Bytes-on-wire ('transferring') drives the determinate bar; the post-bytes wait ('processing') swaps in an indeterminate spinner with an explanatory hint so users don't diff --git a/frontend/src/components/admin/__tests__/PhotoUpload.failureReport.test.tsx b/frontend/src/components/admin/__tests__/PhotoUpload.failureReport.test.tsx new file mode 100644 index 00000000..b6bd9ce3 --- /dev/null +++ b/frontend/src/components/admin/__tests__/PhotoUpload.failureReport.test.tsx @@ -0,0 +1,107 @@ +/** + * Coverage for the upload failure report — the "which files failed" list. + * + * Before this, a partial upload only showed a count ("some files failed"), + * and the backend's per-file `errors[]` were dropped entirely. These tests + * pin that every failure stage is now named with its reason: + * - rejected: from the upload response's `errors: [{filename, error}]` + * - processing: from useUploadProgress's `failedPhotos` + * and that the report can be dismissed. + */ +import { render, screen, within } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; +import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; +import type { ReactElement } from 'react'; + +import { PhotoUpload } from '../PhotoUpload'; + +vi.mock('react-i18next', async () => { + const actual = await vi.importActual('react-i18next'); + return { + ...actual, + useTranslation: () => ({ + t: (_key: string, fallback?: any) => (typeof fallback === 'string' ? fallback : _key), + }), + }; +}); + +vi.mock('react-toastify', () => ({ + toast: { warning: vi.fn(), info: vi.fn(), error: vi.fn(), success: vi.fn() }, +})); + +// The upload POST: 202 Accepted, one file queued, one rejected per-file. +const postMock = vi.fn().mockResolvedValue({ + data: { + successCount: 1, + upload_id: 'u1', + errors: [{ filename: 'too-big.png', error: 'File too large' }], + }, +}); +vi.mock('../../../config/api', () => ({ api: { post: (...a: any[]) => postMock(...a), get: vi.fn() } })); + +// One photo failed in the background worker. +vi.mock('../../../hooks/useUploadProgress', () => ({ + useUploadProgress: () => ({ + snapshots: {}, + error: null, + aggregate: { + total: 2, pending: 0, processing: 0, complete: 1, failed: 1, + failedPhotos: [{ id: 5, filename: 'corrupt.jpg', error: 'Unsupported format' }], + isComplete: true, isReady: true, + }, + }), +})); + +vi.mock('../../../services/categories.service', () => ({ + categoriesService: { getEventCategories: vi.fn().mockResolvedValue([]) }, +})); +vi.mock('../../../services/settings.service', () => ({ + settingsService: { getAllSettings: vi.fn().mockResolvedValue({}) }, +})); + +const renderWithClient = (ui: ReactElement) => { + const queryClient = new QueryClient({ defaultOptions: { queries: { retry: false } } }); + return render({ui}); +}; + +async function uploadOneFile(container: HTMLElement, user: ReturnType) { + const fileInput = container.querySelector('input[type="file"]') as HTMLInputElement; + await user.upload(fileInput, new File([new Uint8Array([1, 2, 3])], 'too-big.png', { type: 'image/png' })); + await user.click(screen.getByRole('button', { name: /common\.upload/ })); +} + +describe('PhotoUpload failure report', () => { + beforeEach(() => postMock.mockClear()); + afterEach(() => vi.clearAllMocks()); + + it('names each failed file with its reason and failure stage', async () => { + const user = userEvent.setup(); + const { container } = renderWithClient(); + + await uploadOneFile(container, user); + + const report = await screen.findByTestId('upload-failure-report'); + + // Rejected file (from the response errors[] that used to be dropped) + expect(within(report).getByText('too-big.png')).toBeInTheDocument(); + expect(within(report).getByText(/File too large/)).toBeInTheDocument(); + expect(within(report).getByText('Rejected')).toBeInTheDocument(); + + // Processing failure (from useUploadProgress.failedPhotos) + expect(within(report).getByText('corrupt.jpg')).toBeInTheDocument(); + expect(within(report).getByText(/Unsupported format/)).toBeInTheDocument(); + expect(within(report).getByText('Processing failed')).toBeInTheDocument(); + }); + + it('can be dismissed', async () => { + const user = userEvent.setup(); + const { container } = renderWithClient(); + + await uploadOneFile(container, user); + const report = await screen.findByTestId('upload-failure-report'); + + await user.click(within(report).getByRole('button', { name: /Dismiss/i })); + expect(screen.queryByTestId('upload-failure-report')).not.toBeInTheDocument(); + }); +}); diff --git a/frontend/src/i18n/locales/de.json b/frontend/src/i18n/locales/de.json index fabc4e7e..3f00380b 100644 --- a/frontend/src/i18n/locales/de.json +++ b/frontend/src/i18n/locales/de.json @@ -169,6 +169,16 @@ "retryFailed": "Fehlgeschlagene erneut versuchen", "uploadComplete": "Upload abgeschlossen!", "someFilesFailed": "Einige Dateien konnten nicht hochgeladen werden", + "failures": { + "title": "{{count}} Datei(en) konnten nicht hochgeladen werden", + "toast": "{{count}} Datei(en) konnten nicht hochgeladen werden – siehe Liste unten.", + "kindRejected": "Abgelehnt", + "kindTransfer": "Übertragung fehlgeschlagen", + "kindProcessing": "Verarbeitung fehlgeschlagen", + "transferReason": "Übertragung fehlgeschlagen", + "unknownReason": "Unbekannter Fehler", + "unknownFile": "Unbekannte Datei" + }, "replaceByName": "Vorhandene Fotos mit gleichem Namen ersetzen", "uploadPhotos": "Fotos hochladen", "uploadMedia": "Fotos & Videos hochladen", diff --git a/frontend/src/i18n/locales/en.json b/frontend/src/i18n/locales/en.json index 8daea867..e2a5adc2 100644 --- a/frontend/src/i18n/locales/en.json +++ b/frontend/src/i18n/locales/en.json @@ -169,6 +169,16 @@ "retryFailed": "Retry failed", "uploadComplete": "Upload complete!", "someFilesFailed": "Some files failed to upload", + "failures": { + "title": "{{count}} file(s) could not be uploaded", + "toast": "{{count}} file(s) could not be uploaded — see the list below.", + "kindRejected": "Rejected", + "kindTransfer": "Transfer failed", + "kindProcessing": "Processing failed", + "transferReason": "Transfer failed", + "unknownReason": "Unknown error", + "unknownFile": "Unknown file" + }, "replaceByName": "Replace existing photos with same name", "uploadPhotos": "Upload Photos", "uploadMedia": "Upload Photos & Videos", From cc26296c58b45783bff8ed09b2b0d2ca6d6207b2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Deuerling?= Date: Tue, 30 Jun 2026 21:22:49 +0200 Subject: [PATCH 2/3] =?UTF-8?q?=F0=9F=90=9B=20Keep=20upload=20modal=20open?= =?UTF-8?q?=20when=20some=20files=20fail?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The failure report lives inside the upload modal, but the modal auto-closed the instant the transfer finished (handleUploadComplete → onClose), unmounting the report before the user could read it — so the "which files failed" list never actually appeared. Split the modal's completion callback in two: - onUploadComplete: refresh the grid only (no close), as bytes land and again when processing finishes - onUploadSettled({ hasFailures }): fired once the transfer settles; the modal auto-closes only on a clean upload and stays open (report visible) when any file failed Also reset the transfer UI when nothing was queued (every file failed), which previously left the modal spinning forever. Add a PhotoUploadModal test covering close-on-clean vs stay-open-on-failure. --- frontend/src/components/admin/PhotoUpload.tsx | 28 ++++++++++- .../src/components/admin/PhotoUploadModal.tsx | 19 ++++++-- .../admin/__tests__/PhotoUploadModal.test.tsx | 48 +++++++++++++++++++ 3 files changed, 89 insertions(+), 6 deletions(-) create mode 100644 frontend/src/components/admin/__tests__/PhotoUploadModal.test.tsx diff --git a/frontend/src/components/admin/PhotoUpload.tsx b/frontend/src/components/admin/PhotoUpload.tsx index 57ff78fd..f85e98a3 100644 --- a/frontend/src/components/admin/PhotoUpload.tsx +++ b/frontend/src/components/admin/PhotoUpload.tsx @@ -13,7 +13,14 @@ import { useUploadProgress } from '../../hooks/useUploadProgress'; interface PhotoUploadProps { eventId: number; + /** Refresh the photo grid. Called early (as bytes land) and again when + * processing finishes. Never closes the modal. */ onUploadComplete?: () => void; + /** Fired once the transfer stage is done, reporting whether any file + * failed. The host (modal) uses this to decide whether to auto-close: + * a clean upload closes as before; a partial failure keeps the modal + * open so the failure report stays visible. */ + onUploadSettled?: (result: { hasFailures: boolean }) => void; } const DEFAULT_MAX_FILES_PER_UPLOAD = 500; @@ -43,7 +50,7 @@ interface UploadFailure { kind: UploadFailureKind; } -export const PhotoUpload: React.FC = ({ eventId, onUploadComplete }) => { +export const PhotoUpload: React.FC = ({ eventId, onUploadComplete, onUploadSettled }) => { const { t } = useTranslation(); const [isUploading, setIsUploading] = useState(false); const [selectedFiles, setSelectedFiles] = useState([]); @@ -234,6 +241,8 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl // Accumulates transfer-stage failures (per-file rejections + whole-chunk // failures) with their reasons, so the report can name each one. const collected: UploadFailure[] = []; + // Whether at least one chunk was accepted for background processing. + let anyQueued = false; try { for (let chunkIndex = 0; chunkIndex < chunks.length; chunkIndex++) { @@ -311,6 +320,7 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl // Backend returns a per-request upload_id. Track it so the // processing-status hook can poll/stream live progress. if (response.data?.upload_id) { + anyQueued = true; const newId = response.data.upload_id as string; setUploadIds((prev) => (prev.includes(newId) ? prev : [...prev, newId])); } @@ -361,6 +371,22 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl onUploadComplete(); } + // Tell the host the transfer stage settled. A clean upload lets the + // modal auto-close (unchanged behaviour); a partial failure keeps it + // open so the failure report below stays visible. + onUploadSettled?.({ hasFailures: collected.length > 0 }); + + // Nothing was queued for background processing (every chunk failed, + // or a pre-async backend) — there's no processing phase to wait on, + // so reset the transfer UI here. The failure report persists. + if (!anyQueued) { + setIsUploading(false); + setUploadProgress(0); + setCurrentChunk(0); + setTotalChunks(0); + setPhase({ kind: 'idle' }); + } + // If the backend never returned an upload_id (e.g. only failures // or pre-async-backend deployment), we have nothing to wait for — // fall through to the finally cleanup which resets state. diff --git a/frontend/src/components/admin/PhotoUploadModal.tsx b/frontend/src/components/admin/PhotoUploadModal.tsx index 41c3207f..59db4fe2 100644 --- a/frontend/src/components/admin/PhotoUploadModal.tsx +++ b/frontend/src/components/admin/PhotoUploadModal.tsx @@ -21,11 +21,19 @@ export const PhotoUploadModal: React.FC = ({ if (!isOpen) return null; + // Refresh the host's grid, but do NOT close here — closing is decided by + // onUploadSettled so a partial-failure upload keeps the modal (and its + // failure report) open. const handleUploadComplete = () => { - if (onUploadComplete) { - onUploadComplete(); + onUploadComplete?.(); + }; + + // Auto-close only on a clean upload; keep the modal open when some files + // failed so the report stays visible until the user dismisses it. + const handleUploadSettled = ({ hasFailures }: { hasFailures: boolean }) => { + if (!hasFailures) { + onClose(); } - onClose(); }; return ( @@ -46,9 +54,10 @@ export const PhotoUploadModal: React.FC = ({ {/* Scrollable Content */}
-
diff --git a/frontend/src/components/admin/__tests__/PhotoUploadModal.test.tsx b/frontend/src/components/admin/__tests__/PhotoUploadModal.test.tsx new file mode 100644 index 00000000..a75872ec --- /dev/null +++ b/frontend/src/components/admin/__tests__/PhotoUploadModal.test.tsx @@ -0,0 +1,48 @@ +/** + * The upload modal must keep itself open when an upload partially fails, so + * the failure report stays visible; a clean upload still auto-closes. We stub + * PhotoUpload with buttons that fire its onUploadSettled callback both ways. + */ +import { render, screen } from '@testing-library/react'; +import userEvent from '@testing-library/user-event'; +import { describe, expect, it, vi } from 'vitest'; + +import { PhotoUploadModal } from '../PhotoUploadModal'; + +vi.mock('react-i18next', async () => { + const actual = await vi.importActual('react-i18next'); + return { + ...actual, + useTranslation: () => ({ t: (_k: string, fb?: any) => (typeof fb === 'string' ? fb : _k) }), + }; +}); + +// Stub PhotoUpload: expose buttons that settle clean vs. with failures. +vi.mock('../PhotoUpload', () => ({ + PhotoUpload: ({ onUploadSettled }: any) => ( +
+ + +
+ ), +})); + +describe('PhotoUploadModal auto-close behaviour', () => { + it('closes after a clean upload', async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + render(); + + await user.click(screen.getByText('settle-clean')); + expect(onClose).toHaveBeenCalledTimes(1); + }); + + it('stays open when some files failed', async () => { + const user = userEvent.setup(); + const onClose = vi.fn(); + render(); + + await user.click(screen.getByText('settle-failed')); + expect(onClose).not.toHaveBeenCalled(); + }); +}); From 1be871cb9a2a9f13b8fafc377b454e3df673aa08 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Andr=C3=A9=20Deuerling?= Date: Wed, 1 Jul 2026 08:03:01 +0200 Subject: [PATCH 3/3] =?UTF-8?q?=F0=9F=90=9B=20Address=20review:=20fix=20sp?= =?UTF-8?q?inner=20hang=20+=20show=20processing=20failures?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Blocker: the "every file rejected" reset never fired because the backend returns `upload_id` unconditionally (with count 0), so `anyQueued` was always true and the completion effect (gated on total > 0) never ran — modal spun forever. Gate `anyQueued` on `count > 0` so a zero-photo response takes the terminal reset path. Concern 1: processing-stage failures were invisible — the modal auto-closed on clean transfer before the worker reported them. Defer the settle/close decision to the completion effect (combining transfer + processing failures), and persist failed photos into `processingFailures` state before `uploadIds` is cleared, so the rows don't vanish the instant they appear. Also: report card gets role="status"/aria-live (nit), and tests now cover the whole-chunk transfer failure, the clean-settle path, and the onUploadSettled contract from the real component. --- frontend/src/components/admin/PhotoUpload.tsx | 67 +++++++---- .../PhotoUpload.failureReport.test.tsx | 107 +++++++++++++----- 2 files changed, 118 insertions(+), 56 deletions(-) diff --git a/frontend/src/components/admin/PhotoUpload.tsx b/frontend/src/components/admin/PhotoUpload.tsx index f85e98a3..7d3f0ea1 100644 --- a/frontend/src/components/admin/PhotoUpload.tsx +++ b/frontend/src/components/admin/PhotoUpload.tsx @@ -68,6 +68,11 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl // validation rejections (response.errors) and whole-chunk failures. // Processing failures are merged in from the progress hook below. const [transferFailures, setTransferFailures] = useState([]); + // Processing failures are captured into state (not read live) because the + // completion effect clears uploadIds, which empties the progress hook's + // failedPhotos — reading live would make the rows vanish the instant they + // appear. + const [processingFailures, setProcessingFailures] = useState([]); const [failuresDismissed, setFailuresDismissed] = useState(false); const fileInputRef = useRef(null); @@ -76,16 +81,12 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl }); // Single source of truth for the "which files failed" report: transfer - // stage failures (collected during handleUpload) plus processing - // failures (live, from the progress hook). Both carry filename + reason. - const failures = useMemo(() => { - const processing: UploadFailure[] = processingAggregate.failedPhotos.map((p) => ({ - filename: p.filename, - reason: p.error || t('upload.failures.unknownReason', 'Unknown error'), - kind: 'processing', - })); - return [...transferFailures, ...processing]; - }, [transferFailures, processingAggregate.failedPhotos, t]); + // stage failures (collected during handleUpload) plus processing failures + // (captured on completion). Both carry filename + reason. + const failures = useMemo( + () => [...transferFailures, ...processingFailures], + [transferFailures, processingFailures] + ); // Fetch categories for this event const { data: categories = [] } = useQuery({ @@ -203,6 +204,7 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl setUploadIds([]); // Clear any prior failure report before this run. setTransferFailures([]); + setProcessingFailures([]); setFailuresDismissed(false); // For large uploads, chunk the files by both count AND size to prevent memory/network issues. @@ -317,9 +319,12 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl }); } } - // Backend returns a per-request upload_id. Track it so the - // processing-status hook can poll/stream live progress. - if (response.data?.upload_id) { + // Track the per-request upload_id so the processing hook can poll + // for live progress — but only when photos were actually queued + // (count > 0). The backend returns an upload_id even when every + // file was rejected (count 0); tracking it there would make us wait + // for a processing phase that never starts, hanging the spinner. + if (response.data?.upload_id && (response.data?.count ?? 0) > 0) { anyQueued = true; const newId = response.data.upload_id as string; setUploadIds((prev) => (prev.includes(newId) ? prev : [...prev, newId])); @@ -371,25 +376,20 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl onUploadComplete(); } - // Tell the host the transfer stage settled. A clean upload lets the - // modal auto-close (unchanged behaviour); a partial failure keeps it - // open so the failure report below stays visible. - onUploadSettled?.({ hasFailures: collected.length > 0 }); - - // Nothing was queued for background processing (every chunk failed, - // or a pre-async backend) — there's no processing phase to wait on, - // so reset the transfer UI here. The failure report persists. + // Settling (and the modal's close decision) is deferred until we know + // the WHOLE outcome, including background processing. If nothing was + // queued (every file rejected, or a pre-async backend), the transfer + // stage is already terminal — settle now and reset the UI. Otherwise + // the processing effect below settles once the worker finishes, so + // processing failures are included before the modal decides to close. if (!anyQueued) { + onUploadSettled?.({ hasFailures: collected.length > 0 }); setIsUploading(false); setUploadProgress(0); setCurrentChunk(0); setTotalChunks(0); setPhase({ kind: 'idle' }); } - - // If the backend never returned an upload_id (e.g. only failures - // or pre-async-backend deployment), we have nothing to wait for — - // fall through to the finally cleanup which resets state. } catch (error: any) { console.error('Upload error:', error); toast.error(error.response?.data?.error || t('toast.uploadError')); @@ -410,6 +410,15 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl if (!processingAggregate.isComplete) return; if (processingAggregate.failed > 0) { + // Persist the failed photos into the report before uploadIds is cleared + // below (which would otherwise empty the progress hook's failedPhotos). + setProcessingFailures( + processingAggregate.failedPhotos.map((p) => ({ + filename: p.filename, + reason: p.error || t('upload.failures.unknownReason', 'Unknown error'), + kind: 'processing' as const, + })) + ); toast.warning( t('upload.processingFailed', { count: processingAggregate.failed }) || `${processingAggregate.failed} photo(s) failed to process` @@ -421,6 +430,12 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl } if (onUploadComplete) onUploadComplete(); + // Now the whole outcome is known — settle. The modal auto-closes only + // when nothing failed at either stage; any transfer OR processing + // failure keeps it open so the report (which lists both) stays visible. + onUploadSettled?.({ + hasFailures: transferFailures.length > 0 || processingAggregate.failed > 0, + }); setIsUploading(false); setUploadProgress(0); setCurrentChunk(0); @@ -578,6 +593,8 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl {!failuresDismissed && failures.length > 0 && (
diff --git a/frontend/src/components/admin/__tests__/PhotoUpload.failureReport.test.tsx b/frontend/src/components/admin/__tests__/PhotoUpload.failureReport.test.tsx index b6bd9ce3..0896b971 100644 --- a/frontend/src/components/admin/__tests__/PhotoUpload.failureReport.test.tsx +++ b/frontend/src/components/admin/__tests__/PhotoUpload.failureReport.test.tsx @@ -3,12 +3,14 @@ * * Before this, a partial upload only showed a count ("some files failed"), * and the backend's per-file `errors[]` were dropped entirely. These tests - * pin that every failure stage is now named with its reason: + * pin that every failure stage is named with its reason: * - rejected: from the upload response's `errors: [{filename, error}]` + * - transfer: from a whole-chunk POST failure (the `catch` path) * - processing: from useUploadProgress's `failedPhotos` - * and that the report can be dismissed. + * plus the settle contract (hasFailures true/false) the modal relies on to + * decide whether to auto-close, and dismissal. */ -import { render, screen, within } from '@testing-library/react'; +import { render, screen, waitFor, within } from '@testing-library/react'; import userEvent from '@testing-library/user-event'; import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { QueryClient, QueryClientProvider } from '@tanstack/react-query'; @@ -30,26 +32,26 @@ vi.mock('react-toastify', () => ({ toast: { warning: vi.fn(), info: vi.fn(), error: vi.fn(), success: vi.fn() }, })); -// The upload POST: 202 Accepted, one file queued, one rejected per-file. -const postMock = vi.fn().mockResolvedValue({ - data: { - successCount: 1, - upload_id: 'u1', - errors: [{ filename: 'too-big.png', error: 'File too large' }], - }, -}); +const postMock = vi.fn(); vi.mock('../../../config/api', () => ({ api: { post: (...a: any[]) => postMock(...a), get: vi.fn() } })); -// One photo failed in the background worker. +// Mutable processing aggregate, swapped per test. Returned only once photos +// are queued (uploadIds non-empty), mirroring the real hook flipping from +// "nothing to track" to "complete" — the transition that fires the settle +// effect. +const clean = () => ({ + total: 0, pending: 0, processing: 0, complete: 0, failed: 0, + failedPhotos: [] as { id: number; filename: string; error: string | null }[], + isComplete: false, isReady: true, +}); +const hoisted = vi.hoisted(() => ({ aggregate: null as any })); vi.mock('../../../hooks/useUploadProgress', () => ({ - useUploadProgress: () => ({ + useUploadProgress: (ids: string[]) => ({ snapshots: {}, error: null, - aggregate: { - total: 2, pending: 0, processing: 0, complete: 1, failed: 1, - failedPhotos: [{ id: 5, filename: 'corrupt.jpg', error: 'Unsupported format' }], - isComplete: true, isReady: true, - }, + aggregate: ids && ids.length > 0 + ? hoisted.aggregate + : { total: 0, pending: 0, processing: 0, complete: 0, failed: 0, failedPhotos: [], isComplete: false, isReady: true }, }), })); @@ -65,40 +67,83 @@ const renderWithClient = (ui: ReactElement) => { return render({ui}); }; -async function uploadOneFile(container: HTMLElement, user: ReturnType) { +async function uploadFile(container: HTMLElement, user: ReturnType) { const fileInput = container.querySelector('input[type="file"]') as HTMLInputElement; - await user.upload(fileInput, new File([new Uint8Array([1, 2, 3])], 'too-big.png', { type: 'image/png' })); + await user.upload(fileInput, new File([new Uint8Array([1, 2, 3])], 'good-photo.png', { type: 'image/png' })); await user.click(screen.getByRole('button', { name: /common\.upload/ })); } describe('PhotoUpload failure report', () => { - beforeEach(() => postMock.mockClear()); + beforeEach(() => { + postMock.mockReset(); + hoisted.aggregate = clean(); + }); afterEach(() => vi.clearAllMocks()); - it('names each failed file with its reason and failure stage', async () => { + it('names rejected + processing failures and settles with hasFailures', async () => { + postMock.mockResolvedValue({ + data: { successCount: 1, count: 1, upload_id: 'u1', errors: [{ filename: 'too-big.png', error: 'File too large' }] }, + }); + hoisted.aggregate = { + total: 2, pending: 0, processing: 0, complete: 1, failed: 1, + failedPhotos: [{ id: 5, filename: 'corrupt.jpg', error: 'Unsupported format' }], + isComplete: true, isReady: true, + }; + const onUploadSettled = vi.fn(); const user = userEvent.setup(); - const { container } = renderWithClient(); + const { container } = renderWithClient(); - await uploadOneFile(container, user); + await uploadFile(container, user); const report = await screen.findByTestId('upload-failure-report'); - - // Rejected file (from the response errors[] that used to be dropped) expect(within(report).getByText('too-big.png')).toBeInTheDocument(); expect(within(report).getByText(/File too large/)).toBeInTheDocument(); expect(within(report).getByText('Rejected')).toBeInTheDocument(); - - // Processing failure (from useUploadProgress.failedPhotos) expect(within(report).getByText('corrupt.jpg')).toBeInTheDocument(); - expect(within(report).getByText(/Unsupported format/)).toBeInTheDocument(); expect(within(report).getByText('Processing failed')).toBeInTheDocument(); + + // Contract with the modal: the real component fires onUploadSettled and, + // because something failed, asks the host NOT to auto-close. + await waitFor(() => expect(onUploadSettled).toHaveBeenCalledWith({ hasFailures: true })); }); - it('can be dismissed', async () => { + it('reports a whole-chunk transfer failure by name', async () => { + postMock.mockRejectedValue({ response: { data: { error: 'Network error' } } }); const user = userEvent.setup(); const { container } = renderWithClient(); - await uploadOneFile(container, user); + await uploadFile(container, user); + + const report = await screen.findByTestId('upload-failure-report'); + expect(within(report).getByText('good-photo.png')).toBeInTheDocument(); + expect(within(report).getByText(/Network error/)).toBeInTheDocument(); + expect(within(report).getByText('Transfer failed')).toBeInTheDocument(); + }); + + it('settles clean when nothing fails, so the modal can auto-close', async () => { + postMock.mockResolvedValue({ data: { successCount: 1, count: 1, upload_id: 'u1', errors: [] } }); + hoisted.aggregate = { + total: 1, pending: 0, processing: 0, complete: 1, failed: 0, + failedPhotos: [], isComplete: true, isReady: true, + }; + const onUploadSettled = vi.fn(); + const user = userEvent.setup(); + const { container } = renderWithClient(); + + await uploadFile(container, user); + + await waitFor(() => expect(onUploadSettled).toHaveBeenCalledWith({ hasFailures: false })); + expect(screen.queryByTestId('upload-failure-report')).not.toBeInTheDocument(); + }); + + it('can be dismissed', async () => { + postMock.mockResolvedValue({ + data: { successCount: 0, count: 0, errors: [{ filename: 'too-big.png', error: 'File too large' }] }, + }); + const user = userEvent.setup(); + const { container } = renderWithClient(); + + await uploadFile(container, user); const report = await screen.findByTestId('upload-failure-report'); await user.click(within(report).getByRole('button', { name: /Dismiss/i }));