diff --git a/backend/__tests__/services/ensureHeroImage.external.test.js b/backend/__tests__/services/ensureHeroImage.external.test.js new file mode 100644 index 00000000..92d2fc96 --- /dev/null +++ b/backend/__tests__/services/ensureHeroImage.external.test.js @@ -0,0 +1,116 @@ +/** + * ensureHeroImage must work for external/reference photos (#1166 follow-up). + * + * resolvePhotoStorageKey returns null for external photos by design, and that + * null used to be handed straight to withLocalCopy, which throws — so the hero + * route caught it and redirected to the full ORIGINAL. #1078 fixed exactly + * this shape for ensurePreviewImage and nobody carried it across. + * + * It only became visible when the Story hero started asking for hero_url + * instead of photo.url: on a managed gallery that is a real saving, on a + * reference-mode gallery it quietly changed nothing. + */ +const path = require('path'); +const fs = require('fs').promises; +const os = require('os'); +const sharp = require('sharp'); + +const EXTERNAL_ROOT = path.join(os.tmpdir(), `picpeak-hero-ext-${process.pid}`); +process.env.EXTERNAL_MEDIA_ROOT = EXTERNAL_ROOT; + +jest.mock('../../src/database/db', () => { + const state = { event: null, updates: [] }; + const api = (table) => { + if (table === 'events') return { where: () => ({ first: async () => state.event }) }; + if (table === 'photos') { + return { where: (criteria) => ({ update: async (values) => { state.updates.push({ criteria, values }); return 1; } }) }; + } + throw new Error(`unexpected table in test: ${table}`); + }; + api.__state = state; + return { db: api }; +}); + +const LocalFsStorage = require('../../src/services/storage/LocalFsStorage'); +const storageModule = require('../../src/services/storage'); +const { db } = require('../../src/database/db'); + +const EVENT = { id: 7, slug: 'nas-wedding', source_mode: 'reference', external_path: 'weddings/2026-08' }; + +async function writeSourceJpeg(absPath, { width = 2400, height = 1600 } = {}) { + await fs.mkdir(path.dirname(absPath), { recursive: true }); + const buf = Buffer.alloc(width * height * 3); + for (let i = 0; i < buf.length; i++) buf[i] = (i * 7) % 256; + await sharp(buf, { raw: { width, height, channels: 3 } }).jpeg({ quality: 90 }).toFile(absPath); +} + +describe('ensureHeroImage — external sources', () => { + let storage; let storageRoot; let imageProcessor; + + beforeAll(async () => { + storageRoot = await fs.mkdtemp(path.join(os.tmpdir(), 'picpeak-hero-store-')); + storage = new LocalFsStorage({ root: storageRoot }); + await storage.init(); + storageModule.setStorageForTesting(storage); + delete require.cache[require.resolve('../../src/services/imageProcessor')]; + imageProcessor = require('../../src/services/imageProcessor'); + await fs.mkdir(path.join(EXTERNAL_ROOT, EVENT.external_path), { recursive: true }); + }, 30000); + + afterAll(async () => { + storageModule.resetStorage(); + await fs.rm(storageRoot, { recursive: true, force: true }).catch(() => {}); + await fs.rm(EXTERNAL_ROOT, { recursive: true, force: true }).catch(() => {}); + }); + + beforeEach(() => { db.__state.event = EVENT; db.__state.updates = []; }); + + it.each(['external', 'reference'])('generates a hero for a %s photo off the mount', async (sourceOrigin) => { + const name = `${sourceOrigin}-hero.jpg`; + await writeSourceJpeg(path.join(EXTERNAL_ROOT, EVENT.external_path, name)); + const photo = { + id: sourceOrigin === 'external' ? 301 : 302, + event_id: EVENT.id, + source_origin: sourceOrigin, + // Root-relative, as stored since #1163: external_relpath is resolved + // from EXTERNAL_MEDIA_ROOT, not from event.external_path. The base- + // relative form this fixture used to carry stopped resolving the moment + // that landed, and ensureHeroImage returned null. + external_relpath: path.join(EVENT.external_path, name), + filename: name, + hero_path: null, + }; + + const key = await imageProcessor.ensureHeroImage(photo); + + // The regression: this returned null and the route redirected to the + // full original. + expect(key).toBeTruthy(); + expect(await storage.exists(key)).toBe(true); + // Per-photo basename, so two events sharing a NAS filename cannot clobber + // each other — same rule as the preview tier. + expect(key).toContain(`ext${photo.id}_`); + expect(db.__state.updates).toEqual([{ criteria: { id: photo.id }, values: { hero_path: key } }]); + }); + + it('returns null rather than throwing when the external source is gone', async () => { + const photo = { + id: 303, event_id: EVENT.id, source_origin: 'external', + external_relpath: path.join(EVENT.external_path, 'not-on-the-mount.jpg'), filename: 'not-on-the-mount.jpg', hero_path: null, + }; + + await expect(imageProcessor.ensureHeroImage(photo)).resolves.toBeNull(); + expect(db.__state.updates).toEqual([]); + }); + + it('returns null for a reference-mode row with no source_origin', async () => { + // Mode falls back to the event's, so resolvePhotoStorageKey yields null. + // That used to reach withLocalCopy and throw out of the function. + const photo = { + id: 304, event_id: EVENT.id, source_origin: null, external_relpath: null, + filename: 'orphan.jpg', path: 'nas-wedding/individual/orphan.jpg', hero_path: null, + }; + + await expect(imageProcessor.ensureHeroImage(photo)).resolves.toBeNull(); + }); +}); diff --git a/backend/src/services/imageProcessor.js b/backend/src/services/imageProcessor.js index 214f9beb..0804cb7d 100644 --- a/backend/src/services/imageProcessor.js +++ b/backend/src/services/imageProcessor.js @@ -570,16 +570,13 @@ async function isHeroValid(heroPath) { * Ensure a hero image exists for a photo, regenerate if needed */ async function ensureHeroImage(photo) { - const { resolvePhotoStorageKey } = require('./photoResolver'); + const { resolvePhotoStorageKey, resolvePhotoFilePath } = require('./photoResolver'); - let sourceKey; + let event; try { - const event = await db('events').where('id', photo.event_id).first(); - sourceKey = resolvePhotoStorageKey(event, photo); - logger.info(`Ensuring hero image for photo ${photo.id} from key: ${sourceKey}`); + event = await db('events').where('id', photo.event_id).first(); } catch (e) { - const msg = (e && e.message) ? e.message : String(e); - logger.error(`Failed to resolve original key for hero image (photo ${photo.id}): ${msg}`); + logger.error(`Failed to load event for hero image (photo ${photo.id}): ${e.message}`); return null; } @@ -591,7 +588,56 @@ async function ensureHeroImage(photo) { logger.warn(`Invalid hero image detected for photo ${photo.id}, regenerating...`); } - const newHeroPath = await withLocalCopy(sourceKey, async (localPath) => { + // External sources never reach the managed backend, so resolvePhotoStorageKey + // returns null for them by design — and this function used to feed that null + // straight to withLocalCopy, which throws, so the hero route fell back to + // redirecting at the full original. #1078 fixed exactly this for + // ensurePreviewImage and nobody carried it across; it only became visible + // when the Story hero started asking for hero_url instead of the original + // (#1166), which on a reference-mode gallery quietly changed nothing. + const isExternal = photo.source_origin === 'external' || photo.source_origin === 'reference'; + + let newHeroPath; + if (isExternal) { + // Mirrors the external branch in ensurePreviewImage: a direct fs read off + // the mount, so no withLocalCopy, and a per-photo outputBasename so two + // events referencing the same NAS basename cannot clobber each other. + let localPath; + try { + localPath = resolvePhotoFilePath(event, photo); + } catch (e) { + logger.error(`Failed to resolve external file for hero image (photo ${photo.id}): ${e.message}`); + return null; + } + const sourceBasename = path.basename(photo.external_relpath || photo.filename || `photo-${photo.id}`); + newHeroPath = await generateHeroImage(localPath, { + regenerate: true, + outputBasename: `ext${photo.id}_${sourceBasename}`, + }); + if (newHeroPath) { + await db('photos').where({ id: photo.id }).update({ hero_path: newHeroPath }); + } + return newHeroPath; + } + + let sourceKey; + try { + sourceKey = resolvePhotoStorageKey(event, photo); + logger.info(`Ensuring hero image for photo ${photo.id} from key: ${sourceKey}`); + } catch (e) { + const msg = (e && e.message) ? e.message : String(e); + logger.error(`Failed to resolve original key for hero image (photo ${photo.id}): ${msg}`); + return null; + } + if (!sourceKey) { + // Reference-mode event holding a row with no source_origin: the mode falls + // back to the event's and resolvePhotoStorageKey returns null. Honour the + // null-on-failure contract rather than feeding it to withLocalCopy. + logger.warn(`No managed storage key for hero image (photo ${photo.id}); skipping generation`); + return null; + } + + newHeroPath = await withLocalCopy(sourceKey, async (localPath) => { const proc = await withProcessableImage(localPath, sourceKey); try { return await generateHeroImage(proc.path, { regenerate: true, outputBasename: proc.outputBasename }); diff --git a/frontend/src/components/gallery/PhotoLightbox.tsx b/frontend/src/components/gallery/PhotoLightbox.tsx index 2c0cc569..b07bb490 100644 --- a/frontend/src/components/gallery/PhotoLightbox.tsx +++ b/frontend/src/components/gallery/PhotoLightbox.tsx @@ -6,7 +6,7 @@ import type { Photo, GalleryPerson } from '../../types'; import { useSavePhotoToDevice } from '../../hooks/useGallery'; import { AuthenticatedImage } from '../common'; import { PhotoFeedback } from './PhotoFeedback'; -import { previewUrlForViewport } from './imageTiers'; +import { lightboxImageUrl } from './imageTiers'; import { feedbackService, type ColorLabel, type KeybindMode } from '../../services/feedback.service'; import { PhotoColorLabels } from './PhotoColorLabels'; import { resolveFeedbackKey, colorShortcutHints } from '../../utils/feedbackKeybinds'; @@ -1143,7 +1143,7 @@ export const PhotoLightbox: React.FC = ({ // original) when preview_url is null — happens // when the toggle is off, when the photo is a // video, or briefly while lazy generation runs. - src={previewUrlForViewport(photo.preview_url, photo) || photo.url} + src={lightboxImageUrl(photo)} alt={photo.filename} fallbackSrc={photo.thumbnail_url || undefined} className="max-w-full max-h-full object-contain select-none pointer-events-none" @@ -1168,7 +1168,7 @@ export const PhotoLightbox: React.FC = ({ setViewport(realWidth, realDpr)); + +const BIG = { width: 4160, height: 6240 }; + +describe('lightboxImageUrl (#1166)', () => { + it('uses the preview tier when the admin opted in', () => { + setViewport(1440, 2, 900); + expect(lightboxImageUrl({ + url: '/api/gallery/g/photo/47', + preview_url: '/api/gallery/g/preview/47', + slideshow_url: '/api/gallery/g/preview/47', + ...BIG, + })).toBe('/api/gallery/g/preview/47'); + }); + + it('uses the preview tier when they did NOT — the reported install', () => { + // The regression, exactly as filed: preview_url null, slideshow_url set. + setViewport(1440, 2, 900); + expect(lightboxImageUrl({ + url: '/api/gallery/g/photo/47', + preview_url: null, + slideshow_url: '/api/gallery/g/preview/47', + ...BIG, + })).toBe('/api/gallery/g/preview/47'); + }); + + it('never serves the original while a derivative exists', () => { + setViewport(1440, 2, 900); + for (const preview_url of [null, undefined, '', '/api/gallery/g/preview/47']) { + expect(lightboxImageUrl({ + url: '/api/gallery/g/photo/47', + preview_url, + slideshow_url: '/api/gallery/g/preview/47', + ...BIG, + })).not.toContain('/photo/'); + } + }); + + it('falls back to the original when neither derivative exists', () => { + // Videos: the server emits null for both, and the player needs the real + // source. + expect(lightboxImageUrl({ + url: '/api/gallery/g/photo/47', + preview_url: null, + slideshow_url: null, + })).toBe('/api/gallery/g/photo/47'); + }); + + it('carries the watermark query through', () => { + // preview_url and slideshow_url are built from the same string server-side + // (gallery.js), so the wm parameter is on whichever one is used — losing it + // would serve an unwatermarked frame to a gallery that asked for one. + setViewport(1440, 2, 900); + expect(lightboxImageUrl({ + url: '/api/gallery/g/photo/47?wm=3', + preview_url: null, + slideshow_url: '/api/gallery/g/preview/47?wm=3', + ...BIG, + })).toBe('/api/gallery/g/preview/47?wm=3'); + }); + + it.each(['image/gif', 'image/apng', 'image/png'])('keeps the original for %s, which the preview tier would flatten', (mime) => { + // generatePreviewImage always encodes JPEG, so routing an animated source + // through it replaces the animation with its first frame — behaviour the + // toggle-off default never had. + setViewport(1440, 2, 900); + expect(lightboxImageUrl({ + url: '/api/gallery/g/photo/47', + preview_url: null, + slideshow_url: '/api/gallery/g/preview/47', + mime_type: mime, + ...BIG, + })).toBe('/api/gallery/g/photo/47'); + }); + + it('catches a PNG that migration 039 mislabelled as image/jpeg', () => { + // 039 backfilled every pre-existing photo's mime_type to image/jpeg, and + // the external-media importer inserts rows with none at all — so trusting + // MIME alone lets exactly the transparent photos through. + setViewport(1440, 2, 900); + expect(lightboxImageUrl({ + url: '/api/gallery/g/photo/47', + preview_url: null, + slideshow_url: '/api/gallery/g/preview/47', + mime_type: 'image/jpeg', + filename: 'logo-with-alpha.png', + ...BIG, + })).toBe('/api/gallery/g/photo/47'); + }); + + it('catches one with no mime_type at all, as external imports write them', () => { + setViewport(1440, 2, 900); + expect(lightboxImageUrl({ + url: '/api/gallery/g/photo/47', + preview_url: null, + slideshow_url: '/api/gallery/g/preview/47', + filename: 'animation.gif', + ...BIG, + })).toBe('/api/gallery/g/photo/47'); + }); + + it('still uses the preview tier for ordinary still formats', () => { + setViewport(1440, 2, 900); + for (const mime_type of ['image/jpeg', 'image/webp', undefined]) { + expect(lightboxImageUrl({ + url: '/api/gallery/g/photo/47', + preview_url: null, + slideshow_url: '/api/gallery/g/preview/47', + mime_type, + ...BIG, + })).toBe('/api/gallery/g/preview/47'); + } + }); + + it('still sizes the fallback tier for the device', () => { + // The #1095 behaviour has to survive the new fallback: a phone must not + // pull the 1920 rendition just because the URL came from slideshow_url. + setViewport(390, 3); + // A tall portrait is bound by height, so a DPR-3 phone genuinely needs the + // top tier — which is served without ?w=, keeping those URLs byte-identical + // to the ones already in browser and CDN caches. + expect(lightboxImageUrl({ + url: '/api/gallery/g/photo/47', + preview_url: null, + slideshow_url: '/api/gallery/g/preview/47', + ...BIG, + })).toBe('/api/gallery/g/preview/47'); + + // Landscape on the same phone needs less, and says so. + expect(lightboxImageUrl({ + url: '/api/gallery/g/photo/47', + preview_url: null, + slideshow_url: '/api/gallery/g/preview/47', + width: 3000, + height: 2000, + })).toBe('/api/gallery/g/preview/47?w=1280'); + }); +}); diff --git a/frontend/src/components/gallery/imageTiers.ts b/frontend/src/components/gallery/imageTiers.ts index 4833fa6c..8f07e60b 100644 --- a/frontend/src/components/gallery/imageTiers.ts +++ b/frontend/src/components/gallery/imageTiers.ts @@ -107,6 +107,66 @@ export function previewUrlForViewport( return withWidth(previewUrl, width); } +/** + * What the lightbox actually puts in an (#1166). + * + * `preview_url` is only emitted when the admin has flipped + * lightbox_preview_enabled, which is off by default — so a stock install fell + * straight through to `url`, the untouched original. A reporter measured + * 16.5 MB per photo where the preview is 345 KB, and the lightbox renders its + * neighbours too, so opening one photo pulled three originals. + * + * `slideshow_url` is the same /preview/:id URL, watermark query and all, but + * emitted unconditionally for images since #1015 — the slideshow has never had + * a fallback worth taking. Preferring it here fixes every existing install + * without an admin touching a setting. + * + * `url` stays as the last resort, which is where videos land (both derivative + * URLs are null for them) and where an image goes if the server ever stops + * emitting either. The preview route generates lazily and redirects to the + * original on any failure, so nothing here can show less than it does today. + */ +export function lightboxImageUrl(photo: { + url: string; + preview_url?: string | null; + slideshow_url?: string | null; + mime_type?: string; + filename?: string; + original_filename?: string | null; + width?: number | null; + height?: number | null; +}): string { + // Animated formats keep the original. generatePreviewImage always encodes + // JPEG (imageProcessor.js:668), so routing an animated source through the + // preview tier would replace the animation with its first frame — a + // regression the toggle-off default never had. + // + // PNG goes with them, for two reasons that share a cause: generatePreviewImage + // also drops ALPHA (a transparent source flattens against a solid + // background), and an APNG is normally reported as image/png rather than + // image/apng, so the specific check alone would miss the common upload path. + // PNG is where transparency is the norm, and it is rare enough in an event + // gallery that the bandwidth given up is small. + // + // MIME is all the frontend has. Animated or alpha WebP reports image/webp + // exactly like an ordinary still, and cannot be told apart here — fixing + // that needs the backend to report it (Sharp's `metadata.pages`, and + // `hasAlpha`) rather than costing every static-WebP gallery the bandwidth + // fix on a guess. That is the known remaining gap. + // Checked against the FILENAME as well as the MIME, because mime_type is not + // trustworthy here: migration 039 backfilled every pre-existing photo as + // image/jpeg regardless of what it was, and the external-media importer + // inserts rows without a mime_type at all. A mislabelled PNG would otherwise + // sail past this and come back flattened. + const ORIGINAL_ONLY = ['image/gif', 'image/apng', 'image/png']; + const ORIGINAL_ONLY_EXT = /\.(gif|apng|png)$/i; + const name = photo.original_filename || photo.filename || ''; + if ((photo.mime_type && ORIGINAL_ONLY.includes(photo.mime_type)) || ORIGINAL_ONLY_EXT.test(name)) { + return photo.url; + } + return previewUrlForViewport(photo.preview_url || photo.slideshow_url, photo) || photo.url; +} + /** * An aspect-preserved rendition for a face avatar (#1096). * diff --git a/frontend/src/components/gallery/layouts/GalleryPremiumLayout.tsx b/frontend/src/components/gallery/layouts/GalleryPremiumLayout.tsx index 5699dbf8..36c1f8ea 100644 --- a/frontend/src/components/gallery/layouts/GalleryPremiumLayout.tsx +++ b/frontend/src/components/gallery/layouts/GalleryPremiumLayout.tsx @@ -31,6 +31,7 @@ import { useDownloadPhoto } from '../../../hooks/useGallery'; import { toast } from 'react-toastify'; import './GalleryPremiumLayout.css'; +import { lightboxImageUrl } from '../imageTiers'; interface PhotoCardProps { photo: Photo; @@ -318,7 +319,17 @@ export const GalleryPremiumLayout: React.FC = ({ // when the admin has flipped the original-filenames toggle (#508). const slides = useMemo(() => { return filteredPhotos.map(photo => ({ - src: photo.url, + // Display source, not the original (#1166). This layout returns early + // from PhotoGridWithLayouts and never renders PhotoLightbox, so it needs + // its own call — without it a premium gallery keeps pulling + // multi-megabyte originals to show a photo on screen. `download` below + // deliberately stays on photo.url: what a guest saves must be the full + // original. + src: lightboxImageUrl(photo), + // The download handler used to recover the photo by matching slide.src + // against photo.url. src is a derivative now, so that lookup would find + // nothing and Download would silently do nothing (#1166 review). + photoId: photo.id, alt: photo.filename, width: photo.width || 1200, height: photo.height || 800, @@ -438,10 +449,15 @@ export const GalleryPremiumLayout: React.FC = ({ } }, [selectedPhotos, slug, t, downloadChoices, onPickResolution]); - const handleDownloadFromLightbox = useCallback((slide: { src?: string }) => { + const handleDownloadFromLightbox = useCallback((slide: { src?: string; photoId?: number }) => { if (!allowDownloads || !slide.src) return; - const photo = filteredPhotos.find(p => p.url === slide.src); + // By id, carried on the slide. Matching on src broke the moment the slide + // stopped being the original — and what Download hands over must stay the + // original regardless of what is rendered. + const photo = slide.photoId != null + ? filteredPhotos.find(p => p.id === slide.photoId) + : filteredPhotos.find(p => p.url === slide.src); if (photo) { analyticsService.trackDownload(photo.id, slug, false); downloadPhotoMutation.mutate({ diff --git a/frontend/src/components/gallery/layouts/story/StoryHero.tsx b/frontend/src/components/gallery/layouts/story/StoryHero.tsx index 90b7def2..3c6e23b3 100644 --- a/frontend/src/components/gallery/layouts/story/StoryHero.tsx +++ b/frontend/src/components/gallery/layouts/story/StoryHero.tsx @@ -39,9 +39,14 @@ export const StoryHero: React.FC = ({
{/* Background */}
- {photo && (photo.url || photo.thumbnail_url) ? ( + {photo && (photo.hero_url || photo.url || photo.thumbnail_url) ? ( = ({ void _galleryId; const [isLoaded, setIsLoaded] = useState(false); + // Don't fetch until the card is near the viewport (#1166). + // + // Every card in a Story gallery mounts at page load — `whileInView` gates the + // ANIMATION, not the render — and AuthenticatedImage fetches from an effect + // on mount, so all of them requested at once. That was tolerable while they + // pointed at `photo.url`, because nothing was generated; pointing them at + // the preview tier means a gallery with cold previews would Sharp-decode + // every original in one burst. + // + // `once` so a card that has loaded never unloads on scroll-away, and the + // same 200px margin the entrance animation uses so the image is already in + // flight by the time the card animates in. + const cardRef = useRef(null); + const isNearViewport = useInView(cardRef, { once: true, margin: '200px' }); + return ( = ({ > = ({ }} className="block w-full h-full" > + {/* The placeholder keeps the card's box while the image is still + out of range, so nothing reflows when it arrives. */} + {!isNearViewport ? ( +