diff --git a/backend/src/routes/adminPhotos.js b/backend/src/routes/adminPhotos.js index 852f170b..0c3e400f 100644 --- a/backend/src/routes/adminPhotos.js +++ b/backend/src/routes/adminPhotos.js @@ -1341,6 +1341,58 @@ router.get('/:eventId/thumbnail/:photoId', adminAuth, requirePermission('photos. } }); +/** + * Aspect-preserved rendition for admin surfaces that need one. + * + * The face avatars need this specifically. faceCropStyle positions a crop by + * scaling the WHOLE frame and offsetting so the face lands centre, which only + * works while the rendition is the entire image at a uniform scale. Thumbnails + * are not: thumbnail_fit is seeded to 'cover' (migration 040), so they are + * centre-cropped and every face avatar rendered against one is silently + * offset. Previews use fit: 'inside', so they are safe. + * + * ?w= is whitelisted the same way the gallery route's is — an open parameter + * would let anyone fill the disk with renditions. + */ +router.get('/:eventId/preview/:photoId', adminAuth, requirePermission('photos.view'), requireEventOwnership, async (req, res) => { + try { + const { eventId, photoId } = req.params; + + const photo = await db('photos').where({ id: photoId, event_id: eventId }).first(); + if (!photo) return res.status(404).json({ error: 'Photo not found' }); + + if (photo.processing_status === 'pending' || photo.processing_status === 'processing') { + res.setHeader('Retry-After', '2'); + return res.status(503).json({ error: 'Preview not ready', status: photo.processing_status }); + } + + const { PREVIEW_WIDTHS, normalizeTierWidth, ensurePreviewImageAtWidth, ensurePreviewImage } = + require('../services/imageProcessor'); + const tierWidth = normalizeTierWidth(req.query.w, PREVIEW_WIDTHS); + + const previewPath = tierWidth + ? (await ensurePreviewImageAtWidth(photo, tierWidth)) || (await ensurePreviewImage(photo)) + : await ensurePreviewImage(photo); + + if (!previewPath) { + return res.status(404).json({ error: 'Preview generation failed' }); + } + + const storage = getStorage(); + const stat = await storage.stat(previewPath); + if (!stat) return res.status(404).json({ error: 'Preview not found' }); + + res.setHeader('Content-Type', 'image/jpeg'); + res.setHeader('Cache-Control', 'private, max-age=3600'); + res.setHeader('Cross-Origin-Resource-Policy', 'cross-origin'); + res.setHeader('Content-Length', stat.size); + (await storage.get(previewPath)).pipe(res); + } catch (error) { + logger.error('Error serving admin preview:', error); + errorResponse(res, error, 500, 'Failed to serve preview'); + } +}); + // Debug endpoint to check photo existence router.get('/:eventId/debug', adminAuth, requirePermission('photos.view'), requireEventOwnership, async (req, res) => { try { diff --git a/frontend/src/components/admin/PeopleManagerModal.tsx b/frontend/src/components/admin/PeopleManagerModal.tsx index dcfc9636..064e1b72 100644 --- a/frontend/src/components/admin/PeopleManagerModal.tsx +++ b/frontend/src/components/admin/PeopleManagerModal.tsx @@ -22,6 +22,7 @@ import { X, Check, Merge, Scissors, EyeOff, Ban, Loader2 } from 'lucide-react'; import { Button, Loading } from '../common'; import { api } from '../../config/api'; import { faceCropStyle } from '../gallery/faceCrop'; +import { adminFacePreviewUrl } from '../gallery/imageTiers'; interface AdminPerson { id: number; @@ -95,7 +96,7 @@ const FaceThumb: React.FC<{ style={{ width: size, height: size, opacity: dim ? 0.4 : 1 }} > = ({ > {photo && ( = ({ > {photo && person.cover ? ( { + const photo = { id: 42, preview_url: '/api/gallery/wed/preview/42' }; + + it('never points a face crop at a thumbnail', () => { + // The regression, stated directly: any URL containing /thumbnail/ is + // cropped by fit:'cover' and will mis-place the face. + expect(facePreviewUrl('wed', photo)).not.toContain('/thumbnail/'); + expect(adminFacePreviewUrl(7, 42)).not.toContain('/thumbnail/'); + }); + + it('requests the small preview tier rather than the full 1920', () => { + // A 64px avatar does not need 1920px, and the strip renders one per + // person — pulling the full tier for each would be its own problem. + expect(facePreviewUrl('wed', photo)).toBe(`/api/gallery/wed/preview/42?w=${FACE_CROP_WIDTH}`); + expect(adminFacePreviewUrl(7, 42)).toBe(`/api/admin/photos/7/preview/42?w=${FACE_CROP_WIDTH}`); + }); + + it('prefers the served preview_url so a watermark query survives', () => { + const wm = { id: 9, preview_url: '/api/gallery/wed/preview/9?wm=1' }; + expect(facePreviewUrl('wed', wm)).toBe(`/api/gallery/wed/preview/9?wm=1&w=${FACE_CROP_WIDTH}`); + }); + + it('still resolves when preview_url is absent', () => { + // preview_url is only emitted when lightbox previews are enabled, but face + // scanning calls ensurePreviewImage for everything it scans — so a preview + // exists on disk for any photo that has a face, and the route can serve it. + expect(facePreviewUrl('wed', { id: 5 })) + .toBe(`/api/gallery/wed/preview/5?w=${FACE_CROP_WIDTH}`); + }); + + it('returns null rather than a wrong URL when it cannot build one', () => { + // Callers fall back to thumbnail_url — still mis-positioned, but visible, + // which beats a broken image. + expect(facePreviewUrl(undefined, { id: 5 })).toBeNull(); + expect(facePreviewUrl('wed', null)).toBeNull(); + expect(facePreviewUrl('wed', undefined)).toBeNull(); + }); + + it('sizes the tier from how much of the frame the face fills', () => { + Object.defineProperty(window, 'devicePixelRatio', { value: 3, configurable: true }); + const frame = { id: 1, width: 6000, height: 4000 }; + + // A face across a hall: 200px in a 6000px frame is ~21px at the 640 tier, + // which faceCropStyle then blows up ~9x. Indistinguishable from the + // mis-positioning bug this whole change is about. + expect(facePreviewUrl('wed', frame, { bbox: [0, 0, 200, 200] })).toContain('w=1920'); + + // A close-up needs nothing like that. + expect(facePreviewUrl('wed', frame, { bbox: [0, 0, 3000, 3000] })).toContain('w=640'); + }); + + it('falls back to the fixed tier when the bbox is unknown', () => { + expect(facePreviewUrl('wed', { id: 1, width: 6000, height: 4000 }, null)) + .toContain(`w=${FACE_CROP_WIDTH}`); + }); + + it('carries admin_preview so avatars do not 401 in preview mode', () => { + // verifyGalleryAccess only accepts the admin cookie when admin_preview=1 is + // on the request (middleware/gallery.js:28), and the preview flow mints no + // gallery JWT — so without this every avatar breaks in exactly the mode an + // admin uses to check a gallery before sending it. + const orig = window.location.search; + Object.defineProperty(window, 'location', { + value: { search: '?admin_preview=1' }, configurable: true, + }); + expect(facePreviewUrl('wed', { id: 5 })).toContain('admin_preview=1'); + Object.defineProperty(window, 'location', { value: { search: orig }, configurable: true }); + }); + + // The helper being correct is not the contract — the call sites using it is. + // Every assertion above passes with all three surfaces still reading + // thumbnail_url, which is exactly the bug. So pin the call sites. + describe('the three face surfaces actually use it', () => { + const surfaces = [ + ['PeopleStrip', '../PeopleStrip.tsx', 'facePreviewUrl'], + ['PeopleSheet', '../PeopleSheet.tsx', 'facePreviewUrl'], + ['PeopleManagerModal', '../../admin/PeopleManagerModal.tsx', 'adminFacePreviewUrl'], + ] as const; + + it.each(surfaces)('%s builds its avatar src from %s', (_name, rel, helper) => { + const src = fs.readFileSync(path.join(__dirname, rel), 'utf8'); + + // Matched inside the src={...} expression, not merely present in the + // file: an import alone satisfies toContain(helper) while the avatar + // still reads thumbnail_url, which is the bug wearing the fix's clothes. + expect(src).toMatch(new RegExp(`src=\\{[^}]*${helper}\\(`)); + + // And no face surface may reintroduce a hardcoded thumbnail path. + expect(src).not.toContain('/thumbnail/${photoId}'); + }); + }); +}); diff --git a/frontend/src/components/gallery/imageTiers.ts b/frontend/src/components/gallery/imageTiers.ts index 22bc3430..819ba8bb 100644 --- a/frontend/src/components/gallery/imageTiers.ts +++ b/frontend/src/components/gallery/imageTiers.ts @@ -15,6 +15,12 @@ export const PREVIEW_WIDTHS = [640, 1280, 1920] as const; +/** Fallback tier for face avatars when the face's size in frame is unknown. */ +export const FACE_CROP_WIDTH = 640; + +/** CSS px of the avatar the crop has to fill; used to size the tier. */ +const FACE_AVATAR_PX = 64; + // Thumbnail tiers are NOT here yet. Emitting a srcset whose candidates the // server ignores is worse than emitting none: the browser would pick the // "600w" candidate, receive the 300px image, and upscale it — the exact @@ -41,7 +47,8 @@ function smallestCovering(needed: number, tiers: readonly number[]): number { * and lands back on the desktop rendition, which is the thing being fixed. * * Without photo dimensions there is nothing to reason about, so it falls back - * to the largest edge the viewport could demand — i.e. today's behaviour. + * to the largest edge the viewport could possibly demand — which resolves to + * the top tier, i.e. exactly today's behaviour. */ export function viewportPreviewWidth(photo?: { width?: number | null; height?: number | null }): number { if (typeof window === 'undefined') return PREVIEW_WIDTHS[PREVIEW_WIDTHS.length - 1]; @@ -58,7 +65,8 @@ export function viewportPreviewWidth(photo?: { width?: number | null; height?: n // Contained in the viewport, so one axis binds; the rendered long edge is // the source long edge times that scale. const scale = Math.min(vw / pw, vh / ph); - return smallestCovering(Math.round(Math.max(pw, ph) * scale * dpr), PREVIEW_WIDTHS); + const renderedLongEdge = Math.max(pw, ph) * scale * dpr; + return smallestCovering(Math.round(renderedLongEdge), PREVIEW_WIDTHS); } /** @@ -103,3 +111,87 @@ export function previewUrlForViewport( if (width === PREVIEW_WIDTHS[PREVIEW_WIDTHS.length - 1]) return previewUrl; return withWidth(previewUrl, width); } + +/** + * An aspect-preserved rendition for a face avatar (#1096). + * + * NOT the thumbnail. faceCropStyle positions the crop by scaling the whole + * frame and offsetting so the face lands centre — which holds only while the + * rendition IS the whole frame. thumbnail_fit is seeded to 'cover' (migration + * 040_add_thumbnail_settings), so thumbnails are centre-cropped on essentially + * every install and every avatar rendered against one is silently offset. It + * presents as a bad detector: a shoulder, the back of a head, a patch of + * background. + * + * Previews use fit: 'inside', so they are the whole frame. 640 is plenty for a + * 64px avatar even at DPR 3, and face scanning has already generated a preview + * for any photo that has a face — faceProcessor calls ensurePreviewImage to + * get something to scan — so this asks for a rendition that is already there. + */ +export function facePreviewUrl( + slug: string | undefined, + photo: { + id: number | string; + preview_url?: string | null; + width?: number | null; + height?: number | null; + } | null | undefined, + cover?: { bbox: number[] } | null, +): string | null { + if (!photo) return null; + const width = faceTierWidth(photo, cover); + + // preview_url carries the watermark query when the server emitted one, so + // prefer it; it is only absent when lightbox previews are off. + if (photo.preview_url) return withWidth(photo.preview_url, width); + if (!slug) return null; + + // Carry admin_preview through. verifyGalleryAccess only accepts the admin + // cookie when admin_preview=1 is on the request (middleware/gallery.js:28), + // and the preview flow deliberately mints no gallery JWT — so a synthesized + // URL without it 401s, and every avatar breaks in exactly the mode an admin + // uses to check their gallery before sending it. + const adminPreview = typeof window !== 'undefined' + && new URLSearchParams(window.location.search).get('admin_preview') === '1' + ? '&admin_preview=1' + : ''; + return `/api/gallery/${slug}/preview/${photo.id}?w=${width}${adminPreview}`; +} + +/** + * Tier for a face crop, sized by how much of the frame the face occupies. + * + * A fixed small tier is wrong for the case that matters most: in a 6000px + * group shot a 200px face is only ~21px at the 640 tier, and faceCropStyle + * then blows that up ~9x to fill a 64px avatar at DPR 3 — visibly mush, and + * indistinguishable from the mis-positioning bug this was meant to fix. + * + * Working back from the avatar: the frame must be large enough that the + * bbox's share of it still covers the avatar's device pixels. A close-up + * lands on 640, a face across a hall lands on 1920. + */ +function faceTierWidth( + photo: { width?: number | null; height?: number | null }, + cover?: { bbox: number[] } | null, +): number { + const avatarDevicePx = FACE_AVATAR_PX + * (typeof window !== 'undefined' ? Math.min(window.devicePixelRatio || 1, 3) : 2); + + const frameLongEdge = Math.max(photo.width || 0, photo.height || 0); + const bboxLongEdge = cover?.bbox ? Math.max(cover.bbox[2] || 0, cover.bbox[3] || 0) : 0; + if (!frameLongEdge || !bboxLongEdge) return FACE_CROP_WIDTH; + + const faceShareOfFrame = bboxLongEdge / frameLongEdge; + return smallestCovering(Math.round(avatarDevicePx / faceShareOfFrame), PREVIEW_WIDTHS); +} + +/** Admin equivalent — the admin API has its own preview route. */ +export function adminFacePreviewUrl( + eventId: number | string, + photoId: number | string, + photo?: { width?: number | null; height?: number | null }, + cover?: { bbox: number[] } | null, +): string { + const width = photo ? faceTierWidth(photo, cover) : FACE_CROP_WIDTH; + return `/api/admin/photos/${eventId}/preview/${photoId}?w=${width}`; +}