From 86dfcc4f116a394e7e092ab3e01f3f1d030bb367 Mon Sep 17 00:00:00 2001 From: Paul Nothaft Date: Sat, 2 May 2026 00:15:20 +0200 Subject: [PATCH] feat(upload): two-state UI + temp dir cleanup (PR-A of async processing) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 of the upload-progress redesign. Two changes that ship UX wins without any architectural surgery — they're a stepping stone for the full async-processing rework that follows in subsequent commits. 1. Two-state progress bar (PhotoUpload.tsx, UserPhotoUpload.tsx) When axios.onUploadProgress reports loaded === total, the request is on the server and the bytes have left the browser. Today the bar sits at 100% for the chunk while the backend runs sharp/ffmpeg/EXIF (often minutes on NFS-backed storage) and users assume the upload froze. The component now distinguishes two phases: - 'transferring' — bytes-on-wire, determinate progress bar. - 'processing' — bytes done, waiting for response. Indeterminate spinner + an explanatory hint that the backend is generating thumbnails / reading metadata and the user can leave the page. Same pattern in UserPhotoUpload (gallery): the per-file checkmark icon is replaced by a Loader2 spinner while the request is in flight after bytes-on-wire finished. 2. Temp directory cleanup (adminPhotos.js) Multer creates temp/upload__/ per request. Files inside it are individually unlinked after they're moved to storage on the success path, but the empty directory was never removed. On error paths three different inline blocks each tried to clean up; the success path was missed entirely. Result: the orphan-empty-dirs accumulation reported in the issue (70+ on the affected instance). Replace the inline cleanup blocks with a single idempotent cleanupTempDir() registered on res.finish + res.close, so it fires exactly once on every exit path (validation 4xx, server 5xx, multer error, success). New translation keys (en/de): upload.transferring, upload.processing, upload.processingHint, upload.processingProgress, upload.processingFailed, upload.retryFailed. --- backend/src/routes/adminPhotos.js | 59 ++++------ frontend/src/components/admin/PhotoUpload.tsx | 109 ++++++++++++++---- .../components/gallery/UserPhotoUpload.tsx | 23 +++- frontend/src/i18n/locales/de.json | 6 + frontend/src/i18n/locales/en.json | 6 + 5 files changed, 141 insertions(+), 62 deletions(-) diff --git a/backend/src/routes/adminPhotos.js b/backend/src/routes/adminPhotos.js index 46ec4661..1a577260 100644 --- a/backend/src/routes/adminPhotos.js +++ b/backend/src/routes/adminPhotos.js @@ -150,29 +150,39 @@ router.post('/:eventId/upload', adminAuth, requirePermission('photos.upload'), r next(); }); }, validateUploadContent, validateUploadedFiles, async (req, res) => { + // Single cleanup site for the multer temp directory — runs on every + // exit path (success, validation 4xx, server 5xx, multer error). The + // previous code had three inline cleanup blocks for individual early + // returns and missed the success path entirely, leaving an empty + // per-request directory behind on every successful upload (#357 review). + let tempCleanupDone = false; + const cleanupTempDir = async () => { + if (tempCleanupDone || !req.tempUploadPath) return; + tempCleanupDone = true; + try { + await fs.rm(req.tempUploadPath, { recursive: true, force: true }); + } catch (e) { + console.error('Failed to clean up temp upload directory:', e); + } + }; + res.on('finish', cleanupTempDir); + res.on('close', cleanupTempDir); + try { const { eventId } = req.params; const { category_id, replace_by_name } = req.body; const replaceByName = replace_by_name === 'true' || replace_by_name === true; - + console.log('Upload request received for event:', eventId); console.log('Body:', req.body); console.log('Files:', req.files ? req.files.length : 'none'); console.log('File details:', req.files?.map(f => ({ name: f.originalname, size: f.size, mimetype: f.mimetype }))); console.log('Category ID received:', category_id); - + // Verify event exists and admin has access const event = await db('events').where({ id: eventId }).first(); if (!event) { console.error('Event not found:', eventId); - // Clean up temp files - if (req.tempUploadPath) { - try { - await fs.rm(req.tempUploadPath, { recursive: true, force: true }); - } catch (e) { - console.error('Failed to clean up temp path:', e); - } - } return res.status(404).json({ error: 'Event not found' }); } @@ -192,14 +202,6 @@ router.post('/:eventId/upload', adminAuth, requirePermission('photos.upload'), r } } if (currentCount + newFilesCount > event.photo_cap) { - // Clean up temp files - if (req.tempUploadPath) { - try { - await fs.rm(req.tempUploadPath, { recursive: true, force: true }); - } catch (e) { - console.error('Failed to clean up temp path:', e); - } - } return res.status(400).json({ error: `Photo cap exceeded. This event allows a maximum of ${event.photo_cap} photos. Currently ${currentCount} photos exist, and you are trying to upload ${newFilesCount} more.` }); @@ -209,14 +211,6 @@ router.post('/:eventId/upload', adminAuth, requirePermission('photos.upload'), r if (!req.files || req.files.length === 0) { console.error('No files in request. req.files:', req.files); console.error('Request body keys:', Object.keys(req.body)); - // Clean up temp files - if (req.tempUploadPath) { - try { - await fs.rm(req.tempUploadPath, { recursive: true, force: true }); - } catch (e) { - console.error('Failed to clean up temp path:', e); - } - } return res.status(400).json({ error: 'No files uploaded' }); } @@ -597,17 +591,8 @@ router.post('/:eventId/upload', adminAuth, requirePermission('photos.upload'), r res.json(response); } catch (error) { console.error('Error uploading photos:', error); - - // Clean up temp upload directory on error - if (req.tempUploadPath) { - try { - await fs.rm(req.tempUploadPath, { recursive: true, force: true }); - console.log(`Cleaned up temp upload directory after error: ${req.tempUploadPath}`); - } catch (e) { - console.error('Failed to clean up temp upload directory:', e); - } - } - + // Temp directory cleanup is handled by the response finish/close + // listeners above, regardless of which exit path fires. res.status(500).json({ error: 'Failed to upload photos' }); } }); diff --git a/frontend/src/components/admin/PhotoUpload.tsx b/frontend/src/components/admin/PhotoUpload.tsx index 326d8a2c..2d664cf6 100644 --- a/frontend/src/components/admin/PhotoUpload.tsx +++ b/frontend/src/components/admin/PhotoUpload.tsx @@ -1,5 +1,5 @@ import React, { useState, useRef, useMemo } from 'react'; -import { Upload, X, Image, Loader2 } from 'lucide-react'; +import { Upload, X, Image, Loader2, Cog } from 'lucide-react'; import { Button } from '../common'; import { clsx } from 'clsx'; import { api } from '../../config/api'; @@ -18,6 +18,15 @@ interface PhotoUploadProps { const DEFAULT_MAX_FILES_PER_UPLOAD = 500; const MAX_FILES_PER_UPLOAD_LIMIT = 2000; +// Upload phase machine. The user perceives "frozen" during 'processing' +// because the bytes are already on the server and we're waiting for +// thumbnail/EXIF/etc. work — the explicit phase + hint message kills +// that perception (#352 / contributor analysis on issue 357 review). +type UploadPhase = + | { kind: 'idle' } + | { kind: 'transferring'; chunkIndex: number; totalChunks: number; bytePct: number } + | { kind: 'processing'; chunkIndex: number; totalChunks: number; filesInChunk: number }; + export const PhotoUpload: React.FC = ({ eventId, onUploadComplete }) => { const { t } = useTranslation(); const [isUploading, setIsUploading] = useState(false); @@ -25,6 +34,7 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl const [uploadProgress, setUploadProgress] = useState(0); const [currentChunk, setCurrentChunk] = useState(0); const [totalChunks, setTotalChunks] = useState(0); + const [phase, setPhase] = useState({ kind: 'idle' }); const [selectedCategoryId, setSelectedCategoryId] = useState(null); const [replaceByName, setReplaceByName] = useState(false); const fileInputRef = useRef(null); @@ -144,11 +154,11 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl setCurrentChunk(chunkIndex + 1); const chunk = chunks[chunkIndex]; const formData = new FormData(); - + chunk.forEach((file) => { formData.append('photos', file); }); - + if (selectedCategoryId) { formData.append('category_id', selectedCategoryId.toString()); } @@ -156,14 +166,44 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl formData.append('replace_by_name', 'true'); } + setPhase({ + kind: 'transferring', + chunkIndex, + totalChunks: chunks.length, + bytePct: 0, + }); + try { const response = await api.post(`/admin/events/${eventId}/upload`, formData, { onUploadProgress: (progressEvent) => { if (progressEvent.total) { - // Calculate overall progress across all chunks const chunkProgress = progressEvent.loaded / progressEvent.total; const overallProgress = ((chunkIndex + chunkProgress) / chunks.length) * 100; setUploadProgress(Math.round(overallProgress)); + + // Once bytes have all left the browser, the request is + // sitting in the backend processing pipeline. Flip to + // 'processing' so the UI explains the wait instead of + // looking frozen at the chunk's max progress. + if (chunkProgress >= 1) { + setPhase((prev) => + prev.kind === 'transferring' && prev.chunkIndex === chunkIndex + ? { + kind: 'processing', + chunkIndex, + totalChunks: chunks.length, + filesInChunk: chunk.length, + } + : prev + ); + } else { + setPhase({ + kind: 'transferring', + chunkIndex, + totalChunks: chunks.length, + bytePct: Math.round(chunkProgress * 100), + }); + } } }, }); @@ -173,7 +213,7 @@ 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)); - + // Continue with next chunk even if one fails continue; } @@ -210,6 +250,7 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl setUploadProgress(0); setCurrentChunk(0); setTotalChunks(0); + setPhase({ kind: 'idle' }); } }; @@ -344,26 +385,48 @@ export const PhotoUpload: React.FC = ({ eventId, onUploadCompl - {/* Progress Bar */} + {/* 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 + assume the upload froze. */} {isUploading && (
-
- - {t('upload.uploading')} - {totalChunks > 1 && ` (${t('common.chunk')} ${currentChunk}/${totalChunks})`} - - {uploadProgress}% -
-
-
-
- {totalChunks > 1 && ( -

- {t('upload.uploadingChunks', { count: selectedFiles.length, total: totalChunks })} -

+ {phase.kind === 'processing' ? ( +
+
+ +
+

+ {t('upload.processing')} + {phase.totalChunks > 1 && ` (${t('common.chunk')} ${phase.chunkIndex + 1}/${phase.totalChunks})`} +

+

+ {t('upload.processingHint')} +

+
+
+
+ ) : ( + <> +
+ + {t('upload.transferring')} + {totalChunks > 1 && ` (${t('common.chunk')} ${currentChunk}/${totalChunks})`} + + {uploadProgress}% +
+
+
+
+ {totalChunks > 1 && ( +

+ {t('upload.uploadingChunks', { count: selectedFiles.length, total: totalChunks })} +

+ )} + )}
)} diff --git a/frontend/src/components/gallery/UserPhotoUpload.tsx b/frontend/src/components/gallery/UserPhotoUpload.tsx index fa45e2ff..6423f157 100644 --- a/frontend/src/components/gallery/UserPhotoUpload.tsx +++ b/frontend/src/components/gallery/UserPhotoUpload.tsx @@ -1,5 +1,5 @@ import React, { useState, useMemo } from 'react'; -import { Upload, X, CheckCircle } from 'lucide-react'; +import { Upload, X, CheckCircle, Loader2 } from 'lucide-react'; import { useTranslation } from 'react-i18next'; import { toast } from 'react-toastify'; import { Button } from '../common'; @@ -24,6 +24,10 @@ export const UserPhotoUpload: React.FC = ({ const [files, setFiles] = useState([]); const [uploading, setUploading] = useState(false); const [uploadProgress, setUploadProgress] = useState<{ [key: string]: number }>({}); + // Per-file processing state — flips to true once axios reports + // bytes-on-wire for that file, so the UI can show "Processing…" + // instead of a static 100% bar while the backend works. + const [processingFiles, setProcessingFiles] = useState<{ [key: string]: boolean }>({}); const { data: publicSettings } = usePublicSettings(); @@ -87,9 +91,18 @@ export const UserPhotoUpload: React.FC = ({ ...prev, [file.name]: progress, })); + if (progress >= 100) { + setProcessingFiles(prev => ({ ...prev, [file.name]: true })); + } } }, }); + // Request resolved → file fully processed by backend. + setProcessingFiles(prev => { + const next = { ...prev }; + delete next[file.name]; + return next; + }); successCount++; } catch (error: any) { // Upload error handled - user notified via UI @@ -185,7 +198,13 @@ export const UserPhotoUpload: React.FC = ({
{uploadProgress[file.name] !== undefined ? (
- {uploadProgress[file.name] === 100 ? ( + {processingFiles[file.name] ? ( + // Bytes are on the server; the request hasn't + // resolved yet because the backend is still + // generating thumbnails / reading EXIF. Show + // a spinner so it doesn't look stuck at 100%. + + ) : uploadProgress[file.name] === 100 ? ( ) : (
diff --git a/frontend/src/i18n/locales/de.json b/frontend/src/i18n/locales/de.json index 891a6c2d..2456c406 100644 --- a/frontend/src/i18n/locales/de.json +++ b/frontend/src/i18n/locales/de.json @@ -131,6 +131,12 @@ "unsupportedFiles": "Einige Dateien wurden übersprungen, da das Format nicht unterstützt wird (JPEG/PNG/WebP/MP4/MOV/WEBM verwenden).", "selectedFiles": "Ausgewählte Dateien", "uploading": "Wird hochgeladen...", + "transferring": "Übertragung", + "processing": "Fotos werden verarbeitet...", + "processingHint": "Dateien sind hochgeladen. PicPeak erstellt jetzt Thumbnails und liest Metadaten. Sie können diese Seite verlassen — die Verarbeitung läuft im Hintergrund weiter.", + "processingProgress": "{{complete}} von {{total}} fertig", + "processingFailed": "{{count}} Foto(s) konnten nicht verarbeitet werden", + "retryFailed": "Fehlgeschlagene erneut versuchen", "uploadComplete": "Upload abgeschlossen!", "uploadFailed": "Upload fehlgeschlagen", "someFilesFailed": "Einige Dateien konnten nicht hochgeladen werden", diff --git a/frontend/src/i18n/locales/en.json b/frontend/src/i18n/locales/en.json index d2d3ba87..0c571243 100644 --- a/frontend/src/i18n/locales/en.json +++ b/frontend/src/i18n/locales/en.json @@ -131,6 +131,12 @@ "unsupportedFiles": "Some files were skipped because the format is not supported (use JPEG/PNG/WebP/MP4/MOV/WEBM).", "selectedFiles": "Selected files", "uploading": "Uploading...", + "transferring": "Transferring", + "processing": "Processing photos...", + "processingHint": "Files are uploaded. PicPeak is now generating thumbnails and reading metadata. You can leave this page — work continues in the background.", + "processingProgress": "{{complete}} of {{total}} done", + "processingFailed": "{{count}} photo(s) failed to process", + "retryFailed": "Retry failed", "uploadComplete": "Upload complete!", "uploadFailed": "Upload failed", "someFilesFailed": "Some files failed to upload",