Merge pull request #708 from andredlng/feat/upload-failure-details

Surface which files failed during photo upload

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';
@@ -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;
@@ -28,7 +35,22 @@ type UploadPhase =
   | { kind: 'transferring'; chunkIndex: number; totalChunks: number; bytePct: number }
   | { kind: 'processing'; chunkIndex: number; totalChunks: number; filesInChunk: number };
 
-export const PhotoUpload: React.FC<PhotoUploadProps> = ({ eventId, onUploadComplete }) => {
+// 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<PhotoUploadProps> = ({ eventId, onUploadComplete, onUploadSettled }) => {
   const { t } = useTranslation();
   const [isUploading, setIsUploading] = useState(false);
   const [selectedFiles, setSelectedFiles] = useState<File[]>([]);
@@ -42,11 +64,29 @@ export const PhotoUpload: React.FC<PhotoUploadProps> = ({ 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<string[]>([]);
+  // 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<UploadFailure[]>([]);
+  // 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<UploadFailure[]>([]);
+  const [failuresDismissed, setFailuresDismissed] = useState(false);
   const fileInputRef = useRef<HTMLInputElement>(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
+  // (captured on completion). Both carry filename + reason.
+  const failures = useMemo<UploadFailure[]>(
+    () => [...transferFailures, ...processingFailures],
+    [transferFailures, processingFailures]
+  );
   
   // Fetch categories for this event
   const { data: categories = [] } = useQuery({
@@ -162,6 +202,10 @@ export const PhotoUpload: React.FC<PhotoUploadProps> = ({ eventId, onUploadCompl
     setIsUploading(true);
     setUploadProgress(0);
     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.
     // #509: the per-chunk byte cap MUST be tunable so users behind Cloudflare Tunnel and other
@@ -195,9 +239,12 @@ export const PhotoUpload: React.FC<PhotoUploadProps> = ({ 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[] = [];
+    // Whether at least one chunk was accepted for background processing.
+    let anyQueued = false;
 
     try {
       for (let chunkIndex = 0; chunkIndex < chunks.length; chunkIndex++) {
@@ -258,17 +305,39 @@ export const PhotoUpload: React.FC<PhotoUploadProps> = ({ eventId, onUploadCompl
             },
           });
 
-          totalUploaded += (response.data?.successCount || chunk.length);
           totalReplaced += (response.data?.replacedCount || 0);
-          // Backend returns a per-request upload_id. Track it so the
-          // processing-status hook can poll/stream live progress.
-          if (response.data?.upload_id) {
+          // 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',
+              });
+            }
+          }
+          // 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]));
           }
         } 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 +357,15 @@ export const PhotoUpload: React.FC<PhotoUploadProps> = ({ 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,
+          })
         );
       }
 
@@ -302,9 +376,20 @@ export const PhotoUpload: React.FC<PhotoUploadProps> = ({ eventId, onUploadCompl
         onUploadComplete();
       }
 
-      // 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.
+      // 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' });
+      }
     } catch (error: any) {
       console.error('Upload error:', error);
       toast.error(error.response?.data?.error || t('toast.uploadError'));
@@ -325,6 +410,15 @@ export const PhotoUpload: React.FC<PhotoUploadProps> = ({ 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`
@@ -336,6 +430,12 @@ export const PhotoUpload: React.FC<PhotoUploadProps> = ({ 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);
@@ -487,6 +587,61 @@ export const PhotoUpload: React.FC<PhotoUploadProps> = ({ eventId, onUploadCompl
         </Button>
       </div>
 
+      {/* 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 && (
+        <div
+          data-testid="upload-failure-report"
+          role="status"
+          aria-live="polite"
+          className="rounded-lg border border-amber-300 dark:border-amber-700/60 bg-amber-50 dark:bg-amber-900/20 p-4"
+        >
+          <div className="flex items-start justify-between gap-3">
+            <div className="flex items-center gap-2 text-amber-800 dark:text-amber-300">
+              <AlertTriangle className="w-5 h-5 flex-shrink-0" />
+              <p className="text-sm font-medium">
+                {t('upload.failures.title', '{{count}} file(s) could not be uploaded', {
+                  count: failures.length,
+                })}
+              </p>
+            </div>
+            <button
+              type="button"
+              onClick={() => setFailuresDismissed(true)}
+              aria-label={t('common.dismiss', 'Dismiss')}
+              className="p-1 -m-1 text-amber-700 dark:text-amber-300 hover:bg-amber-100 dark:hover:bg-amber-800/40 rounded"
+            >
+              <X className="w-4 h-4" />
+            </button>
+          </div>
+          <ul className="mt-3 max-h-48 overflow-y-auto space-y-1.5">
+            {failures.map((f, i) => (
+              <li key={`${f.kind}-${f.filename}-${i}`} className="flex items-start gap-2 text-xs">
+                <span
+                  className={clsx(
+                    'flex-shrink-0 mt-0.5 px-1.5 py-0.5 rounded font-medium whitespace-nowrap',
+                    f.kind === 'rejected' && 'bg-red-100 text-red-700 dark:bg-red-900/40 dark:text-red-300',
+                    f.kind === 'transfer' && 'bg-orange-100 text-orange-700 dark:bg-orange-900/40 dark:text-orange-300',
+                    f.kind === 'processing' && 'bg-purple-100 text-purple-700 dark:bg-purple-900/40 dark:text-purple-300'
+                  )}
+                >
+                  {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')}
+                </span>
+                <span className="min-w-0">
+                  <span className="font-medium text-neutral-800 dark:text-neutral-200 break-all">
+                    {f.filename}
+                  </span>
+                  <span className="text-neutral-500 dark:text-neutral-400"> — {f.reason}</span>
+                </span>
+              </li>
+            ))}
+          </ul>
+        </div>
+      )}
+
       {/* 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

frontend/src/components/admin/PhotoUploadModal.tsx

@@ -21,11 +21,19 @@ export const PhotoUploadModal: React.FC<PhotoUploadModalProps> = ({
 
   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<PhotoUploadModalProps> = ({
 
         {/* Scrollable Content */}
         <div className="flex-1 overflow-y-auto p-6">
-          <PhotoUpload 
-            eventId={eventId} 
+          <PhotoUpload
+            eventId={eventId}
             onUploadComplete={handleUploadComplete}
+            onUploadSettled={handleUploadSettled}
           />
         </div>
       </div>

frontend/src/components/admin/__tests__/PhotoUpload.failureReport.test.tsx

@@ -0,0 +1,152 @@
+/**
+ * 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 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`
+ * plus the settle contract (hasFailures true/false) the modal relies on to
+ * decide whether to auto-close, and dismissal.
+ */
+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';
+import type { ReactElement } from 'react';
+
+import { PhotoUpload } from '../PhotoUpload';
+
+vi.mock('react-i18next', async () => {
+  const actual = await vi.importActual<typeof import('react-i18next')>('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() },
+}));
+
+const postMock = vi.fn();
+vi.mock('../../../config/api', () => ({ api: { post: (...a: any[]) => postMock(...a), get: vi.fn() } }));
+
+// 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: (ids: string[]) => ({
+    snapshots: {},
+    error: null,
+    aggregate: ids && ids.length > 0
+      ? hoisted.aggregate
+      : { total: 0, pending: 0, processing: 0, complete: 0, failed: 0, failedPhotos: [], isComplete: false, 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(<QueryClientProvider client={queryClient}>{ui}</QueryClientProvider>);
+};
+
+async function uploadFile(container: HTMLElement, user: ReturnType<typeof userEvent.setup>) {
+  const fileInput = container.querySelector('input[type="file"]') as HTMLInputElement;
+  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.mockReset();
+    hoisted.aggregate = clean();
+  });
+  afterEach(() => vi.clearAllMocks());
+
+  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(<PhotoUpload eventId={1} onUploadSettled={onUploadSettled} />);
+
+    await uploadFile(container, user);
+
+    const report = await screen.findByTestId('upload-failure-report');
+    expect(within(report).getByText('too-big.png')).toBeInTheDocument();
+    expect(within(report).getByText(/File too large/)).toBeInTheDocument();
+    expect(within(report).getByText('Rejected')).toBeInTheDocument();
+    expect(within(report).getByText('corrupt.jpg')).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('reports a whole-chunk transfer failure by name', async () => {
+    postMock.mockRejectedValue({ response: { data: { error: 'Network error' } } });
+    const user = userEvent.setup();
+    const { container } = renderWithClient(<PhotoUpload eventId={1} />);
+
+    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(<PhotoUpload eventId={1} onUploadSettled={onUploadSettled} />);
+
+    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(<PhotoUpload eventId={1} />);
+
+    await uploadFile(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();
+  });
+});

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<typeof import('react-i18next')>('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) => (
+    <div>
+      <button onClick={() => onUploadSettled?.({ hasFailures: false })}>settle-clean</button>
+      <button onClick={() => onUploadSettled?.({ hasFailures: true })}>settle-failed</button>
+    </div>
+  ),
+}));
+
+describe('PhotoUploadModal auto-close behaviour', () => {
+  it('closes after a clean upload', async () => {
+    const user = userEvent.setup();
+    const onClose = vi.fn();
+    render(<PhotoUploadModal isOpen eventId={1} onClose={onClose} />);
+
+    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(<PhotoUploadModal isOpen eventId={1} onClose={onClose} />);
+
+    await user.click(screen.getByText('settle-failed'));
+    expect(onClose).not.toHaveBeenCalled();
+  });
+});